feat: let tribes declare their own tech tree

[Enn3Developer]

Tribes can now replace nodes of the default tech tree with their own, no wiring to stars/cities/effects yet.

The default tree is data now

The five branches were hardcoded in TechTree, so there was nothing to patch. They now live in resources/tech_tree.json (embedded like troops.json and tribes.json) and get loaded into a TechTreeDefinition, which only holds the shape of the tree: which node is in which branch and in which slot. The researched state stays in the TechTree built from it, so the definition is shared by every player and is immutable.

FromSerializedData refuses a tree where a branch is missing, declared twice, doesn't have exactly 5 nodes or where the same node id shows up more than once.

Overrides

A tribe declares what it replaces:

"tech_overrides": [
  { "replaces": "fishing", "id": "free_diving" }
]

The replaced node is found by its id because every id is unique in the tree, so the branch and the slot don't need to be written down and can't contradict each other. The new node takes the slot of the old one, which means it also takes its tier and its cost.

Overrides only swap in place, they never add or remove nodes: the cost of a node comes from its slot, so a branch with 6 nodes would have a node without a tier. SubTreeTech now throws if it doesn't get exactly MAX_NODES nodes, so a broken tree fails at load instead of silently miscosting techs.

Override returns a new definition, CreateTechTree(tribe) applies the overrides of the tribe and researches its starting node.

Starting tech

Tribe.StartingTech was a {branch, id} pair written by hand: as soon as a tribe overrode its tier 0 node, that id had to change with it or the tribe would silently start with nothing researched. It's now just StartingBranch, the starting node is the tier 0 node of that branch resolved after the overrides, so the two can't drift apart.

Other things in here

• ComputeCost reads the tier off the node instead of scanning for the index; the formula is now (cities * (tier + 1)) + 4, same numbers as before
• Research returns whether the node was found, so a typo isn't silent anymore
• TechTree[branch] and TechTreeDefinition[branch] still throw on an invalid BranchType

Two bugs found on the way

tribes.json didn't deserialize at all: starting_stars was a sibling of tribe instead of a field of it, and TribeSerializedData.TribeType doesn't map to "type" under SnakeCaseLower. Both fixed (the property is now Type, like TroopSerializedData), and there's a test loading the resource so it can't rot again.

TechOverrides can't default to an empty list: the source generated deserializer skips property initializers on types with required properties, so it came back null. It's nullable now.

Tests

18 tests in TechTreeTest, covering the tree loading and its validation, costs and tiers, research, overrides (in place, chained, unknown node, duplicated id, definition untouched), the starting branch with and without an override, and tribes.json loading. Full suite is green: 62 passed, 0 failed.

---

Generated by Claude Code

[Enn3Developer]

Adversarial review

Reviewed the full diff at 1d387f8. No .NET SDK is available in this environment, so everything below is from reading the code — I could not build or run the suite, and the CS1572/IDE0021 calls are from the rules in .editorconfig and <GenerateDocumentationFile>, not from an observed build log.

The direction is right and the two bugs you found on the way are real fixes. Moving the tree into data, deriving the starting tech from the branch after overrides, and keying overrides on the node id rather than a {branch, slot} pair all remove ways for the data to contradict itself. I checked for call sites outside the diff — StartingTech, new TechTree() and TribeSerializedData.TribeType have no other consumers in the solution, so the breaking renames are self-contained. ComputeCost's new formula matches the old one on every slot.

Eight findings, none blocking on their own:

Invariants that moved out of the type they belong to

1. SubTreeTech's constructor validates the node count but not the tiers — and cost now comes from Tier, not from the slot. The count check doesn't prevent the miscosting the PR description says it prevents.
2. TechTreeDefinition's indexer returns the internal string[] behind an IReadOnlyList<string>, so the "immutable, shared by every player" definition is one cast away from being mutated in place.

Bad data that gets in
3. FromSerializedData rejects a missing branch but not an extra one — "type": 9 deserializes fine (allowIntegerValues defaults to true), passes all four validations, poisons the duplicate-id check, and is then silently dropped by TechTree.
4. Tribe.StartingBranch and Tribe.TechOverrides are validated only when CreateTechTree runs, so a malformed tribe fails during game setup rather than at load.

Silent failure
5. ComputeCost still answers 0 for an unknown id. You hardened Research against exactly this, and 0 is a valid price — once stars are wired up, a typo'd tech is free.

Coverage
6. Two of the four FromSerializedData validations have no test, and the "one definition, independent per-player state" property isn't asserted anywhere.

Build hygiene
7. <param name="nodes"> survived the removal of SubTreeTech's primary constructor → CS1572, which .editorconfig does not silence and docs.yaml builds through.
8. Two expression-bodied constructors against csharp_style_expression_bodied_constructors = false:warning; they're the only ones in the solution.

Details and suggested fixes are inline. My picks for this PR are 1, 3 and 5 — 1 because the type now carries a cost-affecting field nothing checks, 3 because FromSerializedData is the only gate in front of the data, and 5 because it turns into a free-tech bug the moment the wiring lands. 2, 4, 6 are fine as follow-ups; 7 and 8 are one-liners.

---

Generated by Claude Code

[Enn3Developer]

The node-count check doesn't actually prevent miscosting — the tier does, and it isn't checked.

The PR description says SubTreeTech now throws on the wrong node count "so a broken tree fails at load instead of silently miscosting techs". But cost no longer comes from the slot, it comes from NodeTech.Tier, and this constructor accepts whatever tiers it is handed. The class remark right above states the invariant ("node 0: tier 0; node 1 and node 2: tier 1; node 3 and node 4: tier 2") and nothing enforces it.

var branch = new SubTreeTech([
  new NodeTech { Id = "climbing",   Tier = 0 },
  new NodeTech { Id = "mining",     Tier = 0 },   // should be 1
  new NodeTech { Id = "meditation", Tier = 0 },   // should be 1
  new NodeTech { Id = "smithery",   Tier = 0 },   // should be 2
  new NodeTech { Id = "philosophy", Tier = 0 }    // should be 2
]);
branch.ComputeCost("philosophy", 4); // 8, should be 16

Five nodes, constructor happy, every tier-2 tech at half price. Today the only caller is TechTree's constructor which passes TierOf(index), so the shipped path is correct — but the type is public with a public constructor and a public Nodes array, and the invariant now lives in the caller instead of in the type.

Since Tier is a pure function of the slot, the cheapest fix is to not let it be passed in at all — have SubTreeTech take the ids and stamp the tiers itself:

public SubTreeTech(IReadOnlyList<string> ids) {
  if (ids.Count != MAX_NODES) { throw ... }
  Nodes = [.. ids.Select((id, index) => new NodeTech { Id = id, Tier = TierOf(index) })];
}

That also removes Tier from NodeTech's required init surface, deletes the duplicated Select in TechTree's constructor, and makes TierOf an implementation detail rather than a contract callers have to remember to honour. If you'd rather keep the current constructor, at least assert nodes[i].Tier == TierOf(i) in the loop.

---

Generated by Claude Code

[Enn3Developer]

An unknown id costs 0 stars — the same silent failure Research was just hardened against.

Research now returns false on a typo so it can't fail silently, which is the right call. ComputeCost sitting right next to it still answers 0 for an id that isn't in the branch, and 0 isn't a sentinel here — it's a perfectly valid price the caller will happily charge:

var cost = branch.ComputeCost("free_divng", cities); // typo → 0
if (player.Stars >= cost) { player.Stars -= cost; branch.Research("free_divng"); }

The player pays nothing, Research returns false, the return value is dropped at the call site, and the tech is never researched. Once stars/cities get wired up this is a free-tech bug that no test will catch, because TestComputeCost asserts exactly this behaviour (branch.ComputeCost("swimming", 2).ShouldBe(0u)).

It's pre-existing behaviour, but the method is being rewritten in this PR and the sibling method is being fixed for precisely this reason, so it's worth making the pair consistent. uint? with null for "no such node" is the smallest change and forces callers to deal with it; throwing is also defensible given Override/FromSerializedData already treat an unknown id as an error everywhere else in this file.

---

Generated by Claude Code

[Enn3Developer]

This checks that no branch is missing, but not that no extra branch is present — an undefined BranchType passes every validation and is then silently dropped.

[JsonConverter(typeof(JsonStringEnumConverter<BranchType>))] is applied with the parameterless constructor, so allowIntegerValues defaults to true and a numeric type deserializes without complaint:

{ "type": 9, "nodes": ["a", "b", "c", "d", "e"] }

(BranchType)9 isn't a declared member, but nothing here rejects it:

• the node-count check passes (5 nodes)
• TryAdd succeeds (no collision with the real branches)
• this loop passes, because all five real branches are still present
• the duplicate-id check does include its nodes, so a phantom branch can make a legitimate tree fail to load with a confusing "node X is declared more than once"

Then TechTree's constructor iterates Enum.GetValues<BranchType>() and drops it — five node ids that exist in the definition, are reachable through Override, and are unreachable from every TechTree. Override on one of those ids succeeds and applies to nothing.

FromSerializedData is the only gate in front of this data, so it's worth closing here. Either validate in the first loop:

if (!Enum.IsDefined(branch.Type)) {
  throw new ArgumentException($"branch {branch.Type} isn't a valid branch", nameof(data));
}

or, equivalently and more cheaply, replace this loop with a single branches.Count != Enum.GetValues<BranchType>().Length check plus the existing per-branch ContainsKey — count equality on top of "every declared branch is defined" rules out both missing and extra in one go.

---

Generated by Claude Code

[Enn3Developer]

The class doc promises immutability, but this hands out the internal array.

IReadOnlyList<string> is a read-only view, not a read-only object — nodes here is the very string[] stored in _branches, and string[] implements IReadOnlyList<string>, so the reference survives a cast:

((string[])definition[BranchType.Fishing])[0] = "not_fishing";

That mutates the shared definition in place. Given the remark on the class ("shared by every player", "immutable, Override returns a new definition instead of modifying this one"), the blast radius is every player created afterwards, plus every Override derived from it — Override clones from _branches, so it inherits the corruption rather than isolating it.

TechTree's constructor is currently the only consumer and only reads, so nothing is broken today; it's the invariant that's unguarded. Cheapest fix is to hand out a real wrapper:

private readonly Dictionary<BranchType, ReadOnlyCollection<string>> _branches;

building each with Array.AsReadOnly(...) in FromSerializedData/Override (which already own freshly-allocated arrays, so no extra copying), or store ImmutableArray<string> if you'd rather not pay the wrapper allocation. Either way Override's .Clone() and the string[]-typed Find helper stay internal, which is where the mutability belongs.

---

Generated by Claude Code

[Enn3Developer]

Nothing validates StartingBranch, so a bad tribe fails during game setup with an undocumented exception instead of at load.

Same root cause as the BranchType note in TechTree.cs: JsonStringEnumConverter<BranchType> accepts integers, so "starting_branch": 9 deserializes fine, RegisterTribes accepts it, and TribeManager[TribeType.X] hands back a Tribe that looks valid. The failure only surfaces here:

techTree[tribe.StartingBranch].Research(definition[tribe.StartingBranch][0]);

as an ArgumentOutOfRangeException out of TechTree's indexer — halfway through building a player, after some players have already been created. CreateTechTree's <exception> tag only documents ArgumentException from Override, so this one is undocumented as well.

TechOverrides on the next property has the same shape of problem: the ids in it are only checked when CreateTechTree runs, so a tribe that names a node that doesn't exist is accepted at registration and blows up later.

RegisterTribe/RegisterTribes is the natural place to catch both — it's the one chokepoint every tribe goes through, and it already throws (Tribes.Add) on a duplicate type, so validating there is consistent. An Enum.IsDefined(tribe.StartingBranch) check plus a dry-run definition.Override(tribe.TechOverrides) would turn every one of these into a load-time error with the tribe's name in the message. That does mean handing the definition to TribeManager, which may be more coupling than you want right now — if so, at least document the ArgumentOutOfRangeException on CreateTechTree.

---

Generated by Claude Code

[Enn3Developer]

<param name="nodes"> is left over from the primary constructor that this PR removed — that's a CS1572 warning.

SubTreeTech was public class SubTreeTech(NodeTech[] nodes), so <param name="nodes"> on the type declaration was valid. Now that the primary constructor is gone and the parameters live on the explicit constructor below, the type has no parameter called nodes, and with <GenerateDocumentationFile>true</GenerateDocumentationFile> in OpenPolytopia.Common.csproj the compiler emits:

CS1572: XML comment has a param tag for 'nodes', but there is no parameter by that name

.editorconfig silences CS1591 but not CS1572, so this is a new warning on every build, and docs.yaml runs dotnet build before docfx.

The <exception> tag has the same problem in spirit — it documents the constructor's behaviour but is attached to the type, so docfx renders it on the class page while the constructor (the thing that actually throws) is documented with nothing. Both tags want to move down onto public SubTreeTech(NodeTech[] nodes), leaving <summary> and <remarks> on the type.

---

Generated by Claude Code

[Enn3Developer]

Expression-bodied constructor — .editorconfig asks for a block body, at warning severity.

csharp_style_expression_bodied_constructors = false:warning

That's IDE0021 ("Use block body for constructor"), and dotnet_analyzer_diagnostic.severity = warning is set too. These are the only two expression-bodied constructors in the solution — every other constructor in OpenPolytopia.Common uses a block body — so this is new noise rather than an existing pattern.

Same applies to TechTree's constructor at line 175, where it also costs readability: a ToDictionary over a collection expression over a Select inside a => is doing enough that a block body with a named local would read better.

---

Generated by Claude Code

[Enn3Developer]

Two of the four validations FromSerializedData claims are untested, and the headline "shared by every player" property has no test at all.

The PR description lists four rejections: branch missing, branch declared twice, branch without exactly MAX_NODES nodes, node id used more than once. Only two are covered here (TestMissingBranch, TestDuplicatedNode). The other two are the ones most likely to break, because the code paths are subtle:

• wrong node count — TestBranchNodesCount exercises SubTreeTech's constructor directly, not FromSerializedData's own check at the top of the loop. Those are separate ifs with separate messages; deleting the one in FromSerializedData leaves the whole suite green (the SubTreeTech throw would still fire, but only later, from TechTree's constructor, and with the "load-time" guarantee gone).
• branch declared twice — the TryAdd branch has no coverage. Note it's also order-dependent with the count check: data.Branches.Add(data.Branches[0]) is a one-liner that covers it.

Separately, the central claim of the design — one definition, independent per-player state — isn't asserted anywhere:

var a = _definition.CreateTechTree();
var b = _definition.CreateTechTree();
a[BranchType.Climbing].Research("climbing");
b[BranchType.Climbing].HasResearched("climbing").ShouldBeFalse();

It passes today (TechTree's constructor allocates fresh NodeTechs per tree), but it's exactly the thing that a future "cache the SubTreeTechs on the definition" optimisation would quietly break, and it'd break as a cross-player state leak rather than a crash. TestOverrideKeepsTheDefinition covers the definition-level half of this; this is the tree-level half.

---

Generated by Claude Code

[Enn3Developer]

Addressed in c24279d. Seven of the eight are fixed, the eighth is documented instead.

1 — tiers aren't checked. Fixed the way you suggested: SubTreeTech now takes the ids and stamps the tiers itself.

public SubTreeTech(IReadOnlyList<string> ids) {
  if (ids.Count != MAX_NODES) { throw ... }
  Nodes = [.. ids.Select((id, index) => new NodeTech { Id = id, Tier = TierOf(index) })];
}

NodeTech.Tier is { get; internal init; } now, so nothing outside the assembly can hand a node a tier that doesn't belong to its slot. TechTree's constructor is a new SubTreeTech(definition[branch]) and the duplicated Select is gone.

2 — the definition hands out its internal array. _branches is a Dictionary<BranchType, ReadOnlyCollection<string>>, wrapped once in the private constructor with Array.AsReadOnly, so FromSerializedData and Override keep working on string[] and the cast doesn't get you a writable array anymore.

3 — an extra branch passes every check. Enum.IsDefined(branch.Type) in the first loop, before the node count check, so a phantom branch can't poison the duplicate-id check either.

5 — an unknown id costs 0. ComputeCost returns uint? now, null for a node that isn't in the branch.

6 — coverage. Added TestBranchWithWrongNodesCount and TestDuplicatedBranch (the two validations that were only covered through SubTreeTech, or not at all), TestUndefinedBranch for the finding above, TestNodeTiers for the stamped tiers and TestIndependentTrees for the per-player state. 67 passed, 0 failed.

7 and 8. <param> and <exception> moved onto the constructor, both constructors have a block body now.

4 — the tribe isn't validated at load. Left as is, only documented: CreateTechTree now declares the ArgumentOutOfRangeException. Validating in RegisterTribes means handing the definition to TribeManager, and right now nothing owns both — the tech tree isn't wired to anything yet, so the load order is going to be decided by whatever wires it. Better done then than guessed now.

---

Generated by Claude Code

[Enn3Developer]

Adversarial review, round 2 — c24279d

Re-reviewed against the new head. Same caveat as last time: no .NET SDK in this environment, so nothing here was compiled or run — the claims below come from reading the code, .editorconfig, and the csproj.

What the fix commit resolved

Six of the eight findings from the first round are closed, and closed properly rather than papered over:

• tiers are derived, not passed — SubTreeTech(IReadOnlyList<string>) stamps TierOf(index) itself, NodeTech.Tier lost required and gained internal init, and TechTree's constructor lost the duplicated Select. The invariant now lives in the type instead of in its caller, which was the point.
• ComputeCost returns uint? — with a remark explaining why 0 was wrong, and TestComputeCost updated.
• Enum.IsDefined on deserialized branches — with a comment naming the converter behaviour that makes it necessary.
• ReadOnlyCollection<string> behind the definition indexer — the cast-and-mutate route is gone.
• CS1572 and both expression-bodied constructors — fixed.
• test coverage — TestUndefinedBranch, TestDuplicatedBranch, TestBranchWithWrongNodesCount, TestNodeTiers and TestIndependentTrees cover the gaps I listed, including the "independent per-player state" property.

The two I raised that weren't code-changed (Tribe.StartingBranch validation, exception documentation) got documentation instead — reasonable, and one of them is the subject of a follow-up below.

This round

Nothing blocking, and no regression from the fix commit. Five findings, roughly in the order I'd act on them:

1. SubTreeTech.Nodes hands out the backing array — the encapsulation fix landed on TechTreeDefinition but not here, and dropping required from Tier is what makes branch.Nodes[4] = new NodeTech { Id = "philosophy" } compile from a consumer assembly and produce the tier-0-in-slot-4 miscost the PR set out to prevent. Latent, not live: nothing writes to Nodes today.
2. Tribe.StartingBranch still has the hole FromSerializedData just closed — same enum, same converter, same resource shape, and the new comment on the tree-side guard documents the hazard explicitly. Follow-up on the still-open thread from round 1.
3. SubTreeTech's constructor doesn't enforce distinct ids — a repeated id makes a slot permanently unreachable and mispriced. FromSerializedData catches it tree-wide, so this is only reachable through the public constructor.
4. HasResearched kept the silent false — the argument written into ComputeCost's new remark applies to it unchanged. Consistency, not a bug.
5. JSON null escapes as NullReferenceException — required doesn't reject explicit nulls, so {"branches": null} bypasses the five documented rejections. Minor; the resource is author-controlled.

If you only take two, take 1 and 2 — they're the ones where the code now states an invariant it doesn't fully hold.

Two things I checked and am not reporting, to save you re-deriving them: the ComputeCost signature change has no callers outside TechTreeTest, and TestDuplicatedBranch's data.Branches.Add(data.Branches[0]) does reach the TryAdd throw rather than tripping an earlier check.

---

Generated by Claude Code

[Enn3Developer]

The encapsulation fix stopped one level short: this hands out the internal array, and it's the exact hole the new remark says is closed.

TechTreeDefinition now returns ReadOnlyCollection<string>, which is the right fix. But SubTreeTech — the type the new remark describes as building "its own nodes to make sure no node can ever get a tier that doesn't belong to its slot" — still exposes its backing array by reference:

var branch = definition.CreateTechTree()[BranchType.Climbing];
branch.Nodes[4] = new NodeTech { Id = "philosophy" };  // Tier is internal init → defaults to 0
branch.ComputeCost("philosophy", 4);                   // 8, should be 16

That compiles from any consumer assembly. Dropping required from Tier is what makes it compile: an object initializer that omits Tier used to be a compile error, now it silently yields a tier-0 node, and slot 4 accepts it. Same reference lets a caller reorder the slots, or write branch.Nodes[0] = null! and turn this[id]'s FirstOrDefault into an NRE.

Nothing in the repo writes to Nodes today — every use is a read — so this is a latent hole rather than a live bug. It's worth closing because the invariant just moved into this type on purpose, and this property is the one door left open:

private readonly NodeTech[] _nodes;

/// <summary>
/// The nodes of this branch, ordered by slot
/// </summary>
public IReadOnlyList<NodeTech> Nodes => _nodes;

TestNodeTiers would swap .Length for .Count; nothing else in the solution touches it.

Two smaller things that came in with the same change:

• the remark on NodeTech.Tier says "only SubTreeTech can set it", but internal means every type in OpenPolytopia.Common can. If you want the doc to be literally true, nesting NodeTech in SubTreeTech with a private init is the only way to get there; otherwise "only this assembly builds nodes" is the accurate phrasing.
• Researched is still a public setter, so branch.Nodes[0].Researched = true bypasses Research and its new bool return. Less serious — the result is the same state — but if Research becomes the audited path for spending stars, an unguarded setter next to it is a trap.

---

Generated by Claude Code

[Enn3Developer]

Follow-up: the undefined-enum hole was closed on the tree side and left open here, and the new code comment proves it's a known hazard.

FromSerializedData now carries:

// the json converter of BranchType accepts numbers too, so a branch that doesn't exist can get this far
if (!Enum.IsDefined(branch.Type)) { ... }

StartingBranch is the same BranchType, deserialized by the same converter, out of the same kind of embedded resource — but no Enum.IsDefined. "starting_branch": 42 in tribes.json deserializes, RegisterTribes accepts it, TribeManager[type] returns a Tribe that looks valid, and the failure lands in CreateTechTree as an ArgumentOutOfRangeException partway through building a player. The <exception> tag added to CreateTechTree documents that crash rather than preventing it, which is a fair call if it was deliberate — flagging it because the two paths are now inconsistent about an identical hazard, not because documenting was wrong.

The gap is one line wherever tribes get validated:

public void RegisterTribe(TribeType type, Tribe tribe) {
  if (!Enum.IsDefined(tribe.StartingBranch)) {
    throw new ArgumentException($"tribe {type} starts on {tribe.StartingBranch}, which isn't a valid branch", nameof(tribe));
  }

  Tribes.Add(type, tribe);
}

TechOverrides on the next property is the deferred-validation half of the same thing and genuinely does need the definition to check, so leaving that one to CreateTechTree is reasonable.

Worth noting separately: TestUndefinedBranch constructs the bad value in C# —

data.Branches.Add(new BranchSerializedData { Type = (BranchType)42, ... });

— so it covers the guard but never exercises the JSON path the comment above the guard cites as the reason it exists. If JsonStringEnumConverter were ever constructed with allowIntegerValues: false, or the converter attribute changed, that test would keep passing while the premise underneath it changed. Deserializing {"type": 42, "nodes": [...]} from a literal string asserts the actual claim.

---

Generated by Claude Code

[Enn3Developer]

The one invariant this constructor still doesn't enforce: ids have to be distinct, or a slot becomes unreachable.

The rewrite is the right shape — tiers can't be wrong anymore because they're derived, not passed. What's left is that this[id] is a linear search returning the first match, so a repeated id makes every later slot with that id dead:

var branch = new SubTreeTech(["climbing", "climbing", "meditation", "smithery", "philosophy"]);
branch.Research("climbing");          // marks slot 0
branch.Nodes[1].Researched;           // false, and no call can ever make it true
branch.ComputeCost("climbing", 2);    // 6 — slot 1 is tier 1, its real price is 8

Slot 1 exists, costs nothing to reach, and can never be researched or priced correctly. Empty and whitespace ids have the same effect, and ["a", "a", "a", "a", "a"] passes the count check while collapsing the entire branch to one node.

FromSerializedData's global duplicate check already rules this out for trees built from tech_tree.json, and Override can't introduce one either, so the shipped path is safe. But this constructor is public on a public type, and the commit that introduced it is titled "make the tech tree types enforce their own invariants" — this is the invariant that didn't come along. It's also the cheapest of the three to check:

if (ids.Distinct().Count() != ids.Count) {
  throw new ArgumentException("a branch can't have the same node id twice", nameof(ids));
}

If you'd rather not pay that on every branch construction, the alternative is to make the constructor internal and let FromSerializedData remain the only gate — that's a defensible answer too, and it makes TestBranchNodesCount the thing that has to move rather than the thing that has to grow a sibling. What's awkward is the current middle ground: public constructor, two of three invariants enforced.

---

Generated by Claude Code

[Enn3Developer]

HasResearched is the third member of the trio and kept the silent default that the other two just lost.

The reasoning added to ComputeCost is exactly right:

It returns null and not 0 because 0 is a price a caller can pay, so a node that doesn't exist would be free

false is a value a caller acts on for the same reason. HasResearched("free_divng") and HasResearched("free_diving") on a tribe that never took the override both answer false, and the caller can't tell "exists, not researched yet" from "no such node":

foreach (var id in uiNodeIds) {
  if (!branch.HasResearched(id)) { ShowAsAvailable(id); }  // a typo'd id renders as a researchable tech
}

Click it and ComputeCost returns null, so whatever you do with the nullable now has to handle a case the UI already committed to — and Research returns false after the star cost was already displayed. Three members, three different ways of saying "no such node": null, false, false.

bool? would make it consistent, though it's the least pleasant of the three signatures to consume. Given this[id] is already public and returns NodeTech?, an alternative that costs nothing is to let callers ask the question directly — branch[id] is { Researched: true } for the check, branch[id] is null for existence — and document HasResearched as the convenience overload that deliberately conflates the two. Either is fine; the thing worth avoiding is having fixed the sibling methods for a stated reason that applies here unchanged.

Not blocking, and unlike the other two there's no caller to break yet.

---

Generated by Claude Code

[Enn3Developer]

Minor: required doesn't reject an explicit JSON null, so malformed data leaves here as an NRE instead of the documented ArgumentException.

required guarantees the property was set, and a JSON null sets it. TechTreeSerializedData.Branches and BranchSerializedData.Nodes are non-nullable List<T>, but neither the source generator nor required enforces that at runtime:

• {"branches": null} → data.Branches.Count on line 51, NullReferenceException
• {"type": "climbing", "nodes": null} → branch.Nodes.Count on line 58, same
• {"nodes": ["a", null, "c", "d", "e"]} → count is 5, TryAdd succeeds, the duplicate check groups the single null into a group of one and passes, and a NodeTech ends up with Id = null despite required string Id. Two nulls anywhere in the tree do throw, with the message node is declared more than once.

The doc block right above this lists five ways the data can be rejected, and this is a sixth that escapes as a different exception type from a different layer. The file is embedded and under your control, so the realistic trigger is a hand-edit or a merge accident rather than untrusted input — hence minor. But FromSerializedData is the only gate in front of the resource, and a null check is one line each:

ArgumentNullException.ThrowIfNull(data.Branches);

plus the same for branch.Nodes, and branch.Nodes.Any(string.IsNullOrWhiteSpace) folded into the existing per-branch validation if you want the id case covered too.

---

Generated by Claude Code

[Enn3Developer]

Round 2 is in e6d5da6. All five findings are addressed — four in code, one in documentation.

1. SubTreeTech.Nodes handed out the backing array — fixed as suggested: a private NodeTech[] _nodes with public IReadOnlyList<NodeTech> Nodes => _nodes;, and this[id] now searches _nodes directly. TestNodeTiers swapped .Length for .Count. Both smaller points came along too: the Tier remark now says "only this assembly builds nodes and SubTreeTech is the only type doing it" instead of claiming something internal can't give, and Researched became internal set so Research stays the single path that can mark a node.

2. Tribe.StartingBranch — validated in RegisterTribe, essentially your snippet, with a comment tying it back to the tree-side guard and noting why TechOverrides can't be checked there. RegisterTribes documents the same exception. TestTribeWithUndefinedStartingBranch covers it, and TestUndefinedBranchFromJson now deserializes {"type": 42, ...} from a literal so the guard's premise gets asserted rather than assumed.

3. distinct ids in the constructor — took the check rather than making the constructor internal: a branch is cheap to build and the constructor being public is what makes it testable. TestBranchDuplicatedNodes uses your exact repro.

4. HasResearched — kept bool, documented instead. The remark now says the conflation is deliberate and points at this[id] for callers that have to tell "no such node" from "not researched yet". bool? reads badly at every call site of a method whose whole job is to be the short answer, and the indexer already gives the precise one for free.

5. JSON null — ArgumentNullException.ThrowIfNull on data.Branches and on each branch.Nodes, plus branch.Nodes.Any(string.IsNullOrWhiteSpace) folded into the per-branch validation so a null or blank id is rejected with the others instead of reaching NodeTech. The doc block lists the new exception. TestNullBranches and TestNullNode cover both.

Build is clean and the suite is at 72 passed / 0 failed / 0 skipped locally.

---

Generated by Claude Code

[Enn3Developer]

Third pass, against e6d5da6. Same caveat as the last two: no .NET SDK in this environment, so nothing here was compiled or run — everything below comes from reading the code, the resources and the merge.

All five round-2 findings are genuinely closed, and the two I called out as the picks got the stronger of the two available fixes: _nodes is private with IReadOnlyList<NodeTech> over it and this[id] searches the field directly, and RegisterTribe validates StartingBranch with a comment tying it to the tree-side guard. TestUndefinedBranchFromJson is the right instinct — it asserts the premise ({"type": 42} deserializing to (BranchType)42) instead of assuming it, which is what the previous test was missing.

Four of the five new findings are the same shape: an invariant was fixed at the gate where it was reported, and the other gates for the same invariant were left alone. That's now happened three rounds running — Enum.IsDefined on branches but not tribes, distinctness in SubTreeTech but not Override, and now null/blank ids in FromSerializedData but not the two other paths that write ids into the tree.

Ranked:

1. Override doesn't check the ids it writes — the one to take. "tech_overrides": [{"replaces": "fishing", "id": ""}] in tribes.json puts an empty id in slot 0 and the tribe starts researched on it; "id": null gets a NodeTech.Id of null through a required string and makes the node addressable by null. The comment on FromSerializedData explaining why required isn't enough applies word for word to TechOverride.Id.
2. The PR doesn't merge, and the natural resolution breaks tribes.json loading. #49 landed the same two fixes the opposite way — it renamed the JSON key to "tribe_type" where this PR renamed the property to Type — so the resolution has to be made in Tribe.cs and tribes.json together. And git already auto-merges starting_stars into the file twice with no conflict marker, which System.Text.Json accepts silently.
3. SubTreeTech's constructor has the same blank-id gap, by the same reasoning you used to justify putting the Distinct check there rather than sealing the constructor.
4. Researched's new remark repeats the exact overclaim this commit fixed on Tier — internal set is an assembly boundary, not a single-caller guarantee.
5. The branch.Nodes null guard has no test that pins it; TestNullNode passes with or without it, since it exercises the element path and ArgumentNullException satisfies Should.Throw<ArgumentException> anyway. Plus the hand-written paramName arguments, which ThrowIfNull already supplies.

None are blocking except the merge, which is blocking by definition. 1 and 3 are one decision — folding the check into SubTreeTech's constructor covers both paths at the cost of a vaguer message.

Two things I checked and am not filing: the ordering of the new validations in FromSerializedData is right (null before .Count, and putting the blank check ahead of the count check doesn't shadow TestBranchWithWrongNodesCount), and Override's duplicate guard holds up against the cases that looked promising — chained swaps, two overrides claiming the same id, and the Id == Replaces no-op all behave. CreateTechTree discarding Research's bool is safe today, since the tree and the id both come from the same definition; it's only worth a line if that stops being true.

---

Generated by Claude Code

[Enn3Developer]

The null/blank id hardening landed on one of the two paths that put ids into the tree.

FromSerializedData now rejects a null or blank node id, with the comment explaining exactly why required doesn't do it for you:

// required only checks that the json set the property, and a json null sets it just fine

TechOverride.Id is required string and reaches the tree through this method, and the same sentence applies to it verbatim — but nothing here checks it. Override only asks whether the id is already in the tree, and a null or blank id never is, so the guard on line 121 waves it straight through.

tech_overrides lives in tribes.json, so this is reachable from the same author-controlled resource the other guard protects:

"tech_overrides": [
  { "replaces": "fishing", "id": "" }
]

Find locates fishing at slot 0, "" != "fishing" and Find(branches, "") is null, so branches[Fishing][0] = "". The private constructor doesn't validate, SubTreeTech's count and Distinct checks both pass, and CreateTechTree then researches it — the tribe starts on a node whose id is the empty string. FromSerializedData would have rejected that exact tree shape with "branch fishing has a node without an id".

With "id": null it's worse: the array gets a null, NodeTech.Id ends up null despite being required string, and since Array.Find compares with ==, the node becomes addressable by null — HasResearched(null) and ComputeCost(null, n) both hit it. A second override with "replaces": null would then find that slot, because Array.IndexOf(nodes, null) stops being −1 once a null is in there.

A null element in the list itself ("tech_overrides": [null]) is the same family — it NREs on techOverride.Replaces.

The cheap fix is to validate the override at the top of the loop, next to where you already reason about ids:

foreach (var techOverride in overrides) {
  ArgumentNullException.ThrowIfNull(techOverride, nameof(overrides));
  if (string.IsNullOrWhiteSpace(techOverride.Id)) {
    throw new ArgumentException("an override can't have a node without an id", nameof(overrides));
  }
  ...

The <exception> block would want the same line the other one got.

---

Generated by Claude Code

[Enn3Developer]

Same gap as the Override comment, at the third gate: this constructor now enforces the count and the distinctness of the ids, but not that an id is actually an id.

new SubTreeTech(["climbing", "mining", "meditation", "smithery", ""])   // fine
new SubTreeTech(["climbing", "mining", "meditation", "smithery", null!]) // fine

Both build a branch whose slot 4 has an unusable id, and the null one produces a NodeTech with Id = null even though it's declared required string.

The reason you gave for taking the Distinct check here rather than making the constructor internal applies unchanged:

a branch is cheap to build and the constructor being public is what makes it testable

If the constructor is public, blank ids are its problem too — and folding the check in here would also cover the Override path for free, since every override eventually flows through this constructor. That might be the better single place for both:

if (ids.Any(string.IsNullOrWhiteSpace)) {
  throw new ArgumentException("a branch can't have a node without an id", nameof(ids));
}

The trade-off is the error message: caught here it says "a branch", caught in Override it can name the offending override. FromSerializedData would keep its own check either way since it wants to name the branch.

---

Generated by Claude Code

[Enn3Developer]

This commit fixed the overclaim on Tier and then wrote the identical one on Researched, four lines down.

Tier went from "only SubTreeTech can set it" to "only this assembly builds nodes and SubTreeTech is the only type doing it" — precise about what internal actually buys, which is the whole point. But Researched is internal set and the new remark says:

SubTreeTech.Research is the only way to mark a node, so it stays the single place where researching something can be paid for

internal doesn't say that. Any type in OpenPolytopia.Common can write branch.Nodes[0].Researched = true — and Nodes is public, so it doesn't even need to reach through SubTreeTech. What the accessor guarantees is that nothing outside the assembly can, which is a real narrowing and worth stating, just not this one. Since the stated goal is that researching is paid for exactly once, and the payment code doesn't exist yet, the wording will be load-bearing when it does.

Same shape as Tier's new remark would do it:

The tier and the researched state are set only inside this assembly, and <see cref="SubTreeTech.Research"/>
is the only place doing it, so researching something has a single place where it can be paid for

Worth checking there's no InternalsVisibleTo in play — there isn't one today, so the test project genuinely can't set it, which is what makes Research's bool return the only way the tests observe this.

---

Generated by Claude Code

[Enn3Developer]

Two small things about the two new ThrowIfNull calls.

The branch.Nodes guard on line 64 has no test. TestNullBranches covers line 55, and TestNullNode reads like it covers this one but doesn't — it nulls an element:

data.Branches[0].Nodes[1] = null!;

which is caught by the IsNullOrWhiteSpace check below, not by line 64. "nodes": null is a distinct shape and required List<string> accepts it for the same reason "branches": null gets through. It's also the one assertion in the pair that would notice if the guard were dropped, since TestNullNode passes with or without it — Should.Throw<ArgumentException> is satisfied by ArgumentNullException too, so neither of the two new tests actually pins line 64. A third one mirroring TestNullBranches closes it:

var data = JsonSerializer.Deserialize<TechTreeSerializedData>(
  """{"branches": [{"type": "climbing", "nodes": null}]}""", _techTreeOptions);

The paramName arguments are hand-written where they don't need to be. ThrowIfNull takes [CallerArgumentExpression("argument")] string? paramName = null, so line 55 gets "data.Branches" from the compiler already and passing it is a no-op that can drift from the expression next to it. Line 64 passes "data.Branches[].Nodes", which isn't an expression the compiler would ever produce and isn't a parameter name either — a caller reading ParamName off the exception now sees three different values from one method ("data" from every ArgumentException, plus those two). Dropping both arguments gives "data.Branches" and "branch.Nodes", which is what ParamName is for.

Minor either way — the guards themselves are right, and putting the null check before data.Branches.Count is the correct order.

---

Generated by Claude Code

[Enn3Developer]

This PR no longer merges, and the obvious resolution silently breaks tribe loading. mergeable_state is dirty: master moved to 3b3db3d (Terrain generation, #49), which touched EmbeddedResources.cs and resources/tribes.json — both of which this PR also rewrites.

The important part isn't the conflict, it's that #49 fixed the same two bugs this PR fixed, in the opposite direction. The PR description says:

TribeSerializedData.TribeType doesn't map to "type" under SnakeCaseLower. Both fixed (the property is now Type, like TroopSerializedData)

master kept public required TribeType TribeType and renamed the JSON key instead:

"tribe_type": "imperius",

So the two halves of the fix now live on opposite sides, in two different files, and only one of them conflicts. Resolving tribes.json in favour of master while this line stays Type leaves a required property with no matching key, and JsonSerializer throws on tribes.json at load — which is exactly the bug both PRs set out to fix, restored by the merge. Whichever convention wins, the pick has to be made in both files at once.

The second one is worse because git resolves it without a conflict marker. Each side moved starting_stars out of its wrong position by a different route — this PR pulled it up into tribe, #49 pushed it down — so the merged file has it twice:

"tribe": {
  "starting_branch": "organization",
  "starting_stars": 7,
  "spawn_rate": { ... },
  "terrain_rate": { ... },
  "starting_stars": 7          // <- from the other side, no conflict raised
}

Duplicate keys are legal JSON and System.Text.Json takes the last one silently, so nothing fails and nothing warns. It only shows up by reading the merged resource. (starting_tech does resolve cleanly to deleted, so that part is fine.)

Worth a look while you're in there: #49 added EmbeddedResources.LoadTroops()/LoadTribes() behind a shared _jsonOptions whose resolver is Combine(TribeGenerationContext.Default, TroopGenerationContext.Default). After the merge, TechTreeData is the one resource with no Load* counterpart and TechTreeGenerationContext is the one context not in that resolver — TestTechTree's hand-built options are currently the only thing that can read it. Not a conflict, just the seam the merge creates.

---

Generated by Claude Code

[Enn3Developer]

Round 3 answered in 1be9e5b (the merge) and 26bb8d8 (the fixes). All five taken, nothing pushed back on.

2 — the merge. Resolved in favour of "type"/Type, so tribes read like troops do (TroopSerializedData.Type with "type" in troops.json); the pick is made in Tribe.cs and tribes.json at once, and flipping it to tribe_type is a two line change if you'd rather keep #49's key. starting_stars is back to one occurrence, in the position #49 gave it. There was a third one git didn't flag at all: TerrainGenerationTest builds a Tribe with StartingTech, which this branch deleted, so the merge compiled to three errors until it moved to StartingBranch.

On the seam you spotted: TechTreeGenerationContext is in the shared resolver now and LoadTechTree() sits next to LoadTroops()/LoadTribes(), so the tests read the resource through the same door as everything else and only the hand written json still needs its own options.

1 and 3 — the ids. Taken as one decision, but at both gates rather than one. Override rejects a null override and an id that isn't an id before it writes anything, so it can still name the override in the message; SubTreeTech rejects the same thing, so a branch built by hand can't get a slot no one can research either. Every path that puts an id in the tree now checks it.

4 — Researched. Rewritten in the shape you gave Tier: the accessor narrows to the assembly, Research is what makes it the single place, and the two claims are stated apart. No InternalsVisibleTo in the project, so the tests still only see it through Research's bool.

5 — the guards. Both paramName arguments dropped, ParamName now reads data.Branches and branch.Nodes from the compiler. TestNullBranchNodes pins line 64 with "nodes": null and Should.Throw<ArgumentNullException>, which is the assertion that would notice if the guard went away.

90 passed / 0 failed / 0 skipped locally, cspell clean.

---

Generated by Claude Code

[Enn3Developer]

Round 4, against 26bb8d8. Still no .NET SDK in this environment, so nothing here was compiled or run — everything below comes from reading the code, the resources and the merge. CI is green on all six checks, so mergeable_state: blocked is a required review, not a failure.

All five round-3 findings are closed, and two of them better than asked. The merge went to "type"/Type with starting_stars back to a single occurrence in #49's position, TechTreeGenerationContext is in the shared resolver and every test that reads the resource now goes through LoadTechTree() — the hand-written options survive only where they're actually needed, for the json literals. And you found the TerrainGenerationTest/StartingTech breakage the merge produced silently, which was the failure mode I described but not one I'd found.

Four findings, three of them one shape. The recurring pattern from rounds 1–3 is still here, but it has narrowed: it used to be an invariant fixed at one gate and left at the others, and now it's null fixed for the contents and left for the containers. 26bb8d8 guarded overrides' elements and skipped the two sibling lists that get deserialized the same way.

1. TechTree.cs 55–66 — {"branches": [null]} is a NullReferenceException. Exactly the guard Override just got, in the loop Override's guard was modelled on. Sits between TestNullNode and TestNullBranchNodes in your own test list, with neither a check nor a test.
2. Tribe.cs 56–59 — same class twice: {"tribes": [null]} and "tribe": null both NRE out of RegisterTribes, and RegisterTribe(type, null!) is public.
3. TechTree.cs 115–124 — Override documents ArgumentNullException and throws it only for elements; Override, FromSerializedData, SubTreeTech and TechTree all dereference their argument unguarded. Plus Replaces gets no id check where Id now does, so a null one reports node isn't in the tree.
4. TechTreeTest.cs 280–289 — TestTribesResource pins Imperius by name rather than checking the file. This test is the one place that owns both a definition and every tribe, so it's where the deferred "overrides aren't validated at load" finding can actually be closed, and the loop is the same length as the pin.

None of these is blocking. 1 and 2 are the only ones that produce a wrong exception type at runtime, and both need a resource that's currently hand-written and correct.

Checked and deliberately not filed. Override's duplicate guard against chained swaps, two overrides claiming one id, and the Id == Replaces no-op — all still correct, and the chain test covers the interesting one. Whitespace inside an id (" fishing" passes IsNullOrWhiteSpace and is a distinct id) — a typo class, not a broken invariant. ComputeCost's unchecked uint arithmetic — not reachable with plausible city counts. Re-deriving the override definition per player instead of per tribe — allocation only, and a tribe has at most a handful of players. TechTree's ctor re-validating what the definition already guarantees — cheap, and it's what makes SubTreeTech usable on its own.

One thing I couldn't verify without an SDK, so treat it as a question rather than a finding: [EnumMember(Value = "climbing")] on BranchType isn't read by JsonStringEnumConverter<T> on net8.0 as far as I know — the attribute STJ honours for this arrived later, and PropertyNamingPolicy doesn't touch enum values. If that's right, "climbing" parses because the read path matches member names case-insensitively and every value here happens to be its member name lowercased, which also means writing a BranchType emits "Climbing", not "climbing". It costs nothing today and the convention comes from TroopType/TribeType on master, not from this PR. It stops costing nothing the first time a branch needs two words — [EnumMember(Value = "free_diving")] FreeDiving would fail to parse. A one-line round trip test through _jsonOptions settles it either way, and the tree being data now makes branches the thing most likely to grow.

Two things I want to credit because they're the kind that only show up when something changes: FromSerializedData requiring every BranchType means adding a member to the enum without adding it to tech_tree.json fails loudly at load rather than producing a tree with a hole, and the id guards landing at all three gates means this[id], Research, HasResearched and ComputeCost are now null-safe for free, since no stored id can match a null.

---

Generated by Claude Code

[Enn3Developer]

A null branch in the json still gets an NullReferenceException, and this is the loop the new Override guard was copied from.

26bb8d8 added ArgumentNullException.ThrowIfNull(techOverride, ...) to Override because a list deserialized from json can hold a null element. data.Branches is the same kind of list and didn't get the same line:

{ "branches": [null] }

ThrowIfNull(data.Branches) passes — the list is there — and then line 60 reads branch.Type off a null and the whole thing comes out as NullReferenceException instead of one of the ArgumentExceptions the doc block lists. required doesn't help here for the reason your own comment on line 54 gives, one level further out: nothing constructs a BranchSerializedData for a json null, so its required members are never looked at.

The gap is visible against your own tests. TestNullNode covers the null element of Nodes, TestNullBranchNodes covers the null list, and the null element of Branches — the case sitting between them — has neither a guard nor a test.

foreach (var branch in data.Branches) {
  // a json null is an element of the list like any other, the same as an override
  ArgumentNullException.ThrowIfNull(branch);

  if (!Enum.IsDefined(branch.Type)) {

Worth noting what already holds up: with every id now non-null and non-blank at all three gates, this[id], Research, HasResearched and ComputeCost are safe against a null id for free — node.Id == null can't match anything, so they answer "no such node" instead of finding one. That's the part of the round-3 hole that closed properly.

---

Generated by Claude Code

[Enn3Developer]

Same class of hole on the tribe side, twice over.

RegisterTribes walks a list that comes straight out of json and dereferences both the element and one of its fields without checking either:

{ "tribes": [null] }                                        // tribe.Type          -> NullReferenceException
{ "tribes": [{ "type": "imperius", "tribe": null }] }        // tribe.StartingBranch -> NullReferenceException

The second one is the more likely of the two to actually happen, because tribe is the field a hand edit is most likely to blank out while restructuring, and it's exactly the shape the starting_stars misplacement had before this PR fixed it.

RegisterTribe is public too, so RegisterTribe(TribeType.Imperius, null!) reaches line 40 the same way. Since it's already the single place where a tribe gets validated, it's also the natural place for the check:

public void RegisterTribe(TribeType type, Tribe tribe) {
  ArgumentNullException.ThrowIfNull(tribe);

  // same as the branches of the tech tree, ...

and one ArgumentNullException.ThrowIfNull(tribe) inside the foreach of RegisterTribes for the element.

While you're in here: Tribes.Add throws a bare ArgumentException on a tribe declared twice in the resource, with the framework's "An item with the same key has already been added." That's the same failure FromSerializedData reports as branch {x} is declared twice, and the doc block above only advertises the StartingBranch one. Pre-existing, but the tech tree side now sets the bar for what these messages look like.

---

Generated by Claude Code

[Enn3Developer]

Override now documents ArgumentNullException but only throws it for the elements, never for the argument.

Override(null!) doesn't reach line 121 at all: ToDictionary runs fine, then foreach asks a null for its enumerator and you get NullReferenceException. The doc comment added on line 115 reads as though the null case is handled, and for an element it is — for the collection it isn't.

It's the same on every public entry point of the new types, none of which guards the argument it immediately dereferences:

call	first dereference	what comes out
Override(null!)	foreach	NullReferenceException
FromSerializedData(null!)	data.Branches	NullReferenceException
new SubTreeTech(null!)	ids.Count	NullReferenceException
new TechTree(null!)	definition[branch]	NullReferenceException

Nullable enable means every one of these needs a null! at the call site, so none of them is going to happen by accident from C#. But that argument applies just as well to techOverride — which also can't be null without null! from C#, and got a guard anyway because the json path can produce one. The types now check everything they're handed inside the argument and nothing about it, which is a strange line to draw, and it's four lines to make it consistent.

Smaller thing in the same block: Id gets a real id check on line 123 and Replaces gets none, so { "replaces": null, "id": "x" } falls through to line 126 and reports node isn't in the tree — correct outcome, message with a hole in it. Folding Replaces into the line 123 check gets both named properly.

---

Generated by Claude Code

[Enn3Developer]

This test names the one tribe in the resource instead of checking the resource, which leaves the one deferred finding deferred for no reason.

The round-1 answer for "the tribe isn't validated at load" was that validating overrides needs the definition and nothing owns both yet, so the load order should be decided by whatever wires it. That's fair for TribeManager — but this test owns both, right here: _definition on one side, every tribe of tribes.json on the other. It just doesn't use them that way. Today it asserts Imperius by hand, and Imperius is the only tribe and has no overrides, so the assertion is about a tribe rather than about the file.

The consequence is the whole point of the feature. The first tribe that ships a tech_overrides block gets no check on it at all: a replaces that names a node nobody has (a typo, or a node another PR renamed) throws ArgumentException out of CreateTechTree, at whatever point a game decides to build that player's tree. Nothing before that says a word, and it's a resource-only edit so nothing in review has to touch C# either.

Looping is the same length as pinning:

[Test]
public void TestTribesResource() {
  var tribes = EmbeddedResources.LoadTribes();
  tribes.ShouldNotBeNull();
  var tribeManager = new TribeManager();
  tribeManager.RegisterTribes(tribes);

  // every tribe of the resource has to build a tree, so an override that names nothing fails here and not mid game
  foreach (var (type, tribe) in tribeManager.Tribes) {
    var techTree = _definition.CreateTechTree(tribe);
    techTree[tribe.StartingBranch].Nodes[0].Researched.ShouldBeTrue($"{type} doesn't start on anything");
  }
}

That keeps what the current test proves — the resource deserializes, registers and starts researched — and makes it hold for whatever gets added next, which is the part that has to survive without someone remembering this test exists. Keep the Imperius assertions too if you want the concrete values pinned; they're cheap and they document the file.

---

Generated by Claude Code

[Enn3Developer]

Round 4 answered in 53e2347. All four taken, and the enum question settled with an SDK — you were right on every detail of it.

1 and 2 — the null containers. {"branches": [null]} and both tribe shapes are ArgumentNullException now. Everything that walks a list out of the json checks the element: FromSerializedData on the branches, RegisterTribes on the tribes plus tribes.Tribes itself, and RegisterTribe on the tribe it's handed, which is what covers "tribe": null and the public RegisterTribe(type, null!) in one line. Tests for the three json shapes and for the direct call.

Took the Tribes.Add note too: registering a tribe twice says tribe {type} is registered more than once instead of the framework's sentence, so the two resources fail the same way, and the doc block says so.

3 — the arguments. Override, FromSerializedData, SubTreeTech and TechTree all guard what they immediately dereference, so the doc block on Override isn't advertising something it only half does. TestNullArguments is the four-line version of your table. Replaces gets its own check next to Id's, so a blank one reports the override with id x doesn't replace anything rather than a message with a hole in it.

4 — TestTribesResource. Loops the way you wrote it, Imperius assertions kept underneath. That's the deferred finding closed: the first tribe shipping a tech_overrides block gets its overrides resolved against the definition by the test suite, so a replaces naming nothing fails in CI instead of mid game.

The enum question — confirmed, all of it. Built your case on net8.0 with JsonStringEnumConverter<T> and [EnumMember]:

write Climbing:   {"type":"Climbing"}
write FreeDiving: {"type":"FreeDiving"}
read {"type":"climbing"}    -> Climbing
read {"type":"Climbing"}    -> Climbing
read {"type":"free_diving"} -> JsonException
read {"type":"FreeDiving"}  -> FreeDiving

So [EnumMember] is dead, reads work by case-insensitive member name, writes emit "Climbing", and free_diving throws — exactly as you described. TestBranchTypeInJson pins both halves with the reason in a comment, so the day it starts mattering the test says what to do.

I didn't fix it here because the fix isn't confined to this PR: it's registering the converter in the options rather than on the enum, and TroopType/TribeType/the troop skills all carry the same dead attribute. Verified the fix works through the source generated context, one line in EmbeddedResources:

Converters = { new JsonStringEnumConverter<BranchType>(JsonNamingPolicy.SnakeCaseLower) }

which gives write FreeDiving: {"type":"free_diving"}, keeps "climbing" reading, and still takes numbers — so the Enum.IsDefined guard stays needed either way. Say the word and it goes in, for the three enums together or for BranchType alone.

97 passed / 0 failed / 0 skipped locally, cspell clean.

---

Generated by Claude Code