[codex] Add Codex DM built-app provider path - #475
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughPR ChangesCodex DM Wrapper & Provider Adapter Routing
Native App Provider Viewer Attachment Across Restarts
Browser Navigation & Roster Catalog Scoping
Documentation & Runbook Updates
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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. Comment |
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
macos/WorldOSApp/Sources/WorldOSApp/Services/ProviderAdapters.swift (1)
111-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-require
play_codex_dm.shfor custom Codex commands.Lines 111-118 and 143-146 fail closed on a missing checked-in wrapper before honoring
preferences.codexCommand, so any custom Codex launch command is broken unlessscripts/play_codex_dm.shalso exists locally.Suggested guard change
- guard FileManager.default.fileExists(atPath: wrapper.path) else { - return ProviderStatus( - kind: kind, - availability: .error, - detail: "Codex CLI found, but scripts/play_codex_dm.sh is missing from this checkout.", - detectedPath: cli - ) - } + if configuredCommand.isEmpty { + guard FileManager.default.fileExists(atPath: wrapper.path) else { + return ProviderStatus( + kind: kind, + availability: .error, + detail: "Codex CLI found, but scripts/play_codex_dm.sh is missing from this checkout.", + detectedPath: cli + ) + } + } @@ - let wrapper = repoPath.appendingPathComponent("scripts/play_codex_dm.sh") - guard FileManager.default.fileExists(atPath: wrapper.path) else { - throw ProviderError.configuration("Codex provider wrapper is missing: scripts/play_codex_dm.sh") - } - let configuredCommand = preferences.codexCommand.trimmingCharacters(in: .whitespacesAndNewlines) + if configuredCommand.isEmpty { + let wrapper = repoPath.appendingPathComponent("scripts/play_codex_dm.sh") + guard FileManager.default.fileExists(atPath: wrapper.path) else { + throw ProviderError.configuration("Codex provider wrapper is missing: scripts/play_codex_dm.sh") + } + } let command = configuredCommand.isEmpty ? defaultCodexCommand : configuredCommandAlso applies to: 143-149
🤖 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 `@macos/WorldOSApp/Sources/WorldOSApp/Services/ProviderAdapters.swift` around lines 111 - 126, The code currently always requires the checked-in wrapper (wrapper.path) before honoring a user-configured Codex command, breaking custom commands; update the logic so the FileManager.exists guard only runs when configuredCommand.isEmpty (i.e., no user command), and when a user command is provided set ProviderStatus.detectedPath to that configuredCommand (not the wrapper or cli); apply the same conditional change in the other occurrence (the block around the later 143-149 code) so custom preferences.codexCommand are accepted without the wrapper file.
🧹 Nitpick comments (1)
servers/engine/tests/test_codex_provider_wrapper.py (1)
48-56: ⚡ Quick winUse timeout-aware
_run_dmcalls for runtime DM tests.One runtime test invokes the wrapper without a timeout, which can hang CI on regressions. Also, the second runtime test bypasses
_run_dm, making invocation behavior inconsistent.Proposed patch
-def _run_dm(args: list[str], env: dict[str, str]) -> subprocess.CompletedProcess: +def _run_dm( + args: list[str], env: dict[str, str], timeout: float | None = None +) -> subprocess.CompletedProcess: return subprocess.run( ["/bin/bash", str(DM_SCRIPT), *args], cwd=ROOT, env=env, capture_output=True, check=False, text=True, + timeout=timeout, ) @@ - result = _run_dm([], env) + result = _run_dm([], env, timeout=20) @@ - result = subprocess.run( - ["/bin/bash", str(DM_SCRIPT)], - cwd=ROOT, - env=env, - capture_output=True, - check=False, - text=True, - timeout=20, - ) + result = _run_dm([], env, timeout=20)Also applies to: 197-197, 246-254
🤖 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 `@servers/engine/tests/test_codex_provider_wrapper.py` around lines 48 - 56, The _run_dm helper currently invokes the wrapper without a timeout and some tests call the wrapper directly, risking hangs; update _run_dm (function _run_dm) to accept a timeout parameter (e.g., timeout: float | None) and pass it to subprocess.run(timeout=timeout), then change the runtime tests that currently call subprocess.run directly (and the runtime test at the second invocation mentioned) to use _run_dm with a reasonable timeout value (e.g., 10s) so all invocations of DM_SCRIPT go through the timeout-aware _run_dm helper; keep DM_SCRIPT and existing args/env usage unchanged.
🤖 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 `@viewer/server.py`:
- Around line 6198-6202: catalog snapshot handling is incorrectly allowing
build_roster_response to fall back to _roster_world_for_campaign when raw_snap
lacks a world_id; instead, extract world_id from catalog_ref only when
present/non-empty and pass that explicit value to build_roster_response (do not
pass an empty string), and update build_roster_response to accept a
None/explicit-missing sentinel for world_id and avoid calling
_roster_world_for_campaign when that sentinel is given; look for the
catalog_ref/raw_snap extraction and the
build_roster_response/_roster_world_for_campaign callers to implement this
guarded-passing behavior.
In `@WorldOS-GUI-RUNBOOK.md`:
- Around line 35-38: Update the sentence that references "2026-06-01" so it
includes an absolute timestamp source or timezone to avoid appearing
future-dated; for example append "recorded at 2026-06-01 UTC" or "recorded at
2026-06-01, local time UTC−07" (or indicate the proof was taken from a local
build log with its ISO8601 timestamp). Edit the paragraph containing the date
string "2026-06-01" to add that explicit timezone/timestamp notation or log
source (e.g., "recorded at <ISO8601> UTC / build log") so operators cannot
misinterpret recency.
In `@WorldOS-OPERATING-GOAL.md`:
- Around line 8-10: Update the "AS OF" and associated state-of-truth entries to
use an explicit ISO 8601 timestamp with local timezone offset instead of a
date-only string (e.g., replace "2026-06-01" with "2026-06-01T00:00:00-07:00" or
the actual local time used) so the canonical operational source is unambiguous;
apply the same change to the related entries in the MAIN BASELINE block and the
other occurrences noted (lines 27–33) so all baseline/proof timestamps include
full date-time plus timezone and retain the existing commit/hash text.
---
Outside diff comments:
In `@macos/WorldOSApp/Sources/WorldOSApp/Services/ProviderAdapters.swift`:
- Around line 111-126: The code currently always requires the checked-in wrapper
(wrapper.path) before honoring a user-configured Codex command, breaking custom
commands; update the logic so the FileManager.exists guard only runs when
configuredCommand.isEmpty (i.e., no user command), and when a user command is
provided set ProviderStatus.detectedPath to that configuredCommand (not the
wrapper or cli); apply the same conditional change in the other occurrence (the
block around the later 143-149 code) so custom preferences.codexCommand are
accepted without the wrapper file.
---
Nitpick comments:
In `@servers/engine/tests/test_codex_provider_wrapper.py`:
- Around line 48-56: The _run_dm helper currently invokes the wrapper without a
timeout and some tests call the wrapper directly, risking hangs; update _run_dm
(function _run_dm) to accept a timeout parameter (e.g., timeout: float | None)
and pass it to subprocess.run(timeout=timeout), then change the runtime tests
that currently call subprocess.run directly (and the runtime test at the second
invocation mentioned) to use _run_dm with a reasonable timeout value (e.g., 10s)
so all invocations of DM_SCRIPT go through the timeout-aware _run_dm helper;
keep DM_SCRIPT and existing args/env usage unchanged.
🪄 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: e5fcc3b1-ca4d-4d60-86a6-1f0b759cface
📒 Files selected for processing (16)
WorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdWorldOS-RUNBOOK.mdmacos/WorldOSApp/Sources/WorldOSApp/Services/AppProcessService.swiftmacos/WorldOSApp/Sources/WorldOSApp/Services/ProviderAdapters.swiftmacos/WorldOSApp/Sources/WorldOSApp/Views/RootView.swiftqa/test_macos_app_static.pyscripts/play_codex_actor.shscripts/play_codex_dm.shservers/engine/tests/test_codex_provider_wrapper.pyviewer/openworlds/screen-create.jsxviewer/openworlds/screen-launcher.jsxviewer/openworlds/screen-roster.jsxviewer/server.pyviewer/tests/test_openworlds_static.pyviewer/tests/test_roster_surface.py
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 `@servers/engine/tests/test_codex_provider_wrapper.py`:
- Around line 48-57: The _run_dm helper currently uses subprocess.run with
timeout which lets the kernel SIGKILL the shell and prevents
scripts/play_codex_dm.sh trap handlers from running, leaving
viewer_supervisor/viewer/server.py (VPID_FILE) and CLAWDND_PLAY_PORT orphaned;
modify _run_dm to start the bash in a new process group/session (use
preexec_fn=os.setsid or start_new_session=True) via subprocess.Popen, call
communicate() with the timeout, and on TimeoutExpired send a termination signal
to the entire process group (os.killpg with SIGTERM, then SIGKILL if needed),
capture stdout/stderr and return a subprocess.CompletedProcess-like result;
ensure you still reference DM_SCRIPT and preserve cwd=ROOT, env,
capture_output/text behavior so the cleanup trap in scripts/play_codex_dm.sh can
run and kill the viewer supervisor.
🪄 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: ec0a723d-0389-4aaa-b2b9-6417b09e5bdc
📒 Files selected for processing (7)
WorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdWorldOS-RUNBOOK.mdmacos/WorldOSApp/Sources/WorldOSApp/Services/ProviderAdapters.swiftservers/engine/tests/test_codex_provider_wrapper.pyviewer/server.pyviewer/tests/test_roster_surface.py
✅ Files skipped from review due to trivial changes (3)
- WorldOS-RUNBOOK.md
- WorldOS-GUI-RUNBOOK.md
- WorldOS-OPERATING-GOAL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- viewer/server.py
- macos/WorldOSApp/Sources/WorldOSApp/Services/ProviderAdapters.swift
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 350-352: The teardown pkill lines assume the session world equals
$WORLD and thus miss processes when minted sessions come from a different
resumed campaign; update the pkill patterns to decouple from $WORLD and match
only on the minted session identifier and script names instead. Replace
occurrences of pkill -f "play_party.sh $WORLD $minted_run" and pkill -f "play.sh
$WORLD $minted_run" and pkill -f "$WORLD $minted_run " with pkill patterns that
search for "play_party.sh $minted_run", "play.sh $minted_run" and any
supervisor/DM/spend loop process names (e.g., supervisor, dm, spend_loop)
combined with $minted_run (or read PIDs from the minted_run-specific pidfiles if
available) so that teardown reliably kills processes tied to minted_run
regardless of $WORLD.
🪄 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: 2e62d3de-bb25-4579-b0e0-6063fb968b75
📒 Files selected for processing (6)
AGENTS.mdWorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdqa/SCORECARD.mdqa/test_macos_app_static.pyqa/ui_playtest_app.sh
✅ Files skipped from review due to trivial changes (1)
- qa/SCORECARD.md
🚧 Files skipped from review as they are similar to previous changes (2)
- WorldOS-GUI-RUNBOOK.md
- qa/test_macos_app_static.py
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced May 31, 2026
This was referenced May 31, 2026
This was referenced May 31, 2026
100yenadmin
marked this pull request as ready for review
May 31, 2026 22:11
This was referenced May 31, 2026
This was referenced May 31, 2026
100yenadmin
added a commit
that referenced
this pull request
May 31, 2026
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
scripts/play_codex_dm.sh, a real Codex Dungeon Master provider wrapper that owns the live viewer, engine/rules/voice MCP contract,chat.jsonl, andplayer_moves.jsonlscripts/play_codex_actor.shas the constrained player/companion helperspeaker:null, no broad mid-session shell state discovery, no unconfigured solo companions, no startup/social safety-cancelled setup calls, and no opening-turnpersist_beat/app-status//__worldos/app-status.jsonfor agent-facing app truth: build, provider, state root, private art, campaign/run, move sink, actor, enabled actions, and endpointsLicensing / CLA
CLA.mdand submit this contribution under the WorldOS Contributor License Agreement.Built-app evidence
Local
dist/WorldOS.appproof succeeded with a non-Claude Codex DM provider, private Baldur's Gate art, visible narration, enabled actions, submitted player move, and DM response.Clean-tree app-code evidence root:
/Volumes/LEXAR/Codex/worldos-built-app-playtest/codex-app-headproof-20260601T043909/Clean-tree highlights:
8bd833fd292def(docs/scorecard pointers after the app-code proof)play-20260531213923on port8766/app-statusreported providercodex, private art present, Alfira as active player, writable move sink, and five enabled actions/moveaccepted and resolved one player actiondm, player, dm/session-surfaceremained live/actionable withcan_act:trueapp-evidence/manifest.jsonPreserved diagnostic evidence:
/Volumes/LEXAR/Codex/worldos-built-app-playtest/codex-app-socialclean-20260601T041604/: gameplay recovered but still had startup provider cancellations/Volumes/LEXAR/Codex/worldos-built-app-playtest/codex-app-traceclean-20260601T042248/: pre-commit trace-clean proof from the same stabilization slice/Volumes/LEXAR/Codex/worldos-built-app-playtest/codex-app-headproof-20260601T043402/: clean-tree proof before forbidding opening-turnpersist_beat; preserved because it exposed the last Codex DM should avoid safety-cancelled setup/social tool calls #479 cancellationThe Photos/Music prompt visible in one earlier screenshot was traced via unified logs to TCC attribution contamination:
responsible=dev.clawdnd.app, actualaccessing=/usr/bin/find. The GUI runbook documents how to classify that separately from product bugs.Validation
bash -n scripts/play_codex_dm.sh qa/ui_playtest_app.shuv run --directory servers/engine python -m pytest tests/test_codex_provider_wrapper.py -q -p no:xdist(20 passed)python3 -m pytest viewer/tests/test_openworlds_static.py -q(38 passed, 2 subtests passed)python3 -m pytest qa/test_macos_app_static.py qa/test_export_app_evidence.py -q(7 passed)git diff --checkWOS_APP_PART=A WOS_APP_KEEP_MINTED_BACKEND=1 WORLDOS_ART_REPO_ROOT=/Users/lume/ClawDnD-val qa/ui_playtest_app.sh codex-app-headproof-20260601T043909 baldurs-gate newbie 1 3.00curl /app-status,curl /session-surface, provider trace error scanFollow-up
This is product progress, not a release verdict. Next step is #466: a clean non-partial RRI sweep. Mac/local or macOS CI supplies built
.appproof;support-vm-1supplies heavy backend/persona sweeps only after VM preflight/auth setup. The agent-grade app testability lane continues under #480-#486.Refs #466, #467, #474, #476, #477, #478, #479, #480, #481, #485, #486.