[codex] Fix Codex provider default and app-status busy turns - #589
Conversation
|
Warning Review limit reached
More reviews will be available in 9 minutes and 18 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 (6)
📝 WalkthroughWalkthroughThis PR pins Codex defaults to gpt-5.5 and updates tests; broadens campaign matching to id or campaign_id; derives pending-player-turn from chat logs to gate /app-status actions; and makes QA handoff gating resilient to transient /app-status errors while adding evidence-manifest blocker handling. ChangesCodex Model Default Pinning
Campaign Matching Enhancement
App Status Pending Player Turn Detection
App Status Error Resilience & QA Handoff Gate
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 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
🧹 Nitpick comments (2)
scripts/play_codex_actor.sh (1)
200-210: ⚡ Quick win
gpt-5.5pin matches Codex’s supported ChatGPT-auth model (repo tests expect--model gpt-5.5)The main risk flagged (that
codex exec --model gpt-5.5would reject the slug) doesn’t apply: OpenAI’s Codex model list includesgpt-5.5, andservers/engine/tests/test_codex_provider_wrapper.py::test_codex_dm_wrapper_run_pins_supported_default_model_with_fake_codexasserts the wrapper invokes Codex with--model gpt-5.5when using the default/auto path. The duplication acrossscripts/play_codex_actor.shandscripts/play_codex_dm.sh(both defaulting to...:-gpt-5.5) is also intentional and covered by static QA assertions.Optional: if older Codex CLI versions ever fail to recognize
gpt-5.5, consider falling back toauto/default/cli-default(or emitting a clearer error) instead of hard-failing allMODE=runruns.🤖 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_actor.sh` around lines 200 - 210, The default model selection currently pins CODEX_MODEL to "gpt-5.5" and sets MODEL_ARGS accordingly; keep that pin (matching tests expecting --model gpt-5.5) and explicitly document it in the header comment, and optionally add a silent fallback path that translates unsupported CLI-model errors into using the empty/auto/default branch (e.g., detect unrecognized model and unset CODEX_MODEL so MODEL_ARGS stays empty) to avoid hard-failing MODE=run; update scripts/play_codex_actor.sh (and mirror in scripts/play_codex_dm.sh) to implement the fallback and add a short comment referencing CODEX_MODEL and MODEL_ARGS so the behavior is clear to readers and test authors.qa/test_app_handoff_gate.py (1)
246-318: ⚡ Quick winConsider explicitly asserting
detailis empty.The review context states the test "asserts ok/bucket/detail output," but currently only
okandbucketare explicitly verified. While theassertTrue(ok, detail)pattern is good for debugging (it showsdetailif the assertion fails), it doesn't verifydetail's value whenokis True.✨ Suggested addition
self.assertTrue(ok, detail) self.assertEqual(bucket, "") +self.assertEqual(detail, "") self.assertIn("busy status probe", network)🤖 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/test_app_handoff_gate.py` around lines 246 - 318, Add an explicit assertion that the returned detail is empty in test_drive_moves_tolerates_transient_app_status_timeout: after the existing assertions on ok and bucket, assert that detail == "" (or use an appropriate empty-check) so the test verifies the detail value rather than only using it as the assertTrue message; reference the local variables detail and ok in the test to locate where to insert this assertion.
🤖 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 5425-5433: The loop that reads lines from
Path(path).read_text(...) counts lines before parsing and treats
json.JSONDecodeError as a malformed record; move the JSON parsing ahead of
incrementing summary["line_count"] so you only increment when json.loads(line)
succeeds, and on JSONDecodeError treat the line as an incomplete trailing row
(skip without incrementing or marking narration/chat present). Update the block
that sets payload and summary["line_count"] (referencing variables payload and
summary["line_count"]) so counting happens after isinstance(payload, dict) /
successful parse.
---
Nitpick comments:
In `@qa/test_app_handoff_gate.py`:
- Around line 246-318: Add an explicit assertion that the returned detail is
empty in test_drive_moves_tolerates_transient_app_status_timeout: after the
existing assertions on ok and bucket, assert that detail == "" (or use an
appropriate empty-check) so the test verifies the detail value rather than only
using it as the assertTrue message; reference the local variables detail and ok
in the test to locate where to insert this assertion.
In `@scripts/play_codex_actor.sh`:
- Around line 200-210: The default model selection currently pins CODEX_MODEL to
"gpt-5.5" and sets MODEL_ARGS accordingly; keep that pin (matching tests
expecting --model gpt-5.5) and explicitly document it in the header comment, and
optionally add a silent fallback path that translates unsupported CLI-model
errors into using the empty/auto/default branch (e.g., detect unrecognized model
and unset CODEX_MODEL so MODEL_ARGS stays empty) to avoid hard-failing MODE=run;
update scripts/play_codex_actor.sh (and mirror in scripts/play_codex_dm.sh) to
implement the fallback and add a short comment referencing CODEX_MODEL and
MODEL_ARGS so the behavior is clear to readers and test authors.
🪄 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: bb1f6eae-dc84-452d-9d95-258ec9834007
📒 Files selected for processing (9)
qa/app_handoff_gate.pyqa/test_app_handoff_gate.pyqa/test_macos_app_static.pyscripts/play_codex_actor.shscripts/play_codex_dm.shservers/engine/tests/test_codex_provider_wrapper.pyviewer/openworlds/app.jsxviewer/server.pyviewer/tests/test_openworlds_static.py
1 participant
Summary
auto/CLI default./openworlds/deep links attached to live campaigns when the route uses either catalog id shape./app-statusreflect a pending player move asbusyinstead of reporting the app as playable while the DM turn is still resolving./app-statustimeouts so slow provider turns are classified as narration progress/failure rather than immediate provider failure.Product Impact
/moveevidence connected on the same build./moveintent submitter; no engine campaign-state write authority moves into the viewer.Validation
python3 -m pytest viewer/tests/test_openworlds_static.py -qpython3 -m pytest qa/test_app_handoff_gate.py qa/test_macos_app_static.py -quv run --directory servers/engine --group dev pytest tests/test_codex_provider_wrapper.py -q -p no:xdistbash -n scripts/play_codex_dm.sh scripts/play_codex_actor.shNotes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests