diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml new file mode 100644 index 0000000..7b16ac8 --- /dev/null +++ b/.github/workflows/codex-review.yml @@ -0,0 +1,408 @@ +name: Codex Review + +on: + workflow_call: + inputs: + trigger-label: + type: string + description: 'Pull request label that triggers the review.' + required: false + default: 'codex-review' + allowed-owners: + type: string + description: 'Comma-separated GitHub repository owners allowed to run this workflow.' + required: false + default: 'matomo-org,innocraft' + automation-paths: + type: string + description: 'Newline- or comma-separated caller-repository paths that must receive human review before Codex runs. Entries ending in / match by prefix.' + required: false + default: | + .github/workflows/codex-review.yml + .github/codex/ + matomo-agent-skills-ref: + type: string + description: 'Ref of matomo-org/matomo-agent-skills to install before running Codex.' + required: false + default: 'main' + matomo-core-repository: + type: string + description: 'Matomo core repository to check out for read-only review context.' + required: false + default: 'matomo-org/matomo' + matomo-core-ref: + type: string + description: 'Matomo core ref to check out for read-only review context.' + required: false + default: '5.x-dev' + plugin-name: + type: string + description: 'Optional plugin name. If empty, the workflow reads name from plugin.json when present.' + required: false + default: '' + codex-model: + type: string + description: 'OpenAI model passed to openai/codex-action. Override only to move off the default.' + required: false + default: 'gpt-5.6-sol' + codex-effort: + type: string + description: 'Reasoning effort passed to openai/codex-action (Codex model_reasoning_effort): minimal, low, medium, high, or xhigh.' + required: false + default: 'xhigh' + secrets: + OPENAI_API_KEY: + description: 'OpenAI API key used by openai/codex-action.' + required: true + +permissions: + contents: none + +# In a reusable (workflow_call) workflow, ${{ github.workflow }} resolves to the CALLER's +# workflow name. If the caller is named "Codex Review" and uses github.workflow in its own +# concurrency group, both groups collide and GitHub cancels with "a deadlock was detected for +# concurrency group ... between a top level workflow and 'codex-review'". Use a static prefix +# unique to this reusable workflow so its group can never match the caller's group. +concurrency: + group: codex-review-reusable-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: true + +jobs: + preflight: + if: ${{ github.event.action == 'labeled' && github.event.label.name == inputs.trigger-label }} + runs-on: ubuntu-24.04 + permissions: + contents: read + pull-requests: read + outputs: + should_run: ${{ steps.inspect.outputs.should_run || steps.request.outputs.should_run }} + safety_failure: ${{ steps.inspect.outputs.safety_failure || steps.request.outputs.safety_failure }} + safety_message: ${{ steps.inspect.outputs.safety_message || steps.request.outputs.safety_message }} + skip_reason: ${{ steps.inspect.outputs.skip_reason || steps.request.outputs.skip_reason }} + skip_message: ${{ steps.inspect.outputs.skip_message || steps.request.outputs.skip_message }} + changed_files: ${{ steps.inspect.outputs.changed_files || steps.request.outputs.changed_files }} + automation_files: ${{ steps.inspect.outputs.automation_files || steps.request.outputs.automation_files }} + base_sha: ${{ steps.request.outputs.base_sha }} + head_sha: ${{ steps.request.outputs.head_sha }} + steps: + - name: Checkout trusted workflow assets + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + repository: ${{ job.workflow_repository }} + ref: ${{ job.workflow_sha }} + path: github-action-tests + persist-credentials: false + + - name: Validate review request + id: request + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 + env: + ALLOWED_OWNERS: ${{ inputs.allowed-owners }} + with: + script: | + const { validateReviewRequest } = require('./github-action-tests/review/preflight.js'); + await validateReviewRequest({ + github, + context, + core, + allowedOwners: process.env.ALLOWED_OWNERS, + }); + + - name: Checkout frozen PR snapshot for preflight + if: ${{ steps.request.outputs.should_inspect == 'true' }} + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + ref: ${{ steps.request.outputs.head_sha }} + path: pr-preflight + persist-credentials: false + fetch-depth: 0 + + - name: Inspect frozen changed files + if: ${{ steps.request.outputs.should_inspect == 'true' }} + id: inspect + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 + env: + AUTOMATION_PATHS: ${{ inputs.automation-paths }} + BASE_SHA: ${{ steps.request.outputs.base_sha }} + HEAD_SHA: ${{ steps.request.outputs.head_sha }} + with: + script: | + const path = require('node:path'); + const { inspectFrozenChanges } = require('./github-action-tests/review/preflight.js'); + inspectFrozenChanges({ + core, + checkoutPath: path.join(process.env.GITHUB_WORKSPACE, 'pr-preflight'), + automationPaths: process.env.AUTOMATION_PATHS, + baseSha: process.env.BASE_SHA, + headSha: process.env.HEAD_SHA, + }); + + codex: + needs: preflight + if: ${{ needs.preflight.outputs.should_run == 'true' }} + runs-on: ubuntu-24.04 + timeout-minutes: 30 + permissions: + contents: read + steps: + - name: Verify OpenAI API key is configured + env: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + run: | + if [ -z "${OPENAI_API_KEY}" ]; then + echo "::error::OPENAI_API_KEY secret is not configured for this repository. Set it (repo or org secret scoped to this repo) before running the Codex review workflow." >&2 + exit 1 + fi + + - name: Checkout trusted workflow assets + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + repository: ${{ job.workflow_repository }} + ref: ${{ job.workflow_sha }} + path: github-action-tests + persist-credentials: false + + - name: Checkout PR + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + ref: ${{ needs.preflight.outputs.head_sha }} + path: pr + persist-credentials: false + fetch-depth: 0 + + - name: Verify frozen PR snapshot + working-directory: pr + env: + BASE_SHA: ${{ needs.preflight.outputs.base_sha }} + HEAD_SHA: ${{ needs.preflight.outputs.head_sha }} + run: | + set -euo pipefail + test "$(git rev-parse HEAD)" = "$HEAD_SHA" + git cat-file -e "${BASE_SHA}^{commit}" + + - name: Checkout Matomo core context + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + repository: ${{ inputs.matomo-core-repository }} + ref: ${{ inputs.matomo-core-ref }} + path: matomo-core + persist-credentials: false + + - name: Map plugin into Matomo core context + id: plugin-context + env: + PLUGIN_NAME_INPUT: ${{ inputs.plugin-name }} + MATOMO_CORE_REF: ${{ inputs.matomo-core-ref }} + MATOMO_CORE_REPOSITORY: ${{ inputs.matomo-core-repository }} + run: | + set -euo pipefail + + plugin_name="$PLUGIN_NAME_INPUT" + matomo_core_sha="$(git -C matomo-core rev-parse HEAD)" + if [ -z "$plugin_name" ] && [ -f pr/plugin.json ]; then + # plugin.json comes from the untrusted PR; a malformed file must not abort the review + # (set -e would kill the step on a JSON.parse throw), so swallow parse errors and treat + # it as "no name", which then falls through to running without a core-layout mapping. + plugin_name="$(node -e "try { const fs = require('fs'); const plugin = JSON.parse(fs.readFileSync('pr/plugin.json', 'utf8')); process.stdout.write(typeof plugin.name === 'string' ? plugin.name : ''); } catch (e) { process.stdout.write(''); }")" + fi + + # plugin_name may come from the untrusted PR plugin.json and is interpolated into a symlink + # path and written to $GITHUB_OUTPUT, so restrict it to a safe plugin-name charset to prevent + # path traversal and step-output injection. Matomo plugin names are PHP-namespace/directory + # identifiers, so [A-Za-z0-9_] is the valid set. On mismatch, drop the mapping and continue. + if [ -n "$plugin_name" ] && ! [[ "$plugin_name" =~ ^[A-Za-z0-9_]+$ ]]; then + echo "::warning::Ignoring invalid plugin name; expected characters [A-Za-z0-9_]. Continuing without core-layout mapping." + plugin_name="" + fi + + plugin_in_core_path="" + if [ -n "$plugin_name" ]; then + mkdir -p matomo-core/plugins + plugin_in_core_path="$GITHUB_WORKSPACE/matomo-core/plugins/$plugin_name" + # The core checkout may already bundle a plugin of this name. Always map the PR + # checkout here so Codex inspects the PR's code, not the stale core copy; replace whatever + # exists (a real core plugin directory or a leftover link). plugin_name is validated to + # [A-Za-z0-9_]+ above, so this rm -rf cannot escape matomo-core/plugins. + rm -rf "$plugin_in_core_path" + ln -s "$GITHUB_WORKSPACE/pr" "$plugin_in_core_path" + fi + + context_file="$RUNNER_TEMP/codex-review-core-context.json" + export CONTEXT_FILE="$context_file" + export PLUGIN_NAME="$plugin_name" + export PLUGIN_IN_CORE_PATH="$plugin_in_core_path" + export MATOMO_CORE_SHA="$matomo_core_sha" + node -e ' + const fs = require("fs"); + fs.writeFileSync(process.env.CONTEXT_FILE, `${JSON.stringify({ + plugin_name: process.env.PLUGIN_NAME, + plugin_repository_path: `${process.env.GITHUB_WORKSPACE}/pr`, + matomo_core_repository: process.env.MATOMO_CORE_REPOSITORY, + matomo_core_ref: process.env.MATOMO_CORE_REF, + matomo_core_sha: process.env.MATOMO_CORE_SHA, + matomo_core_path: `${process.env.GITHUB_WORKSPACE}/matomo-core`, + plugin_in_core_path: process.env.PLUGIN_IN_CORE_PATH, + }, null, 2)}\n`); + ' + + echo "plugin_name=$plugin_name" >> "$GITHUB_OUTPUT" + echo "plugin_in_core_path=$plugin_in_core_path" >> "$GITHUB_OUTPUT" + echo "context_file=$context_file" >> "$GITHUB_OUTPUT" + + - name: Checkout Matomo skills + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + repository: matomo-org/matomo-agent-skills + ref: ${{ inputs.matomo-agent-skills-ref }} + path: matomo-agent-skills + persist-credentials: false + + - name: Install Matomo skills + env: + CODEX_HOME: ${{ runner.temp }}/codex-home + run: | + set -euo pipefail + mkdir -p "$CODEX_HOME/skills" + find matomo-agent-skills/skills -maxdepth 1 -mindepth 1 -type d -name 'matomo-*' -exec cp -R {} "$CODEX_HOME/skills/" \; + test -f "$CODEX_HOME/skills/matomo-review/SKILL.md" + + - name: Configure Codex + env: + CODEX_HOME: ${{ runner.temp }}/codex-home + run: | + set -euo pipefail + mkdir -p "$CODEX_HOME" + cat > "$CODEX_HOME/config.toml" <<'EOF' + project_doc_max_bytes = 0 + web_search = "disabled" + + [shell_environment_policy] + inherit = "core" + ignore_default_excludes = false + exclude = ["*KEY*", "*SECRET*", "*TOKEN*", "GITHUB_*", "ACTIONS_*", "OPENAI_*", "CODEX_*"] + EOF + + - name: Render Codex prompt + env: + PROMPT_TEMPLATE: ${{ github.workspace }}/github-action-tests/review/review-prompt.md + PROMPT_OUTPUT: ${{ runner.temp }}/codex-review-prompt.md + REVIEW_CONTEXT: ${{ runner.temp }}/codex-review-context.json + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + BASE_SHA: ${{ needs.preflight.outputs.base_sha }} + HEAD_REF: ${{ github.event.pull_request.head.ref }} + HEAD_SHA: ${{ needs.preflight.outputs.head_sha }} + CHANGED_FILES: ${{ needs.preflight.outputs.changed_files }} + MATOMO_CORE_CONTEXT: ${{ steps.plugin-context.outputs.context_file }} + MATOMO_CORE_PATH: ${{ github.workspace }}/matomo-core + PLUGIN_NAME: ${{ steps.plugin-context.outputs.plugin_name }} + PLUGIN_IN_CORE_PATH: ${{ steps.plugin-context.outputs.plugin_in_core_path }} + run: | + set -euo pipefail + node github-action-tests/review/render-review-prompt.js + + - name: Run Codex review + id: run_codex + uses: openai/codex-action@10cb888d2ed3b99867f7e7ccff174a861a75aeb6 + with: + openai-api-key: ${{ secrets.OPENAI_API_KEY }} + model: ${{ inputs.codex-model }} + effort: ${{ inputs.codex-effort }} + prompt-file: ${{ runner.temp }}/codex-review-prompt.md + output-file: ${{ runner.temp }}/codex-review-output.json + output-schema-file: ${{ github.workspace }}/github-action-tests/review/review-output.schema.json + codex-home: ${{ runner.temp }}/codex-home + working-directory: ${{ github.workspace }}/pr + sandbox: read-only + safety-strategy: drop-sudo + + - name: Upload Codex diagnostics + if: ${{ always() }} + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a + with: + name: codex-review-output + path: | + ${{ runner.temp }}/codex-review-output.json + ${{ runner.temp }}/codex-review-context.json + ${{ runner.temp }}/codex-review-core-context.json + if-no-files-found: ignore + retention-days: 7 + + post-review: + needs: [preflight, codex] + if: ${{ always() && github.event.action == 'labeled' && github.event.label.name == inputs.trigger-label }} + runs-on: ubuntu-24.04 + permissions: + actions: read + contents: read + issues: write + pull-requests: write + steps: + - name: Checkout trusted workflow assets + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + repository: ${{ job.workflow_repository }} + ref: ${{ job.workflow_sha }} + path: github-action-tests + persist-credentials: false + + - name: Download Codex diagnostics + if: ${{ needs.codex.result == 'success' }} + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c + with: + name: codex-review-output + path: ${{ runner.temp }}/codex-review + + - name: Post review result + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 + env: + CODEX_OUTPUT_DIR: ${{ runner.temp }}/codex-review + CODEX_RESULT: ${{ needs.codex.result }} + PREFLIGHT_RESULT: ${{ needs.preflight.result }} + PREFLIGHT_SAFETY_FAILURE: ${{ needs.preflight.outputs.safety_failure }} + PREFLIGHT_SAFETY_MESSAGE: ${{ needs.preflight.outputs.safety_message }} + PREFLIGHT_SKIP_REASON: ${{ needs.preflight.outputs.skip_reason }} + PREFLIGHT_SKIP_MESSAGE: ${{ needs.preflight.outputs.skip_message }} + REVIEWED_BASE_SHA: ${{ needs.preflight.outputs.base_sha }} + REVIEWED_HEAD_SHA: ${{ needs.preflight.outputs.head_sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + with: + script: | + const postReview = require('./github-action-tests/review/post-review.js'); + await postReview({ github, context, core }); + + cleanup: + needs: [preflight, codex, post-review] + if: ${{ always() && github.event.action == 'labeled' && github.event.label.name == inputs.trigger-label }} + runs-on: ubuntu-24.04 + permissions: + issues: write + pull-requests: write + steps: + - name: Remove trigger label + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 + env: + TRIGGER_LABEL: ${{ inputs.trigger-label }} + with: + script: | + const pr = context.payload.pull_request; + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pr.number, + name: process.env.TRIGGER_LABEL, + }); + } catch (error) { + if (error.status === 404) { + core.info(`${process.env.TRIGGER_LABEL} label was already removed.`); + return; + } + if (error.status === 403) { + core.warning(`Could not remove ${process.env.TRIGGER_LABEL} label because this workflow token lacks permission.`); + return; + } + throw error; + } diff --git a/.github/workflows/test-review-scripts.yml b/.github/workflows/test-review-scripts.yml new file mode 100644 index 0000000..1792c96 --- /dev/null +++ b/.github/workflows/test-review-scripts.yml @@ -0,0 +1,38 @@ +name: Test review scripts + +on: + push: + branches: [main] + paths: + - 'review/**' + - 'package.json' + - '.github/workflows/test-review-scripts.yml' + # Cross-file tests keep the workflow's trust flow and documented defaults aligned with the + # trusted JavaScript modules, so reusable-workflow changes must run this suite too. + - '.github/workflows/codex-review.yml' + pull_request: + paths: + - 'review/**' + - 'package.json' + - '.github/workflows/test-review-scripts.yml' + - '.github/workflows/codex-review.yml' + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-24.04 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 + with: + persist-credentials: false + + - name: Set up Node.js + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e + with: + node-version: '22' + + - name: Run review script tests + run: npm test diff --git a/README.md b/README.md index c4c668b..c000e3d 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,11 @@ This action is able to run certain test suites for Matomo or any Matomo plugin. +The repository also contains a reusable Codex review workflow documented at +[`review`](review/README.md). It is separate from the main tests action and is +triggered from consuming repositories by adding the `codex-review` pull request +label. + ### Inputs * **test-type** diff --git a/package.json b/package.json new file mode 100644 index 0000000..c4fb2e6 --- /dev/null +++ b/package.json @@ -0,0 +1,10 @@ +{ + "name": "matomo-codex-review", + "version": "0.0.0", + "private": true, + "description": "Trusted scripts for the reusable Matomo Codex PR review workflow.", + "license": "GPL-3.0-or-later", + "scripts": { + "test": "node --test review/*.test.js" + } +} diff --git a/review/README.md b/review/README.md new file mode 100644 index 0000000..b649a6d --- /dev/null +++ b/review/README.md @@ -0,0 +1,233 @@ +# Matomo Codex Review Workflow + +This repository provides a reusable Codex PR review workflow for Matomo and +InnoCraft-maintained plugin repositories. + +Reviews are triggered by adding the `codex-review` label to a pull request. The +called workflow checks the PR diff, runs Codex with the Matomo review skills, +posts a structured GitHub pull request review, uploads diagnostics, and removes +the trigger label after the run. + +The workflow implementation lives in +`.github/workflows/codex-review.yml`. The trusted scripts, prompt, and schema +used by that workflow live in this `review/` directory. + +## Usage + +Add this wrapper workflow to each consuming repository: + +```yaml +name: Codex Review + +on: + pull_request_target: + types: [labeled] + +permissions: + contents: none + +jobs: + codex-review: + if: ${{ github.event.label.name == 'codex-review' }} + uses: matomo-org/github-action-tests/.github/workflows/codex-review.yml@main + permissions: + actions: read + contents: read + issues: write + pull-requests: write + secrets: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} +``` + +Pin `matomo-org/github-action-tests/.github/workflows/codex-review.yml` to a tag +or commit SHA when using this outside early rollout. + +## Required Repository Setup + +- Configure `OPENAI_API_KEY` as a repository secret or as an organization secret + scoped to selected repositories. +- Ensure only trusted users can apply the `codex-review` label. +- Create the `codex-review` label in each consuming repository. +- Keep the `pull_request_target` wrapper limited to the reusable-workflow call + shown above. Do not add steps that check out or execute pull request code. +- Run the workflow on GitHub.com. It relies on the reusable-workflow identity + fields `job.workflow_repository` and `job.workflow_sha`, which are not + available on GitHub Enterprise Server. +- Confirm the repository or organization allows the required `GITHUB_TOKEN` + permissions: `actions: read`, `contents: read`, `issues: write`, and + `pull-requests: write`. +- Confirm the repository or organization allows the third-party actions used by + the workflow, including `openai/codex-action`, `actions/checkout`, + `actions/github-script`, `actions/upload-artifact`, and + `actions/download-artifact`. + +The OpenAI key is always supplied by the consuming repository or organization. +This repository does not provide a central key to callers. + +## Core Context + +The workflow checks out a read-only Matomo core tree for review context. +This gives the existing `matomo-review` and plugin architecture skills access to +core files and sibling plugins that standalone plugin repositories do not have. + +By default the workflow checks out `matomo-org/matomo` at `5.x-dev`, matching +the current Matomo development baseline used by plugin work. The resolved core +commit SHA is written to the uploaded `codex-review-core-context.json` +diagnostics artifact for auditability. Override `matomo-core-ref` when a review +needs a different core branch, tag, or commit. + +When a plugin name can be read from `plugin.json`, the workflow also maps the PR +checkout into the core tree at `matomo-core/plugins/` for read-only +inspection. The PR checkout remains the review target, and GitHub inline +comments must use the actual changed paths from the PR diff. + +## Security Model + +This workflow runs an autonomous AI agent (Codex) over pull request content. The +design treats **everything in the PR as untrusted** — the diff, commit messages, +the PR title and body, `plugin.json`, and any `AGENTS.md`/`.codex`-style agent +instruction files. It assumes an attacker may open a PR (or push a branch) for +the sole purpose of making the reviewer leak a secret or take an unwanted action. + +The design does not rely on a single control: the two secrets in play +(`OPENAI_API_KEY` and the `GITHUB_TOKEN`) are separated from the agent by +several independent layers, and the agent runs sandboxed and read-only. The +controls below are grouped by the risk they address. + +### Who can trigger a review + +- The caller wrapper runs **only** on trusted `pull_request_target` `labeled` + events where the label is `codex-review`, so an ordinary push never starts a + review. The wrapper itself never checks out or executes pull request code. +- Applying that label is the trust decision. Restrict who can label PRs in each + consuming repository (see *Required Repository Setup*). +- The called workflow refuses to use the OpenAI key unless the caller repository + owner is in `allowed-owners` (default `matomo-org,innocraft`). This prevents + accidental use by repositories outside the intended organizations. +- **Fork PRs are skipped before any PR checkout or Codex step.** The trusted + `pull_request_target` context lets the posting and cleanup jobs explain the + skip and remove the trigger label, while preflight compares immutable numeric + base/head repository IDs before fork code reaches the runner. Missing or + mismatched IDs fail closed. This means untrusted contributor code only reaches + Codex after a maintainer with label rights has pulled it into a branch of the + repository itself. + +### The agent runs trusted code against an untrusted target + +- The preflight module, review scripts, prompt, and JSON schema are checked out + from **this** shared workflow repository at `job.workflow_sha` (the pinned + trusted commit), never from the caller/PR. A PR that edits `review/*` cannot + change what actually executes, and the security-sensitive preflight logic is + covered by the same Node test suite as review posting. +- External GitHub Actions used by the trusted workflow are pinned by full commit + SHA so tag retargeting cannot silently change what privileged jobs execute. +- The event's exact PR head SHA is checked out into a separate `pr/` directory + that is only ever the *target* of read-only inspection. It is not a source of + executable workflow logic. +- Preflight confirms the live PR repository identity, base, and head still match + the labeled event, derives changed paths from that frozen base/head diff, and + passes the same SHAs to checkout, the prompt, and review posting. Posting + checks both SHAs again and supplies GitHub's head `commit_id`, so a later head + push or base-branch advance cannot be presented as reviewed by an earlier run. +- Every posted review carries a trusted hidden base-SHA marker. A prior review + suppresses a duplicate run only when both its head commit and recorded base + commit match the newly labeled snapshot; legacy or malformed markers default + to a fresh review. +- As defense in depth, a PR that touches reviewer automation paths + (`.github/workflows/codex-review.yml`, `.github/codex/`, configurable via + `automation-paths`) is skipped and flagged for human review first. Preflight + uses `git diff --no-renames`, so renaming a guarded file out of a guarded path + still reports and blocks the deleted source path. +- Changed-path JSON is bounded before it becomes a job output. An exceptionally + large path list fails closed instead of overflowing GitHub's output channel or + being truncated into an incomplete review scope. +- The plugin name read from the untrusted `plugin.json` is validated against + `^[A-Za-z0-9_]+$` before it is used in a filesystem path or written to a step + output, preventing path traversal and step-output injection. + +### Secrets never reach the agent + +- The workflow token defaults to `permissions: contents: none`, and each job + requests only what it needs. The Codex job holds **`contents: read` only** — it + cannot write code, comments, or labels. +- Every checkout, including the PR, uses `persist-credentials: false`, so no + `GITHUB_TOKEN` is left in `pr/.git/config` for the agent to harvest. +- The frozen PR checkout uses `fetch-depth: 0`, so the base commit needed for + the explicit SHA diff is present without a later authenticated fetch. After + checkout, the working tree Codex reads contains no credential material. +- Codex's shell runs under an environment policy that strips secret-bearing + variables (`*KEY*`, `*SECRET*`, `*TOKEN*`, `GITHUB_*`, `ACTIONS_*`, `OPENAI_*`, + `CODEX_*`). Even a prompt-injected command cannot echo the OpenAI key or the + GitHub token out of the environment. +- `OPENAI_API_KEY` is consumed only by the `openai/codex-action` step (pinned by + commit SHA) and is always supplied by the consuming repo/org — this repository + ships no central key. + +### The agent is sandboxed + +- Codex runs with `sandbox: read-only` (it cannot modify the checkout or the + runner), `safety-strategy: drop-sudo` (no privilege escalation), `web_search` + disabled (no exfiltration channel or untrusted fetches), and + `project_doc_max_bytes = 0` (PR-provided project docs are not auto-loaded as + instructions). + +### Prompt-injection resistance + +- The prompt establishes an explicit trust policy: the workflow prompt and the + skills installed from the trusted `matomo-org/matomo-agent-skills` repository + are authoritative, and PR-provided `AGENTS.md`/`.codex`/`.agents/skills` files + are to be treated as reviewed content only — never as instructions, and never + executed. +- PR title and body are injected into the prompt with a single-pass template + render, so untrusted values cannot re-trigger substitution to smuggle in new + placeholders. + +### Review and posting are separated + +- Codex (read-only, untrusted-input-facing) only emits a structured JSON file + validated against `review-output.schema.json`. +- A **separate** `post-review` job — which never runs Codex — holds the + `issues: write` / `pull-requests: write` permissions and turns that validated + output into the GitHub review. The component that writes to the PR is not the + component exposed to untrusted input. +- Posting revalidates every required property and rejects unknown properties, + non-regular files, files outside the downloaded artifact directory, and + output larger than 1 MiB before parsing or making a review mutation. +- Model-authored GitHub user/team mentions are neutralized before public + posting. Untrusted filenames and rule names are rendered as bounded code spans + with control and direction-changing characters made visible. +- Public review bodies are capped at 60,000 characters. Findings that do not fit + remain available in the seven-day `codex-review-output` diagnostics artifact. + +## Inputs + +| Input | Required | Default | Description | +| --- | --- | --- | --- | +| `trigger-label` | no | `codex-review` | Pull request label that triggers the review. | +| `allowed-owners` | no | `matomo-org,innocraft` | Comma-separated repository owner allowlist. | +| `automation-paths` | no | `.github/workflows/codex-review.yml`, `.github/codex/` | Caller-repository paths that require human review before Codex runs. Entries ending in `/` match by prefix. | +| `matomo-agent-skills-ref` | no | `main` | Ref of `matomo-org/matomo-agent-skills` to install. | +| `matomo-core-repository` | no | `matomo-org/matomo` | Matomo core repository used for read-only review context. | +| `matomo-core-ref` | no | `5.x-dev` | Matomo core ref used for read-only review context. | +| `plugin-name` | no | read from `plugin.json` | Plugin name used for the optional core-layout mapping. | +| `codex-model` | no | `gpt-5.6-sol` | OpenAI model passed to `openai/codex-action`. Override only to move off the default. | +| `codex-effort` | no | `xhigh` | Reasoning effort passed to `openai/codex-action` (`minimal`, `low`, `medium`, `high`, or `xhigh`). | + +## Secrets + +| Secret | Required | Description | +| --- | --- | --- | +| `OPENAI_API_KEY` | yes | OpenAI API key passed from the consuming repository or organization secret. | + +## Local Validation + +Run the dependency-free Node test suite after changing the workflow, prompt, +schema, or trusted review scripts: + +```bash +npm test +``` + +CI runs the same suite on Node.js 22. The tests include cross-file workflow and +documentation invariants in addition to preflight, rendering, schema, and +posting edge cases. diff --git a/review/env-utils.js b/review/env-utils.js new file mode 100644 index 0000000..6697669 --- /dev/null +++ b/review/env-utils.js @@ -0,0 +1,21 @@ +'use strict'; + +// Shared environment-variable readers for the review scripts. +// +// requiredEnv throws when a variable is unset. By default it also rejects an empty string, which is +// what render-review-prompt.js wants (an empty PR_NUMBER etc. is a bug). post-review.js runs with +// `if: always()`, so passthrough outputs such as PREFLIGHT_SAFETY_FAILURE can legitimately be an +// empty string when an upstream job did not complete; those callers pass { allowEmpty: true }. +function requiredEnv(name, { allowEmpty = false } = {}) { + const value = process.env[name]; + if (value === undefined || (!allowEmpty && value === '')) { + throw new Error(`Missing required environment variable: ${name}`); + } + return value; +} + +function optionalEnv(name) { + return process.env[name] || ''; +} + +module.exports = { requiredEnv, optionalEnv }; diff --git a/review/env-utils.test.js b/review/env-utils.test.js new file mode 100644 index 0000000..2e1f056 --- /dev/null +++ b/review/env-utils.test.js @@ -0,0 +1,54 @@ +'use strict'; + +// Unit tests for env-utils.js. Run with: node --test review/env-utils.test.js + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { requiredEnv, optionalEnv } = require('./env-utils'); + +function withEnv(t, name, value) { + const saved = process.env[name]; + if (value === undefined) { + delete process.env[name]; + } else { + process.env[name] = value; + } + t.after(() => { + if (saved === undefined) { + delete process.env[name]; + } else { + process.env[name] = saved; + } + }); +} + +test('requiredEnv: returns the value when set', (t) => { + withEnv(t, 'ENV_UTILS_TEST', 'hello'); + assert.equal(requiredEnv('ENV_UTILS_TEST'), 'hello'); +}); + +test('requiredEnv: throws when the variable is unset', (t) => { + withEnv(t, 'ENV_UTILS_TEST', undefined); + assert.throws(() => requiredEnv('ENV_UTILS_TEST'), /Missing required environment variable: ENV_UTILS_TEST/); +}); + +test('requiredEnv: rejects an empty string by default', (t) => { + withEnv(t, 'ENV_UTILS_TEST', ''); + assert.throws(() => requiredEnv('ENV_UTILS_TEST')); +}); + +test('requiredEnv: allowEmpty accepts an empty string but still rejects unset', (t) => { + withEnv(t, 'ENV_UTILS_TEST', ''); + assert.equal(requiredEnv('ENV_UTILS_TEST', { allowEmpty: true }), ''); + + withEnv(t, 'ENV_UTILS_UNSET', undefined); + assert.throws(() => requiredEnv('ENV_UTILS_UNSET', { allowEmpty: true })); +}); + +test('optionalEnv: returns the value or an empty string', (t) => { + withEnv(t, 'ENV_UTILS_TEST', 'x'); + assert.equal(optionalEnv('ENV_UTILS_TEST'), 'x'); + withEnv(t, 'ENV_UTILS_TEST', undefined); + assert.equal(optionalEnv('ENV_UTILS_TEST'), ''); +}); diff --git a/review/markdown-utils.js b/review/markdown-utils.js new file mode 100644 index 0000000..9432bb8 --- /dev/null +++ b/review/markdown-utils.js @@ -0,0 +1,42 @@ +'use strict'; + +const INVISIBLE_OR_DIRECTIONAL_CHARACTER = /[\p{Cc}\p{Cf}\p{Cs}\p{Zl}\p{Zp}]/u; + +function escapeInvisibleCharacters(value) { + return [...String(value)].map((character) => { + if (!INVISIBLE_OR_DIRECTIONAL_CHARACTER.test(character)) { + return character; + } + + const codePoint = character.codePointAt(0); + return ``; + }).join(''); +} + +// Filenames and rule names may contain Markdown delimiters or invisible control characters. A code +// span with a dynamically sized fence keeps those values inert and visually unambiguous. +function formatUntrustedInlineCode(value) { + const escaped = escapeInvisibleCharacters(value); + const longestBacktickRun = Math.max( + 0, + ...(escaped.match(/`+/g) || []).map((run) => run.length), + ); + const fence = '`'.repeat(longestBacktickRun + 1); + return `${fence} ${escaped} ${fence}`; +} + +// GitHub turns model-authored @names and @org/team strings into notifications. Insert a zero-width +// separator after every mention-shaped @ token. This intentionally includes email-shaped text: +// notification prevention is more important than preserving automatic mail links. +function neutralizeGitHubMentions(value) { + return String(value).replace( + /@(?=[A-Za-z0-9][A-Za-z0-9-]*(?:\/[A-Za-z0-9][A-Za-z0-9-]*)?)/gu, + '@\u200b', + ); +} + +module.exports = { + escapeInvisibleCharacters, + formatUntrustedInlineCode, + neutralizeGitHubMentions, +}; diff --git a/review/markdown-utils.test.js b/review/markdown-utils.test.js new file mode 100644 index 0000000..1d66a03 --- /dev/null +++ b/review/markdown-utils.test.js @@ -0,0 +1,37 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { + escapeInvisibleCharacters, + formatUntrustedInlineCode, + neutralizeGitHubMentions, +} = require('./markdown-utils'); + +test('neutralizeGitHubMentions: disables user, team, and email-shaped notification tokens', () => { + const result = neutralizeGitHubMentions( + '@octocat, please ask @matomo-org/security. Email reviewer@example.com or a+b@example.com.', + ); + + assert.equal( + result, + '@\u200boctocat, please ask @\u200bmatomo-org/security. Email reviewer@\u200bexample.com or a+b@\u200bexample.com.', + ); + assert.doesNotMatch(result, /(^|\s)@(?:octocat|matomo-org\/security)\b/); + assert.doesNotMatch(result, /@example\.com/); +}); + +test('escapeInvisibleCharacters: makes control and direction-changing characters visible', () => { + assert.equal( + escapeInvisibleCharacters('line\nname\u202efile.js\0'), + 'linenamefile.js', + ); +}); + +test('formatUntrustedInlineCode: contains Markdown delimiters with a longer code fence', () => { + const result = formatUntrustedInlineCode('src/`odd``name`\nfile.js'); + + assert.match(result, /^``` .* ```$/); + assert.match(result, /src\/`odd``name`file\.js/); +}); diff --git a/review/post-review.js b/review/post-review.js new file mode 100644 index 0000000..19add0e --- /dev/null +++ b/review/post-review.js @@ -0,0 +1,830 @@ +const fs = require('fs'); +const path = require('path'); +const { requiredEnv } = require('./env-utils'); +const { + COMMIT_SHA_PATTERN, + buildCodexReviewHeader, + CODEX_REVIEW_MARKER, + CODEX_INLINE_MARKER, + CODEX_REVIEW_OUTPUT_FILE, +} = require('./review-constants'); +const { + formatUntrustedInlineCode, + neutralizeGitHubMentions, +} = require('./markdown-utils'); + +// Severity ranked low->high; index doubles as the ordering used to reconcile highest_severity. +const SEVERITIES = ['none', 'low', 'medium', 'blocking']; +// The subset a finding/comment may carry ('none' is a review-level state, not a per-finding value). +const FINDING_SEVERITIES = SEVERITIES.filter((severity) => severity !== 'none'); + +const REVIEW_LIMITS = Object.freeze({ + reviewBodyMarkdownMaxLength: 2000, + diagnosticsMarkdownMaxLength: 60000, + reviewOutputFileMaxBytes: 1024 * 1024, + // GitHub's documented review-body limit is higher, but keeping headroom avoids rejecting the + // entire review if trusted framing changes or the API's accounting differs from JS string length. + publicReviewBodyMaxLength: 60000, + inlineCommentsMaxItems: 20, + unplacedFindingsMaxItems: 20, + pathMaxLength: 1024, + findingBodyMaxLength: 1200, + ruleSourceMaxLength: 128, +}); + +function expectedHighestSeverity(findings) { + if (findings.blocking > 0) { + return 'blocking'; + } + if (findings.medium > 0) { + return 'medium'; + } + if (findings.low_polish > 0) { + return 'low'; + } + return 'none'; +} + +function assertString(value, name, { maxLength } = {}) { + if (typeof value !== 'string' || value.trim() === '') { + throw new Error(`${name} must be a non-empty string`); + } + if (maxLength !== undefined && value.length > maxLength) { + throw new Error(`${name} must be at most ${maxLength} characters`); + } +} + +function assertInteger(value, name, { max } = {}) { + if (!Number.isInteger(value) || value < 0) { + throw new Error(`${name} must be a non-negative integer`); + } + if (max !== undefined && value > max) { + throw new Error(`${name} must be at most ${max}`); + } +} + +function assertArray(value, name, { maxItems } = {}) { + if (!Array.isArray(value)) { + throw new Error(`${name} must be an array`); + } + if (maxItems !== undefined && value.length > maxItems) { + throw new Error(`${name} must contain at most ${maxItems} items`); + } +} + +function assertExactObjectKeys(value, name, expectedKeys) { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + throw new Error(`${name} must be an object`); + } + + const actualKeys = Object.keys(value).sort(); + const sortedExpectedKeys = [...expectedKeys].sort(); + if ( + actualKeys.length !== sortedExpectedKeys.length + || actualKeys.some((key, index) => key !== sortedExpectedKeys[index]) + ) { + throw new Error(`${name} must contain exactly these properties: ${sortedExpectedKeys.join(', ')}`); + } +} + +function countFindingsBySeverity(inlineComments, unplacedFindings) { + const findings = { blocking: 0, medium: 0, low_polish: 0 }; + for (const finding of [...inlineComments, ...unplacedFindings]) { + if (finding.severity === 'blocking') { + findings.blocking += 1; + } else if (finding.severity === 'medium') { + findings.medium += 1; + } else if (finding.severity === 'low') { + findings.low_polish += 1; + } + } + return findings; +} + +// Defence-in-depth re-validation of the Codex output. The codex-action already constrains the model +// to review-output.schema.json, so this mirrors that schema as a backstop in case enforcement is +// absent or changes. Keep this in sync with review/review-output.schema.json. +function validateReview(review) { + assertExactObjectKeys(review, 'Codex output', [ + 'review_body_markdown', + 'diagnostics_markdown', + 'highest_severity', + 'findings', + 'inline_comments', + 'unplaced_findings', + ]); + + assertString(review.review_body_markdown, 'review_body_markdown', { + maxLength: REVIEW_LIMITS.reviewBodyMarkdownMaxLength, + }); + // diagnostics_markdown is intentionally not rendered into the review body; it is surfaced only via + // the uploaded codex-review-output artifact, so the PR conversation stays concise. + assertString(review.diagnostics_markdown, 'diagnostics_markdown', { + maxLength: REVIEW_LIMITS.diagnosticsMarkdownMaxLength, + }); + if (!SEVERITIES.includes(review.highest_severity)) { + throw new Error('highest_severity is invalid'); + } + + const findings = review.findings; + assertExactObjectKeys(findings, 'findings', ['blocking', 'medium', 'low_polish']); + const maxFindings = REVIEW_LIMITS.inlineCommentsMaxItems + REVIEW_LIMITS.unplacedFindingsMaxItems; + assertInteger(findings.blocking, 'findings.blocking', { max: maxFindings }); + assertInteger(findings.medium, 'findings.medium', { max: maxFindings }); + assertInteger(findings.low_polish, 'findings.low_polish', { max: maxFindings }); + + assertArray(review.inline_comments, 'inline_comments', { + maxItems: REVIEW_LIMITS.inlineCommentsMaxItems, + }); + assertArray(review.unplaced_findings, 'unplaced_findings', { + maxItems: REVIEW_LIMITS.unplacedFindingsMaxItems, + }); + + for (const [index, comment] of review.inline_comments.entries()) { + assertExactObjectKeys(comment, `inline_comments[${index}]`, [ + 'path', + 'line', + 'side', + 'severity', + 'body', + 'rule_source', + ]); + assertString(comment.path, `inline_comments[${index}].path`, { + maxLength: REVIEW_LIMITS.pathMaxLength, + }); + if (!Number.isInteger(comment.line) || comment.line < 1) { + throw new Error(`inline_comments[${index}].line must be a positive integer`); + } + if (!['LEFT', 'RIGHT'].includes(comment.side)) { + throw new Error(`inline_comments[${index}].side must be LEFT or RIGHT`); + } + if (!FINDING_SEVERITIES.includes(comment.severity)) { + throw new Error(`inline_comments[${index}].severity is invalid`); + } + assertString(comment.body, `inline_comments[${index}].body`, { + maxLength: REVIEW_LIMITS.findingBodyMaxLength, + }); + // rule_source is required by the schema but may be null; it is only read optionally downstream. + if (comment.rule_source !== null && typeof comment.rule_source !== 'string') { + throw new Error(`inline_comments[${index}].rule_source must be a string or null`); + } + if (typeof comment.rule_source === 'string' && comment.rule_source.length > REVIEW_LIMITS.ruleSourceMaxLength) { + throw new Error(`inline_comments[${index}].rule_source must be at most ${REVIEW_LIMITS.ruleSourceMaxLength} characters`); + } + } + + for (const [index, finding] of review.unplaced_findings.entries()) { + // Keep the nullable placement contract explicit: null means "not locatable" while a missing + // property means the structured response did not conform to the schema. + if (!Object.prototype.hasOwnProperty.call(finding || {}, 'path')) { + throw new Error(`unplaced_findings[${index}].path is required`); + } + if (!Object.prototype.hasOwnProperty.call(finding || {}, 'line')) { + throw new Error(`unplaced_findings[${index}].line is required`); + } + assertExactObjectKeys(finding, `unplaced_findings[${index}]`, [ + 'severity', + 'body', + 'path', + 'line', + ]); + if (!FINDING_SEVERITIES.includes(finding.severity)) { + throw new Error(`unplaced_findings[${index}].severity is invalid`); + } + assertString(finding.body, `unplaced_findings[${index}].body`, { + maxLength: REVIEW_LIMITS.findingBodyMaxLength, + }); + if (finding.path !== null && typeof finding.path !== 'string') { + throw new Error(`unplaced_findings[${index}].path must be a string or null`); + } + if (typeof finding.path === 'string' && finding.path.length > REVIEW_LIMITS.pathMaxLength) { + throw new Error(`unplaced_findings[${index}].path must be at most ${REVIEW_LIMITS.pathMaxLength} characters`); + } + if (finding.line !== null && (!Number.isInteger(finding.line) || finding.line < 1)) { + throw new Error(`unplaced_findings[${index}].line must be a positive integer or null`); + } + } + + // Recompute public counts and highest_severity from bounded structured findings rather than + // trusting model-provided summary fields. + review.findings = countFindingsBySeverity(review.inline_comments, review.unplaced_findings); + review.highest_severity = expectedHighestSeverity(review.findings); +} + +function resolveReviewOutputPath(outputDir) { + assertString(outputDir, 'CODEX_OUTPUT_DIR'); + + const resolvedDir = path.resolve(outputDir); + const resolvedOutputPath = path.resolve(resolvedDir, CODEX_REVIEW_OUTPUT_FILE); + const realDir = fs.realpathSync(resolvedDir); + const realOutputPath = fs.realpathSync(resolvedOutputPath); + const relativeOutputPath = path.relative(realDir, realOutputPath); + + if (relativeOutputPath !== CODEX_REVIEW_OUTPUT_FILE) { + throw new Error('Codex output file must resolve inside CODEX_OUTPUT_DIR'); + } + + return realOutputPath; +} + +function readReviewOutput(outputDir) { + const reviewOutputPath = resolveReviewOutputPath(outputDir); + const outputStat = fs.statSync(reviewOutputPath); + if (!outputStat.isFile()) { + throw new Error('Codex output path must be a regular file'); + } + if (outputStat.size > REVIEW_LIMITS.reviewOutputFileMaxBytes) { + throw new Error(`Codex output file must be at most ${REVIEW_LIMITS.reviewOutputFileMaxBytes} bytes`); + } + const raw = fs.readFileSync(reviewOutputPath, 'utf8').trim(); + if (!raw) { + throw new Error('Codex output file is empty'); + } + return JSON.parse(raw); +} + +function parsePatchLines(patch) { + const right = new Set(); + const left = new Set(); + + if (!patch) { + return { right, left }; + } + + let oldLine = 0; + let newLine = 0; + for (const line of patch.split('\n')) { + const hunk = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + oldLine = Number(hunk[1]); + newLine = Number(hunk[2]); + continue; + } + + // The pulls.listFiles `patch` starts at the first @@ header and never contains `---`/`+++` + // file-header lines, so we must NOT skip those prefixes here: a real added line whose source + // begins with `++` renders as `+++...` and a removed line beginning with `--` renders as + // `---...`, and skipping them would desynchronise every following line number in the hunk. Only + // the "\ No newline at end of file" marker needs to be ignored. + if (line.startsWith('\\')) { + continue; + } + + if (line.startsWith('+')) { + right.add(newLine); + newLine += 1; + continue; + } + + if (line.startsWith('-')) { + left.add(oldLine); + oldLine += 1; + continue; + } + + if (line.startsWith(' ')) { + right.add(newLine); + left.add(oldLine); + oldLine += 1; + newLine += 1; + } + } + + return { right, left }; +} + +function formatFinding(finding) { + const location = finding.path + ? ` ${neutralizeGitHubMentions(formatUntrustedInlineCode(`${finding.path}${finding.line ? `:${finding.line}` : ''}`))}` + : ''; + return `- **${formatSeverityBadge(finding.severity)}**${location}: ${neutralizeGitHubMentions(finding.body)}`; +} + +function pluralize(count, singular, plural = `${singular}s`) { + return count === 1 ? singular : plural; +} + +function formatSeverityBadge(severity) { + switch (severity) { + case 'blocking': + return '🚫 Blocking'; + case 'medium': + return '⚠️ Medium'; + case 'low': + return '💬 Low / Polish'; + case 'none': + return '✅ No findings'; + default: + return severity; + } +} + +function formatInlineCommentBody(comment) { + const lines = [ + `**${formatSeverityBadge(comment.severity)}**`, + '', + neutralizeGitHubMentions(comment.body), + ]; + + if (comment.rule_source) { + lines.push('', `Rule source: ${neutralizeGitHubMentions(formatUntrustedInlineCode(comment.rule_source))}`); + } + + // Trailing hidden marker so a later run can identify and delete this comment (see + // deletePreviousCodexInlineComments). + lines.push('', CODEX_INLINE_MARKER); + + return lines.join('\n'); +} + +// Attach `candidate` (an inline comment or a locatable unplaced finding) as an inline review comment +// on the given diff `side` when its line is part of the diff. On success it records both the GitHub +// comment payload and a plain-finding mirror (used to rebuild the body in the 422 fallback). Both +// placement loops in postReview share this so the diff-mapping rules live in one place. +function placeInlineComment({ candidate, side, patchesByPath, comments, placedFindings }) { + const patch = candidate.path ? patchesByPath.get(candidate.path) : undefined; + const lineSet = side === 'RIGHT' ? patch?.right : patch?.left; + if (!patch || !Number.isInteger(candidate.line) || !lineSet.has(candidate.line)) { + return { placed: false, patchMissing: !patch }; + } + + comments.push({ + path: candidate.path, + line: candidate.line, + side, + body: formatInlineCommentBody(candidate), + }); + placedFindings.push({ + severity: candidate.severity, + body: candidate.body, + path: candidate.path, + line: candidate.line, + }); + return { placed: true, patchMissing: false }; +} + +function buildReviewBody(review, unplaced, inlineCount, reviewedBaseSha) { + const hasFindings = review.findings.blocking + review.findings.medium + review.findings.low_polish > 0; + const lines = [ + ...buildCodexReviewHeader(reviewedBaseSha).split('\n'), + `## 🤖 Codex Review: ${formatSeverityBadge(review.highest_severity)}`, + '', + '### Summary', + neutralizeGitHubMentions(review.review_body_markdown.trim()), + '', + '### Findings Overview', + '', + '| Severity | Count |', + '| --- | ---: |', + `| 🚫 Blocking | ${review.findings.blocking} |`, + `| ⚠️ Medium | ${review.findings.medium} |`, + `| 💬 Low / Polish | ${review.findings.low_polish} |`, + ]; + + if (inlineCount > 0) { + lines.push('', `📍 Posted ${inlineCount} inline ${pluralize(inlineCount, 'finding')}.`); + } else if (hasFindings) { + lines.push('', '📍 No findings could be placed inline.'); + } else { + lines.push('', '✅ No inline findings to place.'); + } + + const footer = [ + '', + '### Diagnostics', + 'Detailed review diagnostics are available in the `codex-review-output` workflow artifact.' + ]; + + if (unplaced.length > 0) { + const formattedFindings = unplaced.map(formatFinding); + let displayedCount = 0; + + for (let index = 0; index < formattedFindings.length; index += 1) { + const candidateCount = index + 1; + const omittedCount = formattedFindings.length - candidateCount; + const candidateSection = [ + '', + '
', + 'Unplaced findings', + '', + ...formattedFindings.slice(0, candidateCount), + ...(omittedCount > 0 ? [ + '', + `_${omittedCount} additional unplaced ${pluralize(omittedCount, 'finding')} omitted from this review body. See the workflow artifact for the complete output._`, + ] : []), + '', + '
', + ]; + const candidateBody = `${[...lines, ...candidateSection, ...footer].join('\n')}\n`; + + if (candidateBody.length > REVIEW_LIMITS.publicReviewBodyMaxLength) { + break; + } + displayedCount = candidateCount; + } + + const omittedCount = formattedFindings.length - displayedCount; + lines.push( + '', + '
', + 'Unplaced findings', + '', + ...formattedFindings.slice(0, displayedCount), + ...(omittedCount > 0 ? [ + '', + `_${omittedCount} additional unplaced ${pluralize(omittedCount, 'finding')} omitted from this review body. See the workflow artifact for the complete output._`, + ] : []), + '', + '
' + ); + } + + lines.push(...footer); + + return `${lines.join('\n')}\n`; +} + +function reviewEventForSeverity(severity) { + // Never emit APPROVE: the verdict is produced by an LLM reading the untrusted PR diff, so the + // workflow must not stamp a green approval it cannot guarantee. Non-blocking outcomes are posted + // as a plain COMMENT instead. + if (severity === 'medium' || severity === 'blocking') { + return 'REQUEST_CHANGES'; + } + return 'COMMENT'; +} + +function isDismissableCodexReview(review) { + // Only APPROVED and CHANGES_REQUESTED reviews can be dismissed; GitHub rejects dismissing a + // COMMENTED review with 422. A COMMENTED review does not block the PR, so there is nothing to + // dismiss anyway. + return review + && review.user + // The login of the actor behind github.token, which is what posts and therefore dismisses these + // reviews. If the workflow ever posts under a different identity (e.g. a GitHub App) this must + // be updated, otherwise dismissal silently stops matching. + && review.user.login === 'github-actions[bot]' + && ['APPROVED', 'CHANGES_REQUESTED'].includes(review.state) + && typeof review.body === 'string' + && review.body.includes(CODEX_REVIEW_MARKER); +} + +async function createIssueComment({ github, context, body, core }) { + try { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.payload.pull_request.number, + body: neutralizeGitHubMentions(body), + }); + } catch (error) { + if (error.status === 403) { + core.warning('Could not post PR comment because this workflow token lacks permission.'); + return; + } + throw error; + } +} + +async function deletePreviousCodexInlineComments({ github, context, core, keepReviewId }) { + // Best-effort cleanup: remove inline comments left by earlier Codex runs so they do not pile up. + // Scoped by the hidden marker and the bot identity, and excludes the review just posted + // (keepReviewId). Any failure here is logged and swallowed so it can never fail a posted review. + let reviewComments; + try { + reviewComments = await github.paginate(github.rest.pulls.listReviewComments, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + per_page: 100, + }); + } catch (error) { + core.warning(`Could not list previous review comments: ${error.message}`); + return; + } + + const staleComments = reviewComments.filter( + (comment) => + comment + && comment.user + && comment.user.login === 'github-actions[bot]' + && typeof comment.body === 'string' + && comment.body.includes(CODEX_INLINE_MARKER) + && comment.pull_request_review_id !== keepReviewId, + ); + + for (const comment of staleComments) { + try { + await github.rest.pulls.deleteReviewComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: comment.id, + }); + core.info(`Deleted stale Codex inline comment ${comment.id}.`); + } catch (error) { + core.warning(`Could not delete stale Codex inline comment ${comment.id}: ${error.message}`); + } + } +} + +async function dismissPreviousCodexReviews({ github, context, core, runUrl, keepReviewId }) { + let reviews; + try { + reviews = await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + per_page: 100, + }); + } catch (error) { + core.warning(`Could not list previous pull request reviews: ${error.message}`); + return; + } + + const previousCodexReviews = reviews.filter( + (review) => review.id !== keepReviewId && isDismissableCodexReview(review), + ); + + for (const previousReview of previousCodexReviews) { + try { + await github.rest.pulls.dismissReview({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + review_id: previousReview.id, + message: `Superseded by Codex Review run ${runUrl}.`, + }); + core.info(`Dismissed previous Codex review ${previousReview.id}.`); + } catch (error) { + if (error.status === 403 || error.status === 422) { + core.warning(`Could not dismiss previous Codex review ${previousReview.id}: ${error.message}`); + continue; + } + throw error; + } + } +} + +module.exports = async function postReview({ github, context, core }) { + const pr = context.payload.pull_request; + // These are passthrough outputs from upstream jobs and can be empty strings when a job is skipped, + // so allowEmpty: true treats only a genuinely unset variable as missing. + const safetyFailure = requiredEnv('PREFLIGHT_SAFETY_FAILURE', { allowEmpty: true }) === 'true'; + const safetyMessage = process.env.PREFLIGHT_SAFETY_MESSAGE || ''; + const skipReason = process.env.PREFLIGHT_SKIP_REASON || ''; + const skipMessage = process.env.PREFLIGHT_SKIP_MESSAGE || ''; + const codexResult = requiredEnv('CODEX_RESULT', { allowEmpty: true }); + const runUrl = requiredEnv('RUN_URL', { allowEmpty: true }); + + if (safetyFailure) { + await createIssueComment({ + github, + context, + core, + body: safetyMessage || 'Codex review was not run because this PR changes reviewer automation files.', + }); + return; + } + + if (skipReason) { + await createIssueComment({ + github, + context, + core, + body: skipMessage || `Codex review was skipped during preflight (${skipReason}).`, + }); + return; + } + + if (codexResult !== 'success') { + await createIssueComment({ + github, + context, + core, + body: `Codex review failed before producing a usable review. Workflow run: ${runUrl}`, + }); + return; + } + + const reviewedBaseSha = requiredEnv('REVIEWED_BASE_SHA'); + const reviewedHeadSha = requiredEnv('REVIEWED_HEAD_SHA'); + if (!COMMIT_SHA_PATTERN.test(reviewedBaseSha)) { + throw new Error('REVIEWED_BASE_SHA must be a lowercase 40-character commit SHA'); + } + if (!COMMIT_SHA_PATTERN.test(reviewedHeadSha)) { + throw new Error('REVIEWED_HEAD_SHA must be a lowercase 40-character commit SHA'); + } + + let currentPullRequest; + try { + currentPullRequest = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + }); + } catch (error) { + core.warning(`Could not verify the current pull request head: ${error.message}`); + await createIssueComment({ + github, + context, + core, + body: `Codex review completed, but the current pull request head could not be verified, so no review was posted. Workflow run: ${runUrl}`, + }); + core.setFailed('Could not verify the current pull request head before posting the Codex review.'); + return; + } + + const currentHeadSha = currentPullRequest.data?.head?.sha; + const currentBaseSha = currentPullRequest.data?.base?.sha; + if (currentHeadSha !== reviewedHeadSha) { + core.warning(`Codex reviewed ${reviewedHeadSha}, but the pull request head is now ${currentHeadSha || 'unknown'}.`); + await createIssueComment({ + github, + context, + core, + body: `Codex reviewed commit ${reviewedHeadSha.slice(0, 12)}, but the pull request head changed before the review could be posted. Reapply the review label to review the current head. Workflow run: ${runUrl}`, + }); + return; + } + if (currentBaseSha !== reviewedBaseSha) { + core.warning(`Codex reviewed base ${reviewedBaseSha}, but the pull request base is now ${currentBaseSha || 'unknown'}.`); + await createIssueComment({ + github, + context, + core, + body: `Codex reviewed against base commit ${reviewedBaseSha.slice(0, 12)}, but the pull request base changed before the review could be posted. Reapply the review label to review the current base. Workflow run: ${runUrl}`, + }); + return; + } + + let review; + try { + review = readReviewOutput(requiredEnv('CODEX_OUTPUT_DIR')); + validateReview(review); + } catch (error) { + await createIssueComment({ + github, + context, + core, + body: `Codex review produced invalid structured output, so no approval or request-changes review was submitted. Workflow run: ${runUrl}`, + }); + core.setFailed(error.message); + return; + } + + let files; + try { + files = await github.paginate(github.rest.pulls.listFiles, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + per_page: 100, + }); + } catch (error) { + // Without the changed-file patches we cannot map inline comments, and every other API path + // degrades to a comment rather than a hard failure. Do the same so a valid Codex review still + // surfaces some PR feedback instead of an opaque crashed step. + core.warning(`Could not list pull request files: ${error.message}`); + await createIssueComment({ + github, + context, + core, + body: `Codex review completed, but the changed-file list could not be retrieved, so no review was posted. Workflow run: ${runUrl}`, + }); + return; + } + + // listFiles returns patches for at most ~300 files and omits patches for very large or binary + // files. Inline comments targeting those paths get an empty patch here and fall through to + // unplaced_findings below by design (see the `valid` check) -- this degradation is expected. + const patchesByPath = new Map(); + for (const file of files) { + patchesByPath.set(file.filename, parsePatchLines(file.patch)); + } + + const comments = []; + const unplaced = []; + // Mirror of the placed inline comments as plain findings, used to fold them back into the review + // body if GitHub rejects the inline comments wholesale (see the 422 fallback below). + const placedFindings = []; + + for (const comment of review.inline_comments) { + const { placed, patchMissing } = placeInlineComment({ + candidate: comment, + side: comment.side, + patchesByPath, + comments, + placedFindings, + }); + + if (!placed) { + // Distinguish a patch-less path (listFiles truncation / binary / >~300 changed files) from a + // line the model picked that simply is not part of the diff -- different root causes. + const reason = patchMissing + ? 'no patch was returned for this path (large/binary file or listFiles truncation)' + : `line ${comment.line} (${comment.side}) is not part of the diff`; + core.warning(`Demoted inline comment on ${comment.path}: ${reason}.`); + unplaced.push({ + severity: comment.severity, + body: comment.body, + path: comment.path, + line: comment.line, + }); + } + } + + for (const finding of review.unplaced_findings) { + // Unplaced findings only ever attach to the new side; when they cannot be located they stay in + // the unplaced list as-is. + const { placed } = placeInlineComment({ + candidate: finding, + side: 'RIGHT', + patchesByPath, + comments, + placedFindings, + }); + + if (!placed) { + unplaced.push(finding); + } + } + + const body = buildReviewBody(review, unplaced, comments.length, reviewedBaseSha); + const event = reviewEventForSeverity(review.highest_severity); + + core.info(`Codex review: placing ${comments.length} inline ${pluralize(comments.length, 'comment')}, ${unplaced.length} unplaced, event=${event}.`); + + let created; + try { + created = await github.rest.pulls.createReview({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + commit_id: reviewedHeadSha, + body, + event, + comments, + }); + } catch (error) { + if (error.status === 403) { + await createIssueComment({ + github, + context, + core, + body: `Codex review completed, but the workflow token could not submit a pull request review. Workflow run: ${runUrl}`, + }); + // The new review was never posted, so leave any previous (possibly blocking) review in place. + return; + } + + // GitHub rejects the whole review with 422 if a single inline comment lands on a line it does + // not consider commentable. Rather than lose every finding, retry once without inline comments + // and fold them into the body as unplaced findings. A failure of this retry propagates + // (skipping the cleanup below), again leaving any previous review in place. + if (error.status === 422 && comments.length > 0) { + core.warning(`GitHub rejected the inline comments (422): ${error.message}. Retrying without inline comments.`); + const fallbackBody = buildReviewBody(review, [...unplaced, ...placedFindings], 0, reviewedBaseSha); + created = await github.rest.pulls.createReview({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + commit_id: reviewedHeadSha, + body: fallbackBody, + event, + comments: [], + }); + core.info('Posted a comment-free Codex review after the inline comments were rejected.'); + } else { + throw error; + } + } + + // Clean up after the new review is safely posted. Delete inline comments left by earlier runs + // (keeping the one just created), then supersede earlier reviews. Dismissing first risked removing + // a previous blocking review and then failing to post the replacement, silently unblocking the PR. + await deletePreviousCodexInlineComments({ github, context, core, keepReviewId: created?.data?.id }); + await dismissPreviousCodexReviews({ + github, + context, + core, + runUrl, + keepReviewId: created?.data?.id, + }); +}; + +// Exported for unit testing. The workflow only calls the default postReview export; these named +// helpers are attached so their logic can be exercised in isolation (see post-review.test.js). +module.exports.parsePatchLines = parsePatchLines; +module.exports.resolveReviewOutputPath = resolveReviewOutputPath; +module.exports.readReviewOutput = readReviewOutput; +module.exports.validateReview = validateReview; +module.exports.expectedHighestSeverity = expectedHighestSeverity; +module.exports.countFindingsBySeverity = countFindingsBySeverity; +module.exports.buildReviewBody = buildReviewBody; +module.exports.formatInlineCommentBody = formatInlineCommentBody; +module.exports.reviewEventForSeverity = reviewEventForSeverity; +module.exports.isDismissableCodexReview = isDismissableCodexReview; +module.exports.CODEX_REVIEW_MARKER = CODEX_REVIEW_MARKER; +module.exports.CODEX_INLINE_MARKER = CODEX_INLINE_MARKER; +module.exports.CODEX_REVIEW_OUTPUT_FILE = CODEX_REVIEW_OUTPUT_FILE; +module.exports.REVIEW_LIMITS = REVIEW_LIMITS; diff --git a/review/post-review.test.js b/review/post-review.test.js new file mode 100644 index 0000000..1005076 --- /dev/null +++ b/review/post-review.test.js @@ -0,0 +1,1245 @@ +'use strict'; + +// Unit tests for post-review.js. No external dependencies: run with +// node --test review/post-review.test.js +// The workflow calls the default postReview export; the named helpers are attached for testing. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const postReview = require('./post-review.js'); +const { buildCodexReviewHeader } = require('./review-constants'); +const { + parsePatchLines, + resolveReviewOutputPath, + readReviewOutput, + validateReview, + expectedHighestSeverity, + countFindingsBySeverity, + buildReviewBody, + formatInlineCommentBody, + reviewEventForSeverity, + isDismissableCodexReview, + CODEX_REVIEW_MARKER, + CODEX_INLINE_MARKER, + CODEX_REVIEW_OUTPUT_FILE, + REVIEW_LIMITS, +} = postReview; + +const REVIEWED_HEAD_SHA = 'a'.repeat(40); +const REVIEWED_BASE_SHA = 'b'.repeat(40); + +// --- parsePatchLines -------------------------------------------------------- + +test('parsePatchLines: null and empty patches produce empty sets', () => { + for (const input of [null, undefined, '']) { + const { right, left } = parsePatchLines(input); + assert.equal(right.size, 0); + assert.equal(left.size, 0); + } +}); + +test('parsePatchLines: maps added, removed, and context lines to the correct sides', () => { + // The `patch` field from pulls.listFiles starts at the first @@ hunk header and contains no + // `---`/`+++` file-header lines, so this fixture deliberately omits them. + const patch = [ + '@@ -10,3 +10,4 @@ function context()', + ' context1', + '-removed', + '+added1', + '+added2', + ' context2', + '\\ No newline at end of file', + ].join('\n'); + + const { right, left } = parsePatchLines(patch); + + // RIGHT (new file) covers context + added lines; LEFT (old file) covers context + removed lines. + assert.deepEqual([...right].sort((a, b) => a - b), [10, 11, 12, 13]); + assert.deepEqual([...left].sort((a, b) => a - b), [10, 11, 12]); +}); + +test('parsePatchLines: handles single-line hunk headers without counts', () => { + const patch = ['@@ -1 +1 @@', '-old', '+new'].join('\n'); + const { right, left } = parsePatchLines(patch); + assert.deepEqual([...right], [1]); + assert.deepEqual([...left], [1]); +}); + +test('parsePatchLines: counts source lines that begin with ++ or -- (not file headers)', () => { + // A hunk `patch` never contains `---`/`+++` file headers, so an added line whose source begins + // with `++` renders as `+++...` and a removed line whose source begins with `--` renders as + // `---...`. These are real content lines and must advance the line counters. + const patch = [ + '@@ -1,2 +1,3 @@', + ' ctx', + '---removedDashes', + '+++addedPluses', + '+afterAdded', + ].join('\n'); + + const { right, left } = parsePatchLines(patch); + + // ctx=1, +++addedPluses=2, +afterAdded=3 on the new side; ctx=1, ---removedDashes=2 on the old. + assert.deepEqual([...right].sort((a, b) => a - b), [1, 2, 3]); + assert.deepEqual([...left].sort((a, b) => a - b), [1, 2]); +}); + +test('parsePatchLines: resets line counters across multiple hunks', () => { + const patch = [ + '@@ -1,2 +1,2 @@', + ' a', + '+b', + '@@ -10,2 +20,2 @@', + ' c', + '+d', + ].join('\n'); + + const { right, left } = parsePatchLines(patch); + + assert.deepEqual([...right].sort((a, b) => a - b), [1, 2, 20, 21]); + assert.deepEqual([...left].sort((a, b) => a - b), [1, 10]); +}); + +// --- validateReview --------------------------------------------------------- + +function validReview(overrides = {}) { + return { + review_body_markdown: 'summary', + diagnostics_markdown: 'diagnostics', + highest_severity: 'none', + findings: { blocking: 0, medium: 0, low_polish: 0 }, + inline_comments: [], + unplaced_findings: [], + ...overrides, + }; +} + +test('validateReview: accepts a minimal valid review', () => { + assert.doesNotThrow(() => validateReview(validReview())); +}); + +test('validateReview: recomputes findings and highest_severity from structured findings', () => { + const review = validReview({ + findings: { blocking: 0, medium: 0, low_polish: 0 }, + highest_severity: 'none', + inline_comments: [ + { path: 'a.js', line: 1, side: 'RIGHT', severity: 'medium', body: 'b', rule_source: null }, + { path: 'b.js', line: 2, side: 'RIGHT', severity: 'low', body: 'b', rule_source: null }, + ], + unplaced_findings: [ + { severity: 'blocking', body: 'b', path: null, line: null }, + ], + }); + validateReview(review); + assert.deepEqual(review.findings, { blocking: 1, medium: 1, low_polish: 1 }); + assert.equal(review.highest_severity, 'blocking'); +}); + +test('validateReview: ignores inconsistent model-provided finding counts', () => { + // Counts claim no findings, but a blocking inline comment is present: the normalized review must + // be treated as blocking so it is posted as REQUEST_CHANGES with a truthful overview table. + const blockingComment = validReview({ + findings: { blocking: 0, medium: 0, low_polish: 0 }, + highest_severity: 'none', + inline_comments: [ + { path: 'a.js', line: 1, side: 'RIGHT', severity: 'blocking', body: 'b', rule_source: null }, + ], + }); + validateReview(blockingComment); + assert.deepEqual(blockingComment.findings, { blocking: 1, medium: 0, low_polish: 0 }); + assert.equal(blockingComment.highest_severity, 'blocking'); + + // Likewise an unplaced medium finding outranks all-zero counts. + const mediumUnplaced = validReview({ + findings: { blocking: 0, medium: 0, low_polish: 0 }, + highest_severity: 'none', + unplaced_findings: [{ severity: 'medium', body: 'b', path: null, line: null }], + }); + validateReview(mediumUnplaced); + assert.deepEqual(mediumUnplaced.findings, { blocking: 0, medium: 1, low_polish: 0 }); + assert.equal(mediumUnplaced.highest_severity, 'medium'); + + // A stale blocking count must not turn a lower-severity structured finding into a blocking review. + const structuredFindingsWin = validReview({ + findings: { blocking: 1, medium: 0, low_polish: 0 }, + highest_severity: 'none', + inline_comments: [ + { path: 'a.js', line: 1, side: 'RIGHT', severity: 'low', body: 'b', rule_source: null }, + ], + }); + validateReview(structuredFindingsWin); + assert.deepEqual(structuredFindingsWin.findings, { blocking: 0, medium: 0, low_polish: 1 }); + assert.equal(structuredFindingsWin.highest_severity, 'low'); +}); + +test('validateReview: rejects non-object payloads', () => { + for (const bad of [null, undefined, [], 'x', 42]) { + assert.throws(() => validateReview(bad)); + } +}); + +test('validateReview: rejects unknown properties at every structured object boundary', () => { + assert.throws( + () => validateReview({ ...validReview(), unexpected: true }), + /Codex output must contain exactly these properties/, + ); + assert.throws( + () => validateReview(validReview({ + findings: { blocking: 0, medium: 0, low_polish: 0, critical: 1 }, + })), + /findings must contain exactly these properties/, + ); + assert.throws( + () => validateReview(validReview({ + inline_comments: [{ + path: 'a.js', line: 1, side: 'RIGHT', severity: 'low', body: 'b', rule_source: null, command: 'run me', + }], + })), + /inline_comments\[0\] must contain exactly these properties/, + ); + assert.throws( + () => validateReview(validReview({ + unplaced_findings: [{ + severity: 'low', body: 'b', path: null, line: null, html: '