fix(reliability): stamp engine_logged on DM opening/beat chat rows to kill the duplicate opening (#720) - #727
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPorts a truthfulness guard into the beat driver: adds ChangesEngine-logged DM reply truthfulness guard for cold-open deduplication
Sequence DiagramsequenceDiagram
participant play as play.sh / play_party.sh
participant record as record_dm_reply
participant logger as log_engine_narration
participant engine as servers/engine
participant chat as chat.jsonl
play->>record: record_dm_reply(CAMPAIGN_ID, TEXT, phase)
record->>logger: validate + check recent session narrations
alt narration absent
logger->>engine: uv run server.log_event(campaign_id,"narration", text)
engine-->>logger: success
logger-->>record: return 0
record->>chat: chatlog with {"engine_logged": true}
else narration present or log failed
logger-->>record: return non-zero / skip
record->>chat: chatlog without engine_logged (and emit stderr warning if failure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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. ✨ 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: 1
🤖 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/lib_beat_driver.sh`:
- Around line 200-207: record_dm_reply currently re-sends narration into the
engine via log_engine_narration even when the text already originated from
clawdnd_dm_narration_or_fallback, causing duplicate engine events; change
record_dm_reply to accept an explicit flag/param (e.g., already_engine_logged or
sourced_from_engine) and if that flag is true skip calling log_engine_narration
and directly call chatlog dm with the '{"engine_logged":true}' payload,
otherwise preserve the existing behavior (call log_engine_narration then chatlog
without engine_logged); update callers (the wrappers that pass
clawdnd_dm_narration_or_fallback results) to pass the new flag when the
narration came from the engine.
🪄 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: 6c14cac1-78e6-42f4-bad2-7ec19c85999d
📒 Files selected for processing (4)
qa/lib_beat_driver.shscripts/play.shscripts/play_party.shservers/engine/tests/test_dm_reply_engine_logged.py
added 2 commits
June 9, 2026 21:33
… kill the duplicate opening (#720) The cold-open opening narration rendered TWICE in the OpenWorlds chronicle (vm2-newbie/narrative/veteran sweep): the opening prose lands in two viewer-read sources — the engine per-session log (per-paragraph, fed to /events) AND chat.jsonl (the whole opening as one blob, written by the play wrappers' `chatlog` with no flag). The client's mid-session de-dup (eventsStreamedThisTurnRef) does not guard the cold-open blob (the opening is already complete pre-mount), so it shows up twice. The codex DM path (scripts/play_codex_dm.sh) already solves this with three pieces: a 3-arg `chatlog` (optional extra-JSON merged into the row), a `log_engine_narration` helper, and a `record_dm_reply` helper that stamps {"engine_logged":true} on the chat row IFF the prose was also logged to the engine session log. The client already honors that marker (viewer/openworlds/app.jsx: `if (it.engine_logged === true) return null;`). This ports that PROVEN idiom into the two CLAUDE-DM viewer-backed wrappers. Both play.sh and play_party.sh already source qa/lib_beat_driver.sh, so the three helpers live ONCE in that shared lib (DRY, mirroring the shared clawdnd_dm_remint_session_on_retry) and both wrappers call them: - qa/lib_beat_driver.sh: add chatlog (3-arg) + log_engine_narration + record_dm_reply (read $CHAT/$STATE_DIR/$ROOT from the caller, as the codex versions read $CHAT/$RUN_DIR/$ROOT). On engine-log SUCCESS -> flagged row; on FAILURE -> unflagged row (byte-identical to today; the client's eventsStreamedThisTurnRef backstop still applies). The flag is NEVER stamped unconditionally (a legit /chat-only beat must still render). - scripts/play.sh: drop the one-line chatlog (now shared); MOVE the CAMPAIGN_ID resolution to BEFORE the opening write (record_dm_reply needs the id; the campaign already exists post cold-open dm_turn); route the opening + per-move DM writes through record_dm_reply. - scripts/play_party.sh: drop the one-line chatlog; route the opening, reseat, after-intros, and per-beat DM writes through record_dm_reply. Player/companion rows are left as plain chatlog calls. Wrapper-only (the viewer side is already done + tested). bash 3.2 safe: the heredoc-bearing helpers are only ever called directly, never inside $(...). Tests: new servers/engine/tests/test_dm_reply_engine_logged.py (9 tests, behavioral + static anti-drift) green; the existing remint anti-drift test (10) and the client-side engine_logged de-dup + regression tests (3) stay green; fast_gate Tier-0 (188) passes.
…dup to /events Adversarial review (data-confirmed on real VM runs): the CLAUDE DM frequently logs the opening/beat narration to the engine session log DURING its turn. record_dm_reply's UNCONDITIONAL re-log would then put the prose in the log TWICE -> a SECOND /events row (the viewer keys /events by line-index seq, not text) -> the duplicate is RELOCATED (/events-vs-/events), not fixed. log_engine_narration now appends ONLY when the prose is not already in the recent session-log narration (whitespace-normalized substring, covering single-blob AND per-paragraph logging shapes), but STILL returns success so record_dm_reply stamps engine_logged. Net: the prose lives in the engine log EXACTLY ONCE (for /events + recap/memory) and the redundant /chat blob is dropped -> rendered once. The CODEX DM (which does not self-log) still gets the canonical append. Tests (+3): does-not-double-log-when-already-logged, idempotent-across-per-paragraph (real newlines via $'...'), appends-canonical-when-absent. 12/12 in-file + remint 15 + fast_gate 188 + client contract 3 green. Rebased onto main (incl #719/#726).
100yenadmin
force-pushed
the
fix/720-dup-narration-engine-logged
branch
from
June 9, 2026 14:40
97ea259 to
4de665d
Compare
Member
Author
|
Reviewed before merge (adversarial pass) — found + fixed a relocation flaw. The initial port re-logged the reply to the engine session log unconditionally. But real VM runs show the claude DM already logs the opening narration to the engine during its turn ( Fix (latest commit): |
This was referenced Jun 9, 2026
[security][major] XSS partial sanitization — inner text leaks into chronicle as a player action
#732
Closed
Merged
100yenadmin
added a commit
that referenced
this pull request
Jun 9, 2026
…rnal kind labels/You— artifact (#731, #732) (#740) #731 (XSS partial sanitization): neutralizeMarkup stripped <script> TAGS but left the inner BODY as text ('<script>alert(1)</script>' -> 'alert(1)'), which rode into the chronicle as a player action. Now excise the bodies of script-class/embedded-content tags (script/style/iframe/object/embed/svg/math/template/noscript/...) BEFORE the generic tag strip, so nothing of the payload survives. Benign emphasis prose (<b>hello</b> -> hello) is preserved. #732 (chronicle metadata leak): a recentEvents history row carries the engine's internal SessionLogEntry.kind (narration|dialogue|roll|system|combat). Only narration/action/roll/ dialog had LogEntry branches, so a 'dialogue'/'combat' row fell to the default branch and rendered the raw kind STRING as an uppercase label, with text NOT sanitized. And a player action rendered 'You—…', a formatting artifact reading like DM narration. Now: - new 'dialogue' branch renders sanitized in-world speech (no kind label, scaffolding stripped); - the default branch NEVER renders the raw internal kind (cleanRowLabel drops kind tokens); - player 'You' actions render as clean second-person prose with no 'You—' chrome. Does NOT touch the #720/#727 engine_logged/dedup path or the _pick_campaign/_action_actor campaign-resolution code (#735). Viewer render-only; engine stays sole writer. Tests: viewer/tests/test_chronicle_hygiene.py (19 cases, real shipped fns under Node). Verified: full viewer suite 475 passed / 1 skipped; fast_gate Tier-0 188 passed. Co-authored-by: Eva <arncalso@gmail.com>
100yenadmin
added a commit
that referenced
this pull request
Jun 10, 2026
…t, decontaminate recap/FTS/lean-tail/fallback (#749) (#763) The #743 wrapper heartbeat was (1) a NO-OP for the player — its texts are byte-identical to screen-table.jsx's _WRAPPER_PROGRESS_LINES and app.jsx sanitized the /events row to "" BEFORE notePendingProgress, so the spinner never flipped — and (2) real kind=narration rows that NO engine consumer filtered: recap recited the filler, the FTS ledger indexed it, the lean re-ground tail fed it back to the DM as canon, and dm_narration_fallback recovered it (masking fully-dead beats with stale prose + filler). Keep-and-repair, additive; engine stays the sole writer; old snapshots round-trip: (a) CLIENT: app.jsx /events ingest special-cases exact wrapper lines BEFORE the sanitize-drop — notePendingProgress() (flip streaming) + return null (never render; never sets eventsStreamedThisTurnRef so a dead beat's recovered /chat text still renders). Predicate + constant shared from screen-table.jsx as window-guarded globals. (b) ONE python source of truth: servers/engine/wrapper_progress.py (qa/wrapper_progress_lines.py re-exports it for stdlib-only qa scripts) + tests/test_wrapper_progress_sync.py regex-pins the jsx Set and BOTH sh emit rotations (lib_beat_driver.sh + play_codex_dm.sh) byte-identical. Exact-match filters in recap.format_recap, ledger.backfill (FTS), server._scene_recent_narration, and qa/dm_narration_fallback.py (where a heartbeat BREAKS the trailing block, so a heartbeat-only dead beat honestly recovers nothing). (c) FALLBACK HONESTY: clawdnd_resolve_dm_reply (direct-call front door) flags a #357 recovery; record_dm_reply / clawdnd_chatlog_dm stamp {"fallback_recovered":true} on the dm chat row (consume-once). All 9 runner call sites converted. assert_behavioral semantics unchanged. (d) DEDUP: #727's last-8 substring guard provably swallowed cadence-aligned heartbeats (a run of dead beats logs ONLY heartbeats — the 4-line rotation repeats inside the window). Wrapper lines are now EXEMPT from the dedup (always append); the repeats are inert (never rendered, fully filtered), keeping the exact-match design on every side. Tests (red-first): sync test, recap/FTS/lean-tail exclusion, fallback wrapper-skip + fallback_recovered stamps, dedup bypass + intact prose dedup, viewer node-harness heartbeat-flips-without-rendering + no-suppression guard. Engine 1783 passed, viewer 488 passed, fast_gate PASS, bash -n clean (3.2). Closes #749. Refs #743 #727 #623 #357 #720. Co-authored-by: Eva <arncalso@gmail.com>
100yenadmin
added a commit
that referenced
this pull request
Jun 14, 2026
…nsition meta-text (#752) (#890) * fix(viewer): bound chronicle a11y footprint + suppress inter-beat transition meta-text (#752) The 2026-06-15 confirm sweep flagged #752 MAJOR by 3 of 5 personas (newbie, adversarial, narrative). Verbatim: "Chronicle log grows into one massive block — later beats invisible in a11y tree" / "Oversized chronicle log pushes Actions section out of a11y tree entirely" / "buries action buttons — player can't tell if DM is done" / "hiding all action controls from screen reader". Plus adversarial: "DUPLICATE DM narration for the opening beat" and "Engine META-TEXT transition phrases LEAK into the chronicle between player beats". FELT MECHANISM: the screen-reader / QA blind-player reads the page via a LINEAR, length-capped accessibility snapshot (qa/playwright/palette_server.js does ariaSnapshot().slice(0,9000)) rendered in DOM order. The Chronicle (role="log") renders BEFORE the action palette + composer, so a long run of multi-paragraph DM beats fills the whole snapshot budget and the action controls are sliced off the snapshot ENTIRELY. #402 anchored the bar VISUALLY (a sticky DOM sibling) but a linear capped snapshot can't be fixed by visual positioning. FIX (viewer-only, READ-ONLY — engine stays sole writer): 1. a11y bound: cap the chronicle's ACCESSIBILITY footprint independently of its visual row count — only the most-recent CHRONICLE_A11Y_TAIL (8) rendered rows stay in the a11y tree; older rendered rows are aria-hidden (still fully VISIBLE for sighted scroll-back, full history in the Quest Journal). The latest beat is always exposed; the bound only engages once the rendered list exceeds the tail. Plus a named role="region" aria-label="Actions" so AT can target the controls. New pure exported chronicleRowAriaHidden(i,total) + CHRONICLE_A11Y_TAIL. 2. opening-beat / player-echo DEDUP: pinned the exactly-once contract (the existing #727/#740/#405 dedup already collapses the opening + the Continue/free-text echo; tests lock it so it can't regress). 3. META-TEXT leak: new high-confidence _BEAT_TRANSITION pattern in sanitizeNarration suppresses inter-beat transition stage-directions ("Moving on to the next beat", "Transitioning to the next scene", "Scene transition", "End of beat", "Beginning the next beat", "between the beats", "move to the next part of the story") while preserving real fiction ("heart skips a beat", "tavern scene", "smooth transition from the parapet to the rope", "between the beats of the drum"). Tests (JSX harness, real .jsx transpiled under Node): - viewer/tests/test_chronicle_a11y_bound.py (3): tail constant tight; action controls stay in the a11y tree after 30 rows (only the tail exposed); short chronicle exposes every row. - viewer/tests/test_chronicle_dedup_opening.py (13): opening once across bands; Continue/free-text no double-echo; wrapper + inter-beat transition meta-text suppressed; surrounding real prose preserved; in-world beat/scene/transition prose survives. fast_gate.sh PASS (215 engine). Full viewer suite 596 passed / 1 skipped. * fix(viewer): tighten _BEAT_TRANSITION to stop over-stripping real fiction (review fix) Adversarial review found the meta-text regex silently deleted real fiction sentences ('the end of the act left them breathless', 'by the close of the scene, three lay dead', 'beginning the act of contrition'). Root: the end-of/beginning-the arms used act/chapter/part (real-fiction words) + an optional forward qualifier, so they matched descriptive prose. FIX: those two arms now use only the engine's own struct terms (beat|scene) AND require the phrase to be TERMINAL (the meta note IS the short sentence, ending on the struct) — descriptive fiction embeds the phrase mid-sentence with trailing prose, so it's spared. The leak cases ('End of beat.', 'Beginning the next beat.') still suppress. +5 survive-case tests (the exact over-strip examples). Story quality is the north star — no silent fiction deletion. --------- Co-authored-by: Eva <arncalso@gmail.com>
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.
Closes #720.
The bug
The cold-open opening narration rendered TWICE in the OpenWorlds chronicle (vm2-newbie/narrative/veteran 5-persona sweep, RRI 2026-06-09). The opening prose lands in two viewer-read sources:
seq+sid-keyed, fed to the viewer's/events), andchat.jsonl(the whole opening as one blob, written by the play wrappers'chatlogwith noengine_loggedflag).The client's mid-session de-dup (
eventsStreamedThisTurnRef) assumes "/events lands a turn's paragraphs before its /chat blob" — true mid-session, false on cold-open (the opening is already complete pre-mount). So the opening shows up twice (cosmetic but systemic: doubles the log, shifts the a11y truncation horizon, compounds the latency UX).The fix — port the PROVEN Codex idiom (no new mechanism)
scripts/play_codex_dm.shalready solves this for the codex DM path with three pieces: a 3-argchatlog(optional extra-JSON merged into the row),log_engine_narration, andrecord_dm_reply(stamps{"engine_logged":true}on the chat row iff the prose was also logged to the engine session log). The client side is already done + tested —viewer/openworlds/app.jsxdoesif (it.engine_logged === true) return null;andviewer/tests/test_live_narration_stream.py::test_engine_logged_chat_reply_resolves_without_rendering_duplicatepasses. This PR is wrapper-only (the optional viewer defense-in-depth in #720 is explicitly deferred).This ports that idiom into the two CLAUDE-DM viewer-backed wrappers,
scripts/play.shandscripts/play_party.sh.Design choice: shared lib (DRY), not per-script copies
Both wrappers already source
qa/lib_beat_driver.sh(grep -n lib_beat_driverconfirms), so the three helpers live once in that shared lib — mirroring the existing sharedclawdnd_dm_remint_session_on_retrypattern — and both wrappers call them. The helpers read$CHAT/$STATE_DIR/$ROOTfrom the caller's scope (exactly as the codex versions read$CHAT/$RUN_DIR/$ROOT).qa/lib_beat_driver.sh(+81): addchatlog(3-arg),log_engine_narration,record_dm_reply. On engine-log SUCCESS → flagged row; on FAILURE → unflagged row (byte-identical to today; theeventsStreamedThisTurnRefbackstop still applies). The flag is never stamped unconditionally — a legitimately/chat-only beat must still render.scripts/play.sh: drop the one-linechatlog(now shared); move theCAMPAIGN_IDresolution to before the opening write (record_dm_replyneeds the id; the campaign already exists post cold-opendm_turn); route the opening + per-move DM writes throughrecord_dm_reply.scripts/play_party.sh(CAMPAIGN_IDalready set early from the seed): drop the one-linechatlog; route the opening, reseat, after-intros, and per-beat DM writes throughrecord_dm_reply. Player/companion rows are left as plainchatlogcalls.bash 3.2 safety
The heredoc-bearing helpers (
chatlog,log_engine_narration) are only ever called directly, never inside$(...)(which macOS bash 3.2 mis-parses)./bin/bash -npasses on all three files.Tests
servers/engine/tests/test_dm_reply_engine_logged.py(9 tests): 3-argchatlogmerge + legacy-shape byte-identity;record_dm_replySUCCESS path (seeds a real campaign viaserver.start_world, asserts the dm row carriesengine_logged:trueand the engine session log received the narration); FAILURE path (blank campaign id → unflagged row);log_engine_narrationblank-input rejection; static anti-drift (both wrappers route DM replies throughrecord_dm_reply, no barechatlog dmremains, player/companion rows untouched).servers/engine/tests/test_dm_session_remint.py(10, anti-drift on the same scripts) — green.viewer/tests/test_live_narration_stream.pyengine_logged de-dup + the two regression tests (unstreamed_chat_paragraph_still_renders,terse_turn_after_streamed_turn_still_renders) — green (confirms the wrapper→client contract end-to-end).bash qa/fast_gate.shTier-0 (188 engine + seat-path) — PASS.Additive only; no engine
.pytouched.Summary by CodeRabbit