Raised during review of #77 and judged non-blocking by the final whole-branch review. Grouped because each is small; split if any is picked up on its own.
1. SerializeBlueprint mutates its argument
GridToBlueprintString.SerializeBlueprint sets the version and doubles directions in place on the Blueprint it is handed.
No current caller trips this: all three (GridToBlueprintString.Execute, PlanOrchestrator, NormalizeBlueprints) construct a fresh Blueprint immediately before calling, and CleanBlueprint.Execute allocates new Entity objects every time. The hazard is latent - a future caller wanting both an offset and a non-offset string from one blueprint would get directions doubled twice (4 -> 8 -> 16) with no error.
A doc comment warns about it. Taking a copy would be safer, at the cost of an allocation for a hazard with no caller.
2. ParseBlueprint dereferences Entities unguarded
The direction-conversion loop iterates root.Blueprint.Entities without a null check. Blueprint.Entities is declared null!, so a blueprint JSON with no entities key now throws a NullReferenceException at parse instead of failing later in InitializeContext.
Same failure class as before, so not a regression - but a three-line FactorioToolsException(badInput: true) guard would make the web and WASM paths return a clean bad-input error rather than an unhandled exception.
3. The Vue module dropdown has no oracle
ModuleSelect.vue hardcodes nine module names. Only the three in ItemNames.Vanilla are asserted against the captured Factorio fixture, so the next module rename breaks the dropdown exactly the way effectivity-module did.
The repo already has the pattern for fixing this: plannerDefaults.verified.json is generated from C# by a test. A generated module list would close the gap cheaply.
Raised during review of #77 and judged non-blocking by the final whole-branch review. Grouped because each is small; split if any is picked up on its own.
1.
SerializeBlueprintmutates its argumentGridToBlueprintString.SerializeBlueprintsets the version and doubles directions in place on theBlueprintit is handed.No current caller trips this: all three (
GridToBlueprintString.Execute,PlanOrchestrator,NormalizeBlueprints) construct a freshBlueprintimmediately before calling, andCleanBlueprint.Executeallocates newEntityobjects every time. The hazard is latent - a future caller wanting both an offset and a non-offset string from one blueprint would get directions doubled twice (4 -> 8 -> 16) with no error.A doc comment warns about it. Taking a copy would be safer, at the cost of an allocation for a hazard with no caller.
2.
ParseBlueprintdereferencesEntitiesunguardedThe direction-conversion loop iterates
root.Blueprint.Entitieswithout a null check.Blueprint.Entitiesis declarednull!, so a blueprint JSON with noentitieskey now throws aNullReferenceExceptionat parse instead of failing later inInitializeContext.Same failure class as before, so not a regression - but a three-line
FactorioToolsException(badInput: true)guard would make the web and WASM paths return a clean bad-input error rather than an unhandled exception.3. The Vue module dropdown has no oracle
ModuleSelect.vuehardcodes nine module names. Only the three inItemNames.Vanillaare asserted against the captured Factorio fixture, so the next module rename breaks the dropdown exactly the wayeffectivity-moduledid.The repo already has the pattern for fixing this:
plannerDefaults.verified.jsonis generated from C# by a test. A generated module list would close the gap cheaply.