Skip to content

ci: add Codex PR auto-review (homelab fleet rollout) #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chrisbaker2000 merged 1 commit into from
Jun 23, 2026
Merged

ci: add Codex PR auto-review (homelab fleet rollout) #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions .github/codex/rubric.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
187 changes: 187 additions & 0 deletions .github/codex/rubric.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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."
}
}
}
Loading
Loading