From 851d0b9db094621f949f688106c2e77e143502e7 Mon Sep 17 00:00:00 2001 From: soloturn Date: Tue, 18 Aug 2026 21:00:24 +0200 Subject: [PATCH 1/6] fix(behavior): reject null children in behavior tree JSON at load time A malformed behavior tree (e.g. a stray/trailing comma producing a null array entry, or an explicit "child": null) parsed "successfully" with a silent null child, then crashed later with an unrelated NPE deep in tree execution/copying - SelectorNode#deepCopy or DefaultBehaviorTreeRunner#injectDelegates, depending on when the null child got touched. Root cause: Gson resolves JSON null straight to Java null without invoking BehaviorTreeBuilder's custom deserializer, so nothing ever validated the result before inserting it as a child. Now BehaviorTreeBuilder.getCompositeNode throws a JsonParseException naming the parent node and, for composite children, the index - turning a confusing deep-engine NPE into an immediate, actionable error at load time. Fixes #5099 --- .../core/BehaviorTreeBuilderTest.java | 50 +++++++++++++++++++ .../behavior/core/BehaviorTreeBuilder.java | 8 +++ 2 files changed, 58 insertions(+) create mode 100644 engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java new file mode 100644 index 00000000000..7e6f8e8b817 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java @@ -0,0 +1,50 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.core; + +import com.google.gson.JsonParseException; +import org.junit.jupiter.api.Test; +import org.terasology.engine.logic.behavior.actions.InvertAction; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression coverage for https://github.com/MovingBlocks/Terasology/issues/5099: a malformed behavior tree + * (e.g. a stray/trailing comma producing a null array entry) used to parse "successfully" with a silent + * {@code null} child, only to crash later with an unrelated NPE deep in tree execution/copying + * ({@link org.terasology.engine.logic.behavior.core.SelectorNode#deepCopy()} or + * {@link org.terasology.engine.logic.behavior.DefaultBehaviorTreeRunner}). It should instead fail fast, at + * load time, with a message that points at the malformed tree. + */ +public class BehaviorTreeBuilderTest { + + private final BehaviorTreeBuilder builder = new BehaviorTreeBuilder(); + + @Test + public void validTreeStillParses() { + BehaviorNode node = builder.fromJson("{ selector: [success, failure, success] }"); + + assertEquals(3, node.getChildrenCount()); + } + + @Test + public void nullChildInCompositeArrayFailsLoudlyInsteadOfLater() { + JsonParseException exception = assertThrows(JsonParseException.class, + () -> builder.fromJson("{ selector: [success, null, success] }")); + + assertTrue(exception.getMessage().contains("selector")); + assertTrue(exception.getMessage().contains("index 1")); + } + + @Test + public void nullChildInDecoratorFailsLoudlyInsteadOfLater() { + builder.registerDecorator("invert", InvertAction.class); + + JsonParseException exception = assertThrows(JsonParseException.class, + () -> builder.fromJson("{ invert: { child: null } }")); + + assertTrue(exception.getMessage().contains("invert")); + } +} diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java index 6a39fa87ce3..43c2e489246 100644 --- a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilder.java @@ -231,12 +231,20 @@ private BehaviorNode getCompositeNode(JsonElement json, JsonDeserializationConte addAction((ActionNode) node, action); JsonElement childJson = jsonElement.getAsJsonObject().get("child"); BehaviorNode child = context.deserialize(childJson, BehaviorNode.class); + if (child == null) { + throw new JsonParseException("Malformed behavior tree: decorator '" + type + + "' has no valid child (check for a missing/null \"child\" entry)"); + } node.insertChild(0, child); } else if (jsonElement.isJsonArray()) { List children = context.deserialize(jsonElement, new TypeToken>() { }.getType()); for (int i = 0; i < children.size(); i++) { BehaviorNode child = children.get(i); + if (child == null) { + throw new JsonParseException("Malformed behavior tree: '" + type + + "' has a null child at index " + i + " (check for a stray/trailing comma)"); + } node.insertChild(i, child); } } From 5682fe65acf4c2e5cecaa84b9b15195ae0af72ff Mon Sep 17 00:00:00 2001 From: soloturn Date: Tue, 18 Aug 2026 21:10:14 +0200 Subject: [PATCH 2/6] fix(behavior): isolate a malformed tree's load failure to that asset The previous commit turned the silent-null-child bug into a clear JsonParseException, but that's unchecked - gestalt's asset loader (AssetType.reload, via a PrivilegedExceptionAction) only isolates a single asset's load failure to a log line and an empty Optional for *checked* exceptions. An unchecked exception sails straight past that net and aborts whatever triggered the load - in practice, the whole game, since BehaviorSystem.initialise() eagerly loads every .behavior asset from every module at startup. BehaviorTreeFormat.load(ResourceUrn, List) already declares "throws IOException" for exactly this purpose. Catch the JsonParseException there and rethrow as IOException (urn + cause preserved) so a malformed tree now just fails to load - logged by gestalt as "Failed to load asset ''", already-handled by BehaviorSystem's existing isPresent() check - instead of crashing the game. --- .../asset/BehaviorTreeFormatTest.java | 58 +++++++++++++++++++ .../behavior/asset/BehaviorTreeFormat.java | 7 +++ 2 files changed, 65 insertions(+) create mode 100644 engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java new file mode 100644 index 00000000000..893a3ea04e2 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormatTest.java @@ -0,0 +1,58 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.asset; + +import org.junit.jupiter.api.Test; +import org.terasology.gestalt.assets.ResourceUrn; +import org.terasology.gestalt.assets.format.AssetDataFile; +import org.terasology.gestalt.module.resources.FileReference; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression coverage for https://github.com/MovingBlocks/Terasology/issues/5099. A malformed behavior tree must + * fail this single asset's load with the checked {@link IOException} the {@code AssetFileFormat} contract expects + * - that's what lets gestalt's asset-loading machinery isolate the failure to just this asset (log it, move on) + * instead of an unchecked exception blowing past that safety net and crashing whatever triggered the load. + */ +public class BehaviorTreeFormatTest { + + @Test + public void malformedTreeFailsWithCheckedIOExceptionNotAnUncheckedOne() { + BehaviorTreeFormat format = new BehaviorTreeFormat(); + ResourceUrn urn = new ResourceUrn("engine:malformed"); + List source = Collections.singletonList(new AssetDataFile(jsonFile("{ selector: [success, null, success] }"))); + + IOException exception = assertThrows(IOException.class, () -> format.load(urn, source)); + + assertTrue(exception.getMessage().contains("engine:malformed")); + assertTrue(exception.getMessage().contains("selector")); + } + + private FileReference jsonFile(String json) { + return new FileReference() { + @Override + public String getName() { + return "malformed.behavior"; + } + + @Override + public List getPath() { + return Collections.emptyList(); + } + + @Override + public InputStream open() { + return new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8)); + } + }; + } +} diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java b/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java index e86669e61a7..926686d01cf 100644 --- a/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/asset/BehaviorTreeFormat.java @@ -3,6 +3,7 @@ package org.terasology.engine.logic.behavior.asset; import com.google.common.base.Charsets; +import com.google.gson.JsonParseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.terasology.gestalt.assets.ResourceUrn; @@ -53,6 +54,12 @@ public BehaviorTreeData load(ResourceUrn resourceUrn, List list) } try (InputStream stream = list.get(0).openStream()) { return load(stream); + } catch (JsonParseException e) { + // Gestalt only isolates a single asset's load failure (logs it, returns Optional.empty()) for + // *checked* exceptions - an unchecked JsonParseException would instead propagate all the way out + // and abort whatever triggered the load (e.g. crash the whole game on startup, see #5099). + // Rethrowing as the IOException this method already declares routes it through that safety net. + throw new IOException("Malformed behavior tree asset '" + resourceUrn + "': " + e.getMessage(), e); } } From 2323f1d6a7508ba9ed37a91d044e4d1aeb2d516a Mon Sep 17 00:00:00 2001 From: soloturn Date: Tue, 18 Aug 2026 21:15:19 +0200 Subject: [PATCH 3/6] test(behavior): cover a decorator with a missing child entry CodeRabbit review on #5366: nullChildInDecoratorFailsLoudlyInsteadOfLater only covered an explicit "child": null, not an entirely missing key. Both already fail the same way (context.deserialize(null, ...) resolves to Java null just like a JsonNull element does) - this just adds the coverage, no production code change. --- .../logic/behavior/core/BehaviorTreeBuilderTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java index 7e6f8e8b817..a3fe5026e45 100644 --- a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeBuilderTest.java @@ -47,4 +47,14 @@ public void nullChildInDecoratorFailsLoudlyInsteadOfLater() { assertTrue(exception.getMessage().contains("invert")); } + + @Test + public void missingChildInDecoratorFailsLoudlyInsteadOfLater() { + builder.registerDecorator("invert", InvertAction.class); + + JsonParseException exception = assertThrows(JsonParseException.class, + () -> builder.fromJson("{ invert: {} }")); + + assertTrue(exception.getMessage().contains("invert")); + } } From bfa8239c0e39331d3d814ddc9290af3e19fd2b0e Mon Sep 17 00:00:00 2001 From: soloturn Date: Tue, 18 Aug 2026 21:32:22 +0200 Subject: [PATCH 4/6] feat(behavior): add a repair tool for the trailing-comma bug Follow-up to the load-time rejection added earlier in this PR: once a malformed tree fails to load with a clear message, a content author still has to go find and remove the offending comma by hand. This does that mechanically instead. BehaviorTreeRepairTool walks the raw JSON (via Gson's own parser, not regex on text, so it can't misfire inside a quoted string) and strips phantom null entries from composite child arrays - the array slot a stray/trailing comma produces never represented a real child (nothing sits between the comma and the closing bracket), so removing it loses nothing. An explicit "child": null on a decorator is a different, *not* safely fixable case - a real child was supposed to be there and there's no way to guess what it should have been - so those are reported, not touched, and the file is left untouched if that's the only issue found. repair(Path) backs up the original alongside as .bak before writing. Includes a main() CLI entry point, since a file broken this way stops the game from starting - fixing it can't be an in-game action. --- .../core/BehaviorTreeRepairToolTest.java | 95 ++++++++++ .../behavior/core/BehaviorTreeRepairTool.java | 162 ++++++++++++++++++ 2 files changed, 257 insertions(+) create mode 100644 engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java create mode 100644 engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java new file mode 100644 index 00000000000..6c09eabdb46 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java @@ -0,0 +1,95 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.core; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * See https://github.com/MovingBlocks/Terasology/issues/5099. The tool exists so a content author who hits the + * load-time rejection from {@link BehaviorTreeBuilder} doesn't have to hand-edit the file's JSON to find and + * remove the offending comma - it does the same fix mechanically. + */ +public class BehaviorTreeRepairToolTest { + + @Test + public void trailingCommaInCompositeArrayIsRemoved() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"selector\": [\"success\", \"success\",] }"); + + assertTrue(result.changed); + assertEquals(1, result.nullArrayEntriesRemoved); + assertTrue(result.unfixableIssues.isEmpty()); + // The cleaned JSON should load through the real builder with no custom actions/decorators needed. + BehaviorNode node = new BehaviorTreeBuilder().fromJson(result.cleanedJson); + assertEquals(2, node.getChildrenCount()); + } + + @Test + public void explicitNullInCompositeArrayIsRemoved() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"selector\": [\"success\", null, \"success\"] }"); + + assertTrue(result.changed); + assertEquals(1, result.nullArrayEntriesRemoved); + BehaviorNode node = new BehaviorTreeBuilder().fromJson(result.cleanedJson); + assertEquals(2, node.getChildrenCount()); + } + + @Test + public void nothingToFixIsReportedAsUnchanged() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"selector\": [\"success\", \"success\"] }"); + + assertFalse(result.changed); + assertEquals(0, result.nullArrayEntriesRemoved); + assertTrue(result.unfixableIssues.isEmpty()); + } + + @Test + public void explicitNullDecoratorChildIsReportedNotSilentlyDropped() { + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.clean( + "{ \"invert\": { \"child\": null } }"); + + assertFalse(result.unfixableIssues.isEmpty()); + } + + @Test + public void repairWritesTheFileAndKeepsABackup(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + String original = "{ \"selector\": [\"success\", \"success\",] }"; + Files.write(file, original.getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.repair(file); + + assertTrue(result.changed); + Path backup = dir.resolve("broken.behavior.bak"); + assertTrue(Files.exists(backup)); + assertEquals(original, new String(Files.readAllBytes(backup), StandardCharsets.UTF_8)); + BehaviorNode node = new BehaviorTreeBuilder().fromJson( + new String(Files.readAllBytes(file), StandardCharsets.UTF_8)); + assertEquals(2, node.getChildrenCount()); + } + + @Test + public void repairLeavesUnfixableFileUntouched(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + String original = "{ \"invert\": { \"child\": null } }"; + Files.write(file, original.getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.Result result = BehaviorTreeRepairTool.repair(file); + + assertFalse(result.changed); + assertFalse(Files.exists(dir.resolve("broken.behavior.bak"))); + assertEquals(original, new String(Files.readAllBytes(file), StandardCharsets.UTF_8)); + } +} diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java new file mode 100644 index 00000000000..7ef37122d91 --- /dev/null +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java @@ -0,0 +1,162 @@ +// Copyright 2021 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.behavior.core; + +import com.google.gson.GsonBuilder; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Repairs a class of malformed {@code .behavior} JSON files: a stray/trailing comma in a composite node's child + * array (e.g. {@code { selector: [success, success,] } }) parses as an extra, phantom {@code null} array entry - + * see {@link BehaviorTreeBuilder#getCompositeNode} and https://github.com/MovingBlocks/Terasology/issues/5099. + * That slot never represented a real child (there's nothing between the comma and the closing bracket), so + * removing it loses nothing and is safe to do automatically. + *

+ * A decorator with an explicit {@code "child": null} is a different, not safely auto-fixable case: a + * real child was supposed to be there and isn't, and there's no way to guess what it should have been. Those are + * reported, not touched. A decorator missing the {@code child} key entirely can't be told apart from a legitimate + * childless action without knowing which JSON keys are registered as decorators (that needs a live module + * environment, which this offline tool doesn't have) - it will slip through undetected here and still needs + * {@link BehaviorTreeBuilder}'s own load-time check to be caught. + */ +public final class BehaviorTreeRepairTool { + private static final Logger logger = LoggerFactory.getLogger(BehaviorTreeRepairTool.class); + + private BehaviorTreeRepairTool() { + } + + public static final class Result { + public final boolean changed; + public final int nullArrayEntriesRemoved; + public final List unfixableIssues; + /** The cleaned JSON, or {@code null} if {@link #changed} is false. */ + public final String cleanedJson; + + private Result(boolean changed, int nullArrayEntriesRemoved, List unfixableIssues, String cleanedJson) { + this.changed = changed; + this.nullArrayEntriesRemoved = nullArrayEntriesRemoved; + this.unfixableIssues = unfixableIssues; + this.cleanedJson = cleanedJson; + } + } + + /** + * Cleans the given behavior tree JSON: strips phantom null entries from composite child arrays, and reports + * (without touching) any null/missing decorator child it finds along the way. Does not touch any file. + * + * @param json the raw file contents + * @return the result of the scan - {@link Result#changed} is false if there was nothing to remove + */ + public static Result clean(String json) { + JsonElement root = JsonParser.parseString(json); + List unfixable = new ArrayList<>(); + int[] removed = {0}; + JsonElement cleaned = stripPhantomNulls(root, unfixable, removed); + if (removed[0] == 0) { + return new Result(false, 0, unfixable, null); + } + String cleanedJson = new GsonBuilder().setPrettyPrinting().create().toJson(cleaned); + return new Result(true, removed[0], unfixable, cleanedJson); + } + + /** + * Repairs a {@code .behavior} file on disk in place, if it has any safely-fixable phantom null array entries. + * The original is preserved alongside as {@code .bak}. + *

+ * If the file also has an unfixable issue (a decorator's null/missing child - see the class doc), nothing is + * written: fixing the array entries alone would still leave a broken file, and this tool can't guess what the + * decorator's missing child was supposed to be. Deliberately does not validate the result by loading it + * through {@link BehaviorTreeBuilder}: that requires a live module environment to resolve custom + * action/decorator names, which an offline repair tool doesn't have. + * + * @param file the {@code .behavior} file to repair + * @return the result of the repair attempt + * @throws IOException if the file can't be read, backed up or written + */ + public static Result repair(Path file) throws IOException { + String original = new String(Files.readAllBytes(file), StandardCharsets.UTF_8); + Result result = clean(original); + if (!result.changed) { + return result; + } + if (!result.unfixableIssues.isEmpty()) { + logger.warn("Not repairing '{}': found unfixable issues too - {}", file, result.unfixableIssues); + return new Result(false, result.nullArrayEntriesRemoved, result.unfixableIssues, null); + } + Path backup = file.resolveSibling(file.getFileName() + ".bak"); + Files.write(backup, original.getBytes(StandardCharsets.UTF_8)); + Files.write(file, result.cleanedJson.getBytes(StandardCharsets.UTF_8)); + logger.info("Repaired '{}': removed {} phantom null entr{} (backup at '{}')", + file, result.nullArrayEntriesRemoved, result.nullArrayEntriesRemoved == 1 ? "y" : "ies", backup); + return result; + } + + /** + * Command-line entry point, since a file broken this way stops the game from even starting - + * {@code gradle :engine:run... } isn't an option, this needs to run standalone against the raw file(s). + * + * @param args one or more paths to {@code .behavior} files + */ + public static void main(String[] args) throws IOException { + if (args.length == 0) { + System.err.println("Usage: BehaviorTreeRepairTool [more files...]"); + System.exit(1); + } + for (String arg : args) { + Path file = Path.of(arg); + Result result = repair(file); + if (result.changed) { + System.out.println(file + ": removed " + result.nullArrayEntriesRemoved + " phantom null entr" + + (result.nullArrayEntriesRemoved == 1 ? "y" : "ies") + ", backup saved alongside it."); + } else if (!result.unfixableIssues.isEmpty()) { + System.out.println(file + ": NOT repaired, needs manual fixing - " + result.unfixableIssues); + } else { + System.out.println(file + ": nothing to repair."); + } + } + } + + private static JsonElement stripPhantomNulls(JsonElement element, List unfixable, int[] removedCount) { + if (element.isJsonArray()) { + JsonArray source = element.getAsJsonArray(); + JsonArray cleaned = new JsonArray(); + for (JsonElement child : source) { + if (child.isJsonNull()) { + removedCount[0]++; + } else { + cleaned.add(stripPhantomNulls(child, unfixable, removedCount)); + } + } + return cleaned; + } else if (element.isJsonObject()) { + JsonObject source = element.getAsJsonObject(); + JsonObject cleaned = new JsonObject(); + for (Map.Entry entry : source.entrySet()) { + JsonElement value = entry.getValue(); + if ("child".equals(entry.getKey()) && value.isJsonNull()) { + // A decorator missing its one required child - there's nothing to safely infer here, unlike + // an array's phantom trailing-comma slot. Leave it as-is and flag it instead. + unfixable.add("decorator has a null/missing 'child' - needs a real child added by hand"); + cleaned.add(entry.getKey(), value); + } else { + cleaned.add(entry.getKey(), stripPhantomNulls(value, unfixable, removedCount)); + } + } + return cleaned; + } + return element; + } +} From 78ae9ff9f4d812c778c193fdda26f48bcd1498c2 Mon Sep 17 00:00:00 2001 From: soloturn Date: Tue, 18 Aug 2026 21:40:53 +0200 Subject: [PATCH 5/6] fix(behavior): make the repair tool crash-safe and backup-safe CodeRabbit review: repair() overwrote an existing .bak unconditionally and wrote the repaired JSON straight to the source file - a failure partway through either write could destroy a prior backup or leave the source file truncated. - Back up to the first free .bak / .bak.1 / .bak.2 / ... instead of always .bak, so a repeat run (or someone's own .bak already sitting there) is never silently clobbered. - Write the repaired JSON to a temp file in the same directory first, then move it over the source atomically (ATOMIC_MOVE) - a crash mid-write leaves either the old file or the new one, never a partial one. Adds tests for both: an existing backup survives untouched and a versioned one is created instead, and a successful repair leaves exactly the source file plus one backup behind (no stray temp file). --- .../core/BehaviorTreeRepairToolTest.java | 35 +++++++++++++++++++ .../behavior/core/BehaviorTreeRepairTool.java | 31 +++++++++++++--- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java index 6c09eabdb46..a6f7a5a4c19 100644 --- a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairToolTest.java @@ -9,6 +9,10 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -92,4 +96,35 @@ public void repairLeavesUnfixableFileUntouched(@TempDir Path dir) throws IOExcep assertFalse(Files.exists(dir.resolve("broken.behavior.bak"))); assertEquals(original, new String(Files.readAllBytes(file), StandardCharsets.UTF_8)); } + + @Test + public void repairNeverClobbersAnExistingBackup(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + String original = "{ \"selector\": [\"success\", \"success\",] }"; + Files.write(file, original.getBytes(StandardCharsets.UTF_8)); + Path existingBackup = dir.resolve("broken.behavior.bak"); + String someoneElsesBackup = "not the original - already there before repair() ran"; + Files.write(existingBackup, someoneElsesBackup.getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.repair(file); + + assertEquals(someoneElsesBackup, new String(Files.readAllBytes(existingBackup), StandardCharsets.UTF_8)); + Path versionedBackup = dir.resolve("broken.behavior.bak.1"); + assertTrue(Files.exists(versionedBackup)); + assertEquals(original, new String(Files.readAllBytes(versionedBackup), StandardCharsets.UTF_8)); + } + + @Test + public void repairLeavesOnlyTheFinalFileBehindNoStrayTempFiles(@TempDir Path dir) throws IOException { + Path file = dir.resolve("broken.behavior"); + Files.write(file, "{ \"selector\": [\"success\", \"success\",] }".getBytes(StandardCharsets.UTF_8)); + + BehaviorTreeRepairTool.repair(file); + + try (Stream entries = Files.list(dir)) { + List names = entries.map(p -> p.getFileName().toString()) + .sorted().collect(Collectors.toList()); + assertEquals(Arrays.asList("broken.behavior", "broken.behavior.bak"), names); + } + } } diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java index 7ef37122d91..e54f444db64 100644 --- a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java @@ -14,6 +14,8 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -74,7 +76,10 @@ public static Result clean(String json) { /** * Repairs a {@code .behavior} file on disk in place, if it has any safely-fixable phantom null array entries. - * The original is preserved alongside as {@code .bak}. + * The original is preserved alongside as {@code .bak} - or {@code .bak.1}, {@code .bak.2}, etc. if + * an earlier repair already left a {@code .bak} there, so a repeat run never clobbers a previous backup. + * The repaired content is written to a temporary file first and moved into place atomically, so a failure + * partway through can't leave the source file truncated or corrupt. *

* If the file also has an unfixable issue (a decorator's null/missing child - see the class doc), nothing is * written: fixing the array entries alone would still leave a broken file, and this tool can't guess what the @@ -96,14 +101,32 @@ public static Result repair(Path file) throws IOException { logger.warn("Not repairing '{}': found unfixable issues too - {}", file, result.unfixableIssues); return new Result(false, result.nullArrayEntriesRemoved, result.unfixableIssues, null); } - Path backup = file.resolveSibling(file.getFileName() + ".bak"); - Files.write(backup, original.getBytes(StandardCharsets.UTF_8)); - Files.write(file, result.cleanedJson.getBytes(StandardCharsets.UTF_8)); + Path backup = nextFreeBackupPath(file); + Files.write(backup, original.getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE_NEW); + + Path temporary = Files.createTempFile(file.toAbsolutePath().getParent(), file.getFileName().toString(), ".tmp"); + try { + Files.write(temporary, result.cleanedJson.getBytes(StandardCharsets.UTF_8)); + Files.move(temporary, file, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); + } finally { + Files.deleteIfExists(temporary); + } + logger.info("Repaired '{}': removed {} phantom null entr{} (backup at '{}')", file, result.nullArrayEntriesRemoved, result.nullArrayEntriesRemoved == 1 ? "y" : "ies", backup); return result; } + private static Path nextFreeBackupPath(Path file) { + Path candidate = file.resolveSibling(file.getFileName() + ".bak"); + int suffix = 1; + while (Files.exists(candidate)) { + candidate = file.resolveSibling(file.getFileName() + ".bak." + suffix); + suffix++; + } + return candidate; + } + /** * Command-line entry point, since a file broken this way stops the game from even starting - * {@code gradle :engine:run... } isn't an option, this needs to run standalone against the raw file(s). From 002e2cfe7caab864cfd6cbf7ef8b1dba53ab1ec3 Mon Sep 17 00:00:00 2001 From: soloturn Date: Fri, 21 Aug 2026 11:54:46 +0200 Subject: [PATCH 6/6] fix(checkstyle): move BehaviorTreeRepairTool.Result to the end of the class InnerTypeLast: Result was declared right after the constructor, ahead of the static methods that use it. 5 info-severity findings, one per method after it - checkstyle wants inner types after fields/methods. No functional change. Co-Authored-By: Claude Sonnet 5 --- .../behavior/core/BehaviorTreeRepairTool.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java index e54f444db64..bd0de4aa220 100644 --- a/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java +++ b/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorTreeRepairTool.java @@ -40,21 +40,6 @@ public final class BehaviorTreeRepairTool { private BehaviorTreeRepairTool() { } - public static final class Result { - public final boolean changed; - public final int nullArrayEntriesRemoved; - public final List unfixableIssues; - /** The cleaned JSON, or {@code null} if {@link #changed} is false. */ - public final String cleanedJson; - - private Result(boolean changed, int nullArrayEntriesRemoved, List unfixableIssues, String cleanedJson) { - this.changed = changed; - this.nullArrayEntriesRemoved = nullArrayEntriesRemoved; - this.unfixableIssues = unfixableIssues; - this.cleanedJson = cleanedJson; - } - } - /** * Cleans the given behavior tree JSON: strips phantom null entries from composite child arrays, and reports * (without touching) any null/missing decorator child it finds along the way. Does not touch any file. @@ -182,4 +167,19 @@ private static JsonElement stripPhantomNulls(JsonElement element, List u } return element; } + + public static final class Result { + public final boolean changed; + public final int nullArrayEntriesRemoved; + public final List unfixableIssues; + /** The cleaned JSON, or {@code null} if {@link #changed} is false. */ + public final String cleanedJson; + + private Result(boolean changed, int nullArrayEntriesRemoved, List unfixableIssues, String cleanedJson) { + this.changed = changed; + this.nullArrayEntriesRemoved = nullArrayEntriesRemoved; + this.unfixableIssues = unfixableIssues; + this.cleanedJson = cleanedJson; + } + } }