Skip to content

feat: add CodexRunner for OpenAI Codex CLI backend - #1419

Open
abhi1092 wants to merge 5 commits into
akashgit:mainfrom
abhi1092:feat/codex-runner
Open

abhi1092 wants to merge 5 commits into
akashgit:mainfrom
abhi1092:feat/codex-runner

Conversation

@abhi1092

Copy link
Copy Markdown
Collaborator

Summary

  • Adds CodexRunner (factory/runners/codex.py) — a runner for the OpenAI Codex CLI (npm: @openai/codex), enabling the factory to use Codex as an alternative agent backend via FACTORY_RUNNER=codex or --runner codex
  • Registers it in factory/runners/__init__.py alongside ClaudeRunner and GlaudeRunner
  • Adds 17 tests covering metadata, command building, AGENTS.md handling, usage parsing, and registration

Key design decisions:

  • System prompt is written to AGENTS.md in the project directory (Codex has no --append-system-prompt-file flag). Existing AGENTS.md files are preserved; factory-created ones are cleaned up after the run.
  • Headless mode uses codex exec --json --ask-for-approval never "<task>"
  • Usage parsing handles both OpenAI-style (prompt_tokens/completion_tokens) and standard (input_tokens/output_tokens) field names
  • No session management support (Codex CLI doesn't support --name/--resume/--session-id)

Usage:

factory ceo /path/to/project --runner codex
# or
FACTORY_RUNNER=codex factory ceo /path/to/project
# or in ~/.factory/config.toml
[credentials.codex]
FACTORY_RUNNER = "codex"
OPENAI_API_KEY = "sk-..."

Test plan

  • All 17 new tests pass (pytest tests/test_codex_runner.py -v)
  • All 24 existing runner tests pass (pytest tests/test_runner.py -v)
  • Lint clean (ruff check)
  • Manual test with codex CLI installed and OPENAI_API_KEY set

🤖 Generated with Claude Code

abhi1092 and others added 5 commits August 31, 2026 11:16
Adds a runner for the OpenAI Codex CLI (npm: @openai/codex), enabling
the factory to use Codex as an alternative agent backend via
FACTORY_RUNNER=codex or --runner codex.

Key differences from ClaudeRunner:
- Headless via `codex exec --json --ask-for-approval never "<task>"`
- System prompt injected via AGENTS.md in project dir (no CLI flag)
- Auth via OPENAI_API_KEY
- No session management (name/resume/session-id)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…enticated

Matches ClaudeRunner's approach: the factory assumes the CLI binary is
installed and authenticated, rather than checking for specific env vars.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ty REPL

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use --dangerously-bypass-approvals-and-sandbox for headless (codex exec)
- Use --ask-for-approval never for interactive mode
- Use -C instead of --cd for working directory
- Strip Claude-specific model aliases (sonnet/opus/haiku/claude-*)
  so Codex uses its own default model
- Respect request.skip_permissions instead of always bypassing

Verified with live smoke test against codex-cli 0.151.0.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The AGENTS.md approach had two problems:
- If the project already has an AGENTS.md, the factory's agent prompt
  (researcher.md, builder.md, etc.) was silently dropped
- AGENTS.md is designed for persistent project instructions, not
  per-invocation role prompts

Now combines the system prompt and task into a single positional arg
passed to `codex exec`, which is the reliable way to deliver both
since Codex has no --append-system-prompt-file equivalent.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@RohanAwhad RohanAwhad added the needs_changes Code review found issues requiring changes label Sep 2, 2026
@RohanAwhad

Copy link
Copy Markdown
Collaborator

Code Review

Decision: needs_changes

Scope: Reviewed 5 unique commits (all author-original, no upstream sync). Target branch: main. True PR diff (against merge-base 081f50a5): 3 files, +395/-0factory/runners/codex.py (new, 207 lines), factory/runners/__init__.py (+3 registration), tests/test_codex_runner.py (new, 185 lines).

Intent & Optimality (pr-intent-review)

Understood intent: Add CodexRunner, a thin adapter that lets the factory drive the OpenAI Codex CLI as an alternative agent backend (via --runner codex / FACTORY_RUNNER=codex), plus registration and unit tests.

Optimality: mostly optimal — clean, purely additive, correctly conforms to the Runner Protocol (name, metadata(), build_command, async headless, interactive_run all present with correct signatures). Registration in _RUNNERS, __all__, and entry-point discovery is complete; CLI --runner uses default=None (no choices= enum to update) and factory runners list auto-includes it via get_all_runner_meta().

  • Inline prompt injection (prepend role prompt to task) is a sound divergence from claude.py's file-based --append-system-prompt-file, since Codex has no equivalent flag. It also eliminates the temp-file cleanup path entirely.
  • CLI flags verified against Codex CLI docs: codex exec --json, -C <cwd>, --dangerously-bypass-approvals-and-sandbox (headless), bare codex "<prompt>" + --ask-for-approval never (interactive) — all valid.
  • The primary concern is the headless output parser (Finding 1) — it likely won't match real codex exec --json output.

Breaking Changes (code-review-breaking-change)

No breaking change issues found. The change is purely additive; default runner stays claude. No caller assumes a single runner or that all runners support sessions. Codex silently ignores session_name/resume/background/tmux_persist (consistent with its supports_*=False metadata).

Change Size (code-review-change-size)

Pass. +395/-0 (210 lines non-test production code) is well within the 500/800 thresholds. Single cohesive concern (implementation + registration + tests) — no scope creep. Split not recommended.

Test Integrity (code-review-test-integrity)

No test integrity issues found. 17 new behavioral tests, no deletions, no CI tampering, no skip/xfail.

Note on a subagent false positive (excluded per verification): one analysis pass initially flagged deletions in tests/test_cli.py, tests/test_worktree.py, tests/test_runners.py and a ci.yml change as test tampering. Verified against the GitHub API diff and the true merge-base (git diff 081f50a5..HEAD): this PR touches only the 3 files above — those deletions belong to unrelated work already on main and were an artifact of diffing against a stale main..branch range. No human_review_required condition exists.

Comment Density (code-review-comment-density)

No file-level issue (density well under 30%). Three nit-level docstrings that restate the method name (Findings 5). The module docstring and the "no --append-system-prompt-file flag" comments are useful why context and are correctly kept.

Race Condition (code-review-race-condition-check)

No race condition issues found. build_command performs zero filesystem writes (prompt injected inline, temp_files == []), so the entire class of shared-temp-file races present in claude.py is structurally absent. No AGENTS.md or fixed-path files are created. -C cwd/PROJECT_PATH only scope the subprocess to its own per-request working directory.

Spec Compliance

  • PRD: none found in repo (.dingllm/prd/, .dingllm/specs/ absent) — no spec anchoring possible.
  • Diagrams: none found.

Action Items

  • 1. [MEDIUM] Headless JSON parser will not match real codex exec --json outputfactory/runners/codex.py:154-170
Details

Outside spec scope (no PRD); functional correctness issue.

The parser scans stdout lines in reverse for a dict containing a top-level "result" or "message" key:

if isinstance(parsed, dict) and ("result" in parsed or "message" in parsed):
    data = parsed
    break

Per the Codex CLI docs, codex exec --json emits a JSONL stream of typed event objects (e.g. {"type":"item.completed","item":{"type":"agent_message","text":"..."}}, {"type":"turn.completed","usage":{...}}) — none of which carry a top-level result/message key. As written, data stays None, so:

  • result_text falls back to the entire raw JSONL event stream rather than the assistant's final message, and
  • usage is never populated (_parse_codex_usage is never called on real output).

The unit tests don't catch this because they call _parse_codex_usage directly with synthetic dicts and never exercise the headless JSONL scan path.

Recommendation: Parse the actual Codex event schema — extract the final agent_message item's text for result_text, and read usage from the turn.completed event. Add a headless-level test that feeds a representative Codex JSONL stream and asserts both stdout and usage. If matching the real schema is deferred, at minimum avoid setting result_text to the full raw event log. (supports_usage_telemetry=False already signals telemetry is best-effort, which softens the usage half — but result text pollution remains.)

  • 2. [LOW] Module docstring contradicts the implementation (stale AGENTS.md references)factory/runners/codex.py:5-8
Details

Outside spec scope; doc correctness.

The module docstring still states the system prompt "is injected via an AGENTS.md file in the project directory", but the final implementation (commit 098a9f00) injects the prompt inline and writes no files (temp_files == []). The tests/test_codex_runner.py docstring/TestCombinedPrompt comments similarly reference AGENTS.md. Update both to describe inline injection so the docs match the code.

  • 3. [LOW] PR description is stale — PR body
Details

Outside spec scope; metadata accuracy.

The description says the runner is registered "alongside ClaudeRunner and GlaudeRunner" — there is no GlaudeRunner in the codebase (base __init__.py registered only claude). It also describes the AGENTS.md approach that was later replaced by inline injection. Please update the PR body to reflect the final design (helps reviewers and the changelog).

  • 4. [LOW] Coverage gaps for the runner's execution pathstests/test_codex_runner.py
Details

Outside spec scope; test coverage.

The 17 tests cover metadata, build_command, model resolution, combined-prompt, _parse_codex_usage, and registration — but leave untested: headless() (esp. the JSONL last-line scan — directly related to Finding 1, the highest-value gap), interactive_run() command construction, the skip_permissions=False branches (headless --dangerously-bypass... and interactive --ask-for-approval never), VIRTUAL_ENV stripping / PROJECT_PATH/FACTORY_MODEL env injection, and the cost/duration_ms/num_turns fields of _parse_codex_usage. Adding a headless test would also validate the fix for Finding 1.

  • 5. [LOW/nit] Docstrings that restate the method namefactory/runners/codex.py:133, :183, :36
Details

Outside spec scope; style.

headless ("Run a headless Codex CLI invocation.") and interactive_run ("Run an interactive Codex session as a subprocess.") docstrings restate their names while the non-obvious behavior (reverse-scanning JSONL for the final result) is undocumented. _parse_codex_usage:36 is borderline — the interesting bit (the input_tokens/prompt_tokens dual fallback) is worth a one-line note. Optional.

  • 6. [LOW/nit] No-op try/finally: pass in interactive_runfactory/runners/codex.py:202-207
Details

Outside spec scope; dead code.

interactive_run wraps the subprocess call in try: ... finally: pass. Since there is nothing to clean up (no temp files), the try/finally adds no value and can be dropped for clarity. Optional.


— Reviewed by Coding Agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs_changes Code review found issues requiring changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants