[SOURCE-324] Harden AI review workflow

README.md

@@ -1,14 +1,14 @@
 # code-review-action
 
-A reusable GitHub Actions workflow that runs an AI model as a read-only code reviewer on pull requests, with a three-job security split and support for multiple providers.
+A reusable GitHub Actions workflow that runs an AI model as a read-only code reviewer on pull requests, with a six-stage security split and support for multiple providers.
 
 ## Providers
 
 | Input value | Action used | Secret required |
 |---|---|---|
 | `claude` (default) | [anthropics/claude-code-action](https://github.com/anthropics/claude-code-action) | `anthropic_api_key` |
-| `codex` | [openai/codex-action](https://github.com/openai/codex-action) | `openai_api_key` |
-| `gemini` | [google-github-actions/run-gemini-cli](https://github.com/google-github-actions/run-gemini-cli) | `gemini_api_key` |
+| `codex` | [openai/codex-action](https://github.com/openai/codex-action) with Codex CLI `0.144.5` | `openai_api_key` |
+| `gemini` | [Gemini CLI](https://github.com/google-gemini/gemini-cli) `0.47.0` | `gemini_api_key` |
 
 ## Trigger modes
 
@@ -38,6 +38,7 @@ jobs:
     with:
       provider:      claude        # claude | codex | gemini
       trigger_mode:  on_demand     # always | on_demand
+      review_event:  COMMENT_ONLY  # COMMENT_ONLY | ALL
       prompt_file:   .claude/review-prompt.md   # optional
     secrets:
       anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
@@ -59,12 +60,13 @@ Add the API key for your chosen provider as a repository secret:
 |---|---|---|---|
 | `provider` | string | `claude` | AI provider: `claude`, `codex`, or `gemini`. |
 | `trigger_mode` | string | `always` | `always` runs on PR events; `on_demand` requires a `/dd-review` comment from a write-access collaborator. |
-| `prompt_file` | string | `""` | Newline-separated list of Markdown review guide paths (read from the default branch). Root-level files apply to all PRs; subdirectory files apply only when changed files share that prefix. Falls back to a built-in prompt when empty or no file matches. Mutually exclusive with `prompt_file_pattern`. |
-| `prompt_file_pattern` | string | `""` | Glob pattern (evaluated against the default branch) used to auto-discover review guide files instead of listing them, e.g. `**/codereview_guideline.md`. Every matched file follows the same scoping rule as `prompt_file`. Mutually exclusive with `prompt_file`. |
+| `prompt_file` | string | `""` | Newline-separated list of Markdown review guide paths (read from the default-branch commit pinned by `gate`). Root-level files apply to all PRs; subdirectory files apply only when changed files share that prefix. Falls back to a built-in prompt when empty or no file matches. Mutually exclusive with `prompt_file_pattern`. |
+| `prompt_file_pattern` | string | `""` | Glob pattern (evaluated against the default-branch commit pinned by `gate`) used to auto-discover review guide files instead of listing them, e.g. `**/codereview_guideline.md`. Every matched file follows the same scoping rule as `prompt_file`. Mutually exclusive with `prompt_file`. |
+| `review_event` | string | `COMMENT_ONLY` | `COMMENT_ONLY` always posts a non-approving review. `ALL` allows the model to request changes or approve. |
 
 ## Custom review guide
 
-Pass a newline-separated list of paths via `prompt_file`, or a single glob via `prompt_file_pattern` to auto-discover guide files instead of listing them explicitly. The two inputs are mutually exclusive — the workflow fails fast if both are set. Files are read from the **default branch** only — a PR cannot rewrite its own review instructions.
+Pass a newline-separated list of paths via `prompt_file`, or a single glob via `prompt_file_pattern` to auto-discover guide files instead of listing them explicitly. The two inputs are mutually exclusive — the workflow fails fast if both are set. Files are read from a commit snapshot of the **default branch** pinned by `gate` — a PR cannot rewrite its own review instructions, and a branch update during the run cannot change them.
 
 **Scoping rule:** a file at the repo root applies to every PR; a file under a subdirectory (e.g. `bazel/guide.md`) applies only when at least one changed file lives under that directory. This rule applies identically whether the file came from `prompt_file` or was discovered via `prompt_file_pattern`.
 
@@ -122,23 +124,28 @@ Exit code is `1` when `error` is set, `2` on a usage error, `0` otherwise. `bin/
 
 ## Security model
 
-The pipeline uses a **three-job split**:
+The pipeline uses a **six-stage split** (only the selected provider job runs):
 
 ```
-gate  ──►  review_{provider}  ──►  post
+gate  ──►  start_signal + prepare  ──►  review_{provider}  ──►  post  ──►  finish_signal
 ```
 
 | Job | GitHub permissions | What it does |
 |---|---|---|
-| `gate` | `contents: read`, `pull-requests: read` | Validates the trigger, authorizes the actor (on_demand), resolves PR SHAs. |
+| `gate` | `contents: read`, `pull-requests: read`, `checks: write` | Validates inputs and the trigger, authorizes the actor (on_demand), pins PR and trusted-guide SHAs, and opens the check run. |
+| `start_signal` | `pull-requests: write` | Adds the in-progress reaction for on-demand requests. Never runs AI. |
+| `prepare` | `contents: read`, `pull-requests: read` | Generates a complete local diff for the pinned PR commits and assembles trusted review inputs. |
 | `review_*` | `contents: read`, `pull-requests: read` | Runs the AI with read-only tools. No write permissions. |
 | `post` | `contents: read`, `pull-requests: write` | Downloads the artifact, re-scans, posts the review. Never runs AI. |
+| `finish_signal` | `pull-requests: write`, `checks: write` | Closes the check run, reports technical failures, and updates the on-demand reaction. Never runs AI. |
 
 ### Trust boundaries
 
-- The PR head is checked out into `__untrusted/` (full repo at PR state, for reference during review). The AI is instructed to treat all content there as untrusted user input.
-- Trusted files (review guide, scripts) come from the **default branch** via sparse checkout. The PR cannot modify them.
-- `.claude/settings.json` is **not** checked out to prevent plugin/MCP server loading that would expand the tool surface.
+- Claude and Gemini check the PR head out under `__untrusted/`; Codex uses the workspace root because its action expects a repository there. Claude is limited to `Read`, `Glob`, and `Grep`; Gemini and Codex have their provider-specific instruction/config files removed before model execution, and Codex clears PR-controlled artifact/output paths before downloading trusted inputs.
+- `_prepare/untrusted/` contains the complete local PR diff and API-derived changed-file list for the SHAs pinned by `gate`. The workflow verifies the pull ref and API state during preparation, then checks the head again immediately before submission; it fails if the PR moved or closed.
+- `_prepare/trusted/` contains the assembled review guide, common schema, and validator. The schema and validator are checked out from `job.workflow_sha`, the exact reusable-workflow revision. Review guides are read from the calling repository's default-branch commit pinned by `gate`.
+- `post` treats every downloaded artifact as data: it checks out its validator independently at `job.workflow_sha`, downloads model output into an isolated directory, accepts only a bounded regular `review.json` file, and never executes artifact content.
+- Fork heads are fetched through the base repository's `refs/pull/<number>/head` ref, so authorized `on_demand` reviews do not need credentials for the fork repository.
 
 ### Secret scanning (two passes)
 
@@ -162,16 +169,31 @@ AI output is checked for shell commands (`curl`, `wget`, `bash`, etc.) and attem
 - `persist-credentials: false` on all checkouts — leaves no token in `.git/config`.
 - Fork PRs are skipped in `always` mode to prevent API key exposure.
 - In `on_demand` mode, the commenter's permission is checked via the `collaborators/.../permission` API (repo-scoped, not the org-wide `author_association` which would over-grant).
-- The Claude sentinel `allowed_non_write_users: "__force_sandbox_dummy__"` activates subprocess isolation without granting any permission bypass.
-- `CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "1"` prevents the Anthropic key from leaking into Claude's subprocesses.
-- Concurrency is keyed per PR so a second trigger cancels the prior in-flight run.
+- The Claude action is SHA-pinned and runs in agent mode with only `Read`, `Glob`, and `Grep`; shell and write tools are explicitly denied. Project/local settings are disabled, execution is capped at 10 turns, and its subprocess isolation path provides best-effort credential scrubbing with bubblewrap where supported.
+- Claude uses the action's schema-backed `structured_output`. The workflow validates and scans that exact value before artifact upload; it never searches the execution transcript or repairs malformed output.
+- The Codex action and CLI are both pinned; Codex runs with the `:read-only` permission profile and `drop-sudo`. `AGENTS.md`, `AGENTS.override.md`, and other AI instruction files are removed at every directory depth before execution. Before the action's pre-sandbox CLI installation, PR-controlled `.npmrc` files are removed, npm user configuration is disabled, the public npm registry is selected explicitly, and lifecycle scripts are disabled.
+- Gemini CLI is pinned to `0.47.0`, verified against a fixed SHA-512, and installed without lifecycle scripts or optional keychain/PTY dependencies. It runs in its digest-pinned matching Docker sandbox with an isolated home directory and receives only the Gemini API key plus minimal runtime environment; the GitHub token and Actions command-file paths are not inherited.
+- Gemini extensions and MCP are disabled. Its only tools are `read_file`, `glob`, `grep_search`, and `list_directory`; repository `GEMINI.md` and `.gemini` content is removed at every depth before workspace trust is enabled.
+- Gemini output is captured locally and validated before posting. It is never written to `GITHUB_STEP_SUMMARY`.
+- Provider output is accepted only when it matches the complete shared schema. Invalid event values, missing `side`, unknown fields, and more than 100 comments fail closed instead of being repaired or truncated.
+- Fork reviews are always posted as `COMMENT`; `review_event: ALL` can only pass through approvals or change requests for same-repository pull requests.
+- Completion reactions use the event actually posted after policy enforcement. A model `APPROVE` downgraded by `COMMENT_ONLY`, or a review that falls back to an issue comment, cannot produce an approval reaction.
+- Provider jobs have a 30-minute timeout so a stalled model or dependency fetch cannot occupy a runner indefinitely.
+- Concurrency is keyed per PR and trigger mode so a replacement trigger cancels the prior in-flight run. A canceled run closes its check as `cancelled` without posting a false technical-failure comment or reaction.
 
 ## Schemas
 
-- [`schemas/github-review.json`](schemas/github-review.json) — JSON schema for the AI review payload (GitHub `POST /pulls/{n}/reviews` shape). Used by Claude and Gemini; Codex uses the same shape via an inline schema written at runtime.
+- [`schemas/github-review.json`](schemas/github-review.json) — the single JSON schema for every provider and the GitHub review payload.
+- [`src/scan.js`](src/scan.js) — the shared fail-closed validator and output scanner used in provider jobs and again before posting.
 
 ## Limitations
 
 - Fork PRs are not reviewed in `always` mode (provider API keys would be exposed to untrusted code). Use `on_demand` if you want to review fork PRs selectively.
-- The `gemini` provider uses `--yolo` (auto-approve all tool calls) as required by the upstream action. Tool restriction is enforced via the `settings` input using `tools.core` with snake_case built-in names (`read_file`, `glob`, `grep_search`, `list_directory`).
+- Datadog's strict security pattern assumes `review_event: COMMENT_ONLY`. Selecting `ALL` deliberately relaxes that boundary for same-repository PRs and lets prompt-influenced model output approve or request changes; only enable it where merge policy explicitly permits AI-authored review decisions.
+- Claude's read-only tools can inspect unchanged files for review context. Their filesystem access is broader than the prepared diff, so the PR checkout and all model output remain untrusted.
+- The pinned Claude action installs its fixed CLI version through Anthropic's mutable installer endpoint at runtime; the action SHA does not pin that installer response.
+- The `gemini` provider uses `--approval-mode yolo` only after reducing the tool registry to four read-only tools. It has no shell, write, MCP, or extension tool to auto-approve.
+- Gemini's sandboxed CLI process needs network access to call the Gemini API. The workflow provides no model-callable network tool, but it does not enforce destination-level egress filtering on that API connection.
+- GitHub's pull-request files API returns at most 3,000 files. The workflow detects an incomplete list and fails preparation rather than applying review-guide scope to partial data.
+- Complete diffs larger than 1,000,000 bytes or 20,000 lines fail preparation instead of silently sending a truncated change to the model.
 - All three providers use the same output format (`github-review.json` shape). The `review_event` policy controls whether `REQUEST_CHANGES` and `APPROVE` are passed through or downgraded to `COMMENT`.

schemas/github-review.json

@@ -2,7 +2,7 @@
   "$schema": "http://json-schema.org/draft-07/schema#",
   "$id": "github-review.json",
   "title": "AI review payload",
-  "description": "Shape for GitHub's POST /repos/{owner}/{repo}/pulls/{pull_number}/reviews. `commit_id` is omitted — the post job injects it. `event` allows COMMENT, REQUEST_CHANGES, or APPROVE; the post job enforces the review_event policy. `comments` is capped at 100. `side` and `start_side` are constrained to LEFT/RIGHT so typos fail at schema validation rather than at the GitHub API.",
+  "description": "Shape for GitHub's POST /repos/{owner}/{repo}/pulls/{pull_number}/reviews. `commit_id` is omitted because the post job injects it. `event` allows COMMENT, REQUEST_CHANGES, or APPROVE; the post job enforces the review_event policy. Inline comments are single-line and capped at 100.",
   "type": "object",
   "required": ["body", "event", "comments"],
   "additionalProperties": false,
@@ -14,15 +14,13 @@
       "maxItems": 100,
       "items": {
         "type": "object",
-        "required": ["path", "body", "line"],
+        "required": ["path", "body", "line", "side"],
         "additionalProperties": false,
         "properties": {
-          "path":       { "type": "string" },
+          "path":       { "type": "string", "minLength": 1 },
           "body":       { "type": "string" },
           "line":       { "type": "integer", "minimum": 1 },
-          "side":       { "type": "string", "enum": ["LEFT", "RIGHT"] },
-          "start_line": { "type": "integer", "minimum": 1 },
-          "start_side": { "type": "string", "enum": ["LEFT", "RIGHT"] }
+          "side":       { "type": "string", "enum": ["LEFT", "RIGHT"] }
         }
       }
     }

src/scan.js

@@ -24,53 +24,86 @@ const CANARY_PATTERNS = [
   />>?\s*\$GITHUB_OUTPUT/,
   />>?\s*\$GITHUB_ENV/,
 ];
-function hasToken(v) {
-  if (typeof v === 'string') return TOKEN_PATTERNS.some(p => p.test(v));
-  if (Array.isArray(v))      return v.some(hasToken);
-  if (v && typeof v === 'object') return Object.values(v).some(hasToken);
+
+const VALID_EVENTS = new Set(['COMMENT', 'REQUEST_CHANGES', 'APPROVE']);
+const REVIEW_FIELDS = new Set(['body', 'event', 'comments']);
+const COMMENT_FIELDS = new Set(['path', 'body', 'line', 'side']);
+const FALLBACK_MARKER = '<!-- ai-review-status:suppressed -->';
+
+function matchesTree(value, patterns) {
+  if (typeof value === 'string') return patterns.some(pattern => pattern.test(value));
+  if (Array.isArray(value)) return value.some(item => matchesTree(item, patterns));
+  if (value && typeof value === 'object') {
+    return Object.values(value).some(item => matchesTree(item, patterns));
+  }
   return false;
 }
-function hasCanary(v) {
-  if (typeof v === 'string') return CANARY_PATTERNS.some(p => p.test(v));
-  if (Array.isArray(v))      return v.some(hasCanary);
-  if (v && typeof v === 'object') return Object.values(v).some(hasCanary);
-  return false;
+
+function hasToken(value) {
+  return matchesTree(value, TOKEN_PATTERNS);
+}
+
+function hasCanary(value) {
+  return matchesTree(value, CANARY_PATTERNS);
 }
+
 function makeFallback(msg, runUrl) {
   return {
-    body:     `> [!WARNING]\n> **AI review could not be posted:** ${msg}\n>\n> See [workflow run](${runUrl}) for details.`,
+    body:     `${FALLBACK_MARKER}\n> [!WARNING]\n> **AI review could not be posted:** ${msg}\n>\n> See [workflow run](${runUrl}) for details.`,
     event:    'COMMENT',
     comments: [],
   };
 }
-const VALID_EVENTS = ['COMMENT', 'REQUEST_CHANGES', 'APPROVE'];
+
+function isFallback(review) {
+  return typeof review?.body === 'string' && review.body.startsWith(`${FALLBACK_MARKER}\n`);
+}
+
+function rejectUnknownFields(value, allowedFields, location, errors) {
+  for (const field of Object.keys(value)) {
+    if (!allowedFields.has(field)) errors.push(`${location}.${field} is not allowed`);
+  }
+}
+
 function validateReview(review) {
   const errors = [];
-  if (!review || typeof review !== 'object') {
-    errors.push('review must be a non-null object');
-    return { errors };
+  if (!review || typeof review !== 'object' || Array.isArray(review)) {
+    return { errors: ['review must be a non-null object'] };
   }
+
+  rejectUnknownFields(review, REVIEW_FIELDS, 'review', errors);
   if (typeof review.body !== 'string')
     errors.push('body must be a string');
-  if (!VALID_EVENTS.includes(review.event))
-    errors.push(`event must be one of ${VALID_EVENTS.join('|')}`);
+  if (!VALID_EVENTS.has(review.event))
+    errors.push('event must be one of COMMENT|REQUEST_CHANGES|APPROVE');
+
   if (!Array.isArray(review.comments)) {
     errors.push('comments must be an array');
-  } else {
-    for (let i = 0; i < review.comments.length; i++) {
-      const c = review.comments[i];
-      if (!c || typeof c !== 'object') {
-        errors.push(`comments[${i}] must be an object`);
-      } else {
-        if (typeof c.path !== 'string')
-          errors.push(`comments[${i}].path must be a string`);
-        if (typeof c.body !== 'string')
-          errors.push(`comments[${i}].body must be a string`);
-        if (!Number.isInteger(c.line) || c.line < 1)
-          errors.push(`comments[${i}].line must be a positive integer`);
-      }
+    return { errors };
+  }
+  if (review.comments.length > 100)
+    errors.push('comments must contain at most 100 entries');
+
+  for (let i = 0; i < review.comments.length; i++) {
+    const comment = review.comments[i];
+    const location = `comments[${i}]`;
+    if (!comment || typeof comment !== 'object' || Array.isArray(comment)) {
+      errors.push(`${location} must be an object`);
+      continue;
     }
+
+    rejectUnknownFields(comment, COMMENT_FIELDS, location, errors);
+    if (typeof comment.path !== 'string' || comment.path.length === 0)
+      errors.push(`${location}.path must be a non-empty string`);
+    if (typeof comment.body !== 'string')
+      errors.push(`${location}.body must be a string`);
+    if (!Number.isInteger(comment.line) || comment.line < 1)
+      errors.push(`${location}.line must be a positive integer`);
+    if (comment.side !== 'LEFT' && comment.side !== 'RIGHT')
+      errors.push(`${location}.side must be LEFT or RIGHT`);
   }
+
   return { errors };
 }
-module.exports = { hasToken, hasCanary, makeFallback, validateReview };
+
+module.exports = { hasToken, hasCanary, makeFallback, isFallback, validateReview };