Add Codex support VM persona lane - #516
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR parameterizes VM readiness and playtest execution by provider and player-agent (codex vs claude), making required tools and repo artifacts conditional, adding CLI flags and env propagation, branching playtest backends/player runs, and updating docs and tests. ChangesProvider and Player-Agent Parameterization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
100yenadmin
force-pushed
the
codex/worldos-codex-vm-persona-lane
branch
from
June 1, 2026 11:05
63ccb1c to
f5d8d68
Compare
100yenadmin
marked this pull request as ready for review
June 1, 2026 11:08
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@qa/ui_playtest_app.sh`:
- Around line 648-675: The current use of codex exec with multiple -c flags to
set nested mcp server keys (e.g., mcp_servers.clawdnd-uiplayer.command,
mcp_servers.clawdnd-uiplayer.args, mcp_servers.clawdnd-uiplayer.env_vars, etc.)
may be unsupported; instead either (A) move the clawdnd-uiplayer MCP server
configuration into the persistent config under [mcp_servers.clawdnd-uiplayer] in
config.toml or (B) create the server at runtime with the documented command
(codex mcp add <name> ...) before calling codex exec, then remove the -c
overrides; if you must keep runtime overrides, verify your codex version
supports these -c nested mcp_server keys and add a feature-guard or comment
noting the requirement.
🪄 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: 99281cd0-dc28-47bf-9949-6171788bf6ec
📒 Files selected for processing (7)
WorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdqa/QA_TOOLS.mdqa/support_vm_preflight.pyqa/test_macos_app_static.pyqa/test_support_vm_preflight.pyqa/ui_playtest_app.sh
100yenadmin
force-pushed
the
codex/worldos-codex-vm-persona-lane
branch
from
June 1, 2026 11:18
f5d8d68 to
3900a4f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
qa/ui_playtest_app.sh (1)
664-682:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the Codex MCP override path with a feature/version check.
Line 664–682 relies on
codex exec -c mcp_servers...behavior that has been brittle across Codex CLI docs/versions; if unsupported in the runtime CLI, Part B codex player fails. Add an explicit preflight check (or hard minimum version gate) before invoking this path, with a clear error/fallback instruction.For Codex CLI v0.120.0 and newer, is `codex exec -c "mcp_servers.<name>.command/args/env_vars=..."` officially supported for defining MCP servers at invocation time? Please cite the official Codex CLI docs/reference.Suggested minimal guard
+ if ! codex --version 2>/dev/null | grep -Eq '0\.12[0-9]\.|0\.[2-9][0-9]{2}\.|[1-9]\.'; then + printf '[uipt-app] codex lane requires Codex CLI with supported exec -c mcp_servers.* overrides. Please upgrade Codex CLI or use a config.toml/mcp add flow.\n' >&2 + exit 2 + fi codex exec \ --ignore-user-config \🤖 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 `@qa/ui_playtest_app.sh` around lines 664 - 682, Add a preflight gate before the codex exec -c "mcp_servers..." invocation in qa/ui_playtest_app.sh that detects whether the installed Codex CLI supports per-invocation MCP server overrides (e.g., by running codex --version and ensuring semver >= 0.120.0 or by invoking a harmless feature probe such as codex exec --help and looking for the -c/mcp_servers override strings); if the check fails, emit a clear error and either exit non‑zero or fall back to invoking codex exec without the mcp_servers -c overrides (so the lane remains usable), and ensure the check references the codex exec call and the mcp_servers override keys used in the script so maintainers can locate the guarded block.
🤖 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.
Duplicate comments:
In `@qa/ui_playtest_app.sh`:
- Around line 664-682: Add a preflight gate before the codex exec -c
"mcp_servers..." invocation in qa/ui_playtest_app.sh that detects whether the
installed Codex CLI supports per-invocation MCP server overrides (e.g., by
running codex --version and ensuring semver >= 0.120.0 or by invoking a harmless
feature probe such as codex exec --help and looking for the -c/mcp_servers
override strings); if the check fails, emit a clear error and either exit
non‑zero or fall back to invoking codex exec without the mcp_servers -c
overrides (so the lane remains usable), and ensure the check references the
codex exec call and the mcp_servers override keys used in the script so
maintainers can locate the guarded block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 401b2425-5442-4d07-9aaa-ee2a80dc74ad
📒 Files selected for processing (7)
WorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdqa/QA_TOOLS.mdqa/support_vm_preflight.pyqa/test_macos_app_static.pyqa/test_support_vm_preflight.pyqa/ui_playtest_app.sh
✅ Files skipped from review due to trivial changes (2)
- qa/QA_TOOLS.md
- WorldOS-OPERATING-GOAL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- qa/test_macos_app_static.py
- qa/test_support_vm_preflight.py
- qa/support_vm_preflight.py
100yenadmin
force-pushed
the
codex/worldos-codex-vm-persona-lane
branch
from
June 1, 2026 11:26
3900a4f to
485da1a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
qa/support_vm_preflight.py (2)
723-733:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
required_toolsin the markdown RRI plan output.The JSON report includes lane-specific
required_tools, but the markdown summary currently omits it, which makes quick operator review less reliable.📝 Proposed fix
f"- Provider: `{report['rri_plan']['provider']}`", f"- Player agent: `{report['rri_plan']['player_agent']}`", + f"- Required tools: `{','.join(report['rri_plan'].get('required_tools', []))}`", f"- Support VM scope: `{report['rri_plan']['support_vm_scope']}`",🤖 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 `@qa/support_vm_preflight.py` around lines 723 - 733, The RRI markdown output omits the lane-specific required_tools from report['rri_plan']; add a new list entry using report['rri_plan'].get('required_tools', []) (e.g. f"- Required tools: `{','.join(report['rri_plan'].get('required_tools', []))}`") in the same block that builds the RRI Plan (near the existing f"- Support VM scope" and f"- Do not run on support VM" lines) so the rendered markdown includes the required_tools; ensure you use .get to avoid KeyError when the key is absent.
474-487:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden Codex auth-status classification to avoid treating “not signed in” as authenticated
Lines 474-487:positive_authincludes"signed in"whilenegative_authomits"not signed in", so an unauthenticated message containing the substring can be classified ascodex["auth_status"] = "proven"whenresult.get("ok")is true. Public docs don’t clearly pin the exactcodex auth statusoutput phrases, so cover common negated wording defensively.🔧 Proposed fix
- negative_auth = ("not authenticated", "unauthenticated", "not logged in", "signed out", "inactive") + negative_auth = ( + "not authenticated", + "unauthenticated", + "not logged in", + "not signed in", + "signed out", + "inactive", + ) positive_auth = ("authenticated", "logged in", "signed in")🤖 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 `@qa/support_vm_preflight.py` around lines 474 - 487, The auth classification is vulnerable because positive_auth contains "signed in" while negative_auth is missing common negated phrases; update the negative_auth tuple used in the block (negative_auth, positive_auth, has_auth_marker) to include phrases like "not signed in" and other common negations (e.g., "not signed in", "not signed") so has_auth_marker(lower, negative_auth) will catch negated messages before positive matches; keep the existing order of checks (negative first) and only modify the negative_auth entries referenced in this code.
🤖 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.
Outside diff comments:
In `@qa/support_vm_preflight.py`:
- Around line 723-733: The RRI markdown output omits the lane-specific
required_tools from report['rri_plan']; add a new list entry using
report['rri_plan'].get('required_tools', []) (e.g. f"- Required tools:
`{','.join(report['rri_plan'].get('required_tools', []))}`") in the same block
that builds the RRI Plan (near the existing f"- Support VM scope" and f"- Do not
run on support VM" lines) so the rendered markdown includes the required_tools;
ensure you use .get to avoid KeyError when the key is absent.
- Around line 474-487: The auth classification is vulnerable because
positive_auth contains "signed in" while negative_auth is missing common negated
phrases; update the negative_auth tuple used in the block (negative_auth,
positive_auth, has_auth_marker) to include phrases like "not signed in" and
other common negations (e.g., "not signed in", "not signed") so
has_auth_marker(lower, negative_auth) will catch negated messages before
positive matches; keep the existing order of checks (negative first) and only
modify the negative_auth entries referenced in this code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0908508c-26ba-45c5-919e-351e17035a76
📒 Files selected for processing (7)
WorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdqa/QA_TOOLS.mdqa/support_vm_preflight.pyqa/test_macos_app_static.pyqa/test_support_vm_preflight.pyqa/ui_playtest_app.sh
✅ Files skipped from review due to trivial changes (1)
- qa/QA_TOOLS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- qa/test_macos_app_static.py
- qa/test_support_vm_preflight.py
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
qa/ui_playtest_app.shPart B honorWOS_APP_SELECTED_PROVIDER=codexandWOS_APP_PLAYER_AGENT=codexrequired_toolsin markdown and hardening negated Codex auth parsingTests
bash -n qa/ui_playtest_app.sh scripts/play_codex_dm.sh scripts/play_party.shpython3 -m py_compile qa/support_vm_preflight.pypython3 -m pytest qa/test_support_vm_preflight.py qa/test_macos_app_static.py -qgit diff --checkNotes
This is not release evidence and does not run the support VM. It closes a preflight/harness mismatch so #466 can intentionally run the non-Claude VM persona lane after operator-approved VM sync/auth setup.
Licensing / CLA
Summary by CodeRabbit
New Features
Documentation
Tests