fix(engine): decontaminate recap/recall + harden persist_beat (F07-1, F14-3) - #847
Conversation
…F07-1, F14-3) F07-1 (#772): the cold-open "previously on" recap and the FTS recall ledger were contaminated by combat/system BOOKKEEPING — every run recited "Tough 1 takes 5 force damage / Turn advances to Tough 2" and a recall('Rolan') probe returned 4 of 6 top hits as bookkeeping. Distinct from #749/#763, which exact-matched only the wrapper-heartbeat line. Engine combat-event rows carry payload schema clawdnd.combat_event.v1 (via _log_combat_event); the two session markers are "Session N began" / "Session ended.". recap.format_recap now drops schema-stamped combat rows (narrative combat with no/other payload stays); ledger.backfill skips schema-stamped combat rows AND the two session markers by exact prefix — while keeping every OTHER kind=system row indexed, preserving the documented DM-authored-system-note->recall path (SKILL.md:47). Derived-index-only; no schema change. F14-3 (#795): persist_beat (the every-beat write) burned ~2 beats in a real gate run. Four defects fixed by validate-then-apply: (1) decision chosen=null (and summary/rationale) crashed pydantic string_type -> None-coerce every Decision str field; (2) mem["character_id"] bare-KeyError'd -> resolve via the F14-8 _char resolver (#786) with id/npc_id aliases and an actionable "memories index N: ..." error carrying a did-you-mean; (3) events were appended to the session jsonl BEFORE memories/decision validation (a crash left a partial chronicle, a retry duplicated it) -> the whole batch is now validated BEFORE the first append_log (events-only non-atomic window closed); (4) the quadratic remembered echo (whole growing memory list per item) is now the slim {"id","fact","memory_count"}. advance stays its own sequential locked call; engine-sole-writer + one-lock-one-save preserved. Tests: +3 recap (drop schema-stamped combat; keep narrative/unrelated-payload combat), +3 ledger (skip combat-events + session markers; KEEP DM system note), +8 persist_beat (chosen:null; null str fields; actionable bad-id not bare KeyError; events-not-applied-on-later-failure atomicity; text alias honored; empty rejected; id alias resolves; remembered O(items) not quadratic). Full engine suite 2096 passed; fast_gate Tier-0 PASS. Closes #772 Closes #795 Source: docs/audits/ENGINE-AUDIT-2026-06-11.md
|
Warning Review limit reached
More reviews will be available in 1 hour, 2 minutes, and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1234c54a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _is_combat_event(e) or _is_session_marker(e): | ||
| continue |
There was a problem hiding this comment.
When this lands on an existing campaign whose ledger.sig already matches the snapshot/session files, recall() will not call backfill() because _ensure_fresh only compares the source-file signature. The new skip is therefore never applied to that campaign's existing ledger.db, so the combat/session bookkeeping this commit is meant to remove keeps surfacing until some later log-size change happens. Please include a ledger filter/schema version in the signature, or otherwise invalidate ledger.sig, so existing ledgers rebuild once.
Useful? React with 👍 / 👎.
…rden persist_beat atomicity test (SYN-03, F13-4, F14-3) (#854) SYN-03 (#783): the canon-roster read surfaces were unbounded on the flagship world (2,076 records). list_canon_characters returned ALL records verbatim (~180KB / ~45K tokens) and a load_canon_character miss DUMPED the whole roster as the 28KB error payload — costing cold-open beats (MCP-cap errors + Read-paging the offloaded result). Fix (additive, frozen-wire-safe): - list_canon_characters gains q (name substring) + limit (default 100, cap 200) and a {world_id, total, returned, available, truncated, note?} envelope — find_npcs' limit:int=50 + roster_surface's {total,returned} are the in-file precedents. Small worlds (<=limit) are byte-identical; only the large flagship roster is now paged (it was already an error there). - resolve-then-suggest on EVERY miss (load_canon_character + start_character pickup miss + not-playable case): difflib/substring did_you_mean (<=5) + available_count, NEVER the full list. KEEP the `error` key (play.sh reads it). - new content.suggest_canon_names(world_id, query, limit, playable_only) helper (mirrors itemcatalog.suggest); content.list_canon_characters gains an additive name_contains filter (default "" -> unchanged). Cross-link #316 (the viewer half — Roster screen pool count); viewer untouched. F13-4 (#753): scores_db had no latency columns, so the #753 per-beat budget had no ledger — and the worldos-latency-forensics skill already MANDATES recording s/beat, cold-open-s, turns/beat on every run. Fix (additive, migration-free): - scores_db.py: +s_per_beat / +coldopen_s / +turns_per_beat (REAL cols); _ensure_schema ALTER-adds them, pre-F13-4 rows read back NULL. - new qa/latency_rollup.py derives them from each beat's existing duration_api_ms (the skill's authoritative GENERATION metric) + num_turns: cold open = first beat (reported separately, EXCLUDED from the routine mean), failed/401 beats dropped, negative VM splits clamped at 0. - wire run_duo.sh (emits $RUN.latency.json + prints s/beat) and vm/sweep_v2.sh (copies duo-latency.json) where the .dm.<ns>.jsonl data already exists. TEST-QUALITY (F14-3, hardens #847): test_persist_beat_events_not_applied_when_ later_section_fails was a FALSE-GREEN — the cellar-rats fixture opens no session, so a leaked event row's session id was never persisted and the session_ids-only helper never saw it; the test passed on buggy AND fixed code. Rewrite into a real red->green guard: open a session first (orphan lands on the persisted path) and count via read_log_all (disk tail). PROVEN to go RED against an apply-before- validate persist_beat (orphan row leaks: before -> before+1) and GREEN on HEAD's validate-then-apply. _session_log_lines now reads disk-first so it can't blind future atomicity tests. Source: docs/audits/ENGINE-AUDIT-2026-06-11.md Closes #783 Refs #753, #316, #847 Co-authored-by: Eva <arncalso@gmail.com>
…cent_narration byte caps (SYN-08) (#852) The per-beat lean spine re-parsed the FULL append-only campaign history every beat and emitted count-not-byte-capped payloads, so token mass crept invisibly as campaigns aged (audit projected 30-60KB/beat at 30+ sessions). This bounds the WINDOW without dropping recency — story content the gates read is preserved. - store.read_log_all(tail=N): bounded newest-first walk that short-circuits the whole-history re-parse (F07-11). tail=None stays the full walk, byte-identical. On a 749-row seeded campaign the every-beat recent_narration read drops from 749 -> 525 rows parsed; the bound is constant regardless of campaign size. - recap.format_recap: per-entry ~400-char sentence-boundary soft cap + ~6KB total budget (defaulted), trimming OLDEST-first so the newest beats survive (F07-5, F14-16). NO LLM summarization — trims, never paraphrases. Short beats stay byte-identical. Seeded recap: 8,653B -> 2,617B (69% smaller). - _scene_recent_narration: reads via the bounded tail (lossless window bound, always on); the content byte-cap is DEFAULT-OFF, opt-in via WORLDOS_RECENT_NARRATION_MAX_CHARS (legacy CLAWDND_* honored) per F14-17's story-first posture (ride a long-campaign duo A/B before changing the default). With the opt-in cap=400 the seeded tail drops 5,876B -> 1,852B (68% smaller). Read-only invariant preserved (no save_campaign / updated_at bump). Additive: new params/env default to today's behavior; old snapshots round-trip. Preserves the #847 combat-filter in recap.py. Closes #805 Refs F07-5, F07-11, F13-6, F14-16, F14-17, SYN-08 Source: docs/audits/ENGINE-AUDIT-2026-06-11.md Co-authored-by: Eva <arncalso@gmail.com>
1 participant
What & why (story-gate wave)
Two written-but-never-cleanly-read defects that pollute the DM's story surface.
F07-1 — recap + FTS recall contaminated by combat/system bookkeeping (#772)
Root cause. A
kind="combat"row is either DM prose or an engine bookkeeping row written by_log_combat_event, which stampspayload.schema = "clawdnd.combat_event.v1".recap._STORY_KINDSincluded"combat"wholesale, andledger.backfillindexedcombat+systemrows askind="events". So every cold-open "previously on…" recited "Tough 1 takes 5 force damage / Turn advances to Tough 2", and arecall('Rolan')probe returned 4 of 6 top hits as bookkeeping. Distinct from #749/#763, which exact-matched only the wrapper-heartbeat line — combat-event rows and session markers passed straight through that filter.Fix (derived-index-only, no schema change).
recap.format_recapdrops schema-stamped combat rows; narrative combat (payloadNoneor lacking the schema) stays.ledger.backfillskips schema-stamped combat rows and the two engine session markers ("Session N began"/"Session ended.") by exact prefix (same discipline as [qa][engine] #743 heartbeat is invisible to the player AND contaminates engine memory (recap/FTS/lean tail) #749) — while keeping every otherkind=systemrow indexed, preserving the documented DM-authored-system-note → recall path (SKILL.md:47). All currentsystemrows are markers, so observable behavior is identical today; the spec must not foreclose the DM-system-note contract.F14-3 — persist_beat: chosen:null crash, bare KeyError, events-before-validation non-atomicity (#795)
The every-beat write burned ~2 beats in a real gate run (verbatim transcript
gate-duo2). Validate-then-apply rewrite:chosen: null(andsummary/rationale) crashed pydanticstring_type—.get(k,"")only defaults a missing key, an explicit null still reached thestrfield. Now None-coerced (.get(k) or ""); a still-open decision records withchosen="".mem["character_id"]bare-KeyError'd (Error executing tool persist_beat: 'character_id'). Now resolved via the F14-8_charresolver ([engine][P1] _char bare raise — ~60 tools dead-end on any character-id slip (audit F14-8) #786) withid/npc_idaliases; an unresolvable id raises an actionablememories index N: no character '…' in campaign. Did you mean: …?.append_log, closing the events-only non-atomic window (memories/decision were already fail-atomic — in-memory until the block-end save). Retry-safe.rememberedecho (whole growing memory list per item) → slim{"id","fact","memory_count"}. No external consumer read the oldname/memoryfields.advancestays its own sequential locked call; engine-sole-writer + one-lock-one-save preserved. Additive:kind=None/speaker=None/null str fields are now tolerated (strict improvement); no model field added → old snapshots round-trip.Tests
test_recap.pytest_ledger.pytest_beat_roundtrip.pyVerification
uv run --directory servers/engine python -m pytest -q -p no:xdist).qa/fast_gate.sh: PASS (188 deterministic).chosen=""; bad-id → actionable error + events leg not applied;recall('Rolan …')top hit is story, zero bookkeeping in hits.Closes #772
Closes #795
Source: docs/audits/ENGINE-AUDIT-2026-06-11.md