Codex Review Guidelines

August 17, 2026 · View on GitHub

Project Overview

Meta-repository — SDLC Harness documentation, automation, and a zero-dep Node.js CLI (cli/). Primary codebase is bash scripts + YAML workflows. The CLI distributes hooks, skills, and settings via npx agentic-sdlc-wizard init.

  • CLAUDE_CODE_SDLC_WIZARD.md — The main wizard document
  • .github/workflows/ — CI, PR review, weekly/monthly automation
  • .claude/hooks/ — SDLC enforcement hooks (fire every interaction, ~100 tokens each)
  • .claude/skills/ — Detailed guidance invoked by Claude (sdlc, setup)
  • tests/ — Bash test scripts (Layer 1 logic + Layer 5 E2E)
  • cli/ — npx distribution CLI (zero-dep Node.js)

AI Setup Lanes

This repo recommends four setup lanes — Setup A (Opus 5 + Fable advisor, GPT-5.6 Sol high reviewer, recommended default, trial as of 2026-07-24), Setup B (Sonnet 5 Simple/One-Off: Fable advisor, Sonnet 5 medium driver, GPT-5.6 Sol high reviewer), Setup C (OpusPlan Hybrid: Opus 5 plan mode + Sonnet 5 execute, GPT-5.6 Sol high reviewer), and Setup D (Claude Lite: Sonnet driver, no reviewer). See AI_SETUP_LANES.md for the full pick list.

The lanes are guidance, not a hard rule — maintainer override is always allowed.

Review Focus Areas

1. SDLC Compliance

  • Does the change follow SDLC principles (plan, test, review)?
  • Is there evidence of planning for complex changes?
  • Are tests included or updated?

2. Security

  • Shell injection in bash scripts (unquoted variables, eval, backtick expansion)
  • YAML injection in workflow files (untrusted ${{ }} in run: blocks)
  • Secrets exposure (API keys, tokens in logs or comments)
  • Unsafe variable interpolation (use env: blocks for LLM-generated content)

3. Code Quality

  • Simple and readable?
  • Over-engineered? (KISS principle — this project deletes legacy code aggressively)
  • Follows existing patterns? (check similar files before suggesting new approaches)

4. Testing

  • New features tested?
  • Tests are meaningful (not just for coverage)?
  • Testing diamond: integration > unit with mocks
  • Test scripts use set -e, pass()/fail() helpers, exit 1 on failure

5. Blast Radius — know what SHIPS before weighting a finding

package.json's files lists: cli/, skills/, hooks/, .claude-plugin/, CLAUDE_CODE_SDLC_WIZARD.md, AI_SETUP_LANES.md, CHANGELOG.md.

files is NOT the whole answer — npm always includes README.md and package.json regardless. Verify with npm pack --dry-run rather than reading files; that is the authority. This exact omission caused a real miss: stale effort guidance in README.md was treated as repo-local and shipped anyway.

Ships to every consumer — weight findings here highest:

  • hooks/ → SDLC enforcement in every installed repo. A silently-inert hook here is a P0-class defect: it looks installed and does nothing.
  • skills/ → SDLC guidance everywhere. Must stay byte-identical to cowork/skills/ (see tests/test-cowork-drift.sh). There is no byte ceiling — do not flag one. The 20,000-byte build gate was deleted in #489 because the number was never measured (THRESHOLD_TOKENS 5,000 × a chars-per-token rule of thumb from another vendor, and env-tunable). skills/sdlc/SKILL.md has been well over 40,000 bytes since #489 — deliberately, and no exact figure is quoted here because it changes with every edit and a stale number is the defect this section exists to remove. The audit still reports sizes and still raises TRIM; that is observability, not a gate, and a TRIM line is not a finding.
  • cli/ → the installer. A defect here breaks setup for new users.
  • CLAUDE_CODE_SDLC_WIZARD.md, AI_SETUP_LANES.md, README.md → the shipped guidance consumers act on. README ships despite its absence from files.

Repo-local, ships to nobody — real, but lower stakes:

  • .claude/hooks/, .claude/skills/, .claude/settings.json, scripts/, tests/, ROADMAP.md, AGENTS.md

An earlier version of this section listed only the .claude/ paths and omitted the shipping ones entirely, which inverted the priority. Do not restore that.

If a change affects SDLC behavior, check whether a relevant scenario exists in tests/e2e/scenarios/.

6. Merge evidence — tiers, and what each one actually proves

A clearance is not a mood. scripts/merge-pr.sh accepts specific evidence, and the tiers are not interchangeable. Rank findings about them accordingly.

tierwhat it requireswhat it proves
dual-certified2 distinct reviewers at ≥95%, each bound to the head SHAthe strongest evidence available here
user-approveda maintainer reason stringzero independent reviewers. Honest, visible degradation — never equivalent to the above

Three failure modes worth flagging on sight:

  • A verdict held in a session is not evidence. If a clearance was never posted, the gate cannot see it. Reaching for --user-approved when a real dual clearance exists but went unposted launders a strong result into a weak one.
  • Clearance binds to content, not to a branch. A certification whose candidate_tree does not match the index tree is stale, and a re-review is owed the moment the tree moves. Flag any flow that re-uses a clearance across a tree change.
  • The gate is identity-blind. It accepts any two distinct reviewer strings — it does not verify provider, model, effort, or mode. So "two reviewers agreed" is a weaker claim than it reads as. Do not treat a passing gate as proof that two independent models actually looked.

7. Unbounded reads and silent child failures

This class does not announce itself: nothing errors, and the run simply produces nothing. All four of these were observed in this repo, not theorized.

  • Buffered output hides a hang. --output-format json buffers to completion, so a slow child and a dead one look identical. Observed: zero bytes for ten minutes before a kill. stream-json showed live activity within seconds. Flag any long-running child whose output cannot be observed while it runs.
  • A process with no owner has no fate. A hand-rolled review leg died with zero-byte output, no status, and no process — nobody noticed until someone went looking. Any spawned child needs an owner that propagates its exit status; that is what scripts/run-review-leg.sh exists for, and why bare codex exec is refused by hooks/codex-gate-check.sh.
  • Inherited stdin hangs forever. A child that inherits a terminal stdin and then reads from it blocks with no output and no timeout. Give children /dev/null unless they genuinely need input.
  • Children inherit more than you passed them. Observed: a nominally-blind reviewer child called advisor() and consulted Fable, despite --safe-mode --strict-mcp-config --no-chrome and a four-tool allowlist — it inherited advisorModel from user settings. Flag any claim of isolation that rests on flags alone rather than on an observed transcript.

Code Review Rules

Durable standards for codex review. Codex loads this section automatically — everything above applies too, this is what has bitten us repeatedly and is worth stating as rules.

  1. A test that greps for text is not a test of behavior. This repo has shipped guards that assert on a script's source rather than executing it. Across one 2026-08 session, reviewers found nine assertions passing against broken code, a rule that accepted 1,2,1,2 silently, and a guard that read nothing from the file it guarded while all 15 of its fixture assertions stayed green. Flag any new assertion that greps where it could execute.

  2. Fixtures prove a rule works on documents shaped like the fixtures. Require at least one assertion against the real artifact — mutate it and confirm the rule reports it. Absence of that canary is what let the vacuous guard above survive three review rounds.

  3. Prefer positive anchors to denylists. Asserting "the defining line says X" is winnable. Asserting "no bad phrasing appears anywhere" is not — the synonym space is unbounded, and three rounds of adding alternations each ended with a reviewer naming another. Flag new denylists that guard an open-ended vocabulary.

  4. A guard must not be able to damage what it protects. One cleanup guard deleted a real artifact while testing that real artifacts are not deleted. Prefer a temp directory over narrowing the blast radius of an operation on live paths.

  5. Docs are code. .md changes break tests, and shipped docs are the product. A claim in shipped guidance about a mechanism that does not exist is a defect, not a typo — verify cited scripts, flags, and env vars actually exist.

  6. No unbacked recommendations. If a shipped doc states a default, an effort level, or a performance claim, it needs a source or an explicit "not measured." This repo has shipped confident claims traced to commits with zero supporting measurement.

  7. Match effort to blast radius. Do not demand release-gate rigor on a repo-local test fixture, and do not wave through a one-word change to hooks/.

Review Exceptions

Read CODE_REVIEW_EXCEPTIONS.md before flagging findings. If your finding matches a documented exception, skip it — it has already been evaluated and explicitly accepted.

Severity

  • P0 (Critical): Security vulnerabilities, data loss, CI breakage, silent failures
  • P1 (Must fix): Logic bugs, missing tests for new behavior, broken E2E coverage
  • P2 (Suggestion): Style, readability, minor improvements

Meta-Repo Awareness

  • Docs ARE code — changes to .md files can break tests (tests validate doc content)
  • .github/workflows/ is the most execution-critical path
  • continue-on-error: true and || echo "fallback" patterns mask real failures — always flag these
  • ${{ }} in bash run: blocks with LLM/user content → command injection risk (use env: block instead)
  • macOS ships bash 3.x — no declare -A, no head -n -1