Repository navigation
Conversation
Build a validated container image for OpenAI's Codex CLI (Apache 2.0) on Red Hat OpenShift AI, following the base-image-plus-flavor pattern established by OpenCode. Base image (Containerfile.base): - Compiles Codex v0.144.0 from source (Rust) via cross-compilation - UBI 9 minimal runtime with git, jq, tar, nodejs, npm - Includes bwrap (bubblewrap) sandbox binary - Non-root user (UID 1001, GID 0) for restricted-v2 SCC - Published to quay.io/hmoghani/codex-base:0.144.0 RHOAI flavor (Containerfile): - Extends base with entrypoint.sh for runtime configuration - Inference endpoint configurable via OPENAI_BASE_URL env var - MCP server injection via TOML file, mounted ConfigMap, or JSON env var - Git credential injection via GITHUB_PAT - Session persistence via PVC-backed CODEX_HOME - codex-run wrapper script for oc exec convenience Deployment (deployment.yaml): - ImageStream, BuildConfig, ConfigMaps, PVC, Deployment - Recreate strategy for RWO PVC compatibility - Full restricted-v2 SCC security context Known issue: Codex CLI uses the OpenAI Responses API exclusively with namespace tool types. vLLM 0.19.1 (current RHOAI EA build) does not support these tool types, returning "tool type namespace not supported". Direct /v1/responses calls without tools work correctly. Full Codex compatibility requires a newer vLLM version with namespace tool type support. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two Codex container build variants, runtime initialization, OpenShift resources for persistent vLLM deployment, and documentation covering operation, configuration, troubleshooting, and cleanup. ChangesCodex OpenShift deployment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BuildConfig
participant ImageStream
participant Deployment
participant ConfigMaps
participant WorkspacePVC
participant entrypoint.sh
participant vLLM
BuildConfig->>ImageStream: Publish codex:latest
Deployment->>ImageStream: Pull codex:latest
Deployment->>ConfigMaps: Mount configuration
Deployment->>WorkspacePVC: Mount persistent workspace
Deployment->>entrypoint.sh: Start persistent container
entrypoint.sh->>WorkspacePVC: Initialize CODEX_HOME
entrypoint.sh->>vLLM: Configure Responses API endpoint
entrypoint.sh->>Deployment: Generate codex-run and env.sh
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Large PR detected (1481 lines changed) This PR exceeds 1200 lines of code changes (excluding lock files, generated content, and images). Large PRs are harder to review thoroughly and are more likely to introduce bugs. Consider splitting this PR into smaller, focused changes. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
agents/codex/deployment/deployment.yaml (1)
163-176: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a
SecretforOPENAI_API_KEYinstead of a plaintext env var.Even though the value is
"dummy"for vLLM, the template should demonstrate the secure pattern so operators don't paste real keys directly into the manifest.🔒 Suggested: reference a Secret instead of plaintext value
- name: OPENAI_API_KEY - value: "dummy" + valueFrom: + secretKeyRef: + name: codex-api-key + key: api-keyAnd add a Secret resource (created out-of-band by the operator):
# Create before deploying: # oc create secret generic codex-api-key --from-literal=api-key=dummy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agents/codex/deployment/deployment.yaml` around lines 163 - 176, Replace the plaintext OPENAI_API_KEY value in the deployment environment with a secretKeyRef referencing the operator-created Secret codex-api-key and key api-key. Add a nearby comment documenting the required oc create secret generic codex-api-key --from-literal=api-key=dummy command, without embedding a Secret resource in the manifest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/codex/deployment/entrypoint.sh`:
- Around line 274-282: Remove the OPENAI_API_KEY and CODEX_API_KEY export lines
from the env.sh heredoc in the entrypoint environment-generation block, leaving
only non-secret settings persisted to the PVC. Keep both API keys available
through the container environment for oc exec sessions, and ensure no other
generated file writes these secrets to disk.
- Around line 200-246: Prevent setup_mcp from appending duplicate MCP
definitions to the persistent config.toml on container restart. At the start of
setup_mcp, detect whether config_file already contains MCP server tables
matching [mcp_servers.*]; if present, log that MCP configuration is already
loaded and return before processing MCP_CONFIG_TOML, MCP_CONFIG_JSON, or the
mounted ConfigMap source.
- Around line 178-191: Update the provider setup in the entrypoint configuration
logic to require a non-empty, valid OPENAI_MODEL instead of falling back to
"default" when OPENAI_BASE_URL is set, and fail clearly if it is missing. Reuse
the existing model-entry detection or add equivalent deduplication before
appending the generated model setting, so the model assignment is emitted only
when no staged model already exists; locate the relevant logic around the
OPENAI_MODEL handling and the provider configuration append block.
In `@agents/codex/README.md`:
- Around line 388-414: The MCP setup flow, including setup_mcp() and handling of
MCP_CONFIG_TOML, MCP_CONFIG_JSON, and the mounted mcp-servers.toml, must be
idempotent across restarts. Stop appending managed MCP configuration directly to
persistent /workspace/.codex/config.toml; instead replace a clearly delimited
managed block or rebuild the generated configuration from the base config on
each startup, preventing duplicate [mcp_servers.*] tables.
- Around line 150-153: Update the registry authentication and push commands in
the README to avoid --tls-verify=false; use oc registry login or configure a
trusted registry CA, and pass the token securely via --password-stdin rather
than exposing it in command arguments.
- Around line 141-145: Update the documented OpenShift commands so the
codex-base BuildConfig explicitly uses Containerfile.base via dockerfilePath,
and modify the codex flavor start-build command to pass
BASE_IMAGE=codex-base:latest, ensuring it consumes the freshly built internal
base image.
---
Nitpick comments:
In `@agents/codex/deployment/deployment.yaml`:
- Around line 163-176: Replace the plaintext OPENAI_API_KEY value in the
deployment environment with a secretKeyRef referencing the operator-created
Secret codex-api-key and key api-key. Add a nearby comment documenting the
required oc create secret generic codex-api-key --from-literal=api-key=dummy
command, without embedding a Secret resource in the manifest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 06abf1f5-19b8-4bfb-9a05-9313e6a94ff0
📒 Files selected for processing (7)
agents/codex/README.mdagents/codex/deployment/Containerfileagents/codex/deployment/Containerfile.baseagents/codex/deployment/Containerfile.openshellagents/codex/deployment/README.mdagents/codex/deployment/deployment.yamlagents/codex/deployment/entrypoint.sh
- Split CODEX_EXTRA_ARGS (global) from CODEX_EXEC_ARGS (exec-only). --skip-git-repo-check is only valid for `codex exec`, not interactive mode. The codex-run wrapper now detects the subcommand and applies exec-only args appropriately. - MCP server injection now deduplicates: servers already present in config.toml are skipped on restart. Prevents accumulating duplicate [mcp_servers.*] entries across pod restarts when config is persisted on PVC. Applies to all three injection methods (TOML file, JSON env var, mounted ConfigMap). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove API keys from env.sh persisted to PVC — secrets stay in container env vars only, not written to disk - Require OPENAI_MODEL when OPENAI_BASE_URL is set instead of falling back to invalid "default" model name - Deduplicate model entry in config.toml when staged ConfigMap already sets it - Replace --tls-verify=false with oc registry login in README - Remove cluster-build Option A from README (oc new-build without dockerfilePath doesn't find Containerfile.base) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hmoghani
changed the title
Tested Codex CLI against vLLM v0.19.1 (RHOAI EA) and v0.25.1 (upstream). v0.19.1 returns "tool type namespace not supported" on every request — Codex cannot function. v0.25.1 works end-to-end with Qwen3-8B and tool calling enabled. - Add version compatibility table to Troubleshooting section - Document required vLLM flags (--enable-auto-tool-choice, --tool-call-parser) and model-specific parser values - Update Known Limitations with concrete version guidance - Fix entrypoint: env vars now override staged ConfigMap values for model/provider config (sed replace instead of skip) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Testing gpt-oss-120b on upstream vLLM v0.25.1 with --tool-call-parser openai reproduces the same "tool type namespace not supported" error as RHOAI v0.21.0. The error is a parser limitation, not a version issue: the openai parser does not support the namespace tool type Codex sends. Using qwen3_coder or hermes parsers with compatible models works on v0.24.0+. Tested configurations: - RHOAI v0.21.0 + gpt-oss-120b + parser=openai → fails - Upstream v0.25.1 + gpt-oss-120b + parser=openai → fails (same error) - Upstream v0.25.1 + Qwen3-8B + parser=qwen3_coder → works - Upstream v0.24.0 + Qwen3-8B + parser=qwen3_coder → works Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s, not parser Testing gpt-oss-120b with hermes parser on upstream v0.25.1 still returns "tool type namespace not supported". The rejection is in a gpt-oss-specific code path in vLLM's Responses API layer, before the parser is invoked. Open-source models (Qwen3, Llama) work fine. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ls in vLLM Per vLLM team: namespace tool types are supported for non-harmony (open-source) models but not yet implemented for harmony (gpt-oss) models. This is a known limitation. Updated docs to use correct terminology (harmony vs non-harmony) and remove speculation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shricharan-ks
left a comment
shricharan-ks
left a comment
There was a problem hiding this comment.
Code Review — PR #251 (comprehensive, 2 review agents)
This PR adds a Codex CLI container image for RHOAI — multi-stage Containerfiles (Rust cross-compilation), an entrypoint script handling auth/config/MCP injection, OpenShift deployment manifests, and comprehensive README. 7 files changed, 1533 additions.
The container architecture is well-designed and the README is thorough. However, the entrypoint script has several injection vulnerabilities that should be addressed before merge, plus supply chain concerns in the Containerfiles.
Findings (8, ranked by severity)
1. 🔴 entrypoint.sh:299 — Shell injection via PVC-persisted env.sh (CONFIRMED)
OPENAI_BASE_URL and OPENAI_MODEL are interpolated unsanitized into a generated env.sh script written to the PVC:
echo "export OPENAI_BASE_URL=\"${OPENAI_BASE_URL}\"" >> "${CODEX_DIR}/env.sh"If OPENAI_BASE_URL contains " or backticks (e.g., via a misconfigured ConfigMap), the generated env.sh becomes an injection payload. Since .bashrc sources this file and it lives on the PVC, the payload survives pod restarts and executes on every oc exec session with access to all env vars including API keys.
Fix: Use printf '%s' "$var" with single quotes in the generated script, or write a non-executable env file parsed by the entrypoint rather than sourced by bash.
2. 🔴 entrypoint.sh:184 — TOML injection via model/URL env vars (CONFIRMED)
OPENAI_MODEL and OPENAI_BASE_URL are interpolated into a config.toml heredoc without escaping quotes or newlines. A crafted model name can inject a rogue [model_providers] section, routing all inference traffic (prompts + code context) to an attacker-controlled endpoint.
Fix: Sanitize values (strip quotes/newlines) or use a tool like python3 -c "import toml; ..." to generate the config safely.
3. 🟡 deployment.yaml:174 — API key in plaintext env var (CONFIRMED)
- name: OPENAI_API_KEY
value: "dummy"When a user replaces dummy with a real key, it's stored in plaintext in the Deployment spec. Should use valueFrom: secretKeyRef: pointing to a Secret resource. CodeRabbit also flagged this.
4. 🟡 Containerfile:21 — Default BASE_IMAGE points to personal Quay registry (CONFIRMED)
ARG BASE_IMAGE=quay.io/hmoghani/codex-base:latestThis is a personal namespace with a :latest floating tag. If the account is deleted or the image is overwritten, all downstream builds break. Should point to an org-owned registry (e.g., quay.io/opendatahub/) or clearly document that users must build the base locally first.
5. 🟡 entrypoint.sh:242 — MCP JSON-to-TOML conversion doesn't escape quotes (CONFIRMED)
MCP server args containing double-quotes (e.g., --config={"key":"val"}) produce malformed TOML. The jq wrapping adds outer quotes but doesn't escape inner ones, causing Codex to fail to parse its config and silently drop all MCP servers.
6. 🟡 Containerfile.base:31 — Rolling rust:1-bookworm builder tag (PLAUSIBLE)
The builder stage uses an unpinned rust:1-bookworm while the runtime is digest-pinned. A Rust/Debian update between rebuilds could introduce ABI mismatches with the pinned UBI9 runtime. Pin to at least a patch-level tag (e.g., rust:1.87.0-bookworm).
7. 🟡 README.md:1 — Agent not listed in AGENTS.md non-standard section (CONFIRMED)
This agent has no Makefile, no pyproject.toml, no FastAPI, no /chat/completions, no port 8080, and no tests/. It diverges from every standard convention. The PR should add a codex/deployment entry to the "Non-standard agents" section in AGENTS.md.
8. 🟡 README.md:63 — README says "Node.js 16" but actual version is 18+ (CONFIRMED)
Containerfile.base runs microdnf install nodejs without version pinning. UBI9 repos ship Node.js 18 (Node 16 reached EOL in September 2023). The README claim is inaccurate.
Additional observations (not blocking)
entrypoint.sh:313: Unquoted${CODEX_EXTRA_ARGS}breaks on model names with spaces or glob characters. Use a bash array instead.entrypoint.sh:210: MCP server names used unsanitized in grep/sed regex patterns — names with.*or]cause regex injection.entrypoint.sh:102:GITHUB_PATwritten to.git-credentialsis not scrubbed on exit — stale credentials persist on the PVC after rotation.
What looks good
- Multi-stage build correctly separates Rust compilation from UBI9 runtime
- Non-root execution (UID 1001, restricted-v2 SCC)
- PVC-backed session persistence with config separation (env.sh excludes secrets — though the injection makes this moot)
codex-runwrapper handles both interactive and exec modes cleanly- vLLM compatibility matrix is well-documented with root cause analysis
- MCP dedup logic prevents duplicate injection on pod restarts
Summary
Findings #1 and #2 are blocking — the shell and TOML injection vulnerabilities allow env var values to execute arbitrary commands or redirect inference traffic. Fix those, address the plaintext API key (#3) and personal registry (#4), and this is ready to merge.
Security fixes: - Sanitize OPENAI_BASE_URL and OPENAI_MODEL before interpolating into config.toml and env.sh (prevents TOML/shell injection) - Use single-quoted printf in env.sh instead of double-quoted heredoc - Use secretKeyRef for OPENAI_API_KEY in deployment.yaml Supply chain fixes: - Pin Rust builder image to 1.87.0-bookworm (was rolling :1-bookworm) - Document that quay.io/hmoghani base image will move to org-owned registry; users can build locally with Containerfile.base Documentation fixes: - Fix Node.js version reference (UBI9 ships 18, not 16) - Add codex/deployment to AGENTS.md non-standard agents section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bardenr
left a comment
bardenr
left a comment
There was a problem hiding this comment.
I was able to follow the readme to deploy on rosa. Only had issues with pushing a local image to the clusters image registry and some of the commands using codex-run
|
Tested on OpenShift — two issues found Deployed the PR on our cluster (built from source, deployed to OpenShift with OpenAI backend / gpt-5.6-sol). Found two issues: 1. Missing The cargo build --release --target x86_64-unknown-linux-gnu --bin codex --bin bwrapBut Codex requires Suggested fix — add to the cargo build and binary copy in cargo build --release --target x86_64-unknown-linux-gnu --bin codex --bin bwrap --bin codex-code-mode-hostAnd in Stage 2: COPY --from=builder /opt/codex-code-mode-host /usr/local/bin/codex-code-mode-host2. The entrypoint propagates Suggested fix — add to - name: CODEX_API_KEY
valueFrom:
secretKeyRef:
name: codex-api-key
key: api-key🤖 Generated with Claude Code |
Tested Qwen3-8B and Qwen3-32B on vLLM v0.25.1: simple prompts work but tool execution fails. The models emit tool calls as text (<tool_call> XML tags) instead of structured function_call objects. vLLM's Responses API parser doesn't convert them. Tool-dependent workflows require an OpenAI backend until vLLM fixes this. Added compatibility table, upstream issue links, and updated Known Limitations section. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Link to vllm-project/vllm#49493 in the Responses API Compatibility section so users can track the fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lling works Tested findings: - Qwen3.6-27B + qwen3_coder: full tool calling on upstream vLLM v0.25.1 - Qwen3-32B + hermes: full tool calling on upstream vLLM v0.25.1 - qwen3_coder is for Qwen3.6+; older Qwen3 models need hermes parser - RHOAI 3.5 EA (v0.21.0) fails with "Unexpected message role" — needs upstream v0.25.1+ or RHOAI 3.6 EA (expected v0.26+) Updated compatibility tables, parser guidance, vLLM version requirements, example commands, and Known Limitations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/codex/README.md`:
- Around line 672-675: Update the “Open Source Model Tool Calling” heading to
use “Open-Source,” and add the required blank lines before and after the
compatibility list under that heading to satisfy MD032/blanks-around-lists.
- Around line 521-526: The README’s statement that the namespace tool-type error
occurs on all vLLM versions and parsers overstates the tested evidence. Update
the explanatory paragraph around the harmony-model limitation to qualify this as
applying to the current tested versions and parsers, or substantiate the
absolute claim with coverage across the supported range; preserve the existing
model-specific scope and upstream issue reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 455d065c-b71a-4aa0-85fd-5bb86a58719d
📒 Files selected for processing (2)
agents/codex/README.mdagents/codex/deployment/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/codex/deployment/deployment.yaml
The "Unexpected message role" error on RHOAI v0.21.0 is a Codex + vLLM compatibility issue (Codex sends developer role messages), not model-specific. Updated tables and version requirements accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lint - Qualify "all vLLM versions" to "all tested vLLM versions" for the harmony namespace tool error - Hyphenate "Open-Source" in heading - Add blank line before list (MD032) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The TEMP_FILES array and cleanup_temp_files trap were unused dead code from an earlier MCP injection approach that wrote temp files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Bug: During testing on OpenShift with Qwen 3.6-27B, Root cause: Suggested fix: Insert 🤖 Generated with Claude Code |
When Codex writes runtime sections like [tui.model_availability_nux] or [projects."..."] to config.toml, appending model= at the end places it inside those sections instead of at the top level, causing TOML parse errors on restart. Use sed -i '1i' to insert top-level keys at the beginning. The [model_providers.vllm] section block is still appended since it's a section header. (Reported by msager27) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ming - Add OGX to namespace tool error description (same error occurs) - Use consistent "RHOAI 3.5 EA (vLLM v0.21.0)" naming throughout instead of bare "RHOAI v0.21.0" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shricharan-ks
left a comment
shricharan-ks
left a comment
There was a problem hiding this comment.
Re-review — PR #251 (after 25 commits, addressing all reviewer feedback)
Comprehensive re-review checking all findings from msager27, bardenr, CodeRabbit, and my previous review.
Resolved findings ✅ (10/13)
| Finding | Source | Status | Evidence |
|---|---|---|---|
Missing codex-code-mode-host binary |
msager27 | ✅ FIXED | Containerfile switched to npm install; Containerfile.base now builds --bin codex-code-mode-host |
| config.toml key ordering crash | msager27 | ✅ FIXED | setup_model_provider() now uses sed -i "1i\\..." to insert at top |
| OGX namespace tool errors | msager27 | ✅ FIXED | Documented in README and AGENTS.md |
| codex-run needs full path | bardenr | ✅ FIXED | README uses bash -l which sources .bashrc adding ~/.codex to PATH |
| oc registry info gives internal route | bardenr | ✅ FIXED | README uses oc get route -n openshift-image-registry for external route |
| Shell injection via env.sh | Previous review | ✅ FIXED | sanitize_value() strips ", ', `, $, \, newlines; single-quoted printf; API keys not persisted |
| API key in plaintext deployment.yaml | Previous review | ✅ FIXED | Now uses secretKeyRef referencing K8s Secret codex-api-key |
| Personal registry in Containerfile | Previous review | ✅ FIXED | Main Containerfile and Containerfile.base use digest-pinned UBI9 |
| MCP config duplication on restart | CodeRabbit | ✅ FIXED | All MCP injection methods check grep -q before appending |
| Fallback model name "default" | CodeRabbit | ✅ FIXED | No default fallback exists; errors on missing model |
Remaining findings (3)
1. 🟡 entrypoint.sh:~160 — TOML injection in fallback path (CONFIRMED)
The main setup_model_provider() path correctly calls sanitize_value() on OPENAI_MODEL (line ~173). But the early-return branch when OPENAI_BASE_URL is empty writes OPENAI_MODEL directly without sanitization:
echo "model = \"${OPENAI_MODEL}\"" >> "${config_file}"A crafted OPENAI_MODEL with embedded " or newlines can inject rogue TOML keys. This is the same class of bug that was fixed in the main path but missed in this branch.
Fix: Call sanitize_value on OPENAI_MODEL in the fallback path too.
2. 🟡 entrypoint.sh:~245 — MCP JSON-to-TOML quoting still broken (CONFIRMED)
The jq expression map("\"" + . + "\"") naively wraps MCP args in double quotes without escaping inner quotes. An arg like --config={"key":"val"} produces:
args = ["--config={"key":"val"}"]This is invalid TOML that crashes Codex on startup. From the original review — not yet addressed.
Fix: Use jq's @json filter or escape inner double-quotes with gsub.
3. 🟡 Containerfile.openshell:9 — Personal registry still used (CONFIRMED)
FROM quay.io/hmoghani/openshell-base:latestThe main Containerfile and Containerfile.base were fixed to use registry.access.redhat.com, but Containerfile.openshell still references a personal Quay.io repository.
Summary
10 of 13 findings from all reviewers are resolved — impressive iteration over 25 commits. The remaining 3 are all non-blocking for merge (the TOML injection requires a crafted env var in a branch that rarely fires, the MCP quoting affects a narrow edge case, and Containerfile.openshell is experimental). Fix the sanitize_value gap in the fallback path (#1) as priority.
OpenShell sandbox: - Update Containerfile.openshell to use org-owned base image (quay.io/aipcc/agentic-ci/openshell:0.3.27, RHEL 10) - Use dnf instead of microdnf (RHEL 10) - Add comprehensive OpenShell setup guide to README: Helm install, mTLS cert extraction, sandbox creation, Codex testing - Tested on OpenShell v0.0.86, OpenShift 4.21, Qwen3.6-27B Review fixes: - Sanitize OPENAI_MODEL in fallback path (shricharan-ks #1) - Fix MCP JSON-to-TOML quoting with jq @JSON (shricharan-ks #2) - Update deployment/README.md references Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change #### to ### under ## OpenShell Sandbox to avoid skipping h3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@shricharan-ks thanks for the review! All issues are addressed and fixed. |
aakankshaduggal
left a comment
aakankshaduggal
left a comment
There was a problem hiding this comment.
Tested end-to-end on ROSA cluster (hmoghani-codex namespace):
- Image build:
codex:v4-1785175052built and deployed successfully fromContainerfile(npm variant) - Entrypoint: All setup stages work correctly — env validation, CODEX_HOME symlink, vLLM provider config, MCP dedup,
codex-runwrapper, API key propagation - Model serving: Validated with both Qwen3-8B and Qwen3.6-27B via vLLM (
--tool-call-parser qwen3_coder). Tool calls generate correctly with the 27B model. - Security: API keys via
secretKeyRef,sanitize_value()prevents injection, non-root user (UID 1001), restricted-v2 SCC compatible - MLflow tracing: Built
codex-mlflowlayer on top of this base image — traces export successfully with 10-span traces (AGENT + TOOL + LLM spans)
Minor non-blocking notes:
--tls-verify=falsestill in README registry commands — consideroc registry loginfor production guidance- Unquoted
${CODEX_EXTRA_ARGS}in entrypoint exec calls — edge case with unusual model names
LGTM — well-structured, thoroughly reviewed, and production-tested.
shricharan-ks
left a comment
shricharan-ks
left a comment
There was a problem hiding this comment.
Re-review — All findings resolved ✅
The latest commits (c710d115, 67387f9a) address all 3 remaining findings from the previous review:
Finding 1 (TOML injection in fallback path) — FIXED ✅
The no-BASE_URL fallback now sanitizes OPENAI_MODEL:
echo "model = \"$(sanitize_value "${OPENAI_MODEL}")\"" >> "${config_file}"Finding 2 (MCP JSON-to-TOML quoting) — FIXED ✅
Args now use jq's @json filter which properly escapes embedded quotes:
.args | map(@json) | join(", ")
Finding 3 (Containerfile.openshell personal registry) — FIXED ✅
Changed from quay.io/hmoghani/openshell-base:latest to quay.io/aipcc/agentic-ci/openshell:0.3.27 (org-owned registry, version-pinned tag).
Full scorecard across all reviewers (13/13 resolved)
| Finding | Source | Status |
|---|---|---|
| Missing codex-code-mode-host | msager27 | ✅ |
| config.toml key ordering crash | msager27 | ✅ |
| OGX namespace tool errors doc | msager27 | ✅ |
| codex-run full path | bardenr | ✅ |
| oc registry internal route | bardenr | ✅ |
| Shell injection via env.sh | Previous review | ✅ |
| TOML injection (main path) | Previous review | ✅ |
| TOML injection (fallback path) | Previous review | ✅ |
| API key plaintext → Secret | Previous review | ✅ |
| Personal registry (Containerfile) | Previous review | ✅ |
| Personal registry (openshell) | Previous review | ✅ |
| MCP JSON-to-TOML quoting | Previous review | ✅ |
| MCP config duplication | CodeRabbit | ✅ |
LGTM — ready to merge.
## Summary - Adds MLflow experiment tracking layer for Codex CLI deployments on RHOAI - Each interactive agent turn is exported as a trace to MLflow via the `notify` hook, capturing inputs, outputs, token usage, and child spans for LLM calls and tool invocations - Uses `mlflow-codex-hook.sh` wrapper to ensure SA token auth works regardless of how codex is launched (e.g., `oc exec` bypasses `.bashrc`) - Validated end-to-end on ROSA with Qwen3.6-27B — multi-span traces with tool call child spans ## Files | File | Description | |------|-------------| | `Containerfile.mlflow` | Image layer extending base Codex with Python 3.12, Node.js 22, MLflow SDK, @mlflow/codex | | `setup-mlflow.sh` | Post-entrypoint setup: SA token auth, experiment creation, notify hook registration, config fixups | | `mlflow-codex-hook.sh` | Notify hook wrapper — loads SA token + experiment ID before calling @mlflow/codex | | `deployment-mlflow-patch.yaml` | Strategic merge patch adding MLflow env vars to existing deployment | | `mlflow-tracing.md` | Architecture docs, trace schema, env vars, known issues | | `screenshots/vllm-codex-trace.png` | MLflow trace screenshot showing successful tool call spans | ## Key design decisions - **TOML scoping**: `notify` key must be top-level in `config.toml` — inserting after `[section]` headers causes TOML to nest it, silently breaking hook dispatch. `setup-mlflow.sh` inserts before the first `[section]`. - **Auth via wrapper**: `mlflow-codex-hook.sh` reads the SA token from disk on every invocation rather than relying on env vars, since `oc exec` bypasses `.bashrc`. - **RBAC/TLS**: Uses shared `docs/mlflow-openshift-auth-and-tls.md` patterns — `mlflow-integration` ClusterRole binding + service-ca.crt for TLS. ## Dependencies - Depends on #251 (base Codex container image) merging first ## Jira - Epic: RHAIENG-5925 — MLflow Tracing for Codex CLI Agent Runtimes - RHAIENG-5926 — Validate MLflow tracing with vLLM backends - RHAIENG-5927 — Prototype trace schema for tool calls - RHAIENG-5928 — Document tracing setup ## Test plan - [x] Build `codex-mlflow` image on-cluster via BuildConfig - [x] Deploy with `setup-mlflow.sh` chain, verify pod logs show MLflow setup - [x] Run interactive codex with a coding prompt (merge sort, fibonacci, hello world) - [x] Verify traces appear in MLflow UI with child spans (LLM + TOOL) Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Upstream Disclaimer
The Codex CLI binary installed in this image is a pre-built artifact published by OpenAI via npm (
@openai/codex), not by Red Hat. It has not been built, scanned, or validated according to Red Hat standards.Containerfile.baseis available for users who need a source-built alternative.Files
ContainerfileContainerfile.baseContainerfile.openshellentrypoint.shdeployment.yamlREADME.mddeployment/README.mdvLLM Compatibility
Requires upstream vLLM v0.25.1+. RHOAI 3.5 EA (v0.21.0) returns
"Unexpected message role"because Codex sendsdeveloperrole messages which that vLLM version does not support. RHOAI 3.6 EA (expected v0.26+) should include the fix (upstream PR #43590).Harmony models (gpt-oss) reject namespace tools on all vLLM versions. Tracked: vllm-project/vllm#49493.
qwen3_coderhermesqwen3_coderqwen3_coder"Unexpected message role"— Codex usesdeveloperrole)"tool type namespace not supported")Test plan
bash -lfor both interactive and exec modedeveloperrole support