fix(#648): grace window so a just-armed narrating spinner survives a spurious clear - #666
Merged
Merged
Conversation
…spurious clear Symptom (adversarial sweep, [MAJOR], dropped sat 9→4): after Enter-submitting a Do, the echo appeared in the chronicle but the DM never responded for 3+ minutes — no spinner, no narration, no error, all action buttons still enabled. The DM only began narrating after a manual "Continue" click. Root cause: `pending` (the local "DM is narrating…" spinner state) is armed by postMove → armPending, then a clearPending can fire milliseconds later and wipe it before the DM's first line could possibly land — the immediate post-arm surface poll, a /chat cursor-reset re-reading the PRIOR (already-resolved) turn's line as a fresh resolution (the per-run reset zeroes chatCursor), or a transient campaignId flip tripping the per-run reset effect. The player then perceives "no DM", clicks Continue (which re-arms pending + surfaces the reply). Intermittent → a key driver of G3 single-run variance + a blocker on a clean multi-run mean. Fix: clearPending now keeps a FRESHLY-armed, not-yet-streaming turn alive for a short grace (PENDING_ARM_GRACE_MS = 10s) — far below a real DM beat (~100–150s) so a genuine resolution is never swallowed, above the 4s poll cycle so the one-shot post-arm clear can't beat it. Once /events prose streams (`streaming`) the guard lifts, and the 12-min backstop bypasses it (armPending arms it via setPendingState(null) directly). The "Try again" retry re-arms via armPending (not clearPending), so the tuned #344/#348/#399 recovery path is untouched. Guards (Node-JSX harness — transpiles the real app.jsx under a fake clock): - test_recovery_timing.py: a same-tick clear after arm is a no-op (the spinner survives) AND a clear past the grace still resolves the turn (never stuck on). The #344 contract test now clears past the grace — its real intent (a clean re-arm) goes through armPending, unchanged. - test_live_narration_stream.py: the two turn-resolution tests advance the clock past the grace before the resolving /chat tick (a real beat is ~120s, not the fake-0s they assumed); a new `h.advance` exposes the harness clock. 395 viewer tests pass locally. Addresses #648 (the move-sink → pending-arm contract). Close on a clean-sweep non-repro per the gate discipline.
📝 WalkthroughWalkthroughThis PR adds an arm-grace window to prevent a narrating pending turn from being cleared by spurious same-tick ChangesPending Arm-Grace Protection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
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: 3
🤖 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/openworlds/app.jsx`:
- Around line 377-390: clearPending's current grace check (using
pendingRef.current and PENDING_ARM_GRACE_MS) suppresses legitimate fast
resolution clears; change clearPending to accept a boolean (e.g., forceClear or
isResolution) that, when true, bypasses the PENDING_ARM_GRACE_MS gate, and
update callers that perform real turn resolutions (the code path that consumes a
/chat turn or the event consumer that previously called clearPending) to call
clearPending(true); keep existing behavior for spurious clears by leaving
default calls unchanged; ensure symbols touched: clearPending,
pendingRef.current, PENDING_ARM_GRACE_MS, clearTimers, setPendingState and any
callers that currently invoke clearPending (replace with clearPending(true)
where the clear represents an actual resolved turn).
In `@viewer/tests/test_live_narration_stream.py`:
- Around line 244-247: Replace the ambiguous EN DASH character in the comment
near the advance function so Ruff RUF001 no longer flags it: edit the comment
string that contains “–” (the line describing advancing the fake clock before
the advance: (ms) => { NOW += ms || 0; }) and change the EN DASH to a normal
ASCII hyphen-minus '-' (or an explicit ASCII double hyphen) so the comment uses
only ASCII punctuation.
In `@viewer/tests/test_recovery_timing.py`:
- Line 300: Replace the EN DASH character in the docstring snippet containing
"100–150s" with an ASCII hyphen so the text reads "100-150s"; locate the
docstring that contains the substring "100–150s" in the test_recovery_timing
module and update that character to '-' to satisfy Ruff RUF002.
🪄 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: 48510600-8bad-43e7-b8df-583e4fa1d042
📒 Files selected for processing (3)
viewer/openworlds/app.jsxviewer/tests/test_live_narration_stream.pyviewer/tests/test_recovery_timing.py
Comment on lines
+377
to
+390
| const clearPending = React.useCallback(() => { | ||
| // #648: keep a FRESHLY-armed, not-yet-streaming turn alive through a spurious same-tick clear | ||
| // (see PENDING_ARM_GRACE_MS). The real "Try again" retry path re-arms via armPending (which calls | ||
| // clearTimers itself), NOT clearPending, so this never blocks a legitimate re-arm; it only stops a | ||
| // poll/reset/flip from wiping the narrating spinner before the DM's first line could land. A real | ||
| // resolution arrives long past the grace (or after /events streamed), so it still clears normally. | ||
| const p = pendingRef.current; | ||
| if (p && !p.streaming && typeof p.since === "number" | ||
| && (Date.now() - p.since) < PENDING_ARM_GRACE_MS) { | ||
| return; | ||
| } | ||
| clearTimers(); | ||
| setPendingState(null); | ||
| }, [clearTimers, setPendingState]); |
There was a problem hiding this comment.
Grace-only gating can block legitimate fast turn resolution.
At Line 384-Line 386, every non-streaming clear inside the 10s window is dropped. That also affects real /chat turn-end clears for fast, chat-only turns; if the resolving line is consumed once, pending can remain until recovery/backstop and keep actions gated.
Suggested direction (differentiate stale clear vs real resolution)
- const clearPending = React.useCallback(() => {
+ const clearPending = React.useCallback((opts = {}) => {
+ const resolvedAtMs = Number.isFinite(opts.resolvedAtMs) ? opts.resolvedAtMs : null;
const p = pendingRef.current;
- if (p && !p.streaming && typeof p.since === "number"
- && (Date.now() - p.since) < PENDING_ARM_GRACE_MS) {
+ const withinGrace = p && !p.streaming && typeof p.since === "number"
+ && (Date.now() - p.since) < PENDING_ARM_GRACE_MS;
+ const resolvesCurrentTurn = p && resolvedAtMs !== null && resolvedAtMs >= p.since;
+ if (withinGrace && !resolvesCurrentTurn) {
return;
}
clearTimers();
setPendingState(null);
}, [clearTimers, setPendingState]);- if (dmLineArrived) {
+ if (dmLineArrived) {
+ const lastDm = [...items].reverse().find((it) => it.role === "dm");
+ const resolvedAtMs =
+ (lastDm && typeof lastDm.at === "number") ? (lastDm.at * 1000) : null;
...
- clearPending();
+ clearPending({ resolvedAtMs });
}🤖 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 `@viewer/openworlds/app.jsx` around lines 377 - 390, clearPending's current
grace check (using pendingRef.current and PENDING_ARM_GRACE_MS) suppresses
legitimate fast resolution clears; change clearPending to accept a boolean
(e.g., forceClear or isResolution) that, when true, bypasses the
PENDING_ARM_GRACE_MS gate, and update callers that perform real turn resolutions
(the code path that consumes a /chat turn or the event consumer that previously
called clearPending) to call clearPending(true); keep existing behavior for
spurious clears by leaving default calls unchanged; ensure symbols touched:
clearPending, pendingRef.current, PENDING_ARM_GRACE_MS, clearTimers,
setPendingState and any callers that currently invoke clearPending (replace with
clearPending(true) where the clear represents an actual resolved turn).
Comment on lines
+244
to
+247
| // #648: advance the fake clock (Date.now reads NOW) so a test can resolve a turn AFTER the | ||
| // armPending grace window — a real DM beat lands ~100–150s post-submit, far past the guard, so a | ||
| // same-tick resolve is unrealistic (and now intentionally a no-op for the freshly-armed spinner). | ||
| advance: (ms) => { NOW += ms || 0; }, |
There was a problem hiding this comment.
Replace ambiguous EN DASH to satisfy Ruff RUF001.
Line 245 includes – inside the harness string, which Ruff flags as ambiguous unicode.
Minimal fix
- // armPending grace window — a real DM beat lands ~100–150s post-submit, far past the guard, so a
+ // armPending grace window - a real DM beat lands ~100-150s post-submit, far past the guard, so a🧰 Tools
🪛 Ruff (0.15.15)
[warning] 245-245: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
🤖 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 `@viewer/tests/test_live_narration_stream.py` around lines 244 - 247, Replace
the ambiguous EN DASH character in the comment near the advance function so Ruff
RUF001 no longer flags it: edit the comment string that contains “–” (the line
describing advancing the fake clock before the advance: (ms) => { NOW += ms ||
0; }) and change the EN DASH to a normal ASCII hyphen-minus '-' (or an explicit
ASCII double hyphen) so the comment uses only ASCII punctuation.
| re-reading the prior resolved turn's line as a fresh resolution, or a transient campaignId | ||
| flip tripping the per-run reset. That MUST NOT wipe the just-armed narrating spinner (the | ||
| adversarial's '[MAJOR] no spinner, buttons enabled, no DM for 3+ min until I clicked | ||
| Continue'). The guard is bounded: a REAL resolution (~100–150s later, past the grace) still |
There was a problem hiding this comment.
Use ASCII hyphen in docstring to avoid Ruff RUF002 warning.
Line 300 uses an EN DASH (–) that Ruff flags as ambiguous.
Minimal fix
- Continue'). The guard is bounded: a REAL resolution (~100–150s later, past the grace) still
+ Continue'). The guard is bounded: a REAL resolution (~100-150s later, past the grace) still📝 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
| Continue'). The guard is bounded: a REAL resolution (~100–150s later, past the grace) still | |
| Continue'). The guard is bounded: a REAL resolution (~100-150s later, past the grace) still |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 300-300: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(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 `@viewer/tests/test_recovery_timing.py` at line 300, Replace the EN DASH
character in the docstring snippet containing "100–150s" with an ASCII hyphen so
the text reads "100-150s"; locate the docstring that contains the substring
"100–150s" in the test_recovery_timing module and update that character to '-'
to satisfy Ruff RUF002.
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.
The bug (intermittent G3-killer)
Adversarial sweep,
[MAJOR], dropped sat 9→4: after Enter-submitting a Do, the echo appeared in the chronicle but no spinner, no narration, all buttons still enabled for 3+ min. The DM only narrated after a manual Continue click.Root cause
pending(the local "DM is narrating…" spinner) is armed bypostMove → armPending, then aclearPendingfires milliseconds later and wipes it before the DM's first line could land — via one of:/chatcursor-reset re-reading the prior (already-resolved) turn's line as a fresh resolution (the per-run reset zeroeschatCursor);campaignIdflip tripping the per-run reset effect.The player perceives "no DM", clicks Continue (which re-arms + surfaces the reply). Being intermittent, it's a key driver of G3 single-run variance and blocks a clean multi-run mean.
Fix
clearPendingkeeps a freshly-armed, not-yet-streaming turn alive for a short grace (PENDING_ARM_GRACE_MS = 10s):/eventsprose streams (streaming) the guard lifts;setPendingState(null)directly);armPending(notclearPending) → the tuned [playtest][P1] VALIDATE #343 completeness — veteran hit dead 'Try again' button + Table nav still blocked from Map/Journal #344/[playtest][P1] DM first-response ~60s NO narration → recovery fired; recovery-timeout may pre-empt the legit multi-minute DM opening #348/perf(engine): non-blocking generate_image — off the synchronous DM-turn path #399 recovery path is untouched.Guards (Node-JSX harness — transpiles the real
app.jsxunder a fake clock)test_recovery_timing.py: a same-tick clear after arm is a no-op (spinner survives) AND a clear past the grace still resolves the turn (never stuck on). The [playtest][P1] VALIDATE #343 completeness — veteran hit dead 'Try again' button + Table nav still blocked from Map/Journal #344 contract test clears past the grace — its real intent (clean re-arm) goes througharmPending, unchanged.test_live_narration_stream.py: the two turn-resolution tests advance the clock past the grace before the resolving/chattick (a real beat is ~120s, not the fake-0s they assumed); a newh.advanceexposes the harness clock.395 viewer tests pass locally.
Addresses #648 — close on a clean-sweep non-repro per the gate discipline.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests