fix(rewards): resolve item definitions up front and stabilize reward item order

Resolve every rolled ItemDefinition before mutating the character or
creating the CombatReward row, so a missing definition can no longer
half-grant (XP/silver saved, reward row created, then throw). Also
make the immediate grant response and a later loadRewards replay
agree on item order by sorting both on itemDefinitionId instead of
roll/insertion order.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

apps/api/src/rewards/combat-reward.service.spec.ts

@@ -21,6 +21,7 @@ const ROAD_BANDIT_ID = '30000000-0000-4000-8000-000000000002';
 const ASH_RAT_TABLE = '60000000-0000-4000-8000-000000000001';
 const ROAD_BANDIT_TABLE = '60000000-0000-4000-8000-000000000002';
 const BANDIT_BLADE = '50000000-0000-4000-8000-000000000002';
+const BANDIT_HOOD = '50000000-0000-4000-8000-000000000001';
 
 interface State {
   characters: Character[];
@@ -45,8 +46,25 @@ class FakeRepository<T extends { id: string }> {
     return Promise.resolve(this.rows.find((row) => this.matches(row, where)) ?? null);
   }
 
-  find(options: { where: Partial<T> }): Promise<T[]> {
-    return Promise.resolve(this.rows.filter((row) => this.matches(row, options.where)));
+  find(options: {
+    where: Partial<T>;
+    order?: Partial<Record<keyof T, 'ASC' | 'DESC'>>;
+  }): Promise<T[]> {
+    const matched = this.rows.filter((row) => this.matches(row, options.where));
+    if (!options.order) {
+      return Promise.resolve(matched);
+    }
+
+    // Mirrors TypeORM's `order` clause so tests can prove ordering comes from
+    // the query, not from insertion order happening to line up.
+    const [key, direction] = Object.entries(options.order)[0] as [keyof T, 'ASC' | 'DESC'];
+    const sorted = [...matched].sort((a, b) => {
+      const left = a[key];
+      const right = b[key];
+      const comparison = left < right ? -1 : left > right ? 1 : 0;
+      return direction === 'DESC' ? -comparison : comparison;
+    });
+    return Promise.resolve(sorted);
   }
 
   create(values: Partial<T>): T {
@@ -138,6 +156,14 @@ function createState(overrides: Partial<State> = {}): State {
   };
 }
 
+function fakeDataSource(state: State): EntityManager {
+  // `loadRewards`'s no-manager branch only calls `getRepository`, which
+  // `fakeManager` already implements identically for `DataSource`; reusing
+  // it (rather than duplicating the repository-resolution switch) keeps this
+  // fake backed by the exact same `state` rows as the transactional path.
+  return fakeManager(state);
+}
+
 function fakeLoot(...items: Array<{ itemDefinitionId: string; quantity: number }>): LootService {
   return { rollLoot: jest.fn().mockResolvedValue({ items }) } as unknown as LootService;
 }
@@ -337,6 +363,22 @@ describe('CombatRewardService', () => {
       expect(replayed).toEqual(granted);
       expect(loot.rollLoot).toHaveBeenCalledTimes(1);
     });
+
+    it('reads a persisted reward through the injected DataSource when no manager is passed', async () => {
+      const state = createState();
+      const loot = fakeLoot({ itemDefinitionId: BANDIT_BLADE, quantity: 1 });
+      const dataSource = fakeDataSource(state);
+      const subject = new CombatRewardService(dataSource as never, loot, fixedRandom(0));
+      const manager = fakeManager(state);
+
+      const granted = await subject.grantVictoryRewards(manager, combat());
+
+      // No manager argument: this is the non-transactional read the next
+      // task uses to render a reward screen.
+      const replayed = await subject.loadRewards(COMBAT_ID);
+
+      expect(replayed).toEqual(granted);
+    });
   });
 
   describe('failure handling', () => {
@@ -350,6 +392,54 @@ describe('CombatRewardService', () => {
         ),
       ).rejects.toMatchObject({ code: 'REWARD_STATE_INVALID' });
       expect(state.combatRewardItems).toHaveLength(0);
+      // Every rolled item definition is resolved before any mutation, so a
+      // missing one must leave no reward row and no character grant behind.
+      expect(state.combatRewards).toHaveLength(0);
+      expect(state.characters[0].experience).toBe(12);
+      expect(state.characters[0].silver).toBe(3);
+    });
+  });
+
+  describe('item order', () => {
+    it('orders items by itemDefinitionId in the immediate grant and the replayed reward, regardless of roll order', async () => {
+      const state = createState({
+        itemDefinitions: [
+          {
+            id: BANDIT_BLADE,
+            key: 'bandit-blade',
+            name: 'Räuberklinge',
+            type: ItemType.WEAPON,
+            rarity: ItemRarity.COMMON,
+            iconPath: '/images/items/bandit-blade.png',
+          } as ItemDefinition,
+          {
+            id: BANDIT_HOOD,
+            key: 'bandit-hood',
+            name: 'Räuberkapuze',
+            type: ItemType.ARMOR,
+            rarity: ItemRarity.COMMON,
+            iconPath: '/images/items/bandit-hood.png',
+          } as ItemDefinition,
+        ],
+      });
+      const banditCombat = combat({ monsterDefinitionId: ROAD_BANDIT_ID });
+      // Roll order is blade-then-hood, but BANDIT_HOOD's id sorts before
+      // BANDIT_BLADE's, so this only passes if both response paths sort by
+      // itemDefinitionId rather than returning rows in roll/insertion order.
+      const loot = fakeLoot(
+        { itemDefinitionId: BANDIT_BLADE, quantity: 1 },
+        { itemDefinitionId: BANDIT_HOOD, quantity: 1 },
+      );
+      const subject = service(state, loot, fixedRandom(0));
+      const manager = fakeManager(state);
+
+      const granted = await subject.grantVictoryRewards(manager, banditCombat);
+      const replayed = await subject.loadRewards(banditCombat.id, manager);
+
+      const expectedKeys = ['bandit-hood', 'bandit-blade'];
+      expect(granted.items.map((item) => item.item.key)).toEqual(expectedKeys);
+      expect(replayed?.items.map((item) => item.item.key)).toEqual(expectedKeys);
+      expect(replayed).toEqual(granted);
     });
   });
 });

apps/api/src/rewards/combat-reward.service.ts

@@ -84,6 +84,25 @@ export class CombatRewardService {
     );
     const roll = await this.lootService.rollLoot(monster.lootTableId, manager);
 
+    // Resolve every rolled item definition up front, before any mutation, so
+    // a missing definition throws `rewardStateInvalid()` before the
+    // character's XP/silver are touched or a `CombatReward` row is created.
+    // This keeps a failed grant from leaving partial writes behind.
+    const definitions = manager.getRepository(ItemDefinition);
+    const resolvedDefinitions = new Map<string, ItemDefinition>();
+    for (const rolled of roll.items) {
+      if (resolvedDefinitions.has(rolled.itemDefinitionId)) {
+        continue;
+      }
+      const definition = await definitions.findOneBy({
+        id: rolled.itemDefinitionId,
+      });
+      if (!definition) {
+        throw rewardStateInvalid();
+      }
+      resolvedDefinitions.set(rolled.itemDefinitionId, definition);
+    }
+
     const characters = manager.getRepository(Character);
     const character = await characters.findOne({
       where: { id: combat.characterId },
@@ -105,18 +124,12 @@ export class CombatRewardService {
       }),
     );
 
-    const definitions = manager.getRepository(ItemDefinition);
     const characterItems = manager.getRepository(CharacterItem);
     const rewardItems = manager.getRepository(CombatRewardItem);
-    const items: CombatRewardItemDto[] = [];
+    const granted: Array<{ itemDefinitionId: string; dto: CombatRewardItemDto }> = [];
 
     for (const rolled of roll.items) {
-      const definition = await definitions.findOneBy({
-        id: rolled.itemDefinitionId,
-      });
-      if (!definition) {
-        throw rewardStateInvalid();
-      }
+      const definition = resolvedDefinitions.get(rolled.itemDefinitionId)!;
 
       const existingStack = await characterItems.findOne({
         where: {
@@ -146,9 +159,18 @@ export class CombatRewardService {
         }),
       );
 
-      items.push(this.toItemDto(characterItem.id, definition, rolled.quantity));
+      granted.push({
+        itemDefinitionId: rolled.itemDefinitionId,
+        dto: this.toItemDto(characterItem.id, definition, rolled.quantity),
+      });
     }
 
+    // The immediate response and a later `loadRewards` replay must agree on
+    // item order; both sort on the same stable key (itemDefinitionId, which
+    // `toDto`'s query also orders by) rather than roll order.
+    granted.sort((a, b) => a.itemDefinitionId.localeCompare(b.itemDefinitionId));
+    const items = granted.map((entry) => entry.dto);
+
     return { experience, silver, items };
   }
 
@@ -169,9 +191,15 @@ export class CombatRewardService {
     scope: RepositoryScope,
     reward: CombatReward,
   ): Promise<CombatRewardDto> {
+    // Ordered by itemDefinitionId to agree with the sort `grantVictoryRewards`
+    // applies to its own response — the immediate grant and a later replay
+    // must list items identically.
     const rewardItems = await scope
       .getRepository(CombatRewardItem)
-      .find({ where: { combatRewardId: reward.id } });
+      .find({
+        where: { combatRewardId: reward.id },
+        order: { itemDefinitionId: 'ASC' },
+      });
     const definitions = scope.getRepository(ItemDefinition);
 
     const items: CombatRewardItemDto[] = [];