Repository navigation
fix(character): persist xp across create and update - #81
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 57 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCharacter XP now persists through creation and updates, defaults to zero when absent, and remains unchanged during unrelated updates. XP handling uses nullish fallback, and repository tests cover these behaviors. ChangesCharacter XP handling
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This PR wires character XP through create, update, and reads, with regression coverage for persistence behavior. No actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The characters table has carried an `xp` column since the XP-system migration, but CharacterRepository never read or wrote it: create() and update() both omitted the column and rowToCharacter never mapped it. Since CharacterSchema declares `xp` with .default(0), every read re-synthesized 0 while update() still returned the merged in-memory object — so callers saw their own value echoed back and only discovered the loss on the next get. add_xp compounded it, reading a base of 0 via an `as any` cast and then discarding the result. Dropping the cast lets the type system keep the field honest now that rowToCharacter maps it. No data recovery needed: the column was never written, so every existing character already reads 0. Adds repository tests covering create, update, and the untouched-field case; three of the four fail without this change.
e3fab42 to
a7296fa
Compare
The characters table has carried an `xp` column since the XP-system migration, but CharacterRepository never read or wrote it: create() and update() both omitted the column and rowToCharacter never mapped it. Since CharacterSchema declares `xp` with .default(0), every read re-synthesized 0 while update() still returned the merged in-memory object — so callers saw their own value echoed back and only discovered the loss on the next get. add_xp compounded it, reading a base of 0 via an `as any` cast and then discarding the result. Dropping the cast lets the type system keep the field honest now that rowToCharacter maps it. No data recovery needed: the column was never written, so every existing character already reads 0. Adds repository tests covering create, update, and the untouched-field case; three of the four fail without this change.
* feat(storage): make every tool call all-or-nothing Each MCP tool call now runs in one SQLite transaction that commits or rolls back as a unit. Covers the issue #108 drink-then-Dash case. - Per-connection lock so an awaiting call's open transaction can't absorb another call's writes. The audit write stays inside the lock. - Rolls back on a throw, isError, or an error field in the payload. Those responses now carry isError: true. - CombatManager and WorldManager snapshot in-memory state and restore it on rollback. - Pubsub delivery waits for COMMIT; rolled-back calls announce nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FntDZ4y2gh1q6mw69yAoo * fix(batch): make execute_sequence report failures and roll back - The step parser looked for a <!--JSON:--> marker nothing emits, so every step showed success, stopOnError never fired, and {{step.field}} references resolved to undefined. It now reads the real embedded JSON. - An unresolved reference fails its step instead of passing undefined on. - Each step runs in a savepoint. With stopOnError (the default) any failure rolls back the whole sequence and returns isError. With stopOnError=false, successful steps stand and failed ones leave nothing. - create_characters runs each character in its own savepoint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FntDZ4y2gh1q6mw69yAoo * refactor(storage): one writer per table - items: corpse harvesting goes through ItemRepository, so properties is written (same drift as #81). Harvest is now one transaction. - characters: currency writes and delete go through CharacterRepository. - worlds: tile-cache SQL moved into WorldRepository. - event_logs: new EventLogRepository replaces two hand-written INSERTs. - event_inbox: pollAndConsume is one IMMEDIATE transaction and only returns rows it actually claimed (#95 review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FntDZ4y2gh1q6mw69yAoo * fix(events): deliver events on local transports and tag their encounter - stdio, TCP, unix and WebSocket carry no tenant, so the #95 bridge dropped every event and poll_events was always empty. Single-user mode now writes to the local database. - Combat engines stamp encounterId on published events, so inbox entries record which encounter produced them (#95 review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FntDZ4y2gh1q6mw69yAoo * fix(uow): address review — inbox in-transaction, rewind dice, per-manager keys - The durable inbox subscribes as transactional, so its row is written inside the producing call's transaction and commits or rolls back with it. Live notifications still wait for COMMIT. - Combat snapshots include the RNG position, so a retried action gets the same rolls as the rolled-back one. - Rollback keys include the manager instance, so two managers guarding the same id don't overwrite each other's undo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FntDZ4y2gh1q6mw69yAoo * fix(events): fail the call when its inbox row can't be written Inside a unit of work the inbox row is part of the change. A failed insert now propagates and rolls the producing call back, so a change can't commit without its event. Outside a unit there is no transaction to keep consistent, so the failure is still only logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FntDZ4y2gh1q6mw69yAoo --------- Co-authored-by: Claude <noreply@anthropic.com>
Problem
The
characterstable has carried anxpcolumn since the XP-system migration, butCharacterRepositorynever read or wrote it:create()INSERT omitted the columnupdate()UPDATE omitted the columnrowToCharacter()never mapped itBecause
CharacterSchemadeclaresxpwith.default(0), every read re-synthesized0. Andupdate()still returned the merged in-memory object, so callers saw their own value echoed back as a success and only discovered the loss on the nextget.add_xpcompounded it:(char as any).xp || 0always read a base of0, thenupdate({ xp: newXp })discarded the result. Thatas anycast is what kept the gap invisible to the type system.Reported from live play: setting
xpreturns success echoing the submitted value; an immediategetshows0.Fix
Wire
xpthrough the repository — INSERT, UPDATE,CharacterRow, androwToCharacter— and drop theas anycast inhandleAddXpso the compiler enforces the field from here on.Tests
Adds a
xp persistenceblock totests/storage/character.repo.test.tscovering create, update, and the untouched-field case. Verified as real regression tests: 3 of the 4 fail without this change (the 4th is a default-value guard that passes either way by design).The update test asserts the echoed response and the stored row agree — checking only the return value would have missed the original bug.
Full suite: 2,312 passed / 7 skipped,
npm run buildclean.Notes
No data recovery needed — the column was never written, so every existing character already reads
0.skillProficiencieswas reported alongside this with the same symptom, but it is fully wired at61f72ea(INSERT, UPDATE, androwToCharacterall present) and does not reproduce. That report appears to predate #78.🤖 Generated with Claude Code
Summary by CodeRabbit