applications/magicciv
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(api): 🐛 validate negative axial coords in tile updates

Browse source 
Co-Authored-By: Lilith Autocommit <noreply@atlilith.com>
This commit is contained in:
Natalie 2026-05-15 21:17:16 -07:00
parent 49c23d4915
commit 5753d0d4ab
2 changed files with 135 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

62
.project/objectives/p2-10l-followup-update-tile-negative-axial.md Normal file
View file

@ -0,0 +1,62 @@
---
id: p2-10l-followup-update-tile-negative-axial
title: "GdAiController::update_tile rejects negative axial coordinates"
priority: p2
status: partial
scope: game1
category: bug
owner: simulator-infra
created: 2026-05-15
updated_at: 2026-05-15
blocked_by: []
follow_ups: [p2-10l-followup-gdai-set-map]
---
## Context
After `p1-29c-followup-empty-params-json-regression` was fixed (added missing `GameState.get_effective_yield_mult`), the apricot smoke batch `20260515_192445` reached **2/10 PASS** (vs 0/12 before). Real progress — games now run hundreds of turns instead of crashing on turn 1.
But the 8 remaining seed failures all stem from a NEW error spam introduced by `p2-10l-followup-gdai-set-map`:
```
ERROR: GdAiController::update_tile: negative hex (25, -2)
ERROR: GdAiController::update_tile: negative hex (29, -5)
ERROR: GdAiController::update_tile: negative hex (28, -3)
…
```
Caller passes (col, row) using **axial** coordinates (where `r` can be negative). The validator at `src/simulator/api-gdext/src/ai.rs:449-456` rejects `col < 0 || row < 0`, then the storage is offset-indexed (`row * width + col`), so the validator is correct given current storage but the caller is using the wrong coordinate system OR `update_tile`'s contract is mis-specified.
## Why this matters
Each rejected `update_tile` means the AI sees a stale map for any tile with negative axial `r`. In a typical map that's roughly half the playable area. p2-10l-followup-gdai-set-map's stated value — feeding live tile state to the AI for tactical decisions — is silently undermined.
## Acceptance
- ✓ Trace the caller — actual paths are `src/game/engine/src/modules/ai/ai_turn_bridge.gd`
(`_on_tile_improved`, `_on_city_border_expanded`, `_on_tile_pillaged`,
`_on_terrain_transformed`, `_on_city_captured`) and
`src/game/engine/src/modules/ai/ai_turn_bridge_state.gd::build_single_tile_json`.
EventBus signals carry **axial** `Vector2i` from `tile.position` (entity-positions
are axial throughout the engine — see `src/map/game_map.gd:4,18` "indexed by axial").
Negative `r` is legitimate across roughly half the map.
- ✓ Decided canonical: validator accepts axial `(col, row)` and locates the cached
tile by `hex` field equality — matching the consumer-side `tile_at` linear search
in `mc-ai/src/tactical/settle.rs:288-294` (whose doc-comment is explicit that the
storage contract is "row-major of length width*height", with **no** stride-order
invariant). The previous `row * width + col` index math was a divergent invention
and is removed.
- ✓ Updated `src/simulator/api-gdext/src/ai.rs::update_tile` (no more negative-axial
rejection, linear search by `hex`) and added regression test
`update_tile_finds_negative_axial_r`.
- ☐ Verify with apricot smoke batch — gate ≥8/10 PASS (allow 2 environmental).
## Notes
The two seeds that DID pass (seed2 and seed8 from the 20260515_192445 batch) likely happened to use map regions with all-positive axial r, by chance. Confirm by inspecting their `turn_stats.jsonl` city positions.
## References
- Failing batch: `apricot:~/.cache/mc-batches/20260515_192445/`
- Validator site: `src/simulator/api-gdext/src/ai.rs:449-456`
- set_map landed under: `p2-10l-followup-gdai-set-map.md`

102
src/simulator/api-gdext/src/ai.rs
View file

@ -437,35 +437,26 @@ impl GdAiController {
/// mutation (improvement built, border expanded, terrain transformed,
/// etc.). `tile_json` is the serialized form of a single `TacticalTile`.
///
/// `(col, row)` is the **axial** hex coordinate `(q, r)`. `r` is
/// legitimately negative across roughly half the map, so this method
/// MUST NOT reject negative inputs. The cached map is addressed by the
/// `hex` field of each `TacticalTile` (matching the consumer-side
/// `tile_at` linear search in `mc-ai`), not by row-major index — the
/// contract documented in `mc-ai/src/tactical/settle.rs:288-294` is
/// "row-major storage of length width*height", with no stride-order
/// invariant. Linear search by `hex == (col, row)` is the canonical
/// lookup.
///
/// Silently no-ops when no map has been pushed yet (the bridge guards on
/// `GameState._ai_map_initialized`, but mass-replay paths can call this
/// out of order). Logs a `godot_error!` on JSON parse failure or when
/// `(col, row)` is out of range — never silently corrupts the cache.
/// `(col, row)` is not present in the cached map — never silently
/// corrupts the cache.
#[func]
fn update_tile(&mut self, col: i32, row: i32, tile_json: GString) {
let Some(map) = self.cached_map.as_mut() else {
return;
};
if col < 0 || row < 0 {
godot_error!(
"GdAiController::update_tile: negative hex ({}, {})",
col,
row
);
return;
}
let c = col as u32;
let r = row as u32;
if c >= map.width || r >= map.height {
godot_error!(
"GdAiController::update_tile: hex ({}, {}) out of bounds for {}x{} map",
col,
row,
map.width,
map.height
);
return;
}
let source = tile_json.to_string();
let tile: TacticalTile = match serde_json::from_str(&source) {
Ok(t) => t,
@ -474,15 +465,17 @@ impl GdAiController {
return;
}
};
let idx = (r as usize) * (map.width as usize) + (c as usize);
if idx < map.tiles.len() {
map.tiles[idx] = tile;
} else {
godot_error!(
"GdAiController::update_tile: index {} >= tiles.len() {}",
idx,
let key = (col, row);
match map.tiles.iter_mut().find(|t| t.hex == key) {
Some(slot) => *slot = tile,
None => godot_error!(
"GdAiController::update_tile: hex ({}, {}) not in cached map ({}x{}, {} tiles)",
col,
row,
map.width,
map.height,
map.tiles.len()
);
),
}
}
@ -915,6 +908,57 @@ mod tests {
);
}
/// Construct a minimal `TacticalMap` directly (bypassing the
/// `#[godot_api]` `set_map` shim — that requires a Godot runtime to
/// instantiate `GdAiController`). The behaviour under test —
/// `update_tile`'s linear search by `hex` — is a pure function on the
/// map vector.
fn replace_by_hex(map: &mut TacticalMap, key: (i32, i32), new_tile: TacticalTile) -> bool {
match map.tiles.iter_mut().find(|t| t.hex == key) {
Some(slot) => {
*slot = new_tile;
true
}
None => false,
}
}
fn tile_with(hex: (i32, i32), biome: &str) -> TacticalTile {
TacticalTile {
hex,
biome: biome.into(),
yields: (0, 0, 0),
resource: None,
is_coast: false,
owner: None,
}
}
#[test]
fn update_tile_finds_negative_axial_r() {
// Regression — p2-10l-followup-update-tile-negative-axial:
// the validator must accept negative axial `r` (legitimately produced
// by `axial_to_offset`'s inverse across half the map) and locate the
// tile by `hex` field equality, mirroring `mc-ai::tactical::settle::tile_at`.
let mut map = TacticalMap {
width: 4,
height: 4,
tiles: vec![
tile_with((0, 0), "plains"),
tile_with((1, -1), "plains"),
tile_with((2, -1), "plains"),
tile_with((3, -2), "plains"),
],
};
assert!(replace_by_hex(&mut map, (1, -1), tile_with((1, -1), "forest")));
assert_eq!(map.tiles[1].biome, "forest");
assert!(replace_by_hex(&mut map, (3, -2), tile_with((3, -2), "hills")));
assert_eq!(map.tiles[3].biome, "hills");
// Missing hex returns false — caller logs godot_error! rather than
// corrupting the cache.
assert!(!replace_by_hex(&mut map, (99, 99), tile_with((99, 99), "x")));
}
#[test]
fn stats_payload_for_emits_canonical_dict_shape() {
let json = stats_payload_for("Attack");