Repository navigation
fix: batch engine persistence and contract regressions - #93
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (62)
📝 WalkthroughWalkthroughChangesOpen5e runtime catalog and rules
World and spatial persistence
Turn, combat, and gameplay resolution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CharacterManage
participant Open5eCatalog
participant ItemService
participant Repository
Client->>CharacterManage: create source-backed character
CharacterManage->>Open5eCatalog: resolve character options
Open5eCatalog-->>CharacterManage: rules and provenance
CharacterManage->>ItemService: materialize starting equipment
ItemService->>Repository: create or refresh item records
Repository-->>CharacterManage: persisted equipment
CharacterManage-->>Client: character and applied mechanics
sequenceDiagram
participant Nation
participant TurnManage
participant TurnActionRepository
participant DiplomacyEngine
Nation->>TurnManage: submit actions
TurnManage->>TurnActionRepository: persist queued actions
TurnManage->>TurnActionRepository: load pending actions at resolution
TurnManage->>DiplomacyEngine: apply diplomacy actions
DiplomacyEngine-->>TurnManage: resolution messages
TurnManage->>TurnActionRepository: mark actions resolved
TurnManage-->>Nation: resolved action results
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f30d72da6c
ℹ️ 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".
| const catalogUrl = new URL('../../config/open5e-srd-2014-runtime.json', import.meta.url); | ||
| const parsed = JSON.parse(readFileSync(catalogUrl, 'utf8')); |
There was a problem hiding this comment.
When using the advertised pkg-produced standalone binaries, this runtime file is unavailable: the binary build in esbuild.config.mjs packages only server.cjs with assets: [] and never copies config/open5e-srd-2014-runtime.json. Consequently, the first character or source-item operation that calls getOpen5eCatalog() cannot read the catalog, effectively breaking those tools for binary users; embed/import the catalog or include it as a packaged asset with a binary-safe path.
Useful? React with 👍 / 👎.
| @@ -304,10 +312,14 @@ async function handleMarkReady(args: z.infer<typeof MarkReadySchema>): Promise<o | |||
| // Start resolution | |||
| turnStateRepo.updatePhase(args.worldId, 'resolution'); | |||
There was a problem hiding this comment.
If any queued action throws during resolution—for example, break_alliance with a nonexistent toNationId causes a foreign-key failure—the transaction rolls back its mutations, but this earlier phase update remains committed. The world is then permanently left in resolution, and both submit_actions and mark_ready reject further calls with no recovery action available; move this update into the transaction or restore planning when resolution fails.
Useful? React with 👍 / 👎.
| const character = await handleCharacterCreate({ | ||
| action: 'create', | ||
| ...charData | ||
| } as any) as any; |
There was a problem hiding this comment.
When create_characters omits applySpeciesAbilityBonuses, this direct handler call bypasses CreateSchema and therefore never applies its default(true). Source-backed batch characters consequently differ from individually created characters—for example, a batch-created Hill Dwarf keeps its unadjusted CON/WIS and derives lower HP—despite this path being intended to share authoritative creation behavior.
Useful? React with 👍 / 👎.
| if (healingValue === undefined && typeof effect === 'string' && /heal|restore|regain/i.test(effect)) { | ||
| healingValue = effect; | ||
| } |
There was a problem hiding this comment.
For consumables without an explicit healing property, this substring test treats negative mentions of healing as restorative effects and then rolls the first dice expression in the text. The included SRD Pale Tincture says its poison damage “can't be healed,” so using the materialized poison rolls its 1d6 damage as healing and increases the target's HP; only explicit healing metadata or an unambiguous structured effect should trigger healing.
Useful? React with 👍 / 👎.
Summary
Risk
Acceptance criteria
$refleakage.Verification
npm run buildnpm run test:ci -- --reporter=dot --silent— 143 files passed; 2,245 tests passed; 7 skipped.git diff --checkRelated issues
This batch addresses the confirmed implementation gaps behind #61, #62, #63, #64, #65, #66, #67, #69, #70, #71, #73, #76, #90, and #91, plus the older spatial, inventory, combat, character, and schema regressions verified during the audit.
Deployment note
No Railway deployment was performed. Production remains on engine SHA
2c34688f239382a911fef9575d4fc725feac3781until this PR is reviewed, merged, and deployed through the normal release workflow.Summary by CodeRabbit