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]

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]

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]

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]

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]

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

[Enn3Developer]

reviewed Claude's code: LGTM

[Enn3Developer]

@claude do a quick review of this pr and post findings as inline comments

[claude]

Claude finished @Enn3Developer's task in 2m 8s —— View job

---

Quick review — f710897

• Read trigger comment and gather PR context
• Review current diff against origin/master
• Post findings

Note on delivery: this environment only gives me the single PR-comment tool (mcp__github_comment__update_claude_comment), not a tool that posts individual inline review comments the way the earlier rounds in this thread did. So the findings below are inline in this comment, referenced by file:line, rather than posted as separate line comments.

What I checked

This PR already went through five rounds of adversarial review in this thread, closing 8 + 5 + 5 + 5 + 4 findings across null-handling, tier/id invariants, encapsulation, and the tribes.json/#49 merge. I re-read the current state of OpenPolytopia.Common/TechTree.cs, Tribe.cs, EmbeddedResources.cs, both resource files, and TechTreeTest.cs (372 lines, 34 tests) independently rather than trusting the changelog, and traced every consumer of the old StartingTech/TribeType-as-property API (TerrainGenerationTest.cs, TerrainGenerationNode.cs, Grid.cs, Player.cs, Lobby.cs, GameServer.cs) to confirm none were left on the removed shape.

No new findings. Specifically verified as sound:

• All three "puts an id/branch in the tree" gates (FromSerializedData, Override, SubTreeTech's constructor) now guard null, blank, and duplicate consistently — TechTree.cs:53-101, 123-151, 309-325.
• All three list-of-json entry points (data.Branches, overrides, tribes.Tribes) guard both the container and the element — TechTree.cs:57,63,124,129 and Tribe.cs:68,71,75.
• TechTreeDefinition[branch] hands out ReadOnlyCollection<string>, SubTreeTech.Nodes hands out an IReadOnlyList<NodeTech> backed by a private array — no mutation path into shared state.
• tribes.json/Tribe.cs/TribeSerializedData are consistent on the tribe_type/TribeType convention post-merge (Tribe.cs:227, tribes.json:4), and starting_stars appears exactly once.
• TestTribesResource (TechTreeTest.cs:354-369) now loops every registered tribe through CreateTechTree, so an override naming a non-existent node in the real resource fails in CI rather than mid-game — this was the one deferred finding from round 4 and it's closed.
• ComputeCost's formula, TierOf's slot mapping, and the uint?/bool "no such node" signalling are all self-consistent with their doc comments.

Still open (already flagged by this thread, not new, not blocking)

• [EnumMember] on BranchType/TribeType is dead code under JsonStringEnumConverter<T> on net8/net10 — confirmed in round 4, deliberately left as a documented, tested behavior (TestBranchTypeInJson) rather than fixed here, since the fix touches TroopType too and is out of scope.
• Tribe.TechOverrides is only validated when CreateTechTree runs, not at RegisterTribe time — documented as deferred until something owns both TribeManager and the tech tree definition.

Both are consistent with the reasoning already given in this thread and don't need re-litigating. I have no new blocking or non-blocking issues to add — this matches the author's own "LGTM" pass.