diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index df72f616ca..794c94569f 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -3,16 +3,13 @@ run-name: >- Required Noema Review ${{ github.event.client_payload.target_repository || github.event.pull_request.base.repo.full_name || github.repository }}#${{ github.event.client_payload.pr_number || github.event.pull_request.number || - github.event.workflow_run.pull_requests[0].number || 'event' }}@${{ + 'event' }}@${{ 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.sha }} on: pull_request_target: types: [opened, synchronize, reopened, ready_for_review, closed] - workflow_run: - workflows: ["Required OpenCode Review", "Strix Security Scan"] - types: [completed] # Default-branch-only retry entrypoint; no caller-selected workflow ref. repository_dispatch: types: [noema-review] @@ -22,18 +19,12 @@ concurrency: noema-review-${{ github.event.pull_request.base.repo.full_name || github.event.client_payload.target_repository || github.repository }}-${{ - github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || + github.event.pull_request.number || github.event.client_payload.pr_number || - 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' }} + github.run_id }} + cancel-in-progress: >- + ${{ github.event_name == 'pull_request_target' && + (github.event.action == 'synchronize' || github.event.action == 'closed') }} permissions: contents: read @@ -191,10 +182,6 @@ jobs: runs-on: ubuntu-latest if: >- github.event_name == 'repository_dispatch' - || ( - github.event_name == 'workflow_run' - && github.event.workflow_run.conclusion != 'cancelled' - ) || ( github.event_name == 'pull_request_target' && github.event.action != 'closed' @@ -209,8 +196,8 @@ jobs: 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 || '' }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.client_payload.pr_number || '' }} + EXPECTED_HEAD_SHA: ${{ github.event.pull_request.head.sha || github.event.client_payload.pr_head_sha || '' }} steps: - name: Skip events without pull request context if: env.PR_NUMBER == '' @@ -292,13 +279,13 @@ jobs: GH_TOKEN: ${{ github.token }} run: | set -euo pipefail - if [[ ! "$EXPECTED_HEAD" =~ ^[0-9a-f]{40}$ ]]; then + if [[ ! "$EXPECTED_HEAD_SHA" =~ ^[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}." + if [ "${live_head,,}" != "${EXPECTED_HEAD_SHA,,}" ]; then + echo "::error::Noema trigger is stale; expected ${EXPECTED_HEAD_SHA}, observed ${live_head}." exit 1 fi @@ -332,7 +319,7 @@ jobs: # 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" ' + --arg target "$TARGET_REPOSITORY" --arg head "$EXPECTED_HEAD_SHA" ' .workflow_runs[] | select(.id < $current) | select(.path == ".github/workflows/noema-review.yml") @@ -364,7 +351,7 @@ jobs: sed 's/^/ /' /tmp/noema-supersede-live-head-error >&2 || true exit 0 fi - if [ "${live_head,,}" != "${EXPECTED_HEAD,,}" ]; then + if [ "${live_head,,}" != "${EXPECTED_HEAD_SHA,,}" ]; then echo "::notice::Noema cleanup stopped because the PR head advanced." exit 0 fi @@ -495,6 +482,25 @@ jobs: echo "::add-mask::$app_token" echo "token=$app_token" >>"$GITHUB_OUTPUT" + - name: Validate current pull request head + if: env.PR_NUMBER != '' + env: + GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} + run: | + set -euo pipefail + if ! [[ "$EXPECTED_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::Noema expected head must be a full commit SHA." + exit 1 + fi + pull_request_json="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" + live_state="$(jq -r '.state // empty' <<<"$pull_request_json")" + live_head_sha="$(jq -r '.head.sha // empty' <<<"$pull_request_json")" + if [ "$live_state" != "open" ] || [ "${live_head_sha,,}" != "${EXPECTED_HEAD_SHA,,}" ]; then + printf '::error::Noema review target is closed or stale. expected head=%s; live state=%s head=%s.\n' \ + "$EXPECTED_HEAD_SHA" "${live_state:-missing}" "${live_head_sha:-missing}" + exit 1 + fi + - name: Resolve Noema target repository visibility if: env.PR_NUMBER != '' id: target_visibility @@ -575,4 +581,4 @@ jobs: python3 -m scripts.ci.noema_review_gate \ --repo "$TARGET_REPOSITORY" \ --pr-number "$PR_NUMBER" \ - --expected-head "$EXPECTED_HEAD" + --expected-head "$EXPECTED_HEAD_SHA" diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index cdc1245266..3677f408bd 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -2312,7 +2312,6 @@ 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: 305 permissions: actions: write checks: read @@ -3993,7 +3992,6 @@ jobs: - name: Run OpenCode PR Review model pool id: opencode_review_model_pool if: needs.coverage-evidence.result == 'success' - timeout-minutes: 205 continue-on-error: true env: SHARE: "false" @@ -4004,14 +4002,7 @@ jobs: # the SAME model 5x let a rate-limited/hung leader consume the whole # step, so the pool never reached a healthy fallback model. 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. 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" # A second pass through the same provider catalog repeats the same # quota/format failures and can occupy the required check for hours. # Exhaust each distinct candidate once, then publish the bounded @@ -4020,23 +4011,10 @@ jobs: OPENCODE_DYNAMIC_REVIEW_CADENCE: "true" OPENCODE_SMALL_CHANGE_FILE_THRESHOLD: "3" OPENCODE_MEDIUM_CHANGE_FILE_THRESHOLD: "20" - OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS: "11700" - OPENCODE_SMALL_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS: "11700" - OPENCODE_MEDIUM_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS: "11700" - OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS: "11700" - OPENCODE_UNKNOWN_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - 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: "11700" - OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_TOTAL_BUDGET_SECONDS: "11700" OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_MAX_CYCLES: "1" OPENCODE_BACKOFF_INITIAL_SECONDS: "30" OPENCODE_BACKOFF_MAX_SECONDS: "30" @@ -4059,18 +4037,9 @@ jobs: set -euo pipefail source "$GITHUB_WORKSPACE/scripts/ci/load_contextual_orchestrator_token.sh" set +e - timeout --kill-after=30s "${OPENCODE_POOL_STEP_TIMEOUT_SECONDS:-3600}s" \ - bash "$GITHUB_WORKSPACE/scripts/ci/run_opencode_review_model_pool.sh" + bash "$GITHUB_WORKSPACE/scripts/ci/run_opencode_review_model_pool.sh" pool_status=$? set -e - if [ "$pool_status" -eq 124 ] || [ "$pool_status" -eq 137 ] || [ "$pool_status" -eq 143 ]; then - printf 'OpenCode model pool exceeded the outer %ss step budget; marking the pool exhausted so current-head evidence fallback can publish a bounded reason instead of blocking the org queue.\n' \ - "${OPENCODE_POOL_STEP_TIMEOUT_SECONDS:-3600}" - { - printf 'review_model=\n' - printf 'review_status=exhausted\n' - } >>"$GITHUB_OUTPUT" - fi exit "$pool_status" - name: Exchange OpenCode app token for review writes @@ -4623,7 +4592,6 @@ jobs: # The approval gate normally waits about six minutes, with bounded # extensions for image validation or package/GPU builds plus API and # publication overhead. - timeout-minutes: 36 env: GH_TOKEN: ${{ steps.opencode_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }} CHECK_LOOKUP_GH_TOKEN: ${{ github.token }} @@ -4678,7 +4646,6 @@ jobs: # failed-check diagnosis in this publish step is a short best-effort # augmentation; current-head logs/SARIF remain the authoritative # reason source when the augmentation is unavailable. - OPENCODE_RUN_TIMEOUT_SECONDS: "120" OPENCODE_EXPORT_TIMEOUT_SECONDS: "60" run: | set -euo pipefail @@ -6032,8 +5999,7 @@ jobs: } >"$prompt_file" cd "$OPENCODE_REVIEW_WORKDIR" - if ! timeout --kill-after=30s "${OPENCODE_RUN_TIMEOUT_SECONDS:-120}s" \ - env -u GH_TOKEN -u GITHUB_TOKEN -u OPENCODE_APP_TOKEN \ + if ! env -u GH_TOKEN -u GITHUB_TOKEN -u OPENCODE_APP_TOKEN \ -u ACTIONS_ID_TOKEN_REQUEST_TOKEN -u ACTIONS_ID_TOKEN_REQUEST_URL \ opencode run "$(cat "$prompt_file")" \ --pure \ diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 81faf57757..38cd4c6913 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -251,14 +251,83 @@ jobs: name: opencode-review needs: [coverage-evidence] runs-on: ubuntu-latest - timeout-minutes: 5 permissions: contents: read pull-requests: read id-token: write steps: - - name: Resolve current-head formal OpenCode verdict - id: verdict + - name: Request current-head OpenCode review execution + if: github.event.action != 'closed' + env: + GH_TOKEN: ${{ github.token }} + 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 }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_DRAFT: ${{ github.event.pull_request.draft }} + BASE_BRANCH: ${{ github.event.pull_request.base.ref }} + WORKFLOW_SHA: ${{ github.workflow_sha }} + run: | + set -euo pipefail + helper="$(mktemp)" + trap 'rm -f "$helper"' EXIT + gh api "repos/ContextualWisdomLab/.github/contents/scripts/ci/opencode_review_receipt_gate.py?ref=${WORKFLOW_SHA}" \ + --jq .content | base64 --decode >"$helper" + receipt_state="$(python3 - "$helper" "$TARGET_REPOSITORY" "$PR_NUMBER" "$HEAD_SHA" "$PR_DRAFT" <<'PY' + import importlib.machinery + import importlib.util + import sys + + helper_path, repository, number, head_sha, draft = sys.argv[1:] + loader = importlib.machinery.SourceFileLoader( + "trusted_opencode_receipt_gate", helper_path + ) + spec = importlib.util.spec_from_loader(loader.name, loader) + if spec is None or spec.loader is None: + raise RuntimeError("trusted OpenCode receipt helper could not be loaded") + gate = importlib.util.module_from_spec(spec) + spec.loader.exec_module(gate) + reviews = gate.fetch_reviews(repository, int(number)) + receipt, _reason = gate.evaluate_receipts( + reviews, head_sha, is_draft=draft.lower() == "true" + ) + print("present" if receipt is not None else "missing") + PY + )" + if [ "$receipt_state" = "present" ]; then + echo "Current-head substantive OpenCode verdict already exists; scheduler wake skipped." + exit 0 + fi + if [ "$receipt_state" != "missing" ]; then + echo "::error::Trusted OpenCode receipt helper returned an invalid state." + exit 1 + fi + 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 env: GH_TOKEN: ${{ github.token }} TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} @@ -269,17 +338,16 @@ jobs: set -euo pipefail 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 - 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" ' + verdict="" + while :; do + reviews="$(gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")" + verdict="$(printf '%s\n' "$reviews" | jq -r -s --arg sha "$HEAD_SHA" ' (add // []) | [ .[] @@ -307,62 +375,13 @@ jobs: empty end ')" - 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 + if [ -n "$verdict" ]; then + break + fi + sleep 30 + done + 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/.github/workflows/pr-review-autofix.yml b/.github/workflows/pr-review-autofix.yml index 005303b822..678e8f0014 100644 --- a/.github/workflows/pr-review-autofix.yml +++ b/.github/workflows/pr-review-autofix.yml @@ -450,7 +450,7 @@ jobs: trap restore_workspace_config EXIT cd "$TARGET_WORKSPACE" env -u GITHUB_TOKEN -u GH_TOKEN -u ACTIONS_ID_TOKEN_REQUEST_TOKEN -u ACTIONS_ID_TOKEN_REQUEST_URL \ - timeout 18000 opencode run "$(cat "$prompt_file")" \ + opencode run "$(cat "$prompt_file")" \ --pure \ --agent ci-autofix \ --model "$MODEL" \ @@ -653,7 +653,7 @@ jobs: } trap restore_workspace_config EXIT env -u GITHUB_TOKEN -u GH_TOKEN -u ACTIONS_ID_TOKEN_REQUEST_TOKEN -u ACTIONS_ID_TOKEN_REQUEST_URL \ - timeout 18000 opencode run "$(cat "$prompt_file")" \ + opencode run "$(cat "$prompt_file")" \ --pure \ --agent ci-autofix \ --model "$MODEL" \ diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index 505053287b..672c9b796e 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -69,35 +69,6 @@ on: repository_dispatch: types: [strix-scan] -concurrency: - # Include the event name so default-branch repository_dispatch evidence cannot cancel - # or interleave with the required pull_request_target Strix context that branch - # protection reads. Closed PR events use a separate group so their cancellation - # job can run immediately instead of waiting behind the scan it must cancel. - # - # Rate-limit root-cause fix (2026-08-24): the group is scoped per REPOSITORY - # (not per PR) so sibling pull requests in the same repository scan - # sequentially instead of concurrently. Concurrent per-PR scans each retry - # the shared NVIDIA NIM key up to three times, producing guaranteed - # litellm.RateLimitError storms and fail-closed gate failures across every - # open PR (observed 2026-08-23/24). Serializing per repository and event - # class keeps at most one provider-backed PR scan in flight per class. Push - # and scheduled scans retain the branch ref so one protected branch cannot - # supersede another branch's pending evidence. GitHub's native concurrency - # contract retains one active and one pending run; the scheduler re-dispatches - # the exact current head after pending-run supersession, and accuracy is - # prioritized over scan latency. - group: >- - strix-${{ - github.event_name == 'pull_request_target' && - github.event.action == 'closed' && - format('closed-pr-{0}-{1}', github.event.pull_request.base.repo.full_name, github.event.pull_request.number) || - (github.event_name == 'pull_request_target' || github.event_name == 'repository_dispatch') && - format('{0}-{1}', github.event_name, github.event.client_payload.target_repository || github.event.pull_request.base.repo.full_name || github.repository) || - format('{0}-{1}-{2}', github.event_name, github.repository, github.ref) - }} - cancel-in-progress: false - # Scorecard Token-Permissions (alert #43): keep the workflow-level token # read-only and scope same-repo status publication to the Strix scan job. permissions: @@ -106,8 +77,8 @@ permissions: models: read jobs: - cancel-closed-pr-runs: - if: github.event_name == 'pull_request_target' && github.event.action == 'closed' + cancel-superseded-pr-runs: + if: github.event_name == 'pull_request_target' && (github.event.action == 'synchronize' || github.event.action == 'closed') runs-on: ubuntu-latest # Prefer the established scheduler credential, but let the close event use # its job-scoped token so abandoned scans are cancelled even when that @@ -115,47 +86,84 @@ jobs: permissions: actions: write contents: read + pull-requests: read env: GH_TOKEN: ${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }} TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} - CLOSED_PR_NUMBER: ${{ github.event.pull_request.number }} - CLOSED_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + TARGET_PR_NUMBER: ${{ github.event.pull_request.number }} + TARGET_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_ACTION: ${{ github.event.action }} CURRENT_RUN_ID: ${{ github.run_id }} steps: - - name: Cancel queued and running scans for the closed pull request + - name: Cancel queued and running scans for superseded or closed pull request heads shell: bash run: | set -euo pipefail + live_target_matches() { + local live_pr_json live_action + if ! live_pr_json="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}" 2>/tmp/strix-cleanup-gh-error)"; then + echo "::warning::Strix cleanup could not verify the live pull request; leaving runs unchanged." + sed 's/^/ /' /tmp/strix-cleanup-gh-error >&2 || true + return 1 + fi + live_action="$(jq -r '[.state, .head.sha // ""] | @tsv' <<<"$live_pr_json")" + { [ "$PR_ACTION" = "closed" ] && [ "$live_action" = $'closed\t'"$TARGET_PR_HEAD_SHA" ]; } || + { [ "$PR_ACTION" = "synchronize" ] && [ "$live_action" = $'open\t'"$TARGET_PR_HEAD_SHA" ]; } + } + cancel_runs() { local status="$1" + if ! live_target_matches; then + echo "::notice::Strix cleanup target changed before run selection; leaving runs unchanged." + return 0 + fi 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/strix-close-gh-error)"; then - echo "::warning::Strix close cleanup could not inspect ${TARGET_REPOSITORY}; leaving runs unchanged." - sed 's/^/ /' /tmp/strix-close-gh-error >&2 || true + if ! runs_json="$(gh api --paginate "$runs_url" 2>/tmp/strix-cleanup-gh-error)"; then + echo "::warning::Strix cleanup could not inspect ${TARGET_REPOSITORY}; leaving runs unchanged." + sed 's/^/ /' /tmp/strix-cleanup-gh-error >&2 || true return 0 fi local run_ids - if ! run_ids="$(jq -r --arg pr "$CLOSED_PR_NUMBER" --arg head_sha "$CLOSED_PR_HEAD_SHA" \ - --arg current "$CURRENT_RUN_ID" ' + if ! run_ids="$(jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ + --arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" ' .workflow_runs[] | select((.id | tostring) != $current) | select(.name == "Strix Security Scan") | select(.event == "pull_request_target") - | select(.head_sha == $head_sha or any(.pull_requests[]?; ((.number | tostring) == $pr))) + | ((.display_title // "") | startswith("Strix Security Scan " + $repo + "#" + $pr + "@")) as $title_matches + | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches + | select($title_matches or $metadata_matches) + | ((.display_title // "") | endswith("@" + $head_sha)) as $title_is_current + | ((.pull_requests // []) | any( + ((.number | tostring) == $pr) + and ((.head.sha // "") | ascii_downcase) == ($head_sha | ascii_downcase) + )) as $metadata_is_current + | ((.pull_requests // []) | any( + ((.number | tostring) == $pr) and ((.head.sha // "") != "") + )) as $metadata_has_head + | select( + $action == "closed" + or (($title_matches or $metadata_has_head) and (($title_is_current or $metadata_is_current) | not)) + ) | .id ' <<<"$runs_json")"; then - echo "::warning::Strix close cleanup received invalid run data for ${TARGET_REPOSITORY}; leaving runs unchanged." + echo "::warning::Strix cleanup received invalid run data for ${TARGET_REPOSITORY}; leaving runs unchanged." return 0 fi while IFS= read -r run_id; do [ -n "$run_id" ] || continue - if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/strix-close-cancel-error; then - echo "Cancelled Strix run ${run_id} in ${TARGET_REPOSITORY} for closed PR #${CLOSED_PR_NUMBER}." + if ! live_target_matches; then + echo "::notice::Strix cleanup target changed before cancellation; leaving runs unchanged." + return 0 + fi + if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/strix-cleanup-cancel-error || + gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/force-cancel" >/dev/null 2>>/tmp/strix-cleanup-cancel-error; then + echo "Cancelled obsolete Strix run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." else - echo "::warning::Strix close cleanup could not cancel run ${run_id} in ${TARGET_REPOSITORY}; it may have finished or the credential lacks Actions write access." - sed 's/^/ /' /tmp/strix-close-cancel-error >&2 || true + echo "::warning::Strix cleanup could not cancel run ${run_id} in ${TARGET_REPOSITORY}; it may have finished or the credential lacks Actions write access." + sed 's/^/ /' /tmp/strix-cleanup-cancel-error >&2 || true fi done <<<"$run_ids" } @@ -166,16 +174,22 @@ jobs: strix: if: github.event_name != 'pull_request_target' || github.event.action != 'closed' + concurrency: + # Keep provider-backed scans serial per repository and event class while + # allowing the trusted cleanup job above to retire an obsolete head now. + group: >- + strix-${{ + (github.event_name == 'pull_request_target' || github.event_name == 'repository_dispatch') && + format('{0}-{1}', github.event_name, github.event.client_payload.target_repository || github.event.pull_request.base.repo.full_name || github.repository) || + format('{0}-{1}-{2}', github.event_name, github.repository, github.ref) + }} + cancel-in-progress: false # Large, actively-growing repositories (e.g. contextual-orchestrator) can # legitimately require well over two hours to scan -- this org's own # standing operating directive accepts that central OpenCode/Strix/Noema # scans may take more than two hours per model (docs/product-goal-directive.md). - # The scanner gets a 150-minute process budget and a 155-minute total - # retry budget; the 170-minute step and 200-minute job leave deterministic - # time to preserve partial reports and publish a concrete failure reason. - # Hitting any cap is fail-closed and never turns an incomplete scan into - # an approval. - timeout-minutes: 200 + # Inference has no wall-clock deadline; cancellation is reserved for an + # explicit operator action or a superseded head. runs-on: ubuntu-latest # Least-privilege token scoped to this job (Scorecard alert #43): the scan # exchanges an OIDC token (id-token) and publishes same-repo status evidence @@ -729,7 +743,6 @@ jobs: - name: Run Strix (quick) if: steps.gate.outputs.enabled == 'true' - timeout-minutes: 170 # Security invariant for pull_request_target: execute only from the # trusted base checkout. The gate copies PR-head blobs into an isolated # temporary scope with execute bits stripped, then scans that scope as @@ -770,12 +783,10 @@ jobs: PR_HEAD_SHA: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || github.event.client_payload.pr_head_sha }} IS_PR_EVIDENCE_RUN: ${{ (github.event_name == 'pull_request_target' || github.event.client_payload.pr_number != '') && 'true' || 'false' }} run: | - budget_suffix="TIME""OUT" - process_budget_seconds="9000" - export "LLM_${budget_suffix}=900" - export "STRIX_MEMORY_COMPRESSOR_${budget_suffix}=300" - export "STRIX_PROCESS_${budget_suffix}_SECONDS=$process_budget_seconds" - export "STRIX_TOTAL_${budget_suffix}_SECONDS=9300" + export LLM_TIMEOUT=0 + export STRIX_MEMORY_COMPRESSOR_TIMEOUT=0 + export STRIX_PROCESS_TIMEOUT_SECONDS=0 + export STRIX_TOTAL_TIMEOUT_SECONDS=0 # Recognized signals that the LLM backend was unavailable / starved. # Defined before the gate loop so the bounded retry decision below @@ -798,22 +809,15 @@ jobs: # vulnerability result exists. # # A typed provider outage with no reported vulnerability finding is - # retried with bounded linear backoff inside this step so transient + # retried with linear backoff inside this step so transient # provider failures do not fail the required check on the first # attempt. Genuine findings, configuration failures, and unexpected - # exit codes never retry; the deadline keeps every path inside the - # deterministic 200-minute job budget, and all-terminal outcomes - # remain fail-closed. + # exit codes never retry, and all-terminal outcomes remain fail-closed. strix_run_log="$RUNNER_TEMP/strix_gate_console.log" : > "$strix_run_log" strix_terminal_log="$strix_run_log" strix_rc=0 strix_gate_attempt=1 - strix_gate_deadline=$(( SECONDS + 9600 )) - # Reserve the scanner process budget, not the gate's total wrapper - # budget. The latter includes setup/cleanup overhead already spent - # by the current attempt and can make every retry impossible. - strix_gate_attempt_budget_seconds="$process_budget_seconds" set +e while : ; do strix_attempt_log="$RUNNER_TEMP/strix_gate_console_attempt_${strix_gate_attempt}.log" @@ -850,10 +854,8 @@ jobs: break fi backoff_seconds=$(( ${STRIX_GATE_RETRY_BACKOFF_SECONDS:-90} * strix_gate_attempt )) - retry_reserve_seconds=$(( strix_gate_attempt_budget_seconds + backoff_seconds )) - remaining_seconds=$(( strix_gate_deadline - SECONDS )) - if [ "$strix_gate_attempt" -ge 3 ] || [ "$remaining_seconds" -lt "$retry_reserve_seconds" ]; then - echo "Provider-unavailable Strix attempt ${strix_gate_attempt} reached the bounded retry limit or the remaining job time budget (${remaining_seconds}s) is too small to retry; failing closed." >&2 + if [ "$strix_gate_attempt" -ge 3 ]; then + echo "Provider-unavailable Strix attempt ${strix_gate_attempt} reached the retry limit; failing closed." >&2 break fi echo "Strix provider outage on attempt ${strix_gate_attempt}; retrying after ${backoff_seconds}s backoff." >&2 diff --git a/CHANGELOG.md b/CHANGELOG.md index 43020db98e..f5810d5308 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Avoid redundant merge-scheduler wakes when the trusted receipt predicate + already finds a substantive exact-head OpenCode verdict. Missing, stale, or + fallback-only evidence still dispatches review work, while receipt lookup or + parsing failures remain fail-closed. The shared predicate explicitly rejects + fallback markers even when a normal overview heading is present, and its + live Reviews API reader slurps and flattens every pagination page. +- Grant the Strix stale-run cleanup job read-only pull-request access so its + job token can revalidate live heads in private repositories when optional + scheduler credentials are unavailable. - 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. diff --git a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md index 3a48cdf582..7b9ea7e1ac 100644 --- a/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md +++ b/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md @@ -208,3 +208,21 @@ all five, and auto-optimize routing by cost. like Noema, provision the pinned contextual-orchestrator sidecar and use `orchestrator/free`. The bootstrap still checks out no PR code and binds no Actions secret. +- **2026-08-31 amendment: model inference has no repository- or + application-configured fixed wall-clock timeout.** + OpenCode, Noema, Strix, and their contextual-orchestrator sidecar MUST NOT + impose a fixed wall-clock timeout on model inference, including an initial + completion ping, warm-up, retry, repair verdict, or substantive review call. + A slow reasoning model such as DeepSeek is not unavailable merely because it + takes minutes or hours to produce tokens. Cancellation remains an explicit + operator or superseded-head action. The review bootstrap also MUST NOT impose + fixed wall-clock limits on loopback `/healthz`, DNS/TLS establishment, ZDR + metadata, or provider model-list discovery: those prerequisites can be slow + and a short bound can discard an otherwise usable route before inference. + A hosting platform or runner termination is an external capacity constraint, + not model-unavailability or review evidence. Such an interrupted run is + incomplete and non-authoritative: it MUST NOT approve, merge, or classify the + model as unavailable, and the exact head MUST be retried or resumed on a + runner capable of completing the work. + This amendment supersedes all fixed readiness and inference-attempt budgets + in ADR 0005. diff --git a/docs/adr/0005-sidecar-preflight-token-budget.md b/docs/adr/0005-sidecar-preflight-token-budget.md index f024bc9933..3866281cfe 100644 --- a/docs/adr/0005-sidecar-preflight-token-budget.md +++ b/docs/adr/0005-sidecar-preflight-token-budget.md @@ -1,509 +1,25 @@ -# ADR-0005: Replace the sidecar's fixed-`max_tokens` gateway checks with diagnostic, bounded-retry readiness +# ADR-0005: Sidecar preflight token-budget diagnostics -- Status: proposed +- Status: Superseded by ADR 0003 on 2026-08-31 - Date: 2026-08-30 -- Scope: `ContextualWisdomLab/.github` central review pipelines' vendored `contextual-orchestrator` - sidecar — `scripts/ci/contextual_orchestrator_review_launcher.py`'s existing - `_preflight_review_agents`/`_preflight_with_fallback`, and - `scripts/ci/contextual_orchestrator_review_sidecar.sh`'s separate gateway smoke request — plus three - tracked upstream asks on `ContextualWisdomLab/contextual-orchestrator`. -- Decision: Keep both existing preflight layers (per-candidate launcher probing, and the shell - script's separate end-to-end request to the virtual `orchestrator/free` model) — neither is being - introduced, both already exist and each catches a failure class the other cannot. Fix what is - actually wrong with each with **two distinct, explicitly-bounded retry mechanisms** — one for "got a - response, it was empty because the budget was too small" (escalate budget), one for "got no response - at all, or a transport-level failure" (retry for a possibly-different route) — each drawing from a - small, explicit, shared attempt budget so worst-case latency is bounded and computed, not open-ended. - Track three upstream `contextual-orchestrator` asks (`ContextualWisdomLab/contextual-orchestrator#926`, - `#927`, `#932`) as real, tracked, non-blocking follow-ups. -- Ownership: `.github` owns the sidecar/launcher script and this ADR; `ContextualWisdomLab/contextual-orchestrator` - owns the gateway internals cited as evidence and the three follow-up issues. -- Figma File ID: N/A (no customer UI). +- Scope: Central OpenCode, Noema, and Strix review sidecars -## Context +## Historical context -Central review (`noema-review`/`opencode-review`/`strix`) depends on two separate, already-existing -liveness checks in the vendored sidecar, run in sequence — this ADR fixes both, it introduces neither. -Citations below pin to the exact reviewed blob at `main`'s -`8b3235d22129035b49ac481a40a341002540e2af` so line numbers cannot rot as the files change later. +This ADR originally proposed fixed wall-clock budgets and bounded retries for +review-sidecar readiness and generation. Those timing decisions are no longer +normative. They failed for legitimately slow models and for provider discovery, +OpenRouter ZDR lookup, DNS/TLS setup, and local `/healthz` checks. -1. **Per-candidate launcher probing** (bounded by the sidecar's own 180-second healthz-readiness wait — - see the family-cap comment in the sidecar script; this happens *before* the process can report - healthy, one candidate at a time, within that budget). - [`_preflight_review_agents()`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L200-L271) - sends one bounded `POST` to `client.proxy_send_once` for *each* candidate agent in the admitted - catalog, with a fixed `max_tokens=REVIEW_MAX_OUTPUT_TOKENS` (currently `4096`, - [L38](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L38)) - under a per-attempt - [`REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L45) - ceiling. It keeps every candidate whose response has non-empty text - ([`_chat_response_has_text`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L175-L189) - — checks only `choices[0].message.content`, never inspects `finish_reason`) and raises - `ReviewPreflightError` only if **zero** candidates pass — i.e. it is already an N-of-M ("at least one - must work") design, not a single-candidate gate. - [`_preflight_with_fallback()`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L274-L291) - wraps this with one fallback catalog tier. -2. **The shell script's own virtual-pool smoke request.** Once `/healthz` succeeds (a separate, - already-completed budget — Layer 2 does not draw from Layer 1's 180s), the shell script sends one - `POST /v1/chat/completions` with `"model":"orchestrator/free"` (the *virtual* pool id, not a - specific candidate) and its own fixed `max_tokens`, currently `4096`, under a **120-second** - `curl --max-time`. This 120s value is itself the outcome of a prior, real, evidenced fix in this - exact file (raised from a too-tight 30s after live reproduction on - `ContextualWisdomLab/contextual-orchestrator#921` showed a genuinely-healthy DeepSeek NIM route - needing more than 30s to complete a real generation) — the comment there explicitly documents that - this required-workflow job budgets **120 minutes** total (`timeout-minutes` in - `strix.yml`/`noema-review.yml`) and that *"the org's own stated policy accepts multi-hour central - review latency in favor of accuracy over speed."* This ADR's design deliberately **does not shorten - that 120s value** — doing so would reintroduce the exact regression that prior fix corrected. The - correct fix for a hang, per Devin Review (see Decision §1), is a bounded *retry*, not a shorter - *timeout*. +## Superseding decision -`N` (the `max_tokens` literal) has already been tuned twice: 16 → 4096 (#1436), moving the failure -from "empty content at 16 tokens" (the provider's response consumed the whole budget on internal -reasoning before emitting visible content — see `ModelClient._response_content`'s own anticipated -error message, quoted below) to "120s timeout with zero bytes at 4096 tokens" on a separate run. -Direct owner feedback in response to that outcome, quoted verbatim because it is the reason this ADR -exists: +ADR 0003 governs these operations. Inference, initial ping/preflight, warmup, +retry/repair, provider discovery, OpenRouter ZDR lookup, DNS/TLS setup, and local +health checks have no fixed wall-clock timeout. Work ends only through an +operator action or cancellation of an obsolete PR head. -> "max_tokens 이걸 고정하는 게 말이 안 되는데" — hardcoding this max_tokens doesn't make sense. -> "모델마다 max_tokens 허용치가 다 다른데" — each model has a genuinely different max_tokens allowance. +Response validation remains fail closed. Token-budget diagnostics may explain +empty or truncated output, but they do not impose a wall-clock deadline. -`orchestrator/free` is a heterogeneous pool (`nvidia_nim`, `openai`, `opencode_zen`, `bytez`, -`openrouter`, ... — see `contextual_orchestrator_review_policy.py`'s credential table), and which -candidate a given preflight run draws varies. A fixed `max_tokens` is wrong on two independent, -evidenced axes for a pool like this: - -1. **Reasoning-token overhead differs per model.** A model that spends internal reasoning tokens - before emitting visible content can exhaust a small budget with zero visible output. OpenAI's own - documentation of `finish_reason == "length"` describes exactly this: *"it's likely that max_tokens - is too small and model runs out of tokens before it manages to [complete]"* - ([OpenAI API guide](https://developers.openai.com/api/docs/guides/completions)). -2. **The provider's own hard ceiling on completion tokens differs per model**, and is a genuinely - separate quantity from a model's context window (see Research §3 below). Some providers reject a - request outright if `max_tokens` exceeds what that specific model supports; others support far more - than a generic constant would ever request. A single number can therefore be simultaneously too - small for one model's reasoning overhead and too large for another model's real ceiling. - -The standing session principle governing this decision, also quoted verbatim: "어떠한 휴리스틱과 Rule -of thumbs도 금지" — no heuristics or rules of thumb; a parameter needs actual justification from real -data, not a constant that happens to work today. - -## Research: three questions, checked directly against `contextual-orchestrator` source and, where the -## claim is about external provider behavior, against the providers' own current documentation - -### 1. Does the gateway expose a way to separate a reasoning budget from a content budget? - -**No.** `ReasoningEffortProfile`/`apply_request_profile()` (`reasoning_effort_profile.py`) is real but -**additive, not substitutive**: it always sets `payload["max_tokens"]` regardless of `reasoning_effort`. -OpenAI documents the analogous parameter the same way: `max_completion_tokens` is *"an upper bound for -the number of tokens that can be generated for a completion, **including** visible output tokens and -reasoning tokens"* (same OpenAI guide). The mechanism is also opt-in at `TaskOrchestrator` construction -(`_role_effort_profile(role)` returns `None` unless a `role_effort_catalog` was configured), and the -public `/v1/chat/completions`/`/v1/responses` endpoints this preflight and Strix use both treat a -caller-supplied `reasoning_effort`/`reasoning` field as a documented no-op (`server.py`'s own -docstrings: `_validate_chat_reasoning_effort`, `_validate_responses_reasoning`). - -**Conclusion**: there is no lever, on any caller-facing surface this preflight (or Strix) can reach, -that separates "let the model think as long as it needs" from "cap what it can emit." - -### 2. Is a real-generation preflight even the right liveness mechanism — is there a cheaper or more direct signal? - -**A better-shaped mechanism exists in two places — one already in this sidecar, one further -upstream — but neither is a free non-generation signal.** - -- **Already in this repo**: `_preflight_review_agents()` already probes every candidate individually - and already tolerates any number of individual failures. What it lacks is not the *shape* but a way - to tell "this candidate is down" apart from "this candidate is healthy but its probe's budget was - wrong for it," and (separately) a way to survive a hang with no response at all — see Decision §1. -- **Further upstream, admin-scoped**: `ModelClient.probe()`/`provider_readiness_report()` are the - gateway's own, more mature version of the same idea. Verified directly: `/api/v1/*` GET routes are - authorized at **`admin` scope**, while `/v1/chat/completions` — what the sidecar's bearer token is - scoped for today — is authorized at the narrower **`inference` scope**. Provisioning the sidecar with - an admin-scoped token just for this would be a real privilege widening this ADR does not recommend. - Tracked as `ContextualWisdomLab/contextual-orchestrator#926`. -- **Neither eliminates real generation, and neither eliminates the possibility of a hang.** `probe()` - itself hardcodes `max_tokens: 1` and has no retry of its own. - -**Conclusion**: reuse the shape that already exists in this sidecar; fix its calibration and add -bounded retries (Decision §1); track the upstream, better-tested version as a non-blocking follow-up. - -### 3. If a numeric budget is still needed, can it be derived per-model from real discovered data? - -**Not today.** Neither `DiscoveredModel` (`model_discovery.py`) nor `ModelAgent` (`orchestrator.py`) -carries any field for a model's output-token ceiling or context window — confirmed via full-dataclass -read and grep. This is **two distinct pieces of data, not one** — verified directly against -OpenRouter's current OpenAPI spec (`https://openrouter.ai/openapi.yaml`): `Model.context_length` -(required) is *"Maximum context length in tokens"*; `TopProviderInfo.max_completion_tokens` (nullable -— genuinely absent for some models) is *"Maximum completion tokens from the top provider. Input and -output tokens share the context window, so the effective maximum output for a request is further -limited by the context remaining after input tokens."* Only the second field can directly clamp a -`max_tokens` request parameter. - -**Conclusion**: tracked as `ContextualWisdomLab/contextual-orchestrator#927`, not undertaken here. - -## Decision - -### 1. Two distinct, explicitly-bounded retry mechanisms, not one generic "retry" — and not the same behavior in both layers - -Devin Review correctly found that a single "retry on empty content + `finish_reason == 'length'`" -predicate cannot fix the actual live outage this ADR is responding to: the reproduced failure (job -`99253418179`, cited in the Evidence trail) is a **120-second timeout with zero bytes received** — -there is no response object at all in that case, so there is no `finish_reason` to inspect, and the -original design's retry path would never trigger for it. Fixed by splitting into two independent -triggers. **Layer 1 and Layer 2 use these triggers differently, by structural necessity, not by -inconsistency — the difference is stated once here and referenced everywhere else, rather than -implied and then contradicted section to section (a real self-contradiction Devin Review's third pass -correctly caught in an earlier revision of this text):** - -- **Trigger A — no usable response** (transport timeout, connection failure, or non-2xx status on the - *first* attempt at a given budget). - - **Layer 2**: retry with a fresh attempt at the same `4096` budget, up to the shared attempt cap - (Decision §3). Layer 2 has exactly one check — there is no other candidate to fall back to — so a - hang there must be survived by retrying, or the reproduced outage is not actually fixed. **This - retry is justified even without any guarantee of hitting a different underlying candidate** — see - the route-diversity note below — because it is bounded and strictly better than the current - design's single unconditional attempt with no recovery path at all: worst case, the outcome is - identical and the check still fails closed with the same accurate diagnosis; best case, a - transient failure (a network blip, a momentarily overloaded connection) clears on retry. - - **Known, accepted Layer 2 limitation, verified against actual `contextual-orchestrator` source - (not assumed): a Trigger-B-shaped failure can itself surface at Layer 2 as a Trigger-A non-2xx, - misclassified.** `ModelClient._response_content` raises `ProviderResponseError` for the - reasoning-without-content case (Decision §1's Trigger B, second signature); `server.py`'s request - handler catches `ProviderResponseError` with one blanket handler that always returns `HTTP 502 - invalid_structured_output` with a fixed, generic message — the two distinct `ProviderResponseError` - messages (reasoning-without-content vs. no-content-at-all) collapse to an identical response body, - and neither the caught exception's own message nor any other machine-readable field distinguishes - them (the `except ProviderResponseError:` handler does not even bind the exception). Layer 2's - sidecar script therefore cannot tell this case apart from any other non-2xx and, by elimination, - treats it as Trigger A: retried up to `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS` times against a - candidate the gateway is, by the same reasoning as the Trigger-B/route-diversity note below, more - likely to repeat than diversify away from. **This does not change Layer 2's stated worst case** - (`REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS × 120s` — this failure still consumes attempts from the - same shared Trigger-A budget, not an additional one), but it does mean this specific failure - typically consumes the *entire* retry budget before failing closed, rather than failing fast the - way a correctly-classified Trigger B would (one attempt, ~120s). A correct fix requires a - `contextual-orchestrator` change (a machine-readable field distinguishing the two - `ProviderResponseError` cases through the `/v1/chat/completions` error boundary) — genuinely out of - scope for this sidecar-only ADR and its stacked implementation PR. Fragile string-matching on the - human-readable error message is explicitly rejected as a workaround (this codebase's own - convergence rule rejects heuristics without real, stable signal, and the message text is not - contractually stable). Tracked as `ContextualWisdomLab/contextual-orchestrator#932`; not blocking - this ADR or its implementation. - - **Layer 1**: **no retry**. Layer 1 already probes up to 12 distinct candidates - (`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`); one candidate's timeout simply consumes its existing 10s - slot and the loop moves to the next candidate, exactly as it does today. A same-candidate retry - here would add latency without adding resilience Layer 1's own multi-candidate design does not - already provide. -- **Trigger B — a response was received, `message.content` is not usable text (missing, `null`, - non-string, OR a genuinely empty string `""` — this preflight's own "no content" definition is - deliberately broader than any one downstream library call's exact return-value convention; see the - precision note below), and EITHER `choices[0].finish_reason == "length"` (the OpenAI-documented - signature of "budget too small," cited above) OR the vendored `ModelClient._response_content`'s own - broader signature: a populated `message.reasoning` field with no string `content`** (already - anticipated in the codebase's own error message, quoted in the Evidence trail: *"provider {agent.id} - returned reasoning without content ... increase max_output_tokens"*). **This second condition is not - optional — it is the exact original failure mode PR #1436 responded to** ("empty content at 16 - tokens" moving to a materially larger budget), and a `finish_reason`-only predicate would miss it - entirely: a reasoning model can exhaust its budget mid-reasoning under a `finish_reason` other than - `"length"`, or with no `finish_reason` field present at all — provider `finish_reason` semantics for - this specific case are not verified as uniform across a pool this heterogeneous (`nvidia_nim`, - `openai`, `opencode_zen`, `bytez`, `openrouter`, ...), so relying on `finish_reason` alone would - silently leave a genuinely healthy reasoning-capable candidate misclassified as down — the same class - of false-negative Decision §1's Trigger-A/B split already exists to prevent, just for a different code - path (a real response object this time, not a hang). - - **Precision note, verified directly against the vendored source (not assumed): `_response_content` - checks `isinstance(content, str)` *first* and returns immediately if true — including for a - genuinely empty string `""`, which it treats as a valid (if degenerate) successful return and never - reaches its own `reasoning` check for. `_response_content`'s reasoning-without-content *exception* - therefore fires only when `content` is missing/`null`/non-string, not for `content == ""`.** This - preflight's own predicate is intentionally **broader** than that one exact technical condition: it - treats `content == ""` the same as missing content (matching this same section's own "not usable - text" definition above, and `_chat_response_has_text`'s existing definition, both already used - elsewhere in Layer 1) — an empty visible answer is exactly as useless to a caller as no answer at - all for a *readiness* probe's purposes, regardless of whether `_response_content`'s own downstream - consumption code happens to accept `""` without raising. The citation to `_response_content` above - is the *motivating* signature this preflight generalizes from, not a claim that the implementation - must reproduce that function's exact, narrower branching. - - **Layer 1**: retry that *same* candidate (`client.proxy_send_once(agent, ...)` pins the exact agent - object, so this retry is genuinely attributable to that one candidate) once at a **materially - larger** budget — `REVIEW_PREFLIGHT_ESCALATED_TOKENS` (`4096`, reusing `REVIEW_MAX_OUTPUT_TOKENS`), - up from a `16`-token base probe (`REVIEW_PREFLIGHT_BASE_TOKENS` — a **new, smaller** value than the - `4096` Layer 1 uses today; see Decision §3). This is the only place in either layer where the - budget itself changes. - - **Layer 2**: **no retry on EITHER half of Trigger B — this is a deliberate simplification made - across this ADR's review, not an oversight.** Devin Review's fourth pass found the reason directly: - a Trigger-B response (whichever signature matched) is still `HTTP 200` — the gateway's own routing - layer already recorded that as a *successful* attempt before the sidecar ever inspects the content, - so a subsequent identical request is not a fresh, independent draw against the pool; the gateway's - routing is more likely to *repeat* the same "successful" candidate than to diversify away from it. - Retrying at the same budget against the same likely candidate has no principled reason to produce a - different outcome, so Layer 2 does not attempt it for either signature: an empty response matching - Trigger B at Layer 2 is recorded as not-ready immediately, with whichever signature matched - (`finish_reason` and/or the reasoning-without-content signal) preserved in the report for diagnosis. - -**Route diversity on Layer 2's Trigger-A retry is a best-effort hope, not a verified guarantee, and -this ADR stops trying to force it.** This is the fourth time a version of "does the retry actually -reach a different or better outcome" has come back reshaped across Devin Review's passes on this ADR -(round 2: a too-small budget; round 3: an escalated retry that could hit an unaccountable different -candidate; round 4: the specific case above). Checked directly rather than assumed before accepting -this as final: `contextual_orchestrator/server.py`'s request handling exposes no field to exclude, -deprioritize, or pin away from a specific candidate on a subsequent call — grepped for any such -parameter and found none. Given no verified mechanism to force diversity exists, and per this org's -convention to converge on an honestly-scoped decision rather than iterate indefinitely toward a fully -"solved" design, this ADR's final position is: **Layer 1's genuine N-of-M across truly distinct, -individually-addressed candidates is what does the real resilience and diversity work in this design. -Layer 2 remains what it always was — a single end-to-end smoke test proving the virtual-pool dispatch -path itself works at all — and its bounded retry (Trigger A only) is a modest, honest safety margin -against transient failures, not a pool-exploration mechanism.** If the gateway later exposes a real way -to exclude a specific candidate, that would improve Layer 2's retry meaningfully and should be -revisited then (a natural extension of `ContextualWisdomLab/contextual-orchestrator#926`); this ADR -does not invent that mechanism speculatively. -- **Both triggers draw from one small, shared, explicit retry budget per layer** (Decision §3), not - "one retry per route" unconditionally. -- **A non-2xx rejection on a Layer 1 escalated (Trigger-B) retry** is distinguishable evidence the - *escalated* budget specifically — not the base one — exceeds that one candidate's real ceiling - (genuinely attributable, since the candidate is pinned). Recorded as its own outcome, - `escalated_probe_rejected`, and that candidate is not retried further this run. The complete fix - (knowing each model's real ceiling in advance) is `ContextualWisdomLab/contextual-orchestrator#927`, - not this ADR. -- **A non-2xx rejection on a Layer 2 Trigger-A retry** is recorded as `gateway_retry_rejected` — - deliberately **not** named or described as candidate-ceiling evidence, because Layer 2 structurally - cannot confirm which candidate served the rejected attempt. -- **Every other outcome is not retried**: a non-2xx result, or an empty response matching neither of - Trigger B's two signatures (`finish_reason == "length"` nor a populated `message.reasoning` with no - content), on an attempt that is not eligible for Trigger A or B for that layer (i.e., already the - layer's one retry, or already past its shared budget) is recorded as not-ready immediately. - -### 2. Keep both existing layers — neither replaces the other - -Layer 1's per-candidate checks call `client.proxy_send_once` against explicit candidate agents directly -and structurally cannot detect a bug in the virtual pool's own dispatch/selection code, which is a -different code path. This is not hypothetical: the 2026-08-30 gap-baseline entry for PR #1433 records -exactly this split failure live — the launcher's own per-candidate preflight passed and the server -reported healthy, while the shell script's separate virtual-pool request still came back `HTTP 502`. -Layer 2 also independently reproduced the ADR's own motivating bug live on PR #1449 itself (Evidence -trail). Any redesign that dropped Layer 2 in favor of Layer 1 alone would silently reintroduce both. - -### 3. Explicit, bounded, per-layer retry budgets and the resulting worst-case arithmetic - -Devin Review's third finding is correct: `REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES` (12) candidates each -retried once, unconditionally, would be a real, computed worst-case blowup against Layer 1's own -180-second healthz-readiness budget. Fixed with an explicit shared cap per layer, not an unbounded -"one retry per route": - -- **Layer 1** (bounded by the existing 180s healthz-readiness wait, unchanged): keep the existing - per-attempt timeout (`REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10`, unchanged). The **base probe budget - changes from `4096` (today's value) to a new, smaller `REVIEW_PREFLIGHT_BASE_TOKENS = 16`** — cheap - by design, because the escalation path below corrects for it being wrong, unlike today where a wrong - first (and only) guess is fatal. Trigger A does not need its own retry allowance here (see Decision - §1). Trigger B (escalate to `REVIEW_PREFLIGHT_ESCALATED_TOKENS = 4096`, reusing today's - `REVIEW_MAX_OUTPUT_TOKENS`, on `finish_reason == "length"` OR a populated `message.reasoning` with no - content — see Decision §1's full Trigger B definition) is capped by a new shared counter, - `REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4`, across the whole Layer 1 run (not per-candidate) — once 4 - candidates have consumed an escalation attempt, any further candidate that would otherwise qualify - for Trigger B is instead recorded not-ready immediately with an explicit - `escalation_budget_exhausted` reason. **Worst case (probing only)**: 12 × 10s (base attempts) + 4 × - 10s (escalation attempts) = **160s**, under the existing 180s ceiling with real margin, computed - rather than assumed. **This 160s covers only probing** — it does not include the launcher's own - pre-probe startup work (KV credential registration, `discover_all_models()`'s sequential provider - discovery, ZDR-prioritized catalog construction), which runs first, inside the *same* 180s watchdog. - Verified directly against the vendored `contextual_orchestrator.model_discovery` source during the - implementation pass: discovery alone can take up to ~105s worst case (up to ~7 sequential HTTP calls - at up to 15s each), for a combined real worst case of up to ~265s, not 160s. **Known, accepted, - tracked limitation, not redesigned here**: `ContextualWisdomLab/.github#1455` (filed and reasoned in - full during the implementation PR, `ContextualWisdomLab/.github#1452`) — accepted as non-blocking - because the failure mode requires two unlikely conditions to coincide in one run (discovery near its - own worst case *and* probing separately needing close to its full escalation budget), and no real - discovery-timing telemetry exists yet to justify a specific fix (a shared deadline, scaled-down - probing, or a justified watchdog extension) without guessing, which this ADR's own convergence - principle already rejects (Context, "어떠한 휴리스틱과 Rule of thumbs도 금지"). This ADR does not - reopen that question; see #1455 for the full analysis and options considered. - **Second known, accepted, tracked limitation on this same shared counter**: candidates are probed in - catalog order — deterministic, not random, but not purely alphabetical either: verified directly - against `build_zdr_prioritized_catalog`'s actual sort key - (`contextual_orchestrator_review_policy.py`), eligible rows sort by `(cost_evidence_rank, - zdr_attested_rank, provider, model)` — cost-evidence tier first (constant within `orchestrator/free`, - since every row is already free), ZDR-attested status second (ZDR-attested candidates sort before - non-attested ones, regardless of `require_zdr`), and `(provider, model)` alphabetically only as the - tie-breaker within each same-cost/same-ZDR-status group — and the - 4-escalation budget is consumed strictly first-come-first-served, so a candidate that sorts later in - the catalog can be denied its own escalation attempt purely because 4 earlier candidates already - claimed the shared budget, even if that later candidate would have succeeded at the escalated budget. - Considered and rejected as not cheaply fixable: the budget must stay shared and bounded (unbounded - per-candidate escalation is exactly what round-3's already-fixed finding ruled out), and no selection - policy for *which* candidates get the fixed slots — catalog order, round-robin, random shuffling, - family-priority — removes the underlying trade-off, only changes which arbitrary policy governs it; - picking one without real evidence on which candidates actually need escalation more often would - itself be exactly the unjustified heuristic this ADR's convergence principle already rejects. - Tracked as `ContextualWisdomLab/.github#1458`; revisit if real hosted-run telemetry (already required - below) shows a specific, evidenced bias worth correcting. -- **Layer 2** (bounded only by the job's own 120-minute ceiling, per the org's stated "accuracy over - speed" policy already reasoned in this file — *not* by the 180s Layer 1 budget, which has already - completed by the time Layer 2 runs): keep the existing per-attempt timeout (**120s, unchanged** — not - shortened, per Context above) and the existing **`4096` budget, unchanged throughout — Layer 2 never - escalates** (already proven working on a real hosted run, `contextual-orchestrator#921`; see Decision - §1 for why an escalation tier was considered and dropped here). Allow up to - `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS = 3` total attempts, consumed only by Trigger A (transport - failure/hang/non-2xx) — Trigger B (empty + either its `finish_reason == "length"` or - reasoning-without-content signature) is not retried at Layer 2 at all (Decision §1). **Worst case**: - 3 × 120s = **360s (6 minutes)** — - explicit, bounded, and small relative to the job's 120-minute ceiling; the previous design's worst - case was already 120s for one unconditional attempt with no chance of recovery, so this trades a - bounded amount of additional worst-case latency for surviving exactly the transient-hang class of - failure reproduced live on this ADR's own PR. -- **Initial values are reused precedent, not new guesses** (Devin Review's fourth finding): every - number above is either already deployed in this exact codebase today (`10s`, `120s`, `4096`, `12`) - or has direct external documentation backing it (`16` — the pre-#1436 value this codebase already - ran with, and separately the floor OpenRouter's own schema documents: *"some providers enforce a - minimum of 16"* for the deprecated `max_tokens` field). The two new counters - (`REVIEW_PREFLIGHT_MAX_ESCALATIONS`, `REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS`) are chosen to keep each - layer's worst case under its own already-established ceiling, shown above, not picked by inspection - of "what feels right." The implementation must have both preflight layers emit `finish_reason`, the - reasoning-without-content signal (Trigger B's other half), attempt count, and which trigger fired in - their structured reports (`_preflight_review_agents`'s `routes[]`; the shell script's - `preflight_report`/`gateway` JSON) — this ADR does not implement that - itself (see Status) — specifically so that a **follow-up, evidence-driven pass** — after - observing real hosted runs with this telemetry — can adjust these two counters and the base/escalated - token budgets from real data, which is the methodology this ADR commits to for future tuning: initial - values from direct precedent, refinement from telemetry this change itself introduces, never from - inspection alone. - -### 4. Upstream tracking and rejection of further constant-tuning - -- **Track `ContextualWisdomLab/contextual-orchestrator#926`** (an `inference`-scoped variant of - `provider_readiness_report`/`probe()`) so the sidecar can eventually retire its hand-rolled Layer 1 - loop. Not blocking for §1-3. -- **Track `ContextualWisdomLab/contextual-orchestrator#927`** (real, separately-provenanced - `max_output_tokens`/`context_window` fields, fail-closed when unknown) so `max_tokens` selection can - eventually be derived from real per-model data, including resolving the `escalated_probe_rejected` - case in §1 properly instead of just recording it. Not blocking for §1-3. -- **Track `ContextualWisdomLab/contextual-orchestrator#932`** (a machine-readable field through the - `/v1/chat/completions` error boundary distinguishing `ProviderResponseError`'s reasoning-without-content - cause from its no-content-at-all cause) so Layer 2 can eventually classify a gateway-side - reasoning-without-content failure as Trigger B instead of by-elimination Trigger A (§1). Not blocking - for §1-3. -- **Explicitly reject** further tuning of one global `max_tokens` constant, or of a single generic - "retry," as a terminal fix for either layer. Every single-constant value tried so far (16, 4096) has - failed for a different, evidenced reason tied to pool heterogeneity, and a single undifferentiated - retry predicate does not cover the failure class (a hang) that actually reproduced live on this ADR's - own PR. - -## Consequences - -**This ADR is `proposed`; no code has shipped yet. The consequences below describe what the -implementation is expected to achieve once it lands, verified against this ADR's design — not an -outcome already observed in production.** - -- Once implemented, both preflight layers would become structurally tolerant of an individual attempt - being wrong for a fixed token budget, or hanging/failing transiently, which is the actual shape of - the problem — while keeping every worst case explicit and bounded rather than open-ended. -- Layer 1's worst case would grow from ~120s to a computed 160s, still under its existing 180s - healthz-readiness ceiling. Layer 2's worst case would grow from a single 120s attempt with no - recovery path to up to 360s across bounded retries — small relative to the job's 120-minute ceiling - and consistent with this file's own already-stated "accuracy over speed" policy. -- Keeping Layer 2 (not just Layer 1) would mean the preflight still proves the actual consumer-facing - `orchestrator/free` route works, not only that individual candidates can respond in isolation — - closing the PR #1433 gap class rather than reopening it. Giving Layer 2 a bounded retry (rather than - either a single unconditional attempt or a shortened timeout) is what would actually address the live - 120s-hang reproduction on this ADR's own PR (job `99253418179`) — a shortened timeout alone would not - have, and would have regressed the prior, already-evidenced 30s→120s fix in the same file. Whether it - would have *prevented* that exact reproduction is not claimed with certainty (Layer 2's retry has no - verified route-diversity guarantee — see Decision §1); what it would change is that the check no - longer fails after one unconditional attempt with zero chance of recovery. -- A Layer 1 candidate whose escalated probe is rejected outright (rather than merely still empty) would - be recorded as not-ready with a distinct, honest reason rather than silently retried indefinitely or - misclassified — a known, accepted, documented residual limitation until - `ContextualWisdomLab/contextual-orchestrator#927` lands. Layer 2's retry-diversity limitation - (Decision §1) is accepted the same way, for the same reason: no verified mechanism exists today to - do better. -- A Layer 2 reasoning-without-content failure that surfaces through the gateway as a generic `HTTP 502` - (rather than a `200` with empty content, the case Layer 2's Trigger B was designed around) is - misclassified as Trigger A and retried, rather than failing fast the way a correctly-classified - Trigger B would — accepted the same way as the two limitations above, for the same reason: fixing it - requires a `contextual-orchestrator` change (a machine-readable field through the - `/v1/chat/completions` error boundary distinguishing this cause from any other non-2xx), out of scope - for this sidecar-only ADR, and no in-repo workaround exists that does not depend on fragile, - contractually-unstable message-text matching. Does not change Layer 2's stated worst case (this - failure still draws from the same shared Trigger-A attempt budget). Tracked as - `ContextualWisdomLab/contextual-orchestrator#932`. -- Layer 1's `160s` worst case (Decision §3) covers probing only, not the launcher's own pre-probe - startup work (KV registration, model discovery, catalog construction), which runs first inside the - same 180s watchdog — verified at up to ~105s worst case for discovery alone, for a combined real - worst case of up to ~265s. Accepted the same way as the limitations above: the failure mode needs two - unlikely conditions to coincide, and no real discovery-timing telemetry exists yet to justify a - specific fix without guessing. Tracked as `ContextualWisdomLab/.github#1455`. -- The shared, catalog-order-consumed `REVIEW_PREFLIGHT_MAX_ESCALATIONS` budget can deny a - later-sorting, genuinely healthy candidate its own escalation attempt once 4 earlier candidates have - already claimed the budget — accepted the same way: the budget must stay shared and bounded (an - unbounded per-candidate escalation was already ruled out, Decision §3), and no selection policy for - the fixed slots is justified by real evidence today. Tracked as `ContextualWisdomLab/.github#1458`. -- Items in Decision §4 are real `contextual-orchestrator` feature work, now tracked as real issues, and - would remain explicitly not closed by this ADR even once the sidecar-side implementation lands. -- No production routing default changes are proposed; this is scoped to the sidecar's own liveness - checks. -- **This is currently active, not theoretical**: the live reproduction in the Evidence trail below is - from `noema-review` failing on this ADR's own PR while this ADR was being written, presently - blocking that required check org-wide on every repo that routes through this sidecar. The - implementation follow-up applying this Decision should be prioritized accordingly, not treated as - ordinary backlog. - -## Evidence trail - -All source citations below are permalinks to the exact reviewed blob at -`8b3235d22129035b49ac481a40a341002540e2af` (the `main` commit this research was performed against), so -line numbers cannot rot as these files are edited later. - -- [`_preflight_review_agents`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L200-L271), - [`_preflight_with_fallback`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L274-L291), - [`_chat_response_has_text`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L175-L189), - [`REVIEW_MAX_OUTPUT_TOKENS`/`REVIEW_PREFLIGHT_TIMEOUT_SECONDS`/`REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES`](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_launcher.py#L36-L47) - — the existing Layer 1 mechanism this ADR fixes, not introduces. -- [`scripts/ci/contextual_orchestrator_review_sidecar.sh`, the healthz-wait loop and its 180s budget comment](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_sidecar.sh#L67-L69), - and [the virtual-pool smoke request and its existing 30s→120s rationale](https://github.com/ContextualWisdomLab/.github/blob/8b3235d22129035b49ac481a40a341002540e2af/scripts/ci/contextual_orchestrator_review_sidecar.sh#L430-L475) - — the existing Layer 2 mechanism this ADR fixes, not introduces or shortens. -- 2026-08-30 gap-baseline entry (PR #1433 evidence): *"the shell script's separate, subsequent real - `/v1/chat/completions` gateway smoke request against the now-serving `orchestrator/free` virtual - model came back HTTP 502. This is a different code path than the launcher's own preflight - (`ModelClient.proxy_send_once` against explicit candidate agents)"* — the direct, already-documented - precedent for why Layer 2 cannot be dropped in favor of Layer 1 alone. -- `ModelClient._response_content` (`orchestrator.py:1648-1660`) — the "reasoning without content" - failure this investigation traces to, already anticipated in the codebase's own error message: - *"provider {agent.id} returned reasoning without content; for mlx-lm set - chat_template_args={"enable_thinking": false} or increase max_output_tokens."* -- `ModelClient.apply_effort_profile` / `reasoning_effort_profile.apply_request_profile` — confirms - `max_tokens` is always set regardless of `reasoning_effort`. -- `server.py:3731-3758` (`_validate_chat_reasoning_effort`), `server.py:4775-4809` - (`_validate_responses_reasoning`) — confirms both fields are validated, documented no-ops on the - caller-facing surfaces this preflight and Strix use. -- `ModelClient.probe` (`orchestrator.py:1483-1561`), `TaskOrchestrator.provider_readiness_report` - (`orchestrator.py:3441-3486`), `server.py:5711-5715` — the upstream mechanism, and its admin-scope - gate vs. the `inference`-scoped `/v1/chat/completions`/`/v1/models` handlers. -- **External, directly-fetched citations** (verified live against the providers' own current - documentation before citing, per this org's traceability convention): - - OpenAI, [*Completions API guide*](https://developers.openai.com/api/docs/guides/completions): - `finish_reason == "length"` — *"it's likely that max_tokens is too small and model runs out of - tokens before it manages to [complete]"*; `max_completion_tokens` — *"an upper bound for the - number of tokens that can be generated for a completion, including visible output tokens and - reasoning tokens."* - - OpenRouter, OpenAPI spec (`https://openrouter.ai/openapi.yaml`), `Model.context_length` — - *"Maximum context length in tokens"* (required); `TopProviderInfo.max_completion_tokens` — - *"Maximum completion tokens from the top provider. Input and output tokens share the context - window, so the effective maximum output for a request is further limited by the context - remaining after input tokens"* (nullable); the deprecated `max_tokens` field description — - *"Note: some providers enforce a minimum of 16"* — the direct evidence for this ADR's `16`-token - Layer 1 base probe value. -- `ContextualWisdomLab/contextual-orchestrator#926`, `#927`, `#932` — the three tracked upstream - follow-ups. -- **Live reproduction on this ADR's own PR**, verified directly against the job log rather than taken - on report: `noema-review` on `ContextualWisdomLab/.github#1449` (job `99253418179`, - `https://github.com/ContextualWisdomLab/.github/actions/runs/33310078256/job/99253418179`) — - ``` - 2026-08-30T11:58:29Z healthz and provider-route preflight confirmed after 30s (pid 3973) - 2026-08-30T12:00:29Z curl: (28) Operation timed out after 120002 milliseconds with 0 bytes received - 2026-08-30T12:00:29Z error: gateway preflight request could not reach the local sidecar - ``` - Layer 1 (per-candidate) passed in 30s; Layer 2 (the virtual-pool smoke request) then hung for - exactly the full 120s timeout with **zero bytes received** — no response, no `finish_reason`, - nothing. This is exactly Decision §1's Trigger A case (not Trigger B, which requires a response to - exist) — confirming why the two triggers had to be modeled separately, and why this specific evidence - is what Decision §3's Layer 2 bounded-retry design (up to 3 attempts) exists to survive. +The former attempt counts, retry ceilings, and timeout values in this ADR are +historical evidence only and must not be restored. diff --git a/scripts/ci/contextual_orchestrator_review_launcher.py b/scripts/ci/contextual_orchestrator_review_launcher.py index f115ef2b88..02bc07c28a 100644 --- a/scripts/ci/contextual_orchestrator_review_launcher.py +++ b/scripts/ci/contextual_orchestrator_review_launcher.py @@ -40,10 +40,6 @@ # Provider-neutral sampling: several modern endpoints reject non-default # temperatures, while 1.0 is the OpenAI-compatible default. REVIEW_TEMPERATURE = 1.0 -# A selected route that cannot answer within ten seconds is not reliable enough -# for a required CI gate. With at most twelve sequential candidates, startup is -# bounded below the sidecar's three-minute readiness deadline. -REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10 REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES = 12 REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT = 8 # ADR-0005: a single fixed max_tokens cannot fit every model in a heterogeneous @@ -62,27 +58,7 @@ # number. REVIEW_PREFLIGHT_ESCALATED_TOKENS = REVIEW_MAX_OUTPUT_TOKENS # Shared cap on how many candidates in one preflight run may use the -# escalation retry above, so Layer 1's PROBING worst case stays computed and -# bounded: REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES * REVIEW_PREFLIGHT_TIMEOUT_SECONDS -# + REVIEW_PREFLIGHT_MAX_ESCALATIONS * REVIEW_PREFLIGHT_TIMEOUT_SECONDS -# = 12*10 + 4*10 = 160s, under the sidecar's 180s healthz-readiness wait. See -# docs/adr/0005-sidecar-preflight-token-budget.md, Decision section 3. -# -# KNOWN GAP, tracked (not yet fixed): this 160s covers only probing, not the -# discover_all_models() call that runs before it inside the SAME 180s -# watchdog. Verified directly against the vendored contextual-orchestrator -# source: discover_all_models() makes up to ~7 sequential HTTP calls (the -# shared models.dev fetch, one per PROVIDER_MODEL_SOURCES entry with a -# registered credential, and the OpenRouter ZDR endpoint fetch), each up to -# DISCOVERY_TIMEOUT_SECONDS = 15s -- up to ~105s worst case, before probing's -# own 160s even starts. Combined real worst case is therefore up to ~265s, -# not 160s. See ContextualWisdomLab/.github#1455 for the tracked fix (a -# shared monotonic deadline, scaled-down probing, or an evidence-justified -# watchdog extension) and #1454 for the related, separately-tracked gap that -# a base-probe *success* never confirms the candidate at the real serving -# budget (REVIEW_MAX_OUTPUT_TOKENS). Neither blocks this PR's 7 verified -# findings; both are architecturally significant enough to need their own -# design pass rather than a guessed patch here. +# escalation retry above. It bounds request count, never model response time. REVIEW_PREFLIGHT_MAX_ESCALATIONS = 4 @@ -555,8 +531,8 @@ def _preflight_with_fallback( stage's ending ``escalations_used`` is passed as the fallback stage's starting point, so a run that rejects all 8 primary routes and then probes 4 fallback routes still spends at most 4 escalations total (12 - base attempts + 4 escalations, 160s worst case) instead of up to 8 (200s) - -- which would exceed Layer 1's 180s healthz-readiness wait. Both + base attempts + 4 escalations). This bounds request count, not individual + model response or sidecar readiness time. Both stages' reports remain in the result: the fallback (or sole) stage's report carries the run's final, cumulative ``escalations_used``, and ``primary_attempt`` nests the primary stage's own report -- including its @@ -931,7 +907,6 @@ def main(argv: list[str] | None = None) -> int: loader=load_agents, ) client = ModelClient( - timeout=REVIEW_PREFLIGHT_TIMEOUT_SECONDS, max_output_tokens=REVIEW_MAX_OUTPUT_TOKENS, max_retries=0, temperature=REVIEW_TEMPERATURE, diff --git a/scripts/ci/contextual_orchestrator_review_sidecar.sh b/scripts/ci/contextual_orchestrator_review_sidecar.sh index e4984f643b..0ab2ae66d2 100755 --- a/scripts/ci/contextual_orchestrator_review_sidecar.sh +++ b/scripts/ci/contextual_orchestrator_review_sidecar.sh @@ -246,7 +246,7 @@ publish_sidecar_evidence() { # Optional authoritative ZDR route feed. Failure is non-fatal: the policy falls # back to the dated static attestation table in scripts/ci/zdr_policy.py. -if curl -fsSL --max-time 15 "https://openrouter.ai/api/v1/endpoints/zdr" -o "$zdr_feed" 2>/dev/null; then +if curl -fsSL "https://openrouter.ai/api/v1/endpoints/zdr" -o "$zdr_feed" 2>/dev/null; then log "using live OpenRouter ZDR endpoint feed" zdr_args=(--zdr-endpoints "$zdr_feed") else @@ -330,7 +330,7 @@ cleanup_sidecar_on_error() { trap cleanup_sidecar_on_error EXIT i=0 -until curl -fsSL --max-time 2 "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/healthz" >/dev/null 2>&1; do +until curl -fsSL "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/healthz" >/dev/null 2>&1; do if ! kill -0 "$sidecar_pid" 2>/dev/null; then sidecar_status=0 wait "$sidecar_pid" || sidecar_status=$? @@ -355,18 +355,6 @@ until curl -fsSL --max-time 2 "http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/ fail "sidecar exited before healthz (status ${sidecar_status}); stderr: $(sed -n '1,20p' "$sidecar_stderr")" fi i=$((i + 1)) - # KNOWN GAP, tracked as ContextualWisdomLab/.github#1455 (not yet fixed): - # this 180s covers the launcher's ENTIRE startup sequence -- discovery, - # catalog build, AND preflight probing -- not just probing. Layer 1's own - # "160s worst case" comment - # (contextual_orchestrator_review_launcher.py's REVIEW_PREFLIGHT_MAX_ESCALATIONS) - # accounts only for probing; discover_all_models() runs first, inside this - # same 180s, and can itself take up to ~105s worst case (verified against - # the vendored contextual_orchestrator.model_discovery source: ~7 - # sequential HTTP calls at up to 15s each). - if [ "$i" -ge 180 ]; then - fail "sidecar did not become healthy; stderr: $(sed -n '1,20p' "$sidecar_stderr")" - fi sleep 1 done if [ ! -s "$preflight_report" ]; then @@ -421,25 +409,13 @@ gateway_virtual_model="orchestrator/${orchestrator_pool}" # ContextualWisdomLab/contextual-orchestrator#912 run 33304076516). printf '{"model":"%s","messages":[{"role":"system","content":"You are a helpful assistant."},{"role":"user","content":"Reply with just '\''OK'\''."}],"temperature":1.0,"max_tokens":4096,"stream":false}\n' \ "$gateway_virtual_model" > "$gateway_preflight_request" -# 30s (this check's previous bound) is too tight for a real completion from a -# reasoning-capable free-tier model: exact-evidence reproduction (Strix run -# 33306775025 on ContextualWisdomLab/contextual-orchestrator#921, job -# 99244624298) shows the routing probe marking a DeepSeek NIM route "ready" -# in 18s, then this identical request against that same healthy route being -# cut off by curl's own timeout at exactly 30.0s -- "gateway preflight -# request could not reach the local sidecar" is this curl failure, not an -# actual connectivity problem. This required-workflow job already budgets -# 120 minutes (see timeout-minutes in strix.yml/noema-review.yml), and the -# org's own stated policy accepts multi-hour central review latency in -# favor of accuracy over speed -- a 30s bound on one preflight self-check -# contradicted that policy and rejected a route the routing probe had just -# proven healthy. 120s keeps this a bounded, fail-closed check while giving -# a real reasoning generation room to finish. This value is deliberately kept -# unchanged by ADR-0005 -- shortening it would regress the fix just described. +# This completion is model inference, so ADR-0003 forbids a wall-clock timeout. +# A slow reasoning model may legitimately take hours after routing proves it +# healthy; transport failures still fail closed through curl's exit status. # # ADR-0005 Trigger A: this request goes to the virtual pool, not one pinned # candidate, so a transport failure or non-2xx status here (unreachable -# process, timeout, upstream error) is retried with a fresh attempt at the +# process, upstream error) is retried with a fresh attempt at the # SAME budget, up to REVIEW_PREFLIGHT_GATEWAY_MAX_ATTEMPTS total attempts -- # a same-budget retry may or may not land on a different underlying candidate # (route diversity here is a best-effort hope, not a verified guarantee: the @@ -481,7 +457,7 @@ gateway_attempt=1 gateway_http_status="" while :; do if gateway_http_status="$( - curl -sS --max-time 120 \ + curl -sS \ -o "$gateway_preflight_response" \ -w '%{http_code}' \ -X POST \ diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index d7ef15a2e0..249f94f6b7 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -33,7 +33,6 @@ 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"}) @@ -106,6 +105,7 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: title body isDraft + state headRefOid reviewDecision reviewThreads(first: 100) { @@ -167,6 +167,21 @@ def fetch_pr(repo: str, number: int) -> dict[str, Any]: return pr +def require_expected_head(pr: dict[str, Any], expected_head_sha: str) -> None: + """Fail closed unless the pull request is open at the expected commit.""" + if not re.fullmatch(r"[0-9a-fA-F]{40}", expected_head_sha): + raise RuntimeError("Expected pull request head must be a full commit SHA") + live_head_sha = str(pr.get("headRefOid") or "") + if ( + str(pr.get("state") or "").upper() != "OPEN" + or live_head_sha.lower() != expected_head_sha.lower() + ): + raise RuntimeError( + "Pull request is closed or its head changed before Noema review: " + f"expected {expected_head_sha}, observed {live_head_sha or ''}" + ) + + def review_author(review: dict[str, Any]) -> str: """Return the normalized author login from a review node.""" return ((review.get("author") or {}).get("login") or "").strip() @@ -225,7 +240,19 @@ def fetch_diff(repo: str, number: int) -> tuple[str, bool]: diff = run(["gh", "api", f"repos/{repo}/pulls/{number}", "-H", "Accept: application/vnd.github.v3.diff"]) truncated = len(diff) > MAX_DIFF_CHARS if truncated: - diff = diff[:MAX_DIFF_CHARS] + marker = "[overlong changed line content omitted]" + bounded = diff[: MAX_DIFF_CHARS - len(marker) - 2] + complete, separator, partial = bounded.rpartition("\n") + if not separator: + return diff[:MAX_DIFF_CHARS], truncated + last_hunk = max(complete.rfind("\n@@"), 0 if complete.startswith("@@") else -1) + last_file = max(complete.rfind("\ndiff --git "), 0 if complete.startswith("diff --git ") else -1) + inside_hunk = last_hunk > last_file + if partial.startswith(("+", "-")) and ( + inside_hunk or not partial.startswith(("+++", "---")) + ): + complete += f"\n{partial[0]}{marker}" + diff = complete return diff, truncated @@ -903,7 +930,7 @@ def call_llm( 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 + second, potentially multi-hour model 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 @@ -916,6 +943,16 @@ def call_llm( raise RuntimeError("Noema LLM review unavailable: NOEMA_LLM_API_URL or NOEMA_LLM_API_KEY is not configured.") reject_private_llm_url(api_url) + allowed_locations = [ + {"path": path, "line": line, "side": side} + for path, line, side in sorted(changed_diff_locations(diff)) + ] + location_example = ( + allowed_locations[0] + if allowed_locations + else {"path": "path", "line": 0, "side": "RIGHT"} + ) + prompt = { "role": "user", "content": "\n".join( @@ -923,7 +960,36 @@ def call_llm( "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", "Review the PR diff plus the additional changed-file, review-thread, and CodeGraph context for correctness, security, maintainability, and behavioral regressions.", "Return only JSON with this shape:", - '{"decision":"approve|request_changes|comment","summary":"...","reviewed_lines":[{"path":"path","line":1,"side":"RIGHT|LEFT","analysis":"..."}],"adversarial_validation":{"status":"passed|failed","residual_risk":"...","probes":[{"path":"path","line":1,"side":"RIGHT|LEFT","hypothesis":"...","attack_or_counterexample":"...","evidence":"observed or source-traced result","outcome":"falsified|confirmed"}]},"findings":[{"severity":"high|medium|low","file":"path","line":1,"side":"RIGHT|LEFT","message":"..."}]}', + json.dumps( + { + "decision": "approve|request_changes|comment", + "summary": "...", + "reviewed_lines": [{**location_example, "analysis": "..."}], + "adversarial_validation": { + "status": "passed|failed", + "residual_risk": "...", + "probes": [ + { + **location_example, + "hypothesis": "...", + "attack_or_counterexample": "...", + "evidence": "observed or source-traced result", + "outcome": "falsified|confirmed", + } + ], + }, + "findings": [ + { + "severity": "high|medium|low", + "file": location_example["path"], + "line": location_example["line"], + "side": location_example["side"], + "message": "...", + } + ], + }, + separators=(",", ":"), + ), "Every formal verdict must cite exact changed-side lines. APPROVE requires falsifying concrete regression hypotheses; source or test changes require at least two distinct probes and other changes require at least one. REQUEST_CHANGES requires a confirmed probe at a finding location.", "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", *( @@ -964,7 +1030,7 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request, timeout=NOEMA_LLM_TIMEOUT_SECONDS) as response: # nosec B310 + with opener.open(request) as response: # nosec B310 raw_bytes = response.read() try: raw = decode_llm_response_body(raw_bytes) @@ -1106,8 +1172,10 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: """ 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.") + try: + require_expected_head(pr, expected_head) + except RuntimeError: + print("Pull request is closed or its trigger head is stale; Noema review skipped before model work.") return 0 actor = current_actor() if not actor: @@ -1132,8 +1200,10 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: 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.") + try: + require_expected_head(current_pr, expected_head) + except RuntimeError: + print("Pull request closed or its head changed during review; stale verdict was not published.") return 0 submit_review(repo, number, current_pr, actor, verdict) return 0 diff --git a/scripts/ci/opencode_review_receipt_gate.py b/scripts/ci/opencode_review_receipt_gate.py index fa1026f14d..4dcb24af88 100644 --- a/scripts/ci/opencode_review_receipt_gate.py +++ b/scripts/ci/opencode_review_receipt_gate.py @@ -38,6 +38,14 @@ "OpenCode reviewed the current-head product diff", "OpenCode reviewed the current-head bounded evidence", ) +FALLBACK_APPROVAL_MARKERS = ( + "deterministic current-head evidence", + "deterministic fallback approval", + "model-unavailable evidence fallback", + "did not emit a usable current-head control block", + "scope: `unsupported`", + "model-pool outcome: `unknown`", +) MENTION_RE = re.compile(r"^@opencode-agent\b", re.IGNORECASE) AFIPC_230_HEAD = "5eda857066c9207786d3bdde49826f8f94b98c12" @@ -122,6 +130,10 @@ def is_formal_receipt( body = str(review.get("body") or "") if is_mention_or_malformed(body): return False, "mention, status-only, or malformed payload is not a formal review" + if state == "APPROVED" and any( + marker in body.casefold() for marker in FALLBACK_APPROVAL_MARKERS + ): + return False, "fallback approval is not a substantive formal review" if is_draft and state == "APPROVED": return False, "draft must never receive bot APPROVE" return True, "current-head formal review" @@ -149,6 +161,8 @@ def evaluate_receipts( return review, reason if "never receive bot APPROVE" in reason: return None, reason + if "fallback approval" in reason: + return None, reason if reason.startswith("stale"): stale_hits += 1 continue @@ -179,6 +193,7 @@ def fetch_reviews(repo: str, number: int) -> list[Mapping[str, Any]]: "api", f"repos/{repo}/pulls/{number}/reviews", "--paginate", + "--slurp", ], text=True, stdout=subprocess.PIPE, @@ -190,8 +205,12 @@ def fetch_reviews(repo: str, number: int) -> list[Mapping[str, Any]]: detail = (completed.stderr or completed.stdout or "gh reviews lookup failed").strip() raise ReceiptGateError(f"formal review receipt lookup failed: {detail}") loaded = json.loads(completed.stdout or "[]") - if isinstance(loaded, list): - return [item for item in loaded if isinstance(item, Mapping)] + if ( + isinstance(loaded, list) + and all(isinstance(page, list) for page in loaded) + and all(isinstance(item, Mapping) for page in loaded for item in page) + ): + return [item for page in loaded for item in page] raise ReceiptGateError("formal review receipt lookup returned malformed JSON") diff --git a/scripts/ci/pr_review_fix_scheduler.py b/scripts/ci/pr_review_fix_scheduler.py index 5d13f68108..2e283d0d0a 100755 --- a/scripts/ci/pr_review_fix_scheduler.py +++ b/scripts/ci/pr_review_fix_scheduler.py @@ -17,6 +17,7 @@ complete_paginated_pr_contexts, fetch_open_prs, fetch_pr, + force_cancel_workflow_runs, context_nodes, has_current_head_approval, has_current_head_changes_requested, @@ -32,6 +33,7 @@ complete_paginated_pr_contexts, fetch_open_prs, fetch_pr, + force_cancel_workflow_runs, context_nodes, has_current_head_approval, has_current_head_changes_requested, @@ -54,6 +56,11 @@ ) REPO_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") REPAIR_MODES = frozenset({"review", "rca", "conflict"}) +AUTOFIX_RUN_NAME_RE = re.compile( + r"^PR Review Autofix (?P[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)" + r"#(?P[1-9][0-9]*)@(?P[0-9a-fA-F]{40})$" +) +ACTIVE_RUN_STATUSES = frozenset({"queued", "in_progress", "pending", "requested", "waiting"}) NON_AUTOFIX_CHANGE_REQUEST_MARKERS = ( "merge conflict", "mergestatestatus `dirty`", @@ -104,6 +111,20 @@ def run_json(args: list[str]) -> Any: return json.loads(run(["gh", *args]) or "null") +def live_head_matches(repo: str, pr: dict[str, Any]) -> bool: + """Return whether GitHub still reports the scheduler's exact PR head.""" + payload = run_json(["api", f"repos/{repo}/pulls/{int(pr['number'])}"]) + if not isinstance(payload, dict) or not isinstance(payload.get("head"), dict): + return False + live_head = payload["head"].get("sha") + expected_head = str(pr.get("headRefOid") or "") + return ( + isinstance(live_head, str) + and len(live_head) == 40 + and live_head.lower() == expected_head.lower() + ) + + RATE_LIMIT_ERROR_MARKERS = ("api rate limit exceeded", "secondary rate limit") ISSUE_COMMENTS_RETRY_ATTEMPTS = 2 ISSUE_COMMENTS_RETRY_BACKOFF_SECONDS = 15 @@ -385,9 +406,66 @@ def dispatch_autofix( if dry_run: print("DRY-RUN:", " ".join(args), json.dumps(payload, sort_keys=True)) return + if not live_head_matches(repo, pr): + raise RuntimeError("pull request live head changed before autofix dispatch") run(args, stdin=json.dumps(payload)) +def prepare_autofix_slot( + repo: str, + pr: dict[str, Any], + *, + workflow: str, + workflow_repository: str, + dry_run: bool, +) -> bool | None: + """Cancel older-head workers; return ``None`` when this PR snapshot went stale.""" + dispatch_repo = workflow_repository or repo + payload = run_json( + [ + "api", + f"repos/{dispatch_repo}/actions/workflows/{workflow}/runs", + "-X", + "GET", + "-f", + "event=repository_dispatch", + "-f", + "per_page=100", + "--paginate", + "--slurp", + ] + ) + number = int(pr["number"]) + head = str(pr["headRefOid"]).lower() + same_head = False + stale_ids: list[str] = [] + pages = payload if isinstance(payload, list) else [payload] + for workflow_run in ( + workflow_run + for page in pages + for workflow_run in page.get("workflow_runs", []) + ): + if str(workflow_run.get("status") or "") not in ACTIVE_RUN_STATUSES: + continue + match = AUTOFIX_RUN_NAME_RE.fullmatch( + str(workflow_run.get("display_title") or "") + ) + if not match or match.group("repo") != repo or int(match.group("pr")) != number: + continue + if match.group("head").lower() == head: + same_head = True + else: + stale_ids.append(str(workflow_run["id"])) + if stale_ids: + if dry_run: + print(f"DRY-RUN: would force-cancel stale autofix runs {', '.join(stale_ids)}") + elif not live_head_matches(repo, pr): + return None + else: + force_cancel_workflow_runs(dispatch_repo, stale_ids) + return same_head + + def _base_branch_matches(pr: dict[str, Any], expected: str) -> bool: """Return whether a PR belongs to the configured base scope.""" return expected == "*" or pr.get("baseRefName") == expected @@ -455,6 +533,18 @@ def inspect_pr( ): return "wait", ("recent autofix marker exists for this head",) + slot_state = prepare_autofix_slot( + repo, + pr, + workflow=args.autofix_workflow, + workflow_repository=args.autofix_repository, + dry_run=args.dry_run, + ) + if slot_state is None: + return "wait", ("scheduler PR snapshot is stale; retry with the current live head",) + if slot_state: + return "wait", ("current-head autofix run is already queued or running",) + dispatch_kwargs: dict[str, Any] = { "workflow": args.autofix_workflow, "workflow_repository": args.autofix_repository, diff --git a/scripts/ci/run_opencode_review_model_pool.sh b/scripts/ci/run_opencode_review_model_pool.sh index 986982e9af..80f57d1d43 100644 --- a/scripts/ci/run_opencode_review_model_pool.sh +++ b/scripts/ci/run_opencode_review_model_pool.sh @@ -89,38 +89,6 @@ env_integer_or_default() { fi } -cap_dynamic_cadence_for_queue() { - local timeout_cap budget_cap cycle_cap previous_run_timeout previous_budget_seconds previous_max_cycles - - timeout_cap="$(env_integer_or_default OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS 3600)" - budget_cap="$(env_integer_or_default OPENCODE_DYNAMIC_TOTAL_BUDGET_CAP_SECONDS 7200)" - cycle_cap="$(env_integer_or_default OPENCODE_DYNAMIC_MAX_CYCLES_CAP 0)" - previous_run_timeout="$original_run_timeout" - previous_budget_seconds="$budget_seconds" - previous_max_cycles="$max_cycles" - - if [ "$timeout_cap" -gt 0 ] && [ "$original_run_timeout" -gt "$timeout_cap" ]; then - original_run_timeout="$timeout_cap" - fi - if [ "$budget_cap" -gt 0 ] && [ "$budget_seconds" -gt "$budget_cap" ]; then - budget_seconds="$budget_cap" - fi - if [ "$cycle_cap" -gt 0 ]; then - if [ "$max_cycles" -eq 0 ] || [ "$max_cycles" -gt "$cycle_cap" ]; then - max_cycles="$cycle_cap" - fi - fi - - if [ "$original_run_timeout" != "$previous_run_timeout" ] || - [ "$budget_seconds" != "$previous_budget_seconds" ] || - [ "$max_cycles" != "$previous_max_cycles" ]; then - printf 'OpenCode dynamic review cadence queue cap applied: per-attempt %ss -> %ss, total budget %ss -> %ss, max-cycles %s -> %s; set OPENCODE_DYNAMIC_*_CAP_SECONDS or OPENCODE_DYNAMIC_MAX_CYCLES_CAP to 0 to disable a specific queue cap.\n' \ - "$previous_run_timeout" "$original_run_timeout" \ - "$previous_budget_seconds" "$budget_seconds" \ - "$previous_max_cycles" "$max_cycles" - fi -} - count_changed_files_for_cadence() { local changed_files_file="${OPENCODE_CHANGED_FILES_FILE:-}" @@ -419,33 +387,6 @@ should_skip_model_candidate() { return 1 } -cap_model_run_timeout() { - local model_candidate="$1" - local run_timeout_seconds="$2" - local cap_seconds - - case "$model_candidate" in - nvidia-nim/*) - cap_seconds="$(env_integer_or_default OPENCODE_NVIDIA_NIM_RUN_TIMEOUT_SECONDS 180)" - ;; - opencode-free/*) - cap_seconds="$(env_integer_or_default OPENCODE_FREE_RUN_TIMEOUT_SECONDS 3600)" - ;; - github-models/openai/gpt-5 | github-models/openai/gpt-5-chat) - cap_seconds="$(env_integer_or_default OPENCODE_GITHUB_GPT5_RUN_TIMEOUT_SECONDS 45)" - ;; - *) - printf '%s\n' "$run_timeout_seconds" - return 0 - ;; - esac - if [ "$cap_seconds" -gt 0 ] && [ "$run_timeout_seconds" -gt "$cap_seconds" ]; then - printf '%s\n' "$cap_seconds" - else - printf '%s\n' "$run_timeout_seconds" - fi -} - run_one_model_attempt() { local model_candidate="$1" local attempt="$2" @@ -455,41 +396,41 @@ run_one_model_attempt() { local candidate_output_file="$6" local opencode_json_file="$7" local opencode_export_file="$8" - local run_timeout_seconds export_timeout_seconds opencode_status session_id opencode_stderr_file - local opencode_pid fatal_poll_seconds + local export_timeout_seconds opencode_status session_id opencode_stderr_file + local opencode_pid fatal_kill_grace_seconds fatal_poll_seconds - run_timeout_seconds="${OPENCODE_RUN_TIMEOUT_SECONDS:-3600}" export_timeout_seconds="${OPENCODE_EXPORT_TIMEOUT_SECONDS:-120}" fatal_poll_seconds="${OPENCODE_FATAL_ERROR_POLL_SECONDS:-5}" + fatal_kill_grace_seconds="${OPENCODE_FATAL_KILL_GRACE_SECONDS:-5}" opencode_stderr_file="${opencode_json_file}.stderr" rm -f "$opencode_json_file" "$opencode_stderr_file" "$opencode_export_file" "$candidate_output_file" set +e - timeout --kill-after=30s "${run_timeout_seconds}s" \ - env -u GH_TOKEN -u GITHUB_TOKEN -u OPENCODE_APP_TOKEN \ + env -u GH_TOKEN -u GITHUB_TOKEN -u OPENCODE_APP_TOKEN \ -u ACTIONS_ID_TOKEN_REQUEST_TOKEN -u ACTIONS_ID_TOKEN_REQUEST_URL \ + python3 -c 'import os, sys; os.setsid(); os.execvp(sys.argv[1], sys.argv[1:])' \ opencode run "$(cat "$prompt_file")" \ --pure \ --agent "$agent" \ --model "$model_candidate" \ --format json \ - --title "PR #${PR_NUMBER} OpenCode bounded review ${model_candidate} attempt ${attempt}/${attempts}" \ + --title "PR #${PR_NUMBER} OpenCode review ${model_candidate} attempt ${attempt}/${attempts}" \ >"$opencode_json_file" 2>"$opencode_stderr_file" & opencode_pid=$! # Some providers (github-models ContextOverflowError) log a fatal error and - # then hang instead of exiting, burning the whole run timeout. Watch the JSON + # then hang instead of exiting. Watch the JSON # log while opencode runs and kill the process early so the pool falls # through to the next candidate within seconds instead of minutes. while kill -0 "$opencode_pid" 2>/dev/null; do if has_fatal_provider_error_event "$opencode_json_file"; then - printf 'OpenCode %s attempt %s/%s logged a fatal provider error while still running; killing the hung process instead of waiting out the %ss run timeout.\n' \ - "$model_candidate" "$attempt" "$attempts" "$run_timeout_seconds" - kill "$opencode_pid" 2>/dev/null - for _ in $(seq 1 30); do - kill -0 "$opencode_pid" 2>/dev/null || break + printf 'OpenCode %s attempt %s/%s logged a fatal provider error while still running; cancelling that failed process.\n' \ + "$model_candidate" "$attempt" "$attempts" + kill -TERM -- "-$opencode_pid" 2>/dev/null + for _ in $(seq 1 "$fatal_kill_grace_seconds"); do + kill -0 -- "-$opencode_pid" 2>/dev/null || break sleep 1 done - kill -9 "$opencode_pid" 2>/dev/null + kill -KILL -- "-$opencode_pid" 2>/dev/null break fi sleep "$fatal_poll_seconds" @@ -500,9 +441,6 @@ run_one_model_attempt() { if [ "$opencode_status" -ne 0 ]; then printf 'OpenCode %s attempt %s/%s failed with exit %s.\n' "$model_candidate" "$attempt" "$attempts" "$opencode_status" emit_sanitized_opencode_failure_detail "$opencode_json_file" "$opencode_stderr_file" - if [ "$opencode_status" -eq 124 ] || [ "$opencode_status" -eq 137 ]; then - printf 'OpenCode %s attempt %s/%s timed out after %ss; falling through within the remaining retry budget instead of blocking the org queue.\n' "$model_candidate" "$attempt" "$attempts" "$run_timeout_seconds" - fi if is_fatal_provider_failure "$opencode_json_file"; then printf 'OpenCode %s attempt %s/%s hit a fatal provider error (context window, token budget, quota, or model unavailable); skipping remaining attempts for this model.\n' "$model_candidate" "$attempt" "$attempts" return 2 @@ -542,13 +480,9 @@ run_one_model_attempt() { } main() { - local attempts schema_repair_attempts effective_attempts budget_seconds deadline now remaining model_candidate attempt safe_model prompt_file candidate_output_file - local opencode_json_file opencode_export_file agent retry_sleep original_run_timeout run_status cycle_sleep cycle max_cycles - local uncapped_run_timeout - local changed_file_count small_file_threshold medium_file_threshold + local attempts schema_repair_attempts effective_attempts model_candidate attempt safe_model prompt_file candidate_output_file + local opencode_json_file opencode_export_file agent retry_sleep run_status cycle_sleep cycle max_cycles local invalid_control_cap max_total_attempts total_attempts alive_candidates - local nim_budget_seconds nim_elapsed_seconds nim_remaining_seconds - local nim_attempt_started nim_attempt_elapsed non_nim_candidate_count local -A dead_candidate_reasons invalid_control_counts local -a model_candidates @@ -556,52 +490,20 @@ main() { # control-rejected output or has exhausted provider credits must stop # consuming paid requests instead of cycling until the retry budget # elapses (run 30120972549 burned the org OpenRouter credit in ~102 - # cycles of re-sent full prompts). Timeouts/deadlines are untouched. + # cycles of re-sent full prompts). These are request-count guards, not clocks. invalid_control_cap="$(env_integer_or_default OPENCODE_INVALID_CONTROL_OUTPUT_CAP 3)" max_total_attempts="$(env_integer_or_default OPENCODE_POOL_MAX_TOTAL_ATTEMPTS 30)" total_attempts=0 attempts="${OPENCODE_MODEL_ATTEMPTS:-3}" schema_repair_attempts="$(env_integer_or_default OPENCODE_SCHEMA_REPAIR_ATTEMPTS 1)" - original_run_timeout="${OPENCODE_RUN_TIMEOUT_SECONDS:-3600}" - budget_seconds="${OPENCODE_TOTAL_RETRY_BUDGET_SECONDS:-1500}" max_cycles="${OPENCODE_POOL_MAX_CYCLES:-0}" if [ "${CENTRAL_REVIEW_PROCESS_FALLBACK_ELIGIBLE:-false}" = "true" ]; then - original_run_timeout="${OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS:-3600}" - budget_seconds="${OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_TOTAL_BUDGET_SECONDS:-3600}" max_cycles="${OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_MAX_CYCLES:-1}" - printf 'Central review-process evidence fallback eligible for scope "%s"; limiting OpenCode model pool to %ss per attempt, %ss total budget, and %s cycle(s) so provider delay is logged before the publish fallback evaluates current-head peer evidence.\n' \ - "${CENTRAL_REVIEW_PROCESS_FALLBACK_SCOPE_LABEL:-unsupported}" "$original_run_timeout" "$budget_seconds" "$max_cycles" + printf 'Central review-process evidence fallback eligible for scope "%s"; limiting OpenCode model pool by cycle count only.\n' \ + "${CENTRAL_REVIEW_PROCESS_FALLBACK_SCOPE_LABEL:-unsupported}" elif [ "${OPENCODE_DYNAMIC_REVIEW_CADENCE:-false}" = "true" ]; then - small_file_threshold="$(env_integer_or_default OPENCODE_SMALL_CHANGE_FILE_THRESHOLD 3)" - medium_file_threshold="$(env_integer_or_default OPENCODE_MEDIUM_CHANGE_FILE_THRESHOLD 20)" - if changed_file_count="$(count_changed_files_for_cadence)"; then - if [ "$changed_file_count" -le "$small_file_threshold" ]; then - original_run_timeout="$(env_integer_or_default OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS 900)" - budget_seconds="$(env_integer_or_default OPENCODE_SMALL_CHANGE_TOTAL_BUDGET_SECONDS 2100)" - elif [ "$changed_file_count" -le "$medium_file_threshold" ]; then - original_run_timeout="$(env_integer_or_default OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS 3600)" - budget_seconds="$(env_integer_or_default OPENCODE_MEDIUM_CHANGE_TOTAL_BUDGET_SECONDS 3900)" - else - original_run_timeout="$(env_integer_or_default OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS 3600)" - budget_seconds="$(env_integer_or_default OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS 7200)" - fi - max_cycles="$(env_integer_or_default OPENCODE_DYNAMIC_MAX_CYCLES 0)" - cap_dynamic_cadence_for_queue - printf 'OpenCode dynamic review cadence selected %ss per attempt and %ss total budget for %s changed file(s); max-cycles=%s.\n' \ - "$original_run_timeout" "$budget_seconds" "$changed_file_count" "$max_cycles" - else - original_run_timeout="$(env_integer_or_default OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS 3600)" - budget_seconds="$(env_integer_or_default OPENCODE_UNKNOWN_CHANGE_TOTAL_BUDGET_SECONDS 3900)" - max_cycles="$(env_integer_or_default OPENCODE_DYNAMIC_MAX_CYCLES 0)" - cap_dynamic_cadence_for_queue - printf 'OpenCode dynamic review cadence could not read OPENCODE_CHANGED_FILES_FILE; using %ss per attempt and %ss total budget; max-cycles=%s.\n' \ - "$original_run_timeout" "$budget_seconds" "$max_cycles" - fi - fi - deadline=0 - if [ "$budget_seconds" -gt 0 ]; then - deadline=$((SECONDS + budget_seconds)) + max_cycles="$(env_integer_or_default OPENCODE_DYNAMIC_MAX_CYCLES 0)" fi : >"$OPENCODE_OUTPUT_FILE" cd "$OPENCODE_REVIEW_WORKDIR" @@ -613,23 +515,8 @@ main() { fi exit 1 fi - nim_budget_seconds="$(env_integer_or_default OPENCODE_NVIDIA_NIM_TOTAL_BUDGET_SECONDS 900)" - nim_elapsed_seconds=0 - non_nim_candidate_count=0 - for model_candidate in "${model_candidates[@]}"; do - if ! is_nvidia_nim_candidate "$model_candidate"; then - non_nim_candidate_count=$((non_nim_candidate_count + 1)) - fi - done - if [ "$non_nim_candidate_count" -gt 0 ] && - [ "$budget_seconds" -gt 0 ] && - [ "$nim_budget_seconds" -ge "$budget_seconds" ]; then - nim_budget_seconds=$((budget_seconds / 2)) - printf 'OpenCode NVIDIA NIM combined runtime budget was capped at %ss so %s non-NIM fallback candidate(s) retain retry budget.\n' \ - "$nim_budget_seconds" "$non_nim_candidate_count" - fi - printf 'Configured OpenCode model pool: candidates=%s attempts=%s per-model-timeout=%ss retry-budget=%ss max-cycles=%s NVIDIA-NIM-combined-budget=%ss.\n' \ - "${#model_candidates[@]}" "$attempts" "$original_run_timeout" "$budget_seconds" "$max_cycles" "$nim_budget_seconds" + printf 'Configured OpenCode model pool: candidates=%s attempts=%s max-cycles=%s; model inference has no wall-clock timeout.\n' \ + "${#model_candidates[@]}" "$attempts" "$max_cycles" cycle=1 while :; do @@ -643,12 +530,6 @@ main() { if should_skip_model_candidate "$model_candidate"; then continue fi - if is_nvidia_nim_candidate "$model_candidate" && - [ "$nim_elapsed_seconds" -ge "$nim_budget_seconds" ]; then - printf 'Skipping OpenCode %s because the NVIDIA NIM combined runtime budget of %ss is exhausted; preserving the remaining retry budget for fallback candidates.\n' \ - "$model_candidate" "$nim_budget_seconds" - continue - fi assert_reasoning_effort_for_candidate "$model_candidate" safe_model="${model_candidate//[\/:]/-}" prompt_file="${RUNNER_TEMP}/opencode-review-${safe_model}-prompt.md" @@ -666,20 +547,6 @@ main() { printf 'OpenCode %s schema-repair attempt %s/%s will re-review from trusted evidence with a non-replayable control checklist.\n' \ "$model_candidate" "$attempt" "$effective_attempts" fi - now="$SECONDS" - if is_nvidia_nim_candidate "$model_candidate" && - [ "$nim_elapsed_seconds" -ge "$nim_budget_seconds" ]; then - printf 'Stopping OpenCode %s retries because the NVIDIA NIM combined runtime budget of %ss is exhausted.\n' \ - "$model_candidate" "$nim_budget_seconds" - break - fi - if [ "$deadline" -gt 0 ] && [ "$now" -ge "$deadline" ]; then - printf 'OpenCode model pool retry deadline elapsed before %s attempt %s/%s.\n' "$model_candidate" "$attempt" "$effective_attempts" - if finish_pool_without_model; then - exit 0 - fi - exit 1 - fi if [ "$max_total_attempts" -gt 0 ] && [ "$total_attempts" -ge "$max_total_attempts" ]; then printf 'OpenCode model pool reached the per-run provider attempt ceiling of %s attempts; ending the pool to bound provider spend. Set OPENCODE_POOL_MAX_TOTAL_ATTEMPTS=0 to disable.\n' "$max_total_attempts" if finish_pool_without_model; then @@ -688,36 +555,12 @@ main() { exit 1 fi total_attempts=$((total_attempts + 1)) - remaining="$original_run_timeout" - if [ "$deadline" -gt 0 ]; then - remaining=$((deadline - now)) - fi - OPENCODE_RUN_TIMEOUT_SECONDS="$original_run_timeout" - if [ "$deadline" -gt 0 ] && [ "$OPENCODE_RUN_TIMEOUT_SECONDS" -gt "$remaining" ]; then - OPENCODE_RUN_TIMEOUT_SECONDS="$remaining" - fi - if is_nvidia_nim_candidate "$model_candidate"; then - nim_remaining_seconds=$((nim_budget_seconds - nim_elapsed_seconds)) - if [ "$OPENCODE_RUN_TIMEOUT_SECONDS" -gt "$nim_remaining_seconds" ]; then - printf 'OpenCode %s combined NVIDIA NIM budget cap selected %ss instead of %ss so fallback candidates retain retry budget.\n' \ - "$model_candidate" "$nim_remaining_seconds" "$OPENCODE_RUN_TIMEOUT_SECONDS" - OPENCODE_RUN_TIMEOUT_SECONDS="$nim_remaining_seconds" - fi - fi - uncapped_run_timeout="$OPENCODE_RUN_TIMEOUT_SECONDS" - OPENCODE_RUN_TIMEOUT_SECONDS="$(cap_model_run_timeout "$model_candidate" "$OPENCODE_RUN_TIMEOUT_SECONDS")" - if [ "$OPENCODE_RUN_TIMEOUT_SECONDS" -lt "$uncapped_run_timeout" ]; then - printf 'OpenCode %s runtime cap selected %ss instead of %ss because this provider has a bounded failover window.\n' \ - "$model_candidate" "$OPENCODE_RUN_TIMEOUT_SECONDS" "$uncapped_run_timeout" - fi - export OPENCODE_RUN_TIMEOUT_SECONDS - printf 'OpenCode %s attempt %s/%s using %ss run timeout with %ss retry budget remaining.\n' "$model_candidate" "$attempt" "$effective_attempts" "$OPENCODE_RUN_TIMEOUT_SECONDS" "$remaining" + printf 'OpenCode %s attempt %s/%s has no model inference timeout.\n' "$model_candidate" "$attempt" "$effective_attempts" agent="${OPENCODE_AGENT:-ci-review-fallback}" if [ "$attempt" -eq 1 ] && [ -n "${OPENCODE_FIRST_ATTEMPT_AGENT:-}" ]; then agent="$OPENCODE_FIRST_ATTEMPT_AGENT" fi run_status=0 - nim_attempt_started="$SECONDS" if run_one_model_attempt "$model_candidate" "$attempt" "$effective_attempts" "$agent" "$prompt_file" "$candidate_output_file" "$opencode_json_file" "$opencode_export_file"; then cp "$candidate_output_file" "$OPENCODE_OUTPUT_FILE" record_review_model "$model_candidate" @@ -726,12 +569,6 @@ main() { else run_status=$? fi - if is_nvidia_nim_candidate "$model_candidate"; then - nim_attempt_elapsed=$((SECONDS - nim_attempt_started)) - nim_elapsed_seconds=$((nim_elapsed_seconds + nim_attempt_elapsed)) - printf 'OpenCode NVIDIA NIM combined runtime used %ss/%ss after %s attempt %s/%s.\n' \ - "$nim_elapsed_seconds" "$nim_budget_seconds" "$model_candidate" "$attempt" "$effective_attempts" - fi if [ "$run_status" -ne 3 ] && is_credit_exhausted_failure "$opencode_json_file" "${opencode_json_file}.stderr"; then dead_candidate_reasons[$model_candidate]="provider credits exhausted (HTTP 402 / payment required)" printf 'OpenCode %s provider credits are exhausted; marking this candidate failed for the rest of the run so retries cannot accrue further spend.\n' "$model_candidate" @@ -754,9 +591,6 @@ main() { fi if [ "$attempt" -lt "$effective_attempts" ] && [ "$attempt" -lt "$attempts" ]; then retry_sleep="$(backoff_sleep "$attempt")" - if [ "$deadline" -gt 0 ] && [ $((SECONDS + retry_sleep)) -gt "$deadline" ]; then - retry_sleep=$((deadline - SECONDS)) - fi if [ "$retry_sleep" -gt 0 ]; then printf 'Retrying OpenCode after exponential backoff of %ss.\n' "$retry_sleep" sleep "$retry_sleep" @@ -779,7 +613,7 @@ main() { exit 1 fi - printf 'OpenCode completed a full model-candidate cycle without a valid control conclusion; continuing until a model succeeds or the retry budget/GitHub Actions job timeout is reached.\n' + printf 'OpenCode completed a full model-candidate cycle without a valid control conclusion.\n' if [ "$max_cycles" -gt 0 ] && [ "$cycle" -ge "$max_cycles" ]; then printf 'OpenCode model pool reached configured max cycle count %s without a valid control conclusion.\n' "$max_cycles" if finish_pool_without_model; then @@ -787,18 +621,7 @@ main() { fi exit 1 fi - printf 'OpenCode retry budget and the workflow step timeout remain the outer guards for invalid or unavailable provider output.\n' cycle_sleep="${OPENCODE_POOL_CYCLE_SLEEP_SECONDS:-60}" - if [ "$deadline" -gt 0 ] && [ $((SECONDS + cycle_sleep)) -gt "$deadline" ]; then - cycle_sleep=$((deadline - SECONDS)) - if [ "$cycle_sleep" -le 0 ]; then - printf 'OpenCode model pool retry deadline elapsed after cycle %s.\n' "$cycle" - if finish_pool_without_model; then - exit 0 - fi - exit 1 - fi - fi printf 'Restarting OpenCode model pool after %ss.\n' "$cycle_sleep" sleep "$cycle_sleep" cycle=$((cycle + 1)) diff --git a/scripts/ci/strix_quick_gate.sh b/scripts/ci/strix_quick_gate.sh index cd94796243..3a563d7020 100755 --- a/scripts/ci/strix_quick_gate.sh +++ b/scripts/ci/strix_quick_gate.sh @@ -31,7 +31,7 @@ ATTEMPT_LOGS_DIR="$STRIX_RUNTIME_DIR/gate-attempts" STRIX_SCAN_WORKING_DIR="$STRIX_RUNTIME_DIR/scan-cwd" STRIX_SCAN_OUTPUT_DIR="$STRIX_SCAN_WORKING_DIR/strix_runs" STRIX_REPORTS_DIR="$ACTIVE_REPORTS_DIR" -STRIX_PROCESS_TIMEOUT_SECONDS="${STRIX_PROCESS_TIMEOUT_SECONDS:-1200}" +STRIX_PROCESS_TIMEOUT_SECONDS="${STRIX_PROCESS_TIMEOUT_SECONDS:-0}" STRIX_TOTAL_TIMEOUT_SECONDS="${STRIX_TOTAL_TIMEOUT_SECONDS:-0}" STRIX_DISABLE_PR_SCOPING="${STRIX_DISABLE_PR_SCOPING:-1}" # shellcheck disable=SC2034 # consumed by sourced normalize_model helper diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index d08c2cdd9e..9b58be0fbe 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -200,7 +200,8 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_contains "$workflow_file" "branches: [main, develop, master]" "strix workflow scans GitHub Flow and Git Flow protected branches" assert_file_contains "$workflow_file" "pull_request_target:" "strix workflow uses trusted PR trigger" assert_file_contains "$workflow_file" "group: >-" "strix workflow defines an explicit concurrency group" - assert_file_contains "$workflow_file" "format('closed-pr-{0}-{1}', github.event.pull_request.base.repo.full_name, github.event.pull_request.number)" "strix workflow gives closed PR cleanup an independent concurrency group" + assert_file_contains "$workflow_file" "cancel-superseded-pr-runs:" "strix workflow runs superseded-head cleanup outside the provider scan queue" + assert_file_not_contains "$workflow_file" "format('closed-pr-{0}-{1}'" "strix cleanup does not need a second concurrency queue" assert_file_contains "$workflow_file" "format('{0}-{1}', github.event_name, github.event.client_payload.target_repository ||" "strix workflow scopes active evidence per repository and event class" assert_file_contains "$workflow_file" "format('{0}-{1}-{2}', github.event_name, github.repository, github.ref)" "strix workflow keeps protected-branch push evidence in ref-specific queues" assert_file_contains "$workflow_file" "github.event.client_payload.target_repository ||" "strix manual dispatch concurrency scopes to the target repository when provided" @@ -210,7 +211,7 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_not_contains "$workflow_file" "format('pr-{0}-{1}'" "strix workflow does not keep stale head-specific concurrency groups" assert_file_contains "$workflow_file" "cancel-in-progress: false" "strix workflow does not cancel an in-progress provider scan" assert_file_not_contains "$workflow_file" "queue: max" "strix workflow uses only supported GitHub concurrency keys" - assert_file_contains "$workflow_file" "default-branch repository_dispatch evidence cannot cancel" "strix workflow documents manual evidence isolation from branch protection contexts" + assert_file_contains "$workflow_file" "format('{0}-{1}', github.event_name," "strix workflow isolates repository_dispatch evidence from pull-request evidence" assert_file_contains "$workflow_file" "re-dispatches exact-head evidence" "strix workflow documents current-head queue recovery" assert_file_contains "$workflow_file" "refs/pull//head has already advanced before this queued run starts" "strix workflow documents stale scan queue avoidance" status_token_count="$(grep -c '^[[:space:]]*GITHUB_STATUS_TOKEN:' "$workflow_file")" @@ -296,11 +297,12 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_contains "$workflow_file" "Provision contextual-orchestrator Strix sidecar" "strix workflow provisions the central contextual-orchestrator sidecar" assert_file_contains "$workflow_file" "CONTEXTUAL_ORCHESTRATOR_BASE_URL" "strix workflow uses the sidecar base URL" assert_file_contains "$workflow_file" "CONTEXTUAL_ORCHESTRATOR_TOKEN" "strix workflow uses the sidecar token" - assert_file_contains "$workflow_file" "timeout-minutes: 200" "strix workflow job budget preserves multi-hour scans and artifact publication margin" - assert_file_contains "$workflow_file" "timeout-minutes: 170" "strix workflow scan step permits legitimate 150-minute repository reviews" - assert_file_contains "$workflow_file" 'budget_suffix="TIME""OUT"' "strix workflow builds budget env keys without visible timeout signal text" - assert_file_contains "$workflow_file" 'export "STRIX_TOTAL_${budget_suffix}_SECONDS=9300"' "strix workflow preserves a 155-minute bounded total Strix budget" - assert_file_contains "$workflow_file" 'process_budget_seconds="9000"' "strix workflow gives a legitimate scan up to 150 minutes" + assert_file_not_contains "$workflow_file" "timeout-minutes: 200" "strix workflow job must not cap model inference" + assert_file_not_contains "$workflow_file" "timeout-minutes: 170" "strix scan step must not cap model inference" + assert_file_contains "$workflow_file" 'export LLM_TIMEOUT=0' "strix disables the model client inference timeout" + assert_file_contains "$workflow_file" 'export STRIX_MEMORY_COMPRESSOR_TIMEOUT=0' "strix disables the memory-compressor inference timeout" + assert_file_contains "$workflow_file" 'export STRIX_PROCESS_TIMEOUT_SECONDS=0' "strix disables the scanner process timeout" + assert_file_contains "$workflow_file" 'export STRIX_TOTAL_TIMEOUT_SECONDS=0' "strix disables the total scanner timeout" assert_file_contains "$workflow_file" 'Error code:[[:space:]]*500[^[:cntrl:]]*internal_error' "strix workflow retries contextual-orchestrator internal provider failures" assert_file_contains "$workflow_file" 'strix_gate_console.log" "$GITHUB_WORKSPACE/strix_runs/gate-console.log' "strix workflow preserves partial console output after failures and timeouts" assert_file_contains "$REPO_ROOT/scripts/ci/strix_quick_gate.sh" "gate-last-attempt.log" "strix gate preserves the last partial attempt before runtime cleanup" @@ -754,7 +756,7 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_contains "$workflow_file" "Never return raw tool-call markup" "opencode review prompt forbids raw tool-call transcripts as final review output" assert_file_contains "$workflow_file" "Do not spend the session listing every changed path before reviewing" "opencode review prompt prevents fallback sessions from exhausting steps on file listing" assert_file_contains "$workflow_file" "Always return a final control block instead of a progress summary" "opencode review prompt requires a gate conclusion instead of a progress summary" - assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'timeout --kill-after=30s "${run_timeout_seconds}s"' "opencode review model pool has a kill-after bounded timeout" + assert_file_not_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'timeout --kill-after=30s "${run_timeout_seconds}s"' "opencode review model pool must not cap inference" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'env -u GH_TOKEN -u GITHUB_TOKEN -u OPENCODE_APP_TOKEN' "opencode review model pool scrubs GitHub credentials before model execution" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "assert_reasoning_effort_for_candidate" "opencode review validates high reasoning effort before running capable model candidates" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "assert_opencode_reasoning_effort.py" "opencode review reuses the central reasoning effort guard" @@ -773,22 +775,19 @@ 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: 305' "opencode review target contains evidence, the bounded long-review pool, publication, Noema handoff, and cleanup overhead" + assert_file_not_contains "$workflow_file" 'timeout-minutes: 325' "opencode review target must not cap inference" 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_not_contains "$workflow_file" 'timeout-minutes: 205' "opencode model pool step must not cap inference" 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: "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_not_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS:' "opencode primary review has no inference timeout" + assert_file_not_contains "$workflow_file" 'OPENCODE_FREE_RUN_TIMEOUT_SECONDS:' "opencode free-tier review has no inference timeout" 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" -assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_RUN_TIMEOUT_SECONDS:-3600' "opencode pool defaults primary run timeout to hour-class (~3600s) for large repos" -assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS 3600' "opencode pool dynamic timeout cap defaults to hour-class (~3600s)" -assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_FREE_RUN_TIMEOUT_SECONDS 3600' "opencode free-tier failover timeout is hour-class (~3600s)" -assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_NVIDIA_NIM_RUN_TIMEOUT_SECONDS 180' "opencode NVIDIA NIM candidate runtime cap defaults to three minutes" -assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_NVIDIA_NIM_TOTAL_BUDGET_SECONDS 900' "opencode NVIDIA NIM combined runtime cap defaults to fifteen minutes" + assert_file_not_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'timeout --kill-after=30s' "opencode pool has no inference kill timer" + assert_file_not_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_NVIDIA_NIM_TOTAL_BUDGET_SECONDS' "opencode NVIDIA NIM inference has no combined runtime cap" - assert_file_contains "$workflow_file" 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "11700"' "opencode model pool exits before the step timeout so the approval gate can publish a reason" + assert_file_not_contains "$workflow_file" 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS:' "opencode model pool has no wall-clock retry budget" assert_file_contains "$workflow_file" 'OPENCODE_POOL_MAX_CYCLES: "1"' "opencode model pool exhausts each candidate only once before bounded fallback" assert_file_not_contains "$workflow_file" 'opencode-exhausted-retry:' "opencode model exhaustion retries stay owned by the least-privilege central scheduler" assert_file_not_contains "$workflow_file" 'RETRY_DISPATCH_TOKEN' "opencode does not retain a recursive write-token dispatch path" @@ -874,8 +873,8 @@ assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" ' assert_file_contains "$workflow_file" 'load_selected_review_output()' "opencode approval step has a direct selected-output fallback when the overview comment is stale or invalid" assert_file_contains "$workflow_file" "gate result from Review Overview comment" "opencode approval step distinguishes overview-comment gate results" assert_file_contains "$workflow_file" "gate result from selected OpenCode output" "opencode approval step can recover from an invalid overview by validating the selected successful output" - assert_file_contains "$workflow_file" 'timeout-minutes: 36' "opencode approval step has a bounded wall-clock timeout that covers dynamically extended image and package/GPU checks" - assert_file_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS: "120"' "opencode publish-stage diagnosis is a short best-effort augmentation" + assert_file_not_contains "$workflow_file" 'timeout-minutes: 36' "opencode approval step must not cap model diagnosis" + assert_file_not_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS:' "opencode publish-stage diagnosis has no inference timeout" assert_file_not_contains "$workflow_file" "rekick_model_pool_on_exhaustion" "opencode publication must not rerun the exhausted model catalog after the model-pool step" assert_file_contains "$workflow_file" "publish stage performs no duplicate model-catalog pass" "opencode publication logs that exhausted model retries are delegated to the scheduler" assert_file_contains "$workflow_file" 'timeout --kill-after=15s "${OPENCODE_EXPORT_TIMEOUT_SECONDS:-120}s"' "opencode failed-check diagnosis bounds export so the publication gate cannot hang silently" @@ -946,11 +945,11 @@ assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" ' assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "OPENROUTER_API_KEY is not configured" "opencode model pool skips OpenRouter candidates when the org secret is absent" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "scoped NVIDIA_NIM_API_KEY is not configured" "opencode model pool skips NVIDIA NIM candidates when the scoped credential is absent" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "configured max cycle count" "opencode model pool exits before the job timeout after configured cycles" - assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS:-1500' "opencode model pool keeps a bounded default retry budget unless the workflow explicitly disables it" + assert_file_not_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS' "opencode model pool has no wall-clock retry budget" 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: "11700"' "opencode catalog fallback uses the full pool review budget" + assert_file_not_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS:' "opencode catalog fallback permits arbitrarily slow provider sessions" 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" @@ -1319,7 +1318,7 @@ assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" ' assert_file_contains "$workflow_file" "collect_failed_check_evidence.sh" "opencode review workflow collects failed check logs and annotations" assert_file_contains "$workflow_file" 'HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }}' "opencode evidence step passes the live validated HEAD_SHA to failed-check evidence collection" assert_file_contains "$workflow_file" "FAILED_CHECK_EVIDENCE_ATTEMPTS" "opencode review workflow bounds waiting for peer check failures before model review" - assert_file_contains "$workflow_file" 'timeout-minutes: 205' "opencode model stage has a bounded long-review multi-provider timeout" + assert_file_not_contains "$workflow_file" 'timeout-minutes: 205' "opencode model stage has no inference timeout" assert_file_contains "$workflow_file" 'timeout-minutes: 12' "opencode evidence preparation has a bounded peer-check wait timeout" assert_file_contains "$workflow_file" 'FAILED_CHECK_EVIDENCE_ATTEMPTS: "6"' "opencode review workflow keeps pre-model peer-check waiting bounded for required workflow DX" assert_file_contains "$workflow_file" 'FAILED_CHECK_EVIDENCE_SLEEP_SECONDS: "5"' "opencode review workflow retries peer-check evidence without stalling the model stage for Strix-scale durations" diff --git a/tests/test_contextual_orchestrator_review_runtime_preflight.py b/tests/test_contextual_orchestrator_review_runtime_preflight.py index 7093e8a3d0..559c2d1e99 100644 --- a/tests/test_contextual_orchestrator_review_runtime_preflight.py +++ b/tests/test_contextual_orchestrator_review_runtime_preflight.py @@ -430,8 +430,8 @@ def test_gateway_preflight_max_tokens_is_synchronized_with_the_routing_probe() - ) -def test_gateway_preflight_curl_timeout_tolerates_real_reasoning_latency() -> None: - """The end-to-end gateway check's curl timeout must not undercut real completion latency. +def test_gateway_preflight_has_no_inference_timeout() -> None: + """The end-to-end gateway check must not cap real completion latency. Regression for the 2026-08-30 gateway-preflight-timeout incident: exact- evidence reproduction (Strix run 33306775025 on @@ -440,22 +440,51 @@ def test_gateway_preflight_curl_timeout_tolerates_real_reasoning_latency() -> No identical gateway request against that same healthy route being cut off at exactly curl's configured bound -- "gateway preflight request could not reach the local sidecar" was that timeout, not a real connectivity - failure. This asserts the bound is generous enough to tolerate a real - reasoning generation (well above the routing probe's own 10s - per-candidate budget) rather than the previous 30s, which rejected a - route the routing probe had just proven healthy. + failure. The request therefore has no wall-clock bound. """ sidecar = _SIDECAR.read_text(encoding="utf-8") - match = re.search(r"curl -sS --max-time (\d+) \\\n\s*-o \"\$gateway_preflight_response\"", sidecar) - assert match, "sidecar must send the gateway preflight request with an explicit curl --max-time" - gateway_preflight_timeout_seconds = int(match.group(1)) + request_block = sidecar.rsplit("curl -sS", 1)[1].split( + '"http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/v1/chat/completions"', 1 + )[0] + assert "--max-time" not in request_block - assert gateway_preflight_timeout_seconds >= 120, ( - "gateway preflight curl --max-time " - f"({gateway_preflight_timeout_seconds}s) must tolerate real reasoning-model " - "completion latency; 30s was observed cutting off a route the routing probe " - "had just proven ready" + +def test_sidecar_discovery_and_health_have_no_wall_clock_timeout() -> None: + sidecar = _SIDECAR.read_text(encoding="utf-8") + + lines = sidecar.splitlines() + + def curl_command(url: str) -> tuple[str, int]: + index = next(index for index, line in enumerate(lines) if url in line) + start = index + while start and lines[start - 1].rstrip().endswith("\\"): + start -= 1 + end = index + while lines[end].rstrip().endswith("\\"): + end += 1 + command = " ".join(line.strip().removesuffix("\\") for line in lines[start : end + 1]) + assert re.search(r"\bcurl\b", command) + return command, end + + timeout_option = re.compile( + r"(?:^|\s)(?:-m(?:\s|$)|--[a-z-]*(?:time|timeout)[a-z-]*(?:=|\s|$))" + ) + zdr_command, _ = curl_command("https://openrouter.ai/api/v1/endpoints/zdr") + health_command, health_command_end = curl_command( + 'http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/healthz' + ) + for command in (zdr_command, health_command): + assert timeout_option.search(command) is None + assert re.search(r"(?:^|\s)timeout(?:\s|$)", command) is None + + health_loop = "\n".join(lines[health_command_end + 1 :]).split("\ndone", 1)[0] + assert 'kill -0 "$sidecar_pid"' in health_loop + assert health_loop.count("fail ") == 1 + assert health_loop.index('kill -0 "$sidecar_pid"') < health_loop.index("fail ") + assert not re.search( + r"\b(?:break|exit|timeout)\b|\s-(?:ge|gt|le|lt)\s|\bif\s+\(\(", + health_loop, ) @@ -1402,7 +1431,6 @@ def test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case namespace = _load_launcher() preflight = namespace["_preflight_with_fallback"] max_escalations = namespace["REVIEW_PREFLIGHT_MAX_ESCALATIONS"] - timeout_seconds = namespace["REVIEW_PREFLIGHT_TIMEOUT_SECONDS"] primary_limit = namespace["REVIEW_PREFLIGHT_PRIMARY_ROUTE_LIMIT"] total_route_limit = namespace["REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES"] fallback_limit = total_route_limit - primary_limit @@ -1430,12 +1458,6 @@ def test_fallback_escalation_budget_is_shared_with_primary_and_bounds_worst_case assert report["primary_attempt"]["escalations_used"] == max_escalations total_attempts = len(client.calls) - worst_case_seconds = total_attempts * timeout_seconds - assert worst_case_seconds <= 160, ( - f"worst-case preflight time ({worst_case_seconds}s across " - f"{total_attempts} attempts) must stay within the 160s the ADR " - "computes and the 180s healthz-readiness watchdog allows" - ) # Exactly the ADR's own worst-case arithmetic: 12 base attempts (one per # candidate across both stages) + 4 escalations (the shared cap) = 16. assert total_attempts == total_route_limit + max_escalations @@ -1593,14 +1615,13 @@ def failing_loader(value: str) -> list[object]: assert not path.exists() -def test_preflight_transport_is_bounded_and_provider_neutral() -> None: - """Sequential route probes must fit inside the sidecar startup budget.""" +def test_preflight_transport_has_no_inference_timeout_and_is_provider_neutral() -> None: launcher = _LAUNCHER.read_text(encoding="utf-8") assert "REVIEW_MAX_OUTPUT_TOKENS = 4096" in launcher assert "REVIEW_TEMPERATURE = 1.0" in launcher - assert "REVIEW_PREFLIGHT_TIMEOUT_SECONDS = 10" in launcher - assert "timeout=REVIEW_PREFLIGHT_TIMEOUT_SECONDS" in launcher + assert "REVIEW_PREFLIGHT_TIMEOUT_SECONDS" not in launcher + assert "ModelClient(\n timeout=" not in launcher assert "max_retries=0" in launcher assert "temperature=REVIEW_TEMPERATURE" in launcher diff --git a/tests/test_contextual_orchestrator_sidecar_unbounded_wait_contract.py b/tests/test_contextual_orchestrator_sidecar_unbounded_wait_contract.py new file mode 100644 index 0000000000..fca11c1577 --- /dev/null +++ b/tests/test_contextual_orchestrator_sidecar_unbounded_wait_contract.py @@ -0,0 +1,73 @@ +"""Semantic regression tests for the review-sidecar unbounded-wait contract.""" + +from __future__ import annotations + +from pathlib import Path +import re + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_SIDECAR = _REPO_ROOT / "scripts/ci/contextual_orchestrator_review_sidecar.sh" +_CURL_TIMEOUT_OPTION = re.compile( + r"(?:--connect-timeout(?:=|\s)|--max-time(?:=|\s)|(?:^|\s)-m(?:=|\s|[0-9]))", + re.MULTILINE, +) +_FINITE_WAIT_GUARD = re.compile( + r"(?:\b(?:attempt|attempts|retry|retries|poll|polls|deadline|timeout|elapsed|i)\b[^\n]*" + r"(?:-ge|-gt|>=|>|-le|-lt|<=|<))", + re.IGNORECASE, +) + + +def _shell_command_containing(script: str, marker: str) -> str: + """Return the shell command that contains ``marker``, including continuations.""" + lines = script.splitlines() + marker_index = next(index for index, line in enumerate(lines) if marker in line) + start = marker_index + while start > 0 and lines[start - 1].rstrip().endswith("\\"): + start -= 1 + while start > 0 and "curl " not in lines[start] and "curl" not in lines[start]: + start -= 1 + end = marker_index + while end < len(lines) - 1 and lines[end].rstrip().endswith("\\"): + end += 1 + command = "\n".join(lines[start : end + 1]) + assert "curl" in command + return command + + +def _health_poll_block(script: str) -> str: + """Return the complete healthz polling loop, from ``until`` through ``done``.""" + match = re.search( + r"(?ms)^until curl[^\n]*?/healthz[^\n]*; do\n(?P.*?)^done$", + script, + ) + assert match is not None, "sidecar must retain an explicit healthz polling loop" + return match.group(0) + + +def test_discovery_and_health_curl_commands_have_no_wall_clock_timeout_options() -> None: + """Every discovery/health curl must remain free of finite curl timeout options.""" + script = _SIDECAR.read_text(encoding="utf-8") + commands = ( + _shell_command_containing(script, "https://openrouter.ai/api/v1/endpoints/zdr"), + _shell_command_containing( + script, + 'http://${ORCHESTRATOR_HOST}:${ORCHESTRATOR_PORT}/healthz', + ), + ) + + for command in commands: + assert _CURL_TIMEOUT_OPTION.search(command) is None, command + + +def test_health_polling_has_no_attempt_or_elapsed_deadline() -> None: + """Health polling may fail on sidecar exit, but never on a local time/attempt budget.""" + script = _SIDECAR.read_text(encoding="utf-8") + block = _health_poll_block(script) + + assert _FINITE_WAIT_GUARD.search(block) is None, block + assert "SIDECAR_READINESS_TIMEOUT" not in block + assert "READINESS_DEADLINE" not in block + assert "timeout_seconds" not in block.casefold() + assert 'kill -0 "$sidecar_pid"' in block + assert 'fail "sidecar exited before healthz' in block diff --git a/tests/test_github_hourly_conflict_repair.py b/tests/test_github_hourly_conflict_repair.py index 7d98837eb2..b4b8e5d6af 100644 --- a/tests/test_github_hourly_conflict_repair.py +++ b/tests/test_github_hourly_conflict_repair.py @@ -60,6 +60,11 @@ def capture_dispatch(_repo: str, _pr: dict[str, Any], **kwargs: Any) -> None: captured.update(kwargs) monkeypatch.setattr(scheduler, "dispatch_autofix", capture_dispatch) + monkeypatch.setattr( + scheduler, + "prepare_autofix_slot", + lambda *_args, **_kwargs: False, + ) monkeypatch.setattr( scheduler, "create_fix_marker", diff --git a/tests/test_noema_orchestrator_workflow_contract.py b/tests/test_noema_orchestrator_workflow_contract.py index 3b9c5baeef..5355a8ca89 100644 --- a/tests/test_noema_orchestrator_workflow_contract.py +++ b/tests/test_noema_orchestrator_workflow_contract.py @@ -209,52 +209,19 @@ def _expected_head_from_workflow_run_event(event: dict) -> str: ) -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. - """ +def test_standalone_noema_expected_head_uses_trusted_trigger_context() -> None: + """Standalone Noema binds review work to the PR or dispatch head.""" 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 || '' }}" + "EXPECTED_HEAD_SHA: ${{ github.event.pull_request.head.sha || " + "github.event.client_payload.pr_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 + assert "github.event.workflow_run" not in workflow 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. - """ + """The retired workflow_run trigger cannot fabricate Noema review context.""" + assert "workflow_run:" not in workflow_text("noema-review.yml") workflow_run_event = {"workflow_run": {"head_sha": "c" * 40, "pull_requests": []}} assert _expected_head_from_workflow_run_event(workflow_run_event) == "" @@ -281,7 +248,7 @@ def _run_stale_trigger_step( "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", "TARGET_REPOSITORY": "ContextualWisdomLab/example", "PR_NUMBER": "7", - "EXPECTED_HEAD": expected_head, + "EXPECTED_HEAD_SHA": expected_head, "GH_TOKEN": "synthetic-token", } return subprocess.run( # noqa: S603, S607 diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5229605627..6272ff2b59 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -62,16 +62,10 @@ def test_noema_concurrency_and_live_head_cleanup_preserve_current_review(): """ 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 "github.event.workflow_run" not in concurrency + assert "github.event.action == 'synchronize'" in concurrency + assert "github.event.action == 'closed'" in concurrency + assert "cancel-in-progress: true" not 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" @@ -98,7 +92,7 @@ def test_noema_concurrency_and_live_head_cleanup_preserve_current_review(): in cleanup ) assert "could not re-verify the live PR head before cancelling" in cleanup - assert '"${live_head,,}" != "${EXPECTED_HEAD,,}"' in cleanup + assert '"${live_head,,}" != "${EXPECTED_HEAD_SHA,,}"' in cleanup assert 'endswith("@" + $head)' in cleanup assert "| not)" in cleanup @@ -123,7 +117,7 @@ def test_noema_superseded_cleanup_selects_only_other_heads_of_same_pr(): 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_marker = '--arg target "$TARGET_REPOSITORY" --arg head "$EXPECTED_HEAD_SHA" \'\n' start = workflow.index(start_marker) + len(start_marker) end = workflow.index('\n \' <<<"$runs_json"', start) selector = workflow[start:end] @@ -145,9 +139,9 @@ def test_noema_superseded_cleanup_selects_only_other_heads_of_same_pr(): ) 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 "EXPECTED_HEAD_SHA:" in workflow + assert "--expected-head \"$EXPECTED_HEAD_SHA\"" in workflow + assert '"${live_head,,}" != "${EXPECTED_HEAD_SHA,,}"' in workflow assert workflow.index("Reject a stale trigger before credential or model setup") < workflow.index( "Select fail-closed Noema reviewer credential" ) @@ -171,7 +165,7 @@ def test_noema_superseded_cleanup_matches_a_sibling_run_by_pull_requests_array() 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_marker = '--arg target "$TARGET_REPOSITORY" --arg head "$EXPECTED_HEAD_SHA" \'\n' start = workflow.index(start_marker) + len(start_marker) end = workflow.index('\n \' <<<"$runs_json"', start) selector = workflow[start:end] @@ -344,7 +338,7 @@ def test_superseded_cleanup_preserves_current_and_newer_run_ids(tmp_path: Path) """#!/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 [[ "$*" == *"/pulls/7"* ]]; then printf '%s\n' "$EXPECTED_HEAD_SHA"; exit 0; fi if [[ "$*" == *"actions/runs?status="* ]]; then cat "$FAKE_RUNS"; exit 0; fi """, encoding="utf-8", @@ -354,7 +348,7 @@ def test_superseded_cleanup_preserves_current_and_newer_run_ids(tmp_path: Path) [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", + "EXPECTED_HEAD_SHA": current_head, "CURRENT_RUN_ID": "200", "FAKE_RUNS": str(fixture), "FAKE_CALLS": str(calls)}, capture_output=True, text=True, check=False, ) @@ -411,7 +405,7 @@ def test_superseded_cleanup_survives_a_transient_live_head_lookup_failure( [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", + "EXPECTED_HEAD_SHA": current_head, "CURRENT_RUN_ID": "200", "FAKE_RUNS": str(fixture), "FAKE_CALLS": str(calls)}, capture_output=True, text=True, check=False, ) @@ -613,6 +607,7 @@ def make_pr(**overrides): "title": "Noema", "body": "", "isDraft": False, + "state": "OPEN", "headRefOid": "head", "reviews": {"nodes": []}, "reviewThreads": {"nodes": []}, @@ -708,6 +703,20 @@ def test_existing_noema_review_matches_actor_and_head(): assert not noema.existing_noema_review(make_pr(reviews={"nodes": [review(commit="old", login="noema")]}), "noema") +def test_require_expected_head_rejects_invalid_closed_and_stale_targets(): + head = "a" * 40 + noema.require_expected_head(make_pr(headRefOid=head), head) + noema.require_expected_head(make_pr(headRefOid=head), head.upper()) + with pytest.raises(RuntimeError, match="closed or its head changed"): + noema.require_expected_head(make_pr(headRefOid=head, state=None), head) + with pytest.raises(RuntimeError, match="full commit SHA"): + noema.require_expected_head(make_pr(headRefOid=head), "short") + with pytest.raises(RuntimeError, match="closed or its head changed"): + noema.require_expected_head(make_pr(headRefOid=head, state="CLOSED"), head) + with pytest.raises(RuntimeError, match="closed or its head changed"): + noema.require_expected_head(make_pr(headRefOid="b" * 40), head) + + def test_current_actor_fetch_diff_and_json_extraction(monkeypatch): monkeypatch.setenv("NOEMA_REVIEW_ACTOR", "cwl-noema-review[bot]") monkeypatch.setenv("NOEMA_REVIEW_INSTALLATION_ID", "123") @@ -731,10 +740,26 @@ def app_identity(args, **kwargs): monkeypatch.setattr(noema, "run", app_identity) assert noema.current_actor() == "cwl-noema-review[bot]" - monkeypatch.setattr(noema, "run", lambda *args, **kwargs: "x" * (noema.MAX_DIFF_CHARS + 5)) + source = "complete\n" + "x" * (noema.MAX_DIFF_CHARS + 5) + monkeypatch.setattr(noema, "run", lambda *args, **kwargs: source) + diff, truncated = noema.fetch_diff("owner/repo", 1) + assert truncated + assert diff == "complete" + + source = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+" + "x" * noema.MAX_DIFF_CHARS + monkeypatch.setattr(noema, "run", lambda *args, **kwargs: source) diff, truncated = noema.fetch_diff("owner/repo", 1) assert truncated - assert len(diff) == noema.MAX_DIFF_CHARS + assert diff.endswith("+[overlong changed line content omitted]") + assert ("a.py", 1, "RIGHT") in noema.changed_diff_locations(diff) + assert len(diff) <= noema.MAX_DIFF_CHARS + + source = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+++" + "x" * noema.MAX_DIFF_CHARS + monkeypatch.setattr(noema, "run", lambda *args, **kwargs: source) + diff, truncated = noema.fetch_diff("owner/repo", 1) + assert truncated + assert diff.endswith("+[overlong changed line content omitted]") + assert ("a.py", 1, "RIGHT") in noema.changed_diff_locations(diff) assert noema.extract_json_object('{"decision":"approve"}') == {"decision": "approve"} assert noema.extract_json_object('prefix {"decision":"comment"} suffix') == {"decision": "comment"} @@ -1140,7 +1165,7 @@ def test_call_llm_skips_repair_retry_when_head_moves_before_it_fires(monkeypatch 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 + potentially multi-hour model 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 @@ -1219,7 +1244,8 @@ def test_inspect_and_review_reports_stale_before_repair_retry_cleanly(monkeypatc """``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() + head = "a" * 40 + pr = make_pr(headRefOid=head) 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)) @@ -1238,7 +1264,7 @@ def fake_call_llm(*args, **kwargs): lambda *args, **kwargs: pytest.fail("stale-during-repair verdict must not publish"), ) - assert noema.inspect_and_review("owner/repo", 7, "head") == 0 + assert noema.inspect_and_review("owner/repo", 7, head) == 0 def test_call_llm_fails_closed_after_repeated_malformed_envelope(monkeypatch): @@ -1507,7 +1533,7 @@ def open(self, request, timeout=None): 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["timeout"] is None assert seen["body"]["model"] == "review-model" assert "extra review context" in seen["body"]["messages"][1]["content"] @@ -1659,7 +1685,8 @@ def test_format_findings_and_submit_review(monkeypatch): def test_inspect_and_review_skip_paths(monkeypatch): - clean_pr = make_pr() + head = "a" * 40 + clean_pr = make_pr(headRefOid=head) calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") @@ -1669,32 +1696,34 @@ 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, "head") == 0 + assert noema.inspect_and_review("owner/repo", 7, head) == 0 assert calls cases = [ - (make_pr(isDraft=True), "noema"), - (make_pr(reviews={"nodes": [review(login="noema", body="")]}), "noema"), + (make_pr(headRefOid=head, isDraft=True), "noema"), + (make_pr(headRefOid=head, reviews={"nodes": [review(commit=head, login="noema", body="")]}), "noema"), ] for pr, actor in cases: 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, "head") == 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, "head") + 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, "head") + noema.inspect_and_review("owner/repo", 7, head) def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatch): + head = "a" * 40 pr = make_pr( + headRefOid=head, reviews={"nodes": [review("CHANGES_REQUESTED")]}, reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}, statusCheckRollup={"contexts": {"nodes": [{"__typename": "StatusContext", "context": "ci", "state": "FAILURE"}]}}, @@ -1708,18 +1737,18 @@ 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, "head") == 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, "fetch_pr", lambda repo, number: make_pr(headRefOid="b" * 40)) 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 + assert noema.inspect_and_review("owner/repo", 7, "a" * 40) == 0 def test_expected_head_comparison_is_case_insensitive(monkeypatch): @@ -1735,7 +1764,10 @@ def test_expected_head_comparison_is_case_insensitive(monkeypatch): def test_head_movement_stops_before_review_publication(monkeypatch): - pull_requests = iter((make_pr(), make_pr(headRefOid="new"))) + head = "a" * 40 + pull_requests = iter( + (make_pr(headRefOid=head), make_pr(headRefOid="b" * 40)) + ) 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)) @@ -1751,12 +1783,33 @@ def test_head_movement_stops_before_review_publication(monkeypatch): "submit_review", lambda *args, **kwargs: pytest.fail("stale verdict must not publish"), ) - assert noema.inspect_and_review("owner/repo", 7, "head") == 0 + assert noema.inspect_and_review("owner/repo", 7, head) == 0 + + +def test_closed_during_model_stops_before_review_publication(monkeypatch): + head = "a" * 40 + pull_requests = iter( + (make_pr(headRefOid=head), make_pr(headRefOid=head, state="CLOSED")) + ) + 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"}) + monkeypatch.setattr( + noema, + "submit_review", + lambda *args, **kwargs: pytest.fail("closed PR 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") + head = "abc123def0" * 4 + pr = make_pr(headRefOid=head) 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)) @@ -1766,13 +1819,14 @@ def test_uppercase_expected_head_is_not_stale_before_model_work(monkeypatch): calls = [] monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) - assert noema.inspect_and_review("owner/repo", 7, "ABC123DEF0") == 0 + assert noema.inspect_and_review("owner/repo", 7, head.upper()) == 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"))) + head = "abc123def0" * 4 + pull_requests = iter((make_pr(headRefOid=head), make_pr(headRefOid=head))) 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)) @@ -1786,10 +1840,27 @@ def test_uppercase_expected_head_is_not_stale_before_publication(monkeypatch): calls = [] monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) - assert noema.inspect_and_review("owner/repo", 7, "ABC123DEF0") == 0 + assert noema.inspect_and_review("owner/repo", 7, head.upper()) == 0 assert calls +def test_inspect_and_review_rechecks_head_before_publication(monkeypatch): + head = "a" * 40 + stale = make_pr(headRefOid="b" * 40) + responses = iter([make_pr(headRefOid=head), stale]) + submitted = [] + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(responses)) + 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"}) + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: submitted.append(args)) + + assert noema.inspect_and_review("owner/repo", 7, head) == 0 + assert submitted == [] + + def test_call_llm_rejects_empty_review_content(monkeypatch): monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") @@ -2024,8 +2095,8 @@ def read(self): ).encode() class Opener: - def open(self, request, timeout): - assert timeout == noema.NOEMA_LLM_TIMEOUT_SECONDS + def open(self, request, timeout=None): + assert timeout is None payloads.append(json.loads(request.data)) return Response(invalid if len(payloads) == 1 else valid) @@ -2035,6 +2106,19 @@ def open(self, request, timeout): 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"] + assert ( + '"reviewed_lines":[{"path":"tool.py","line":1,"side":"LEFT"' + in payloads[1]["messages"][1]["content"] + ) + + +def test_noema_adr_forbids_fixed_model_inference_timeouts() -> None: + """Long-running reasoning must not be misclassified as provider failure.""" + adr = Path("docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md").read_text() + normalized = " ".join(adr.split()) + + assert "MUST NOT impose a fixed wall-clock timeout on model inference" in normalized + assert "initial completion ping" in normalized def test_substantive_approve_requires_exact_changed_lines_and_falsified_probes(): diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 35409b42bb..2854e0671f 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1814,11 +1814,8 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "is_context_overflow_failure" in model_pool_runner assert "tokens_limit_reached" in model_pool_runner assert "skipping remaining attempts for this model" in model_pool_runner - assert "using %ss run timeout with %ss retry budget remaining" in model_pool_runner - assert ( - "timed out after %ss; falling through within the remaining retry budget" - in model_pool_runner - ) + assert "has no model inference timeout" in model_pool_runner + assert "timed out after %ss" not in model_pool_runner assert "emit_sanitized_opencode_failure_detail" in model_pool_runner assert "OpenCode provider failure metadata" in model_pool_runner assert "provider-controlled content suppressed" in model_pool_runner @@ -1896,20 +1893,11 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "Install central adversarial harness runtime" not in workflow 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: "11700"' - in workflow - ) - assert ( - 'OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_TOTAL_BUDGET_SECONDS: "11700"' - in workflow - ) + assert "OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS" not in workflow + assert "OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_TOTAL_BUDGET_SECONDS" not in workflow assert 'OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_MAX_CYCLES: "1"' in workflow assert "Central review-process evidence fallback eligible" in model_pool_runner - assert ( - "provider delay is logged before the publish fallback evaluates current-head peer evidence" - in model_pool_runner - ) + assert "limiting OpenCode model pool by cycle count only" in model_pool_runner assert "model pool was intentionally skipped" not in workflow assert ( "current-head deterministic central review-process evidence is clean" @@ -1975,23 +1963,15 @@ 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: 305", workflow) + assert not re.search(r"opencode-review-target:[\s\S]{0,4000}?timeout-minutes: 325", workflow) assert "timeout-minutes: 12" in workflow - assert re.search( + assert not re.search( r"Run OpenCode PR Review model pool[\s\S]{0,240}timeout-minutes: 205", workflow ) - assert 'OPENCODE_SMALL_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow - 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: "11700"' in workflow - assert 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "11700"' in workflow - assert 'OPENCODE_POOL_STEP_TIMEOUT_SECONDS: "12000"' in workflow - assert ( - 'timeout --kill-after=30s "${OPENCODE_POOL_STEP_TIMEOUT_SECONDS:-3600}s"' - in workflow - ) - assert "OpenCode model pool exceeded the outer" in workflow + assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "5400"' not in workflow + assert 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "11700"' not in workflow + assert 'OPENCODE_POOL_STEP_TIMEOUT_SECONDS: "12000"' not in workflow + assert "OpenCode model pool exceeded the outer" not in workflow assert 'OPENCODE_POOL_MAX_CYCLES: "1"' in workflow assert re.search( r"Run OpenCode PR Review model pool[\s\S]{0,280}continue-on-error: true", @@ -2001,7 +1981,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): r"Publish central OpenCode fast approval[\s\S]{0,900}timeout-minutes: 34", workflow, ) - assert re.search( + assert not re.search( r"Publish OpenCode review outcome[\s\S]{0,900}timeout-minutes: 36", workflow ) assert workflow.count('APPROVAL_CHECK_WAIT_ATTEMPTS: "36"') == 2 @@ -2013,35 +1993,21 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): workflow.count("current-head package/GPU build checks are still running") == 2 ) assert 'CHECK_LOOKUP_GH_API_TIMEOUT_SECONDS: "15"' in workflow - assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "120"' in workflow + assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "120"' not in workflow assert ( "Skipping publish-step failed-check OpenCode diagnosis for central review-process self-repair" in workflow ) assert 'OPENCODE_MODEL_CANDIDATES: "contextual-orchestrator/orchestrator/free"' in workflow assert 'OPENCODE_MODEL_ATTEMPTS: "1"' 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 assert 'OPENCODE_POOL_MAX_CYCLES: "1"' in workflow assert 'OPENCODE_DYNAMIC_REVIEW_CADENCE: "true"' in workflow assert ( "OPENCODE_CHANGED_FILES_FILE: ${{ runner.temp }}/opencode-changed-files.txt" 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: "11700"' in workflow - assert 'OPENCODE_MEDIUM_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' 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: "11700"' in workflow - assert 'OPENCODE_UNKNOWN_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' 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 assert 'OPENCODE_DYNAMIC_MAX_CYCLES: "1"' in workflow assert 'OPENCODE_BACKOFF_MAX_SECONDS: "30"' in workflow publish_step = workflow.split(" - name: Publish OpenCode review outcome", 1)[ @@ -2070,8 +2036,8 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): not in publish_step ) assert "MODEL: contextual-orchestrator/orchestrator/free" in publish_step - assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "120"' in publish_step - assert "${OPENCODE_RUN_TIMEOUT_SECONDS:-120}s" in publish_step + assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "120"' not in publish_step + assert "${OPENCODE_RUN_TIMEOUT_SECONDS:-120}s" not in publish_step assert ( 'timeout --kill-after=15s "${OPENCODE_EXPORT_TIMEOUT_SECONDS:-120}s"' in publish_step @@ -2117,8 +2083,8 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): ) assert "while :" in model_pool_runner assert "should_skip_model_candidate" in model_pool_runner - assert "cap_model_run_timeout" in model_pool_runner - assert "bounded failover window" in model_pool_runner + assert "cap_model_run_timeout" not in model_pool_runner + assert "bounded failover window" not in model_pool_runner assert "run_central_adversarial_harness" not in model_pool_runner assert "finish_pool_without_model" in model_pool_runner assert "central-current-head-adversarial-harness" not in model_pool_runner @@ -2126,19 +2092,16 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "mini/nano review models are disabled" in model_pool_runner assert "OPENAI_API_KEY is not configured" in model_pool_runner assert "configured max cycle count" in model_pool_runner - assert ( - "OpenCode dynamic review cadence selected %ss per attempt" in model_pool_runner - ) - assert "count_changed_files_for_cadence" in model_pool_runner + assert "OpenCode dynamic review cadence selected %ss per attempt" not in model_pool_runner assert ( "OpenCode model pool has no configured model candidates." in model_pool_runner ) - assert "OPENCODE_TOTAL_RETRY_BUDGET_SECONDS:-1500" in model_pool_runner + assert "OPENCODE_TOTAL_RETRY_BUDGET_SECONDS:-1500" not in model_pool_runner assert ( "completed a full model-candidate cycle without a valid control conclusion" in model_pool_runner ) - assert "retry budget/GitHub Actions job timeout" in model_pool_runner + assert "retry budget/GitHub Actions job timeout" not in model_pool_runner assert ( "OpenCode model pool exhausted before producing a valid control conclusion." in model_pool_runner @@ -2295,72 +2258,13 @@ def test_opencode_excludes_queue_self_check_from_every_failed_check_path(): assert retained == [{"name": "real-peer-check", "conclusion": "FAILURE"}] -def test_opencode_job_timeout_contains_full_sequential_review_budget(): - """Keep the outer job alive through evidence, review, and publication.""" +def test_opencode_job_has_no_model_inference_timeout(): + """Generating review work must be cancellable, not killed by a clock.""" workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text(encoding="utf-8") - - def timeout_minutes(pattern: str) -> int: - match = re.search(pattern, workflow, re.MULTILINE) - assert match, f"missing timeout contract: {pattern}" - return int(match.group(1)) - - job_timeout = timeout_minutes( - r"^ opencode-review-target:\n[\s\S]{0,4000}?^ timeout-minutes: (\d+)$" - ) - evidence_timeout = timeout_minutes( - r"^ - name: Prepare bounded OpenCode review evidence\n" - r"[\s\S]{0,200}?^ timeout-minutes: (\d+)$" - ) - model_pool_timeout = timeout_minutes( - r"^ - name: Run OpenCode PR Review model pool\n" - r"[\s\S]{0,300}?^ timeout-minutes: (\d+)$" - ) - fast_publish_timeout = timeout_minutes( - r"^ - name: Publish central OpenCode fast approval\n" - r"[\s\S]{0,500}?^ timeout-minutes: (\d+)$" - ) - normal_publish_timeout = timeout_minutes( - r"^ - name: Publish OpenCode review outcome\n" - r"[\s\S]{0,1200}?^ timeout-minutes: (\d+)$" - ) - noema_handoff_timeout = timeout_minutes( - r"^ - name: Dispatch Noema after current-head OpenCode approval\n" - r"[\s\S]{0,500}?^ timeout-minutes: (\d+)$" - ) - setup_and_cleanup_margin = 30 - required_timeout = ( - evidence_timeout - + model_pool_timeout - + max(fast_publish_timeout, normal_publish_timeout) - + noema_handoff_timeout - + setup_and_cleanup_margin - ) - - assert job_timeout >= required_timeout, ( - "opencode-review-target can terminate before publishing the bounded " - f"current-head result: job={job_timeout}m required={required_timeout}m" - ) - - -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 + target = workflow.split(" opencode-review-target:\n", 1)[1] + assert "timeout-minutes: 325" not in target.split(" steps:\n", 1)[0] + assert "timeout-minutes: 205" not in target + assert 'timeout --kill-after=30s "${OPENCODE_POOL_STEP_TIMEOUT_SECONDS' not in target def test_opencode_approval_gate_shell_is_parseable(): @@ -2681,9 +2585,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 '[ "$SUPPLIED_HEAD_REF" = "$live_head_ref" ]' 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\'' @@ -3148,10 +3053,9 @@ def test_peer_check_wait_budget_fits_publication_step_timeouts(): assert slow_image_attempts == [60, 60] assert sleeps == [10, 10] assert fast_timeout is not None - assert publish_timeout is not None + assert publish_timeout is None wait_seconds = (max(slow_build_attempts[0], slow_image_attempts[0]) - 1) * sleeps[0] assert int(fast_timeout.group(1)) * 60 - wait_seconds >= 120 - assert int(publish_timeout.group(1)) * 60 - wait_seconds >= 240 def test_slow_peer_wait_matches_only_image_validation_checks(): diff --git a/tests/test_opencode_model_pool_runner.py b/tests/test_opencode_model_pool_runner.py index 08d17f0008..2965d4c55c 100644 --- a/tests/test_opencode_model_pool_runner.py +++ b/tests/test_opencode_model_pool_runner.py @@ -160,6 +160,9 @@ def run_failed_model( ' [ -z "${FAKE_OPENCODE_PROMPT_CAPTURE:-}" ] || printf \'%s\\n\' "$2" > "$FAKE_OPENCODE_PROMPT_CAPTURE"\n' ' [ -z "${FAKE_OPENCODE_JSON:-}" ] || printf \'%s\\n\' "$FAKE_OPENCODE_JSON"\n' ' [ -z "${FAKE_OPENCODE_STDERR:-}" ] || printf \'%s\\n\' "$FAKE_OPENCODE_STDERR" >&2\n' + ' if [ "${FAKE_OPENCODE_SPAWN_TERM_IGNORING_CHILD:-}" = 1 ]; then\n' + " (trap '' TERM; sleep 120) &\n" + " fi\n" ' sleep "${FAKE_OPENCODE_HANG_SECONDS:-0}"\n' ' exit "${FAKE_OPENCODE_RUN_EXIT:-1}"\n' "fi\n" @@ -583,6 +586,26 @@ def test_fatal_provider_error_kills_hung_opencode_run_early( assert elapsed < 25 +def test_fatal_provider_error_kills_term_ignoring_descendant(tmp_path: Path) -> None: + """Fatal cancellation kills the whole dedicated group, including descendants.""" + start = time.monotonic() + result = run_failed_model( + tmp_path, + json_line=( + '{"type":"error","error":{"name":"ProviderQuotaError","data":' + '{"message":"insufficient_quota: request rejected"}}}' + ), + extra_env={ + "FAKE_OPENCODE_HANG_SECONDS": "120", + "FAKE_OPENCODE_SPAWN_TERM_IGNORING_CHILD": "1", + }, + ) + + assert result.returncode == 1 + assert "logged a fatal provider error while still running" in result.stdout + assert time.monotonic() - start < 25 + + def test_model_text_quoting_error_signatures_does_not_kill_run(tmp_path: Path) -> None: """Model prose mentioning fatal signatures never kills a healthy streaming run.""" result = run_failed_model( @@ -704,7 +727,7 @@ def test_attempt_ceiling_bounds_provider_spend(tmp_path: Path) -> None: def test_dynamic_review_cadence_uses_small_change_timeout(tmp_path: Path) -> None: - """Small PRs fail through hung/unavailable providers quickly with a visible budget reason.""" + """Changed-file cadence never reintroduces an inference deadline.""" result = run_failed_model( tmp_path, changed_files=["pyproject.toml", "uv.lock"], @@ -719,20 +742,9 @@ def test_dynamic_review_cadence_uses_small_change_timeout(tmp_path: Path) -> Non ) assert result.returncode == 1 - assert ( - "OpenCode dynamic review cadence selected 7s per attempt and 11s total budget " - "for 2 changed file(s); max-cycles=1." - ) in result.stdout - attempt_budget = re.search( - r"OpenCode github-models/openai/gpt-5 attempt 1/1 using (\d+)s run timeout " - r"with (\d+)s retry budget remaining\.", - result.stdout, - ) - assert attempt_budget is not None - run_timeout, remaining_budget = map(int, attempt_budget.groups()) - assert 1 <= run_timeout <= 7 - assert run_timeout <= remaining_budget <= 11 - assert "retry budget remaining." in result.stdout + assert "model inference has no wall-clock timeout" in result.stdout + assert "7s per attempt" not in result.stdout + assert "attempt 1/1 has no model inference timeout" in result.stdout def test_dynamic_review_cadence_caps_large_change_queue_budget(tmp_path: Path) -> None: @@ -742,8 +754,8 @@ def test_dynamic_review_cadence_caps_large_change_queue_budget(tmp_path: Path) - tmp_path, changed_files=changed_files, extra_env={ - "OPENCODE_DYNAMIC_REVIEW_CADENCE": "true", - "OPENCODE_DYNAMIC_MAX_CYCLES": "0", + "OPENCODE_DYNAMIC_REVIEW_CADENCE": "true", + "OPENCODE_DYNAMIC_MAX_CYCLES": "1", "OPENCODE_DYNAMIC_TOTAL_BUDGET_CAP_SECONDS": "1", "OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS": "3600", "OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS": "7200", @@ -753,21 +765,8 @@ def test_dynamic_review_cadence_caps_large_change_queue_budget(tmp_path: Path) - ) assert result.returncode == 1 - # Default dynamic timeout cap is now 3600s (hour-class large-repo allowance), - # so per-attempt 3600s is not reduced; only the total budget cap (1s) applies. - assert ( - "OpenCode dynamic review cadence queue cap applied: per-attempt 3600s -> 3600s, " - "total budget 7200s -> 1s, max-cycles 0 -> 0" - ) in result.stdout or ( - "total budget 7200s -> 1s" in result.stdout - and "OpenCode dynamic review cadence selected 3600s per attempt and 1s total budget " - "for 21 changed file(s); max-cycles=0." in result.stdout - ) - assert ( - "OpenCode dynamic review cadence selected 3600s per attempt and 1s total budget " - "for 21 changed file(s); max-cycles=0." - ) in result.stdout - assert "OpenCode model pool reached configured max cycle count" not in result.stdout + assert "model inference has no wall-clock timeout" in result.stdout + assert "total budget" not in result.stdout assert ( "OpenCode model pool exhausted before producing a valid control conclusion." in result.stdout @@ -785,19 +784,9 @@ def test_github_gpt5_runtime_cap_preserves_queue_budget(tmp_path: Path) -> None: ) assert result.returncode == 1 - assert ( - "OpenCode github-models/openai/gpt-5 runtime cap selected 3s instead of 9s " - "because this provider has a bounded failover window." - ) in result.stdout - attempt_budget = re.search( - r"OpenCode github-models/openai/gpt-5 attempt 1/1 using (\d+)s run timeout " - r"with (\d+)s retry budget remaining\.", - result.stdout, - ) - assert attempt_budget is not None - run_timeout, remaining_budget = map(int, attempt_budget.groups()) - assert run_timeout == 3 - assert run_timeout <= remaining_budget <= 30 + assert "model inference has no wall-clock timeout" in result.stdout + assert "runtime cap selected" not in result.stdout + assert "attempt 1/1 has no model inference timeout" in result.stdout def test_free_provider_runtime_cap_preserves_queue_budget(tmp_path: Path) -> None: @@ -812,10 +801,8 @@ def test_free_provider_runtime_cap_preserves_queue_budget(tmp_path: Path) -> Non ) assert result.returncode == 1 - assert ( - "OpenCode opencode-free/nemotron-3-ultra-free runtime cap selected 3s " - "instead of 9s because this provider has a bounded failover window." - ) in result.stdout + assert "model inference has no wall-clock timeout" in result.stdout + assert "runtime cap selected" not in result.stdout def test_nvidia_nim_candidate_requires_key( @@ -848,10 +835,8 @@ def test_nvidia_nim_runtime_cap_preserves_queue_budget(tmp_path: Path) -> None: ) assert result.returncode == 1 - assert ( - "OpenCode nvidia-nim/nvidia/nemotron-3-ultra-550b-a55b runtime cap " - "selected 3s instead of 9s because this provider has a bounded failover window." - ) in result.stdout + assert "model inference has no wall-clock timeout" in result.stdout + assert "runtime cap selected" not in result.stdout def test_nvidia_nim_combined_budget_preserves_fallback_attempt( @@ -880,12 +865,9 @@ def test_nvidia_nim_combined_budget_preserves_fallback_attempt( ) assert result.returncode == 1 - assert "OpenCode NVIDIA NIM combined runtime used" in result.stdout - assert ( - "Skipping OpenCode nvidia-nim/nvidia/nemotron-3-super-120b-a12b " - "because the NVIDIA NIM combined runtime budget of 1s is exhausted" - in result.stdout - ) + assert "OpenCode NVIDIA NIM combined runtime used" not in result.stdout + assert "model inference has no wall-clock timeout" in result.stdout + assert "combined runtime budget" not in result.stdout assert "OpenCode opencode-free/nemotron-3-ultra-free attempt 1/2" in result.stdout assert "schema-repair attempt 2/2" not in result.stdout diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 7fb4456f56..8f8047ff10 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -16,6 +16,19 @@ 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") +RECEIPT_HELPER = Path("scripts/ci/opencode_review_receipt_gate.py") + + +def request_review_script() -> str: + """Extract the production scheduler-wake run block.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + step = workflow.split( + " - name: Request current-head OpenCode review execution\n", 1 + )[1] + block = step.split(" run: |\n", 1)[1].split( + "\n - name: Fail closed", 1 + )[0] + return textwrap.dedent(block) def review(*, state: str, commit_id: str = HEAD, body: str = "") -> dict[str, object]: @@ -108,16 +121,23 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non 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: 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 "timeout-minutes:" not in target_job.split(" steps:\n", 1)[0] assert "id-token: write" in target_job.split(" steps:\n", 1)[0] - 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 'event_type:"merge-scheduler"' in workflow + assert "trigger_reviews:true" in workflow + dispatch_step = target_job.split( + " - name: Request current-head OpenCode review execution", 1 + )[1].split(" - name: Fail closed", 1)[0] + assert "scripts/ci/opencode_review_receipt_gate.py" in dispatch_step + assert "github.workflow_sha" in dispatch_step + assert "evaluate_receipts" in dispatch_step + assert dispatch_step.index("evaluate_receipts") < dispatch_step.index( + "exchange_github_app_token" + ) + assert "Current-head substantive OpenCode verdict already exists; scheduler wake skipped." in dispatch_step + assert "while :; do" in target_job + assert "sleep 30" in target_job + assert "enable_auto_merge:false" in workflow 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 @@ -127,14 +147,79 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non ) -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.""" +@pytest.mark.parametrize( + ("reviews", "dispatches"), + ( + ([{"id": 7, **review(state="APPROVED", body="## Verdict\nApprove")}], 0), + ([{"id": 8, **review(state="CHANGES_REQUESTED", body="## Verdict\nRequest changes")}], 0), + ([], 1), + ([{"id": 9, **review(state="APPROVED", commit_id="b" * 40, body="## Verdict\nApprove")}], 1), + ([{"id": 10, **review(state="APPROVED", body="## Pull request overview\n\ndeterministic fallback approval")}], 1), + ), +) +def test_scheduler_wake_reuses_trusted_receipt_predicate( + tmp_path: Path, reviews: list[dict[str, object]], dispatches: int +) -> None: + """Only missing, stale, or fallback-only evidence wakes the scheduler.""" + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + calls = tmp_path / "dispatches" + fake_gh = fake_bin / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then + python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER" +elif [[ "$*" == *"/pulls/7/reviews"* ]]; then + printf '[%s]' "$FAKE_REVIEWS" +elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then + printf 'dispatch\n' >>"$DISPATCH_CALLS" +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + fake_curl = fake_bin / "curl" + fake_curl.write_text( + """#!/usr/bin/env bash +[[ "$*" == *"exchange_github_app_token"* ]] && printf '{"token":"app"}' || printf '{"value":"oidc"}' +""", + encoding="utf-8", + ) + fake_curl.chmod(0o755) + env = { + **os.environ, + "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", + "REAL_RECEIPT_HELPER": str(RECEIPT_HELPER.resolve()), + "FAKE_REVIEWS": json.dumps(reviews), + "DISPATCH_CALLS": str(calls), + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request", + "ACTIONS_ID_TOKEN_REQUEST_URL": "https://token.example", + "OIDC_AUDIENCE": "opencode-github-action", + "OPENCODE_API_BASE_URL": "https://api.opencode.ai", + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "PR_DRAFT": "false", + "BASE_BRANCH": "main", + "WORKFLOW_SHA": "c" * 40, + "GH_TOKEN": "token", + } + result = subprocess.run( + ["bash", "-c", request_review_script()], env=env, text=True, capture_output=True + ) + assert result.returncode == 0, result.stderr + actual = calls.read_text(encoding="utf-8").count("dispatch") if calls.exists() else 0 + assert actual == dispatches + + +def test_formal_receipt_wake_remains_available_without_bounding_runner_polling() -> None: + """The receipt wake path coexists with the unbounded required review wait.""" required = WORKFLOW.read_text(encoding="utf-8") dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") assert "for attempt in" not in required + assert "while :; do" 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 diff --git a/tests/test_opencode_review_receipt_gate.py b/tests/test_opencode_review_receipt_gate.py index 9e558e94df..c971e2128a 100644 --- a/tests/test_opencode_review_receipt_gate.py +++ b/tests/test_opencode_review_receipt_gate.py @@ -80,6 +80,18 @@ def test_draft_never_accepts_bot_approve_as_receipt() -> None: assert "no current-head formal" in reason +def test_fallback_approval_with_product_heading_is_not_substantive() -> None: + """A normal overview cannot disguise deterministic fallback evidence.""" + fallback = review( + commit=receipt.AFIPC_230_HEAD, + state="APPROVED", + body="## Pull request overview\n\ndeterministic fallback approval", + ) + found, reason = receipt.evaluate_receipts([fallback], receipt.AFIPC_230_HEAD) + assert found is None + assert "fallback" in reason + + def test_status_comment_and_mention_payloads_are_not_receipts() -> None: """Issue-comment status text and @mentions cannot green the required check.""" status = review( @@ -237,13 +249,14 @@ def test_receipt_cli_and_fetch(tmp_path: Path, capsys, monkeypatch) -> None: def fake_run(args, **kwargs): assert args[0] == "gh" + assert args[-2:] == ["--paginate", "--slurp"] return type( "Completed", (), { "returncode": 0, "stdout": json.dumps( - [review(commit=receipt.AFIPC_230_HEAD, state="CHANGES_REQUESTED")] + [[review(commit=receipt.AFIPC_230_HEAD, state="CHANGES_REQUESTED")]] ), "stderr": "", }, @@ -251,6 +264,26 @@ def fake_run(args, **kwargs): monkeypatch.setattr(receipt.subprocess, "run", fake_run) assert receipt.fetch_reviews("ContextualWisdomLab/.github", 1392) + + def fake_pages(args, **kwargs): + return type( + "Completed", + (), + { + "returncode": 0, + "stdout": json.dumps( + [ + [review(commit="b" * 40, review_id=1)], + [review(commit=receipt.AFIPC_230_HEAD, review_id=2)], + ] + ), + "stderr": "", + }, + )() + + monkeypatch.setattr(receipt.subprocess, "run", fake_pages) + assert [item["id"] for item in receipt.fetch_reviews("ContextualWisdomLab/.github", 1392)] == [1, 2] + monkeypatch.setattr(receipt.subprocess, "run", fake_run) assert ( receipt.main( [ diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index b2e29b9c13..9f81076199 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 = "cdc1245266403f0b238558ecbab528d1557412dd" +REVIEW_DISPATCH_BLOB_SHA = "3677f408bd6b99577fd7d5923fbd67c91f437ae0" def _workflow_text(path: Path) -> str: diff --git a/tests/test_pr_review_fix_hourly_contract.py b/tests/test_pr_review_fix_hourly_contract.py index 072ba4d8b3..a31562550c 100644 --- a/tests/test_pr_review_fix_hourly_contract.py +++ b/tests/test_pr_review_fix_hourly_contract.py @@ -327,6 +327,7 @@ def fake_run(args: list[str], *, stdin: str | None = None) -> str: return "" monkeypatch.setattr(scheduler, "run", fake_run) + monkeypatch.setattr(scheduler, "live_head_matches", lambda _repo, _pr: True) pr = _current_head_change_request("Failed check evidence reports Strix failed.") scheduler.dispatch_autofix( diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 3b4416bdc3..9860eeaec7 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -37,6 +37,146 @@ def test_recent_fix_marker_is_head_scoped(): assert not fix.recent_fix_marker_exists([{"body": f"{fix.FIX_MARKER} head_sha={head} epoch=oops -->"}], head, 24 * 3600) +def test_prepare_autofix_slot_deduplicates_head_and_cancels_only_stale(monkeypatch): + """A long-running exact-head worker survives while its older sibling is cancelled.""" + head = "a" * 40 + stale = "b" * 40 + requests = [] + monkeypatch.setattr( + fix, + "run_json", + lambda args: requests.append(args) + or [ + { + "workflow_runs": [ + { + "id": 99, + "status": "completed", + "display_title": "unrelated first page", + } + ] + }, + { + "workflow_runs": [ + { + "id": 1, + "status": "in_progress", + "display_title": f"PR Review Autofix owner/repo#7@{head}", + }, + { + "id": 2, + "status": "queued", + "display_title": f"PR Review Autofix owner/repo#7@{stale}", + }, + { + "id": 3, + "status": "in_progress", + "display_title": f"PR Review Autofix owner/repo#8@{stale}", + }, + {"id": 4, "status": "in_progress", "display_title": "malformed"}, + ] + }, + ], + ) + cancelled = [] + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda repo, ids: cancelled.append((repo, ids)), + ) + monkeypatch.setattr(fix, "live_head_matches", lambda _repo, _pr: True) + + assert fix.prepare_autofix_slot( + "owner/repo", + make_pr(headRefOid=head), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) + assert cancelled == [(fix.DEFAULT_AUTOFIX_REPOSITORY, ["2"])] + assert "--paginate" in requests[0] + assert "--slurp" in requests[0] + + +def test_inspect_pr_reports_stale_snapshot_without_dispatch(monkeypatch): + """A moved head is not mislabeled as an active worker or dispatched stale.""" + args = fix.parse_args(["--repo", "owner/repo", "--base-branch", "main"]) + monkeypatch.setattr(fix, "needs_autofix", lambda _pr: (True, ("review",))) + monkeypatch.setattr(fix, "issue_comments", lambda _repo, _number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: None) + monkeypatch.setattr( + fix, + "dispatch_autofix", + lambda *_args, **_kwargs: pytest.fail("stale snapshot must not dispatch"), + ) + + assert fix.inspect_pr("owner/repo", make_pr(), args) == ( + "wait", + ("scheduler PR snapshot is stale; retry with the current live head",), + ) + + +def test_prepare_autofix_slot_dry_run_preserves_stale_worker(monkeypatch, capsys): + """Dry-run reports an older head without mutating Actions state.""" + stale = "b" * 40 + monkeypatch.setattr( + fix, + "run_json", + lambda _args: { + "workflow_runs": [ + { + "id": 2, + "status": "waiting", + "display_title": f"PR Review Autofix owner/repo#7@{stale}", + } + ] + }, + ) + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda *_args: pytest.fail("dry-run must not cancel"), + ) + + assert not fix.prepare_autofix_slot( + "owner/repo", + make_pr(), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=True, + ) + assert "would force-cancel stale autofix runs 2" in capsys.readouterr().out + + +def test_prepare_autofix_slot_preserves_new_head_workers_after_head_advance(monkeypatch): + """A stale scheduler snapshot cannot cancel a newer live-head worker.""" + monkeypatch.setattr( + fix, + "run_json", + lambda _args: { + "workflow_runs": [ + { + "id": 2, + "status": "in_progress", + "display_title": f"PR Review Autofix owner/repo#7@{'b' * 40}", + } + ] + }, + ) + monkeypatch.setattr(fix, "live_head_matches", lambda _repo, _pr: False) + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda *_args: pytest.fail("advanced head must preserve active workers"), + ) + + assert fix.prepare_autofix_slot( + "owner/repo", + make_pr(), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) is None def test_terminal_failed_check_triggers_rca_without_prior_opencode_review(): """Exact-head check evidence can start RCA without a circular review prerequisite.""" pr = make_pr( @@ -156,6 +296,7 @@ def test_draft_with_failed_check_dispatches_rca(monkeypatch): }, ) monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) monkeypatch.setattr( fix, "dispatch_autofix", @@ -189,6 +330,7 @@ def test_conflict_repair_precedes_failed_check_rca(monkeypatch): }, ) monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) monkeypatch.setattr( fix, "dispatch_autofix", @@ -315,6 +457,7 @@ def test_process_queue_dispatches_same_repo_current_head(monkeypatch, capsys): monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr]) monkeypatch.setattr(fix, "needs_autofix", lambda pr: (True, ("current-head OpenCode requested changes",))) monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) monkeypatch.setattr( fix, "dispatch_autofix", @@ -838,6 +981,7 @@ def fake_run(argv, *, stdin=None): assert "DRY-RUN: would create autofix marker" in capsys.readouterr().out fix.create_fix_marker("owner/repo", pr, dry_run=False) + monkeypatch.setattr(fix, "live_head_matches", lambda _repo, _pr: True) fix.dispatch_autofix( "owner/repo", pr, @@ -866,6 +1010,25 @@ def fake_run(argv, *, stdin=None): assert payload["client_payload"]["target_repository"] == "owner/repo" +def test_dispatch_autofix_rejects_advanced_live_head(monkeypatch): + """Revalidate the exact head immediately before repository dispatch.""" + monkeypatch.setattr(fix, "live_head_matches", lambda _repo, _pr: False) + monkeypatch.setattr( + fix, + "run", + lambda *_args, **_kwargs: pytest.fail("advanced head must not dispatch"), + ) + + with pytest.raises(RuntimeError, match="live head changed"): + fix.dispatch_autofix( + "owner/repo", + make_pr(), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) + + def test_is_rate_limit_error_matches_known_github_signatures(): """Rate-limit detection matches GitHub's primary and secondary wording.""" assert fix.is_rate_limit_error(RuntimeError("gh: API rate limit exceeded for installation ID 1")) @@ -1085,6 +1248,7 @@ def test_inspect_pr_dispatches_failed_check_rca(monkeypatch): ) captured = {} monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) monkeypatch.setattr( fix, "dispatch_autofix", @@ -1107,6 +1271,7 @@ def test_inspect_pr_dispatches_conflict_resolution(monkeypatch): """An approved conflicting PR dispatches autofix in resolve_conflict mode.""" captured = {} monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) monkeypatch.setattr( fix, "dispatch_autofix", @@ -1127,6 +1292,7 @@ def test_process_queue_includes_conflict_resolution_candidates(monkeypatch, caps pr = _approved_dirty_pr(baseRefName="feature-base") monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr]) monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) monkeypatch.setattr( fix, "dispatch_autofix", diff --git a/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py b/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py index c5a0c965b5..af1dfd71ef 100644 --- a/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py +++ b/tests/test_pr_review_fix_scheduler_direct_rca_regressions.py @@ -10,6 +10,12 @@ from scripts.ci import pr_review_fix_scheduler as fix +@pytest.fixture(autouse=True) +def isolate_active_autofix_inventory(monkeypatch: Any) -> None: + """Keep RCA unit tests independent of live GitHub Actions inventory.""" + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: False) + + def make_pr(*, is_draft: bool = False) -> dict[str, Any]: """Return a clean same-repository PR with review and failed-check evidence.""" head = "a" * 40 diff --git a/tests/test_repository_branch_coverage_review_schedulers.py b/tests/test_repository_branch_coverage_review_schedulers.py index 4928e18046..596df2ee0a 100644 --- a/tests/test_repository_branch_coverage_review_schedulers.py +++ b/tests/test_repository_branch_coverage_review_schedulers.py @@ -62,8 +62,8 @@ def read(self) -> bytes: class Opener: """Open one deterministic provider response.""" - def open(self, _request: Any, timeout: int) -> Response: - assert timeout == noema.NOEMA_LLM_TIMEOUT_SECONDS + def open(self, _request: Any, timeout: int | None = None) -> Response: + assert timeout is None return Response() monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 5a295da25f..f065837eb6 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -241,14 +241,7 @@ 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 - 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: + if filename != "noema-review.yml": assert "cancel-in-progress: true" in workflow if filename in { "close-empty-pr.yml", @@ -261,27 +254,13 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: elif filename == "opencode-review.yml": 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.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 + assert "github.event.workflow_run" not in concurrency_contract + assert "noema-review-${{" in concurrency_contract + assert "github.event_name" not in concurrency_contract.split( + "cancel-in-progress:", 1 + )[0] + assert "github.event.action == 'synchronize'" in concurrency_contract + assert "github.event.action == 'closed'" in concurrency_contract else: if filename in {"codeql-pr.yml", "osv-scanner-pr.yml", "scorecard-pr.yml"}: assert "github.event_name == 'pull_request'" in concurrency_contract @@ -350,9 +329,9 @@ def test_strix_serializes_provider_evidence_per_repository() -> None: Root cause (2026-08-23/24): sibling PRs scanned concurrently, each retrying the shared NVIDIA NIM key three times, producing litellm.RateLimitError storms and fail-closed gate failures on every open PR. The concurrency group - now scopes one scan at a time per repository and event class. GitHub retains - one active and one pending run per group; the scheduler re-dispatches exact - current-head evidence when a pending run is superseded. + now scopes the scan job per repository and event class. The cleanup job is + outside that queue so a synchronize event can immediately retire an older + exact-head run without allowing sibling scans to overlap. """ workflow = workflow_text("strix.yml") concurrency_contract = workflow.split("concurrency:", 1)[1].split( @@ -363,10 +342,6 @@ def test_strix_serializes_provider_evidence_per_repository() -> None: assert "github.event.client_payload.target_repository" in concurrency_contract assert "github.event.pull_request.base.repo.full_name" in concurrency_contract assert "github.repository" in concurrency_contract - assert ( - "format('closed-pr-{0}-{1}', github.event.pull_request.base.repo.full_name, " - "github.event.pull_request.number)" - ) in concurrency_contract assert ( "format('{0}-{1}', github.event_name, github.event.client_payload.target_repository || " "github.event.pull_request.base.repo.full_name || github.repository)" @@ -383,9 +358,26 @@ def test_strix_serializes_provider_evidence_per_repository() -> None: assert "cancel-in-progress: false" in workflow assert "cancel-in-progress: true" not in workflow.split("jobs:", 1)[0] assert "queue: max" not in workflow - assert "scheduler" in concurrency_contract - assert "default-branch repository_dispatch evidence cannot cancel" in workflow - assert "RateLimitError" in concurrency_contract + assert workflow.index("cancel-superseded-pr-runs:") < workflow.index("concurrency:") + cleanup_job = workflow.split(" cancel-superseded-pr-runs:", 1)[1].split( + " strix:", 1 + )[0] + assert "github.event.action == 'synchronize'" in cleanup_job + assert 'endswith("@" + $head_sha)' in cleanup_job + assert "/force-cancel" in cleanup_job + assert 'gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}"' in cleanup_job + assert "could not verify the live pull request" in cleanup_job + assert "target changed before run selection" in cleanup_job + assert "target changed before cancellation" in cleanup_job + assert cleanup_job.index("if ! live_target_matches") < cleanup_job.index( + 'runs_url="repos/${TARGET_REPOSITORY}/actions/runs?status=${status}&per_page=100"' + ) + assert cleanup_job.rindex("if ! live_target_matches") < cleanup_job.index( + 'gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel"' + ) + assert "actions: write" in cleanup_job + assert "pull-requests: read" in cleanup_job + assert "actions/checkout" not in cleanup_job assert ( "refs/pull//head has already advanced before this queued run starts" in workflow @@ -409,6 +401,126 @@ def test_strix_install_normalizes_executable_permissions_before_hashing() -> Non ) +def test_strix_cleanup_uses_pr_metadata_when_custom_title_is_absent() -> None: + """Required-workflow runs retain exact PR/head cleanup without run-name rendering.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup selector") + workflow = workflow_text("strix.yml") + marker = '--arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" \'\n' + start = workflow.index(marker) + len(marker) + end = workflow.index('\n \' <<<"$runs_json"', start) + runs = { + "workflow_runs": [ + {"id": 1, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "old"}}]}, + {"id": 2, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, + {"id": 3, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7}]}, + {"id": 4, "name": "Strix Security Scan", "event": "pull_request_target", "display_title": "Strix Security Scan owner/repo#7@old", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, + {"id": 5, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 8, "head": {"sha": "old"}}]}, + ] + } + result = subprocess.run( + [jq, "-r", "--arg", "pr", "7", "--arg", "head_sha", "current", "--arg", "action", "synchronize", "--arg", "repo", "owner/repo", "--arg", "current", "99", workflow[start:end]], + input=json.dumps(runs), + text=True, + capture_output=True, + check=True, + ) + assert result.stdout.splitlines() == ["1"] + + +def _run_strix_cleanup(tmp_path: Path, pull_states: list[dict[str, object]]) -> str: + """Execute the production cleanup step against a stateful fake ``gh``.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup") + step = workflow_step( + workflow_text("strix.yml"), + "Cancel queued and running scans for superseded or closed pull request heads", + ) + run_block = step.split(" run: |\n", 1)[1].split("\n strix:", 1)[0] + script = textwrap.dedent(run_block) + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + calls = tmp_path / "calls" + pulls = tmp_path / "pulls" + pulls.write_text( + "\n".join(json.dumps(state) for state in pull_states) + "\n", + encoding="utf-8", + ) + fake_gh = fake_bin / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == *"/pulls/7"* ]]; then + count_file="${FAKE_PULLS}.count" + count=0 + [[ ! -f "$count_file" ]] || count="$(cat "$count_file")" + count=$((count + 1)) + printf '%s' "$count" >"$count_file" + sed -n "${count}p" "$FAKE_PULLS" + exit 0 +fi +if [[ "$*" == *"actions/runs?status=queued"* ]]; then + printf '%s\n' '{"workflow_runs":[{"id":100,"name":"Strix Security Scan","event":"pull_request_target","pull_requests":[{"number":7,"head":{"sha":"old"}}]}]}' + exit 0 +fi +if [[ "$*" == *"actions/runs?status="* ]]; then + printf '%s\n' '{"workflow_runs":[]}' + exit 0 +fi +exit 0 +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + env = { + **os.environ, + "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", + "FAKE_CALLS": str(calls), + "FAKE_PULLS": str(pulls), + "TARGET_REPOSITORY": "owner/repo", + "TARGET_PR_NUMBER": "7", + "TARGET_PR_HEAD_SHA": "current", + "PR_ACTION": "synchronize", + "CURRENT_RUN_ID": "999", + } + subprocess.run(["bash", "-c", script], env=env, check=True, capture_output=True, text=True) + return calls.read_text(encoding="utf-8") + + +def test_old_strix_cleanup_never_lists_or_cancels_after_live_head_advanced( + tmp_path: Path, +) -> None: + """A late old synchronize job must stop before selecting current runs.""" + calls = _run_strix_cleanup( + tmp_path, [{"state": "open", "head": {"sha": "newer"}}] * 5 + ) + + assert "actions/runs?status=" not in calls + assert "/cancel" not in calls + assert "/force-cancel" not in calls + + +def test_strix_cleanup_revalidates_after_selection_before_cancellation( + tmp_path: Path, +) -> None: + """A head advance after selection must prevent the pending mutation.""" + calls = _run_strix_cleanup( + tmp_path, + [ + {"state": "open", "head": {"sha": "current"}}, + {"state": "open", "head": {"sha": "newer"}}, + ] + + [{"state": "open", "head": {"sha": "newer"}}] * 4, + ) + + assert "actions/runs?status=queued" in calls + assert "/actions/runs/100/cancel" not in calls + assert "/actions/runs/100/force-cancel" not in calls + + def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() -> None: """Close events should cancel old runs without starting expensive jobs.""" workflows = ( @@ -426,32 +538,39 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - workflow = workflow_text(filename) assert "closed" in workflow - assert "cancel-closed-pr-runs:" 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 + if filename == "strix.yml": + assert "cancel-superseded-pr-runs:" in workflow + assert "Cancel queued and running scans for superseded or closed pull request heads" in workflow + assert ( + "secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN " + "|| github.token" + ) in workflow + assert "DISPATCH_REPOSITORY" not in workflow + assert "TARGET_PR_HEAD_SHA" in workflow + assert 'select(.event == "pull_request_target")' in workflow + assert 'select(.event == "repository_dispatch")' not in workflow + assert "(.pull_requests // [])" in workflow + assert ".head.sha // \"\"" in workflow + assert "leaving runs unchanged" in workflow + assert ( + "for active_status in queued in_progress requested waiting pending" + in workflow + ) + cleanup_job = workflow.split(" cancel-superseded-pr-runs:", 1)[1].split( + " strix:", 1 + )[0] + elif filename == "noema-review.yml": + assert "cancel-closed-pr-runs:" in workflow + assert "Cancel queued and running Noema reviews for the closed pull request" in workflow assert "leaving runs unchanged" in workflow - next_job = "strix" if filename == "strix.yml" else "noema-review" cleanup_job = workflow.split(" cancel-closed-pr-runs:", 1)[1].split( - f" {next_job}:", 1 + " noema-review:", 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 "cancel-closed-pr-runs:" in workflow assert ( "PR closed; this run only cancels older runs through workflow concurrency." in workflow @@ -466,11 +585,10 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - assert "${{ secrets." not in opencode_bootstrap strix_workflow = workflow_text("strix.yml") - # Strix serializes per repository (rate-limit root-cause fix): close-event - # runs still cancel superseded same-PR evidence through their own - # cancel-closed-pr-runs job, while scan jobs queue instead of cancelling. + # Strix serializes scans per repository while cleanup stays outside that + # queue so synchronize and close events can immediately retire old work. assert "cancel-in-progress: false" in strix_workflow - assert "Serialize Strix scans per repository" in strix_workflow or "per REPOSITORY" in strix_workflow + assert "Keep provider-backed scans serial per repository" in strix_workflow def test_close_empty_pr_metadata_lookup_retries_and_fails_open() -> None: @@ -487,10 +605,8 @@ def test_close_empty_pr_metadata_lookup_retries_and_fails_open() -> None: def test_cancelled_review_workflow_runs_do_not_spawn_more_queue_work() -> None: """Prevent cancelled review runs from creating follow-up queue work.""" - for filename in ("noema-review.yml", "pr-review-merge-scheduler.yml"): - workflow = workflow_text(filename) - - assert "github.event.workflow_run.conclusion != 'cancelled'" in workflow + workflow = workflow_text("pr-review-merge-scheduler.yml") + assert "github.event.workflow_run.conclusion != 'cancelled'" in workflow def test_required_workflow_trusted_source_refs_are_not_input_controlled() -> None: @@ -520,16 +636,23 @@ def test_required_workflow_trusted_source_refs_are_not_input_controlled() -> Non assert "GITHUB_CONTEXT_JSON: ${{ toJSON(github) }}" in workflow -def test_noema_triggers_serialize_one_review_per_pull_request() -> None: - """Serialize every Noema trigger type for one pull request.""" +def test_noema_triggers_preserve_standalone_pull_request_review() -> None: + """Noema reviews PRs independently of the other review workflows.""" workflow = workflow_text("noema-review.yml") concurrency_contract = workflow.split("permissions:", 1)[0] - assert "github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number" in concurrency_contract + assert "workflow_run:" not in concurrency_contract + assert "github.event.workflow_run" not in workflow + assert "github.event.pull_request.number" in concurrency_contract assert "github.event.client_payload.pr_number" 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 + assert "noema-review-${{" in concurrency_contract + assert "github.event_name" not in concurrency_contract.split( + "cancel-in-progress:", 1 + )[0] + assert "github.event.action == 'synchronize'" in concurrency_contract + assert "github.event.action == 'closed'" in concurrency_contract + assert "cancel-in-progress: true" not in concurrency_contract + assert '[ "${live_head_sha,,}" != "${EXPECTED_HEAD_SHA,,}" ]' in workflow def test_noema_review_credentials_and_orchestrator_configuration_fail_closed() -> None: diff --git a/tests/test_strix_backend_unavailable_after_exempted_finding.py b/tests/test_strix_backend_unavailable_after_exempted_finding.py index 650db6d253..f9b75e313d 100644 --- a/tests/test_strix_backend_unavailable_after_exempted_finding.py +++ b/tests/test_strix_backend_unavailable_after_exempted_finding.py @@ -276,16 +276,14 @@ def test_real_finding_after_continuation_never_retries(self) -> None: self.assertEqual(returncode, 1) self.assertEqual(calls, 1) - def test_retry_contract_preserves_logs_and_process_attempt_budget(self) -> None: - """Retries retain every attempt and reserve the scanner process budget.""" + def test_retry_contract_preserves_logs_without_wall_clock_budget(self) -> None: + """Retries retain every attempt without imposing an inference deadline.""" workflow = STRIX_WORKFLOW.read_text(encoding="utf-8") self.assertIn('strix_attempt_log="$RUNNER_TEMP/strix_gate_console_attempt_', workflow) self.assertIn('cat "$strix_attempt_log" >> "$strix_run_log"', workflow) - self.assertIn( - 'strix_gate_attempt_budget_seconds="$process_budget_seconds"', - workflow, - ) + self.assertNotIn("strix_gate_attempt_budget_seconds", workflow) + self.assertNotIn("STRIX_PROCESS_TIMEOUT_SECONDS:", workflow) self.assertNotIn("STRIX_TOTAL_TIMEOUT_SECONDS:", workflow) self.assertNotIn('remaining_seconds" -lt 600', workflow)