Ooda Pr Codex Review

/ooda-pr-codex-review

Drives one PR through observe → orient → decide → act — optionally running local codex review as a sixth orient axis on the same OODA tick. Each invocation returns one Outcome; the caller dispatches on the exit code alone.

When --codex-review-ceiling off (the default) the codex axis is inert and behavior is bit-equivalent to /ooda-pr. When enabled, codex review's per-level batches stream alongside the PR loop's existing waits, contribute candidates into the same Urgency ladder, and structurally gate merge (no approval / merge while the codex ladder has unresolved levels). The recorder shares the PR state-root with /ooda-pr, so running either skill on the same PR walks the same per-PR ledger.

Names

Always invoke run; never the binary directly. run performs the rebuild step (cargo build --release --quiet) before exec'ing the binary, so the binary path is fresh whenever run last completed. Invoking the binary directly skips the rebuild and may serve a stale build relative to the current source tree.

Type spine

Boundary types are defined in the ooda-core library crate (/home/cory/code/skills/ooda-core/) and shared with the three sibling OODA binaries. This binary depends on ooda-core via path dep and instantiates each generic type over its domain-specific ActionKind enum — the merged PR + codex-review variant set:

pub type Outcome      = ooda_core::Outcome<ActionKind>;
pub type Decision     = ooda_core::Decision<ActionKind>;
pub type DecisionHalt = ooda_core::DecisionHalt<ActionKind>;
pub type HaltReason   = ooda_core::HaltReason<ActionKind>;
pub type Action       = ooda_core::Action<ActionKind>;

Automation, Urgency, TargetEffect, BlockerKey, Terminal, and the ActionKindName trait are re-exported from ooda-core. ActionKind is per-binary — it carries the 22 PR-merge variants (FixCi, AddressThreads, Rebase, …) plus the three codex-review variants (RunCodexReviewBatch, AwaitCodexReviewBatch, AddressCodexReviewBatch) and implements ActionKindName.

Variant name ≠ stderr header. Rust variant names (DoneSucceeded, DoneAborted, Paused) are internal — the neutral verbs that fit every binary in the family. Stderr header strings (DoneMerged, DoneClosed, Paused) are this binary's caller contract, emitted by the per-binary render_outcome function. The Outcomes table below shows both columns.

Per-binary code (not lifted): runner.rs::run_loop (carries the codex-axis flock acquisition + head-SHA refresh, distinct from the three sibling runners), recorder.rs, decide/action.rs::ActionKind and its ActionKindName impl, the codex-axis observe / orient / decide sub-trees, and From<LoopError> for Outcome (this binary's LoopError carries a CodexObserve variant in addition to the PR-side variants).

See ooda-core/README.md and ooda-core/src/lib.rs for the shared-spine design rationale.

Calling discipline

$? MUST reflect ooda-pr's exit when ooda-pr runs. Two distinct concerns:

1. ooda-pr must actually run. false && ooda-pr ... short-circuits and ooda-pr never executes; $? will reflect the left side's exit, not ooda-pr's. Structure invocations so ooda-pr runs unconditionally.
2. Nothing may inject another exit code into $?. Pipes (ooda-pr | foo), stderr-merging pipes (ooda-pr |&, ooda-pr 2>&1 | foo), backgrounding (ooda-pr &), command substitution (out=$(ooda-pr ...)), and any subsequent command (ooda-pr; echo x) replace $? with another process's exit.

Safe patterns:

• ooda-pr alone on the line: ~/.claude/skills/ooda-pr-codex-review/run owner/repo 42
• ooda-pr as the last command after ;: pwd; ~/.claude/skills/ooda-pr-codex-review/run owner/repo 42
• ooda-pr on the right of && after a reliably-succeeding left side: cd /tmp && ~/.claude/skills/ooda-pr-codex-review/run owner/repo 42

Capturing stderr: redirect to a file (ooda-pr ... 2>file) for a single invocation. For repeated invocations, use append redirection (2>>file) or a fresh file per run. File redirection does not affect $?. Process substitution (ooda-pr 2> >(tee file)) also preserves $?. The forms that break $? are stderr piping (|&, 2>&1 |). The durable audit trail is always written under the state root.

Separate Bash tool calls in the same agent turn are fine — each Bash call is an independent shell with its own $?.

Driving discipline

Loop mode is meant to run to a halt. The only correct stopping points are the halt-class exit codes: 0 (DoneMerged), 1 (Paused), 3 (HandoffHuman), 4 (HandoffAgent), 5 (DoneClosed), 6 (StuckRepeated), 7 (StuckCapReached), and 70 (BinaryError). Stopping anywhere else — including after exit 2 (WouldAdvance) — is premature.

Anti-patterns that stop the loop early. These are common agent mistakes; the binary is not at fault.

• Probing repeatedly with inspect. Inspect is a one-shot snapshot. After the first inspect (or as the very first call on an unfamiliar PR), drop inspect and re-invoke as the loop. Re-inspect-ing in place of running the loop is the most common way agents stall a PR.
• Treating WouldAdvance as a halt. Exit 2 is inspect-only. The action shown is what the loop would do; re-invoke without inspect to actually do it. Do not report WouldAdvance to the user and stop.
• Re-inspect-ing after a Handoff* action completes. The action's effect needs to land in fresh observation, which the next loop iteration will do — not the next inspect. After a Handoff returns and you complete the requested action, re-invoke in loop mode (no inspect).
• Shrinking --max-iter as a "safety" cap. The default (50) exists because Wait iterations (15s/30s/60s) are how the loop polls slow external systems (CI runs, bot reviews, scheduled jobs). Capping at 3, 5, or 10 routinely converts a normal wait into a spurious StuckCapReached (exit 7). Use the default unless you have a specific reason; if exit 7 fires from a wait-heavy run, re-invoke with a higher cap, not a lower one.
• Inferring "stuck" from long wait runs. A run that spends minutes in Wait(1m) polling for a CI check or a bot review is working correctly. The wait is the action. Let it finish.

After a Handoff* (exit 3 or 4). Surface the handoff to the user (header + iter-log + handoff blob; see Handoff* prompt format → "Surface to the user"). Then perform the requested action and re-invoke /ooda-pr-codex-review in loop mode (no inspect). The loop's first iteration re-observes the now-modified state and either selects the next action or halts.

Time budget. ooda-pr is iteration-bounded, not wall-clock-bounded. A loop run can legitimately take 30+ minutes if external systems are slow. Plan for that; don't artificially cut it short. If you genuinely need a wall-clock deadline, impose it externally — but expect that doing so will produce spurious StuckCapReached results, not faster convergence.

How to call

~/.claude/skills/ooda-pr-codex-review/run [options] <owner/repo> <pr>           # loop mode
~/.claude/skills/ooda-pr-codex-review/run inspect [options] <owner/repo> <pr>   # one pass

Argument rules:

• <owner/repo> and <pr> are required, in that order.
• Flags may interleave between or after the positionals.
• inspect, when present, must come before either positional. Flags may appear before inspect. The parser consumes the first inspect token (when no positional has yet been seen) as the mode subcommand; any later inspect token falls through to the positional vector. The resulting UsageError text depends on the positional vector that builds: "invalid pull request number: not a number: inspect" when a subsequent inspect lands in the <pr> slot (e.g. inspect owner/repo inspect), "invalid repo slug: missing '/'" when a duplicate inspect becomes positional[0] before the slug (e.g. inspect inspect 99 — the second inspect lands in the slug slot), or "expected exactly 2 positionals (owner/repo, pr); got " when the total positional count ends up ≠ 2 (e.g. owner/repo inspect 99 produces 3 positionals).
• -h / --help short-circuits all other validation via a pre-scan: if either token appears anywhere in the argument list, usage is printed to stdout and the process exits 0 before any other flag is parsed.
• Repeating --max-iter or --status-comment is a UsageError. Repeating --state-root is also a UsageError.

The run script rebuilds the release binary on demand and execs it. The wrapper invokes cargo build --release --quiet, so successful incremental rebuilds are silent; only warnings and errors reach stderr from cargo. Stderr emitted before ooda-pr starts (i.e. before any line documented in "Stderr surface" below) is wrapper / cargo diagnostic noise, not part of the binary's contract. The binary's own per-iteration logs, stack note, comment status lines, and final variant block are the contract — see "Stderr surface" for the full inventory. If the cargo build fails, run exits with cargo's exit code (typically 101 for compile error) and ooda-pr does not execute — treat such codes as BinaryError-equivalent (see catch-all).

Codex review axis

When --codex-review-ceiling != off, observe gains a sixth axis that scans the local filesystem for in-flight codex review batches. Orient projects this into a CodexReviewReport whose status is one of:

current_level is derived: walk floor → ceiling, find the first level that isn't Complete { all-clean } for the current head SHA. This makes ladder climbing implicit — no per-level "advance" action is needed because a clean batch at level N means orient picks level N+1 next iteration.

New ActionKind variants

Critical urgency on RunCodexReviewBatch preempts everything else — when the codex axis has work to spawn, it goes first. BlockingWait on AwaitCodexReviewBatch competes with WaitForCi/WaitForCopilotReview etc., so the two pipelines' waits naturally serialize through the same Urgency sort. BlockingFix on AddressCodexReviewBatch competes with AddressThreads / FixCi — codex issues get the same priority class as PR review thread issues.

Head-SHA-keyed batch directories

Each batch lives under <workspace>/codex/levels/<L>/<head_sha[:12]>/, stamped with head_sha.txt. When a fix-agent pushes a commit and the next iteration observes a new head_ref_oid, prior batches survive on disk as cache but are ignored by orient (different short-SHA → no matching batch dir → BatchState::NotStarted → fresh spawn at the same level). This is the entire mechanism for "stale codex verdicts after a push".

Codex axis workspace

Codex subprocesses spawn into a PR-keyed workspace under the state root, separate from the run-opaque runs/ tree:

<state-root>/workspaces/pr-codex-review/<owner>/<repo>/<pr>/codex/
  .lock                        advisory flock; held for the run's lifetime
  levels/<L>/<head_sha[:12]>/
    head_sha.txt               stamped on spawn; gates scan_batch
    <L>-1.log                  stdout/stderr of codex review subprocess
    <L>-1.exit                 exit status when child finished
    <L>-2.log
    <L>-2.exit
    ...

The workspace is cross-run by design (cache survives commits + PR re-invocations); the run-opaque core does not know about it. The recorder records observations of this workspace as DomainSpecific events in the run's events.jsonl.

Concurrency

The codex axis acquires an advisory flock(2) on <workspace>/codex/.lock at startup (when ceiling != off). Concurrent ooda-pr-codex-review runs against the same PR with codex enabled return BinaryError(WouldBlock) rather than racing on batch dirs / head_sha.txt. The lock is FD-tied and releases on process exit (including SIGKILL — the kernel closes the FD). Stale .lock files from crashed processes never block subsequent runs. Per-run events.jsonl files live under distinct run-id directories, so concurrent runs never collide on them.

Resolving codex spawn errors

Codex subprocess failures classify as BinaryError (exit 70):

The orchestrator should treat exit 70 from a codex-enabled invocation as triage-only: do not auto-retry; inspect the <L>-<slot>.log referenced in the BinaryError message.

Always-On State

Every invocation with valid <owner/repo> and <pr> writes durable state via the domain-neutral ooda-state model. The default root resolves in order:

1. --state-root PATH
2. $OODA_STATE_HOME
3. $XDG_STATE_HOME/ooda
4. ~/.local/state/ooda
5. the platform temp directory

The state root is shared across every OODA agent on the machine; the on-disk path carries no domain identity. PR identity (forge, slug, number, codex-axis config) lives only inside the run's RunStarted event payload.

<state-root>/
├── runs/<run-id>/
│   ├── events.jsonl              ← append-only typed event log
│   └── blobs/<sha>.<ext>         ← content-addressed per-run blobs
├── live/<run-id>                 ← empty marker; presence = "active"
└── workspaces/pr-codex-review/<owner>/<repo>/<pr>/
    ├── status-comment.json       ← cross-run dedup state
    └── codex/                    ← codex spawn workspace (see above)

Each run is fully opaque (timestamp + entropy + pid). events.jsonl holds typed records — run_started, iteration_observed, iteration_oriented, iteration_decided, iteration_executed, iteration_waited, iteration_handoff, plus domain_specific for domain-vocabulary events the typed schema does not model directly. Terminal events are run_halted / run_stalled / run_cap_reached; the live/<run-id> marker is deleted when one fires.

RunStarted.target carries the PR identity:

{
  "slug": "owner/repo",
  "pr": 42,
  "mode": "loop",
  "max_iter": 50,
  "status_comment": false,
  "codex_review": { "floor": "low", "ceiling": "high", "n": 3 }
}

codex_review is null when the axis is disabled.

Agent entrypoint: walk runs/ for the desired run-id (cockpit tails live/ for active runs; audit consumers sort runs/ by mtime). Open the run's events.jsonl and project. Per-iteration artifacts (normalized observations, oriented snapshot, candidates, decision, dashboard, handoff body, tool-call stdout/stderr) live as content-addressed blobs under the same run's blobs/; events reference them by BlobRef { sha, size, ext }.

Outcomes

Each successful invocation produces exactly one Outcome and emits it as the final stderr block (header + variant payload). Dispatch on $? alone — no stderr parsing required for dispatch. Handoff* callers additionally consume the prompt block (stderr content following the header), and UsageError callers may surface the usage text — but neither parses stderr to determine which variant fired; that's $?.

The --help short-circuit is an exception: it exits 0 without constructing an Outcome at all (stdout receives the usage text; the binary writes nothing to stderr on this path — though the run wrapper may have already emitted cargo warnings/errors per the wrapper-diagnostics caveat above).

Stderr surface. Stderr is divided into a diagnostic prefix (varies by mode and flags) and a final variant block (the Outcome's emission). Listed by emission site:

• Loop mode, per iteration (interleaved in iteration order):
  • [iter N] <ActionKind> (<Automation>) blocker: <BlockerKey> for Execute decisions — note the parentheses, distinct from the colon-separated WouldAdvance: <ActionKind>:<Automation> header form (a single regex over both surfaces will mis-parse). Example: [iter 3] WaitForCi (Wait(1m)) blocker: ci_pending: Build.
  • [iter N] halt: <DecisionHaltName> for halts with no action payload. For AgentNeeded / HumanNeeded halts, the line is [iter N] halt: <DecisionHaltName> blocker: <BlockerKey> (e.g. [iter 5] halt: AgentNeeded blocker: unresolved_threads). <DecisionHaltName> is one of a finite five-element set of strings (two of which contain parentheses, so paren-splitting tokenizers or \w+-style regexes will split them): Success, Terminal(Succeeded), Terminal(Aborted), AgentNeeded, HumanNeeded. Each maps to a boundary Outcome variant: Success → Paused (exit 1), Terminal(Succeeded) → DoneSucceeded (exit 0, stderr header DoneMerged), Terminal(Aborted) → DoneAborted (exit 5, stderr header DoneClosed), AgentNeeded → HandoffAgent (exit 4), HumanNeeded → HandoffHuman (exit 3). Payloads are not expanded in the iter-log line; the boundary emission carries them — Handoff* in a content-addressed blob under runs/<run-id>/blobs/<sha>.md (pointed to by the stderr see: line), Stuck* in the :<BlockerKey> projection, terminal/Paused with no payload.
  • When --status-comment is set: [iter N] comment: posted, [iter N] comment: <PostError>, or silently skipped on the common dedup-no-change case. (See "comment lines" below.)
• Inspect mode, before the variant block (at most one each, in this order):
  • stack: <base> → <root> if the PR's immediate base differs from the resolved stack root used for branch-rule lookups. Inspect-only by design: one-shot diagnostics get the stack note for context; loop mode does not emit it at all (it's static for a given PR, and per-iteration repetition would be noise).
  • When --status-comment is set: comment: posted, comment: skipped (unchanged), or comment: <PostError>.
• Final variant block (last emission, both modes): the Outcome header, optionally followed by a single pointer line see: <abs-path-to-handoff-blob> (Handoff*) or the usage block (UsageError). The path points at a content-addressed blob under runs/<run-id>/blobs/<sha>.md; the prompt body lives in that blob, not on stderr — see Handoff* prompt format below.

Diagnostic surfaces are recorded as domain_specific events (trace_line, observe_started, observe_finished, …) in the run's events.jsonl; stderr remains the binary boundary, and $? remains the dispatch contract.

Comment lines (when --status-comment is set):

Stderr placeholders:

• <ActionKind> — the action's variant name (e.g. Rebase, AddressThreads). Payload-stripped: WaitForBotReview, not WaitForBotReview { reviewers: [...] }. The renderer uses ActionKind::name(), a hand-written &'static str per variant.
• <BlockerKey> — the action's blocker identifier (defined below). The type enforces only non-empty. Construction sites interpolate typed payloads (CheckName, GitHubLogin, etc.) into format strings, so values can include any characters those types allow — typical values are ASCII-only with colons and spaces (e.g. ci_fail: Build / test), but unicode is possible if upstream payloads carry it. See the BlockerKey section for sample values and the consequences for parsing the <ActionKind>:<BlockerKey> projection.
• <Automation> — Full or Wait(<duration>). The renderer has arms for all 4 Automation variants, but decide routes Agent/Human to halts (HandoffAgent / HandoffHuman) before they could reach a WouldAdvance. Only Full/Wait(_) appear here in practice — invariant established at the decide boundary, not the render boundary. <duration> is rendered as <seconds>s (under 1 minute), <minutes>m (whole minutes), or <minutes>m<seconds>s for the mixed case. Current actions only construct intervals of 15s, 30s, or 60s — so the surface forms callers will actually see are Wait(15s), Wait(30s), and Wait(1m). The <minutes>m<seconds>s form (e.g. Wait(1m30s)) and Wait(0s) are representable by the formatter but no current action constructs them.

Header format. The stderr headers with no payload — exactly DoneMerged, DoneClosed, Paused (underlying variants DoneSucceeded, DoneAborted, Paused) — emit only the header token on the line (no colon, no trailing space). All other variants emit <Header>: <details> (colon and one ASCII space, then payload). A regex matching the header must allow both forms: ^(DoneMerged|DoneClosed|Paused)$ for the no-payload headers, ^<Header>: for the rest. There is no StuckCapReached: (bare-colon) form — StuckCapReached always carries an Action and always emits the <ActionKind>:<BlockerKey> payload.

1:1 variant-to-exit-code mapping is the design rule. Each variant has a unique exit code; $? is sufficient for dispatch.

Exit codes 8–63 and 65–69 are unassigned. The binary never emits them in current source. They are held in reserve for future Outcome variants (additions follow the assigned-table style: a new variant gets a new code in this range, no code is ever reused). Codes ≥64 follow BSD sysexits starting at UsageError = 64.

Payload conventions

Each variant carries exactly the evidence its caller needs:

• Stuck* carries the action whose (kind, blocker) pair triggered the halt. The <ActionKind>:<BlockerKey> projection on stderr is informational only — the action is the witness. StuckRepeated carries the repeated non-Wait action. StuckCapReached carries the last action act ran successfully (Wait or non-Wait) — the most recent triage anchor when the cap fires.
• HandoffAgent / HandoffHuman are spelled as separate variants (rather than Handoff(Recipient, Action)) so the recipient is observable from the variant name and exit code, preserving 1:1 dispatch.
• WouldAdvance(Action) carries the action; the action's automation field is rendered on stderr to tell the caller what act would do. No separate pace payload — it lives on the action.
• BinaryError(String) is intentionally opaque at the boundary. Loop mode flattens a typed loop-error union (observe failure, codex-observe failure, or act failure) into the string; inspect mode can only surface observe failures (no act call). Invariant: the string contains no newlines — any newline in the underlying error is replaced with a space at construction, so the stderr header always occupies exactly one line.
• Paused carries no payload. Paused means decide selected no candidate action — there is no action to carry. Diagnostic context for "why no candidate" lives in the orient log (surfaced via --status-comment), not in the Outcome.
• UsageError(String) carries the parser's diagnostic, also newline-free.

Catch-all (uncaught exit codes)

ooda-pr-codex-review deliberately produces only the exit codes assigned in the table: {0..=7, 64, 70, 130, 143}. Codes 8–63 and 65–69 are unassigned and never emitted by current source. Any exit code outside the assigned set indicates an uncaught binary failure: typical causes are Rust panics (101), OS signal kills the loop did not trap (128 + signal — e.g. 137 for SIGKILL/OOM, 139 for SIGSEGV), or run wrapper failures (cargo build error).

The caller should treat such codes as BinaryError-equivalent for dispatch (alert; do not interpret stderr as a structured contract — it is a panic message o

…(truncated)