feat(config): make the codex runtime selectable - #6927
Conversation
PR Summary by QodoEnable Codex runtime selection and model overrides
AI Description
Diagram
High-Level Assessment
Files changed (44)
|
|
🤖 Review · Commit: |
Site previewPreview: https://25b6369e-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
fe65f48 to
8d51f21
Compare
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
933a5f1 to
e14e3fd
Compare
|
🤖 Finished Review · ✅ Success · Started 7:19 PM UTC · Completed 7:41 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.88 |
|
Risk Assessment: elevated (3/5) DetailsElevated risk preserved from prior assessment. Tier 1 unchanged: 50 files, 1044 lines, large blast radius, 7 protected paths (CI workflows changed), test ratio 0.14. Tier 2 shows high 30-day churn and elevated fix/revert density on core runtime files, offset by very fresh code age, single-author contention, and zero sentiment/revert flags. Tier 3 is favorable: well-scoped issue #6920, PR matches checklist item E exactly, opt-in runtime selection via capability gate acts as a feature flag with default disabled. Previous runRisk Assessment: elevated (3/5) DetailsElevated risk preserved from prior assessment. Tier 1 unchanged: 50 files, 1044 lines, large blast radius, 7 protected paths (6 CI workflows), CI workflows changed, test ratio 0.14. Tier 2 shows extreme 30-day churn and high fix/revert density on core files like run.go and architecture.md, offset by very fresh code age and zero sentiment flags. Tier 3 is favorable: well-scoped issue #6920, PR matches checklist item E exactly, opt-in runtime selection acts as a feature gate. Weighted composite at the 2/3 boundary; the structural risk from protected-path count, blast radius, and high file churn on core CLI and config paths sustains the elevated level. Previous run (2)Risk Assessment: elevated (3/5) DetailsElevated risk unchanged from prior assessment. Tier 1 signals identical (50 files, 1044 lines, 7 protected paths including 6 CI workflows, large blast radius). High churn on core files offset by very recent code age, additive new files, and opt-in runtime selection acting as a feature gate. Well-structured 5-PR stack with clear issue scope (#6920), non-first-time author, test coverage present (0.14 ratio). Previous run (3)Risk Assessment: elevated (3/5) DetailsElevated risk unchanged from prior assessment. Tier 1 signals identical (50 files, 1044 lines, 7 protected paths including 6 CI workflows, large blast radius). High churn on core files (config.go, run.go, registry.go) offset by additive new files and documentation. Well-structured 5-PR stack with clear issue scope; this PR slice focuses on config/docs/CLI rather than security-sensitive credential plumbing. Non-first-time author, test coverage present (0.14 ratio). Previous run (4)Risk Assessment: elevated (3/5) DetailsElevated risk driven by large blast radius (50 files, 7 protected paths including 6 CI workflows), large change size (1042 lines), and active churn on key files. Mitigated by non-first-time author, additive feature with clear rollback path, well-defined issue scope (Part E of 5-PR stack), and test coverage present (7 test files, 0.14 ratio). |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
Next steps:
|
|
🤖 Review · Commit: |
2d31a58 to
a098af9
Compare
Adds "codex" to config.ValidRuntimes(), so an org, a repo or a single agents: entry can select it the way pi has been selectable since #6464. The stub added with the image pin was registered in Resolve() but kept out of ValidRuntimes() precisely so nothing could reach it before Bootstrap and Run existed; with the runtime implemented, that gate comes off. Everything that enumerates runtimes derives from ValidRuntimes(), so the error text in config validation, the manifest validator, run overrides, `fullsend github setup`/`repos` and the behaviour-test step all pick codex up for free. The two places that restate the list by hand — the --runtime flag help and the CLI reference tables — are updated here, as are the manifest comments naming the installable runtimes. codex also joins the interactive runtime prompt: userRuntimeChoices() filters ValidRuntimes() down to the human-selectable set, which is codex now that it is no longer a stub. opencode stays a stub and stays rejected through every config path. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
Adds docs/runtimes/codex.md and wires codex into the pages that enumerate runtimes, so a reader choosing one sees three options rather than two. The page follows the shape the other runtime pages use — what it is, one command, models, at-a-glance, running it locally, behaviour differences, what is not yet exercised, troubleshooting — and states only codex's own side; cross-runtime comparison stays in runtimes.md, and the hardening rationale stays out of user docs entirely. runtimes.md gains a codex row in the status table, a third column in the comparison and config-key tables, and a models paragraph. "Choosing between claude and pi" becomes "Choosing a runtime" now that it covers three, with the old heading kept as an explicit anchor so existing links still land. The local-run guide's OpenAI section covered "GPT on pi only"; codex takes the same key and the same harness requirements, so the heading and its body now name both runtimes — again with the old anchor preserved, and pi.md's inbound link moved to the new one. The WIF guide's opening sentence gets the same treatment; nothing else there is runtime-neutral yet, so nothing else changed. Several passages describe behaviour whose exact wording depends on the runtime implementation's smoke evidence — the plan block transcript, the preflight error text, the platform matrix, the exact troubleshooting messages. Those carry TODO(D) markers rather than invented text. The links to ADR 0099 and the contributing page's codex section are held the same way: the markdown link checker rejects targets that do not exist yet. Verified: lychee (offline, --include-fragments) reports 0 errors over 932 unique links, and `vitepress build docs` renders runtimes/codex.html with no dead links. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
Adds features/runtime/codex-openai.feature, the codex counterpart of
pi-openai.feature: the repo's `runtime: codex` reaches backend selection,
the runner exchanges the job's OIDC token into a run-scoped OpenShell
provider, and codex reaches api.openai.com holding nothing but the
gateway's placeholder. A failed exchange or a rejected placeholder fails
the workflow, so a successful run is the assertion.
Gated on `runtime-codex-openai` and deliberately not in the Makefile
default — it needs an OpenAI organization mapped to the pool repositories.
Codex has no Vertex path, so unlike pi (whose runtime-pi scenario runs on
every behaviour job) this is its *only* behaviour scenario: until that
organization exists codex has no default behaviour coverage at all, and its
evidence is unit tests, recorded fixtures and local smoke runs. The feature
header, the Makefile comment and the behaviour-testing guide all say so
rather than leaving it to be inferred from an absent capability.
Two step definitions:
* `a codex agent "<name>" defined as:` — the agent-committing helper was
never pi-specific (it commits frontmatter + body and inlines a result
fixture), so it is renamed givenRuntimeAgent and both step wordings
land on it.
* `the codex output stream records at least one tool call` — asserts a
completed command_execution item in the tee'd `codex exec --json`
stream (output.jsonl). That artifact's shape is fullsend's own, unlike
codex's rollout session files, and the check refuses to accept one of
those by requiring a top-level codex ThreadEvent type.
Refs #6920
Assisted-by: Claude (implementation)
Signed-off-by: Wayne Sun <gsun@redhat.com>
…ULLSEND_CODEX_MODEL `FULLSEND_PI_MODEL` was a single hard-coded case in resolveRunOverrides. Codex needs the same thing, so the gate becomes a runtimeModelEnv(runtime) lookup and the pi case is one entry in it. Why codex needs one: it serves OpenAI models only and needs one named, while the fleet harnesses all ship `model: opus`. Without a runner-side knob a repo moving to codex would have to edit every harness it runs. FULLSEND_CODEX_MODEL is that one place, and it mirrors the pi knob exactly — read only when codex is the runtime actually selected, below --model and FULLSEND_MODEL and above the agents: entry and the harness. It is a per-repo default, not an override, and when it decides the model the plan block and metrics.json's override_source name it like any other source. The lookup deliberately lists only the runtimes that need a knob rather than deriving FULLSEND_<NAME>_MODEL for all of them: that would document variables nobody asked for on claude and the test runtimes, and dummy-playback could not have one at all — a hyphen is not legal in an environment variable name. No new plumbing. run.go already resolves the config runtime and re-runs the override resolution so a runtime-scoped alias can be gated on the selected runtime; that path was written for pi and generalises unchanged. The precedence tests now cover both knobs and that neither leaks into the other's runtime. Typed `refactor` rather than `feat`: the substance is the generalisation, and the release-notes-worthy capability in this PR is the selectable codex runtime itself, not its environment variable (COMMITS.md, "`feat` is for end users"). Also corrects a drift the knob exposed: the --model flag help and its row in docs/cli/run.md both said `provider/id` applies "on pi", which is now true of codex too. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
Replaces every TODO(D) placeholder in docs/runtimes/codex.md with what the
runtime actually does, now that the codex runtime and ADR 0099 are on the
branch. Nothing here is paraphrased: the messages are quoted from
codex_run.go and codex_bootstrap.go, the behaviour from the smoke evidence
recorded in the contributing doc.
* The model refusal is the real text, and the alias case is the one shown
— it is the failure a repo on fleet harnesses actually hits. Both other
cases (foreign prefix, no model at all) are named as failing the same
way with the same two fixes.
* The plan block gains the Agent/exit lines, with a note that the version
on it is the codex CLI from Bootstrap's preflight, not the model.
* A stale image is named by its real preflight failure,
`codex --version` exited 127, and the version floor is stated the way
pi.md did before its release shipped.
* The hook-guard entry quotes the exit-97 message and mentions the sibling
config-pinning guard, which is a credential-leak guard rather than a
hook one.
* Two entries added from the smoke run: the denied `GET /v1/models` at
startup is expected and non-fatal (codex probes the catalog; the profile
allows only POST /v1/responses), and a model the account cannot serve
fails as reconnects plus a turn.failed carrying the 404 rather than at
startup — which is why the verdict comes from the stream, not the exit
code.
Two behaviour bullets were wrong and are corrected rather than expanded. A
PostToolUse block withholds the tool output from the model entirely; it does
not pass it through with a note, which is what the page claimed. And the
repository's own `.agents/skills` *are* discovered — leaving the project
untrusted suppresses `.codex/` config, not skills — so the page now says so
and adds that they are covered by the content scans and that codex's bundled
skills are switched off. The artifact-side caveat is stated too: output.jsonl
keeps raw command output either way, since the hooks protect the model's
context rather than the run's artifacts.
The links to ADR 0099 and the contributing page's codex section are un-held
now that both targets exist.
Refs #6920
Assisted-by: Claude (implementation)
Signed-off-by: Wayne Sun <gsun@redhat.com>
…docs
Grok and Codex sol review of the codex enablement PR.
The critical one is a real hole in the feature this PR adds. The documented
CI route for FULLSEND_CODEX_MODEL — set it as a repository variable — never
worked: setup-agent-env.sh's override_keys forwards FULLSEND_PI_MODEL and
FULLSEND_PI_PROVIDER but not the codex knob, so the value was silently
dropped before the agent job ever saw it. It is in the list now, with shell
and Go tests covering both the plain and the role-prefixed name.
Codex was accepted by config but invisible or misdescribed in most places a
person would look:
* the interactive `github setup` prompt offered only [claude] and [pi]
while accepting "codex". The menu lists it now, and the prompt test
asserts every runtime the prompt accepts is also offered, so the next
runtime cannot repeat this.
* --runtime help on `agent`/`github setup`/`repos`, the setup PR body,
docs/cli/{agent,repos,run}.md, and the getting-started guides all said
"claude or pi".
* `github setup --runtime codex` gave no stale-image warning; pi's had one.
* registry.go, codex.go, runtime-implementation.md, sandbox-topology.md
and images/README.md still called codex a stub kept out of
ValidRuntimes() — which this PR is what changes.
* architecture.md's runtime diagram had no codex node, though the config
node above it listed the key.
Four documentation claims were wrong rather than merely missing, which is
the worse kind:
* The effort table said max maps to xhigh. D's mapping is the identity —
an earlier draft did remap it and the change was reverted.
* `tools:` was described as enforced by the pre-tool hook. It is not: a
Bash(...) list is recorded and not enforced on codex, entries with no
codex tool are dropped with a warning, and the tool-allowlist hook is
opt-in behind FULLSEND_TOOL_ALLOWLIST. Overstating an enforcement
control is the worst direction to be wrong in.
* PostToolUse was described as withholding output whenever a sanitizer
fires. Only a canary block withholds; the redaction, unicode and
suppression stages cannot rewrite built-in tool output on codex and
instead tell the model what the result contained.
* Sub-agents were described as a missing tool. Codex has spawn_agent;
what is missing is fullsend's persona roster for it.
Also: the guard failure messages and the model-refusal text are now quoted
from the code with their exit codes (127, 97, 97, 98) rather than
paraphrased; the artifacts row records that only uncompressed rollouts are
extracted and that codex-debug.log is redacted like the rest; the plan block
shows the model spec as the runner prints it; "opt-in per org" became "per
repo, per agent, or a repos.yaml default" in both the page and the
ValidRuntimes comment, since org mode is deprecated (ADR 0044); and the
OpenAI WIF guide names codex wherever it genuinely follows the same
credential path.
The behaviour step matched `"type":"command_execution"` as a substring,
which an item.started or a declined command satisfies just as well. It now
parses each line and requires an item.completed whose status is completed,
and isCodexStreamFile is structural for the same reason the production
parser is.
Refs #6920
Assisted-by: Claude (implementation), Grok (review), Codex gpt-5.6-sol (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
Grok's re-review of the enablement PR: the structural work was confirmed, what was left is copy that still reads as though pi were the only alternative to Claude Code. Runtime enumerations: `defaults.runtime` values in the repos reference, the valid `runtime` values in the layered-config reference, and the `runtime` example in the run reference. `--model` help on `agent set` and `run` (and both reference tables) said `provider/id` applies "on pi" — it applies on codex too, with the constraint that codex takes OpenAI ids only, which the help now states rather than leaving to the page. OpenAI wording: the `--openai-*` flags, the CLI index, the operations guide and the cli-internals tree all described the WIF identifiers as "GPT on pi". They are the same identifiers for codex — it uses the same run-scoped provider — so they now say "on pi or codex". Same for the run reference's "OpenAI credentials on pi" heading and the `inference.openai` description. The run reference's artifacts table listed only claude-debug.log and pi-debug.log; codex-debug.log is a run artifact too. behaviour-testing.md described the generic gated-scenario pattern but led with pi's step name as though it were the only one. It now names both agent steps and says they commit the same file, which is what the shared helper does. Two code comments were left asserting the old state: registry.go still listed codex among the stub runtimes excluded from ValidRuntimes(), which this PR is what changes, and the ValidRuntimes comment described pi as opt-in "per org/repo" when org mode is deprecated (ADR 0044). The codex stale-image warning now names fullsend-code alongside fullsend-sandbox, as pi's does — both images carry the pin. Two test-facing fixes: codexStreamLines ignored scanner.Err(), so a line past the buffer or an unreadable file would have been reported as "the agent ran no tools" — an assertion failure blaming the agent for a read problem — and it now returns the error for the step to surface. And TestResolveFromPerRepoConfig gained a positive `runtime: codex` case, so the per-repo path is asserted to resolve the codex backend rather than only asserted not to reject it. Refs #6920 Assisted-by: Claude (implementation), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
… advisory Codex sol's re-review of the enablement PR. The positive org-config case I added — `Defaults.Runtime = "codex"` passing validation — extended coverage of a deprecated surface. Org mode is on its way out (ADR 0044), so codex's selectability is asserted where it is actually used: the per-repo key and an `agents:` entry, both of which keep their assertions. pi's existing org case is left as it is; removing it is not this PR's business. The other item was a wording problem with teeth. The runtime prompt and three doc sentences said `review` and `retro` "stay on Claude Code" on codex, which reads as something fullsend enforces. Nothing pins those roles — a repo-wide `runtime: codex` applies to them like any other agent, and they will run, just in a single context without the persona roster. The prompt now offers it as a recommendation, and the codex page says plainly that it is a recommendation rather than a restriction and names the way to make it one (`runtime: claude` on those agents' entries), which is the part a reader actually needs. Refs #6920 Assisted-by: Claude (implementation), Codex gpt-5.6-sol (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Qodo found that the documented repository-variable route only works on one of the two ways a role workflow runs. reusable-dispatch.yml passes FULLSEND_REPO_VARS into every setup-agent-env.sh step it drives, but the standalone reusable-<role>.yml workflows — the ones a repo calls directly — do not, so setup-agent-env.sh sees no repository variables at all and every override is dropped before the agent job starts. For codex that is the difference between working and not: the fleet harnesses ship `model: opus`, and without FULLSEND_CODEX_MODEL reaching the job the run is refused for naming a Claude model. The gap is not new and not codex-specific — FULLSEND_MODEL, FULLSEND_EFFORT, FULLSEND_RUNTIME and pi's knobs were dropped on the same route — but codex is what made it visible, and it is one env line per workflow, mirroring what dispatch already does. The GitLab CI template sets these variables itself rather than calling setup-agent-env.sh, so it is unaffected. Refs #6920 Assisted-by: Claude (implementation), Qodo (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…x page
Review bot findings plus a UX read of docs/runtimes/codex.md.
`fullsend agent set --model` help still said `provider/id` applies "on pi";
it matches `fullsend run`'s wording now, including that codex takes OpenAI
ids only. setup-agent-env.sh's comment above override_keys named only
FULLSEND_PI_MODEL when the list carries both knobs. The shell test's codex
cases were numbered 6b and 6c against an existing "# 6." further down; the
cases are sequential again, which meant renumbering the pre-existing 6a too
rather than leaving one oddity to explain the other.
The page itself was 296 lines against pi.md's 214, and the excess was
design notes rather than steps. Reading it as someone adopting codex for
the first time:
* The PostToolUse bullet explained the hook pipeline in its own
vocabulary — canary hits, redaction and unicode and suppression stages.
A reader wants to know that a tool result is either withheld or
annotated but never edited, and that artifacts are scrubbed regardless.
* The skills bullet walked through where each scan happens. What changes
for the reader is that skills work as they do on Claude Code and that
codex's bundled ones are off.
* "Not yet exercised" was three paragraphs of provenance. It now leads
with the advice — start on a disposable repo, keep review and retro on
Claude Code — and gives the pin as a config snippet instead of prose,
with the version-by-version run record left in the contributing page,
which already carries it.
* The permission bullet argued the security model; it now states it.
Nothing accurate was dropped: the quoted allowlist warning, the
recommendation-not-restriction wording on review/retro, and the untested
WIF path all survive, in fewer words.
Refs #6920
Assisted-by: Claude (implementation), fullsend review bot (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
…ectable PR #6923 added a TestAgentSettings_Validate row asserting an agents: entry cannot select codex while it was a stub. This PR adds codex to ValidRuntimes(); the positive coverage lives in TestPerRepoConfigValidate_Runtime, TestResolveFromPerRepoConfig and TestResolveForAgent_RejectsStubRuntimes, so the rejection row goes. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
a098af9 to
5bf423c
Compare
Outdated: the only finding (test name) is a cosmetic LOW already handled on the branch; all threads resolved; dismissed so the last PR of the stack can enter the merge queue.
|
🤖 Finished Review · ✅ Success · Started 1:13 AM UTC · Completed 1:30 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.79 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/cli/run.go(file-level): Line 1036 · [low] stale-reference
The user-facing warning message says 'model %q has a provider/id form, which is pi's' — but with codex now selectable, provider/id is also codex's model form. The comment on line 1032 has the same issue. The logic itself is correct; only the wording is stale.
Suggested fix: Change the comment and warning to mention both pi and codex.
pkg/behaviourtest/steps/runtime_test.go(file-level): Line 75 · [low] naming-consistency
Test function is named TestGivenPiAgent_CommitsDefinitionWithFixtureInlined but the function under test was renamed from givenPiAgent to givenRuntimeAgent. The test body already calls givenRuntimeAgent.
Suggested fix: Rename to TestGivenRuntimeAgent_CommitsDefinitionWithFixtureInlined.
pkg/behaviourtest/steps/runtime.go(file-level): Line 31 · [low] stale-reference
Doc comment on registerRuntimeSteps says '(pi today — see features/runtime/pi.feature)' but the PR adds codex as a second real runtime with its own feature file (codex-openai.feature).
Suggested fix: Update to mention codex or generalize.
|
🤖 Finished Retro · ✅ Success · Started 1:39 AM UTC · Completed 1:55 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.99 |
Retro: PR #6927 —
|
Summary
Makes
runtime: codexselectable and documents it.config.ValidRuntimes()+=codexand every enumeration (interactive setup prompt,--runtimehelp onrun/agent/github/repos, stale-image warning forCODEX_VERSION, CLI reference tables, architecture/glossary/getting-started guides).FULLSEND_CODEX_MODEL: a runtime-scoped model override generalised from the pi-only case (runtimeModelEnv), gated on the selected runtime, echoed as its own source in the plan block andoverride_source; forwarded into the agent job bysetup-agent-env.sh(with shell tests). Codex requires an explicit OpenAI model, and fleet harnesses saymodel: opus, so this is the per-repo default route;model: openai/<id>on the agents: entry is the other.docs/runtimes/codex.mdas a walkthrough (example → models → at a glance → running it locally → behaviour differences → not yet exercised → troubleshooting) with error and guard texts quoted verbatim from PR D; runtimes.md status/comparison/config-key/precedence rows; local-run and WIF guides; VitePress sidebar.features/runtime/codex-openai.feature(runtime-codex-openai, not in the default capability set — codex has no Vertex path, so it has no default behaviour-CI coverage until an OpenAI org WIF mapping exists) with a structural output-stream step.Review rounds
sol + Grok, then a Grok re-review: CRITICAL (
FULLSEND_CODEX_MODELmissing from the workflow's variable allowlist) fixed with tests; four doc claims corrected against PR D's code (identity effort mapping,tools:recorded not enforced, canary block vs sanitizer warn,spawn_agentexists but no roster); every remaining pi-only enumeration and copy site updated.Verification (macOS, image from PR A)
Fleet-shaped
triageandreviewfrom the agents clone on codex: fleetmodel: opusrefused with the documented text;FULLSEND_CODEX_MODEL=openai/gpt-5.6-luna→ triage exit 0 with labels and comment, review exit 0 withrisk/lowapplied (post-script 422 only because a PR cannot be reviewed by its own author). Whole stack builds and tests green on the Fedora x86_64 host.Part of a five-PR stack for #6920 (Codex as an agent runtime): A image pin → B stream parser → C OpenAI credential seeder → D runtime core (ADR 0099) → E enable + docs. Each PR is reviewable on its own diff; they merge bottom-up. Plan and verified Codex facts:
research/fullsend-codex-runtime-plan.mdin the ai-workspace-public research repo (to be linked once pushed).Refs #6920
Assisted-by: Claude (implementation and review orchestration), Codex gpt-5.6-sol (review), Grok 4.6 (review)
Pre-existing bug fixed here
The six standalone reusable role workflows (
reusable-{triage,review,code,fix,prioritize,retro}.yml) calledsetup-agent-env.shwithoutFULLSEND_REPO_VARS, so no repository override (FULLSEND_MODEL,FULLSEND_EFFORT,FULLSEND_RUNTIME, pi's knobs) reached runs on that route — onlyreusable-dispatch.ymlpassed them. Codex made it visible because a dropped override there is a refused run rather than a silently wrong model. Fixed by passing the variables on all six, mirroring dispatch.