fix(sim): transfer subject/vassal bonds during empire merges - #39
Conversation
mergeEmpires transferred fleets, artifacts, factions, alliances, and relationships from the absorbed empire to the dominant one, but never touched state.subjects. A merged empire that was a vassal/tributary (or an overlord of others) left dangling subject records, which the next stepSubjects pass would either silently drop or resolve into a spurious liberation event instead of correctly transferring the bond. Subject ties now transfer to the dominant empire, mirroring the cleanup removeEmpireFromGalaxy already does on empire death. A tie directly between the absorbed and dominant empire collapses (both sides are now one empire); a transfer that would violate the one-overlord/no-cycle invariant falls back to liberation. Declared build/lint/headless-report checks in .noet/VERIFICATION.md and recorded evidence via noet verify. Also recorded a constraint: new per-empire relational state needs transfer logic in both mergeEmpires and removeEmpireFromGalaxy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018o55CaUYZM5bNccWjsctnc
csheldrick
left a comment
There was a problem hiding this comment.
Two changes are needed before I would merge this.
-
Avoid the new
Merge.ts↔Subjects.tscircular dependency.Subjects.tsalready importsmergeEmpires; this PR makesMerge.tsimportbreakSubjectRelation,subjectOf, andsubjectsOfback fromSubjects.ts. Even if the current ESM cycle happens to initialize successfully, it creates a fragile dependency between core simulation modules. Move the shared subject-query/transfer primitives into a neutral module, or implement the merge-side state transformation without importing the module that callsmergeEmpires. -
Add focused regression coverage for the subject-transfer branches. The 10k-tick report is useful integration evidence, but it does not prove the specific cases introduced here. Cover at least: absorbed subject transfers to dominant; absorbed overlord's subjects transfer to dominant; a direct dominant↔absorbed subject tie collapses; conflicting transfer liberates instead of creating a second overlord or chain; no surviving relation references the absorbed ID.
The Noet record added by this PR is genuinely useful and appropriately captures the cross-cutting invariant. The implementation direction also looks correct; these requests are about making the fix structurally safe and checkable.
…er tests Address review feedback on PR #39: - Extract subject-relation primitives (subjectOf, subjectsOf, isSubjectPair, createSubjectRelation, breakSubjectRelation, SUBJECT_STATUS_LABEL, STATUS_PROFILE) into a new SubjectRelations.ts with no dependency on Merge.ts. Merge.ts now imports from there instead of from Subjects.ts, breaking the Merge <-> Subjects cycle (Subjects.ts still imports mergeEmpires for peaceful integration). Subjects.ts re-exports the primitives so existing call sites (Diplomacy, Tick, Simulation, UI panels) are unaffected. - Add src/sim/Merge.test.ts using Node's built-in test runner, covering the five subject-transfer branches introduced by the merge fix: absorbed's subject status transfers, absorbed's own subjects transfer, a direct dominant<->absorbed tie collapses, and conflicting transfers on both the subject and overlord side liberate instead of violating the one-overlord/ no-cycle invariant. Verified these tests fail against the pre-fix code and pass against the fix. - New `npm run test:unit` script and CI step; exclude *.test.ts from the browser tsconfig project (it has no Node types) and note the new unit tests in CLAUDE.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018o55CaUYZM5bNccWjsctnc
|
Addressed both points:
All declared Generated by Claude Code |
Summary
mergeEmpirestransferred fleets, artifacts, factions, alliances, and diplomatic relationships from the absorbed empire to the dominant one, but never touchedstate.subjects. A merged empire that was a vassal/tributary (or an overlord of others) left dangling subject records — the nextstepSubjectspass would silently drop the relation or fire a spurious liberation event, instead of correctly carrying the bond forward.removeEmpireFromGalaxyalready performs on empire death. A tie directly between the absorbed and dominant empire collapses (both sides are now the same empire); a transfer that would violate the one-overlord/no-cycle invariant falls back to liberation, same as elsewhere in the codebase.npm run build/npm run lint/npm run reportas checks in.noet/VERIFICATION.mdand recorded passing evidence vianoet verify run+noet verify check(includes the determinism replay guard)..noet/CONSTRAINTS.md: new per-empire relational state needs transfer logic in bothmergeEmpiresandremoveEmpireFromGalaxy, since this bug is exactly a case of the former being added without touching the latter's counterpart.Test plan
npm run build(typecheck + Vite build)npm run lintnpm run report -- --milestones 1000,3000,10000— 10k-tick run with 20 empire merges and ongoing subject churn, no crashes; determinism replay check passednoet verify run/noet verify check— all declared checks green and fresh🤖 Generated with Claude Code
Generated by Claude Code