feat(qa): idle-gap companion prefetch behind WORLDOS_COMPANION_PREFETCH (Wave-2 increment 3, default OFF)#1025
Open
100yenadmin wants to merge 1 commit into
Open
feat(qa): idle-gap companion prefetch behind WORLDOS_COMPANION_PREFETCH (Wave-2 increment 3, default OFF)#1025100yenadmin wants to merge 1 commit into
100yenadmin wants to merge 1 commit into
Conversation
…CH (Wave-2 increment 3, default OFF)
Per-beat latency in scripts/play_party.sh is OUTPUT-DECODE-bound (~57 tok/s; a
single Sonnet companion turn is ~35s of decode), and today that decode only
STARTS after the human posts their move. But between the DM narrating beat-K and
the human posting, the wrapper is just WAITING in the idle `sleep 2` loop while
the human reads + decides — dead decode time.
This reclaims that free think-gap: the instant beat-K's narration is recorded
(after record_dm_reply + soft_tick), speculatively kick the single companion's
beat-(K+1) reaction — reacting to beat-K's NARRATION ONLY (it cannot see the
human's not-yet-posted move) — into its own moves file via the existing
`turn actor` facade path, OVERLAPPING the ~35s decode with the human's think-time.
When the human's move lands, if a valid prefetch is present we REUSE it (skip the
fresh decode → the ~35s was already spent during think-time); otherwise we decode
normally (== today's latency, the worst case).
Invalidation guard (correctness > speed; when unsure, RE-DECODE):
- NON-COMBAT only — combat is initiative-sequential, so a companion can't
pre-decide its turn before the human's; we never prefetch during/into combat.
- SINGLE companion (N==1) only — no multi-companion ordering speculation.
- Re-decode (discard) if, between the kick and the human's move: combat started,
the current location changed, the human's move addresses the companion by name,
or the speculative turn produced no moves. Discard truncates the stale
speculative lines back out of the moves file so they are NEVER relayed.
Cost/turn accounting: the speculative turn runs through the SAME `turn actor`
path (stream → $COMBINED) and counts AGENT_TURNS exactly once at kick time —
whether later reused OR discarded (its decode was spent honestly either way); the
reuse path does not re-count. One writer at a time: the prefetch is a lone
`claude -p` issued in the think-gap when nothing else runs (no DM turn; the human
is a person) — never &-backgrounded, no added concurrency, no OOM risk.
Flag DEFAULT OFF and flag-OFF path byte-identical: all new logic is behind
`[ "${WORLDOS_COMPANION_PREFETCH:-0}" = 1 ]` guards (or inert new globals).
actor_move gains an optional 7th skip_decode arg (default empty → today's decode
for every 6-arg caller); companion_moves' new reuse branch is unreachable unless
the flag is set AND a valid prefetch exists. Verified: bash -n clean, shellcheck
clean (only the 2 pre-existing SC1091 notes), and a runtime trace shows the
flag-off companion_moves/actor_move path is byte-identical to origin/main
(same relayed block, same AGENT_TURNS, same single `turn actor` decode).
Out of scope (unchanged): qa/run_party.sh (its "player" is an AI agent, so a
prefetch there would add real concurrency). Engine remains sole writer; DM
remains sole narrator/resolver; the prefetch changes only WHEN the existing
single companion turn is issued, never the trust boundary.
📝 WalkthroughWalkthrough
ChangesSpeculative Companion Prefetch
Sequence DiagramsequenceDiagram
participant Human
participant BeatLoop
participant _maybe_prefetch_companion
participant _prefetch_valid
participant actor_move
rect rgba(100, 149, 237, 0.5)
note over BeatLoop,_maybe_prefetch_companion: Cold-open / beat end
BeatLoop->>_maybe_prefetch_companion: kick speculative turn (non-combat, single companion)
_maybe_prefetch_companion->>actor_move: turn actor (background, saves cursor+location)
actor_move-->>_maybe_prefetch_companion: moves written to moves file
end
Human->>BeatLoop: submit move (pmsg)
rect rgba(144, 238, 144, 0.5)
note over BeatLoop,actor_move: Validate and consume or discard
BeatLoop->>_prefetch_valid: pmsg
alt valid (no combat/location change, companion not addressed)
_prefetch_valid-->>BeatLoop: PREFETCH_REUSE set
BeatLoop->>actor_move: skip_decode=1 (harvest prefetched moves)
else invalid
_prefetch_valid->>BeatLoop: discard (truncate moves file)
BeatLoop->>actor_move: full decode
end
end
BeatLoop->>BeatLoop: reset PREFETCH_REUSE after DM beat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
🤖 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 `@scripts/play_party.sh`:
- Around line 577-581: The speculative pre-move turn on line 577 uses the
canonical companion session (COMP_SIDS[0]), but the discard mechanism
(_discard_prefetch) only truncates the moves file without fully resetting the
session state, leaving stale speculative prompts in the transcript. Replace the
use of the canonical session (COMP_SIDS[0]) with a separate disposable or
promotable actor session for the prefetch turn, ensuring that
invalidation/discarding can cleanly reset state without affecting the canonical
transcript. This change should also be applied to the other speculative turn
calls mentioned in the comment (around lines 625-640).
- Around line 608-616: The prefetch invalidation logic has two issues: the
location check uses a condition that skips validation when PREFETCH_LOC is empty
(should fail closed instead), and the grep command treats the companion name as
a regex pattern which can cause false matches if the name contains
metacharacters. Fix the first condition around the location check to remove the
`[ -n "$PREFETCH_LOC" ]` guard so the check happens even when location is
unknown and invalidates the prefetch. Replace the `grep -qi --
"${COMP_NAMES[0]}"` call with `grep -qFi -- "${COMP_NAMES[0]}"` to use
fixed-string matching (the -F flag) instead of regex pattern matching.
🪄 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: fc298782-2b77-4c41-9889-e44e902e1674
📒 Files selected for processing (1)
scripts/play_party.sh
Comment on lines
+577
to
+581
| turn actor "${COMP_SIDS[0]}" 0 "The DM says: | ||
|
|
||
| $dm_says | ||
|
|
||
| This is a SPECULATIVE pre-move: the human player has not acted yet. React to the scene as it stands — take your next action(s) using your tools (say / do / request_check / cast_spell / use_item / attack; look or my_sheet first if useful). Tools only, no narration." "${COMP_CFGS[0]}" >/dev/null |
There was a problem hiding this comment.
Isolate speculative actor sessions before discard can be trusted.
Line 577 resumes the canonical companion session for the speculative turn (first=0 maps to --resume "$sid" in turn()), but _discard_prefetch only truncates the moves file. After any invalidation, the “fresh” decode still resumes a transcript that already contains the stale speculative prompt/tool turn, so discard is not equivalent to the non-prefetch path. Use a disposable/promotable actor session for prefetch, or add explicit rollback/commit semantics before enabling invalidation.
Also applies to: 625-640
🤖 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_party.sh` around lines 577 - 581, The speculative pre-move turn
on line 577 uses the canonical companion session (COMP_SIDS[0]), but the discard
mechanism (_discard_prefetch) only truncates the moves file without fully
resetting the session state, leaving stale speculative prompts in the
transcript. Replace the use of the canonical session (COMP_SIDS[0]) with a
separate disposable or promotable actor session for the prefetch turn, ensuring
that invalidation/discarding can cleanly reset state without affecting the
canonical transcript. This change should also be applied to the other
speculative turn calls mentioned in the comment (around lines 625-640).
Comment on lines
+608
to
+616
| if [ -n "$PREFETCH_LOC" ] && [ "$loc" != "$PREFETCH_LOC" ]; then | ||
| echo "[play-party] prefetch: INVALID — location changed ($PREFETCH_LOC → $loc) since the prefetch; re-decoding." >&2 | ||
| return 1 | ||
| fi | ||
| # The human's move directly addresses/targets THIS companion → the speculative reaction (to narration, | ||
| # not to the human) is likely stale. Case-insensitive whole-name substring match on the companion's name. | ||
| if printf '%s' "$pmsg" | grep -qi -- "${COMP_NAMES[0]}"; then | ||
| echo "[play-party] prefetch: INVALID — your move addresses ${COMP_NAMES[0]} directly; re-decoding so it reacts to you." >&2 | ||
| return 1 |
There was a problem hiding this comment.
Make prefetch invalidation fail closed and match names literally.
The location check skips invalidation when the kick-time location is empty, and grep -qi treats the companion name as a regex. That can reuse a stale prefetch when location is unknown or when a name contains regex metacharacters. Prefer failing closed on missing locations and using fixed-string matching for names.
Proposed fix
- if [ -n "$PREFETCH_LOC" ] && [ "$loc" != "$PREFETCH_LOC" ]; then
+ if [ -z "${PREFETCH_LOC:-}" ] || [ -z "${loc:-}" ] || [ "$loc" != "$PREFETCH_LOC" ]; then
echo "[play-party] prefetch: INVALID — location changed ($PREFETCH_LOC → $loc) since the prefetch; re-decoding." >&2
return 1
fi
@@
- if printf '%s' "$pmsg" | grep -qi -- "${COMP_NAMES[0]}"; then
+ if [ -n "${COMP_NAMES[0]:-}" ] && printf '%s' "$pmsg" | grep -Fqi -- "${COMP_NAMES[0]}"; then
echo "[play-party] prefetch: INVALID — your move addresses ${COMP_NAMES[0]} directly; re-decoding so it reacts to you." >&2
return 1
fi📝 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
| if [ -n "$PREFETCH_LOC" ] && [ "$loc" != "$PREFETCH_LOC" ]; then | |
| echo "[play-party] prefetch: INVALID — location changed ($PREFETCH_LOC → $loc) since the prefetch; re-decoding." >&2 | |
| return 1 | |
| fi | |
| # The human's move directly addresses/targets THIS companion → the speculative reaction (to narration, | |
| # not to the human) is likely stale. Case-insensitive whole-name substring match on the companion's name. | |
| if printf '%s' "$pmsg" | grep -qi -- "${COMP_NAMES[0]}"; then | |
| echo "[play-party] prefetch: INVALID — your move addresses ${COMP_NAMES[0]} directly; re-decoding so it reacts to you." >&2 | |
| return 1 | |
| if [ -z "${PREFETCH_LOC:-}" ] || [ -z "${loc:-}" ] || [ "$loc" != "$PREFETCH_LOC" ]; then | |
| echo "[play-party] prefetch: INVALID — location changed ($PREFETCH_LOC → $loc) since the prefetch; re-decoding." >&2 | |
| return 1 | |
| fi | |
| # The human's move directly addresses/targets THIS companion → the speculative reaction (to narration, | |
| # not to the human) is likely stale. Case-insensitive whole-name substring match on the companion's name. | |
| if [ -n "${COMP_NAMES[0]:-}" ] && printf '%s' "$pmsg" | grep -Fqi -- "${COMP_NAMES[0]}"; then | |
| echo "[play-party] prefetch: INVALID — your move addresses ${COMP_NAMES[0]} directly; re-decoding so it reacts to you." >&2 | |
| return 1 |
🤖 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_party.sh` around lines 608 - 616, The prefetch invalidation
logic has two issues: the location check uses a condition that skips validation
when PREFETCH_LOC is empty (should fail closed instead), and the grep command
treats the companion name as a regex pattern which can cause false matches if
the name contains metacharacters. Fix the first condition around the location
check to remove the `[ -n "$PREFETCH_LOC" ]` guard so the check happens even
when location is unknown and invalidates the prefetch. Replace the `grep -qi --
"${COMP_NAMES[0]}"` call with `grep -qFi -- "${COMP_NAMES[0]}"` to use
fixed-string matching (the -F flag) instead of regex pattern matching.
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.
Wave-2 increment 3 (from the first-principles decision record). The measured latency lever for our decode-bound beats (~57 tok/s) is to overlap the companion's ~35 s decode with the human's free think-time — the gap in
scripts/play_party.shwhere the wrapper waits for the human to post their browser move.Mechanism (flag-gated, DEFAULT OFF)
The instant the DM's beat-K narration is recorded, speculatively decode the single companion's reaction to that narration into its moves file (via the existing
actor_move/facade path). When the human's next move arrives, if the speculative move is still valid, reuse it (skip the fresh decode); else discard + re-decode (== today). Because the human reads the narration from the browser independently of the wrapper, their think-time overlaps the decode either way → per-beat wall-clock becomesmax(think, decode)instead ofthink + decode, saving up to one full companion decode. The kick is blocking, never&-backgrounded → exactly oneclaude -pruns alone during the human gap (the DM turn is done; the human is a person) → no added concurrency, no OOM risk.Safety
git diff+ a runtime trace):actor_movegains an optional 7thskip_decodearg (existing 6-arg callers unchanged); thecompanion_movesreuse branch'selseis today's exactcm=…; AGENT_TURNS++; all helpers (_maybe_prefetch_companion/_prefetch_valid/_discard_prefetch) early-return when the flag is off; new globals are inert.play_party.sh(N=1);run_party.shuntouched (its AI player would add concurrency).bash -n+shellcheckclean (only pre-existing SC1091).Verification status
Structural only (host just OOM'd; no heavy runs). The measured latency win + the companion-quality check are deferred: ship-dark (flag default OFF) until (a) the increment-1 Tier-1-vs-Tier-2 quality/cost A/B runs on the support VM, and (b) an interactive
play_partysession with the flag ON confirms the ~one-decode wall-clock drop with identical story score and clean budget accounting.Summary by CodeRabbit
Note: These changes are primarily internal improvements with no direct impact on user-facing gameplay features.