From 6837229e5134e03edd69dafd8cb4728fa851987b Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:04:11 +0800 Subject: [PATCH 1/6] refactor(review): retire duplicate AIAudit PR reviewer Co-Authored-By: Codex --- .github/actionlint.yaml | 5 - .github/codex_auto_merge_policy.json | 23 +- .github/workflows/codex_pr_review.yml | 156 --- README.md | 23 +- README.zh-CN.md | 15 +- client/gateway_client.py | 5 +- docs/ai_autonomy_architecture.md | 48 +- prompts/pr_review.md | 59 - scripts/run_codex_pr_review.py | 1244 ------------------ service/dual_review_primary.py | 39 +- service/org_health.py | 1 - tests/test_codex_audit_service_complexity.py | 19 - tests/test_dual_review_primary.py | 70 +- tests/test_org_health.py | 3 +- tests/test_run_codex_pr_review.py | 816 ------------ tests/test_single_pr_reviewer_contract.py | 51 + 16 files changed, 146 insertions(+), 2431 deletions(-) delete mode 100644 .github/workflows/codex_pr_review.yml delete mode 100644 prompts/pr_review.md delete mode 100644 scripts/run_codex_pr_review.py delete mode 100644 tests/test_run_codex_pr_review.py create mode 100644 tests/test_single_pr_reviewer_contract.py diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index 36c35fc3..ffee4173 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -1,8 +1,3 @@ self-hosted-runner: labels: - codex-vps -paths: - .github/workflows/codex_pr_review.yml: - ignore: - - 'workflow_repository' - - 'workflow_sha' diff --git a/.github/codex_auto_merge_policy.json b/.github/codex_auto_merge_policy.json index 50d2bfd4..37fee8f4 100644 --- a/.github/codex_auto_merge_policy.json +++ b/.github/codex_auto_merge_policy.json @@ -23,26 +23,5 @@ } }, "max_changed_files": 30, - "max_changed_lines": 2000, - "pr_review": { - "enabled": true, - "block_severities": [ - "critical", - "high" - ], - "skip_paths": [ - "docs/**", - "**.md", - "**.txt", - "**.json", - "**.csv", - "LICENSE", - ".github/dependabot*" - ], - "skip_risk_levels": [ - "low" - ], - "timeout_minutes": 20, - "max_diff_lines": 2400 - } + "max_changed_lines": 2000 } diff --git a/.github/workflows/codex_pr_review.yml b/.github/workflows/codex_pr_review.yml deleted file mode 100644 index 6d35e872..00000000 --- a/.github/workflows/codex_pr_review.yml +++ /dev/null @@ -1,156 +0,0 @@ -name: Codex PR Review - -# Runs on PRs AND can be called as a reusable workflow from other repos. -# Consumer repos use: -# uses: QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@main -# secrets: -# CODEX_AUDIT_SERVICE_URL: ${{ secrets.CODEX_AUDIT_SERVICE_URL }} -on: - pull_request_target: - types: [opened, synchronize, reopened] - workflow_call: - inputs: - caller_concurrency_key: - description: "Stable caller-side key used to cancel stale review jobs for the same PR." - required: false - type: string - allow_unconfigured_backend: - description: "Deprecated compatibility input. The review check always fails closed when no backend is available." - required: false - type: boolean - default: false - api_fallback_enabled: - description: "Optional true/false override for direct API fallback. Reusable callers default to false and do not inherit repository variables." - required: false - type: string - default: "false" - direct_api_primary_enabled: - description: "Optional true/false override for API-only PR review. Reusable callers default to false and do not inherit repository variables." - required: false - type: string - default: "false" - secrets: - CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN: - description: "Token that can read QuantStrategyLab/AIAuditBridge when this workflow is called from another private repo." - required: false - ANTHROPIC_API_KEY: - required: false - OPENAI_API_KEY: - required: false - CODEX_AUDIT_SERVICE_URL: - required: false - -permissions: - contents: read - id-token: write - issues: write - pull-requests: write - -concurrency: - group: codex-pr-review-${{ github.repository }}-${{ inputs.caller_concurrency_key || github.event.pull_request.number || github.run_id }} - cancel-in-progress: true - -jobs: - review: - runs-on: ubuntu-latest - timeout-minutes: 30 - env: - FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" - - steps: - - name: Reject unsupported fork pull requests - if: github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository - run: | - echo "::error::Codex review is not configured for fork pull requests; merge remains blocked." >&2 - exit 1 - - - name: Checkout review target - uses: actions/checkout@v6 - with: - path: source - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || github.sha }} - persist-credentials: false - - - name: Validate AIAuditBridge self-review ref - if: github.repository == 'QuantStrategyLab/AIAuditBridge' - env: - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: | - set -euo pipefail - if [ "${GITHUB_EVENT_NAME}" != "pull_request_target" ] || [ -z "${PR_HEAD_SHA}" ]; then - echo "::error::AIAuditBridge self-review requires pull_request_target with a PR head SHA." >&2 - exit 1 - fi - - - name: Checkout bridge review scripts - uses: actions/checkout@v6 - with: - repository: ${{ github.repository == 'QuantStrategyLab/AIAuditBridge' && 'QuantStrategyLab/AIAuditBridge' || job.workflow_repository }} - ref: ${{ github.repository == 'QuantStrategyLab/AIAuditBridge' && github.event.pull_request.base.sha || job.workflow_sha }} - path: bridge - token: ${{ secrets.CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN || github.token }} - persist-credentials: false - - - name: Set up Python - uses: actions/setup-python@v6 - with: - python-version: "3.11" - - - name: Run Codex PR Review - id: review - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - ANTHROPIC_MODEL: ${{ vars.ANTHROPIC_MODEL || 'claude-sonnet-4-6' }} - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} - OPENAI_MODEL: ${{ vars.OPENAI_MODEL || 'gpt-5.4-mini' }} - CODEX_AUDIT_SERVICE_URL: ${{ secrets.CODEX_AUDIT_SERVICE_URL }} - CODEX_AUDIT_SERVICE_AUDIENCE: ${{ vars.CODEX_AUDIT_SERVICE_AUDIENCE || 'quant-codex-audit' }} - CODEX_PR_REVIEW_REPO_ROOT: ${{ github.workspace }}/source - CODEX_PR_REVIEW_REUSABLE_CALL: ${{ github.repository != 'QuantStrategyLab/AIAuditBridge' && 'true' || 'false' }} - CODEX_PR_REVIEW_API_FALLBACK_INPUT: ${{ inputs.api_fallback_enabled }} - CODEX_PR_REVIEW_DIRECT_API_PRIMARY_INPUT: ${{ inputs.direct_api_primary_enabled }} - CODEX_PR_REVIEW_API_FALLBACK_DEFAULT: ${{ vars.CODEX_PR_REVIEW_API_FALLBACK_ENABLED || 'true' }} - CODEX_PR_REVIEW_DIRECT_API_PRIMARY_DEFAULT: ${{ vars.CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED || 'true' }} - working-directory: source - run: | - set -euo pipefail - bridge_script="${GITHUB_WORKSPACE}/bridge/scripts/run_codex_pr_review.py" - if [ -f "${bridge_script}" ]; then - script_path="${bridge_script}" - else - echo "::error::Trusted Codex review script not found. Ensure the bridge checkout can read QuantStrategyLab/AIAuditBridge." >&2 - exit 1 - fi - resolve_boolean() { - local name="$1" requested="$2" fallback="$3" value - if [ "${CODEX_PR_REVIEW_REUSABLE_CALL}" = "true" ]; then - # Only consumers reach this branch; their explicit/default input is authoritative. - value="${requested}" - else - # AIAuditBridge self-review has no workflow_call inputs; retain its local policy. - value="${fallback}" - fi - value="$(printf '%s' "${value}" | tr '[:upper:]' '[:lower:]')" - case "${value}" in - true|false) printf '%s' "${value}" ;; - *) echo "::error::${name} must be true or false" >&2; return 1 ;; - esac - } - if ! api_fallback_enabled="$(resolve_boolean CODEX_PR_REVIEW_API_FALLBACK_ENABLED "${CODEX_PR_REVIEW_API_FALLBACK_INPUT}" "${CODEX_PR_REVIEW_API_FALLBACK_DEFAULT}")"; then - exit 1 - fi - if ! direct_api_primary_enabled="$(resolve_boolean CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED "${CODEX_PR_REVIEW_DIRECT_API_PRIMARY_INPUT}" "${CODEX_PR_REVIEW_DIRECT_API_PRIMARY_DEFAULT}")"; then - exit 1 - fi - CODEX_PR_REVIEW_API_FALLBACK_ENABLED="${api_fallback_enabled}" \ - CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED="${direct_api_primary_enabled}" \ - timeout --signal=TERM --kill-after=60s 25m python -I "${script_path}" - - - name: Upload review diagnostics - if: always() - uses: actions/upload-artifact@v7 - with: - name: codex-pr-review-${{ github.event.pull_request.number || github.run_id }}-${{ github.run_id }} - path: source/data/output/codex_pr_review/ - if-no-files-found: warn diff --git a/README.md b/README.md index d533860c..330ef62e 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ ## What this repository is -AIAuditBridge is the QuantStrategyLab AI audit automation bridge. It runs Codex VPS/service-backed audit workflows first, with OpenAI/Anthropic API fallback for approved reviews and low-risk fix pull requests. +AIAuditBridge is the QuantStrategyLab AI audit automation bridge. It runs Codex VPS/service-backed monthly audit workflows first, with OpenAI/Anthropic API fallback for approved audits and low-risk remediation pull requests. It produces research, audit, or orchestration artifacts. It should not submit broker orders or mutate live allocations by itself. @@ -24,7 +24,7 @@ The service also exposes a structured automation triage endpoint for failure dia ## Architecture boundary -AIAuditBridge is the organization-local AI audit boundary for QuantStrategyLab. Source repositories dispatch review requests to this repository; they should not embed raw `codex exec` commands, direct provider API calls, model routing, or fallback policy themselves. +AIAuditBridge is the organization-local AI audit boundary for QuantStrategyLab. Source repositories dispatch monthly audit requests to this repository; they should not embed raw `codex exec` commands, direct provider API calls, model routing, or fallback policy themselves. Current execution model: @@ -37,7 +37,7 @@ Keep this boundary inside the `QuantStrategyLab` organization. Do not move Quant Codex execution is service-only: the workflow calls a QuantStrategyLab-owned HTTPS/443 Codex audit service from a standard GitHub-hosted runner. The service returns review text or structured patch suggestions only. AIAuditBridge still owns clone, path validation, patch application, commit, push, PR creation, and issue comments. -PR review is fail-closed: a blocking finding cannot be bypassed by a label or by retry count. Only when the same normalized finding survives a new PR head does a separate Codex arbitration pass return `clear`, `block`, or `ambiguous`. Only `clear` allows merge; `block`, `ambiguous`, and arbitration failures keep the `review` check failing. Repositories that call `AIAuditBridge` `codex_pr_review.yml@main` inherit this behavior. +GitHub PR review has one AI owner: the GitHub Codex App. AIAuditBridge does not run a second PR reviewer or publish a parallel AI-review check. The deterministic `Codex Review Gate`, source CI, unresolved-conversation protection, and branch protection remain independent fail-closed merge controls. When `CODEX_AUDIT_AUTO_MERGE=true`, the bridge requests guarded auto-merge by adding the `auto-merge-ok` label to the generated PR only after the changed-file surface is low or medium risk and the file / total changed-line caps stay within policy. The bridge ensures the configured label exists before applying it; if the source token cannot create labels, create the label manually before enabling guarded auto-merge. If a source checkout contains `.github/codex_auto_merge_policy.json`, the bridge reads the baseline policy before Codex edits run, then uses that baseline policy before falling back to its built-in defaults. High-risk, unknown, policy-changing, file-removal/rename/copy, or invalid-policy surfaces are labeled with the configured human-review label (`human-review-required` by default) instead of `auto-merge-ok`, and the source issue comment includes the risk reasons and files for operator review. The bridge does not call GitHub native auto-merge directly; source repositories must keep their own CI and merge-guard workflow in control of the final merge decision. @@ -91,17 +91,6 @@ Configure these values in `QuantStrategyLab/AIAuditBridge`: uses `task_default` to defer provider selection to the task policy. - Monthly audits with `CODEX_AUDIT_PROVIDER=auto` fall back to the configured API reviewers when the Codex service hits quota/capacity failures. -- PR review workflows can fall back to direct API review on recoverable Codex - service failures through `CODEX_PR_REVIEW_API_FALLBACK_ENABLED=true` or the - reusable workflow input `api_fallback_enabled`. The reusable workflow input - accepts string values `true`/`false`; when omitted it defers to repository - variables and then defaults to `true` for compatibility. Codex-only callers - should pass `api_fallback_enabled: "false"`. API-only PR review when no - service URL is configured is controlled separately by - `CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED` or reusable workflow input - `direct_api_primary_enabled`; this uses the same `true`/`false`, variable, - and compatibility default rules and should be set to `"false"` for Codex-only - callers. - Repository variable `CODEX_AUDIT_SERVICE_MODEL` for the VPS Codex service primary path; `VPS Codex Service Ops` deploy writes it into the systemd unit. - Optional repository variable `CODEX_AUDIT_SERVICE_REASONING_EFFORT` for a @@ -110,9 +99,9 @@ Configure these values in `QuantStrategyLab/AIAuditBridge`: - Optional service-side model routing variables: `AI_GATEWAY_LLM_LOW_COMPLEXITY_MODEL`, `AI_GATEWAY_LLM_MEDIUM_COMPLEXITY_MODEL`, and - `AI_GATEWAY_LLM_HIGH_COMPLEXITY_MODEL`. PR review callers submit - `task=pr_review` with low/medium/high complexity hints; the VPS service keeps - Codex auth local and chooses the final Codex model. + `AI_GATEWAY_LLM_HIGH_COMPLEXITY_MODEL`. Audit callers submit task-specific + low/medium/high complexity hints; the VPS service keeps Codex auth local and + chooses the final Codex model. - Optional service-side reasoning routing variables: `CODEX_AUDIT_SERVICE___REASONING_EFFORT`, `CODEX_AUDIT_SERVICE__COMPLEXITY_REASONING_EFFORT`, and diff --git a/README.zh-CN.md b/README.zh-CN.md index 2368faa6..507860ff 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -15,7 +15,7 @@ ## 这个仓库是什么 -AIAuditBridge 是 QuantStrategyLab 的 AI 审计自动化桥接工具。优先运行 Codex VPS/service-backed 审计 workflow，并对获批的 review 和低风险修复 PR 提供 OpenAI/Anthropic API fallback。 +AIAuditBridge 是 QuantStrategyLab 的 AI 审计自动化桥接工具。优先运行 Codex VPS/service-backed 月度审计 workflow，并对获批的审计和低风险修复 PR 提供 OpenAI/Anthropic API fallback。 它产出研究、审计或编排类 artifact，不应自行提交券商订单，也不应直接修改 live allocation。 @@ -24,7 +24,7 @@ AIAuditBridge 是 QuantStrategyLab 的 AI 审计自动化桥接工具。优先 ## 架构边界 -AIAuditBridge 是 QuantStrategyLab 组织内的 AI 审计调用边界。各 source repository 只负责派发审计请求，不应在自身 workflow 中直接拼接 `codex exec`、直接调用 provider API、实现模型路由或 fallback 策略。 +AIAuditBridge 是 QuantStrategyLab 组织内的 AI 审计调用边界。各 source repository 只负责派发月度审计请求，不应在自身 workflow 中直接拼接 `codex exec`、直接调用 provider API、实现模型路由或 fallback 策略。 当前执行模型： @@ -37,7 +37,7 @@ AIAuditBridge 是 QuantStrategyLab 组织内的 AI 审计调用边界。各 sour Codex 执行现在只走 service backend：workflow 从 GitHub-hosted runner 调用 QuantStrategyLab 自有的 HTTPS/443 Codex audit service。service 只返回 review 文本或结构化 patch 建议；clone、路径校验、patch apply、commit、push、PR 和 issue comment 仍由 AIAuditBridge 负责。 -PR review 采用 fail-closed：blocking finding 不可通过 label 或重试次数绕过。只有同一归一化 finding 在作者提交新 PR head 后仍然存在时，独立 Codex 仲裁才会返回 `clear`、`block` 或 `ambiguous`；只有 `clear` 放行，`block`、`ambiguous` 和仲裁失败都保持 `review` check 失败。复用 `AIAuditBridge` 的 `codex_pr_review.yml@main` 的仓库会自动继承该行为。 +GitHub PR 的 AI review 只有一个责任方：GitHub Codex App。AIAuditBridge 不再运行第二套 PR reviewer，也不发布平行的 AI review check。确定性的 `Codex Review Gate`、源仓 CI、未解决会话保护和分支保护仍各自 fail-closed。 当 `CODEX_AUDIT_AUTO_MERGE=true` 时，bridge 会先检查变更文件面和总增删行数，只在低风险或中风险且未超过 policy 上限时给生成的 PR 添加 `auto-merge-ok` label，请求源仓库的受控自动合并。bridge 会在打标前按需创建配置的 label；如果源仓 token 没有创建 label 的权限，需要先手动创建该 label。若 source checkout 里存在 `.github/codex_auto_merge_policy.json`，bridge 会在 Codex 执行修改前读取基线策略，否则才使用内置默认值。高风险、未知文件面、策略文件变更、文件移除/重命名/复制或无效 policy 配置不会添加 `auto-merge-ok`，而是给 PR 添加配置的人工复核 label（默认 `human-review-required`），并在源 issue 评论中列出风险原因和文件，等待人工复核。bridge 不会直接调用 GitHub native auto-merge。最终是否合并仍由源仓库自己的 CI 和 merge-guard workflow 决定。 @@ -89,15 +89,6 @@ AIAuditBridge 只使用 service backend。workflow 运行在 `ubuntu-latest`， 把 provider 选择交给 task policy。 - repository variable `CODEX_AUDIT_SERVICE_MODEL`，VPS Codex service 主路径模型； `VPS Codex Service Ops` deploy 会写入 systemd unit。 -- PR review 可以通过 repository variable - `CODEX_PR_REVIEW_API_FALLBACK_ENABLED=true` 或 reusable workflow input - `api_fallback_enabled` 在可恢复的 Codex service 失败后启用 direct API - fallback。reusable workflow input 使用字符串 `true`/`false`；省略时先使用 - repository variable，再为兼容旧调用方默认 `true`。只走 Codex 的调用方应传入 - `api_fallback_enabled: "false"`。当未配置 service URL 时，是否允许 API-only - PR review 由 `CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED` 或 reusable - workflow input `direct_api_primary_enabled` 单独控制；该项使用同样的变量和 - 兼容默认规则，Codex-only 调用方应设为 `"false"`。 - workflow 已配置 `id-token: write`，用于向 service 提供 GitHub Actions OIDC token。 service host 启动示例： diff --git a/client/gateway_client.py b/client/gateway_client.py index a341e880..ec7d06ea 100644 --- a/client/gateway_client.py +++ b/client/gateway_client.py @@ -141,8 +141,10 @@ def execute( self, prompt: str, *, + task: str = "execute", mode: str = "review_only", model: str | None = None, + complexity: str = "", source_repository: str | None = None, source_ref: str = "main", timeout: float | None = None, @@ -157,10 +159,11 @@ def execute( try: token = _fetch_oidc_token(self.config.audience) payload = json.dumps({ - "task": "execute", + "task": task, "prompt": prompt, "mode": mode, "model": model or self.config.default_execute_model, + "complexity": complexity, "source_repository": source_repository or self.config.source_repository, "source_ref": source_ref, "timeout_seconds": int(timeout), diff --git a/docs/ai_autonomy_architecture.md b/docs/ai_autonomy_architecture.md index a36d7eae..70044706 100644 --- a/docs/ai_autonomy_architecture.md +++ b/docs/ai_autonomy_architecture.md @@ -32,7 +32,7 @@ AIAuditBridge 是 QuantStrategyLab 的 AI 审计控制面，负责： -- 接收源仓库的月度审计 / PR review 请求； +- 接收源仓库的月度审计请求； - 通过 GitHub Actions OIDC 认证来源； - 克隆源仓库并构造上下文； - 调用 Codex service，必要时回退到 OpenAI / Anthropic API； @@ -48,18 +48,12 @@ AIAuditBridge 是 QuantStrategyLab 的 AI 审计控制面，负责： - 使用 `CODEX_AUDIT_SERVICE_URL` 指向服务端。 - 支持 guarded auto-merge。 -- `codex_pr_review.yml` - - 处理 PR review。 - - 支持 Codex service + 直接 API fallback。 - - 通过中央 Contract Oscillation Guard 保存受限的 blocking finding 历史并仲裁契约冲突。 - - 上传诊断 artifact。 - - `codex_review_gate.yml` - 只执行确定性的 secret / path / metadata 静态门禁。 - 使用受信任 base 代码检查 PR diff，并通过 Checks API 把 `Codex Review Gate` 明确发布到 current head SHA；API 失败时 fail closed。 - - Codex connector 的原生 GitHub review 与 unresolved threads 仅作为非 required - advisory evidence，不再镜像成仓库自建 check。 + - GitHub Codex App 是唯一 AI PR reviewer；AIAuditBridge 不再运行第二套 reviewer。 + - Codex App review 与 unresolved threads 不再镜像成仓库自建 AI check。 - `monthly-orchestrator.yml` - 生成月度审计 issue。 @@ -100,10 +94,6 @@ AIAuditBridge 是 QuantStrategyLab 的 AI 审计控制面，负责： - 月审主流程。 - 包括 repo/task 校验、service patch contract、path guard、PR 创建、label 管理、auto-merge 请求、stale label cleanup。 -- `scripts/run_codex_pr_review.py` - - PR review 主流程。 - - service 失败时可按条件回退到 API review。 - - `scripts/gate_codex_app_review.py` - 以 current-head 静态 check 的形式保护合并；不处理 AI review verdict。 @@ -203,37 +193,17 @@ AIAuditBridge 是 QuantStrategyLab 的 AI 审计控制面，负责： - merge queue / required checks； - 失败后的 retrigger 逻辑。 -#### 3.3.1 Contract Oscillation Guard - -Contract Oscillation Guard 是 `AIAuditBridge` 的中央 PR review gate 语义，不是要求每个消费者仓库新增一套 branch rule。消费者仍使用原有 required check、branch protection 和 merge queue；guard 不提供 label、admin 或人工确认绕过。 - -trusted review comment 只保存最近固定轮数、固定字节上限且脱敏后的 blocking finding 摘要，包括 head SHA、file、category、severity、description 和 suggestion。历史只能由已验证的 review bot comment 恢复；legacy comment 没有 history marker 时保持兼容，但既有 blocker 会被迁移为 `invalid_history` 并继续 fail closed，不能因一次 clean review 自动清除。畸形或超限 history 同样 fail closed。 - -若 `overflow` / `invalid_history` 状态中没有可供仲裁的 trusted prior finding，系统不得用空上下文自动 `clear`。此时需要人工确认 source-of-truth 后修复或删除损坏的 trusted bot state，再重新运行普通 required review check；这只恢复可审计状态，不直接放行 merge，也不绕过 branch protection。 - -当同一 file/category/severity 的前后 finding 可能要求相反行为时，独立仲裁必须同时读取上一轮 finding、当前 finding 和累计 PR diff，并优先以公共接口、schema、tests、docs 等 source-of-truth 判断： - -- source-of-truth 足以证明当前 finding 为 false positive 时，仲裁可 `clear`； -- 当前 finding 有明确契约依据时保持 `block`； -- 证据不足、结果 ambiguous 或仲裁失败时继续 blocked。 - -一旦确认或无法排除 contract conflict，结构化结果固定为 `contract_conflict=true`、`auto_fix_allowed=false`、`next_action=contract_arbitration`，禁止自动 remediation 继续反向修改代码。系统只要求一次人工契约确认；确认应落到公共接口、schema、tests 或 docs 的明确变更后，再由普通 review/check 链路重新验证，而不是绕过 gate。 - -`verdict=clear` 表示 source-of-truth 已证明当前 finding 为 false positive，因此 required review check 可以通过；即使历史上检测到 `contract_conflict=true`，仍保持 `auto_fix_allowed=false`，防止执行线程继续改代码。这是对错误 finding 的独立仲裁结论，不是绕过 branch protection。`block`、`ambiguous` 或仲裁失败才必须继续 blocked。 - -已 `cleared` 的 finding key 是历史匹配边界，不得继续回溯并复活更旧的同 key blocker；未被 clear 的多个 current finding key 则必须从最近历史轮分别聚合后统一交给仲裁，不能只取第一个命中的 round。 - -#### 3.3.2 Blocking finding 的可达性证据 +#### 3.3.1 单一 PR reviewer 边界 -repository Review 只有在 PR 上下文能够同时证明 exact changed path/line、当前 caller/entry point（可为既有路径或本 PR 新增路径）或明确声明的 public untrusted boundary、当前配置和输入下可达，以及具体 correctness/security/data-integrity 影响时，才能给出 `critical` 或 `high`。证据不足的 finding 必须降为 `medium/low` 或省略，不能依靠 future consumer、伪造内部对象或通用 defense-in-depth 推测阻塞当前 PR。 +GitHub Codex App 是唯一 AI PR reviewer。AIAuditBridge 只保留月度审计和低风险修复职责，不维护第二套 review verdict、finding 历史、重试或仲裁状态。 -Review 不得为了 hypothetical 风险要求新增 parser、store、registry 或 event-persistence 层。只有当前变更已经暴露对应真实边界，而且缺陷能从该边界到达时，才允许提出此类修改建议。 +合并仍必须同时满足源仓 CI、确定性 `Codex Review Gate`、未解决会话保护和 branch protection。任何自动化都不得用 label、admin 或自建 AI check 绕过这些控制。 ### P1：强烈建议补的缺口 #### 3.4 缺少统一的任务状态机 -月度审计、PR review、修复、重试、回退、人工升级，这些状态现在是靠脚本和 GitHub 流程串起来的。 +月度审计、修复、重试、回退、人工升级，这些状态现在是靠脚本和 GitHub 流程串起来的。 建议显式建模： @@ -335,7 +305,7 @@ Review 不得为了 hypothetical 风险要求新增 parser、store、registry 建议动作： -- 把每次月审 / PR review / auto-fix 的结果持久化； +- 把每次月审 / auto-fix 的结果持久化； - 记录：问题类型、provider、模型、风险级别、是否需要人工、是否 merge 成功、是否复发； - 在 dashboard 上展示： - 自动处理成功率； @@ -399,7 +369,7 @@ Review 不得为了 hypothetical 风险要求新增 parser、store、registry ### 可以放心推进的部分 - 月度审计 issue 的生成与调度； -- 低风险 review / 修复； +- 低风险审计 / 修复； - 受控 auto-merge 请求； - 服务健康与 quota 监控； - 失败后 fallback 和 retry。 diff --git a/prompts/pr_review.md b/prompts/pr_review.md deleted file mode 100644 index 7c79bef1..00000000 --- a/prompts/pr_review.md +++ /dev/null @@ -1,59 +0,0 @@ -You are reviewing a pull request for a **production quantitative trading and data pipeline codebase**. - -## Review priorities (in order) - -1. **Security**: credential leaks, injection vectors, unauthorized data access -2. **Correctness**: logic errors, wrong calculations, data corruption -3. **Crash risks**: unhandled exceptions, null pointer dereferences, resource exhaustion -4. **Data integrity**: silent data loss, incorrect transformations, schema violations -5. **API compatibility**: breaking changes to function signatures, configuration formats -6. **Race conditions**: concurrent access to shared state, inconsistent reads - -## What NOT to flag - -- Code style or formatting preferences -- Variable/function naming suggestions -- Missing type annotations -- Documentation quality -- Minor refactoring opportunities -- Test coverage suggestions - -## Review completeness - -- Assign **critical** or **high** only when the supplied PR context proves an exact changed path/line, a current caller or entry point proven by the supplied PR context whether pre-existing or introduced by this PR or an explicitly declared public untrusted boundary, reachability under current configuration and inputs, and concrete correctness, security, or data-integrity impact. State the reachability and impact in the description. If any element is missing, downgrade it to medium or low or omit it. -- Do not block on a hypothetical future consumer, forged internal object state, or generic defense-in-depth concern. Do not request a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary and the defect is reachable through it. -- Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. -- Review only the supplied current exact-head diff. Do not inherit findings, fingerprints, retry counts, or arbitration state from any prior head. -- Do not invent backward-compatibility requirements that are absent from the repository and PR contract. If both explicitly define a clean-slate namespace, check for accidental legacy fallback instead of requesting dual-read or migration. This never overrides security or data-integrity findings. -- Only for a public JSON/wire contract proven by the reachability rule above, check optional-key presence versus explicit null, recursive JSON-safe types, every identity-bearing integer range, one canonical timestamp representation, deterministic round-trips and digests, immutability, and identifier/path safety. - -## Severity definitions - -| Severity | Definition | Example | -|----------|-----------|---------| -| critical | Causes data loss, security breach, or production crash | SQL injection, credential in plaintext, deletion without backup | -| high | Produces wrong results or breaks downstream systems | Wrong formula, API signature change, resource leak | -| medium | Degrades reliability or performance under load | Missing error handling, N+1 query, unbounded growth | -| low | Misleading or confusing but not dangerous | Stale comment, redundant code, unclear intent | - -## Output format - -Return exactly one JSON object (do not wrap in markdown fences): - -```json -{ - "summary": "Brief assessment of the PR (1-3 sentences)", - "findings": [ - { - "severity": "critical", - "category": "security", - "file": "path/to/file.py", - "line": 42, - "description": "What's wrong", - "suggestion": "How to fix it" - } - ] -} -``` - -If no issues found, return `"findings": []`. diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py deleted file mode 100644 index 6fc5419b..00000000 --- a/scripts/run_codex_pr_review.py +++ /dev/null @@ -1,1244 +0,0 @@ -#!/usr/bin/env python3 -"""Run Codex review on a PR diff and block merge when serious issues are found. - -Uses the existing Codex audit service backend (same as monthly reviews). -Evaluates findings against the repo's codex_auto_merge_policy.json. -Exits non-zero when blocked, which fails the GitHub Actions check run. -""" - -from __future__ import annotations - -import base64 -import hashlib -import json -import os -import re -import sys -import time -import urllib.error -import urllib.parse -import urllib.request -from pathlib import Path -from string import Template -from typing import Any - -# --------------------------------------------------------------------------- -# Configuration (aligned with CodexAuditBridge) -# --------------------------------------------------------------------------- - -API_BASE = "https://api.github.com" -BRIDGE_ROOT = Path(__file__).resolve().parents[1] -ROOT = Path(os.environ.get("CODEX_PR_REVIEW_REPO_ROOT") or os.environ.get("GITHUB_WORKSPACE") or Path.cwd()).resolve() -if str(BRIDGE_ROOT) not in sys.path: - sys.path.insert(0, str(BRIDGE_ROOT)) - -POLICY_PATH = ROOT / ".github" / "codex_auto_merge_policy.json" -PROMPT_TEMPLATE_PATH = BRIDGE_ROOT / "prompts" / "pr_review.md" -DEFAULT_SERVICE_AUDIENCE = "quant-codex-audit" -DEFAULT_TIMEOUT_MINUTES = 20 -DEFAULT_MAX_CONTEXT_LINES = 800 -TASK_COMPLEXITY_LOW = "low" -TASK_COMPLEXITY_MEDIUM = "medium" -TASK_COMPLEXITY_HIGH = "high" -TASK_COMPLEXITY_LEVELS = (TASK_COMPLEXITY_LOW, TASK_COMPLEXITY_MEDIUM, TASK_COMPLEXITY_HIGH) -CODEX_SERVICE_FALLBACK_SIGNALS = ( - "429", - "too many requests", - "rate limit", - "quota", - "codex exec failed", -) -NO_REVIEW_BACKEND_CONFIGURED = ( - "No Codex service URL or API key configured. " - "Set CODEX_AUDIT_SERVICE_URL, ANTHROPIC_API_KEY, or OPENAI_API_KEY." -) - -# Risk → block mapping -BLOCK_SEVERITIES = frozenset({"critical", "high"}) -COMMENT_SEVERITIES = frozenset({"critical", "high", "medium", "low"}) -HEAD_SHA_MARKER_PREFIX = "" -REVIEW_COMPLETED_MARKER_PREFIX = "" -DECISION_MARKER_SUFFIX = " -->" - - -class ReviewError(RuntimeError): - pass - - -# --------------------------------------------------------------------------- -# GitHub API helpers -# --------------------------------------------------------------------------- - - -def github_request( - token: str, method: str, path: str, payload: dict[str, Any] | None = None -) -> Any: - url = path if path.startswith("https://") else f"{API_BASE}{path}" - data = json.dumps(payload).encode("utf-8") if payload is not None else None - req = urllib.request.Request( - url, - data=data, - method=method, - headers={ - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github+json", - "Content-Type": "application/json", - "X-GitHub-Api-Version": "2022-11-28", - "User-Agent": "codex-pr-review", - }, - ) - try: - with urllib.request.urlopen(req, timeout=30) as resp: - body = resp.read().decode("utf-8") - except urllib.error.HTTPError as exc: - body = exc.read().decode("utf-8", errors="replace") - raise ReviewError(f"GitHub API {method} {url} failed: {exc.code} {body[:600]}") from exc - return json.loads(body) if body else {} - - -def env_value(name: str, default: str = "") -> str: - return os.environ.get(name, default).strip() - - -def parse_bool(value: str | bool | None) -> bool: - if isinstance(value, bool): - return value - return str(value or "").strip().lower() in {"1", "true", "yes", "y", "on"} - - -# --------------------------------------------------------------------------- -# Policy loading (reuses evaluate_codex_pr_merge.py logic) -# --------------------------------------------------------------------------- - - -def load_policy(token: str = "", repo: str = "", base_ref: str = "") -> dict[str, Any]: - """Load the risk policy, falling back to safe defaults.""" - if token and repo and base_ref: - return _load_policy_from_trusted_ref(token, repo, base_ref) - - if not POLICY_PATH.exists(): - return _default_policy() - - try: - payload = json.loads(POLICY_PATH.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): - return _fail_closed("invalid auto-merge policy JSON") - - return _validate_policy_payload(payload) - - -def _load_policy_from_trusted_ref(token: str, repo: str, ref: str) -> dict[str, Any]: - path = urllib.parse.quote(".github/codex_auto_merge_policy.json", safe="") - ref_query = urllib.parse.quote(ref, safe="") - try: - payload = github_request(token, "GET", f"/repos/{repo}/contents/{path}?ref={ref_query}") - except ReviewError as exc: - if "failed: 404" in str(exc): - return _default_policy() - return _fail_closed("could not load trusted auto-merge policy") - if not isinstance(payload, dict): - return _fail_closed("invalid trusted auto-merge policy response") - try: - encoded = str(payload.get("content") or "") - raw = base64.b64decode(encoded).decode("utf-8") - policy = json.loads(raw) - except (ValueError, json.JSONDecodeError): - return _fail_closed("invalid trusted auto-merge policy JSON") - return _validate_policy_payload(policy) - - -def _validate_policy_payload(payload: Any) -> dict[str, Any]: - if not isinstance(payload, dict): - return _fail_closed("invalid auto-merge policy format") - if payload.get("version") != 1: - return _fail_closed("unsupported policy version") - return payload - - -def _default_policy() -> dict[str, Any]: - return { - "version": 1, - "blocked_path_patterns": [ - r"(^|/)(\.env|.*secret.*|.*credential.*|.*token.*|.*private.*|.*\.pem|.*\.key)$", - ], - "risk_policy": { - "low": { - "prefixes": ["docs/", "tests/"], - "exact": ["README.md", "README.zh-CN.md"], - "reason": "docs/tests/readme-only changes", - }, - "high": {"reason": "source code changes require review"}, - }, - "max_changed_files": 30, - "max_changed_lines": 2000, - "pr_review": {}, - } - - -def _fail_closed(reason: str) -> dict[str, Any]: - return { - "policy_errors": [reason], - "blocked_path_patterns": [r".*"], - "risk_policy": { - "low": {"prefixes": [], "exact": [], "reason": reason}, - "high": {"reason": reason}, - }, - } - - -# --------------------------------------------------------------------------- -# File risk classification -# --------------------------------------------------------------------------- - - -def classify_file_risk( - file_path: str, policy: dict[str, Any] -) -> tuple[str, str]: - """Return (risk_level, reason) for a single file path.""" - policy.get("policy_errors", []) - - # Blocked patterns (secrets, credentials, etc.) - blocked_patterns = policy.get("blocked_path_patterns", []) - for pattern in blocked_patterns: - try: - if re.search(pattern, file_path, re.IGNORECASE): - return ("high", f"matches blocked path pattern: {pattern}") - except re.error: - continue - - risk_policy = policy.get("risk_policy", {}) - low = risk_policy.get("low", {}) - low_prefixes = low.get("prefixes", []) - low_exact = set(low.get("exact", [])) - medium_exact = set( - risk_policy.get("medium", {}).get("exact", []) - ) - - # Normalize path - normalized = file_path.strip() - while normalized.startswith("./"): - normalized = normalized[2:] - - if not normalized: - return ("high", "empty path") - - if normalized in low_exact or any( - normalized.startswith(prefix) for prefix in low_prefixes - ): - return ("low", "docs/test/readme change") - - if normalized in medium_exact: - return ("medium", "monthly-review helper changed") - - return ("high", "source code change") - - -def changed_files_are_low_risk(paths: list[str], policy: dict[str, Any]) -> bool: - """Return True when every changed path is low-risk under the policy.""" - return bool(paths) and all(classify_file_risk(path, policy)[0] == TASK_COMPLEXITY_LOW for path in paths) - - -# --------------------------------------------------------------------------- -# PR diff fetching -# --------------------------------------------------------------------------- - - -def fetch_pr_diff(token: str, repo: str, pr_number: int) -> str: - """Fetch the unified diff for a PR.""" - diff_url = f"{API_BASE}/repos/{repo}/pulls/{pr_number}" - req = urllib.request.Request( - diff_url, - method="GET", - headers={ - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github.v3.diff", - "X-GitHub-Api-Version": "2022-11-28", - "User-Agent": "codex-pr-review", - }, - ) - try: - with urllib.request.urlopen(req, timeout=30) as resp: - return resp.read().decode("utf-8", errors="replace") - except urllib.error.HTTPError as exc: - body = exc.read().decode("utf-8", errors="replace") - raise ReviewError(f"Failed to fetch PR diff: {exc.code} {body[:600]}") from exc - - -def fetch_pr_files(token: str, repo: str, pr_number: int) -> list[dict[str, Any]]: - """Fetch the list of changed files in a PR.""" - files: list[dict[str, Any]] = [] - page = 1 - while True: - payload = github_request( - token, - "GET", - f"/repos/{repo}/pulls/{pr_number}/files?per_page=100&page={page}", - ) - if not isinstance(payload, list) or not payload: - break - files.extend(payload) - if len(payload) < 100: - break - page += 1 - return files - - -# --------------------------------------------------------------------------- -# Review prompt -# --------------------------------------------------------------------------- - - -def build_review_prompt(diff: str, pr_title: str, pr_body: str, repo: str) -> str: - """Build the Codex review prompt with the PR diff and structured output instructions.""" - diff_limited = _truncate_lines(diff, DEFAULT_MAX_CONTEXT_LINES * 3) - - template = Template( - """You are reviewing a pull request for a production codebase. Your job is to find bugs, security issues, and logic errors that could cause real problems. - -## PR Context - -- Repository: ${REPO} -- PR Title: ${TITLE} - -${BODY} - -## Review Instructions - -1. Focus on **security vulnerabilities, logic errors, data corruption, crash bugs, race conditions, and API compatibility breaks**. -2. Do NOT flag: code style, formatting, naming suggestions, minor refactoring preferences, or documentation issues. -3. Do not emit a finding that concludes no code change is needed. For OIDC, `job_workflow_ref` is absent for explicit direct callers; flag a bypass only when a non-direct repository can reach the direct-caller path despite the allowlists. -4. Assign **critical** or **high** only when the supplied PR context proves all of the following: an exact changed path and line; a current caller or entry point proven by the supplied PR context, whether pre-existing or introduced by this PR, or an explicitly declared public untrusted boundary; reachability under the current configuration and inputs; and a concrete correctness, security, or data-integrity impact. State that reachability and impact in the finding description. If any element is missing, downgrade it to medium or low or omit it. -5. Do not block on a hypothetical future consumer, forged internal object state, or generic defense-in-depth concern. Do not request a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary and the defect is reachable through it. -6. Review the entire diff holistically and report all independent actionable findings in one response. Do not stop after the first blocking issue. -7. Review only the supplied current exact-head diff. Do not inherit findings, fingerprints, retry counts, or arbitration state from any prior head. -8. Do not invent backward-compatibility requirements that are absent from the repository and PR contract. When the repository and PR explicitly define a clean-slate namespace with legacy compatibility out of scope, review that boundary for accidental fallback instead of requesting dual-read or migration. This never overrides security or data-integrity findings. -9. Only for a public JSON/wire contract proven by rule 4, check optional-key presence versus explicit null, recursive JSON-safe types, every identity-bearing integer range, one canonical timestamp representation, deterministic round-trips and digests, immutability, and identifier/path safety. -10. For each finding, classify its severity: - - **critical**: security vulnerability, data loss, production crash - - **high**: logic error that produces wrong results, API break, memory/connection leak - - **medium**: missing error handling, performance degradation, race condition - - **low**: misleading comment, unclear variable name, redundant code - -## Output Format - -Return exactly one JSON object and no surrounding prose: - -```json -{ - "summary": "Brief summary of the review (1-3 sentences)", - "findings": [ - { - "severity": "critical|high|medium|low", - "category": "security|bug|performance|logic|reliability", - "file": "relative/path/to/file.py", - "line": 42, - "description": "What the problem is", - "suggestion": "How to fix it" - } - ] -} -``` - -If there are no findings, return an empty `findings` array. - -## PR Diff - -```diff -${DIFF} -```""" - ) - - return template.safe_substitute( - REPO=repo, - TITLE=pr_title, - BODY=f"### PR Description\n\n{pr_body}" if pr_body.strip() else "", - DIFF=diff_limited, - ) - - -def review_implementation_digest() -> str: - """Return the identity of the trusted bridge implementation that reviews a PR.""" - digest = hashlib.sha256() - for path in (Path(__file__), PROMPT_TEMPLATE_PATH): - digest.update(path.read_bytes()) - return digest.hexdigest()[:24] - - -def _truncate_lines(text: str, max_lines: int) -> str: - lines = text.splitlines() - if len(lines) <= max_lines: - return text - half = max_lines // 2 - return ( - "\n".join(lines[:half]) - + f"\n\n... [{len(lines) - max_lines} lines truncated] ...\n\n" - + "\n".join(lines[-half:]) - ) - - -def _normalize_complexity(value: str) -> str: - normalized = (value or "").strip().lower() - if normalized in TASK_COMPLEXITY_LEVELS: - return normalized - return "" - - -def _estimate_review_complexity( - diff: str, - changed_files: list[str], - *, - title: str = "", - body: str = "", -) -> str: - diff_lines = len((diff or "").splitlines()) - file_count = len([f for f in changed_files if f]) - prompt_chars = len((diff or "")) + len((title or "")) + len((body or "")) - - if diff_lines >= 1800 or file_count >= 15 or prompt_chars >= 18000: - return TASK_COMPLEXITY_HIGH - if diff_lines >= 600 or file_count >= 6 or prompt_chars >= 7000: - return TASK_COMPLEXITY_MEDIUM - return TASK_COMPLEXITY_LOW - - -def _direct_api_model_for_complexity(provider: str, complexity: str) -> str: - level = _normalize_complexity(complexity) - if not level: - return "" - prefix = "ANTHROPIC" if provider == "anthropic" else "OPENAI" - for name in ( - f"CODEX_AUDIT_{prefix}_{level.upper()}_COMPLEXITY_MODEL", - f"{prefix}_{level.upper()}_COMPLEXITY_MODEL", - f"{prefix}_MODEL_{level.upper()}", - ): - value = env_value(name) - if value: - return value - return "" - - -# --------------------------------------------------------------------------- -# Codex service integration -# --------------------------------------------------------------------------- - - -def request_github_oidc_token(audience: str) -> str: - request_url = env_value("ACTIONS_ID_TOKEN_REQUEST_URL") - request_token = env_value("ACTIONS_ID_TOKEN_REQUEST_TOKEN") - if not request_url or not request_token: - raise ReviewError( - "GitHub OIDC environment unavailable. Set permissions: id-token: write." - ) - separator = "&" if "?" in request_url else "?" - url = f"{request_url}{separator}audience={urllib.parse.quote(audience)}" - req = urllib.request.Request( - url, - method="GET", - headers={ - "Authorization": f"Bearer {request_token}", - "Accept": "application/json", - "User-Agent": "codex-pr-review-oidc", - }, - ) - with urllib.request.urlopen(req, timeout=30) as resp: - payload = json.loads(resp.read().decode("utf-8")) - token = payload.get("value") if isinstance(payload, dict) else None - if not isinstance(token, str) or not token: - raise ReviewError("GitHub OIDC token response missing token value") - return token - - -def run_codex_service_review(prompt: str, timeout_minutes: int, complexity: str = "", changed_file_count: int = 0, changed_line_count: int = 0) -> str: - """Submit a review job to the Codex audit service and wait for completion.""" - service_url = env_value("CODEX_AUDIT_SERVICE_URL") - if not service_url: - raise ReviewError("CODEX_AUDIT_SERVICE_URL is not configured") - - service_url = service_url.strip().rstrip("/") - audience = env_value("CODEX_AUDIT_SERVICE_AUDIENCE", DEFAULT_SERVICE_AUDIENCE) - - # Submit job - oidc_token = request_github_oidc_token(audience) - payload = { - "source_repository": env_value("GITHUB_REPOSITORY"), - "source_ref": env_value("GITHUB_REF_NAME", "main"), - "task": "pr_review", - "mode": "review_only", - "prompt": prompt, - # The VPS owns the Codex CLI model configuration. Its configured - # model is validated at deployment time; forwarding an API catalog - # model here can select one unavailable to the CLI account. - "complexity": _normalize_complexity(complexity) or "auto", - "changed_files": int(changed_file_count), - "changed_lines": int(changed_line_count), - "timeout_seconds": timeout_minutes * 60, - } - submit_resp = _service_request( - "POST", - f"{service_url}/v1/codex-audit/jobs", - oidc_token, - payload, - ) - job_id = submit_resp.get("job_id") - if not isinstance(job_id, str) or not job_id: - raise ReviewError("Codex service did not return a job id") - - # Poll for completion - deadline = time.time() + timeout_minutes * 60 + 120 - poll_interval = 5 - job_url = f"{service_url}/v1/codex-audit/jobs/{job_id}" - while time.time() < deadline: - time.sleep(poll_interval) - poll_interval = min(poll_interval * 2, 30) - job_payload = _service_request("GET", job_url, request_github_oidc_token(audience), None) - status = job_payload.get("status") - if status == "succeeded": - output = job_payload.get("output") - if not isinstance(output, str): - raise ReviewError("Codex service response missing text output") - return output.strip() - if status == "failed": - error = str(job_payload.get("error") or "unknown failure") - failure_category = str(job_payload.get("failure_category") or "").strip() - category_suffix = f" [{failure_category}]" if failure_category else "" - raise ReviewError(f"Codex service job failed{category_suffix}: {error[:600]}") - if status not in {"queued", "running"}: - raise ReviewError(f"Unexpected Codex service status: {status!r}") - - raise ReviewError("Codex service job timed out") - - -def _service_review_should_fallback(exc: ReviewError) -> bool: - message = str(exc).lower() - return any(signal in message for signal in CODEX_SERVICE_FALLBACK_SIGNALS) - - -def _review_backend_is_unconfigured(exc: ReviewError) -> bool: - message = str(exc).strip() - normalized = message.lower() - return message == NO_REVIEW_BACKEND_CONFIGURED or "oidc repository is not allowed" in normalized - - -def _review_capacity_is_unavailable(exc: ReviewError) -> bool: - message = str(exc).lower() - return ( - "[quota_or_capacity_failure]" in message - or "usage limit" in message - or "daily budget exceeded" in message - or "codex service job failed [unknown_failure]: codex exec failed" in message - ) - - -def _api_fallback_enabled() -> bool: - return parse_bool(env_value("CODEX_PR_REVIEW_API_FALLBACK_ENABLED", "true")) - - -def _direct_api_primary_enabled() -> bool: - return parse_bool(env_value("CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED", "true")) - - -def run_codex_review_with_fallback( - prompt: str, - timeout_minutes: int, - complexity: str = "", - changed_file_count: int = 0, - changed_line_count: int = 0, -) -> str: - service_url = env_value("CODEX_AUDIT_SERVICE_URL") - service_failure: Exception | None = None - if service_url: - try: - print(f"Running Codex review via service: {service_url}") - return run_codex_service_review( - prompt, - timeout_minutes, - complexity=complexity, - changed_file_count=changed_file_count, - changed_line_count=changed_line_count, - ) - except ReviewError as exc: - if not _service_review_should_fallback(exc): - raise - service_failure = exc - print(f"::warning::Codex service review failed: {exc}") - except (json.JSONDecodeError, OSError, urllib.error.URLError) as exc: - service_failure = exc - print(f"::error::Codex service review failed: {exc}") - - if service_failure is not None and not _api_fallback_enabled(): - raise ReviewError(f"Codex service review failed and direct API fallback is disabled: {service_failure}") - if not service_url and not _direct_api_primary_enabled(): - raise ReviewError(NO_REVIEW_BACKEND_CONFIGURED) - - print("Running Codex review via direct API") - try: - return run_direct_api_review(prompt, complexity=complexity) - except ReviewError as exc: - if service_failure is not None and _review_backend_is_unconfigured(exc): - raise ReviewError( - f"Codex service review failed and no direct API fallback is configured: {service_failure}" - ) from exc - raise - - -def _service_request( - method: str, url: str, oidc_token: str, payload: dict[str, Any] | None -) -> dict[str, Any]: - data = json.dumps(payload).encode("utf-8") if payload is not None else None - req = urllib.request.Request( - url, - data=data, - method=method, - headers={ - "Authorization": f"Bearer {oidc_token}", - "Content-Type": "application/json", - "Accept": "application/json", - "User-Agent": "codex-pr-review-client", - }, - ) - try: - with urllib.request.urlopen(req, timeout=60) as resp: - body = resp.read().decode("utf-8") - except urllib.error.HTTPError as exc: - detail = exc.read().decode("utf-8", errors="replace") - raise ReviewError(f"Codex service request failed: {exc.code} {detail[:600]}") from exc - result = json.loads(body) - if not isinstance(result, dict): - raise ReviewError("Codex service returned invalid JSON") - return result - - -# --------------------------------------------------------------------------- -# Direct API review (fallback when service is unavailable) -# --------------------------------------------------------------------------- - - -def run_direct_api_review(prompt: str, complexity: str = "") -> str: - """Run review directly via Anthropic or OpenAI API.""" - anthropic_key = env_value("ANTHROPIC_API_KEY") - openai_key = env_value("OPENAI_API_KEY") - - provider_order = [ - "openai", - "anthropic", - ] - normalized = _normalize_complexity(complexity) - if normalized in (TASK_COMPLEXITY_HIGH, TASK_COMPLEXITY_MEDIUM): - provider_order = ["anthropic", "openai"] - - for provider in provider_order: - if provider == "anthropic" and anthropic_key: - return _run_anthropic_review( - prompt, - anthropic_key, - model=_direct_api_model_for_complexity(provider, normalized), - ) - if provider == "openai" and openai_key: - return _run_openai_review( - prompt, - openai_key, - model=_direct_api_model_for_complexity(provider, normalized), - ) - - raise ReviewError(NO_REVIEW_BACKEND_CONFIGURED) - - -def _run_anthropic_review(prompt: str, api_key: str, model: str = "") -> str: - model = env_value("ANTHROPIC_MODEL", "claude-sonnet-4-6") if not model else model - system = "You are a careful code reviewer. Return only the JSON object as specified." - payload = { - "model": model, - "max_tokens": 4000, - "system": system, - "messages": [{"role": "user", "content": prompt}], - } - req = urllib.request.Request( - "https://api.anthropic.com/v1/messages", - data=json.dumps(payload).encode("utf-8"), - method="POST", - headers={ - "x-api-key": api_key, - "anthropic-version": "2023-06-01", - "Content-Type": "application/json", - "User-Agent": "codex-pr-review", - }, - ) - try: - with urllib.request.urlopen(req, timeout=120) as resp: - body = json.loads(resp.read().decode("utf-8")) - except urllib.error.HTTPError as exc: - detail = exc.read().decode("utf-8", errors="replace") - raise ReviewError(f"Anthropic API failed: {exc.code} {detail[:600]}") from exc - - content = body.get("content", []) - if not isinstance(content, list): - raise ReviewError("Unexpected Anthropic response format") - text_parts = [ - str(block.get("text", "")) - for block in content - if isinstance(block, dict) and block.get("type") == "text" - ] - return "\n\n".join(text_parts) - - -def _run_openai_review(prompt: str, api_key: str, model: str = "") -> str: - model = env_value("OPENAI_MODEL", "gpt-5.4-mini") if not model else model - system = "You are a careful code reviewer. Return only the JSON object as specified." - payload = { - "model": model, - "messages": [ - {"role": "system", "content": system}, - {"role": "user", "content": prompt}, - ], - } - req = urllib.request.Request( - f"{env_value('OPENAI_API_BASE_URL', 'https://api.openai.com/v1').rstrip('/')}/chat/completions", - data=json.dumps(payload).encode("utf-8"), - method="POST", - headers={ - "Authorization": f"Bearer {api_key}", - "Content-Type": "application/json", - "User-Agent": "codex-pr-review", - }, - ) - try: - with urllib.request.urlopen(req, timeout=120) as resp: - body = json.loads(resp.read().decode("utf-8")) - except urllib.error.HTTPError as exc: - detail = exc.read().decode("utf-8", errors="replace") - raise ReviewError(f"OpenAI API failed: {exc.code} {detail[:600]}") from exc - - choices = body.get("choices", []) - if not isinstance(choices, list) or not choices: - raise ReviewError("Unexpected OpenAI response format") - message = choices[0].get("message", {}) - return str(message.get("content", "")) - - -# --------------------------------------------------------------------------- -# Response parsing -# --------------------------------------------------------------------------- - - -def parse_review_output( - text: str, - *, - require_findings: bool = True, - required_keys: tuple[str, ...] = (), -) -> dict[str, Any]: - """Extract the JSON review result from Codex/API output.""" - stripped = text.strip() - - # Try to extract from markdown code fence - fence_match = re.fullmatch( - r"```(?:json)?\s*(.*?)\s*```", stripped, flags=re.DOTALL | re.IGNORECASE - ) - if fence_match: - stripped = fence_match.group(1).strip() - - candidates: list[dict[str, Any]] = [] - try: - payload = json.loads(stripped) - if isinstance(payload, dict): - candidates.append(payload) - except json.JSONDecodeError: - decoder = json.JSONDecoder() - for index, char in enumerate(stripped): - if char != "{": - continue - try: - payload, _end = decoder.raw_decode(stripped[index:]) - except json.JSONDecodeError: - continue - if isinstance(payload, dict): - candidates.append(payload) - - for payload in candidates: - if require_findings and not isinstance(payload.get("findings"), list): - continue - if any(key not in payload for key in required_keys): - continue - return payload - - if require_findings: - raise ReviewError(f"Failed to parse Codex review output with a findings array: {stripped[:500]}") - if required_keys: - raise ReviewError(f"Failed to parse Codex review output with required keys: {stripped[:500]}") - raise ReviewError(f"Failed to parse Codex review output as JSON: {stripped[:500]}") - - -# --------------------------------------------------------------------------- -# Findings evaluation -# --------------------------------------------------------------------------- - - -def evaluate_findings( - findings: list[dict[str, Any]], - changed_files: list[dict[str, Any]], - policy: dict[str, Any], -) -> dict[str, Any]: - """Evaluate Codex findings against the risk policy. - - Returns a decision dict with: - - blocked: whether merge should be blocked - - blocking_findings: findings that cause blocking - - non_blocking_findings: findings that are reported but don't block - - risk_summary: human-readable summary - """ - blocking: list[dict[str, Any]] = [] - non_blocking: list[dict[str, Any]] = [] - file_risk_cache: dict[str, tuple[str, str]] = {} - - # Build a set of changed file paths - changed_paths: set[str] = set() - file_statuses: dict[str, str] = {} - for f in changed_files: - path = f.get("filename", "").strip() - if path: - changed_paths.add(path) - file_statuses[path] = f.get("status", "") - - for finding in findings: - if not isinstance(finding, dict): - continue - - severity = str(finding.get("severity", "")).strip().lower() - file_path = str(finding.get("file", "")).strip() - - # Classify the file's risk level - if file_path not in file_risk_cache: - file_risk_cache[file_path] = classify_file_risk(file_path, policy) - file_risk, file_risk_reason = file_risk_cache[file_path] - - # Determine if this finding should block - should_block = ( - severity in BLOCK_SEVERITIES - and file_risk == "high" - and file_path in changed_paths # only block on actually changed files - ) - - enriched = { - **finding, - "file_risk": file_risk, - "file_risk_reason": file_risk_reason, - } - - if should_block: - blocking.append(enriched) - else: - non_blocking.append(enriched) - - blocked = len(blocking) > 0 - - # Build summary - all_findings = blocking + non_blocking - summary_parts = [] - if blocked: - summary_parts.append( - f"🚫 **Merge blocked**: {len(blocking)} serious issue(s) found in high-risk files" - ) - elif all_findings: - total = len(all_findings) - summary_parts.append( - f"✅ **Merge allowed**: {total} finding(s) reported but none are blocking" - ) - else: - summary_parts.append("✅ **Merge allowed**: No issues found") - - return { - "blocked": blocked, - "blocking_findings": blocking, - "non_blocking_findings": non_blocking, - "total_findings": len(all_findings), - "summary": "\n\n".join(summary_parts), - } - - -# --------------------------------------------------------------------------- -# PR comment -# --------------------------------------------------------------------------- - - -def build_pr_comment( - decision: dict[str, Any], - pr_url: str, - *, - reviewed_head_sha: str = "", - review_completed: bool = True, -) -> str: - """Build a comment containing only the current exact-head result.""" - lines = [ - "", - f"{HEAD_SHA_MARKER_PREFIX}{reviewed_head_sha}{HEAD_SHA_MARKER_SUFFIX}", - f"{REVIEW_COMPLETED_MARKER_PREFIX}{str(review_completed).lower()}{DECISION_MARKER_SUFFIX}", - f"{IMPLEMENTATION_MARKER_PREFIX}{review_implementation_digest()}{IMPLEMENTATION_MARKER_SUFFIX}", - "## 🤖 Codex PR Review", - "", - decision["summary"], - "", - ] - - blocking = decision["blocking_findings"] - if blocking: - lines.extend([ - "### 🚫 Blocking Issues", - "", - "These issues must be fixed before this PR can be merged:", - "", - ]) - for i, f in enumerate(blocking, 1): - lines.extend(_format_finding(i, f)) - - non_blocking = decision["non_blocking_findings"] - if non_blocking: - lines.extend([ - "### ℹ️ Other Findings", - "", - ]) - for i, f in enumerate(non_blocking, 1): - lines.extend(_format_finding(i, f)) - - lines.extend([ - "---", - f"*Review by Codex PR Review bot • [PR]({pr_url})*", - ]) - - return "\n".join(lines) - - -def _format_finding(index: int, finding: dict[str, Any]) -> list[str]: - severity = finding.get("severity", "unknown") - category = finding.get("category", "general") - file_path = finding.get("file", "?") - line = finding.get("line") - description = finding.get("description", "No description") - suggestion = finding.get("suggestion", "") - - emoji = {"critical": "🔴", "high": "🟠", "medium": "🟡", "low": "🔵"}.get(severity, "⚪") - - lines = [ - f"#### {index}. {emoji} [{severity.upper()}] {category.title()} in `{file_path}`", - "", - f"> {description}", - ] - if line: - lines[-1] += f" (line {line})" - if suggestion: - lines.extend(["", f"**Suggestion:** {suggestion}"]) - lines.append("") - return lines - - -# --------------------------------------------------------------------------- -# Existing comment management -# --------------------------------------------------------------------------- - - -def find_existing_review_comment( - token: str, repo: str, pr_number: int -) -> tuple[int | None, str]: - """Find an existing Codex review comment on the PR. - - Returns ``(comment_id, body)``. ``comment_id`` is ``None`` when absent. - """ - marker = "" - page = 1 - while True: - comments = github_request( - token, - "GET", - f"/repos/{repo}/issues/{pr_number}/comments?per_page=100&page={page}&sort=created&direction=desc", - ) - if not isinstance(comments, list): - break - for comment in comments: - if _is_trusted_review_comment(comment) and marker in str(comment.get("body", "")): - return comment.get("id"), str(comment.get("body") or "") - if len(comments) < 100: - break - page += 1 - return None, "" - - -def _is_trusted_review_comment(comment: Any) -> bool: - """Accept state only from a complete trusted GitHub comment record.""" - if not isinstance(comment, dict): - return False - user = comment.get("user") - if not isinstance(user, dict): - return False - expected_login = env_value("CODEX_PR_REVIEW_COMMENT_AUTHOR", "github-actions[bot]").strip().casefold() - actual_login = str(user.get("login") or "").strip().casefold() - if not expected_login or actual_login != expected_login: - return False - if str(user.get("type") or "").strip().casefold() != "bot": - return False - if not isinstance(comment.get("id"), int) or comment["id"] <= 0: - return False - if not isinstance(comment.get("created_at"), str) or not comment["created_at"].strip(): - return False - app = comment.get("performed_via_github_app") - if app is not None and ( - not isinstance(app, dict) - or str(app.get("slug") or "").strip().casefold() != "github-actions" - ): - return False - return True - - -def parse_review_implementation_digest(body: str) -> str: - match = re.search( - rf"{re.escape(IMPLEMENTATION_MARKER_PREFIX)}([0-9a-f]{{24}}){re.escape(IMPLEMENTATION_MARKER_SUFFIX)}", - body or "", - ) - return match.group(1) if match else "" - - -def upsert_pr_comment( - token: str, repo: str, pr_number: int, body: str -) -> None: - """Create or update the Codex review comment on the PR.""" - existing_id, _existing_body = find_existing_review_comment(token, repo, pr_number) - if existing_id: - github_request( - token, - "PATCH", - f"/repos/{repo}/issues/comments/{existing_id}", - {"body": body}, - ) - print(f"Updated existing review comment #{existing_id}") - else: - github_request( - token, - "POST", - f"/repos/{repo}/issues/{pr_number}/comments", - {"body": body}, - ) - print("Posted new review comment") - - -def write_decision_outputs(decision_payload: dict[str, Any]) -> None: - """Persist the decision and publish the same contract fields to GitHub Actions.""" - output_dir = Path("data/output/codex_pr_review") - output_dir.mkdir(parents=True, exist_ok=True) - (output_dir / "decision.json").write_text( - json.dumps(decision_payload, ensure_ascii=False, indent=2) + "\n", - encoding="utf-8", - ) - - github_output = os.environ.get("GITHUB_OUTPUT") - if github_output: - with open(github_output, "a", encoding="utf-8") as f: - f.write(f"blocked={'true' if decision_payload['blocked'] else 'false'}\n") - f.write(f"total_findings={decision_payload['total_findings']}\n") - f.write(f"blocking_count={len(decision_payload['blocking_findings'])}\n") - f.write( - f"review_completed={'true' if decision_payload['review_completed'] else 'false'}\n" - ) - f.write(f"reviewed_head_sha={decision_payload['reviewed_head_sha']}\n") - f.write(f"failure_kind={decision_payload.get('failure_kind', '')}\n") - - -def publish_review_decision( - token: str, - repo: str, - pr_number: int, - pr_url: str, - decision: dict[str, Any], - *, - exit_code: int, - current_head_sha: str = "", - review_completed: bool = True, -) -> int: - """Publish one current-head comment, artifact, and step-output decision.""" - upsert_pr_comment( - token, - repo, - pr_number, - build_pr_comment( - decision, - pr_url, - reviewed_head_sha=current_head_sha, - review_completed=review_completed, - ), - ) - write_decision_outputs( - { - **decision, - "review_completed": review_completed, - "reviewed_head_sha": current_head_sha if review_completed else "", - "current_head_sha": current_head_sha, - "review_implementation_digest": review_implementation_digest(), - } - ) - return exit_code - - -def publish_retryable_review_failure( - token: str, - repo: str, - pr_number: int, - pr_url: str, - current_head_sha: str, -) -> int: - """Record an external retryable failure without marking the head reviewed.""" - return publish_review_decision( - token, - repo, - pr_number, - pr_url, - { - "blocked": False, - "blocking_findings": [], - "non_blocking_findings": [], - "total_findings": 0, - "summary": ( - "⚠️ **Review incomplete**: external retryable failure; " - "this head has not been successfully reviewed." - ), - "failure_kind": "external_retryable", - }, - exit_code=1, - current_head_sha=current_head_sha, - review_completed=False, - ) - - -# --------------------------------------------------------------------------- -# Main -# --------------------------------------------------------------------------- - - -def main() -> int: - token = env_value("GH_TOKEN") or env_value("GITHUB_TOKEN") - if not token: - print("::error::GH_TOKEN or GITHUB_TOKEN is required", file=sys.stderr) - return 1 - - repo = env_value("GITHUB_REPOSITORY") - if not repo: - print("::error::GITHUB_REPOSITORY is not set", file=sys.stderr) - return 1 - - # Get PR context from the event - event_path = Path(os.environ.get("GITHUB_EVENT_PATH", "")) - if not event_path.exists(): - print("::error::GITHUB_EVENT_PATH not found", file=sys.stderr) - return 1 - - event = json.loads(event_path.read_text(encoding="utf-8")) - pr = event.get("pull_request") or {} - pr_number = pr.get("number") - if not pr_number: - print("::error::No pull request number in event", file=sys.stderr) - return 1 - - pr_title = str(pr.get("title", "")) - pr_body = str(pr.get("body", "")) - pr_url = str(pr.get("html_url", "")) - head = pr.get("head") if isinstance(pr.get("head"), dict) else {} - current_head_sha = str(head.get("sha") or "").strip().lower() - if not re.fullmatch(r"[0-9a-f]{7,64}", current_head_sha): - print("::error::Pull request exact head SHA is missing or invalid", file=sys.stderr) - return 1 - - print(f"Reviewing PR #{pr_number}: {pr_title}") - - # Fetch changed files for risk classification - changed_files = fetch_pr_files(token, repo, pr_number) - changed_paths = [f.get("filename", "") for f in changed_files] - print(f"Changed files ({len(changed_paths)}): {', '.join(changed_paths[:10])}" - + (f" and {len(changed_paths) - 10} more..." if len(changed_paths) > 10 else "")) - - # Load policy from the trusted base ref. The PR head checkout is untrusted - # and may include policy changes that should be reviewed as data, not used - # as live guardrail configuration for this same review. - base = pr.get("base") if isinstance(pr.get("base"), dict) else {} - base_repo = base.get("repo") if isinstance(base.get("repo"), dict) else {} - policy = load_policy( - token, - str(base_repo.get("full_name") or repo), - str(base.get("sha") or ""), - ) - if policy.get("policy_errors"): - print(f"::warning::Policy errors: {policy['policy_errors']}") - - # Each invocation reviews only the event's exact head. Prior PR comments are - # presentation records, never input to the current application decision. - all_low_risk = changed_files_are_low_risk(changed_paths, policy) - if all_low_risk and changed_paths: - print("All changed files are low-risk (docs/tests). Skipping Codex review.") - decision = { - "blocked": False, - "blocking_findings": [], - "non_blocking_findings": [], - "total_findings": 0, - "summary": "✅ **Merge allowed**: All changes are in docs/tests — Codex review skipped.", - } - return publish_review_decision( - token, - repo, - pr_number, - pr_url, - decision, - exit_code=0, - current_head_sha=current_head_sha, - ) - - diff = fetch_pr_diff(token, repo, pr_number) - print(f"Fetched diff: {len(diff)} chars, {len(diff.splitlines())} lines") - - prompt = build_review_prompt(diff, pr_title, pr_body, repo) - print(f"Built review prompt: {len(prompt)} chars") - - try: - complexity = _estimate_review_complexity( - diff, changed_paths, title=pr_title, body=pr_body - ) - output = run_codex_review_with_fallback( - prompt, - DEFAULT_TIMEOUT_MINUTES, - complexity=complexity, - changed_file_count=len(changed_paths), - changed_line_count=len(diff.splitlines()), - ) - except ReviewError as exc: - print(f"::warning::Codex review external retryable failure: {exc}") - return publish_retryable_review_failure( - token, repo, pr_number, pr_url, current_head_sha - ) - - print(f"Codex output: {len(output)} chars") - try: - review = parse_review_output(output) - except ReviewError as exc: - print(f"::warning::Codex review parse failure is retryable: {exc}") - return publish_retryable_review_failure( - token, repo, pr_number, pr_url, current_head_sha - ) - - findings = review.get("findings", []) - if not isinstance(findings, list): - findings = [] - print(f"Found {len(findings)} issue(s)") - - decision = evaluate_findings(findings, changed_files, policy) - if decision["blocked"]: - print("::error::Merge blocked: serious issues found on current exact head") - else: - print("Review passed: no blocking issues on current exact head") - return publish_review_decision( - token, - repo, - pr_number, - pr_url, - decision, - exit_code=1 if decision["blocked"] else 0, - current_head_sha=current_head_sha, - ) - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/service/dual_review_primary.py b/service/dual_review_primary.py index 17e8936a..7b7eada3 100644 --- a/service/dual_review_primary.py +++ b/service/dual_review_primary.py @@ -4,10 +4,11 @@ import json import os -import urllib.error from pathlib import Path from typing import Any +from client.config import GatewayConfig +from client.gateway_client import AiGatewayClient from service.dual_review import VERDICT_INVALID, VERDICT_UNAVAILABLE from service.dual_review_secondary import parse_llm_review_output @@ -86,23 +87,16 @@ def run_codex_primary_review( raise RuntimeError("CODEX_AUDIT_SERVICE_URL is not configured") timeout = int(timeout_minutes or os.environ.get("DUAL_REVIEW_PRIMARY_TIMEOUT_MINUTES", "15")) - from scripts.run_codex_pr_review import ReviewError, run_codex_service_review - - try: - output = run_codex_service_review( - prompt, - timeout_minutes=timeout, - complexity="high", - ) - except json.JSONDecodeError as exc: - return { - "source": "codex_primary", - "verdict": VERDICT_INVALID, - "confidence": 0.0, - "error": str(exc), - } - except ReviewError as exc: - message = str(exc) + result = AiGatewayClient(GatewayConfig.from_env()).execute( + prompt, + task="promotion_review", + mode="review_only", + complexity="high", + source_repository=os.environ.get("GITHUB_REPOSITORY") or None, + timeout=timeout * 60, + ) + if not result.success: + message = result.error or result.note or "Codex service review unavailable" unavailable_markers = ( "daily budget exceeded", "quota", @@ -120,14 +114,7 @@ def run_codex_primary_review( "confidence": 0.0, "error": message, } - except (urllib.error.URLError, OSError) as exc: - return { - "source": "codex_primary", - "verdict": VERDICT_UNAVAILABLE, - "confidence": 0.0, - "error": str(exc), - } - return parse_primary_review_output(output) + return parse_primary_review_output(result.output) def primary_review_available() -> bool: diff --git a/service/org_health.py b/service/org_health.py index 3f514b85..0e26c945 100644 --- a/service/org_health.py +++ b/service/org_health.py @@ -46,7 +46,6 @@ "Auto Merge Dependabot PR", "Check", "CI", - "Codex PR Review", "Codex Review Gate", "Monthly Orchestrator", "Secret Scan", diff --git a/tests/test_codex_audit_service_complexity.py b/tests/test_codex_audit_service_complexity.py index b0f0091e..5c2fbef4 100644 --- a/tests/test_codex_audit_service_complexity.py +++ b/tests/test_codex_audit_service_complexity.py @@ -13,13 +13,6 @@ _service = importlib.util.module_from_spec(spec) spec.loader.exec_module(_service) # type: ignore[arg-type] -PR_REVIEW_SCRIPT_PATH = Path(__file__).resolve().parents[1] / "scripts" / "run_codex_pr_review.py" -pr_spec = importlib.util.spec_from_file_location("run_codex_pr_review_test", PR_REVIEW_SCRIPT_PATH) -if pr_spec is None or pr_spec.loader is None: # pragma: no cover - defensive guard for env issues - raise RuntimeError(f"Failed to load module spec from {PR_REVIEW_SCRIPT_PATH}") -_pr_review = importlib.util.module_from_spec(pr_spec) -pr_spec.loader.exec_module(_pr_review) # type: ignore[arg-type] - class TestComplexityModelRouting(unittest.TestCase): """Validate complexity-to-model adaptation behavior used in codex-audit service.""" @@ -137,17 +130,5 @@ def test_codex_adapter_preserves_quota_category_when_tail_is_truncated(self) -> repo_dir=Path(__file__).resolve().parents[1], ) - def test_direct_api_model_for_complexity_reads_provider_specific_env(self) -> None: - with mock.patch.dict( - "os.environ", - {"CODEX_AUDIT_OPENAI_LOW_COMPLEXITY_MODEL": "gpt-low"}, - clear=True, - ): - self.assertEqual( - _pr_review._direct_api_model_for_complexity("openai", "low"), - "gpt-low", - ) - - if __name__ == "__main__": unittest.main() diff --git a/tests/test_dual_review_primary.py b/tests/test_dual_review_primary.py index b9d7c582..ebe6cf4d 100644 --- a/tests/test_dual_review_primary.py +++ b/tests/test_dual_review_primary.py @@ -1,13 +1,49 @@ from __future__ import annotations +import json import unittest -from unittest.mock import patch +from unittest.mock import MagicMock, patch +from client.config import GatewayConfig +from client.gateway_client import AiGatewayClient, AiResult from service.dual_review import VERDICT_INVALID, VERDICT_UNAVAILABLE from service.dual_review_primary import build_primary_prompt, parse_primary_review_output, run_codex_primary_review class DualReviewPrimaryTests(unittest.TestCase): + def test_gateway_execute_preserves_task_and_complexity(self) -> None: + submit_response = MagicMock() + submit_response.__enter__.return_value.read.return_value = b'{"job_id":"job-1"}' + poll_response = MagicMock() + poll_response.__enter__.return_value.read.return_value = ( + b'{"status":"succeeded","output":"ok"}' + ) + config = GatewayConfig( + service_url="https://service.invalid", + source_repository="QuantStrategyLab/AIAuditBridge", + ) + + with ( + patch("client.gateway_client._fetch_oidc_token", return_value="oidc"), + patch( + "client.gateway_client.urllib.request.urlopen", + side_effect=[submit_response, poll_response], + ) as urlopen, + patch("client.gateway_client.time.sleep"), + ): + result = AiGatewayClient(config).execute( + "review", + task="promotion_review", + complexity="high", + timeout=1, + ) + + self.assertTrue(result.success) + request = urlopen.call_args_list[0].args[0] + payload = json.loads(request.data) + self.assertEqual(payload["task"], "promotion_review") + self.assertEqual(payload["complexity"], "high") + def test_build_primary_prompt_includes_evidence_summary(self) -> None: from pathlib import Path import json @@ -34,29 +70,37 @@ def test_parse_primary_review_output(self) -> None: self.assertEqual(review["source"], "codex_primary") @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) - @patch("scripts.run_codex_pr_review.run_codex_service_review") + @patch("service.dual_review_primary.AiGatewayClient.execute") def test_budget_error_is_unavailable(self, review) -> None: - from scripts.run_codex_pr_review import ReviewError - - review.side_effect = ReviewError("Daily budget exceeded") + review.return_value = AiResult.unavailable("codex", "Daily budget exceeded") result = run_codex_primary_review(prompt="review") self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) + review.assert_called_once_with( + "review", + task="promotion_review", + mode="review_only", + complexity="high", + source_repository=None, + timeout=900, + ) @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) - @patch("scripts.run_codex_pr_review.run_codex_service_review") + @patch("service.dual_review_primary.AiGatewayClient.execute") def test_capacity_error_is_unavailable(self, review) -> None: - from scripts.run_codex_pr_review import ReviewError - - review.side_effect = ReviewError("Codex service request failed: 401 too many active jobs: max 10") + review.return_value = AiResult.unavailable( + "codex", + "Codex service request failed: 401 too many active jobs: max 10", + ) result = run_codex_primary_review(prompt="review") self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) - @patch("scripts.run_codex_pr_review.run_codex_service_review") + @patch("service.dual_review_primary.AiGatewayClient.execute") def test_protocol_error_is_invalid(self, review) -> None: - from scripts.run_codex_pr_review import ReviewError - - review.side_effect = ReviewError("response did not contain review JSON") + review.return_value = AiResult.unavailable( + "codex", + "response did not contain review JSON", + ) result = run_codex_primary_review(prompt="review") self.assertEqual(result["verdict"], VERDICT_INVALID) diff --git a/tests/test_org_health.py b/tests/test_org_health.py index d4d7f1a6..cc5c9599 100644 --- a/tests/test_org_health.py +++ b/tests/test_org_health.py @@ -439,9 +439,10 @@ def test_read_org_health_limits_default_monitored_workflows(self) -> None: {"id": 1, "name": "CI", "state": "active"}, {"id": 2, "name": "Docs", "state": "active"}, {"id": 3, "name": "Codex PR Review", "state": "active"}, + {"id": 4, "name": "Codex Review Gate", "state": "active"}, ] selected = org_health._monitored_workflows(workflows) - self.assertEqual([item["name"] for item in selected], ["CI", "Codex PR Review"]) + self.assertEqual([item["name"] for item in selected], ["CI", "Codex Review Gate"]) def test_read_org_health_serves_expired_stale_cache_and_refreshes_in_background(self) -> None: cache_key = (("QuantStrategyLab/cached",), "CODEX_AUDIT_SERVICE_GITHUB_TOKEN", "token", "all") diff --git a/tests/test_run_codex_pr_review.py b/tests/test_run_codex_pr_review.py deleted file mode 100644 index 77ee10c1..00000000 --- a/tests/test_run_codex_pr_review.py +++ /dev/null @@ -1,816 +0,0 @@ -from __future__ import annotations - -import base64 -import json -import os -import subprocess -import sys -import tempfile -import unittest -from pathlib import Path -from unittest.mock import patch - -import scripts.run_codex_pr_review as run_codex_pr_review -from scripts.run_codex_pr_review import ReviewError, run_codex_review_with_fallback - - -class RunCodexPrReviewTests(unittest.TestCase): - def _write_event(self, tmpdir: str, files: list[str]) -> str: - event = { - "pull_request": {"number": 7, "head": {"sha": "abc1234"}}, - } - path = Path(tmpdir) / "event.json" - path.write_text( - json.dumps(event), - encoding="utf-8", - ) - return str(path) - - def _run_main_with_review( - self, tmpdir: str, output: str | Exception, previous_comment: str = "" - ) -> tuple[int, object, object, dict[str, object]]: - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": self._write_event( - tmpdir, ["scripts/run_codex_pr_review.py"] - ), - } - backend_patch = ( - patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", side_effect=output) - if isinstance(output, Exception) - else patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", return_value=output) - ) - previous_cwd = os.getcwd() - try: - os.chdir(tmpdir) - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "scripts/run_codex_pr_review.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="current diff"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(99, previous_comment)), - backend_patch as backend, - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - result = run_codex_pr_review.main() - finally: - os.chdir(previous_cwd) - decision = json.loads( - (Path(tmpdir) / "data/output/codex_pr_review/decision.json").read_text(encoding="utf-8") - ) - return result, backend, comment, decision - - def test_changed_files_are_low_risk_only_for_docs_and_tests(self) -> None: - policy = run_codex_pr_review.load_policy() - self.assertTrue(run_codex_pr_review.changed_files_are_low_risk(["docs/guide.md", "tests/test_x.py"], policy)) - self.assertFalse(run_codex_pr_review.changed_files_are_low_risk(["src/app.py"], policy)) - - def test_review_prompt_states_direct_oidc_contract(self) -> None: - prompt = run_codex_pr_review.build_review_prompt("diff", "title", "", "org/repo") - self.assertIn("`job_workflow_ref` is absent for explicit direct callers", prompt) - self.assertIn("Do not emit a finding that concludes no code change is needed", prompt) - - def test_review_prompt_requires_holistic_contract_review(self) -> None: - prompt = run_codex_pr_review.build_review_prompt( - "diff", - "clean-slate contract", - "Legacy compatibility is explicitly out of scope.", - "org/repo", - ) - self.assertIn("report all independent actionable findings in one response", prompt) - self.assertIn("Do not stop after the first blocking issue", prompt) - self.assertIn("current exact-head diff", prompt) - self.assertIn("clean-slate", prompt) - self.assertIn("optional-key presence versus explicit null", prompt) - self.assertIn("every identity-bearing integer", prompt) - self.assertIn("one canonical timestamp representation", prompt) - - def test_review_prompt_requires_reachability_evidence_for_blockers(self) -> None: - prompt = run_codex_pr_review.build_review_prompt( - "diff", - "bounded implementation", - "Future consumers are out of scope.", - "org/repo", - ) - - self.assertIn("current caller or entry point proven by the supplied PR context", prompt) - self.assertIn("introduced by this PR", prompt) - self.assertIn("explicitly declared public untrusted boundary", prompt) - self.assertIn("current configuration and inputs", prompt) - self.assertIn("downgrade it to medium or low", prompt) - self.assertIn("hypothetical future consumer", prompt) - self.assertIn("Do not request a new parser, store, registry, or event-persistence layer", prompt) - - def test_repository_review_template_uses_the_same_reachability_gate(self) -> None: - template = run_codex_pr_review.PROMPT_TEMPLATE_PATH.read_text(encoding="utf-8") - - self.assertIn("current caller or entry point proven by the supplied PR context", template) - self.assertIn("introduced by this PR", template) - self.assertIn("explicitly declared public untrusted boundary", template) - self.assertIn("downgrade it to medium or low", template) - self.assertIn("hypothetical future consumer", template) - - def test_review_script_never_imports_from_the_pr_checkout(self) -> None: - source = Path(run_codex_pr_review.__file__).read_text(encoding="utf-8") - self.assertNotIn("SOURCE_ROOT = BRIDGE_ROOT.parent / \"source\"", source) - - def test_isolated_review_runtime_imports_from_the_trusted_bridge(self) -> None: - result = subprocess.run( - [sys.executable, "-I", str(Path(run_codex_pr_review.__file__))], - env={"GITHUB_EVENT_PATH": "does-not-exist"}, - capture_output=True, - text=True, - check=False, - ) - self.assertEqual(result.returncode, 1) - self.assertIn("GH_TOKEN or GITHUB_TOKEN is required", result.stderr) - self.assertNotIn("ModuleNotFoundError", result.stderr) - - def test_parse_review_output_accepts_a_valid_json_prefix(self) -> None: - self.assertEqual( - run_codex_pr_review.parse_review_output('{"summary":"ok","findings":[]}\nReviewer metadata follows.'), - {"summary": "ok", "findings": []}, - ) - with self.assertRaisesRegex(ReviewError, "findings"): - run_codex_pr_review.parse_review_output('{"ok":true}\nReviewer metadata follows.') - - def test_existing_review_comment_ignores_forged_marker(self) -> None: - forged = { - "id": 1, - "body": "\nforged state", - "user": {"login": "attacker"}, - } - trusted = { - "id": 2, - "body": "\ntrusted", - "user": {"id": 418, "login": "github-actions[bot]", "type": "Bot"}, - "created_at": "2026-07-12T00:00:00Z", - } - with patch("scripts.run_codex_pr_review.github_request", return_value=[forged, trusted]): - comment = run_codex_pr_review.find_existing_review_comment("token", "org/repo", 7) - - self.assertEqual(comment, (2, trusted["body"])) - - def test_review_comment_records_implementation_identity(self) -> None: - body = run_codex_pr_review.build_pr_comment( - {"summary": "ok", "blocking_findings": [], "non_blocking_findings": []}, - "https://example.test/pr/7", - ) - self.assertEqual( - run_codex_pr_review.parse_review_implementation_digest(body), - run_codex_pr_review.review_implementation_digest(), - ) - - def test_repository_policy_has_no_bypass_fields(self) -> None: - policy = run_codex_pr_review.load_policy() - self.assertTrue( - {"ack_labels", "auto_converge_after", "block_on_review_failure"}.isdisjoint(policy["pr_review"]) - ) - self.assertEqual(run_codex_pr_review._default_policy()["pr_review"], {}) - - def test_load_policy_uses_trusted_base_ref(self) -> None: - trusted_policy = { - "version": 1, - "blocked_path_patterns": [], - "risk_policy": { - "low": {"prefixes": ["trusted/"], "exact": ["SAFE.md"], "reason": "trusted"}, - "high": {"reason": "trusted high"}, - }, - } - encoded = base64.b64encode(json.dumps(trusted_policy).encode("utf-8")).decode("ascii") - with patch("scripts.run_codex_pr_review.github_request", return_value={"content": encoded}) as request: - policy = run_codex_pr_review.load_policy("token", "org/repo", "base-sha") - - self.assertEqual(policy["risk_policy"]["low"]["exact"], ["SAFE.md"]) - request.assert_called_once() - - def test_service_failure_falls_back_to_direct_api(self) -> None: - with ( - patch.dict( - os.environ, - { - "CODEX_AUDIT_SERVICE_URL": "https://service.example", - "CODEX_PR_REVIEW_API_FALLBACK_ENABLED": "true", - }, - clear=True, - ), - patch( - "scripts.run_codex_pr_review.run_codex_service_review", - side_effect=ReviewError("HTTP 429 Too Many Requests"), - ), - patch("scripts.run_codex_pr_review.run_direct_api_review", return_value="api review") as direct_api, - ): - output = run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - self.assertEqual(output, "api review") - direct_api.assert_called_once_with("Review this PR.", complexity="high") - - def test_service_failure_does_not_fallback_to_direct_api_when_disabled(self) -> None: - with ( - patch.dict( - os.environ, - { - "CODEX_AUDIT_SERVICE_URL": "https://service.example", - "CODEX_PR_REVIEW_API_FALLBACK_ENABLED": "false", - }, - clear=True, - ), - patch( - "scripts.run_codex_pr_review.run_codex_service_review", - side_effect=ReviewError("HTTP 429 Too Many Requests"), - ), - patch("scripts.run_codex_pr_review.run_direct_api_review") as direct_api, - ): - with self.assertRaises(ReviewError) as raised: - run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - self.assertIn("direct API fallback is disabled", str(raised.exception)) - direct_api.assert_not_called() - - def test_direct_api_runs_when_service_url_is_unset(self) -> None: - with ( - patch.dict( - os.environ, - { - "OPENAI_API_KEY": "test-key", - "CODEX_PR_REVIEW_API_FALLBACK_ENABLED": "true", - }, - clear=True, - ), - patch("scripts.run_codex_pr_review.run_direct_api_review", return_value="api review") as direct_api, - ): - output = run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - self.assertEqual(output, "api review") - direct_api.assert_called_once_with("Review this PR.", complexity="high") - - def test_direct_api_runs_when_service_url_unset_even_if_service_fallback_disabled(self) -> None: - with ( - patch.dict( - os.environ, - { - "OPENAI_API_KEY": "test-key", - "CODEX_PR_REVIEW_API_FALLBACK_ENABLED": "false", - "CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED": "true", - }, - clear=True, - ), - patch("scripts.run_codex_pr_review.run_direct_api_review", return_value="api review") as direct_api, - ): - output = run_codex_review_with_fallback("Review this PR.", timeout_minutes=20) - - self.assertEqual(output, "api review") - direct_api.assert_called_once() - - def test_direct_api_is_blocked_when_service_url_unset_and_primary_disabled(self) -> None: - with ( - patch.dict( - os.environ, - { - "OPENAI_API_KEY": "test-key", - "CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED": "false", - }, - clear=True, - ), - patch("scripts.run_codex_pr_review.run_direct_api_review") as direct_api, - ): - with self.assertRaises(ReviewError) as raised: - run_codex_review_with_fallback("Review this PR.", timeout_minutes=20) - - self.assertEqual(str(raised.exception), run_codex_pr_review.NO_REVIEW_BACKEND_CONFIGURED) - direct_api.assert_not_called() - - - def test_service_fallback_without_api_keys_preserves_service_failure(self) -> None: - with ( - patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), - patch( - "scripts.run_codex_pr_review.run_codex_service_review", - side_effect=ReviewError("HTTP 429 Too Many Requests"), - ), - ): - with self.assertRaises(ReviewError) as raised: - run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - self.assertIn("Codex service review failed", str(raised.exception)) - self.assertFalse(run_codex_pr_review._review_backend_is_unconfigured(raised.exception)) - - def test_service_auth_failure_does_not_fall_back_to_direct_api(self) -> None: - with ( - patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), - patch( - "scripts.run_codex_pr_review.run_codex_service_review", - side_effect=ReviewError("Codex service request failed: 401 Unauthorized"), - ), - patch("scripts.run_codex_pr_review.run_direct_api_review") as direct_api, - ): - with self.assertRaises(ReviewError): - run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - direct_api.assert_not_called() - - def test_main_blocks_high_risk_on_review_infra_error(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["src/app.py"]) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "src/app.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/src/app.py b/src/app.py"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch( - "scripts.run_codex_pr_review.run_codex_review_with_fallback", - side_effect=ReviewError("Codex service job timed out"), - ) as backend, - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - comment.assert_called_once() - backend.assert_called_once() - self.assertIn("Review incomplete", comment.call_args.args[3]) - - def test_main_does_not_carry_blockers_across_heads(self) -> None: - prior = """ - - - -""" - with tempfile.TemporaryDirectory() as tmpdir: - result, backend, comment, _decision = self._run_main_with_review( - tmpdir, '{"summary":"clear","findings":[]}', prior - ) - - self.assertEqual(result, 0) - backend.assert_called_once() - body = comment.call_args.args[3] - self.assertIn("codex-pr-review-head-sha:abc1234", body) - for historical_marker in ("streak", "fingerprint", "history"): - self.assertNotIn(f"codex-pr-review-{historical_marker}", body) - - def test_backend_failure_is_retryable_and_not_a_successful_review(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - result, _backend, comment, decision = self._run_main_with_review( - tmpdir, ReviewError("quota_or_capacity_failure") - ) - - self.assertEqual(result, 1) - self.assertFalse(decision["review_completed"]) - body = comment.call_args.args[3] - self.assertIn("codex-pr-review-completed:false", body) - self.assertIn("external retryable failure", body) - self.assertNotIn("Merge allowed", body) - - def test_parse_failure_records_external_retry_without_review_success(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - result, _backend, comment, decision = self._run_main_with_review( - tmpdir, "not json" - ) - - self.assertEqual(result, 1) - self.assertFalse(decision["review_completed"]) - self.assertEqual(decision["reviewed_head_sha"], "") - self.assertEqual(decision["current_head_sha"], "abc1234") - self.assertEqual(decision["failure_kind"], "external_retryable") - self.assertIn("codex-pr-review-completed:false", comment.call_args.args[3]) - - def test_main_retries_when_review_quota_is_unavailable(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["src/app.py"]) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "src/app.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/src/app.py b/src/app.py"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch( - "scripts.run_codex_pr_review.run_codex_review_with_fallback", - side_effect=ReviewError("Codex service job failed [quota_or_capacity_failure]: usage limits reached"), - ), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - comment.assert_called_once() - self.assertIn("Review incomplete", comment.call_args.args[3]) - self.assertNotIn("Merge blocked", comment.call_args.args[3]) - - def test_main_retries_when_daily_budget_is_exhausted(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["src/app.py"]) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "src/app.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/src/app.py b/src/app.py"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch( - "scripts.run_codex_pr_review.run_codex_review_with_fallback", - side_effect=ReviewError( - 'Codex service request failed: 429 {"error": "Daily budget exceeded: ' - '$0.0000 remaining, $0.0500 needed"}' - ), - ), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - self.assertIn("Review incomplete", comment.call_args.args[3]) - self.assertNotIn("Merge blocked", comment.call_args.args[3]) - - def test_main_retries_when_codex_exec_failure_is_unclassified(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["src/app.py"]) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "src/app.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/src/app.py b/src/app.py"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch( - "scripts.run_codex_pr_review.run_codex_review_with_fallback", - side_effect=ReviewError("Codex service job failed [unknown_failure]: codex exec failed (rc=1)"), - ), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - self.assertIn("Review incomplete", comment.call_args.args[3]) - - def test_main_skips_low_risk_docs_before_calling_review_backend(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["docs/guide.md", "tests/test_x.py"]) - policy = run_codex_pr_review._default_policy() - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "docs/guide.md"}, {"filename": "tests/test_x.py"}]), - patch("scripts.run_codex_pr_review.load_policy", return_value=policy), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch( - "scripts.run_codex_pr_review.run_codex_review_with_fallback", - side_effect=ReviewError("Codex service job timed out"), - ) as backend, - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 0) - - comment.assert_called_once() - backend.assert_not_called() - - def test_main_blocks_unconfigured_backend_even_with_legacy_opt_in(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["scripts/run_codex_pr_review.py"]) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - "CODEX_PR_REVIEW_ALLOW_UNCONFIGURED_BACKEND": "true", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "scripts/run_codex_pr_review.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", side_effect=ReviewError(run_codex_pr_review.NO_REVIEW_BACKEND_CONFIGURED)), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - comment.assert_called_once() - self.assertIn("Review incomplete", comment.call_args.args[3]) - - def test_main_ignores_legacy_bypass_policy_fields(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event = { - "pull_request": { - "number": 7, - "title": "feat: risky", - "body": "", - "html_url": "https://example.test/pr/7", - "labels": [{"name": "review-ack"}], - "head": {"sha": "abc1234"}, - "base": {"sha": "base123", "repo": {"full_name": "org/repo"}}, - } - } - event_path = Path(tmpdir) / "event.json" - event_path.write_text(json.dumps(event), encoding="utf-8") - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": str(event_path), - "GITHUB_EVENT_NAME": "pull_request", - } - review_json = json.dumps( - { - "summary": "blocking issue", - "findings": [ - { - "severity": "high", - "category": "security", - "file": "scripts/run_codex_pr_review.py", - "line": 1, - "description": "example blocking finding", - "suggestion": "fix it", - } - ], - } - ) - with ( - patch.dict(os.environ, env, clear=True), - patch( - "scripts.run_codex_pr_review.fetch_pr_files", - return_value=[{"filename": "scripts/run_codex_pr_review.py"}], - ), - patch( - "scripts.run_codex_pr_review.fetch_pr_diff", - return_value="diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py", - ), - patch( - "scripts.run_codex_pr_review.load_policy", - return_value={ - "version": 1, - "pr_review": { - "ack_labels": ["review-ack"], - "auto_converge_enabled": True, - "auto_converge_after": 1, - "block_on_review_failure": True, - }, - }, - ), - patch( - "scripts.run_codex_pr_review.find_existing_review_comment", - return_value=(None, ""), - ), - patch( - "scripts.run_codex_pr_review.run_codex_review_with_fallback", - return_value=review_json, - ), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - comment.assert_called_once() - body = comment.call_args.args[3] - self.assertNotIn("will not block merge", body) - - def test_main_fails_closed_on_unconfigured_backend_without_opt_in(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["scripts/run_codex_pr_review.py"]) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "scripts/run_codex_pr_review.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py"), - patch("scripts.run_codex_pr_review.load_policy", return_value=run_codex_pr_review._default_policy()), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", side_effect=ReviewError(run_codex_pr_review.NO_REVIEW_BACKEND_CONFIGURED)), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - comment.assert_called_once() - self.assertIn("Review incomplete", comment.call_args.args[3]) - - def test_main_fails_closed_on_infrastructure_failure_when_policy_requires_it(self) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - event_path = self._write_event(tmpdir, ["scripts/run_codex_pr_review.py"]) - policy_path = Path(tmpdir) / ".github" / "codex_auto_merge_policy.json" - policy_path.parent.mkdir(parents=True, exist_ok=True) - policy_path.write_text( - json.dumps( - { - "version": 1, - "pr_review": { - "block_on_review_failure": True, - "auto_converge_after": 3, - }, - } - ), - encoding="utf-8", - ) - env = { - "GH_TOKEN": "token", - "GITHUB_REPOSITORY": "org/repo", - "GITHUB_EVENT_PATH": event_path, - "GITHUB_EVENT_NAME": "pull_request", - "CODEX_PR_REVIEW_REPO_ROOT": tmpdir, - } - with ( - patch.dict(os.environ, env, clear=True), - patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "scripts/run_codex_pr_review.py"}]), - patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py"), - patch("scripts.run_codex_pr_review.find_existing_review_comment", return_value=(None, "")), - patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", side_effect=ReviewError("Codex service job timed out")), - patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, - ): - self.assertEqual(run_codex_pr_review.main(), 1) - - comment.assert_called_once() - self.assertIn("Review incomplete", comment.call_args.args[3]) - - def test_service_timeout_does_not_fall_back_to_direct_api(self) -> None: - with ( - patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), - patch( - "scripts.run_codex_pr_review.run_codex_service_review", - side_effect=ReviewError("Codex service job timed out"), - ), - patch("scripts.run_codex_pr_review.run_direct_api_review") as direct_api, - ): - with self.assertRaises(ReviewError): - run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - direct_api.assert_not_called() - - def test_oidc_repo_not_allowed_counts_as_unconfigured_backend(self) -> None: - exc = ReviewError('Codex service request failed: 401 {"status":"error","error":"OIDC repository is not allowed"}') - self.assertTrue(run_codex_pr_review._review_backend_is_unconfigured(exc)) - - def test_service_exec_failure_falls_back_to_direct_api(self) -> None: - with ( - patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), - patch( - "scripts.run_codex_pr_review.run_codex_service_review", - side_effect=ReviewError("Codex service job failed: codex exec failed (rc=1): boom"), - ), - patch( - "scripts.run_codex_pr_review.run_direct_api_review", - return_value='{"findings":[]}', - ) as direct_api, - ): - output = run_codex_review_with_fallback( - "Review this PR.", - timeout_minutes=20, - complexity="high", - changed_file_count=3, - changed_line_count=120, - ) - - direct_api.assert_called_once() - self.assertEqual(output, '{"findings":[]}') - - def test_service_review_refreshes_oidc_token_while_polling(self) -> None: - responses = [ - {"job_id": "job-1"}, - {"status": "succeeded", "output": "{}"}, - ] - with ( - patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example", "GITHUB_REPOSITORY": "org/repo"}, clear=True), - patch("scripts.run_codex_pr_review.request_github_oidc_token", side_effect=["submit-token", "poll-token"]) as oidc, - patch("scripts.run_codex_pr_review._service_request", side_effect=responses) as request, - patch("scripts.run_codex_pr_review.time.sleep"), - ): - output = run_codex_pr_review.run_codex_service_review("prompt", timeout_minutes=1) - - self.assertEqual(output, "{}") - self.assertEqual(oidc.call_count, 2) - self.assertEqual(request.call_args_list[0].args[2], "submit-token") - self.assertEqual(request.call_args_list[1].args[2], "poll-token") - - def test_service_review_uses_deployed_cli_model(self) -> None: - responses = [ - {"job_id": "job-1"}, - {"status": "succeeded", "output": "{}"}, - ] - with ( - patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example", "GITHUB_REPOSITORY": "org/repo"}, clear=True), - patch("scripts.run_codex_pr_review.request_github_oidc_token", side_effect=["submit-token", "poll-token"]), - patch("scripts.run_codex_pr_review._service_request", side_effect=responses) as request, - patch("scripts.run_codex_pr_review.time.sleep"), - ): - output = run_codex_pr_review.run_codex_service_review("prompt", timeout_minutes=1) - - self.assertEqual(output, "{}") - self.assertNotIn("model", request.call_args_list[0].args[3]) - - -class CodexPrReviewWorkflowTest(unittest.TestCase): - def test_reusable_workflow_runs_bridge_script_against_source_checkout(self) -> None: - workflow = Path(".github/workflows/codex_pr_review.yml").read_text(encoding="utf-8") - self.assertIn("pull_request_target:", workflow) - self.assertNotIn(" pull_request:\n types: [opened, synchronize, reopened]", workflow) - self.assertNotIn(" review:\n if:", workflow) - self.assertIn("Reject unsupported fork pull requests", workflow) - self.assertIn("Codex review is not configured for fork pull requests", workflow) - self.assertIn("path: source", workflow) - self.assertIn("path: bridge", workflow) - self.assertIn("CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN", workflow) - self.assertIn("caller_concurrency_key", workflow) - self.assertIn("allow_unconfigured_backend", workflow) - self.assertIn("api_fallback_enabled", workflow) - self.assertIn("direct_api_primary_enabled", workflow) - self.assertIn("Optional true/false override for direct API fallback", workflow) - self.assertIn("Optional true/false override for API-only PR review", workflow) - self.assertIn('default: "false"', workflow) - self.assertIn("type: string", workflow) - self.assertNotIn("CODEX_PR_REVIEW_ALLOW_UNCONFIGURED_BACKEND", workflow) - self.assertIn("CODEX_PR_REVIEW_API_FALLBACK_ENABLED", workflow) - self.assertIn("CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED", workflow) - self.assertIn("CODEX_PR_REVIEW_REUSABLE_CALL", workflow) - self.assertIn("CODEX_PR_REVIEW_API_FALLBACK_INPUT", workflow) - self.assertIn("CODEX_PR_REVIEW_DIRECT_API_PRIMARY_INPUT", workflow) - self.assertIn("resolve_boolean", workflow) - self.assertIn("must be true or false", workflow) - self.assertIn("tr '[:upper:]' '[:lower:]'", workflow) - self.assertIn('if ! api_fallback_enabled="$(resolve_boolean', workflow) - self.assertIn('timeout --signal=TERM --kill-after=60s 25m python -I "${script_path}"', workflow) - self.assertIn("inputs.caller_concurrency_key || github.event.pull_request.number || github.run_id", workflow) - self.assertNotIn("Validate bridge checkout token", workflow) - self.assertIn("required: false", workflow) - self.assertIn("job.workflow_repository", workflow) - self.assertIn("github.event.pull_request.base.sha", workflow) - self.assertIn("github.event.pull_request.head.sha", workflow) - self.assertIn("Validate AIAuditBridge self-review ref", workflow) - self.assertIn("AIAuditBridge self-review requires pull_request_target with a PR head SHA.", workflow) - self.assertIn("persist-credentials: false", workflow) - self.assertIn("job.workflow_sha", workflow) - self.assertIn("token: ${{ secrets.CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN || github.token }}", workflow) - self.assertNotIn("CODEX_AUDIT_DISPATCH_TOKEN", workflow) - self.assertNotIn("bridge_ref", workflow) - self.assertIn("CODEX_PR_REVIEW_REPO_ROOT: ${{ github.workspace }}/source", workflow) - self.assertIn("working-directory: source", workflow) - self.assertIn("bridge/scripts/run_codex_pr_review.py", workflow) - self.assertIn("Trusted Codex review script not found", workflow) - self.assertNotIn("source/scripts/run_codex_pr_review.py", workflow) - self.assertIn("source/data/output/codex_pr_review/", workflow) - - def test_repo_root_can_be_overridden_for_reusable_workflow(self) -> None: - source = Path("scripts/run_codex_pr_review.py").read_text(encoding="utf-8") - self.assertIn("CODEX_PR_REVIEW_REPO_ROOT", source) - self.assertIn("BRIDGE_ROOT = Path(__file__).resolve().parents[1]", source) - self.assertIn("if str(BRIDGE_ROOT) not in sys.path:", source) - self.assertIn('PROMPT_TEMPLATE_PATH = BRIDGE_ROOT / "prompts" / "pr_review.md"', source) diff --git a/tests/test_single_pr_reviewer_contract.py b/tests/test_single_pr_reviewer_contract.py new file mode 100644 index 00000000..60221fe7 --- /dev/null +++ b/tests/test_single_pr_reviewer_contract.py @@ -0,0 +1,51 @@ +from __future__ import annotations + +import json +from pathlib import Path + +from service.org_health import DEFAULT_WORKFLOW_ALLOWLIST + + +ROOT = Path(__file__).resolve().parents[1] +RETIRED_PATHS = ( + ".github/workflows/codex_pr_review.yml", + "prompts/pr_review.md", + "scripts/run_codex_pr_review.py", + "tests/test_run_codex_pr_review.py", +) + + +def test_github_codex_app_is_the_only_ai_pr_reviewer() -> None: + for relative_path in RETIRED_PATHS: + assert not (ROOT / relative_path).exists(), relative_path + + actionlint_config = (ROOT / ".github/actionlint.yaml").read_text(encoding="utf-8") + assert "codex_pr_review" not in actionlint_config + + workflow_text = "\n".join( + path.read_text(encoding="utf-8") + for path in (ROOT / ".github/workflows").glob("*.yml") + ) + assert "name: Codex PR Review" not in workflow_text + assert "name: Codex Review Gate" in workflow_text + assert (ROOT / ".github/workflows/codex_audit.yml").is_file() + assert (ROOT / ".github/workflows/monthly-orchestrator.yml").is_file() + + +def test_retired_pr_reviewer_is_not_advertised_as_active() -> None: + policy = json.loads( + (ROOT / ".github/codex_auto_merge_policy.json").read_text(encoding="utf-8") + ) + assert "pr_review" not in policy + assert "Codex PR Review" not in DEFAULT_WORKFLOW_ALLOWLIST + + for relative_path in ( + "README.md", + "README.zh-CN.md", + "docs/ai_autonomy_architecture.md", + ): + content = (ROOT / relative_path).read_text(encoding="utf-8") + assert "codex_pr_review.yml" not in content + assert "run_codex_pr_review.py" not in content + assert "CODEX_PR_REVIEW_API_FALLBACK_ENABLED" not in content + assert "CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED" not in content From 816d00e65402a01763a1c5e67b4b2233180f9a82 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:23:05 +0800 Subject: [PATCH 2/6] fix(review): retain fail-closed compatibility entrypoint Co-Authored-By: Codex --- .github/workflows/codex_pr_review.yml | 43 +++++++++++++++++++++++ tests/test_single_pr_reviewer_contract.py | 8 ++++- 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/codex_pr_review.yml diff --git a/.github/workflows/codex_pr_review.yml b/.github/workflows/codex_pr_review.yml new file mode 100644 index 00000000..1ab437cc --- /dev/null +++ b/.github/workflows/codex_pr_review.yml @@ -0,0 +1,43 @@ +name: Retired Codex PR Review Compatibility + +# Compatibility-only entry point for disabled legacy callers. +# GitHub Codex App is the sole AI PR reviewer. +on: + workflow_call: + inputs: + caller_concurrency_key: + required: false + type: string + allow_unconfigured_backend: + required: false + type: boolean + default: false + api_fallback_enabled: + required: false + type: string + default: "false" + direct_api_primary_enabled: + required: false + type: string + default: "false" + secrets: + CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN: + required: false + ANTHROPIC_API_KEY: + required: false + OPENAI_API_KEY: + required: false + CODEX_AUDIT_SERVICE_URL: + required: false + +permissions: {} + +jobs: + retired: + runs-on: ubuntu-latest + timeout-minutes: 1 + steps: + - name: Reject retired AIAudit PR review calls + run: | + echo "::error::AIAudit PR review is retired; use the GitHub Codex App." + exit 1 diff --git a/tests/test_single_pr_reviewer_contract.py b/tests/test_single_pr_reviewer_contract.py index 60221fe7..a5ce4da5 100644 --- a/tests/test_single_pr_reviewer_contract.py +++ b/tests/test_single_pr_reviewer_contract.py @@ -8,17 +8,23 @@ ROOT = Path(__file__).resolve().parents[1] RETIRED_PATHS = ( - ".github/workflows/codex_pr_review.yml", "prompts/pr_review.md", "scripts/run_codex_pr_review.py", "tests/test_run_codex_pr_review.py", ) +COMPATIBILITY_WORKFLOW = ROOT / ".github/workflows/codex_pr_review.yml" def test_github_codex_app_is_the_only_ai_pr_reviewer() -> None: for relative_path in RETIRED_PATHS: assert not (ROOT / relative_path).exists(), relative_path + compatibility_workflow = COMPATIBILITY_WORKFLOW.read_text(encoding="utf-8") + assert "workflow_call:" in compatibility_workflow + assert "pull_request_target:" not in compatibility_workflow + assert "run_codex_pr_review.py" not in compatibility_workflow + assert "exit 1" in compatibility_workflow + actionlint_config = (ROOT / ".github/actionlint.yaml").read_text(encoding="utf-8") assert "codex_pr_review" not in actionlint_config From 8eae7d111238f9b9146480983898f431757303d4 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:39:11 +0800 Subject: [PATCH 3/6] fix(review): revoke retired reviewer runtime paths Co-Authored-By: Codex --- .github/workflows/vps_codex_service_ops.yml | 6 ++-- client/gateway_client.py | 7 ++-- docs/async_service_deployment.md | 8 ++--- docs/drift_oidc_rotation.md | 2 +- scripts/deploy_codex_audit_service.sh | 6 ++-- service/org_health.py | 13 ++++++- tests/test_dual_review_primary.py | 9 ++++- tests/test_oidc_reusable_workflow_auth.py | 39 ++++++++++++--------- tests/test_org_health.py | 15 ++++++++ tests/test_run_monthly_codex_audit.py | 24 +++++-------- tests/test_single_pr_reviewer_contract.py | 10 ++++++ 11 files changed, 90 insertions(+), 49 deletions(-) diff --git a/.github/workflows/vps_codex_service_ops.yml b/.github/workflows/vps_codex_service_ops.yml index 9e075eb5..e95c755c 100644 --- a/.github/workflows/vps_codex_service_ops.yml +++ b/.github/workflows/vps_codex_service_ops.yml @@ -57,11 +57,11 @@ jobs: CODEX_AUDIT_SSH_UNBAN_IP: ${{ inputs.ssh_unban_ip }} CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES: QuantStrategyLab/AIAuditBridge,QuantStrategyLab/BinancePlatform,QuantStrategyLab/CharlesSchwabPlatform,QuantStrategyLab/CnEquitySnapshotPipelines,QuantStrategyLab/CnEquityStrategies,QuantStrategyLab/CryptoLivePoolPipelines,QuantStrategyLab/CryptoStrategies,QuantStrategyLab/FirstradePlatform,QuantStrategyLab/HkEquitySnapshotPipelines,QuantStrategyLab/HkEquityStrategies,QuantStrategyLab/IBKRGatewayManager,QuantStrategyLab/InteractiveBrokersPlatform,QuantStrategyLab/LongBridgePlatform,QuantStrategyLab/MarketSignalSources,QuantStrategyLab/PoliticalEventTrackingResearch,QuantStrategyLab/QmtPlatform,QuantStrategyLab/QuantAdvisorResearch,QuantStrategyLab/QuantPlatformKit,QuantStrategyLab/QuantRuntimeSettings,QuantStrategyLab/QuantStrategyPlugins,QuantStrategyLab/ResearchSignalContextPipelines,QuantStrategyLab/SchwabTokenAutoRefresher,QuantStrategyLab/UsEquitySnapshotPipelines,QuantStrategyLab/UsEquityStrategies # workflow_dispatch emits protected-main workflow_ref claims; the deploy script pins delegated QPK code by exact job_workflow_ref SHA. - CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS: QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main,QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/BinancePlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CharlesSchwabPlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CnEquitySnapshotPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CnEquityStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CryptoLivePoolPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CryptoStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/FirstradePlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/HkEquitySnapshotPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/HkEquityStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/IBKRGatewayManager/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/InteractiveBrokersPlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/LongBridgePlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/MarketSignalSources/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/PoliticalEventTrackingResearch/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QmtPlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantAdvisorResearch/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantPlatformKit/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantStrategyPlugins/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/ResearchSignalContextPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/SchwabTokenAutoRefresher/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/UsEquitySnapshotPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/UsEquityStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CnEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/UsEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/CryptoStrategies/.github/workflows/drift-check.yml@refs/heads/main - CODEX_AUDIT_SERVICE_ALLOWED_REFS: refs/heads/main,refs/pull/*/merge + CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS: QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main,QuantStrategyLab/CnEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/UsEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/CryptoStrategies/.github/workflows/drift-check.yml@refs/heads/main + CODEX_AUDIT_SERVICE_ALLOWED_REFS: refs/heads/main # Rotation tracked in #64; remove the old SHA by 2026-07-18 after final strategy-run verification. # Must match the immutable QPK `uses:` ref pinned by all strategy drift callers. - CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS: QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@86458c44b06593b6d7a1602b3c38e7a1c143ef17,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@d0a081ca5868faaf1a6dd870cf4b93643978cd11,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@fcddef20eea5deb876e739263042acdcb3e9cd1b,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@4f8465b28a6787d39d21e50f9d95a77841d6ad56,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@651c9ac4f37ce6e7fe1bac84dc7646cd5abc9e6e + CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS: QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@d0a081ca5868faaf1a6dd870cf4b93643978cd11,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@fcddef20eea5deb876e739263042acdcb3e9cd1b,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@4f8465b28a6787d39d21e50f9d95a77841d6ad56,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@651c9ac4f37ce6e7fe1bac84dc7646cd5abc9e6e CODEX_AUDIT_SERVICE_ALLOWED_DIRECT_REPOSITORIES: QuantStrategyLab/AIAuditBridge CODEX_AUDIT_SERVICE_ALLOWED_SOURCE_REPOSITORIES: QuantStrategyLab/AIAuditBridge,QuantStrategyLab/BinancePlatform,QuantStrategyLab/CharlesSchwabPlatform,QuantStrategyLab/CnEquitySnapshotPipelines,QuantStrategyLab/CnEquityStrategies,QuantStrategyLab/CryptoLivePoolPipelines,QuantStrategyLab/CryptoStrategies,QuantStrategyLab/FirstradePlatform,QuantStrategyLab/HkEquitySnapshotPipelines,QuantStrategyLab/HkEquityStrategies,QuantStrategyLab/IBKRGatewayManager,QuantStrategyLab/InteractiveBrokersPlatform,QuantStrategyLab/LongBridgePlatform,QuantStrategyLab/MarketSignalSources,QuantStrategyLab/PoliticalEventTrackingResearch,QuantStrategyLab/QmtPlatform,QuantStrategyLab/QuantAdvisorResearch,QuantStrategyLab/QuantPlatformKit,QuantStrategyLab/QuantRuntimeSettings,QuantStrategyLab/QuantStrategyPlugins,QuantStrategyLab/ResearchSignalContextPipelines,QuantStrategyLab/SchwabTokenAutoRefresher,QuantStrategyLab/UsEquitySnapshotPipelines,QuantStrategyLab/UsEquityStrategies CODEX_AUDIT_SERVICE_MODEL: ${{ vars.CODEX_AUDIT_SERVICE_MODEL }} diff --git a/client/gateway_client.py b/client/gateway_client.py index ec7d06ea..0777f0ff 100644 --- a/client/gateway_client.py +++ b/client/gateway_client.py @@ -157,7 +157,7 @@ def execute( started = time.time() try: - token = _fetch_oidc_token(self.config.audience) + submit_token = _fetch_oidc_token(self.config.audience) payload = json.dumps({ "task": task, "prompt": prompt, @@ -174,7 +174,7 @@ def execute( f"{self.config.service_url}/v1/ai/execute/jobs", data=payload, method="POST", - headers=_headers(token), + headers=_headers(submit_token), ) with urllib.request.urlopen(req, timeout=30) as resp: job = json.loads(resp.read().decode("utf-8")) @@ -187,10 +187,11 @@ def execute( deadline = time.time() + timeout + 60 while time.time() < deadline: time.sleep(poll_interval) + poll_token = _fetch_oidc_token(self.config.audience) req2 = urllib.request.Request( f"{self.config.service_url}/v1/ai/execute/jobs/{job_id}", method="GET", - headers=_headers(token), + headers=_headers(poll_token), ) try: with urllib.request.urlopen(req2, timeout=30) as resp2: diff --git a/docs/async_service_deployment.md b/docs/async_service_deployment.md index 4a1a6b53..bf42ebdb 100644 --- a/docs/async_service_deployment.md +++ b/docs/async_service_deployment.md @@ -31,9 +31,9 @@ The historical self-hosted direct-Codex workflows in `SelfHostedCodexAuditBridge - `QuantStrategyLab/AIAuditBridge` is public, so it may contain only client/orchestration code. Its service URL, provider fallback keys, GitHub App private key, and Cloudflare origin stay in GitHub or Cloudflare secrets. - The VPS service should allow only `QuantStrategyLab/AIAuditBridge` in `CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES`. - The VPS service should require explicit `CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS` for the canonical `AIAuditBridge` workflows only. -- The VPS service should keep `CODEX_AUDIT_SERVICE_ALLOWED_REFS` as narrow as the enabled workflows allow, normally `refs/heads/main` plus `refs/pull/*/merge` for PR review smoke tests. +- The VPS service should keep `CODEX_AUDIT_SERVICE_ALLOWED_REFS` as narrow as the enabled workflows allow, normally `refs/heads/main`. - Keep `CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORY_VISIBILITIES=public` unless the bridge repository is intentionally private. -- The VPS service should keep `CODEX_AUDIT_SERVICE_ALLOWED_SOURCE_REPOSITORIES` limited to current audit source repositories and PR review targets. +- The VPS service should keep `CODEX_AUDIT_SERVICE_ALLOWED_SOURCE_REPOSITORIES` limited to current audit and drift source repositories. - The Cloudflare Worker stores only `CODEX_AUDIT_ORIGIN_URL` as a Worker secret. Do not commit the origin URL if it exposes infrastructure details. - Job IDs are random and status reads still require service authentication. Job responses never include the original prompt. - Static service bearer tokens are no longer supported; production calls must use GitHub Actions OIDC. @@ -56,8 +56,8 @@ After merging the async service code, run the manual `VPS Codex Service Ops` wor ```bash CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES=QuantStrategyLab/AIAuditBridge \ -CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS='QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main,QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/pull/*/merge' \ -CODEX_AUDIT_SERVICE_ALLOWED_REFS='refs/heads/main,refs/pull/*/merge' \ +CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS='QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main' \ +CODEX_AUDIT_SERVICE_ALLOWED_REFS='refs/heads/main' \ CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORY_VISIBILITIES='public' \ CODEX_AUDIT_SERVICE_ALLOWED_SOURCE_REPOSITORIES='QuantStrategyLab/AIAuditBridge,QuantStrategyLab/CryptoLivePoolPipelines,QuantStrategyLab/HkEquitySnapshotPipelines,QuantStrategyLab/UsEquitySnapshotPipelines,QuantStrategyLab/ResearchSignalContextPipelines' \ CODEX_AUDIT_SERVICE_AUDIENCE=quant-codex-audit \ diff --git a/docs/drift_oidc_rotation.md b/docs/drift_oidc_rotation.md index 9fc120b4..44cf4777 100644 --- a/docs/drift_oidc_rotation.md +++ b/docs/drift_oidc_rotation.md @@ -12,6 +12,6 @@ To rotate the QPK reusable workflow without an untrusted or unavailable window: Current rotation: retain `644cd9002ae92f2aaca6f7efb4afa4986fae05ea`, `d0a081ca5868faaf1a6dd870cf4b93643978cd11`, `fcddef20eea5deb876e739263042acdcb3e9cd1b`, and `4f8465b28a6787d39d21e50f9d95a77841d6ad56` only until CN, US, and crypto are verified on `651c9ac4f37ce6e7fe1bac84dc7646cd5abc9e6e`; [issue #64](https://github.com/QuantStrategyLab/AIAuditBridge/issues/64) tracks removal by 2026-07-18. The deploy workflow verifies that every allowlisted QPK SHA resolves to `reusable-drift-check.yml` before changing the service. -Never use a wildcard for `job_workflow_ref`. Strategy drift delegation must use an exact QPK SHA. The existing AIAuditBridge PR-review entry remains on protected `main` only while organization consumers still call `codex_pr_review.yml@main`; migrate that entry to a SHA only together with all consumer workflow pins. +Never use a wildcard for `job_workflow_ref`. Strategy drift delegation must use an exact QPK SHA. AIAuditBridge PR-review OIDC entries are retired and must not be restored; GitHub Codex App is the sole AI PR reviewer. The service also enforces that any allowed strategy `drift-check.yml` caller presents a `job_workflow_ref` for QuantPlatformKit's `reusable-drift-check.yml`. A different allowlisted reusable workflow cannot be substituted. diff --git a/scripts/deploy_codex_audit_service.sh b/scripts/deploy_codex_audit_service.sh index 5204c8e7..65d00b02 100644 --- a/scripts/deploy_codex_audit_service.sh +++ b/scripts/deploy_codex_audit_service.sh @@ -11,12 +11,12 @@ ALLOWED_REPOSITORIES="${CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES:-QuantStrategyL # Direct review and strategy workflow identities are pinned to protected main because GitHub emits workflow_ref with the dispatch branch. # Delegated reusable code is constrained separately by the exact job_workflow_ref SHA below. # The ref allowlist retains PR merge refs because GitHub can preserve the incoming PR ref for reusable calls; _verify_github_oidc requires both allowlists. -ALLOWED_WORKFLOW_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS:-QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main,QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/BinancePlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CharlesSchwabPlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CnEquitySnapshotPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CnEquityStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CryptoLivePoolPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CryptoStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/FirstradePlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/HkEquitySnapshotPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/HkEquityStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/IBKRGatewayManager/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/InteractiveBrokersPlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/LongBridgePlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/MarketSignalSources/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/PoliticalEventTrackingResearch/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QmtPlatform/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantAdvisorResearch/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantPlatformKit/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/QuantStrategyPlugins/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/ResearchSignalContextPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/SchwabTokenAutoRefresher/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/UsEquitySnapshotPipelines/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/UsEquityStrategies/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/CnEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/UsEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/CryptoStrategies/.github/workflows/drift-check.yml@refs/heads/main}" -ALLOWED_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_REFS:-refs/heads/main,refs/pull/*/merge}" +ALLOWED_WORKFLOW_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS:-QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main,QuantStrategyLab/CnEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/UsEquityStrategies/.github/workflows/drift-check.yml@refs/heads/main,QuantStrategyLab/CryptoStrategies/.github/workflows/drift-check.yml@refs/heads/main}" +ALLOWED_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_REFS:-refs/heads/main}" ALLOWED_REPOSITORY_VISIBILITIES="${CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORY_VISIBILITIES:-public}" # Single source of truth for delegated drift code. Rotation #64 removes the old SHA by 2026-07-18. # Rotate with the two-SHA procedure in docs/drift_oidc_rotation.md. -ALLOWED_JOB_WORKFLOW_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS:-QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main,QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@86458c44b06593b6d7a1602b3c38e7a1c143ef17,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@d0a081ca5868faaf1a6dd870cf4b93643978cd11,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@fcddef20eea5deb876e739263042acdcb3e9cd1b,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@4f8465b28a6787d39d21e50f9d95a77841d6ad56,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@651c9ac4f37ce6e7fe1bac84dc7646cd5abc9e6e}" +ALLOWED_JOB_WORKFLOW_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS:-QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@d0a081ca5868faaf1a6dd870cf4b93643978cd11,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@fcddef20eea5deb876e739263042acdcb3e9cd1b,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@4f8465b28a6787d39d21e50f9d95a77841d6ad56,QuantStrategyLab/QuantPlatformKit/.github/workflows/reusable-drift-check.yml@651c9ac4f37ce6e7fe1bac84dc7646cd5abc9e6e}" ALLOWED_DIRECT_REPOSITORIES="${CODEX_AUDIT_SERVICE_ALLOWED_DIRECT_REPOSITORIES:-QuantStrategyLab/AIAuditBridge}" ALLOWED_SOURCE_REPOSITORIES="${CODEX_AUDIT_SERVICE_ALLOWED_SOURCE_REPOSITORIES:-QuantStrategyLab/AIAuditBridge,QuantStrategyLab/BinancePlatform,QuantStrategyLab/CharlesSchwabPlatform,QuantStrategyLab/CnEquitySnapshotPipelines,QuantStrategyLab/CnEquityStrategies,QuantStrategyLab/CryptoLivePoolPipelines,QuantStrategyLab/CryptoStrategies,QuantStrategyLab/FirstradePlatform,QuantStrategyLab/HkEquitySnapshotPipelines,QuantStrategyLab/HkEquityStrategies,QuantStrategyLab/IBKRGatewayManager,QuantStrategyLab/InteractiveBrokersPlatform,QuantStrategyLab/LongBridgePlatform,QuantStrategyLab/MarketSignalSources,QuantStrategyLab/PoliticalEventTrackingResearch,QuantStrategyLab/QmtPlatform,QuantStrategyLab/QuantAdvisorResearch,QuantStrategyLab/QuantPlatformKit,QuantStrategyLab/QuantRuntimeSettings,QuantStrategyLab/QuantStrategyPlugins,QuantStrategyLab/ResearchSignalContextPipelines,QuantStrategyLab/SchwabTokenAutoRefresher,QuantStrategyLab/UsEquitySnapshotPipelines,QuantStrategyLab/UsEquityStrategies}" JOB_DIR="${CODEX_AUDIT_SERVICE_JOB_DIR:-/var/lib/codex-audit-bridge/jobs}" diff --git a/service/org_health.py b/service/org_health.py index 0e26c945..83af3e15 100644 --- a/service/org_health.py +++ b/service/org_health.py @@ -52,6 +52,8 @@ "VPS Codex Service Ops", "ci", ) +_RETIRED_WORKFLOW_NAMES = {"codex pr review"} +_RETIRED_WORKFLOW_FILENAMES = {"codex_pr_review.yml"} def _split_repo_env(name: str) -> list[str]: @@ -359,11 +361,20 @@ def _workflow_matches_allowlist(workflow: dict[str, Any], allowlist: set[str]) - return name in allowlist or path in allowlist or filename in allowlist +def _is_retired_workflow(workflow: dict[str, Any]) -> bool: + name = str(workflow.get("name") or "").strip().lower() + path = str(workflow.get("path") or "").strip().lower() + filename = path.rsplit("/", 1)[-1] + return name in _RETIRED_WORKFLOW_NAMES or filename in _RETIRED_WORKFLOW_FILENAMES + + def _monitored_workflows(workflows: list[dict[str, Any]]) -> list[dict[str, Any]]: active = [ workflow for workflow in workflows - if isinstance(workflow, dict) and not str(workflow.get("state") or "").startswith("disabled") + if isinstance(workflow, dict) + and not str(workflow.get("state") or "").startswith("disabled") + and not _is_retired_workflow(workflow) ] allowlist = _workflow_allowlist() selected = [workflow for workflow in active if _workflow_matches_allowlist(workflow, allowlist)] diff --git a/tests/test_dual_review_primary.py b/tests/test_dual_review_primary.py index ebe6cf4d..8f0f1ffd 100644 --- a/tests/test_dual_review_primary.py +++ b/tests/test_dual_review_primary.py @@ -24,7 +24,10 @@ def test_gateway_execute_preserves_task_and_complexity(self) -> None: ) with ( - patch("client.gateway_client._fetch_oidc_token", return_value="oidc"), + patch( + "client.gateway_client._fetch_oidc_token", + side_effect=["submit-oidc", "poll-oidc"], + ) as fetch_oidc_token, patch( "client.gateway_client.urllib.request.urlopen", side_effect=[submit_response, poll_response], @@ -43,6 +46,10 @@ def test_gateway_execute_preserves_task_and_complexity(self) -> None: payload = json.loads(request.data) self.assertEqual(payload["task"], "promotion_review") self.assertEqual(payload["complexity"], "high") + self.assertEqual(fetch_oidc_token.call_count, 2) + self.assertEqual(request.get_header("Authorization"), "Bearer submit-oidc") + poll_request = urlopen.call_args_list[1].args[0] + self.assertEqual(poll_request.get_header("Authorization"), "Bearer poll-oidc") def test_build_primary_prompt_includes_evidence_summary(self) -> None: from pathlib import Path diff --git a/tests/test_oidc_reusable_workflow_auth.py b/tests/test_oidc_reusable_workflow_auth.py index 17566985..167ae121 100644 --- a/tests/test_oidc_reusable_workflow_auth.py +++ b/tests/test_oidc_reusable_workflow_auth.py @@ -20,57 +20,62 @@ def _verify(self, payload: dict[str, object], env: dict[str, str]) -> dict[str, return auth.verify_github_oidc("header.payload.signature") def test_non_direct_caller_requires_trusted_reusable_workflow(self) -> None: + qpk_job_ref = ( + "QuantStrategyLab/QuantPlatformKit/.github/workflows/" + "reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea" + ) payload: dict[str, object] = { "aud": "quant-codex-audit", "iss": auth.GITHUB_OIDC_ISSUER, "exp": int(time.time()) + 300, "repository": "QuantStrategyLab/QuantRuntimeSettings", - "workflow_ref": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main", + "workflow_ref": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/ci.yml@refs/heads/main", "ref": "refs/heads/main", "repository_visibility": "public", } env = { "CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES": "QuantStrategyLab/AIAuditBridge,QuantStrategyLab/QuantRuntimeSettings", - "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main", + "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/ci.yml@refs/heads/main", "CODEX_AUDIT_SERVICE_ALLOWED_REFS": "refs/heads/main", "CODEX_AUDIT_SERVICE_ALLOWED_DIRECT_REPOSITORIES": "QuantStrategyLab/AIAuditBridge", - "CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS": "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main", + "CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS": qpk_job_ref, "CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORY_VISIBILITIES": "public", } with self.assertRaisesRegex(PermissionError, "job workflow ref is required"): self._verify(payload, env) - payload["job_workflow_ref"] = "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main" + payload["job_workflow_ref"] = qpk_job_ref self.assertEqual(self._verify(payload, env)["repository"], "QuantStrategyLab/QuantRuntimeSettings") - def test_exact_audit_bridge_sha_is_allowlisted_without_broadening_job_ref(self) -> None: - exact_job_ref = ( + def test_retired_audit_bridge_sha_is_not_allowlisted(self) -> None: + retired_job_ref = ( "QuantStrategyLab/AIAuditBridge/.github/workflows/" "codex_pr_review.yml@86458c44b06593b6d7a1602b3c38e7a1c143ef17" ) + qpk_job_ref = ( + "QuantStrategyLab/QuantPlatformKit/.github/workflows/" + "reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea" + ) payload: dict[str, object] = { "aud": "quant-codex-audit", "iss": auth.GITHUB_OIDC_ISSUER, "exp": int(time.time()) + 300, "repository": "QuantStrategyLab/QuantRuntimeSettings", - "workflow_ref": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main", - "job_workflow_ref": exact_job_ref, + "workflow_ref": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/ci.yml@refs/heads/main", + "job_workflow_ref": retired_job_ref, "ref": "refs/heads/main", "repository_visibility": "public", } env = { "CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES": "QuantStrategyLab/QuantRuntimeSettings", - "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main", + "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/ci.yml@refs/heads/main", "CODEX_AUDIT_SERVICE_ALLOWED_REFS": "refs/heads/main", "CODEX_AUDIT_SERVICE_ALLOWED_DIRECT_REPOSITORIES": "QuantStrategyLab/AIAuditBridge", - "CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS": exact_job_ref, + "CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS": qpk_job_ref, "CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORY_VISIBILITIES": "public", } - self.assertEqual(self._verify(payload, env)["repository"], "QuantStrategyLab/QuantRuntimeSettings") - - payload["job_workflow_ref"] = f"{exact_job_ref}0" with self.assertRaisesRegex(PermissionError, "job workflow ref is not allowed"): self._verify(payload, env) @@ -80,15 +85,15 @@ def test_direct_audit_bridge_caller_does_not_require_reusable_workflow(self) -> "iss": auth.GITHUB_OIDC_ISSUER, "exp": int(time.time()) + 300, "repository": "QuantStrategyLab/AIAuditBridge", - "workflow_ref": "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main", + "workflow_ref": "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main", "ref": "refs/heads/main", } env = { "CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES": "QuantStrategyLab/AIAuditBridge", - "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main", + "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main", "CODEX_AUDIT_SERVICE_ALLOWED_REFS": "refs/heads/main", "CODEX_AUDIT_SERVICE_ALLOWED_DIRECT_REPOSITORIES": "QuantStrategyLab/AIAuditBridge", - "CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS": "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main", + "CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS": "", } self.assertEqual(self._verify(payload, env)["repository"], "QuantStrategyLab/AIAuditBridge") @@ -114,7 +119,7 @@ def test_strategy_drift_requires_trusted_qpk_reusable_workflow(self) -> None: "QuantStrategyLab/QuantPlatformKit/.github/workflows/" "reusable-drift-check.yml@644cd9002ae92f2aaca6f7efb4afa4986fae05ea" ) - audit_job_ref = "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main" + audit_job_ref = "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main" payload: dict[str, object] = { "aud": "quant-codex-audit", "iss": auth.GITHUB_OIDC_ISSUER, diff --git a/tests/test_org_health.py b/tests/test_org_health.py index cc5c9599..1d8bdb55 100644 --- a/tests/test_org_health.py +++ b/tests/test_org_health.py @@ -444,6 +444,21 @@ def test_read_org_health_limits_default_monitored_workflows(self) -> None: selected = org_health._monitored_workflows(workflows) self.assertEqual([item["name"] for item in selected], ["CI", "Codex Review Gate"]) + def test_read_org_health_fallback_excludes_retired_pr_reviewer(self) -> None: + workflows = [ + { + "id": 1, + "name": "Codex PR Review", + "path": ".github/workflows/codex_pr_review.yml", + "state": "active", + }, + {"id": 2, "name": "Custom Health", "state": "active"}, + ] + + selected = org_health._monitored_workflows(workflows) + + self.assertEqual([item["name"] for item in selected], ["Custom Health"]) + def test_read_org_health_serves_expired_stale_cache_and_refreshes_in_background(self) -> None: cache_key = (("QuantStrategyLab/cached",), "CODEX_AUDIT_SERVICE_GITHUB_TOKEN", "token", "all") cached_result = { diff --git a/tests/test_run_monthly_codex_audit.py b/tests/test_run_monthly_codex_audit.py index b466d674..471d77cd 100644 --- a/tests/test_run_monthly_codex_audit.py +++ b/tests/test_run_monthly_codex_audit.py @@ -330,7 +330,7 @@ def test_codex_audit_service_oidc_rejects_missing_workflow_allowlist(self) -> No ): codex_audit_service._verify_github_oidc("header.payload.signature") - def test_codex_audit_service_oidc_rejects_pilot_pr_workflow_ref(self) -> None: + def test_codex_audit_service_oidc_rejects_retired_pr_workflow_ref(self) -> None: payload = { "aud": "quant-codex-audit", "iss": codex_audit_service.GITHUB_OIDC_ISSUER, @@ -343,8 +343,7 @@ def test_codex_audit_service_oidc_rejects_pilot_pr_workflow_ref(self) -> None: env = { "CODEX_AUDIT_SERVICE_ALLOWED_REPOSITORIES": "QuantStrategyLab/AIAuditBridge,QuantStrategyLab/QuantRuntimeSettings", "CODEX_AUDIT_SERVICE_ALLOWED_WORKFLOW_REFS": ( - "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_pr_review.yml@refs/heads/main," - "QuantStrategyLab/QuantRuntimeSettings/.github/workflows/codex_pr_review.yml@refs/heads/main" + "QuantStrategyLab/AIAuditBridge/.github/workflows/codex_audit.yml@refs/heads/main" ), "CODEX_AUDIT_SERVICE_ALLOWED_REFS": "refs/heads/main,refs/pull/*/merge", "CODEX_AUDIT_SERVICE_ALLOWED_DIRECT_REPOSITORIES": "QuantStrategyLab/AIAuditBridge", @@ -2424,8 +2423,8 @@ def test_vps_deploy_adds_nginx_audit_route_without_router_service(self) -> None: self.assertIn("os.open(component, flags_dir, dir_fd=fd)", deploy_script) self.assertIn('"max_consecutive_failures": 3', deploy_script) self.assertIn("workflow_ref with the dispatch branch", deploy_script) - self.assertIn('ALLOWED_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_REFS:-refs/heads/main,refs/pull/*/merge}"', deploy_script) - self.assertNotIn("codex_pr_review.yml@refs/pull/*/merge", deploy_script) + self.assertIn('ALLOWED_REFS="${CODEX_AUDIT_SERVICE_ALLOWED_REFS:-refs/heads/main}"', deploy_script) + self.assertNotIn("codex_pr_review.yml@", deploy_script) self.assertIn("QuantStrategyLab/AIAuditBridge", deploy_script) self.assertIn("QuantStrategyLab/QuantRuntimeSettings", deploy_script) self.assertIn("QuantStrategyLab/QuantPlatformKit", deploy_script) @@ -2462,7 +2461,7 @@ def test_vps_deploy_adds_nginx_audit_route_without_router_service(self) -> None: self.assertIn("protected `main`", rotation) self.assertIn("both the current and next exact QPK SHAs", rotation) self.assertIn("Never use a wildcard", rotation) - self.assertIn("PR-review entry remains on protected `main`", rotation) + self.assertIn("PR-review OIDC entries are retired", rotation) self.assertIn("different allowlisted reusable workflow cannot be substituted", rotation) self.assertIn("issue #64", rotation) self.assertIn("resolves to `reusable-drift-check.yml`", rotation) @@ -2511,11 +2510,7 @@ def test_vps_deploy_defaults_match_workflow_allowlists(self) -> None: workflow_value = workflow_line.removeprefix(f"{workflow_name}: ") self.assertEqual(script_value, workflow_value) - def test_vps_deploy_persists_exact_audit_bridge_pr_review_sha(self) -> None: - exact_job_ref = ( - "QuantStrategyLab/AIAuditBridge/.github/workflows/" - "codex_pr_review.yml@86458c44b06593b6d7a1602b3c38e7a1c143ef17" - ) + def test_vps_deploy_excludes_retired_audit_bridge_pr_review_refs(self) -> None: deploy_script = Path("scripts/deploy_codex_audit_service.sh").read_text(encoding="utf-8") workflow = Path(".github/workflows/vps_codex_service_ops.yml").read_text(encoding="utf-8") script_line = next( @@ -2526,11 +2521,8 @@ def test_vps_deploy_persists_exact_audit_bridge_pr_review_sha(self) -> None: for line in workflow.splitlines() if line.strip().startswith("CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS: ") ) - script_value = script_line.split(":-", 1)[1][:-2] - workflow_value = workflow_line.removeprefix("CODEX_AUDIT_SERVICE_ALLOWED_JOB_WORKFLOW_REFS: ") - - self.assertIn(exact_job_ref, script_value.split(",")) - self.assertIn(exact_job_ref, workflow_value.split(",")) + self.assertNotIn("codex_pr_review.yml@", script_line) + self.assertNotIn("codex_pr_review.yml@", workflow_line) if __name__ == "__main__": diff --git a/tests/test_single_pr_reviewer_contract.py b/tests/test_single_pr_reviewer_contract.py index fa351074..148d02ff 100644 --- a/tests/test_single_pr_reviewer_contract.py +++ b/tests/test_single_pr_reviewer_contract.py @@ -57,3 +57,13 @@ def test_retired_pr_reviewer_is_not_advertised_as_active() -> None: assert "run_codex_pr_review.py" not in content assert "CODEX_PR_REVIEW_API_FALLBACK_ENABLED" not in content assert "CODEX_PR_REVIEW_DIRECT_API_PRIMARY_ENABLED" not in content + + +def test_retired_pr_reviewer_is_not_authorized_by_deployment_defaults() -> None: + for relative_path in ( + ".github/workflows/vps_codex_service_ops.yml", + "scripts/deploy_codex_audit_service.sh", + ): + content = (ROOT / relative_path).read_text(encoding="utf-8") + assert "codex_pr_review.yml@" not in content + assert "86458c44b06593b6d7a1602b3c38e7a1c143ef17" not in content From 0c3ba0d81d058215d92943233ad3fab00ab54c67 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:43:43 +0800 Subject: [PATCH 4/6] fix(review): preserve structured capacity failures Co-Authored-By: Codex --- service/dual_review_primary.py | 8 +++++++- tests/test_dual_review_primary.py | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/service/dual_review_primary.py b/service/dual_review_primary.py index 7b7eada3..ed7699f3 100644 --- a/service/dual_review_primary.py +++ b/service/dual_review_primary.py @@ -97,6 +97,9 @@ def run_codex_primary_review( ) if not result.success: message = result.error or result.note or "Codex service review unavailable" + failure_category = "" + if isinstance(result.raw, dict): + failure_category = str(result.raw.get("failure_category") or "").strip().lower() unavailable_markers = ( "daily budget exceeded", "quota", @@ -107,7 +110,10 @@ def run_codex_primary_review( "request timed out", "too many active jobs", ) - verdict = VERDICT_UNAVAILABLE if any(marker in message.lower() for marker in unavailable_markers) else VERDICT_INVALID + unavailable = failure_category == "quota_or_capacity_failure" or any( + marker in message.lower() for marker in unavailable_markers + ) + verdict = VERDICT_UNAVAILABLE if unavailable else VERDICT_INVALID return { "source": "codex_primary", "verdict": verdict, diff --git a/tests/test_dual_review_primary.py b/tests/test_dual_review_primary.py index 8f0f1ffd..b48a5849 100644 --- a/tests/test_dual_review_primary.py +++ b/tests/test_dual_review_primary.py @@ -101,6 +101,21 @@ def test_capacity_error_is_unavailable(self, review) -> None: result = run_codex_primary_review(prompt="review") self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) + @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) + @patch("service.dual_review_primary.AiGatewayClient.execute") + def test_structured_capacity_failure_is_unavailable(self, review) -> None: + review.return_value = AiResult( + provider="codex", + model="codex-cli", + success=False, + error="rate limit exceeded", + raw={"failure_category": "quota_or_capacity_failure"}, + ) + + result = run_codex_primary_review(prompt="review") + + self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) + @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) @patch("service.dual_review_primary.AiGatewayClient.execute") def test_protocol_error_is_invalid(self, review) -> None: From 6c37ff14df47a7673d65045eec8ea4d5a6c07511 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:47:40 +0800 Subject: [PATCH 5/6] test(review): isolate repository environment Co-Authored-By: Codex --- tests/test_dual_review_primary.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/test_dual_review_primary.py b/tests/test_dual_review_primary.py index b48a5849..1387cf93 100644 --- a/tests/test_dual_review_primary.py +++ b/tests/test_dual_review_primary.py @@ -76,7 +76,13 @@ def test_parse_primary_review_output(self) -> None: self.assertEqual(review["verdict"], "approve") self.assertEqual(review["source"], "codex_primary") - @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) + @patch.dict( + "os.environ", + { + "CODEX_AUDIT_SERVICE_URL": "https://service.invalid", + "GITHUB_REPOSITORY": "", + }, + ) @patch("service.dual_review_primary.AiGatewayClient.execute") def test_budget_error_is_unavailable(self, review) -> None: review.return_value = AiResult.unavailable("codex", "Daily budget exceeded") From d1fa49c8f2526ed30430d03497801a973c0dddf6 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:59:01 +0800 Subject: [PATCH 6/6] fix(review): preserve gateway failure semantics Co-Authored-By: Codex --- client/gateway_client.py | 59 +++++++++++++++++++++--- service/dual_review_primary.py | 10 ++++- tests/test_dual_review_primary.py | 74 ++++++++++++++++++++++++++++++- 3 files changed, 133 insertions(+), 10 deletions(-) diff --git a/client/gateway_client.py b/client/gateway_client.py index 0777f0ff..981187f6 100644 --- a/client/gateway_client.py +++ b/client/gateway_client.py @@ -34,8 +34,22 @@ class AiResult: raw: Any = None @classmethod - def unavailable(cls, provider: str, reason: str) -> "AiResult": - return cls(provider=provider, model="", success=False, error=reason, note=reason) + def unavailable( + cls, + provider: str, + reason: str, + *, + failure_category: str = "", + ) -> "AiResult": + raw = {"failure_category": failure_category} if failure_category else None + return cls( + provider=provider, + model="", + success=False, + error=reason, + note=reason, + raw=raw, + ) @dataclass(frozen=True) @@ -151,12 +165,12 @@ def execute( poll_interval: float | None = None, ) -> AiResult: """Async Codex execution via ``POST /v1/ai/execute/jobs`` + polling.""" - self._breaker.before_call() timeout = timeout or self.config.timeout_execute poll_interval = poll_interval or self.config.poll_interval started = time.time() try: + self._breaker.before_call() submit_token = _fetch_oidc_token(self.config.audience) payload = json.dumps({ "task": task, @@ -181,7 +195,11 @@ def execute( job_id = job.get("job_id") if not isinstance(job_id, str) or not job_id: - return AiResult.unavailable("codex", "No job_id from gateway") + return AiResult.unavailable( + "codex", + "No job_id from gateway", + failure_category="patch_contract_failure", + ) # Poll until completion deadline = time.time() + timeout + 60 @@ -216,14 +234,41 @@ def execute( ) self._breaker.on_failure() - return AiResult.unavailable("codex", "Job polling timed out") + return AiResult.unavailable( + "codex", + "Job polling timed out", + failure_category="transient_service_failure", + ) except CircuitBreakerOpenError: - return AiResult.unavailable("codex", "Circuit breaker open — service unavailable") + return AiResult.unavailable( + "codex", + "Circuit breaker open — service unavailable", + failure_category="transient_service_failure", + ) except urllib.error.HTTPError as exc: self._breaker.on_failure() body = exc.read().decode("utf-8", errors="replace")[:500] - return AiResult.unavailable("codex", f"HTTP {exc.code}: {body}") + if exc.code == 429: + category = "quota_or_capacity_failure" + elif exc.code >= 500: + category = "transient_service_failure" + elif exc.code in {401, 403}: + category = "auth_or_config_failure" + else: + category = "unknown_failure" + return AiResult.unavailable( + "codex", + f"HTTP {exc.code}: {body}", + failure_category=category, + ) + except (urllib.error.URLError, OSError) as exc: + self._breaker.on_failure() + return AiResult.unavailable( + "codex", + str(exc), + failure_category="transient_service_failure", + ) except Exception as exc: self._breaker.on_failure() return AiResult.unavailable("codex", str(exc)) diff --git a/service/dual_review_primary.py b/service/dual_review_primary.py index ed7699f3..7f1bd375 100644 --- a/service/dual_review_primary.py +++ b/service/dual_review_primary.py @@ -89,7 +89,7 @@ def run_codex_primary_review( timeout = int(timeout_minutes or os.environ.get("DUAL_REVIEW_PRIMARY_TIMEOUT_MINUTES", "15")) result = AiGatewayClient(GatewayConfig.from_env()).execute( prompt, - task="promotion_review", + task="dual_review", mode="review_only", complexity="high", source_repository=os.environ.get("GITHUB_REPOSITORY") or None, @@ -110,7 +110,13 @@ def run_codex_primary_review( "request timed out", "too many active jobs", ) - unavailable = failure_category == "quota_or_capacity_failure" or any( + unavailable = failure_category in { + "auth_or_config_failure", + "quota_or_capacity_failure", + "service_restart", + "stale_job_timeout", + "transient_service_failure", + } or any( marker in message.lower() for marker in unavailable_markers ) verdict = VERDICT_UNAVAILABLE if unavailable else VERDICT_INVALID diff --git a/tests/test_dual_review_primary.py b/tests/test_dual_review_primary.py index 1387cf93..da32c4a3 100644 --- a/tests/test_dual_review_primary.py +++ b/tests/test_dual_review_primary.py @@ -2,9 +2,11 @@ import json import unittest +import urllib.error from unittest.mock import MagicMock, patch from client.config import GatewayConfig +from client.errors import CircuitBreakerOpenError from client.gateway_client import AiGatewayClient, AiResult from service.dual_review import VERDICT_INVALID, VERDICT_UNAVAILABLE from service.dual_review_primary import build_primary_prompt, parse_primary_review_output, run_codex_primary_review @@ -51,6 +53,40 @@ def test_gateway_execute_preserves_task_and_complexity(self) -> None: poll_request = urlopen.call_args_list[1].args[0] self.assertEqual(poll_request.get_header("Authorization"), "Bearer poll-oidc") + def test_gateway_execute_labels_network_failure(self) -> None: + config = GatewayConfig( + service_url="https://service.invalid", + source_repository="QuantStrategyLab/AIAuditBridge", + ) + with ( + patch("client.gateway_client._fetch_oidc_token", return_value="oidc"), + patch( + "client.gateway_client.urllib.request.urlopen", + side_effect=urllib.error.URLError("temporary DNS failure"), + ), + ): + result = AiGatewayClient(config).execute("review") + + self.assertFalse(result.success) + self.assertEqual(result.raw, {"failure_category": "transient_service_failure"}) + + def test_gateway_execute_labels_open_circuit(self) -> None: + client = AiGatewayClient( + GatewayConfig( + service_url="https://service.invalid", + source_repository="QuantStrategyLab/AIAuditBridge", + ) + ) + with patch.object( + client._breaker, + "before_call", + side_effect=CircuitBreakerOpenError("circuit open"), + ): + result = client.execute("review") + + self.assertFalse(result.success) + self.assertEqual(result.raw, {"failure_category": "transient_service_failure"}) + def test_build_primary_prompt_includes_evidence_summary(self) -> None: from pathlib import Path import json @@ -90,7 +126,7 @@ def test_budget_error_is_unavailable(self, review) -> None: self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) review.assert_called_once_with( "review", - task="promotion_review", + task="dual_review", mode="review_only", complexity="high", source_repository=None, @@ -122,6 +158,42 @@ def test_structured_capacity_failure_is_unavailable(self, review) -> None: self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) + @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) + @patch("service.dual_review_primary.AiGatewayClient.execute") + def test_structured_network_failure_is_unavailable(self, review) -> None: + review.return_value = AiResult( + provider="codex", + model="codex-cli", + success=False, + error="temporary DNS failure", + raw={"failure_category": "transient_service_failure"}, + ) + + result = run_codex_primary_review(prompt="review") + + self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) + + @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) + @patch("service.dual_review_primary.AiGatewayClient.execute") + def test_structured_service_outages_are_unavailable(self, review) -> None: + for category in ( + "auth_or_config_failure", + "service_restart", + "stale_job_timeout", + ): + with self.subTest(category=category): + review.return_value = AiResult( + provider="codex", + model="codex-cli", + success=False, + error=category, + raw={"failure_category": category}, + ) + + result = run_codex_primary_review(prompt="review") + + self.assertEqual(result["verdict"], VERDICT_UNAVAILABLE) + @patch.dict("os.environ", {"CODEX_AUDIT_SERVICE_URL": "https://service.invalid"}) @patch("service.dual_review_primary.AiGatewayClient.execute") def test_protocol_error_is_invalid(self, review) -> None: