diff --git a/.github/codex/rubric.md b/.github/codex/rubric.md new file mode 100644 index 0000000..2555f84 --- /dev/null +++ b/.github/codex/rubric.md @@ -0,0 +1,89 @@ +# Codex PR Review — Homelab — Claude Code-optimized + +You are a senior code reviewer reviewing a GitHub pull request on a **self-hosted homelab repository** (part of Chris Baker's home infrastructure stack running on a Mac Mini). The downstream consumer of your review is **Claude Code** — a coding agent that will programmatically read each finding and use it to make follow-up edits. Optimize every part of your output for machine parseability and direct action. + +Your output is validated against a JSON schema (`.github/codex/rubric.schema.json`). Adhere strictly. The schema constrains shape; this file teaches substance. + +**Schema requires every field to be emitted on every object** (OpenAI Structured Outputs strict mode). For semantically-absent values use these sentinels — never omit the key: + +| Field | "Absent" representation | +|---|---| +| `line_end` (single-line finding) | Set equal to `line_start`. | +| `refs` (no related findings) | `[]` (empty array). | +| `tags` (no category tags) | `[]` (empty array). | +| `fix_diff` (structural change, no patch) | `""` (empty string). | +| `verification_command` (diff is self-evidence) | `""` (empty string). | + +## Repository context + +This repository is one component of a self-hosted homelab. The fleet is a mixed stack — infer the specific stack of THIS repo from the diff and the files present. Across the homelab you will encounter: + +- **TypeScript / Node.js** — OpenClaw gateway plugins (dispatcher tools, hooks, channels; built with esbuild to `dist/index.js`), web apps (Next.js), and services. +- **Python** — the agent-memory-server (FastAPI/uvicorn), fleet/health monitors, Paperless AI pipelines, and helper scripts. +- **Bash/zsh** (`*.sh`, `Makefile` targets) — service lifecycle, monitors, backups, log rotation, update flows. Often linted by `shellcheck`. +- **JSON / YAML config** — service configs, cron job definitions, LaunchAgent/LaunchDaemon plists, CI workflows. +- **Markdown** — runbooks, audit reports, context (`CLAUDE.md`) files. + +This is a **live production-for-the-household system**: gateways exposed via Tailscale Serve / Cloudflare Tunnel, secrets in local `.env` files and credential stores, and many cron jobs. **Operational safety and secret hygiene matter more than code elegance.** It is largely single-tenant — do not raise multi-tenant or horizontal-scale concerns. + +## What to review — tiered by stack (apply the tiers relevant to this diff) + +**Tier 1 — Shell safety (highest signal in script-heavy repos).** +- Unquoted variable expansions (`$VAR` vs `"$VAR"`) — word-splitting/globbing bugs, especially paths that may contain spaces. +- Missing `set -euo pipefail` (or equivalent) in scripts that mutate state, delete files, or restart services. +- `rm -rf` / destructive ops with an unvalidated or possibly-empty variable in the path (`rm -rf "$DIR/"` where `$DIR` could be unset → `rm -rf /`). +- Command injection: untrusted input (webhook payloads, env, filenames) interpolated into `eval`, `bash -c`, or unquoted command strings. +- Pipelines that mask failure, `cd` without `|| exit`, races in start/stop scripts. Prefer `shellcheck`-clean; cite the SC code when you know it (e.g. SC2086). + +**Tier 2 — Secret & credential hygiene (treat as BLOCKER-class).** +- Any hardcoded secret, token, password, API key, or private key in a tracked file → **BLOCKER**. +- Secrets echoed to logs, committed `.env` values, or `set -x` left on around credential handling. +- World-readable secret files, secrets passed on the command line (visible in `ps`), or copied outside their credential store. + +**Tier 3 — Operational correctness.** +- Cron / job-schedule changes: schedule sanity, idempotency, overlap, failure visibility (does a failure alert or silently no-op?). +- LaunchAgent/LaunchDaemon plist correctness (KeepAlive, RunAtLoad, absolute paths, label uniqueness). +- Backup/restore flows: rsync invariants, retention, restore-tested path. +- Health/fleet monitors: false-negative risk (an alert that can't fire), rate-limit / heal-loop correctness. +- Network exposure: any change widening Tailscale Serve / Cloudflare Tunnel / loopback boundaries. + +**Tier 4 — Python / Node / TypeScript correctness.** +- Unhandled error paths, broad `except:` / swallowed catches, resource leaks (unclosed files/connections/timers), blocking calls in async paths. +- Input validation at service boundaries (HTTP-exposed services on the LAN/Tailscale; webhook signature/replay handling). +- Module-level mutable state shared across concurrent requests/sessions (race conditions). +- Dependency/version pins and lockfile consistency. For OpenClaw plugins: `contracts.tools` declared for each registered tool; `activation.onCapabilities:["hook"]` for hook-only plugins; telemetry emitted via the shared `PluginTelemetry`. + +**Tier 5 — Maintainability & docs.** +- Magic numbers/paths that should be constants or config; logic duplicated across files that should be shared. +- Runbook / `CLAUDE.md` drift: a behavior change with no doc update. +- Dead code, commented-out blocks, TODO without an owner. Missing/weak tests on changed logic. + +## Severity definitions + +| Severity | Meaning | Examples | +|---|---|---| +| `BLOCKER` | Must fix before merge. Correctness, safety, or secret-leak issue with high confidence. | Hardcoded secret; `rm -rf` on an unguarded variable; a script that can corrupt service state; command/signature-bypass injection. | +| `SHOULD_FIX` | Non-blocking but high-value; Claude Code should flag for follow-up. | Unquoted `$VAR` in a non-destructive path; missing `pipefail`; a cron job with no failure alert; missing doc/test update; broad `except`; an unguarded shutdown handler. | +| `NIT` | Style/polish. | Inconsistent quoting, a clearer name, a redundant `cat`. | + +**Bias:** when uncertain between a safety BLOCKER and SHOULD_FIX for a *destructive* or *secret-handling* path, round UP — the cost of a bad op here is real (data loss, exposed service). For everything else, round toward SHOULD_FIX/NIT to avoid noise. + +## Confidence calibration (0.0–1.0) + +- **0.95+** — "I would bet on this." `shellcheck` would reject it; a hardcoded secret is literally in the diff; an unguarded `rm -rf`. +- **0.80–0.94** — High confidence, but depends on runtime context not fully visible (e.g. whether `$DIR` is guaranteed set upstream). +- **0.60–0.79** — Plausible issue; flag it, but say what would confirm/refute it. +- **<0.60** — Do not emit as a finding. Put it in `notes_for_claude_code` if it's worth a look. + +## Output discipline + +- **`issue`**: one terse sentence stating the problem. No preamble. +- **`why`**: the concrete consequence (what breaks, what leaks, what silently fails). Up to ~700 chars for a BLOCKER; keep SHOULD_FIX/NIT tight. +- **`fix_diff`**: a minimal unified-diff patch when the fix is a localized edit; `""` for structural changes. +- **`verification_command`**: a command that proves the fix when one exists and is cheap (e.g. `shellcheck path/to/script.sh`, `python -m py_compile mod.py`, `pnpm vitest run x.test.ts`); else `""`. +- **`file` / `line_start` / `line_end`**: anchor every finding to the diff. +- **`tags`**: from `{shell, secrets, security, cron, launchd, backup, network, python, node, typescript, plugin, config, docs, tests, maintainability, style}` as applicable. + +Sandbox is `read-only` with no network. If a concern can't be verified from the diff alone (e.g. whether an env var is always set, whether a service tolerates the restart), say so explicitly in `why` and cap confidence accordingly — don't assert a runtime fact you can't check. + +For `test_coverage`: ops/config changes are commonly `NOT_APPLICABLE`; call `GAPS` only when changed logic (a function, a handler) plausibly could be covered and isn't. For `description_vs_implementation`: check the PR body against the diff and flag drift. `what_works_well`: 1–3 genuine positives. `notes_for_claude_code`: anything sub-threshold, plus what you couldn't verify from the sandbox. diff --git a/.github/codex/rubric.schema.json b/.github/codex/rubric.schema.json new file mode 100644 index 0000000..49fea28 --- /dev/null +++ b/.github/codex/rubric.schema.json @@ -0,0 +1,187 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "$id": "https://withfurther.com/schemas/codex-pr-review-v1.json", + "title": "Codex PR Review v1", + "description": "Structured PR review output emitted by openai/codex-action and consumed downstream by Claude Code. Stable contract — bump $id when changing field shapes.", + "type": "object", + "additionalProperties": false, + "required": [ + "verdict", + "summary", + "stats", + "findings", + "test_coverage", + "description_vs_implementation", + "what_works_well", + "notes_for_claude_code" + ], + "properties": { + "verdict": { + "type": "string", + "enum": ["APPROVE", "COMMENT", "REQUEST_CHANGES"], + "description": "APPROVE only if zero BLOCKER findings AND test coverage adequate AND description matches. Otherwise COMMENT (questions/non-blocking) or REQUEST_CHANGES (material gaps)." + }, + "summary": { + "type": "string", + "minLength": 10, + "maxLength": 280, + "description": "One sentence stating what the PR does and what the verdict hinges on. No restating diff." + }, + "stats": { + "type": "object", + "additionalProperties": false, + "required": ["files_changed", "lines_added", "lines_removed", "new_functions", "new_tests"], + "properties": { + "files_changed": { "type": "integer", "minimum": 0 }, + "lines_added": { "type": "integer", "minimum": 0 }, + "lines_removed": { "type": "integer", "minimum": 0 }, + "new_functions": { "type": "integer", "minimum": 0, "description": "Count of net-new exported/top-level functions in the diff." }, + "new_tests": { "type": "integer", "minimum": 0, "description": "Count of net-new it()/test()/describe() blocks in the diff." } + } + }, + "findings": { + "type": "array", + "maxItems": 30, + "description": "Discrete, actionable issues. Claude Code reads each as one work item. Order: BLOCKER first, then SHOULD_FIX, then NIT. Within a severity, highest confidence first. Cluster NITs per file — do NOT emit one finding per restated-code-comment line.", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "id", + "severity", + "tier", + "file", + "line_start", + "line_end", + "issue", + "why", + "confidence", + "fix_diff", + "refs", + "tags", + "verification_command" + ], + "properties": { + "id": { + "type": "string", + "pattern": "^F[0-9]+$", + "description": "Monotonic identifier F1, F2, … unique within this review. Used by `refs` cross-links." + }, + "severity": { + "type": "string", + "enum": ["BLOCKER", "SHOULD_FIX", "NIT"], + "description": "BLOCKER: must fix before merge (tier 2-4 correctness, security, AI-service divergence). SHOULD_FIX: non-blocking but high-value (DRY, perf, hygiene). NIT: style/reflex (tier 1)." + }, + "tier": { + "type": "integer", + "minimum": 1, + "maximum": 5, + "description": "Maps to rubric tier: 1=style-reflex, 2=correctness, 3=backend, 4=AI-service, 5=consistency." + }, + "file": { + "type": "string", + "minLength": 1, + "description": "Repo-relative path. Must be present in `git diff`'s changed-files set." + }, + "line_start": { + "type": "integer", + "minimum": 1, + "description": "Starting line in the head commit. 1-indexed." + }, + "line_end": { + "type": "integer", + "minimum": 1, + "description": "Ending line, inclusive. Equal to line_start for single-line findings." + }, + "issue": { + "type": "string", + "minLength": 5, + "maxLength": 280, + "description": "One-sentence problem statement. Do NOT restate the code — Claude Code will read the file." + }, + "why": { + "type": "string", + "minLength": 5, + "maxLength": 700, + "description": "The concrete consequence: correctness/security/perf/maintainability impact. Keep it to one sentence for SHOULD_FIX/NIT; a BLOCKER may run 2-3 sentences to show the reasoning chain that surfaces the bug (FIN-695). Not a mandate to be verbose — only headroom when the reasoning is subtle." + }, + "fix_diff": { + "type": "string", + "description": "Unified diff Claude Code may apply directly with `git apply`. Empty string if the fix is structural (rename, file-split, redesign) and a patch is misleading.", + "maxLength": 4000 + }, + "confidence": { + "type": "number", + "minimum": 0, + "maximum": 1, + "description": "0.95+ = I would bet on this. 0.75-0.94 = strong signal, worth raising. 0.60-0.74 = speculative — flag it as such in the why. <0.60 = do not emit." + }, + "refs": { + "type": "array", + "items": { "type": "string", "pattern": "^F[0-9]+$" }, + "maxItems": 5, + "description": "Optional list of related finding IDs in this same review (caller/callee, root-cause/symptom)." + }, + "tags": { + "type": "array", + "items": { + "type": "string", + "pattern": "^[a-z][a-z0-9-]*$" + }, + "maxItems": 4, + "description": "Category tags for downstream filtering. Open vocabulary — lowercase, hyphen-separated tokens. Common values include `security`, `correctness`, `performance`, `maintainability`, `style`, `tests`, `docs`, `consent`, `pii`, `ai-prompt-drift`, `react-hooks`, `backend`, `infra`, `dependency`, but the schema does not constrain to these. A closed enum here previously failed strict-mode validation when Codex emitted reasonable-sounding tags outside the list, killing the whole review (Finley's PR #190 review, FIN-269)." + }, + "verification_command": { + "type": "string", + "maxLength": 500, + "description": "The specific sandbox command (or short description of inspection) used to verify this finding's claim. REQUIRED for BLOCKER/SHOULD_FIX findings about external-system behavior (OpenAI API rules, GHA semantics, third-party action internals, runner sandbox, SDK methods) — those claims cannot be confidently asserted without sandbox-verifiable evidence per the verification-discipline rule in `code-pr-review-codex.md`. For findings about diff-visible code that needs no separate verification (the diff itself IS the evidence), emit an empty string \"\". Examples: \"cat .github/workflows/codex-review.yml | head -50\", \"diff hunk at file.ts:42-50 directly shows the issue\", \"rg 'PATTERN' --type ts (5 matches; consistent form)\", \"could not verify from sandbox — emitted as SHOULD_FIX per verification-discipline rule\"." + } + } + } + }, + "test_coverage": { + "type": "object", + "additionalProperties": false, + "required": ["verdict", "note"], + "properties": { + "verdict": { + "type": "string", + "enum": ["ADEQUATE", "GAPS", "MISSING", "NOT_APPLICABLE"], + "description": "NOT_APPLICABLE only for docs-only, lockfile-only, or no-runtime-code changes." + }, + "note": { + "type": "string", + "minLength": 5, + "maxLength": 400, + "description": "If GAPS or MISSING, list specific file:line ranges that lack coverage." + } + } + }, + "description_vs_implementation": { + "type": "object", + "additionalProperties": false, + "required": ["consistent", "note"], + "properties": { + "consistent": { "type": "boolean" }, + "note": { + "type": "string", + "minLength": 5, + "maxLength": 400, + "description": "If consistent=false, list specific capabilities the description claims that don't ship, or that ship but aren't described." + } + } + }, + "what_works_well": { + "type": "array", + "maxItems": 5, + "items": { "type": "string", "minLength": 10, "maxLength": 200 }, + "description": "2-4 specific good decisions in this diff. NOT generic praise. Empty array is acceptable for trivial PRs." + }, + "notes_for_claude_code": { + "type": "array", + "maxItems": 5, + "items": { "type": "string", "minLength": 5, "maxLength": 300 }, + "description": "Free-form notes addressed to a downstream coding agent: caveats, things you could not verify in the sandbox, files worth reading even though no finding was emitted, follow-up suggestions." + } + } +} diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml new file mode 100644 index 0000000..05bc892 --- /dev/null +++ b/.github/workflows/codex-review.yml @@ -0,0 +1,474 @@ +name: codex-review + +# Optional, parallel Codex-based PR review. Runs alongside Finley's +# Anthropic Managed Agents (`agent_type: "code"`) Opus reviewer — both +# reviewers land on the same PR with distinct attribution. Codex output +# is shaped as a strict JSON document (see +# `.github/codex/rubric.schema.json`) and rendered to a +# markdown PR comment with the raw JSON embedded between HTML-comment +# sentinels for downstream parsing by Claude Code. +# +# Cost: at the pinned model (see env.CODEX_REVIEW_MODEL below — gpt-5.5) +# effort=xhigh, budget ~$0.30–$3.00 per non-trivial PR — the top of the prior +# medium-effort band (~$0.10–$1.00) shifts up (roughly 2–3× on large diffs) +# because xhigh spends materially more reasoning tokens + wall-clock for an +# exhaustive review (FIN-692). This is the deliberate thoroughness lever: Opus +# routinely caught issues medium-effort Codex missed on the same PR (FIN-691). +# The job timeout-minutes: 15 should still be ample; watch for timeouts on very +# large diffs. Re-baseline the per-PR cost from BQ telemetry after a model +# change (token efficiency differs across models). Monitor BQ telemetry +# (finding-count delta, BLOCKER FP rate, per-PR token cost) over the +# FIN-691 validation window. +# Concurrency-canceled on re-pushes so only the latest head SHA runs. +# +# Security posture mirrors ci.yml: zero interpolation of +# `github.event.*` fields into `run:` shell commands. All untrusted +# context routed through `env:` blocks. Codex output is written to +# a temp markdown file and posted via `gh ... --body-file `, +# so PR-controlled content cannot escape shell quoting. See: +# https://github.blog/security/vulnerability-research/how-to-catch-github-actions-workflow-injections-before-attackers-do/ +# +# Action pinned to commit SHA for SOC 2 supply-chain immutability — +# bump SHA + version comment together when upgrading codex-action. + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + +permissions: + contents: read + pull-requests: write + +# Single source of truth for the Codex review model. Change THIS one value (or +# set the repo/org Actions variable `CODEX_REVIEW_MODEL` to override every repo +# at once) to upgrade the model — it drives both the `codex-action` `model:` +# input AND the rendered `**Model:**` footer in lockstep, so the two can never +# drift. The `vars.CODEX_REVIEW_MODEL` fallback means: no variable set → the +# literal default below; variable set → that value wins fleet-wide. +env: + CODEX_REVIEW_MODEL: ${{ vars.CODEX_REVIEW_MODEL || 'gpt-5.5' }} + +concurrency: + group: codex-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + review: + name: Codex review + # Skip drafts (Codex tokens are not free), fork PRs (secrets are + # not exposed to fork-PR contexts anyway; explicit skip keeps the + # failure clean rather than producing a confusing 401), and + # bot-authored PRs (Dependabot / Renovate / etc.). The bot skip + # mirrors `allow-bots: "false"` on the codex-action step but + # short-circuits at the job level so the render step (with + # `if: always()`) doesn't post a misleading "Codex did not produce + # a review" comment on every bot PR. + if: | + github.event.pull_request.draft == false && + github.event.pull_request.head.repo.full_name == github.repository && + github.event.pull_request.user.type != 'Bot' + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Checkout PR head + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + # Full history so Codex can `git diff base..head` for the + # diff under review and walk caller/callee context. + fetch-depth: 0 + ref: ${{ github.event.pull_request.head.sha }} + # Don't leave GITHUB_TOKEN in local git config during a job that + # processes untrusted PR-controlled content. The Codex sandbox is + # read-only and drops sudo, but defense-in-depth. + persist-credentials: false + + - name: Run Codex review + id: codex + # `continue-on-error: true` keeps the action's failure scoped + # to this step so the overall job exits green. The fallback + # comment in the render step already communicates "Codex did + # not produce a review" — but without this, the workflow's CI + # check ALSO shows red on the PR, which contradicts the + # "non-blocking by design" contract written into the comment + # body. The action's true outcome is still readable via + # `steps.codex.outcome` (raw) vs `steps.codex.conclusion` + # (post-continue-on-error). Caught by Codex's own run-#5 review + # of this PR (FIN-269). + continue-on-error: true + # Release notes: https://github.com/openai/codex-action/releases/tag/v1.8 + uses: openai/codex-action@e0fdf01220eb9a88167c4898839d273e3f2609d1 # v1.8 + env: + # PR metadata Codex consumes at its discretion via shell + # commands inside the sandbox. Routing through env: (rather + # than interpolating into the prompt) prevents PR-title / + # PR-body injection into the action's invocation. + REPO_FULL_NAME: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + with: + openai-api-key: ${{ secrets.OPENAI_CODEX_REVIEW_KEY }} + # Pin the @openai/codex CLI for determinism + SOC 2 supply-chain + # hygiene (the action's default floats the CLI between runs, so the + # engine running the review was non-reproducible — FIN-694). 0.139.0 + # is the current `latest` (stable, non-alpha) dist-tag; 0.140.x is + # alpha-only. Verified it defines the `xhigh` reasoning-effort tier + # (codex-rs/protocol/src/openai_models.rs `XHigh => "xhigh"` at the + # rust-v0.139.0 tag) + --output-schema. 0.139.0 post-dates gpt-5.5's + # 2026-04-23 launch, so this CLI recognises the gpt-5.5 model id + # (the model id is passed through to the API, not a CLI feature). + # A bump is + # a SOC 2 change-management event — same discipline as the model + + # action-SHA pins; bump deliberately. + codex-version: "0.139.0" + prompt-file: .github/codex/rubric.md + output-schema-file: .github/codex/rubric.schema.json + output-file: ${{ runner.temp }}/codex-review.json + # `sandbox: read-only` — Codex cannot write to disk or make + # network calls. Independent of safety-strategy. + sandbox: read-only + # `safety-strategy: drop-sudo` — drops sudo from the runner's + # default user BEFORE invoking Codex, preventing in-memory + # reads of OPENAI_CODEX_REVIEW_KEY. NOT `read-only`: that + # input only constrains the filesystem/network sandbox and + # explicitly does NOT drop sudo (per action.yml docs, Codex + # could still memory-read the key under safety-strategy: + # read-only). drop-sudo is the action's default; pinning + # explicitly so the choice is documented and grep-able. + safety-strategy: drop-sudo + # Model: pinned to gpt-5.5 (OpenAI's newest frontier model and the + # officially-recommended Codex model per + # https://developers.openai.com/codex/models — "codex -m gpt-5.5"). + # The agentic `-codex` line (gpt-5.3-codex etc.) was folded into the + # general frontier model for Codex use, so gpt-5.5 supersedes the + # prior gpt-5.3-codex pin. gpt-5.5 is unsupported on a ChatGPT-account + # Codex login but fine via the OPENAI_CODEX_REVIEW_KEY API key. + # + # Inheriting the CLI default (empty string) tracks "OpenAI's + # recommended model" but lags by months — we were stuck on + # gpt-5-codex until 2026-05-27 because the action's default + # didn't auto-upgrade past v1.8's defaults. Explicit pin + # gives us deterministic review behavior + lets us bump + # deliberately when 5.4 / 5.5 / spark variants stabilize. + # Bump together with the codex-action SHA above when both move. + # Single source of truth: the workflow-level `env.CODEX_REVIEW_MODEL` + # (default gpt-5.5, overridable via the `CODEX_REVIEW_MODEL` Actions + # variable). Change it there — NOT here — to upgrade the model. + # 2026-06-21: bumped gpt-5.3-codex → gpt-5.5 (matches wfr-finley; + # gpt-5.5 is OpenAI's recommended Codex model, supersedes the + # agentic -codex line). + model: ${{ env.CODEX_REVIEW_MODEL }} + # effort: xhigh — maximum reasoning budget for the most thorough + # review (FIN-692). medium was calibrated for interactive coding + # speed, not exhaustive bug-hunting; OpenAI's own Codex /review was + # hardcoded low and raising effort yielded "much more comprehensive + # reviews", and their code-review skill itself runs xhigh. This is + # the single biggest recall lever (FIN-691). Depth is controlled by + # effort here, NOT by swapping the model — the model pin lives in + # CODEX_REVIEW_MODEL above. Cost/latency rise: see the header block. + effort: xhigh + allow-bots: "false" + + - name: Upload raw JSON as artifact + # `hashFiles()` resolves paths relative to GITHUB_WORKSPACE, so a + # `${{ runner.temp }}/codex-review.json` check would always return '' + # even when the file exists (verified empirically on PR #190 first + # success run — file was present for the render step but artifact + # upload skipped). `if-no-files-found: warn` on the action handles + # the missing-file case directly without needing a step-level guard. + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: codex-review-${{ github.event.pull_request.head.sha }} + path: ${{ runner.temp }}/codex-review.json + retention-days: 30 + if-no-files-found: warn + + - name: Render review and post PR comment + # Skip on job-level OR step-level cancellation. `!cancelled()` + # covers job cancellation (concurrency.cancel-in-progress); the + # step-outcome check covers any future per-step `timeout-minutes`. + if: ${{ !cancelled() && steps.codex.outcome != 'cancelled' }} + env: + # GH_TOKEN is the only secret in scope for `gh`. Untrusted + # PR-derived strings are routed through env: variables and + # passed to `gh` via file (`--body-file`) or jq -r (which + # treats input as data, not code). + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + REVIEW_FILE: ${{ runner.temp }}/codex-review.json + CODEX_OUTCOME: ${{ steps.codex.outcome }} + run: | + set -euo pipefail + + # Composition strategy: heredocs throughout to keep the + # markdown readable inline. Unquoted heredocs (`</dev/null 2>&1; then + : # fall through to the success branch below + elif [[ -s "$REVIEW_FILE" ]]; then + # --- Branch: codex produced output, but it failed JSON parsing --- + cat > "$BODY_FILE" <Failure of this optional review is non-blocking and does not gate merging. + ${MARKER_CLOSE} + EOF + else + # --- Branch: codex failed entirely (no output file) --- + cat > "$BODY_FILE" <Failure of this optional review is non-blocking and does not gate merging. + ${MARKER_CLOSE} + EOF + fi + + # --- Branch: success — render JSON to markdown --- + if [[ -s "$REVIEW_FILE" ]] && jq empty "$REVIEW_FILE" >/dev/null 2>&1; then + # Size guard: GitHub's issue-comment body is hard-capped at + # 65 536 characters. The schema allows up to 30 findings, + # each with a fix_diff up to 4KB — pathologically large + # reviews (mass-style refactor PRs, big schema-design PRs) + # can blow past the cap when the rendered findings + the + # raw-JSON copy are concatenated. Trim fix_diff to 600 + # chars per finding when the raw JSON crosses 24KB; full + # diff stays in the workflow artifact. + if [[ $(wc -c < "$REVIEW_FILE") -gt 24000 ]]; then + RENDER_FILE="${RUNNER_TEMP}/codex-review-trimmed.json" + jq ' + .findings |= map( + if (.fix_diff | length) > 600 + then .fix_diff = (.fix_diff[0:600] + "\n...") + else . + end + ) + ' "$REVIEW_FILE" > "$RENDER_FILE" + fi + + # jq template renders the structured JSON into Claude + # Code-parseable markdown. Stable headings + per-finding + # blocks. Severity emojis are display-only; the machine- + # readable severity lives in the embedded JSON block. + RENDERED=$(jq -r ' + def sev_emoji(s): + if s == "BLOCKER" then "🔴" + elif s == "SHOULD_FIX" then "🟡" + else "🟢" + end; + def verdict_line(v): + if v == "APPROVE" then "✅ **APPROVE**" + elif v == "REQUEST_CHANGES" then "🛑 **REQUEST CHANGES**" + else "💬 **COMMENT**" + end; + "## " + verdict_line(.verdict) + "\n\n" + + "**Summary:** " + .summary + "\n\n" + + "**Diff stats:** " + (.stats.files_changed|tostring) + " file(s) · " + + "+" + (.stats.lines_added|tostring) + "/-" + (.stats.lines_removed|tostring) + " lines · " + + (.stats.new_functions|tostring) + " new fn · " + + (.stats.new_tests|tostring) + " new test(s)\n\n" + + (if (.findings | length) == 0 + then "## Findings\n\n_None._\n\n" + else "## Findings (" + (.findings|length|tostring) + ")\n\n" + + ([.findings[] | + "### " + .id + " " + sev_emoji(.severity) + " `[" + .severity + "]` · `" + + .file + ":" + (.line_start|tostring) + + (if .line_end and .line_end != .line_start then "-" + (.line_end|tostring) else "" end) + + "` · tier " + (.tier|tostring) + " · confidence " + + (.confidence * 100 | floor / 100 | tostring) + "\n\n" + + "**Issue:** " + .issue + "\n\n" + + "**Why:** " + .why + "\n\n" + + (if .fix_diff and .fix_diff != "" + then "**Suggested fix:**\n```diff\n" + .fix_diff + "\n```\n\n" + else "_(structural change — no patch)_\n\n" + end) + + (if .refs and (.refs | length) > 0 + then "**Related:** " + (.refs | join(", ")) + "\n\n" + else "" + end) + + (if .tags and (.tags | length) > 0 + then "**Tags:** " + (.tags | map("`" + . + "`") | join(" ")) + "\n\n" + else "" + end) + ] | join("---\n\n")) + end) + + "## Test coverage — " + .test_coverage.verdict + "\n\n" + + .test_coverage.note + "\n\n" + + "## PR description vs implementation — " + + (if .description_vs_implementation.consistent then "consistent ✅" else "discrepancy ⚠️" end) + "\n\n" + + .description_vs_implementation.note + "\n\n" + + (if .what_works_well and (.what_works_well | length) > 0 + then "## What works well\n\n" + + ([.what_works_well[] | "- " + .] | join("\n")) + "\n\n" + else "" + end) + + (if .notes_for_claude_code and (.notes_for_claude_code | length) > 0 + then "## Notes for Claude Code\n\n" + + ([.notes_for_claude_code[] | "- " + .] | join("\n")) + "\n\n" + else "" + end) + ' "$RENDER_FILE") + + # Compose success-branch comment body: marker + header + jq + # render + raw-JSON block (between sentinels for Claude Code + # parsers) + marker close. One unquoted heredoc with backtick + # escapes (\`), then cat the raw JSON, then a second heredoc + # for the trailing fence + close. + # + # 4-backtick fence (Finley advisory 4 from PR #190 review): + # the embedded JSON's `fix_diff` strings can legally contain + # literal triple-backticks (suggesting markdown fixes, etc). + # A 3-backtick outer fence would close prematurely at the + # first inner ``` and break GitHub's markdown rendering of + # the
block. 4-backtick outer fence cannot collide + # with 3-backtick inner content because GitHub markdown + # treats the LONGER fence as the outer boundary. The + # consumer-side bridge regex (`tool-bridge.ts` + # extractCodexReviewFromComments) accepts both 3- and + # 4-backtick fences during the rollout window — once all + # in-flight comments have re-rendered with 4-backticks, the + # 3-backtick branch can be retired. + { + cat <**Model:** \`${CODEX_REVIEW_MODEL}\` · **Sandbox:** \`read-only\` · **Reviewing:** \`${SHORT_SHA}\` · Findings carry stable \`[BLOCKER|SHOULD_FIX|NIT]\` tags + \`file:line\` refs + confidence scalars for Claude Code consumption. Raw JSON below + as workflow [artifact](${RUN_URL}). + + ${RENDERED} + + --- + +
Raw JSON (for Claude Code parsers) + + ${JSON_OPEN} + \`\`\`\`json + EOF + cat "$RENDER_FILE" + cat < + ${MARKER_CLOSE} + EOF + } > "$BODY_FILE" + + # Hard backstop (layer 2 — FIN-695 review F1): the field-trims + # above shrink the embedded JSON but cannot bound the pathological + # max-shape review — the
raw-JSON block duplicates every + # field, so a 30-finding review can still exceed GitHub's + # 65 536-byte comment cap (verified by simulation: ~73KB even after + # trimming). The RENDERED markdown alone is usually well under the + # cap, so when the composed body crosses a conservative 64 000-byte + # threshold, re-compose WITHOUT the embedded JSON block — it is + # always preserved in the workflow artifact. Since the FIN-695 + # why-cap raise to 700, a pathological max-shape review can render + # past 64KB even without the JSON, so the layer-3 stub below is the + # final guarantee. On this rare path the downstream bridge reads + # the JSON from the artifact, not the comment. + if [[ $(wc -c < "$BODY_FILE") -gt 64000 ]]; then + cat > "$BODY_FILE" <**Model:** \`${CODEX_REVIEW_MODEL}\` · **Sandbox:** \`read-only\` · **Reviewing:** \`${SHORT_SHA}\` · Raw JSON omitted from this comment (the rendered review exceeded GitHub's 65 536-byte cap) — the full structured review is in the workflow [artifact](${RUN_URL}). + + ${RENDERED} + ${MARKER_CLOSE} + EOF + fi + + # Hard backstop (layer 3 — FIN-698): even the rendered-only body + # (layer 2, no embedded JSON) can exceed GitHub's 65 536-byte cap + # at the schema ceiling now that `why` allows up to 700 chars + # (FIN-695) — 30 findings with long why/issue/verification_command + # render past 64KB on their own. When that happens, post a minimal + # stub: verdict + finding count + a pointer to the workflow artifact + # (which always holds the full structured review and which the + # Finley arbitration bridge reads). Guarantees the comment posts. + if [[ $(wc -c < "$BODY_FILE") -gt 64000 ]]; then + STUB_VERDICT=$(jq -r '.verdict' "$REVIEW_FILE") + STUB_COUNT=$(jq -r '.findings | length' "$REVIEW_FILE") + cat > "$BODY_FILE" <**Model:** \`${CODEX_REVIEW_MODEL}\` · **Sandbox:** \`read-only\` · **Reviewing:** \`${SHORT_SHA}\`. + + **Verdict: ${STUB_VERDICT}** · ${STUB_COUNT} finding(s). This review was too large to render inline — even without the embedded JSON it exceeded GitHub's 65 536-byte comment cap. The full structured review (every finding with \`file:line\`, severity, confidence, and fix) is in the workflow [artifact](${RUN_URL}) and the check output. + ${MARKER_CLOSE} + EOF + fi + fi + + # Find an existing Codex review comment on this PR (marker-keyed) + # and update in place — keeps PR conversation clean across re-pushes. + # `--paginate` is load-bearing: `gh api` defaults to per_page=30, + # so on any PR with >30 issue comments the marker comment lands + # on page 2+ and a non-paginated lookup misses it, causing the + # next branch to post a duplicate Codex comment instead of + # updating in place. Streaming jq (`.[] | select | .id` + `head`) + # composes cleanly across paginated responses; the previous + # `[.[] | ...] | first // empty` form does not. + # Caught by Finley's review of this PR (FIN-269). + EXISTING_ID=$(gh api --paginate \ + "repos/${REPO}/issues/${PR_NUMBER}/comments" \ + --jq '.[] | select(.user.login == "github-actions[bot]") | select(.body | test("^\\s*")) | .id' \ + | tail -n 1) + + if [[ -n "$EXISTING_ID" ]]; then + echo "Updating existing Codex comment id=${EXISTING_ID}" + gh api --method PATCH \ + "repos/${REPO}/issues/comments/${EXISTING_ID}" \ + --field body=@"$BODY_FILE" + else + echo "Posting new Codex review comment" + gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file "$BODY_FILE" + fi