FEnigma/TODO.md

# Bug Backlog (from user report, 2026-08-11)

Status legend: [x] fixed+tested, [~] partially addressed, [ ] open/needs input

- [x] Allies and enemies seem to share indices.
      First pass on this was wrong: I only checked that targets and allies
      are separate id namespaces (they are, always were) and stopped there.
      The actual bug was one level down: `Board.add_target`/`add_ally`'s
      auto-id assignment (`used = {t.id for t in self.targets if t.type ==
      type_}`) was scoped **per type**, not per group — a Tank and an
      Infantry auto-added back to back both got id "A", each type getting
      its own independent A/B/C... sequence instead of sharing one across
      the whole group. Fixed: the id namespace split is targets-vs-allies
      ONLY, type never subdivides it further. New regression test
      (`test_auto_id_is_shared_across_types_within_targets_and_within_allies`).
- [x] Regression FROM the fix above, caught via a real traceback: sharing
      one A/B/C... sequence across a whole group (instead of per-type)
      made it much easier to actually run out of the 26 letters --
      `next(c for c in string.ascii_uppercase if c not in used)` raises
      `StopIteration` the instant all 26 are taken, silently killing
      whatever button click triggered `add_target`/`add_ally` (this is
      what "Accept as"/"Accept all" doing nothing turned out to be, see
      below). Fixed with `_next_free_id()`: rolls over to two-letter ids
      ("AA", "AB", ...) instead of raising, can't run out. New test
      (`test_auto_id_survives_past_26_entities_in_one_group`).
- [x] Ally type 'ally' is called Enemy on map title.
      `icons._target_type_label` (now public `icons.target_type_label`)
      already special-cased this for the type picker, but the map's
      right-click popover heading, "Change type (...)" button, and toast all
      printed `obj.type.value` directly instead, so an Ally with the
      ad-hoc TargetType.ENEMY still showed "Enemy" everywhere except the
      picker itself. Fixed in `app.py` (`_display_name`, and the three
      spots using it).
- [x] Reordering firing commands lags UI hard.
      `FiringPanel._reorder()` was calling `self.on_change()` — app.py's
      full-app refresh (re-solve every target's clue graph, dedupe, redraw
      the map, THEN rebuild the panel) — on every single drag-drop, even
      though reordering touches no location/clue/coord state at all. Now
      calls a local `self.refresh()` instead.
- [x] "Always show geo" doesn't reliably work / blast radius should stay
      shown too.
      `GridCanvas._draw_geo_overlays()`'s candidate list was
      `reference_points + targets` only — Allies have a `show_geo_desc` pin
      in the UI and can carry OCR'd clues too, but were never drawn.
      Added. `_draw_blast_radius()` only ever looked at `self.selected`,
      ignoring `show_geo_desc` entirely, so pinning it and then selecting/
      deselecting something else made it vanish; now iterates every
      selected-or-pinned target.
- [x] Clearing the board doesn't clear allies.
      `Board.clear()` cleared everything except `self.allies`. Fixed, plus
      the "clear board?" confirm-dialog's early-return guard (which skipped
      the whole action if only allies were on the board) now checks allies
      too.
- [x] Allow right-click on Clear button: clear all enemies/units/flights,
      keep spotters/RPs/nest.
      New `Board.clear_units()` + a right-click popover on the header's
      Clear button wired to it.
- [x] On map-reading error: save a screenshot locally to adapt the algo.
      New `debug_capture.py` — `save_map_read_failure()` writes the PNG +
      the solver's rejection reason under
      `$XDG_DATA_HOME/fenigma/debug_captures/failures/`, wired into
      `app.py`'s `_start_map_import`.
- [x] When the user corrects the grid, store screenshot + ground truth too.
      `debug_capture.save_grid_correction()`, wired into `_accept_grid`:
      fires only when the accepted `GridSolution` isn't the one auto-solve
      produced (the user actually dragged a handle in GridFixDialog), saves
      both solutions under `.../debug_captures/corrections/`.
- [x] Many map screenshots seem to get read as text; if nothing relevant is
      found, also store the image to check whether it was actually a map.
      `debug_capture.save_maybe_map()`, wired into `_ocr_png`: fires when a
      screenshot (not a plain-text paste) fell through to the OCR/text path
      and `_merge_all` found nothing at all. Saved under
      `.../debug_captures/maybe_map/`.
- [x] Unable to parse 3 given chat messages (Infantry "taking fire" fire-
      support requests).
      A different grammar from the existing Marine Garrison fire-support
      request: reversed shell word order ("Requesting X Shell" vs "X Shells
      requested"), a bare "before/by <time>" deadline (no "Requested"/
      dashes), and either a direct "on our position at <coord>" or a
      bearing/distance offset from that same inline position (not a named
      board entity, so resolved directly via
      `solver.point_from_bearing_distance` rather than through a Clue).
      New extractors in `ocr.py`, wired into `parse_intel_blocks`'s
      `flush()`. 3 new regression tests, all passing (`tests/test_ocr.py`).
- [x] Follow-up bug in the above: the bearing/distance-offset variant
      names TWO different places (the reporting unit's own position, and
      a separate fire point offset from it), but only produced one Target
      entity, sitting at the offset point but still labeled with the
      unit's own type/id (e.g. "Infantry#11" at a spot no infantry is
      actually at). Math itself was right; the single-entity shape wasn't.
      Now produces two entries: the original (Infantry#N etc.) keeps its
      own reported position with no shell/deadline, and a new synthetic
      `Strike#<TypeWord><id>` entry (e.g. `Strike#Infantry11`) carries the
      shell/deadline at the computed offset coord. 2 more regression tests.

- [x] When the user deletes/replaces the map screenshot, capture whatever
      units they confirmed as ground truth for it.
      `ScreenshotImport.baseline_targets`/`baseline_allies` (a snapshot of
      `board.targets`/`board.allies` taken when the grid is confirmed,
      `Target`/`Ally` are identity-hashable so these are plain sets of the
      live objects) let `app.py` tell "added while this screenshot was up"
      apart from "was already on the board". `Proposal` also now records
      `confirmed_type` (what the user actually accepted it as, which can
      differ from the detector's own guess via "Accept as..."). All of it
      -- every proposal's accept/reject/undecided verdict, plus every
      target/ally added with no matching proposal at all (a manual add or
      an OCR-text merge run alongside the screenshot) -- is saved via
      `debug_capture.save_marker_ground_truth()` under
      `.../debug_captures/marker_ground_truth/`. Wired into all three
      places a screenshot stops being "the active one": explicit drop, a
      new screenshot pasted straight over it, and window close.

## Resolved via a real traceback (not guessed)

- [x] "Accept as" / "Accept all" on proposed targets doing nothing.
      A real traceback from the running app nailed it: `StopIteration`
      from `Board.add_ally`'s id auto-assignment once 26 allies existed
      already (see the id-namespace regression entry above) — every
      accept attempt after that silently died before the ally/target
      ever got added, popover already closed by the time it happened.
      Fixed there; not a separate bug.
- [x] "Accept as…" (the type-picker submenu on a proposal, and "Change
      type" on an already-placed entity) opening to a visibly empty/
      unchanged popover. This one left no traceback at all -- confirmed
      live with temporary debug prints that the button's `clicked` signal
      fires, the icon grid builds successfully (all N types), and
      `Popover.set_child()` on the already-open outer popover reports the
      right `visible=True`/width/height afterward... but the compositor
      never actually repaints that reused surface, so nothing new ever
      appeared on screen. Fixed by not resizing the existing open
      popover at all: popping it down and opening a genuinely new one
      (fresh native surface) at the same anchor point instead. Same fix
      applied to both call sites (`_open_proposal_menu`'s `show_type`,
      `_open_entity_menu`'s `show_type`, the latter refactored to share
      the same `_reopen_with()` helper).
- [x] New: mark a Target as underground, at a hardening tier (1-3),
      rendered as the game's own Armor-tier additive badge stacked on
      the icon. `Target.underground_tier: int | None`, a "Mark
      underground" entry in the entity-edit popover (tier picker reusing
      the same fresh-popover fix above), and `GridCanvas` draws the
      badge above the marker's icon, overlapping down into it by
      `_ADDITIVE_OVERLAP_PX` -- both the diamond icon's top corner and
      the badge's bottom are tapered to a near-point, not a flat edge,
      so bbox-exact touching still read as a gap; a real pixel overlap
      is what actually looks contiguous (confirmed against the game's
      own stacked-badge screenshots). Badge is scaled/positioned off the
      art's real opaque content (PIL `getbbox()`), not its PNG canvas --
      the additive files carry a lot of off-center transparent padding
      that made the badge look tiny and floating if sized off the raw
      canvas.

## Needs more scope / your input before I keep going

- [~] Enemy type detection needs to be more robust; read the entity id
      label so dedup is reliable; detect death from the log.

      Started on the id-reading piece: `map_vision.read_marker_id` reads
      each marker's own small "#<N>" label (distinct from the big
      per-cell grid label `read_cell_label` reads) via the SAME template-
      correlation approach as `read_cell_label`, not OCR -- this text
      sits over the same aerial-photo backdrop that this module's own
      docstring says defeated every detection-based approach tried for
      grid labels, so pytesseract (already tried elsewhere in this repo,
      `ocr.py`, for a different image domain: flat scanned paper, not
      photo-textured) was skipped in favor of the approach already proven
      here. Wired end-to-end: `find_markers` -> `Proposal.detected_id` ->
      `debug_capture.save_marker_ground_truth`'s JSON. Reads against
      `ScreenshotImport.full_image` (sharper than the WORK_W image
      detection itself runs against) when available. Crop region and
      `MIN_MARKER_ID_SCORE` are a single-screenshot calibration (see
      `read_marker_id`'s own docstring) -- UNVALIDATED against a real
      ground-truth batch (none of the 6 existing captures have a
      confirmed id to check against, they all predate this). New unit
      tests (`tests/test_map_vision_marker_id.py`) only cover the
      synthetic-render round-trip, not real-screenshot accuracy.

      Measured type-detection reliability against the 6 existing
      `marker_ground_truth` captures (72 accepted proposals total,
      2026-08-13): **0/72 (0%) had ANY confident `detected_unit` guess**
      -- `classify_marker` returned `None` on every single one, every
      side, every capture. Not "guesses wrong" -- never confident enough
      to answer at all. Spot-checked directly against one real marker
      crop (a hostile Infantry, confirmed by the user): best match was
      "Underground Fort" at score 0.376 (Infantry wasn't even in the top
      8), against a `min_score=0.55` floor `classify_marker` requires --
      not a close miss, a real correlation failure. The clean rendered
      icon templates `icon_bank()` matches against apparently don't
      correlate well with how markers actually look in a real screenshot
      (compression/blur/aerial-photo texture underneath), unlike text
      glyphs (`read_cell_label`'s measured 0.73-0.87 vs 0.40-0.56) where
      the same template-correlation idea works well. Added `unit_score`/
      `unit_margin` to `Proposal`/ground-truth JSON (previously only
      pass/fail `unit` was logged) so every future capture shows exactly
      how far off a guess was, not just None -- there was no way to tell
      "barely missed the bar" from "wildly wrong" before this.

      Death-detection-from-log is still fully unstarted -- no log-parsing
      code exists in this repo at all yet, real scope work (find/access
      the game's log, agree a "<Type>#<id> Destroyed" grammar, wire it
      into a dedup key) rather than a quick pass.

      Follow-ups from user feedback after the above landed:
      - `_accept_proposal` now actually USES `detected_id` (it was only
        being logged before, never applied) -- an accepted proposal's
        entity id prefers the detected number over auto-assignment,
        falling back on a collision. 4 new regression tests
        (`tests/test_app_accept_proposal.py`).
      - Auto-assignment itself (`Board.add_target`/`add_ally` with no
        `id_`/no usable detection) changed from one shared letter
        sequence per group (targets, or allies) to its own sequence per
        TYPE within each group -- Tank#A/Infantry#A rather than
        Tank#A/Infantry#B. This directly reverses an earlier deliberate
        fix in this same file (see the "Allies and enemies seem to share
        indices" entry above, which moved FROM per-type TO
        shared-per-group) -- that fix is still correct for what it fixed
        (targets-vs-allies must stay separate namespaces), just not for
        per-type-vs-shared, which the user has now clarified the other
        way. Tests in `test_models.py` updated to match (renamed
        `test_auto_id_is_shared_across_types...` ->
        `test_auto_id_is_per_type...`, since it now asserts the opposite).

        First pass at this ALSO switched auto-assignment from letters to
        plain numbers (1/2/3...), reasoning that it should match what
        `detected_id` looks like when read successfully. Wrong -- caught
        by the user immediately: auto-assignment (no real id known, a
        manual add or an accept with no confident read) and a genuinely
        detected id need to stay visually distinct, or a made-up
        auto-assigned number could collide with, or be mistaken for, a
        real one. Reverted back to `_next_free_id` (letters, rolling
        over to "AA"/"AB"/... past 26 rather than raising
        `StopIteration`) as the auto-assignment fallback, scoped per
        type same as above; plain numbers are reserved for an id
        `_accept_proposal` is actually confident was read off the
        marker itself (`Proposal.detected_id`), passed straight through
        as `id_` and never touching auto-assignment at all.
      - `_accept_proposal` now actually USES `detected_id` (it was only
        being logged before, never applied) -- an accepted proposal's
        entity id prefers the detected number over auto-assignment,
        falling back on a collision (scoped per type, same bug fixed
        in two places: this collision pre-check, and the "Change ID"
        popover's own check, which still enforced the OLD shared-per-
        group rule after the auto-assignment change above and rejected
        valid renames across types). 4 new regression tests
        (`tests/test_app_accept_proposal.py`).
      - `detected_id` is now shown, not just logged: the proposal
        popover's heading (", id #8") and the pending-proposal's own
        on-map label (`? #8 G8 5:4`) both show it while there's still a
        screenshot up to check it against by eye.

## Windows build (packaging/windows) -- real progress, not yet a clean pass

Booted the actual dockur/windows build VM and drove it live (VNC) to find
out what's really failing, rather than guessing from the README's own
"UNTESTED end to end" note. Three real, separate bugs found and fixed,
each confirmed live against the real VM, not just read off a diff:

- [x] The build watcher was never actually installed at all, despite
      `install_progress.log` claiming every provisioning step succeeded.
      `C:\OEM` (dockur's `/oem` staging dir) doesn't reliably persist past
      Windows Setup finishing -- exactly what the (already-uncommitted,
      now committed) `install.bat`/`watch_build.bat` fix diagnosed, just
      never verified against a real run before now. A 2-day-old
      `BUILD_REQUEST` had been sitting unclaimed the whole time. Manually
      re-applied the fix's logic live once (copied the corrected files to
      `C:\FenigmaBuild`, registered the Startup-folder entry) and
      confirmed on a full container restart that the watcher now
      auto-starts on login and picks up a pending request with zero
      manual intervention -- the actual fix, not just my live patch, is
      what's doing that.
- [x] `pip install pytesseract` fails outright: MSYS2's mingw64 Python
      enforces PEP 668 ("externally-managed-environment"), which
      `install.bat` never accounted for. Needs `--break-system-packages`.
- [x] `import fenigma.app` fails with `ModuleNotFoundError: No module
      named 'cv2'` even after `pacman -S mingw-w64-x86_64-opencv`
      succeeds -- that package is the C++ library only. The actual
      Python bindings are a SEPARATE package, `mingw-w64-x86_64-
      python-opencv`, that `install.bat`'s dependency list simply never
      included. (`pip install opencv-python-headless` as a fallback
      doesn't work either and shouldn't be relied on: MSYS2's mingw64
      Python uses a different ABI than PyPI's Windows wheels
      -- `cp314-mingw_x86_64_msvcrt_gnu` vs `win_amd64` -- so pip can
      never use a prebuilt wheel there, only build from source, which
      then needs a full separate native toolchain -ninja/cmake/gcc- this
      VM doesn't have either.)

  All three are one-line fixes once known. `install.bat`'s pacman package
  list and pip install line need these applied for a from-scratch VM to
  provision correctly (currently they're only proven fixed live on this
  session's VM, not yet folded back into the committed `install.bat` --
  do that before relying on a fresh `./build_windows.sh` run from
  scratch).

  With all three fixed, `import fenigma.app` succeeds and a real build
  attempt got all the way through source copy, sanity check, dist-tree
  assembly, and WiX harvest+compile (`candle.exe`) -- further than this
  pipeline has ever gotten. Two more issues surfaced right at the finish
  line:
  - [x] `product.wxs`'s `Version` needs strict WiX `x.x.x.x` numeric
        form -- a `0.1.0-test` version string (my own test invocation,
        not `build_windows.sh`'s real default) fails `candle.exe` with
        CNDL0108/CNDL0010. Not a real bug, just don't pass a version with
        a suffix.
  - [x] `light.exe` (final MSI linking) did not finish within 15 minutes
        on a first retry (4 CPU / 8GB RAM VM) before the RAM-conscious
        auto-shutdown killed it -- turned out to be genuinely just slow
        (process was active, 343MB working set, not hung on a dialog),
        not a real bug: retried with a 40-minute budget and it finished
        `light.exe` itself in a few more minutes.
  - [ ] ...and then failed for a REAL reason right at the very end:
        `light.exe`'s ICE80 validation rejected essentially every
        harvested file -- "This 32BitComponent ... uses 64BitDirectory".
        `product.wxs`'s own `INSTALLFOLDER` is correctly under
        `ProgramFiles64Folder` (a 64-bit mingw64 toolchain is what's
        actually being packaged), but nothing was making the components
        agree -- a real, on-disk mismatch, not a transient VM issue.
        First fix attempt (`-platform x64` on `heat.exe`'s harvest) was
        WRONG -- re-verified live, identical ICE80 failures afterward
        (confirmed the corrected `build.bat` had actually reached the VM
        this time, ruling out a stale-copy repeat of the earlier watcher
        bug). WiX v3's `heat.exe -platform` only affects registry-key
        harvesting, it never stamps `Win64="yes"` on components. Real
        fix: `-arch x64` on `candle.exe` (the COMPILE step, not the
        harvest step) -- sets the default Win64/Platform for every
        component compiled from either source file, hand-authored
        (`product.wxs`) or harvested (`files.wxs`) alike, the standard
        WiX v3 way to make a whole package consistently 64-bit. Kept the
        harmless-but-insufficient `-platform x64` on `heat.exe` too.
        Applied, NOT YET re-verified live (this session's VM time is
        long since past reasonable for one sitting -- next run should
        confirm a real `.msi`, but treat that as still unconfirmed until
        it actually happens).
        Once confirmed, revisit the still-slow `light.exe` step itself
        (the ~1GB+ bulk-copied mingw64 dist tree, see README's own "not
        lean" note) as a separate, real perf follow-up -- 40 minutes for
        one MSI link is a lot, even once it stops failing outright.

## OCR: new fire-support-request grammar gaps (from real user-pasted messages)

- [x] A "taking fire" report's reporting unit ("Infantry#11 taking
      fire!...") was being added as a hostile Target, not a friendly
      Ally -- see the id-scheme entry above for the "no Friendly/Hostile
      prefix word exists in this grammar" root cause and the fix
      (`_TAKING_FIRE_RE`, `force_ally`, and splitting the shell/deadline
      into a synthetic StrikeRequest even for the no-offset "on our
      position" case, which previously kept them on the entity itself --
      fine when it was wrongly a Target, silently lost once correctly an
      Ally, since Ally tuples carry no shell/deadline fields at all). 4
      existing tests updated, all still passing plus the rest of the
      suite (51 total).

- [ ] A multi-shell sequential request ("Requesting TEAR Shell first,
      then HE Shell, at bearing...") only captures the FIRST shell
      (TEAR) into the structured `shell` field -- "then HE Shell" isn't
      parsed into anything. Less urgent than it first looked though: the
      full original message text (both shells, in order) is already
      preserved as-is and shown to the player via the coord dialog's
      description view (`Location.desc_raw`, set from the same `raw`
      every merged target/ally carries) -- nothing is silently LOST, it's
      just not machine-parsed into a queryable second-shell field. Real
      scope question before building that: does the board/firing-panel
      data model even have a place to put a second shell for one strike
      request today, or does this need a new field/shape entirely --
      worth confirming it's actually wanted (vs. "read the raw text
      yourself, it's right there") before spending the design effort.
- [x] "Answer by 10:30:00" turned out to be one bug, not two. The
      phrasing itself was never the problem -- `_TAKING_FIRE_TIME_RE`
      already matches any `before|by <time>`, "Answer BY 10:30:00"
      included. The REAL bug: a same-message "Important: ..." follow-up
      line was misread as a brand new named entity header (the
      last-resort bare-`<name>:` fallback rule matched "Important:"
      itself with nothing excluding common prose lead-ins), creating a
      bogus `Target#Important` that stole "Answer by 10:30:00" into ITS
      own `requested_time` instead of the real report's. Fixed with a
      blocklist (`_BARE_NAME_HEADER_BLOCKLIST`: important/note/warning/
      attention/caution/alert/reminder/priority) on that fallback rule --
      once the phantom split stopped happening, the deadline resolved
      onto the right entry with no separate fix needed. New regression
      test, confirmed against the user's real pasted message (with an
      assumed `Infantry#N taking fire!` header line prepended, since
      their paste seems to have been cropped before it).

- [x] "\<Type\>#\<id\> Destroyed" kill-feed parsing already exists and
      already marks the matching Target dead (`parse_destroyed`,
      `_merge_targets`'s own destroyed-handling block in app.py) --
      confirmed working end-to-end against a real 9-entry kill-feed
      paste, including multi-word types ("Enemy Mechanized Infantry#2
      Destroyed" correctly resolved to INFANTRY_MECHANIZED). This was
      already-existing, working functionality, not something needing to
      be built.
      One real gap found in the same test: "Enemy Field Gun#1 Destroyed"
      silently dropped. Two bugs stacked, both fixed:
      - [x] "Field Gun" is just the game's own alt name for plain
            Artillery (confirmed by the user directly) -- not a missing
            unit type needing a new enum member/icon after all. Added to
            `_TYPE_WORD_ALIASES` next to AmmoCache/CoastalBattery.
      - [x] Even with that alias, it still didn't resolve: `_ALLY_PREFIX_RE`
            only ever stripped a leading "Friendly"/"Hostile" word, never
            "Enemy" -- so `squash_multiword_ids`'s "EnemyFieldGun#1"
            token got alias-looked-up and fuzzy-matched as a WHOLE
            ("EnemyFieldGun" vs "Artillery", nowhere close), not just its
            "FieldGun" part. "Enemy Mechanized Infantry#2" only ever
            worked by fuzzy-match ACCIDENT (a long, distinctive type
            string still clears the ratio threshold with "Enemy" stuck
            to the front; a short, unrelated one like Artillery doesn't).
            `_ALLY_PREFIX_RE` now strips "Enemy" too, with a lookahead
            requiring something after it -- a BARE "Enemy#N" is
            `TargetType.ENEMY` itself (its own value IS "Enemy"),
            stripping unconditionally would've left an empty type_word
            and broken every ad-hoc "Enemy#N Destroyed" report instead.
      3 new regression tests, all passing (54 total).