fix(coldopen): guard player-PC seating + raise Part-A poll window (no_actor reliability) - #570
Merged
Merged
Conversation
…_actor reliability)
ISSUE 1 (no_actor): the .app drives scripts/play_party.sh, which pre-seeds
companions but leaves the human's PC to the DM's cold-open turn — DM-stochastic.
Forensic run g1 (veteran persona) minted can_act:true but the DM ended after
start_world without ever calling create_character(kind=player): party=[],
characters={NPCs only}, chat empty → viewer readiness=degraded /
failure_bucket=no_actor ("no active player actor is seated"), an unplayable
surface the persona can't escape. (A prior newbie run DID seat — stochastic miss.)
Fix: (a) strengthen the cold-open prompt to make seating the PC the FIRST,
mandatory tool action (create_character kind=player, add_to_party); (b) add a
snapshot-backed seating GUARD after the cold open that mirrors viewer/server.py
_action_actor (a party member whose record is kind=player) — if unseated, retry
the cold open ONCE on a fresh session with a hard seat directive, then FAIL LOUD
("COLD-OPEN SEATED NO PC", exit 1) rather than hand a silent no_actor session.
ISSUE 2 (Part-A poll too short): the #356 mint poll was 70 x 3s = 210s, shorter
than a max-effort cold open (~280-400s, WORLDOS_COLDOPEN_TIMEOUT=400) → spurious
FAIL on a slow-but-healthy mint. Raise to a 420s-default, env-overridable knob
(WOS_APP_PART_A_DEADLINE); the poll loop now derives its iteration count from it.
Tests: two static regressions in test_adversarial_release.py lock both fixes.
bash -n + py_compile clean; guard validated against the real g1 (unseated) and
g1-b (seated, PC Zevlor) snapshots.
📝 WalkthroughWalkthroughThis PR strengthens the playtest setup reliability by making the mint-polling window deadline-driven (instead of fixed) and ensuring the player character is always seated during cold-open with automatic retry and explicit failure handling. ChangesPlaytest Session Initialization Reliability
Possibly Related PRs
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
servers/engine/tests/test_adversarial_release.py (1)
193-199: ⚡ Quick winTighten this regression around the actual guard branch.
These assertions only prove the file contains the substrings somewhere. The test can still pass if the loud-fail branch loses its
exit 1, or if the seated-PC check drifts away frompartymembership while anotherget("kind") == "player"remains elsewhere.Suggested fix
+import re + def test_coldopen_play_party_guards_player_pc_seating(): @@ - assert 'get("kind") == "player"' in text + assert re.search( + r"pc_seated\(\).*?party.*?get\(\"kind\"\)\s*==\s*\"player\"", + text, + re.S, + ) @@ - assert "COLD-OPEN SEATED NO PC" in text and "exit 1" in text + assert re.search(r"COLD-OPEN SEATED NO PC.*?exit 1", text, re.S)🤖 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_adversarial_release.py` around lines 193 - 199, The current assertions only check for scattered substrings; instead locate the actual guard branch in the generated text and assert the required tokens appear together and in order. For example, find the contiguous guard block that contains "pc_seated()", then assert within that block you see 'get("kind") == "player"', that the block contains "retrying the cold open ONCE" followed by "COLD-OPEN SEATED NO PC", and that "exit 1" appears inside the same block (or within a small proximity window). Use an indexed search or a regex that matches the sequence ("pc_seated" ... 'get("kind") == "player"' ... "retrying the cold open ONCE" ... "COLD-OPEN SEATED NO PC" ... "exit 1") to ensure the test fails if the loud-fail branch or the seated-PC check drifts away.
🤖 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`:
- Line 78: PART_A_DEADLINE is taken directly from WOS_APP_PART_A_DEADLINE and
used in arithmetic to compute part_a_polls, which can become 0 if the env var is
non-numeric or non-positive; validate and sanitize the override by parsing it as
an integer (e.g., use a safe integer parse/regex) and ensure it is >0 before
using it, otherwise fall back to the default 420; then use the
validated/normalized PART_A_DEADLINE value in the existing part_a_polls
calculation and subsequent clamp so Part A cannot collapse to a single poll.
In `@servers/engine/tests/test_adversarial_release.py`:
- Around line 203-205: The module docstring contains ambiguous Unicode
punctuation (an en dash and a multiplication sign) causing RUF002; replace the
en dash in "280–400s" with an ASCII hyphen ("280-400s") and replace the
multiplication sign in "70 × 3s" with an ASCII "x" or plain asterisk (e.g., "70
x 3s" or "70 * 3s") so the docstring in test_adversarial_release.py uses only
ASCII punctuation.
---
Nitpick comments:
In `@servers/engine/tests/test_adversarial_release.py`:
- Around line 193-199: The current assertions only check for scattered
substrings; instead locate the actual guard branch in the generated text and
assert the required tokens appear together and in order. For example, find the
contiguous guard block that contains "pc_seated()", then assert within that
block you see 'get("kind") == "player"', that the block contains "retrying the
cold open ONCE" followed by "COLD-OPEN SEATED NO PC", and that "exit 1" appears
inside the same block (or within a small proximity window). Use an indexed
search or a regex that matches the sequence ("pc_seated" ... 'get("kind") ==
"player"' ... "retrying the cold open ONCE" ... "COLD-OPEN SEATED NO PC" ...
"exit 1") to ensure the test fails if the loud-fail branch or the seated-PC
check drifts away.
🪄 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: 5cbaf806-5c04-4b1e-9493-28c59243d40b
📒 Files selected for processing (3)
qa/ui_playtest_app.shscripts/play_party.shservers/engine/tests/test_adversarial_release.py
| # the old 210s poll (70 × 3s) was SHORTER than a max-effort cold open, so a slow-but-healthy | ||
| # mint timed out as a spurious FAIL. Give the poll a 420s budget (just past the cold-open | ||
| # deadline), env-overridable for fast inner loops. | ||
| PART_A_DEADLINE="${WOS_APP_PART_A_DEADLINE:-420}" |
There was a problem hiding this comment.
Validate WOS_APP_PART_A_DEADLINE before using it in arithmetic.
A non-numeric or non-positive override can collapse part_a_polls to 0, and the clamp then turns Part A into a single poll. That reintroduces the same spurious FAIL mode this change is fixing.
Suggested fix
PART_A_DEADLINE="${WOS_APP_PART_A_DEADLINE:-420}"
+case "$PART_A_DEADLINE" in
+ ''|*[!0-9]*)
+ printf '[uipt-app] WOS_APP_PART_A_DEADLINE must be a positive integer (got %s)\n' "$PART_A_DEADLINE" >&2
+ exit 2
+ ;;
+esac
+[ "$PART_A_DEADLINE" -ge 1 ] || {
+ printf '[uipt-app] WOS_APP_PART_A_DEADLINE must be >= 1 (got %s)\n' "$PART_A_DEADLINE" >&2
+ exit 2
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PART_A_DEADLINE="${WOS_APP_PART_A_DEADLINE:-420}" | |
| PART_A_DEADLINE="${WOS_APP_PART_A_DEADLINE:-420}" | |
| case "$PART_A_DEADLINE" in | |
| ''|*[!0-9]*) | |
| printf '[uipt-app] WOS_APP_PART_A_DEADLINE must be a positive integer (got %s)\n' "$PART_A_DEADLINE" >&2 | |
| exit 2 | |
| ;; | |
| esac | |
| [ "$PART_A_DEADLINE" -ge 1 ] || { | |
| printf '[uipt-app] WOS_APP_PART_A_DEADLINE must be >= 1 (got %s)\n' "$PART_A_DEADLINE" >&2 | |
| exit 2 | |
| } |
🤖 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` at line 78, PART_A_DEADLINE is taken directly from
WOS_APP_PART_A_DEADLINE and used in arithmetic to compute part_a_polls, which
can become 0 if the env var is non-numeric or non-positive; validate and
sanitize the override by parsing it as an integer (e.g., use a safe integer
parse/regex) and ensure it is >0 before using it, otherwise fall back to the
default 420; then use the validated/normalized PART_A_DEADLINE value in the
existing part_a_polls calculation and subsequent clamp so Part A cannot collapse
to a single poll.
Comment on lines
+203
to
+205
| """Cold-open reliability: the Part-A (#356) mint poll must outlast the max-effort cold open | ||
| (~280–400s). The old 210s window (70 × 3s) was a spurious FAIL; the deadline is now a | ||
| 420s-default, env-overridable knob.""" |
There was a problem hiding this comment.
Replace the ambiguous unicode punctuation in the docstring.
Ruff is already flagging the en dash and multiplication sign here (RUF002), so this adds avoidable lint noise.
Suggested fix
- """Cold-open reliability: the Part-A (`#356`) mint poll must outlast the max-effort cold open
- (~280–400s). The old 210s window (70 × 3s) was a spurious FAIL; the deadline is now a
+ """Cold-open reliability: the Part-A (`#356`) mint poll must outlast the max-effort cold open
+ (~280-400s). The old 210s window (70 x 3s) was a spurious FAIL; the deadline is now a📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Cold-open reliability: the Part-A (#356) mint poll must outlast the max-effort cold open | |
| (~280–400s). The old 210s window (70 × 3s) was a spurious FAIL; the deadline is now a | |
| 420s-default, env-overridable knob.""" | |
| """Cold-open reliability: the Part-A (`#356`) mint poll must outlast the max-effort cold open | |
| (~280-400s). The old 210s window (70 x 3s) was a spurious FAIL; the deadline is now a | |
| 420s-default, env-overridable knob.""" |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 204-204: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 204-204: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
🤖 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_adversarial_release.py` around lines 203 - 205, The
module docstring contains ambiguous Unicode punctuation (an en dash and a
multiplication sign) causing RUF002; replace the en dash in "280–400s" with an
ASCII hyphen ("280-400s") and replace the multiplication sign in "70 × 3s" with
an ASCII "x" or plain asterisk (e.g., "70 x 3s" or "70 * 3s") so the docstring
in test_adversarial_release.py uses only ASCII punctuation.
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.
Fixes two cold-open RELIABILITY issues blocking the WorldOS .app gate.
ISSUE 1 —
no_actor(no PC seated)Root cause (forensic, from run
g1): the .app drivesscripts/play_party.sh(ProviderAdapters.swift:62), which pre-seeds the companions but deliberately leaves the human's PC to be created live by the DM's cold-open turn (play.sh-parity "the DM hands you a character" feel). That makes PC seating DM-stochastic. In theg1veteran run the DM mintedcan_act:trueearly (move-sink + campaign bound) but its cold-open turn ended afterstart_worldwithout ever callingcreate_character(kind="player")— leavingparty=[],characters={NPCs only},chat.jsonlempty. The viewer then reportsreadiness=degraded/failure_bucket="no_actor"("no active player actor is seated") — an unplayable surface the persona can't escape (viewer/server.py_action_actorreturnsNonewhen no party member iskind="player"). A prior newbie run DID seat (PC Rolan), confirming a stochastic miss, not a code break.Fix (guard over prompt-only, per the brief):
play_party.sh) so seating the PC is the FIRST, mandatory tool action —create_characterwithkind="player"+add_to_party=true, with explicit "a cold open that ends with no seated PC is a BROKEN session" language.viewer/server.py_action_actor(a seated PC = apartymember whose record iskind="player"). If unseated → retry the cold open ONCE on a fresh session id with a hard seat-only directive (mirrors the existing DM-turn retry pattern); if still unseated → FAIL LOUD (COLD-OPEN SEATED NO PC,exit 1) rather than hand the player a silentno_actor/degradedsession.The guard was validated against the real
g1snapshot (unseated → fires) and theg1-bsnapshot (PC Zevlor seated → passes through).ISSUE 2 — Part-A poll window too short
The native #356 / cold-open mint poll in
qa/ui_playtest_app.shwas70 × 3s = 210s, shorter than a max-effort cold open (~280–400s;WORLDOS_COLDOPEN_TIMEOUT=400) → spurious FAIL on a slow-but-healthy mint. Raised to a 420s default via a new env-overridable knobWOS_APP_PART_A_DEADLINE; the poll loop now derives its iteration count from the deadline (no more hardcodedseq 1 70).Tests / verification
servers/engine/tests/test_adversarial_release.pylock both fixes.bash -n(both scripts) +py_compile(guard python + test file) clean.test_issue50,test_dm_session_remint,test_macos_app_static) re-confirmed green.Scope: no gateway/wire-contract changes.
Summary by CodeRabbit
Bug Fixes
Tests