Fix Codex DM provider trace cancellations - #500
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 27 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 (2)
📝 WalkthroughWalkthroughCodex DM wrapper now logs narration events directly to the engine instead of relying on chat-row persistence. The wrapper discovers the active campaign ID, calls ChangesCodex DM engine narration logging and viewer deduplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/play_codex_dm.sh (1)
523-533: ⚡ Quick winExtract the duplicated engine-log/chat-write flow into one helper.
The opening and move blocks duplicate the same control flow and warning/chat metadata logic, which increases drift risk when one path changes later.
♻️ Suggested refactor
+record_dm_reply() { + local campaign_id="$1" reply="$2" phase="$3" + if log_engine_narration "$campaign_id" "$reply"; then + chatlog dm "$reply" '{"engine_logged":true}' + else + echo "[codex-dm-provider] warning: could not record ${phase} narration through engine" >&2 + chatlog dm "$reply" + fi +} ... -ENGINE_LOGGED=0 -if log_engine_narration "$ACTIVE_CAMPAIGN_ID" "$OPENING"; then - ENGINE_LOGGED=1 -else - echo "[codex-dm-provider] warning: could not record opening narration through engine" >&2 -fi -if [ "$ENGINE_LOGGED" -eq 1 ]; then - chatlog dm "$OPENING" '{"engine_logged":true}' -else - chatlog dm "$OPENING" -fi +record_dm_reply "$ACTIVE_CAMPAIGN_ID" "$OPENING" "opening" ... -ENGINE_LOGGED=0 -if log_engine_narration "$ACTIVE_CAMPAIGN_ID" "$REPLY"; then - ENGINE_LOGGED=1 -else - echo "[codex-dm-provider] warning: could not record move narration through engine" >&2 -fi -if [ "$ENGINE_LOGGED" -eq 1 ]; then - chatlog dm "$REPLY" '{"engine_logged":true}' -else - chatlog dm "$REPLY" -fi +record_dm_reply "$ACTIVE_CAMPAIGN_ID" "$REPLY" "move"Also applies to: 566-576
🤖 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 `@scripts/play_codex_dm.sh` around lines 523 - 533, The opening/move blocks duplicate the engine-log then chat-write pattern (ENGINE_LOGGED, log_engine_narration, chatlog), so extract that flow into a single helper (e.g., log_engine_and_chat or record_narration_and_chat) that: calls log_engine_narration with the campaign and text, emits the same warning to stderr when it fails, and then calls chatlog with the original message and JSON metadata '{"engine_logged":true}' only when the engine log succeeded; replace both the opening and move blocks with calls to this helper to eliminate duplication and keep behavior identical.
🤖 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.
Nitpick comments:
In `@scripts/play_codex_dm.sh`:
- Around line 523-533: The opening/move blocks duplicate the engine-log then
chat-write pattern (ENGINE_LOGGED, log_engine_narration, chatlog), so extract
that flow into a single helper (e.g., log_engine_and_chat or
record_narration_and_chat) that: calls log_engine_narration with the campaign
and text, emits the same warning to stderr when it fails, and then calls chatlog
with the original message and JSON metadata '{"engine_logged":true}' only when
the engine log succeeded; replace both the opening and move blocks with calls to
this helper to eliminate duplication and keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd753883-a1ad-4031-bb87-566560382f9f
📒 Files selected for processing (8)
WorldOS-GUI-RUNBOOK.mdWorldOS-OPERATING-GOAL.mdWorldOS-RUNBOOK.mdqa/SCORECARD.mdscripts/play_codex_dm.shservers/engine/tests/test_codex_provider_wrapper.pyviewer/openworlds/app.jsxviewer/tests/test_live_narration_stream.py
1 participant
Summary
/chatDM line asengine_loggedso/chatremains the turn-resolution signal without duplicating visible chronicle prose.qa/SCORECARD.mdwith the final built-app Codex DM should avoid safety-cancelled setup/social tool calls #479 evidence.Built-App Evidence
/Volumes/LEXAR/Codex/worldos-built-app-playtest/codex-479-traceclean-nodup-proof-20260601T003002Z/b081092dist/WorldOS.applocal Macplay-20260601003008, campaigncamp_ffd97ffdb6ef, port8767/app-status: private art present at/Users/lume/ClawDnD-val/content/worlds/_private, Alfira active, writable/move,can_act=true, five enabled actions/session-surface: remained live/actionable after one accepted/resolved/moveprovider-errors.after-move.jsonreports zero parse errors and zero failed/error tool calls (completed=10,in_progress=10)Tests
bash -n scripts/play_codex_dm.shpython3 -m pytest servers/engine/tests/test_codex_provider_wrapper.py viewer/tests/test_live_narration_stream.py qa/test_macos_app_static.py -qgit diff --checkqa/ui_playtest_app.shPart A plus one manual/moveRelease Note
This is trace-clean branch diagnostic evidence for #479, not a full RRI release verdict. Release still requires the full #466 five-persona RRI with Mac built-app evidence plus support-VM heavy sweep evidence.
Refs #479. Do not close #479 until maintainers either accept this branch proof as sufficient after merge or a merged-main rerun proves non-reproduction.
Summary by CodeRabbit
Bug Fixes
Improvements