[dmccoystephenson]

Summary

• Grid::removeLocation previously reset the ids of every entity in the removed location, even one also stored in another location. Such an entity stayed present with "N/S" and -1 ids, and a directional move on it threw. It now keeps its grid id, and if its location id named the removed location, that id now names a location still holding it. An entity left in no location is detached as before.
• Grid::getLocation and Grid::getLocationByCoordinates errors now name the id or coordinates requested and the grid's id, matching Environment::getEntity. The exception type is unchanged.
• The README "Entity Identity" and "Errors" sections are updated. The class table is unaffected.

Design choice

The new check compares addresses, unlike the library's usual id matching. With id matching, a same-id twin elsewhere in the grid would have kept a genuinely detached entity's ids alive, which would have been a regression. Test 74 pins this. When an entity remains in several locations, its location id is set to the first of them in grid order.

Test plan

• CI "Library tests" failed on the tests-only commit a723e09, aborting on the Test 72 assertion entity.getGridId() == 5 (exit 134)
• CI "Library tests" failed on the removeLocation commit 6ec604f, with Tests 72-75 passing and Test 76 aborting on its first assertion (exit 134)
• CI "Library tests" passed on the final commit e14728c, with 76 of 76 registered tests reporting success, and the CMake ctest step passed
• Test 73 was not observed red on its own, because assert aborted at Test 72 first. On the old code it would fail on the same grid id reset.
• run_tests_asan.sh was not run: CI has no AddressSanitizer job (tracked in CI does not run run_tests_asan.sh, so AddressSanitizer failures are not caught on PRs #75), and local execution is denied. The change copies a vector of pointers and alters no allocation or entity lifetime.

Local execution of tests_executable is denied in this session, so CI is the test anchor.

Skipped

#75 was left for a later cycle. Its fix edits .github/workflows/ci.yml, which is on the do-not-auto-merge list, so it needs human review and is independent of this change.

Closes #73
Closes #74

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

🤖 Generated with Claude Code

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Self-review rubric (head e14728c). This is a same-session review anchored on CI, not an independent one.

• Scope: PASS — the four changed files all serve Grid::removeLocation clears the ids of an entity that is still placed in another location #73 or Grid location lookups throw 'Location not found' without naming what was looked up #74. src/grid.cpp and src/header/grid.h hold both changes, src/tests.cpp holds Tests 72-76, and README.md has one sentence added to each of two paragraphs.
• Tests-new: PASS — no public method was added. The new private Grid::updateIdsOfDetachedEntity is exercised through removeLocation by Tests 72-75.
• Tests-fix: PASS, with one gap — red runs were taken from CI because local execution is denied. Test 72 failed on a723e09 at entity.getGridId() == 5, and Test 76 failed on 6ec604f at its first assertion. Both pass on e14728c. Test 73 was not observed red alone, since assert aborted at Test 72 first. Tests 74 and 75 are guards for the new code path and pass on the old code by design, so they carry no red evidence.
• Sibling structure: PASS — no new file. The new tests follow the Test N - ... / --- Success pattern, and CRLF line endings were preserved in src/tests.cpp.
• Sibling renames: PASS — nothing was renamed.
• Docs: PASS — the "Entity Identity" and "Errors" sections were updated, and the ## Classes table is unaffected.
• Issue resolution: PASS — Grid::removeLocation clears the ids of an entity that is still placed in another location #73's surface (Grid::removeLocation) and both lookups named in Grid location lookups throw 'Location not found' without naming what was looked up #74 were changed, and each symptom is asserted. The directional-move throw from Grid::removeLocation clears the ids of an entity that is still placed in another location #73 is covered by the moveEntityUp assertion in Test 72.
• Local test suite: PASS via CI — Library tests (make + CMake) reported 76 successes for 76 registered tests, and ctest passed. The Strata and browser jobs also passed on e14728c.
• Header/definition parity: PASS — the one new declaration in src/header/grid.h is defined at src/grid.cpp:77.
• Include guards: PASS — no new header.
• Test registration: PASS — all five new tests are called from main(), and the count of 76 agrees.

Findings folded in from the diff:

• src/grid.cpp:66 — the pointer vector is copied before erase because the element is destroyed by it. updateIdsOfDetachedEntity then runs against the remaining locations, so another location sharing the id is seen on the first pass and handled on its own later pass (Test 75).
• src/grid.cpp:81 — this is an address comparison, unlike the id matching used elsewhere. It was chosen deliberately so that a twin does not keep a detached entity's ids alive (Test 74). Flagged as a judgment call, not as blocking.
• Judgment call: when an entity remains in several locations, the first in grid order is named. The README describes this only as "a location that still holds it", so the order is not promised.

Gaps: run_tests_asan.sh was not run, since CI has no AddressSanitizer job (tracked in #75) and local execution is denied. The change adds a local copy of a pointer vector and alters no allocation or entity lifetime, but the sanitizer result remains unverified.

Merge-gate check: no changed path matches the do-not-auto-merge list. No workflow file or Makefile was changed, and no single file lost more than 50 lines.

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

---

drafted by Claude on behalf of Daniel Stephenson