Fix iterator invalidation in Grid::removeLocation

[dmccoystephenson]

Summary

• Grid::removeLocation erased through an iterator and then incremented that same, now-invalidated iterator. The return value of std::vector::erase is now captured, and the iterator is only advanced when no erasure occurred.
• The target location id is now copied into a local before the loop, so an argument that itself refers to an element of locations is never dereferenced after the erasure that invalidates it.
• Test 35 (testRemovingLocationFromGrid) and Test 36 (testRemovingLocationFromMiddleOfGrid) were added to src/tests.cpp and registered in main(). The first adds a sentinel location ("extra-location") to a grid and removes it; the second removes an interior location so that the post-erase continuation path is exercised rather than only the trailing-element case that exits the loop immediately. Both assert the resulting location count and that no surviving location carries the removed id.
• The stale comment in src/tests.cpp that documented Grid::removeLocation as intentionally untested was removed, since the function is now covered.

Closes #31

Test plan

• make — compiles cleanly with no warnings or errors.
• bash run_tests.sh — UNVERIFIED. Compilation succeeds, but execution of the resulting tests_executable binary is denied by this session's command allow-list, so the suite could not be run and the stash-and-run regression check (assert failure with the fix reverted, success with it restored) could not be performed empirically. Because src/grid.cpp and src/tests.cpp are modified, this PR is deliberately left open for a human with a working runtime rather than merged autonomously.

Deferred this cycle

• run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33 (run_tests_asan.sh disables leak detection for a leak that PR Fix exception-by-pointer leak; add missing test coverage #32 already fixed) was not picked up: its acceptance criterion is a confirmed clean AddressSanitizer run, and neither the sanitizer build nor the resulting binary can be executed in this session. Re-enabling leak detection without observing a clean pass would be an unverified change to the project's memory-safety gate, so the issue is left open.
• A separate behavior gap in the same function — entities of a removed location keep a locationId naming a location the grid no longer holds — was filed as Grid::removeLocation leaves entities in the removed location pointing at a nonexistent location id #36 rather than folded in here, to keep this PR scoped to the iterator-invalidation defect.

Documentation check

README.md's ## Classes table was reviewed against src/header/*.h: the four listed classes (Entity, Environment, Grid, Location) and their descriptions remain accurate, and no class or public surface was added by this change.

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Self-review rubric (scored against the diff and against command output, adversarially — assume FAIL absent direct evidence):

• Scope: PASS — only src/grid.cpp and src/tests.cpp are touched, both required by Grid::removeLocation invalidates its iterator after erase, causing undefined behavior #31. One addition goes beyond the patch proposed in the issue; see the note below.
• Tests-new: PASS — no new public method is introduced, and the previously untested Grid::removeLocation is now exercised by two cases (trailing element and interior element).
• Tests-fix: FAIL — unverifiable in this session. The prescribed empirical check (stash the fix, confirm the new tests fail, restore, confirm they pass) could not be performed, because execution of the compiled tests_executable is denied by this session's command allow-list. The check is not being scored from reasoning; it is being reported as not performed.
• Sibling structure: PASS — both new functions follow the surrounding void test<Behavior>() / "Test N - <description>" / " --- Success" pattern and are registered by name in main().
• Sibling renames: no signal — no identifier was renamed.
• Docs: PASS — the only documentation source of truth is README.md's ## Classes table, which lists Entity, Environment, Grid, Location; no class or public surface changed.
• Issue resolution: PASS — Grid::removeLocation invalidates its iterator after erase, causing undefined behavior #31 names Grid::removeLocation in src/grid.cpp as the defect site, and that function is the one changed; nothing in Grid::removeLocation invalidates its iterator after erase, causing undefined behavior #31 is left partially addressed.
• Local test suite: FAIL — unverified. make compiles cleanly, but the suite itself was never run for the reason above.
• Header/definition parity: PASS — no declaration was added to src/header/grid.h, so no definition can be missing.
• Include guards: no signal — no header file was added.
• Test registration: PASS — src/tests.cpp:690 and src/tests.cpp:691 call testRemovingLocationFromGrid() and testRemovingLocationFromMiddleOfGrid() from main().

Findings folded in from the line-level pass (no formal review object is available in this session, so they are recorded here):

• src/grid.cpp:60 — std::string targetId = location.getId(); is not part of the patch proposed in Grid::removeLocation invalidates its iterator after erase, causing undefined behavior #31. It is included because the same function has a second latent hazard: an argument that aliases an element of locations is left dangling by the erase, and the original loop kept calling location.getId() afterwards. Reviewers who prefer the issue's literal patch should say so and this line can be dropped.
• src/tests.cpp:619 — against the pre-fix implementation this test signals the defect by aborting on undefined behavior, not by a clean assertion failure. That is a real signal but a fragile one; the assertions themselves only distinguish correct from incorrect behavior in a build where the UB does not happen to be benign.
• src/tests.cpp end of file — the file still lacks a trailing newline. This is pre-existing and was deliberately not corrected here, to keep the diff scoped.
• src/grid.cpp:57–68 — a behavior gap was noticed while writing these tests: a removed location's entities keep a locationId naming a location the grid no longer holds, which contradicts the convention Grid::removeEntity follows. It is out of scope here and has been filed as Grid::removeLocation leaves entities in the removed location pointing at a nonexistent location id #36.

Summary: the change itself is the standard erase-return idiom and is scoped tightly to #31, but two rubric items could not be anchored on a real run, so this PR should not be merged autonomously — a human with a working runtime should execute bash run_tests.sh (and ideally the stash-and-run check) before merging.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson