From e40316a12a973243fedcf315dbe4ff63210bbcb4 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 12:19:51 +0200 Subject: [PATCH 1/7] fix(reporter): pre-fill the GitHub issue form with a crash summary 'Report Issue' just opened REPORT_ISSUE_LINK as-is - a bare https://github.com/.../issues/new, always blank, discarding everything the dialog already knows about the crash. Adds CrashSummary, which builds a title (exception class + message) and a Markdown body from three sources: - the exception itself (available directly, no parsing needed) - class, message, and a capped stack trace extract - the engine version and active module list, which only exist in the crashed process' own log output (the reporter runs in its own JVM per #52's subprocess isolation, so it has no other way to reach them) - extracted via regex against two fixed log lines TerasologyEngine already emits at startup (TerasologyVersion's bracketed key=value dump, and one "Activating module: id:version" line per active module) - the OS, read directly via System.getProperty (same machine, same session as the crash - no parsing needed) - the uploaded PasteBin link, when the user uploaded one GitHubIssueLinkBuilder turns that into a real pre-filled URL via GitHub's own title=/body= query parameters. Both are plain, dependency- free classes - no Swing - so they're covered directly by CrashSummaryTest/GitHubIssueLinkBuilderTest without needing a headless UI harness. Second fix from #53 (item 3 of 5); log tab ordering, the dead forum link, and the Discord invite are still open follow-ups. Co-Authored-By: Claude Sonnet 5 --- .../terasology/crashreporter/RootPanel.java | 8 +- .../crashreporter/pages/CrashSummary.java | 143 ++++++++++++++++++ .../pages/FinalActionsPanel.java | 14 +- .../pages/GitHubIssueLinkBuilder.java | 31 ++++ .../crashreporter/pages/CrashSummaryTest.java | 94 ++++++++++++ .../pages/GitHubIssueLinkBuilderTest.java | 37 +++++ 6 files changed, 324 insertions(+), 3 deletions(-) create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java create mode 100644 cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java diff --git a/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java index 21de16e..19a9904 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/RootPanel.java @@ -74,7 +74,13 @@ public String get() { } }); pages.add(uploadPanel); - pages.add(new FinalActionsPanel(properties, new Supplier() { + pages.add(new FinalActionsPanel(properties, exception, new Supplier() { + + @Override + public String get() { + return errorMessagePanel.getLog(); + } + }, new Supplier() { @Override public URL get() { diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java new file mode 100644 index 0000000..2799a83 --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -0,0 +1,143 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.net.URL; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Builds a pre-filled GitHub issue title/body from a crash: the exception itself (available + * directly, no parsing needed), plus the engine version and active module list, which only exist + * in the crashed process' own log output - the reporter runs in its own JVM (see #52, subprocess + * isolation) and has no other way to reach them. + *

+ * The regexes here mirror two fixed, narrow log lines the engine emits at startup - see + * {@code TerasologyEngine#logEnvironmentInfo} ({@code TerasologyVersion#toString}'s + * {@code [buildNumber=..., ..., engineVersion=X, displayVersion=Y]} format) and + * {@code RegisterMods} ({@code "Activating module: :"}, once per active module). + * Log formatting is not a published API and can drift; a change there degrades this to a blank + * "unknown"/empty-list extract rather than failing the report itself. + */ +public final class CrashSummary { + + private static final int MAX_STACK_LINES = 15; + private static final int MAX_MODULES_LISTED = 30; + private static final int MAX_TITLE_MESSAGE_LENGTH = 80; + + private static final Pattern ENGINE_VERSION_PATTERN = Pattern.compile("engineVersion=([^,\\]]*)"); + private static final Pattern DISPLAY_VERSION_PATTERN = Pattern.compile("displayVersion=([^,\\]]*)"); + private static final Pattern ACTIVE_MODULE_PATTERN = Pattern.compile("Activating module: (\\S+:\\S+)"); + + private final Throwable exception; + private final String stackTraceExtract; + private final String engineVersion; + private final String displayVersion; + private final List activeModules; + + private CrashSummary(Throwable exception, String stackTraceExtract, String engineVersion, + String displayVersion, List activeModules) { + this.exception = exception; + this.stackTraceExtract = stackTraceExtract; + this.engineVersion = engineVersion; + this.displayVersion = displayVersion; + this.activeModules = activeModules; + } + + public static CrashSummary extract(Throwable exception, String combinedLogText) { + String text = combinedLogText != null ? combinedLogText : ""; + return new CrashSummary(exception, extractStackTrace(exception), + firstGroup(ENGINE_VERSION_PATTERN, text), firstGroup(DISPLAY_VERSION_PATTERN, text), + extractActiveModules(text)); + } + + private static String extractStackTrace(Throwable exception) { + StringWriter sink = new StringWriter(); + exception.printStackTrace(new PrintWriter(sink)); + String[] lines = sink.toString().split("\r?\n"); + StringBuilder builder = new StringBuilder(); + int limit = Math.min(lines.length, MAX_STACK_LINES); + for (int i = 0; i < limit; i++) { + builder.append(lines[i]).append('\n'); + } + if (lines.length > limit) { + builder.append("... ").append(lines.length - limit).append(" more line(s) - see the full log\n"); + } + return builder.toString().trim(); + } + + private static String firstGroup(Pattern pattern, String text) { + Matcher matcher = pattern.matcher(text); + return matcher.find() ? matcher.group(1).trim() : ""; + } + + private static List extractActiveModules(String text) { + List modules = new ArrayList<>(); + Matcher matcher = ACTIVE_MODULE_PATTERN.matcher(text); + while (matcher.find()) { + String module = matcher.group(1); + if (!modules.contains(module)) { + modules.add(module); + } + } + return modules; + } + + /** + * @return a short, single-line issue title: the exception's simple class name, plus a + * truncated message if it has one. + */ + public String buildTitle() { + StringBuilder title = new StringBuilder("Crash: ").append(exception.getClass().getSimpleName()); + String message = exception.getLocalizedMessage(); + if (message != null && !message.trim().isEmpty()) { + String trimmed = message.length() > MAX_TITLE_MESSAGE_LENGTH + ? message.substring(0, MAX_TITLE_MESSAGE_LENGTH - 3) + "..." + : message; + title.append(": ").append(trimmed); + } + return title.toString(); + } + + /** + * @param pastebinLink the uploaded log link, or {@code null} if the user skipped upload + * @return a Markdown issue body with the exception extract, environment info, and a link to + * the full logs + */ + public String buildBody(URL pastebinLink) { + StringBuilder body = new StringBuilder(); + body.append("### Exception\n\n```\n").append(stackTraceExtract).append("\n```\n\n"); + + body.append("### Environment\n\n"); + body.append("- Terasology version: ").append(engineVersion.isEmpty() ? "unknown" : engineVersion); + if (!displayVersion.isEmpty()) { + body.append(" (").append(displayVersion).append(')'); + } + body.append('\n'); + body.append("- OS: ").append(System.getProperty("os.name")).append(' ') + .append(System.getProperty("os.version")).append(" (").append(System.getProperty("os.arch")).append(")\n"); + body.append("- Active modules:"); + if (activeModules.isEmpty()) { + body.append(" none found in logs\n"); + } else { + body.append('\n'); + int shown = Math.min(activeModules.size(), MAX_MODULES_LISTED); + for (int i = 0; i < shown; i++) { + body.append(" - ").append(activeModules.get(i)).append('\n'); + } + if (activeModules.size() > shown) { + body.append(" - ... ").append(activeModules.size() - shown).append(" more\n"); + } + } + + body.append("\n### Full logs\n\n"); + body.append(pastebinLink != null ? "[PasteBin](" + pastebinLink + ")\n" : "(not uploaded)\n"); + + return body.toString(); + } +} diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java index ebacd35..fcca439 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java @@ -39,6 +39,10 @@ public class FinalActionsPanel extends JPanel { private static final long serialVersionUID = 2639334979749507943L; + private final Throwable exception; + + private final Supplier logTextSupplier; + private final Supplier uploadedFile; private final JTextArea linkText; @@ -47,8 +51,11 @@ public class FinalActionsPanel extends JPanel { private boolean pageComplete; - public FinalActionsPanel(GlobalProperties properties, Supplier uploadedFile) { + public FinalActionsPanel(GlobalProperties properties, Throwable exception, Supplier logTextSupplier, + Supplier uploadedFile) { + this.exception = exception; + this.logTextSupplier = logTextSupplier; this.uploadedFile = uploadedFile; setLayout(new BorderLayout(0, 10)); @@ -89,7 +96,10 @@ public void actionPerformed(ActionEvent e) { @Override public void actionPerformed(ActionEvent e) { - openInBrowser(properties.get(KEY.REPORT_ISSUE_LINK)); + CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get()); + String link = GitHubIssueLinkBuilder.build(properties.get(KEY.REPORT_ISSUE_LINK), + summary.buildTitle(), summary.buildBody(uploadedFile.get())); + openInBrowser(link); pageComplete = true; firePropertyChange("pageComplete", !pageComplete, pageComplete); } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java new file mode 100644 index 0000000..bcbac99 --- /dev/null +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java @@ -0,0 +1,31 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; + +/** + * Builds a GitHub new-issue URL pre-filled via the {@code title}/{@code body} query parameters + * GitHub's issue-creation form accepts. + */ +public final class GitHubIssueLinkBuilder { + + private GitHubIssueLinkBuilder() { + } + + public static String build(String baseUrl, String title, String body) { + String query = "title=" + encode(title) + "&body=" + encode(body); + return baseUrl + (baseUrl.contains("?") ? "&" : "?") + query; + } + + private static String encode(String value) { + try { + return URLEncoder.encode(value, "UTF-8"); + } catch (UnsupportedEncodingException e) { + // UTF-8 is a standard charset every JVM implementation is required to support. + throw new AssertionError(e); + } + } +} diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java new file mode 100644 index 0000000..6065361 --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -0,0 +1,94 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.Test; + +import java.net.MalformedURLException; +import java.net.URL; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression tests for #53 item 3: "Report Issue" opened a blank GitHub form instead of one + * pre-filled with the crash summary. + */ +class CrashSummaryTest { + + private static final String LOG_TEXT = + "10:00:00.000 [main] INFO o.t.e.version.TerasologyVersion - " + + "[buildNumber=42, buildId=42, buildTag=Terasology-42, buildUrl=, jobName=Terasology/engine/develop, " + + "dateTime=2026-08-20, displayVersion=Aeternum, engineVersion=5.4.0-SNAPSHOT]\n" + + "10:00:00.100 [main] INFO o.t.e.core.TerasologyEngine - OS: Linux, arch: amd64, version: 6.12.85\n" + + "10:00:01.000 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: engine:5.4.0-SNAPSHOT\n" + + "10:00:01.010 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: CoreAssets:2.4.0\n" + + "10:00:01.020 [main] INFO o.t.e.core.modes.loadProcesses.RegisterMods - Activating module: CoreAssets:2.4.0\n"; + + @Test + void extractsEngineAndDisplayVersion() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("5.4.0-SNAPSHOT"), "Expected the engine version in the body, got: " + body); + assertTrue(body.contains("Aeternum"), "Expected the display version in the body, got: " + body); + } + + @Test + void extractsActiveModulesAndDeduplicates() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("engine:5.4.0-SNAPSHOT"), "Expected engine module, got: " + body); + // Occurs twice in the log (duplicate "Activating module" line) - should appear once in the body. + int occurrences = body.split("CoreAssets:2\\.4\\.0", -1).length - 1; + assertEquals(1, occurrences, "Expected the duplicate module line deduplicated, got: " + body); + } + + @Test + void missingVersionAndModulesDegradeGracefullyInsteadOfFailing() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here"); + String body = summary.buildBody(null); + + assertTrue(body.contains("unknown"), "Expected a fallback for a missing version, got: " + body); + assertTrue(body.contains("none found in logs"), "Expected a fallback for an empty module list, got: " + body); + } + + @Test + void titleUsesExceptionClassAndMessage() { + CrashSummary summary = CrashSummary.extract(new IllegalStateException("world was null"), LOG_TEXT); + + assertEquals("Crash: IllegalStateException: world was null", summary.buildTitle()); + } + + @Test + void bodyIncludesTheExceptionExtract() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("kaboom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertTrue(body.contains("kaboom"), "Expected the exception message in the body, got: " + body); + assertTrue(body.contains("RuntimeException"), "Expected the exception type in the body, got: " + body); + } + + @Test + void bodyIncludesThePastebinLinkWhenUploaded() throws MalformedURLException { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + URL link = new URL("https://pastebin.com/abc123"); + + String body = summary.buildBody(link); + + assertTrue(body.contains("https://pastebin.com/abc123"), "Expected the PasteBin link in the body, got: " + body); + } + + @Test + void bodyNotesWhenUploadWasSkipped() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + + String body = summary.buildBody(null); + + assertFalse(body.contains("null"), "A skipped upload must not leak the literal string \"null\" into the body: " + body); + assertTrue(body.contains("not uploaded"), "Expected a note that upload was skipped, got: " + body); + } +} diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java new file mode 100644 index 0000000..ece4418 --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java @@ -0,0 +1,37 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class GitHubIssueLinkBuilderTest { + + @Test + void appendsTitleAndBodyAsQueryParameters() { + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "Crash: NullPointerException", "some body text"); + + assertTrue(link.startsWith("https://github.com/MovingBlocks/Terasology/issues/new?")); + assertTrue(link.contains("title=Crash%3A+NullPointerException"), link); + assertTrue(link.contains("body=some+body+text"), link); + } + + @Test + void encodesSpecialCharactersInTitleAndBody() { + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "a & b", "line1\nline2"); + + assertTrue(link.contains("title=a+%26+b"), link); + assertTrue(link.contains("body=line1%0Aline2"), link); + } + + @Test + void usesAmpersandWhenBaseUrlAlreadyHasAQueryString() { + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new?template=bug", "t", "b"); + + assertEquals("https://example.com/issues/new?template=bug&title=t&body=b", link); + } +} From 8d76c1329b3d3d44b6bb368bd0543814ed7f34a1 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 14:33:01 +0200 Subject: [PATCH 2/7] fix(reporter): list exceptions from every log tab in the issue body CrashSummary.buildBody() only ever showed the single in-process Throwable the reporter happened to be invoked with. Found while testing with multiple log tabs (init/menu/game): the game.log tab had its own NullPointerException, but "File an issue on GitHub" only pre-filled the crash that triggered the reporter, silently leaving the other one out even though ErrorMessagePanel#getLog() already combines every tab's text into what CrashSummary gets. New "### Other exceptions found in logs" section scans that combined text for stack traces (any "some.FullyQualified.NameException[: message]" header line immediately followed by "at "/"Caused by:" frames - the shape every JVM logging framework prints a Throwable in), attributes each to its tab, and skips the primary exception's own entry so it isn't listed twice when the crashed process also logged it in its own log file. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/pages/CrashSummary.java | 75 ++++++++++++++++++- .../crashreporter/pages/CrashSummaryTest.java | 44 +++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index 2799a83..f89f4df 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -29,31 +29,41 @@ public final class CrashSummary { private static final int MAX_STACK_LINES = 15; private static final int MAX_MODULES_LISTED = 30; private static final int MAX_TITLE_MESSAGE_LENGTH = 80; + private static final int MAX_OTHER_EXCEPTIONS_LISTED = 10; private static final Pattern ENGINE_VERSION_PATTERN = Pattern.compile("engineVersion=([^,\\]]*)"); private static final Pattern DISPLAY_VERSION_PATTERN = Pattern.compile("displayVersion=([^,\\]]*)"); private static final Pattern ACTIVE_MODULE_PATTERN = Pattern.compile("Activating module: (\\S+:\\S+)"); + private static final Pattern LOG_TAB_PATTERN = Pattern.compile("(?m)^=== (.*) ===$"); + // A log-formatted stack trace: a "some.FullyQualified.NameException[: message]" header line + // immediately followed by one or more "at ..."/"Caused by: ..." frame lines - the shape every + // JVM logging framework prints a Throwable in (Logback's %ex, java.util.logging, a raw + // printStackTrace()), regardless of which class emits it. + private static final Pattern STACK_TRACE_HEADER_PATTERN = Pattern.compile( + "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n((?:[ \\t]*(?:at |Caused by:)[^\\n]*\\n?)+)"); private final Throwable exception; private final String stackTraceExtract; private final String engineVersion; private final String displayVersion; private final List activeModules; + private final List otherExceptions; private CrashSummary(Throwable exception, String stackTraceExtract, String engineVersion, - String displayVersion, List activeModules) { + String displayVersion, List activeModules, List otherExceptions) { this.exception = exception; this.stackTraceExtract = stackTraceExtract; this.engineVersion = engineVersion; this.displayVersion = displayVersion; this.activeModules = activeModules; + this.otherExceptions = otherExceptions; } public static CrashSummary extract(Throwable exception, String combinedLogText) { String text = combinedLogText != null ? combinedLogText : ""; return new CrashSummary(exception, extractStackTrace(exception), firstGroup(ENGINE_VERSION_PATTERN, text), firstGroup(DISPLAY_VERSION_PATTERN, text), - extractActiveModules(text)); + extractActiveModules(text), extractOtherExceptions(text, exception)); } private static String extractStackTrace(Throwable exception) { @@ -88,6 +98,56 @@ private static List extractActiveModules(String text) { return modules; } + /** + * Scans every log tab (not just the one that triggered this report - see {@link #buildBody}) for + * other stack traces, so a crash whose real cause is an earlier exception logged in a different + * tab (e.g. during init) isn't left out of the pre-filled issue just because it wasn't the + * in-process {@code exception} the reporter happened to be invoked with. + * + * @return "tab name: header line" for each distinct exception found, skipping {@code exception}'s + * own header line - that one is already covered by {@link #stackTraceExtract}. + */ + private static List extractOtherExceptions(String combinedLogText, Throwable exception) { + String primaryHeader = exception.toString().trim(); + List found = new ArrayList<>(); + + Matcher tabMatcher = LOG_TAB_PATTERN.matcher(combinedLogText); + int tabStart = -1; + String tabName = null; + while (true) { + boolean hasNext = tabMatcher.find(); + int nextStart = hasNext ? tabMatcher.start() : combinedLogText.length(); + if (tabName != null) { + collectExceptionHeaders(combinedLogText.substring(tabStart, nextStart), tabName, primaryHeader, found); + } + if (!hasNext) { + break; + } + tabName = tabMatcher.group(1); + tabStart = tabMatcher.end(); + } + // No "=== tab ===" headers at all - a single combined-log caller (e.g. a direct test) rather + // than ErrorMessagePanel#getLog(); scan the whole text as one unnamed tab. + if (tabName == null) { + collectExceptionHeaders(combinedLogText, "log", primaryHeader, found); + } + return found; + } + + private static void collectExceptionHeaders(String tabText, String tabName, String primaryHeader, List found) { + Matcher matcher = STACK_TRACE_HEADER_PATTERN.matcher(tabText); + while (matcher.find()) { + String header = (matcher.group(1) + (matcher.group(2) != null ? matcher.group(2) : "")).trim(); + if (header.equals(primaryHeader)) { + continue; + } + String entry = tabName + ": " + header; + if (!found.contains(entry)) { + found.add(entry); + } + } + } + /** * @return a short, single-line issue title: the exception's simple class name, plus a * truncated message if it has one. @@ -135,6 +195,17 @@ public String buildBody(URL pastebinLink) { } } + if (!otherExceptions.isEmpty()) { + body.append("\n### Other exceptions found in logs\n\n"); + int shown = Math.min(otherExceptions.size(), MAX_OTHER_EXCEPTIONS_LISTED); + for (int i = 0; i < shown; i++) { + body.append("- `").append(otherExceptions.get(i)).append("`\n"); + } + if (otherExceptions.size() > shown) { + body.append("- ... ").append(otherExceptions.size() - shown).append(" more - see the full log\n"); + } + } + body.append("\n### Full logs\n\n"); body.append(pastebinLink != null ? "[PasteBin](" + pastebinLink + ")\n" : "(not uploaded)\n"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 6065361..96ad4ce 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -91,4 +91,48 @@ void bodyNotesWhenUploadWasSkipped() { assertFalse(body.contains("null"), "A skipped upload must not leak the literal string \"null\" into the body: " + body); assertTrue(body.contains("not uploaded"), "Expected a note that upload was skipped, got: " + body); } + + // Regression: ErrorMessagePanel#getLog() combines every log tab, not just the one that + // triggered the report, but only the in-process exception ever made it into the pre-filled + // issue - an exception logged in a different tab (e.g. an earlier init-time failure) was + // silently left out even though it was right there in the combined text. + @Test + void bodyListsExceptionsFoundInOtherLogTabs() { + String combinedLog = "=== Terasology-init.log ===\n" + LOG_TEXT + + "\n=== Terasology-game.log ===\n" + + "10:10:05.123 [main] ERROR o.t.e.core.TerasologyEngine - Uncaught exception in main loop\n" + + "java.lang.NullPointerException: world was null\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; + + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog); + String body = summary.buildBody(null); + + assertTrue(body.contains("### Other exceptions found in logs"), "Expected a section listing it, got: " + body); + assertTrue(body.contains("Terasology-game.log: java.lang.NullPointerException: world was null"), + "Expected the other tab's exception attributed to its tab, got: " + body); + } + + @Test + void bodyDoesNotDuplicateThePrimaryExceptionAsAnOtherException() { + RuntimeException primary = new RuntimeException("boom"); + // The crash is very often also logged (by the crashed process itself) in one of its own + // log tabs - that's the same exception, not another one, and must not be listed twice. + String combinedLog = "=== Terasology-game.log ===\n" + + primary + "\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; + + CrashSummary summary = CrashSummary.extract(primary, combinedLog); + String body = summary.buildBody(null); + + assertFalse(body.contains("### Other exceptions found in logs"), + "The primary exception's own log entry must not be listed as an \"other\" exception, got: " + body); + } + + @Test + void bodyOmitsTheOtherExceptionsSectionWhenThereAreNone() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + String body = summary.buildBody(null); + + assertFalse(body.contains("### Other exceptions found in logs"), "Expected no such section, got: " + body); + } } From 4c74297ccfda15fe5e66c8e17e2d8620fa3d4e64 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 14:51:47 +0200 Subject: [PATCH 3/7] fix(reporter): list every exception, one row each naming its log tab Splitting this into a separate "Exception" block (the primary crash, full multi-line trace) and a completely separate "Other exceptions found in logs" section far below Environment made the two hard to connect and easy to miss - reported as confusing after testing live. Now every exception found - the primary one first, then every other one - is a single row directly under "### Exceptions", each naming the log tab it was found in (or "this crash" if it wasn't logged in any tab, which is normal - it's the in-process exception that triggered the report). That section comes immediately before "### Environment". The primary exception is attributed to whichever tab also logged it, instead of being duplicated as a separate "other" entry. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/pages/CrashSummary.java | 172 ++++++++++-------- .../crashreporter/pages/CrashSummaryTest.java | 29 ++- 2 files changed, 117 insertions(+), 84 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index f89f4df..f09f633 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -3,8 +3,6 @@ package org.terasology.crashreporter.pages; -import java.io.PrintWriter; -import java.io.StringWriter; import java.net.URL; import java.util.ArrayList; import java.util.List; @@ -13,9 +11,9 @@ /** * Builds a pre-filled GitHub issue title/body from a crash: the exception itself (available - * directly, no parsing needed), plus the engine version and active module list, which only exist - * in the crashed process' own log output - the reporter runs in its own JVM (see #52, subprocess - * isolation) and has no other way to reach them. + * directly, no parsing needed), plus every other exception found across the crashed process' own + * log tabs, and the engine version/active module list - the reporter runs in its own JVM (see #52, + * subprocess isolation) and has no other way to reach any of the log-only information. *

* The regexes here mirror two fixed, narrow log lines the engine emits at startup - see * {@code TerasologyEngine#logEnvironmentInfo} ({@code TerasologyVersion#toString}'s @@ -26,10 +24,10 @@ */ public final class CrashSummary { - private static final int MAX_STACK_LINES = 15; private static final int MAX_MODULES_LISTED = 30; private static final int MAX_TITLE_MESSAGE_LENGTH = 80; - private static final int MAX_OTHER_EXCEPTIONS_LISTED = 10; + private static final int MAX_EXCEPTIONS_LISTED = 10; + private static final String NO_TAB_LABEL = "this crash"; private static final Pattern ENGINE_VERSION_PATTERN = Pattern.compile("engineVersion=([^,\\]]*)"); private static final Pattern DISPLAY_VERSION_PATTERN = Pattern.compile("displayVersion=([^,\\]]*)"); @@ -43,73 +41,91 @@ public final class CrashSummary { "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n((?:[ \\t]*(?:at |Caused by:)[^\\n]*\\n?)+)"); private final Throwable exception; - private final String stackTraceExtract; + private final List exceptionRows; + private final int moreExceptionsCount; private final String engineVersion; private final String displayVersion; private final List activeModules; - private final List otherExceptions; - private CrashSummary(Throwable exception, String stackTraceExtract, String engineVersion, - String displayVersion, List activeModules, List otherExceptions) { + private CrashSummary(Throwable exception, List exceptionRows, int moreExceptionsCount, + String engineVersion, String displayVersion, List activeModules) { this.exception = exception; - this.stackTraceExtract = stackTraceExtract; + this.exceptionRows = exceptionRows; + this.moreExceptionsCount = moreExceptionsCount; this.engineVersion = engineVersion; this.displayVersion = displayVersion; this.activeModules = activeModules; - this.otherExceptions = otherExceptions; } public static CrashSummary extract(Throwable exception, String combinedLogText) { String text = combinedLogText != null ? combinedLogText : ""; - return new CrashSummary(exception, extractStackTrace(exception), + + List rows = buildExceptionRows(exception, text); + int moreCount = 0; + if (rows.size() > MAX_EXCEPTIONS_LISTED) { + moreCount = rows.size() - MAX_EXCEPTIONS_LISTED; + rows = new ArrayList<>(rows.subList(0, MAX_EXCEPTIONS_LISTED)); + } + + return new CrashSummary(exception, rows, moreCount, firstGroup(ENGINE_VERSION_PATTERN, text), firstGroup(DISPLAY_VERSION_PATTERN, text), - extractActiveModules(text), extractOtherExceptions(text, exception)); + extractActiveModules(text)); } - private static String extractStackTrace(Throwable exception) { - StringWriter sink = new StringWriter(); - exception.printStackTrace(new PrintWriter(sink)); - String[] lines = sink.toString().split("\r?\n"); - StringBuilder builder = new StringBuilder(); - int limit = Math.min(lines.length, MAX_STACK_LINES); - for (int i = 0; i < limit; i++) { - builder.append(lines[i]).append('\n'); + /** + * Builds one row per distinct exception found - the one that triggered this report first + * (attributed to whichever log tab also logged it, if any - see {@link #NO_TAB_LABEL}), then + * every other exception found across the log tabs, so a crash whose real cause is an earlier + * exception logged in a different tab (e.g. during init) isn't left out of the pre-filled issue + * just because it wasn't the in-process {@code exception} the reporter happened to be invoked + * with. + */ + private static List buildExceptionRows(Throwable exception, String combinedLogText) { + String primaryHeader = exception.toString().trim(); + List found = findAllExceptions(combinedLogText); + + String primaryTab = null; + for (ExceptionEntry entry : found) { + if (entry.header.equals(primaryHeader)) { + primaryTab = entry.tabName; + break; + } } - if (lines.length > limit) { - builder.append("... ").append(lines.length - limit).append(" more line(s) - see the full log\n"); + + List rows = new ArrayList<>(); + rows.add(formatRow(primaryTab, primaryHeader)); + for (ExceptionEntry entry : found) { + if (entry.header.equals(primaryHeader)) { + continue; + } + String row = formatRow(entry.tabName, entry.header); + if (!rows.contains(row)) { + rows.add(row); + } } - return builder.toString().trim(); + return rows; } - private static String firstGroup(Pattern pattern, String text) { - Matcher matcher = pattern.matcher(text); - return matcher.find() ? matcher.group(1).trim() : ""; + private static String formatRow(String tabName, String header) { + String label = tabName != null ? tabName : NO_TAB_LABEL; + String message = header.length() > MAX_TITLE_MESSAGE_LENGTH + ? header.substring(0, MAX_TITLE_MESSAGE_LENGTH - 3) + "..." + : header; + return "**" + label + "**: `" + message + "`"; } - private static List extractActiveModules(String text) { - List modules = new ArrayList<>(); - Matcher matcher = ACTIVE_MODULE_PATTERN.matcher(text); - while (matcher.find()) { - String module = matcher.group(1); - if (!modules.contains(module)) { - modules.add(module); - } + private static final class ExceptionEntry { + private final String tabName; + private final String header; + + private ExceptionEntry(String tabName, String header) { + this.tabName = tabName; + this.header = header; } - return modules; } - /** - * Scans every log tab (not just the one that triggered this report - see {@link #buildBody}) for - * other stack traces, so a crash whose real cause is an earlier exception logged in a different - * tab (e.g. during init) isn't left out of the pre-filled issue just because it wasn't the - * in-process {@code exception} the reporter happened to be invoked with. - * - * @return "tab name: header line" for each distinct exception found, skipping {@code exception}'s - * own header line - that one is already covered by {@link #stackTraceExtract}. - */ - private static List extractOtherExceptions(String combinedLogText, Throwable exception) { - String primaryHeader = exception.toString().trim(); - List found = new ArrayList<>(); + private static List findAllExceptions(String combinedLogText) { + List found = new ArrayList<>(); Matcher tabMatcher = LOG_TAB_PATTERN.matcher(combinedLogText); int tabStart = -1; @@ -118,7 +134,7 @@ private static List extractOtherExceptions(String combinedLogText, Throw boolean hasNext = tabMatcher.find(); int nextStart = hasNext ? tabMatcher.start() : combinedLogText.length(); if (tabName != null) { - collectExceptionHeaders(combinedLogText.substring(tabStart, nextStart), tabName, primaryHeader, found); + collectExceptionHeaders(combinedLogText.substring(tabStart, nextStart), tabName, found); } if (!hasNext) { break; @@ -127,25 +143,36 @@ private static List extractOtherExceptions(String combinedLogText, Throw tabStart = tabMatcher.end(); } // No "=== tab ===" headers at all - a single combined-log caller (e.g. a direct test) rather - // than ErrorMessagePanel#getLog(); scan the whole text as one unnamed tab. + // than ErrorMessagePanel#getLog(); scan the whole text with no tab attribution. if (tabName == null) { - collectExceptionHeaders(combinedLogText, "log", primaryHeader, found); + collectExceptionHeaders(combinedLogText, null, found); } return found; } - private static void collectExceptionHeaders(String tabText, String tabName, String primaryHeader, List found) { + private static void collectExceptionHeaders(String tabText, String tabName, List found) { Matcher matcher = STACK_TRACE_HEADER_PATTERN.matcher(tabText); while (matcher.find()) { String header = (matcher.group(1) + (matcher.group(2) != null ? matcher.group(2) : "")).trim(); - if (header.equals(primaryHeader)) { - continue; - } - String entry = tabName + ": " + header; - if (!found.contains(entry)) { - found.add(entry); + found.add(new ExceptionEntry(tabName, header)); + } + } + + private static String firstGroup(Pattern pattern, String text) { + Matcher matcher = pattern.matcher(text); + return matcher.find() ? matcher.group(1).trim() : ""; + } + + private static List extractActiveModules(String text) { + List modules = new ArrayList<>(); + Matcher matcher = ACTIVE_MODULE_PATTERN.matcher(text); + while (matcher.find()) { + String module = matcher.group(1); + if (!modules.contains(module)) { + modules.add(module); } } + return modules; } /** @@ -166,12 +193,20 @@ public String buildTitle() { /** * @param pastebinLink the uploaded log link, or {@code null} if the user skipped upload - * @return a Markdown issue body with the exception extract, environment info, and a link to - * the full logs + * @return a Markdown issue body: every exception found (one row each, naming the log tab it was + * found in), then environment info, then a link to the full logs */ public String buildBody(URL pastebinLink) { StringBuilder body = new StringBuilder(); - body.append("### Exception\n\n```\n").append(stackTraceExtract).append("\n```\n\n"); + + body.append("### Exceptions\n\n"); + for (String row : exceptionRows) { + body.append("- ").append(row).append('\n'); + } + if (moreExceptionsCount > 0) { + body.append("- ... ").append(moreExceptionsCount).append(" more - see the full log\n"); + } + body.append('\n'); body.append("### Environment\n\n"); body.append("- Terasology version: ").append(engineVersion.isEmpty() ? "unknown" : engineVersion); @@ -195,17 +230,6 @@ public String buildBody(URL pastebinLink) { } } - if (!otherExceptions.isEmpty()) { - body.append("\n### Other exceptions found in logs\n\n"); - int shown = Math.min(otherExceptions.size(), MAX_OTHER_EXCEPTIONS_LISTED); - for (int i = 0; i < shown; i++) { - body.append("- `").append(otherExceptions.get(i)).append("`\n"); - } - if (otherExceptions.size() > shown) { - body.append("- ... ").append(otherExceptions.size() - shown).append(" more - see the full log\n"); - } - } - body.append("\n### Full logs\n\n"); body.append(pastebinLink != null ? "[PasteBin](" + pastebinLink + ")\n" : "(not uploaded)\n"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 96ad4ce..2bbcbf5 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -95,9 +95,11 @@ void bodyNotesWhenUploadWasSkipped() { // Regression: ErrorMessagePanel#getLog() combines every log tab, not just the one that // triggered the report, but only the in-process exception ever made it into the pre-filled // issue - an exception logged in a different tab (e.g. an earlier init-time failure) was - // silently left out even though it was right there in the combined text. + // silently left out even though it was right there in the combined text. All exceptions - the + // primary one and every other one found - are listed together, one row each, before + // "### Environment", not split across two separate sections. @Test - void bodyListsExceptionsFoundInOtherLogTabs() { + void bodyListsExceptionsFoundInOtherLogTabsAsARowNamingTheTab() { String combinedLog = "=== Terasology-init.log ===\n" + LOG_TEXT + "\n=== Terasology-game.log ===\n" + "10:10:05.123 [main] ERROR o.t.e.core.TerasologyEngine - Uncaught exception in main loop\n" @@ -107,13 +109,17 @@ void bodyListsExceptionsFoundInOtherLogTabs() { CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog); String body = summary.buildBody(null); - assertTrue(body.contains("### Other exceptions found in logs"), "Expected a section listing it, got: " + body); - assertTrue(body.contains("Terasology-game.log: java.lang.NullPointerException: world was null"), - "Expected the other tab's exception attributed to its tab, got: " + body); + assertTrue(body.contains("### Exceptions"), "Expected a single unified exceptions section, got: " + body); + assertTrue(body.contains("**Terasology-game.log**: `java.lang.NullPointerException: world was null`"), + "Expected the other tab's exception as its own row naming the tab, got: " + body); + int exceptionsIndex = body.indexOf("### Exceptions"); + int environmentIndex = body.indexOf("### Environment"); + assertTrue(exceptionsIndex >= 0 && environmentIndex > exceptionsIndex, + "Expected \"### Exceptions\" before \"### Environment\", got: " + body); } @Test - void bodyDoesNotDuplicateThePrimaryExceptionAsAnOtherException() { + void bodyAttributesThePrimaryExceptionToItsOwnTabInsteadOfListingItTwice() { RuntimeException primary = new RuntimeException("boom"); // The crash is very often also logged (by the crashed process itself) in one of its own // log tabs - that's the same exception, not another one, and must not be listed twice. @@ -124,15 +130,18 @@ void bodyDoesNotDuplicateThePrimaryExceptionAsAnOtherException() { CrashSummary summary = CrashSummary.extract(primary, combinedLog); String body = summary.buildBody(null); - assertFalse(body.contains("### Other exceptions found in logs"), - "The primary exception's own log entry must not be listed as an \"other\" exception, got: " + body); + assertTrue(body.contains("**Terasology-game.log**: `java.lang.RuntimeException: boom`"), + "Expected the primary exception's row attributed to the tab it was found in, got: " + body); + int rows = body.split("\n- \\*\\*", -1).length - 1; + assertEquals(1, rows, "Expected exactly one row - the primary exception must not also be listed as an \"other\" one: " + body); } @Test - void bodyOmitsTheOtherExceptionsSectionWhenThereAreNone() { + void bodyAttributesThePrimaryExceptionToThisCrashWhenNotFoundInAnyTab() { CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); String body = summary.buildBody(null); - assertFalse(body.contains("### Other exceptions found in logs"), "Expected no such section, got: " + body); + assertTrue(body.contains("**this crash**: `java.lang.RuntimeException: boom`"), + "Expected a fallback label when the exception isn't found in any log tab, got: " + body); } } From 0c345c5f7381eed3c4290f5da9767c333a6bb00b Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 16:54:56 +0200 Subject: [PATCH 4/7] fix(reporter): keep the full stack trace for every exception listed The previous one-line-per-exception format was too terse to actually debug from - a real fix needs the trace, not just a class name and message. Every exception found now gets its own labeled code block (full trace, capped at 15 lines like before) instead of a single-line summary, still all together under "### Exceptions" before "### Environment". Also: on macOS the crash reporter always relaunches in a subprocess (requiresProcessIsolation()), which reconstructs the exception from just its class name and message - its own stack trace points into CrashReporter's own relaunch machinery, not the real crash site. So whenever the primary exception was also logged in one of the log tabs (the normal case for an engine-level crash handler), the trace captured from that log text is used instead - it's the real one. The exception object's own trace is now only a fallback for when nothing better is available. Fixed a real bug found while writing this: collectExceptionHeaders() and framesFromThrowable() both called String.trim() on the captured frame text, which silently ate the leading tab off the *first* frame line ("\tat ...") since trim() only strips from the very edges of the whole string, not per-line - every trace in the pre-filled issue would have rendered its first frame without the indentation the rest have. New stripTrailingWhitespace() strips only the trailing newline. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/pages/CrashSummary.java | 138 ++++++++++++------ .../crashreporter/pages/CrashSummaryTest.java | 36 +++-- 2 files changed, 118 insertions(+), 56 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index f09f633..04f03cd 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -3,6 +3,8 @@ package org.terasology.crashreporter.pages; +import java.io.PrintWriter; +import java.io.StringWriter; import java.net.URL; import java.util.ArrayList; import java.util.List; @@ -10,13 +12,21 @@ import java.util.regex.Pattern; /** - * Builds a pre-filled GitHub issue title/body from a crash: the exception itself (available - * directly, no parsing needed), plus every other exception found across the crashed process' own - * log tabs, and the engine version/active module list - the reporter runs in its own JVM (see #52, - * subprocess isolation) and has no other way to reach any of the log-only information. + * Builds a pre-filled GitHub issue title/body from a crash: the exception itself, plus every other + * exception found across the crashed process' own log tabs, and the engine version/active module + * list - the reporter runs in its own JVM (see #52, subprocess isolation) and has no other way to + * reach any of the log-only information. *

- * The regexes here mirror two fixed, narrow log lines the engine emits at startup - see - * {@code TerasologyEngine#logEnvironmentInfo} ({@code TerasologyVersion#toString}'s + * On macOS, that subprocess isolation means the in-process {@code Throwable} passed to + * {@link #extract} is a best-effort reconstruction from just its class name and message (see + * {@code CrashReporter#reconstructThrowable}) - its own stack trace points into the reporter's own + * relaunch machinery, not the real crash site. Whenever the crash was also logged in one of the log + * tabs (the normal case for an engine-level crash handler), the trace captured from that log text is + * the real one and is used instead; the reconstructed exception's own trace is only a fallback for + * when nothing better is available. + *

+ * The version/module regexes here mirror two fixed, narrow log lines the engine emits at startup - + * see {@code TerasologyEngine#logEnvironmentInfo} ({@code TerasologyVersion#toString}'s * {@code [buildNumber=..., ..., engineVersion=X, displayVersion=Y]} format) and * {@code RegisterMods} ({@code "Activating module: :"}, once per active module). * Log formatting is not a published API and can drift; a change there degrades this to a blank @@ -24,6 +34,7 @@ */ public final class CrashSummary { + private static final int MAX_STACK_LINES = 15; private static final int MAX_MODULES_LISTED = 30; private static final int MAX_TITLE_MESSAGE_LENGTH = 80; private static final int MAX_EXCEPTIONS_LISTED = 10; @@ -41,16 +52,16 @@ public final class CrashSummary { "(?m)^([\\w$]+(?:\\.[\\w$]+)+(?:Exception|Error))(:[^\\n]*)?\\n((?:[ \\t]*(?:at |Caused by:)[^\\n]*\\n?)+)"); private final Throwable exception; - private final List exceptionRows; + private final List exceptionBlocks; private final int moreExceptionsCount; private final String engineVersion; private final String displayVersion; private final List activeModules; - private CrashSummary(Throwable exception, List exceptionRows, int moreExceptionsCount, + private CrashSummary(Throwable exception, List exceptionBlocks, int moreExceptionsCount, String engineVersion, String displayVersion, List activeModules) { this.exception = exception; - this.exceptionRows = exceptionRows; + this.exceptionBlocks = exceptionBlocks; this.moreExceptionsCount = moreExceptionsCount; this.engineVersion = engineVersion; this.displayVersion = displayVersion; @@ -60,67 +71,108 @@ private CrashSummary(Throwable exception, List exceptionRows, int moreEx public static CrashSummary extract(Throwable exception, String combinedLogText) { String text = combinedLogText != null ? combinedLogText : ""; - List rows = buildExceptionRows(exception, text); + List blocks = buildExceptionBlocks(exception, text); int moreCount = 0; - if (rows.size() > MAX_EXCEPTIONS_LISTED) { - moreCount = rows.size() - MAX_EXCEPTIONS_LISTED; - rows = new ArrayList<>(rows.subList(0, MAX_EXCEPTIONS_LISTED)); + if (blocks.size() > MAX_EXCEPTIONS_LISTED) { + moreCount = blocks.size() - MAX_EXCEPTIONS_LISTED; + blocks = new ArrayList<>(blocks.subList(0, MAX_EXCEPTIONS_LISTED)); } - return new CrashSummary(exception, rows, moreCount, + return new CrashSummary(exception, blocks, moreCount, firstGroup(ENGINE_VERSION_PATTERN, text), firstGroup(DISPLAY_VERSION_PATTERN, text), extractActiveModules(text)); } /** - * Builds one row per distinct exception found - the one that triggered this report first - * (attributed to whichever log tab also logged it, if any - see {@link #NO_TAB_LABEL}), then - * every other exception found across the log tabs, so a crash whose real cause is an earlier - * exception logged in a different tab (e.g. during init) isn't left out of the pre-filled issue - * just because it wasn't the in-process {@code exception} the reporter happened to be invoked - * with. + * Builds one Markdown block per distinct exception found - the one that triggered this report + * first, then every other exception found across the log tabs - so a crash whose real cause is + * an earlier exception logged in a different tab (e.g. during init) isn't left out of the + * pre-filled issue just because it wasn't the in-process {@code exception} the reporter happened + * to be invoked with. */ - private static List buildExceptionRows(Throwable exception, String combinedLogText) { + private static List buildExceptionBlocks(Throwable exception, String combinedLogText) { String primaryHeader = exception.toString().trim(); List found = findAllExceptions(combinedLogText); - String primaryTab = null; + ExceptionEntry primary = null; for (ExceptionEntry entry : found) { if (entry.header.equals(primaryHeader)) { - primaryTab = entry.tabName; + primary = entry; break; } } + // Not logged anywhere - fall back to the exception object's own trace. On macOS that trace + // is a best-effort reconstruction (see the class javadoc) rather than the real crash site, + // but it's all that's available. + if (primary == null) { + primary = new ExceptionEntry(null, primaryHeader, framesFromThrowable(exception)); + } - List rows = new ArrayList<>(); - rows.add(formatRow(primaryTab, primaryHeader)); + List blocks = new ArrayList<>(); + blocks.add(formatBlock(primary)); for (ExceptionEntry entry : found) { if (entry.header.equals(primaryHeader)) { continue; } - String row = formatRow(entry.tabName, entry.header); - if (!rows.contains(row)) { - rows.add(row); + String block = formatBlock(entry); + if (!blocks.contains(block)) { + blocks.add(block); } } - return rows; + return blocks; + } + + private static String formatBlock(ExceptionEntry entry) { + String label = entry.tabName != null ? entry.tabName : NO_TAB_LABEL; + String combined = entry.frames.isEmpty() ? entry.header : entry.header + "\n" + entry.frames; + return "**" + label + "**\n\n```\n" + truncateTrace(combined) + "\n```"; + } + + private static String truncateTrace(String combined) { + String[] lines = combined.split("\r?\n"); + StringBuilder builder = new StringBuilder(); + int limit = Math.min(lines.length, MAX_STACK_LINES); + for (int i = 0; i < limit; i++) { + builder.append(lines[i]).append('\n'); + } + if (lines.length > limit) { + builder.append("... ").append(lines.length - limit).append(" more line(s) - see the full log\n"); + } + return builder.toString().trim(); } - private static String formatRow(String tabName, String header) { - String label = tabName != null ? tabName : NO_TAB_LABEL; - String message = header.length() > MAX_TITLE_MESSAGE_LENGTH - ? header.substring(0, MAX_TITLE_MESSAGE_LENGTH - 3) + "..." - : header; - return "**" + label + "**: `" + message + "`"; + private static String framesFromThrowable(Throwable exception) { + StringWriter sink = new StringWriter(); + exception.printStackTrace(new PrintWriter(sink)); + String full = sink.toString(); + // printStackTrace()'s first line is exception.toString() - already the header - so only the + // "at ..."/"Caused by: ..." frames after it are needed here. + int newlineIndex = full.indexOf('\n'); + return newlineIndex >= 0 ? stripTrailingWhitespace(full.substring(newlineIndex + 1)) : ""; + } + + /** + * Like {@link String#trim()} but only at the end - frame lines are indented with a leading tab + * ({@code "\tat ..."}), which a plain {@code trim()} would strip from the first line along with + * the trailing newline it's actually meant to remove. + */ + private static String stripTrailingWhitespace(String s) { + int end = s.length(); + while (end > 0 && Character.isWhitespace(s.charAt(end - 1))) { + end--; + } + return s.substring(0, end); } private static final class ExceptionEntry { private final String tabName; private final String header; + private final String frames; - private ExceptionEntry(String tabName, String header) { + private ExceptionEntry(String tabName, String header, String frames) { this.tabName = tabName; this.header = header; + this.frames = frames; } } @@ -154,7 +206,8 @@ private static void collectExceptionHeaders(String tabText, String tabName, List Matcher matcher = STACK_TRACE_HEADER_PATTERN.matcher(tabText); while (matcher.find()) { String header = (matcher.group(1) + (matcher.group(2) != null ? matcher.group(2) : "")).trim(); - found.add(new ExceptionEntry(tabName, header)); + String frames = stripTrailingWhitespace(matcher.group(3)); + found.add(new ExceptionEntry(tabName, header, frames)); } } @@ -193,20 +246,19 @@ public String buildTitle() { /** * @param pastebinLink the uploaded log link, or {@code null} if the user skipped upload - * @return a Markdown issue body: every exception found (one row each, naming the log tab it was - * found in), then environment info, then a link to the full logs + * @return a Markdown issue body: every exception found, one labeled code block each (naming the + * log tab it was found in), then environment info, then a link to the full logs */ public String buildBody(URL pastebinLink) { StringBuilder body = new StringBuilder(); body.append("### Exceptions\n\n"); - for (String row : exceptionRows) { - body.append("- ").append(row).append('\n'); + for (String block : exceptionBlocks) { + body.append(block).append("\n\n"); } if (moreExceptionsCount > 0) { - body.append("- ... ").append(moreExceptionsCount).append(" more - see the full log\n"); + body.append("... ").append(moreExceptionsCount).append(" more - see the full log\n\n"); } - body.append('\n'); body.append("### Environment\n\n"); body.append("- Terasology version: ").append(engineVersion.isEmpty() ? "unknown" : engineVersion); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 2bbcbf5..0ac9bc0 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -96,10 +96,11 @@ void bodyNotesWhenUploadWasSkipped() { // triggered the report, but only the in-process exception ever made it into the pre-filled // issue - an exception logged in a different tab (e.g. an earlier init-time failure) was // silently left out even though it was right there in the combined text. All exceptions - the - // primary one and every other one found - are listed together, one row each, before + // primary one and every other one found - are listed together, one labeled code block each + // (full trace, not just a one-line header - a real fix needs the actual trace), before // "### Environment", not split across two separate sections. @Test - void bodyListsExceptionsFoundInOtherLogTabsAsARowNamingTheTab() { + void bodyListsExceptionsFoundInOtherLogTabsWithTheirFullTraceNamingTheTab() { String combinedLog = "=== Terasology-init.log ===\n" + LOG_TEXT + "\n=== Terasology-game.log ===\n" + "10:10:05.123 [main] ERROR o.t.e.core.TerasologyEngine - Uncaught exception in main loop\n" @@ -110,8 +111,9 @@ void bodyListsExceptionsFoundInOtherLogTabsAsARowNamingTheTab() { String body = summary.buildBody(null); assertTrue(body.contains("### Exceptions"), "Expected a single unified exceptions section, got: " + body); - assertTrue(body.contains("**Terasology-game.log**: `java.lang.NullPointerException: world was null`"), - "Expected the other tab's exception as its own row naming the tab, got: " + body); + assertTrue(body.contains("**Terasology-game.log**\n\n```\njava.lang.NullPointerException: world was null\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n```"), + "Expected the other tab's exception as its own labeled block with the full trace, got: " + body); int exceptionsIndex = body.indexOf("### Exceptions"); int environmentIndex = body.indexOf("### Environment"); assertTrue(exceptionsIndex >= 0 && environmentIndex > exceptionsIndex, @@ -122,7 +124,9 @@ void bodyListsExceptionsFoundInOtherLogTabsAsARowNamingTheTab() { void bodyAttributesThePrimaryExceptionToItsOwnTabInsteadOfListingItTwice() { RuntimeException primary = new RuntimeException("boom"); // The crash is very often also logged (by the crashed process itself) in one of its own - // log tabs - that's the same exception, not another one, and must not be listed twice. + // log tabs - that's the same exception, not another one, and must not be listed twice. It's + // also the *real* trace (see the class javadoc on macOS reconstruction), so it must be used + // in preference to the exception object's own (possibly fake) trace. String combinedLog = "=== Terasology-game.log ===\n" + primary + "\n" + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"; @@ -130,18 +134,24 @@ void bodyAttributesThePrimaryExceptionToItsOwnTabInsteadOfListingItTwice() { CrashSummary summary = CrashSummary.extract(primary, combinedLog); String body = summary.buildBody(null); - assertTrue(body.contains("**Terasology-game.log**: `java.lang.RuntimeException: boom`"), - "Expected the primary exception's row attributed to the tab it was found in, got: " + body); - int rows = body.split("\n- \\*\\*", -1).length - 1; - assertEquals(1, rows, "Expected exactly one row - the primary exception must not also be listed as an \"other\" one: " + body); + assertTrue(body.contains("**Terasology-game.log**\n\n```\njava.lang.RuntimeException: boom\n" + + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n```"), + "Expected the primary exception's block attributed to the tab it was found in, using that tab's real trace, got: " + + body); + int blocks = body.split("\\*\\*Terasology-game\\.log\\*\\*", -1).length - 1; + assertEquals(1, blocks, "Expected exactly one block - the primary exception must not also be listed as an \"other\" one: " + + body); } @Test - void bodyAttributesThePrimaryExceptionToThisCrashWhenNotFoundInAnyTab() { - CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { + RuntimeException primary = new RuntimeException("boom"); + CrashSummary summary = CrashSummary.extract(primary, LOG_TEXT); String body = summary.buildBody(null); - assertTrue(body.contains("**this crash**: `java.lang.RuntimeException: boom`"), - "Expected a fallback label when the exception isn't found in any log tab, got: " + body); + assertTrue(body.contains("**this crash**\n\n```\njava.lang.RuntimeException: boom\n"), + "Expected a fallback label and the exception's own trace when it isn't found in any log tab, got: " + body); + assertTrue(body.contains(CrashSummaryTest.class.getName()), + "Expected this test's own stack frame in the fallback trace, got: " + body); } } From 93da6916f7ebd897141be5a18f51ba61e3699c3c Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 17:03:36 +0200 Subject: [PATCH 5/7] fix(reporter): include the 5 log lines logged right before each exception Requested after testing: knowing only the exception itself often isn't enough to understand what led up to it. Each exception block now starts with up to 5 lines of whatever was logged immediately before it in that tab, then the exception's own header and trace as before. The in-process fallback trace (see the previous commit's javadoc on macOS reconstruction) has no log text to pull context from, so it's unaffected. Context is not subject to the trace's own 15-line cap - a few lines of what led up to the crash shouldn't cost trace detail. Found and fixed a boundary bug while writing this: the substring for each log tab started right after "=== tab ===", before that line's own newline, so every tab's text began with a blank artifact line - counted as a real (empty) line of context. Tab boundaries now skip past that line terminator, so the 5 lines captured are always real log content. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/pages/CrashSummary.java | 49 +++++++++++++++++-- .../crashreporter/pages/CrashSummaryTest.java | 26 +++++++++- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index 04f03cd..efbf76d 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -38,6 +38,7 @@ public final class CrashSummary { private static final int MAX_MODULES_LISTED = 30; private static final int MAX_TITLE_MESSAGE_LENGTH = 80; private static final int MAX_EXCEPTIONS_LISTED = 10; + private static final int CONTEXT_LINES_BEFORE = 5; private static final String NO_TAB_LABEL = "this crash"; private static final Pattern ENGINE_VERSION_PATTERN = Pattern.compile("engineVersion=([^,\\]]*)"); @@ -103,9 +104,9 @@ private static List buildExceptionBlocks(Throwable exception, String com } // Not logged anywhere - fall back to the exception object's own trace. On macOS that trace // is a best-effort reconstruction (see the class javadoc) rather than the real crash site, - // but it's all that's available. + // but it's all that's available. There's no log text to pull leading context from either. if (primary == null) { - primary = new ExceptionEntry(null, primaryHeader, framesFromThrowable(exception)); + primary = new ExceptionEntry(null, primaryHeader, framesFromThrowable(exception), ""); } List blocks = new ArrayList<>(); @@ -125,7 +126,11 @@ private static List buildExceptionBlocks(Throwable exception, String com private static String formatBlock(ExceptionEntry entry) { String label = entry.tabName != null ? entry.tabName : NO_TAB_LABEL; String combined = entry.frames.isEmpty() ? entry.header : entry.header + "\n" + entry.frames; - return "**" + label + "**\n\n```\n" + truncateTrace(combined) + "\n```"; + String trace = truncateTrace(combined); + // Context isn't part of the trace itself, so it's not subject to truncateTrace()'s + // MAX_STACK_LINES cap - a few lines of what led up to the crash shouldn't cost trace detail. + String content = entry.context.isEmpty() ? trace : entry.context + "\n" + trace; + return "**" + label + "**\n\n```\n" + content + "\n```"; } private static String truncateTrace(String combined) { @@ -168,11 +173,13 @@ private static final class ExceptionEntry { private final String tabName; private final String header; private final String frames; + private final String context; - private ExceptionEntry(String tabName, String header, String frames) { + private ExceptionEntry(String tabName, String header, String frames, String context) { this.tabName = tabName; this.header = header; this.frames = frames; + this.context = context; } } @@ -192,7 +199,16 @@ private static List findAllExceptions(String combinedLogText) { break; } tabName = tabMatcher.group(1); + // tabMatcher.end() lands right after "===", before that line's own terminator - skip it + // too, so each tab's text starts at its real first content line instead of with a blank + // artifact line (which precedingLines() would otherwise count as logged context). tabStart = tabMatcher.end(); + if (tabStart < combinedLogText.length() && combinedLogText.charAt(tabStart) == '\r') { + tabStart++; + } + if (tabStart < combinedLogText.length() && combinedLogText.charAt(tabStart) == '\n') { + tabStart++; + } } // No "=== tab ===" headers at all - a single combined-log caller (e.g. a direct test) rather // than ErrorMessagePanel#getLog(); scan the whole text with no tab attribution. @@ -207,8 +223,31 @@ private static void collectExceptionHeaders(String tabText, String tabName, List while (matcher.find()) { String header = (matcher.group(1) + (matcher.group(2) != null ? matcher.group(2) : "")).trim(); String frames = stripTrailingWhitespace(matcher.group(3)); - found.add(new ExceptionEntry(tabName, header, frames)); + String context = precedingLines(tabText, matcher.start(), CONTEXT_LINES_BEFORE); + found.add(new ExceptionEntry(tabName, header, frames, context)); + } + } + + /** + * @return up to {@code maxLines} lines of whatever was logged right before {@code beforeIndex} in + * {@code text} - what led up to a crash is often as useful for diagnosing it as the trace + * itself, and it's only available here (the reporter's own {@link #exception} carries no + * log context of its own). + */ + private static String precedingLines(String text, int beforeIndex, int maxLines) { + String[] lines = text.substring(0, beforeIndex).split("\r?\n", -1); + int end = lines.length; + // A trailing empty element only ever means the substring ended in a newline - i.e. the line + // right before the match, not a real blank log line - so it isn't context to show. + if (end > 0 && lines[end - 1].isEmpty()) { + end--; + } + int start = Math.max(0, end - maxLines); + StringBuilder builder = new StringBuilder(); + for (int i = start; i < end; i++) { + builder.append(lines[i]).append('\n'); } + return stripTrailingWhitespace(builder.toString()); } private static String firstGroup(Pattern pattern, String text) { diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 0ac9bc0..730f09f 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -111,9 +111,12 @@ void bodyListsExceptionsFoundInOtherLogTabsWithTheirFullTraceNamingTheTab() { String body = summary.buildBody(null); assertTrue(body.contains("### Exceptions"), "Expected a single unified exceptions section, got: " + body); - assertTrue(body.contains("**Terasology-game.log**\n\n```\njava.lang.NullPointerException: world was null\n" + assertTrue(body.contains("**Terasology-game.log**\n\n```\n" + + "10:10:05.123 [main] ERROR o.t.e.core.TerasologyEngine - Uncaught exception in main loop\n" + + "java.lang.NullPointerException: world was null\n" + "\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n```"), - "Expected the other tab's exception as its own labeled block with the full trace, got: " + body); + "Expected the other tab's exception as its own labeled block, with the line logged right before it and the full " + + "trace, got: " + body); int exceptionsIndex = body.indexOf("### Exceptions"); int environmentIndex = body.indexOf("### Environment"); assertTrue(exceptionsIndex >= 0 && environmentIndex > exceptionsIndex, @@ -143,6 +146,25 @@ void bodyAttributesThePrimaryExceptionToItsOwnTabInsteadOfListingItTwice() { + body); } + @Test + void bodyIncludesOnlyTheLastFiveLinesLoggedBeforeTheException() { + StringBuilder combinedLog = new StringBuilder("=== Terasology-game.log ===\n"); + for (int i = 1; i <= 8; i++) { + combinedLog.append("log line ").append(i).append('\n'); + } + combinedLog.append("java.lang.NullPointerException: world was null\n") + .append("\tat org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:200)\n"); + + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), combinedLog.toString()); + String body = summary.buildBody(null); + + assertFalse(body.contains("log line 1\n") || body.contains("log line 2\n") || body.contains("log line 3\n"), + "Expected only the last 5 lines of context, not all 8, got: " + body); + assertTrue(body.contains("log line 4\nlog line 5\nlog line 6\nlog line 7\nlog line 8\n" + + "java.lang.NullPointerException: world was null"), + "Expected the last 5 lines directly before the exception's header, got: " + body); + } + @Test void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { RuntimeException primary = new RuntimeException("boom"); From 17e8b8771b7587800e47c79ef51c8ea199553e47 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 18:40:07 +0200 Subject: [PATCH 6/7] feat(reporter): pre-fill Terasology's real issue form instead of a custom body @BenjaminAmos noted "Report Issue" should use the repo's existing crash-bug-report template rather than a bespoke Markdown format, and suggested converting it to a GitHub issue form so individual fields could be pre-populated by ID via URL query (MovingBlocks/Terasology#5390 does that conversion). New GlobalProperties.KEY.REPORT_ISSUE_TEMPLATE: when a downstream app sets it (cr-terasology now does, to "crash-bug-report.yml"), FinalActionsPanel builds a `template=`+per-field-ID query via a new GitHubIssueLinkBuilder.build(baseUrl, template, title, fields) overload and CrashSummary.buildIssueFormFields(), landing the crash summary in that form's real "Terasology Version"/"Operating System"/"Java Version"/"What actually happened"/"Log details"/"Additional Infos" fields instead of overwriting the whole issue with a custom body. Apps without a configured template (cr-destsol, standalone cr-core) keep the existing generic buildTitle()/buildBody() title+body fallback unchanged - REPORT_ISSUE_TEMPLATE is engine-agnostic cr-core plumbing, but the field IDs it targets when set are inherently tied to whichever form the downstream app's own repo defines, so this can never be the unconditional default. Also reapplied GitHubIssueLinkBuilder's null-baseUrl guard here (this branch predates that fix, added directly on merge-train earlier this session) - build() must not silently produce a broken "null?title=..." link when REPORT_ISSUE_LINK isn't configured. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/GlobalProperties.java | 1 + .../crashreporter/pages/CrashSummary.java | 51 ++++++++++++++++++ .../pages/FinalActionsPanel.java | 14 ++++- .../pages/GitHubIssueLinkBuilder.java | 34 +++++++++++- .../crashreporter/pages/CrashSummaryTest.java | 53 +++++++++++++++++++ .../pages/GitHubIssueLinkBuilderTest.java | 42 +++++++++++++++ .../main/resources/crashreporter.properties | 1 + 7 files changed, 192 insertions(+), 4 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java index 7c7cade..a0f8c3a 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java @@ -18,6 +18,7 @@ public enum KEY { SUPPORT_FORUM_LINK, JOIN_DISCORD_LINK, REPORT_ISSUE_LINK, + REPORT_ISSUE_TEMPLATE, RES_BANNER_IMAGE, RES_SERVER_ICON, diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java index efbf76d..64192fb 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/CrashSummary.java @@ -7,7 +7,9 @@ import java.io.StringWriter; import java.net.URL; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -326,4 +328,53 @@ public String buildBody(URL pastebinLink) { return body.toString(); } + + /** + * @param pastebinLink the uploaded log link, or {@code null} if the user skipped upload + * @return field ID to value for the issue form named by + * {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_TEMPLATE} - only + * meaningful when a downstream app has configured one (see + * {@link GitHubIssueLinkBuilder#build(String, String, String, Map)}); the IDs here match + * Terasology's own {@code crash-bug-report.yml}. Fields with nothing extractable are + * omitted so the user's own blank field is left for them to fill in, rather than being + * pre-filled with something misleading like "unknown". + */ + public Map buildIssueFormFields(URL pastebinLink) { + Map fields = new LinkedHashMap<>(); + + if (!engineVersion.isEmpty()) { + String version = displayVersion.isEmpty() ? engineVersion : engineVersion + " (" + displayVersion + ")"; + fields.put("terasology_version", version); + } + fields.put("operating_system", System.getProperty("os.name") + " " + System.getProperty("os.version") + + " (" + System.getProperty("os.arch") + ")"); + fields.put("java_version", System.getProperty("java.version")); + + StringBuilder actual = new StringBuilder(); + for (String block : exceptionBlocks) { + actual.append(block).append("\n\n"); + } + if (moreExceptionsCount > 0) { + actual.append("... ").append(moreExceptionsCount).append(" more - see the full log\n"); + } + fields.put("actual_behavior", actual.toString().trim()); + + if (pastebinLink != null) { + fields.put("log_details", "[PasteBin](" + pastebinLink + ")"); + } + + if (!activeModules.isEmpty()) { + StringBuilder modules = new StringBuilder("Active modules:\n"); + int shown = Math.min(activeModules.size(), MAX_MODULES_LISTED); + for (int i = 0; i < shown; i++) { + modules.append("- ").append(activeModules.get(i)).append('\n'); + } + if (activeModules.size() > shown) { + modules.append("- ... ").append(activeModules.size() - shown).append(" more\n"); + } + fields.put("additional_context", modules.toString().trim()); + } + + return fields; + } } diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java index fcca439..4e33e9d 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/FinalActionsPanel.java @@ -97,8 +97,18 @@ public void actionPerformed(ActionEvent e) { @Override public void actionPerformed(ActionEvent e) { CrashSummary summary = CrashSummary.extract(exception, logTextSupplier.get()); - String link = GitHubIssueLinkBuilder.build(properties.get(KEY.REPORT_ISSUE_LINK), - summary.buildTitle(), summary.buildBody(uploadedFile.get())); + String baseUrl = properties.get(KEY.REPORT_ISSUE_LINK); + String template = properties.get(KEY.REPORT_ISSUE_TEMPLATE); + String link; + if (template != null && !template.isEmpty()) { + // The downstream app has its own issue *form* - land the summary in its real + // fields instead of overwriting the whole thing with a bespoke body. + link = GitHubIssueLinkBuilder.build(baseUrl, template, summary.buildTitle(), + summary.buildIssueFormFields(uploadedFile.get())); + } else { + link = GitHubIssueLinkBuilder.build(baseUrl, summary.buildTitle(), + summary.buildBody(uploadedFile.get())); + } openInBrowser(link); pageComplete = true; firePropertyChange("pageComplete", !pageComplete, pageComplete); diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java index bcbac99..cff644b 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java @@ -5,10 +5,15 @@ import java.io.UnsupportedEncodingException; import java.net.URLEncoder; +import java.util.Map; /** - * Builds a GitHub new-issue URL pre-filled via the {@code title}/{@code body} query parameters - * GitHub's issue-creation form accepts. + * Builds a GitHub new-issue URL, pre-filled either via the classic {@code title}/{@code body} query + * parameters (works against any repo, regardless of what issue templates it has - the safe default), + * or, when a downstream app has one, via an issue *form*'s own field IDs (see + * {@link org.terasology.crashreporter.GlobalProperties.KEY#REPORT_ISSUE_TEMPLATE}) - a + * {@code template=} query parameter plus one parameter per field ID, landing the crash summary in + * the repo's own real template instead of overwriting it with a bespoke body. */ public final class GitHubIssueLinkBuilder { @@ -16,10 +21,35 @@ private GitHubIssueLinkBuilder() { } public static String build(String baseUrl, String title, String body) { + if (baseUrl == null) { + return null; + } String query = "title=" + encode(title) + "&body=" + encode(body); return baseUrl + (baseUrl.contains("?") ? "&" : "?") + query; } + /** + * @param template the issue form's filename (e.g. {@code "crash-bug-report.yml"}), as it appears + * under {@code .github/ISSUE_TEMPLATE/} in the target repo + * @param fields field ID to value - entries with a {@code null}/empty value are omitted, leaving + * that field for the user to fill in themselves rather than pre-filling it blank + */ + public static String build(String baseUrl, String template, String title, Map fields) { + if (baseUrl == null) { + return null; + } + StringBuilder query = new StringBuilder("template=").append(encode(template)) + .append("&title=").append(encode(title)); + for (Map.Entry field : fields.entrySet()) { + String value = field.getValue(); + if (value == null || value.isEmpty()) { + continue; + } + query.append('&').append(field.getKey()).append('=').append(encode(value)); + } + return baseUrl + (baseUrl.contains("?") ? "&" : "?") + query; + } + private static String encode(String value) { try { return URLEncoder.encode(value, "UTF-8"); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java index 730f09f..afb982b 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/CrashSummaryTest.java @@ -7,9 +7,11 @@ import java.net.MalformedURLException; import java.net.URL; +import java.util.Map; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -176,4 +178,55 @@ void bodyFallsBackToTheExceptionsOwnTraceWhenNotFoundInAnyTab() { assertTrue(body.contains(CrashSummaryTest.class.getName()), "Expected this test's own stack frame in the fallback trace, got: " + body); } + + // buildIssueFormFields() feeds GitHubIssueLinkBuilder's template-based overload (see + // GitHubIssueLinkBuilderTest) - used instead of buildBody() when a downstream app has configured + // REPORT_ISSUE_TEMPLATE, landing the summary in that issue form's own fields. + @Test + void issueFormFieldsIncludeVersionOsAndTheExceptionBlocks() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + Map fields = summary.buildIssueFormFields(null); + + assertEquals("5.4.0-SNAPSHOT (Aeternum)", fields.get("terasology_version")); + assertTrue(fields.get("operating_system").contains(System.getProperty("os.name")), fields.get("operating_system")); + assertEquals(System.getProperty("java.version"), fields.get("java_version")); + assertTrue(fields.get("actual_behavior").contains("RuntimeException: boom"), fields.get("actual_behavior")); + } + + @Test + void issueFormFieldsOmitVersionWhenNotFoundInsteadOfSayingUnknown() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), "no relevant lines here"); + Map fields = summary.buildIssueFormFields(null); + + // Unlike buildBody()'s "unknown" fallback, an omitted field is left blank in the actual issue + // form for the user to fill in themselves - "unknown" pre-filled into a real form field would + // read as if the reporter deliberately couldn't tell, not as an untouched field. + assertNull(fields.get("terasology_version"), "Expected no terasology_version entry, got: " + fields); + } + + @Test + void issueFormFieldsOmitLogDetailsWhenUploadWasSkipped() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + Map fields = summary.buildIssueFormFields(null); + + assertNull(fields.get("log_details"), "Expected no log_details entry when nothing was uploaded, got: " + fields); + } + + @Test + void issueFormFieldsIncludeThePastebinLinkWhenUploaded() throws MalformedURLException { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + URL link = new URL("https://pastebin.com/abc123"); + + Map fields = summary.buildIssueFormFields(link); + + assertTrue(fields.get("log_details").contains("https://pastebin.com/abc123"), fields.get("log_details")); + } + + @Test + void issueFormFieldsIncludeActiveModulesUnderAdditionalContext() { + CrashSummary summary = CrashSummary.extract(new RuntimeException("boom"), LOG_TEXT); + Map fields = summary.buildIssueFormFields(null); + + assertTrue(fields.get("additional_context").contains("engine:5.4.0-SNAPSHOT"), fields.get("additional_context")); + } } diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java index ece4418..bbcb421 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java @@ -5,11 +5,24 @@ import org.junit.jupiter.api.Test; +import java.util.LinkedHashMap; +import java.util.Map; + import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; class GitHubIssueLinkBuilderTest { + @Test + void returnsNullWhenBaseUrlIsNotConfigured() { + // REPORT_ISSUE_LINK is only set by downstream apps (cr-terasology, cr-destsol, ...), not by + // cr-core's own defaults - build() must not silently produce a broken "null?title=..." link. + assertNull(GitHubIssueLinkBuilder.build(null, "t", "b")); + assertNull(GitHubIssueLinkBuilder.build(null, "template.yml", "t", new LinkedHashMap<>())); + } + @Test void appendsTitleAndBodyAsQueryParameters() { String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", @@ -34,4 +47,33 @@ void usesAmpersandWhenBaseUrlAlreadyHasAQueryString() { assertEquals("https://example.com/issues/new?template=bug&title=t&body=b", link); } + + @Test + void formBuildAppendsTemplateTitleAndEachFieldAsItsOwnQueryParameter() { + Map fields = new LinkedHashMap<>(); + fields.put("terasology_version", "5.4.0-SNAPSHOT"); + fields.put("operating_system", "Mac OS X"); + + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "crash-bug-report.yml", "Crash: NullPointerException", fields); + + assertTrue(link.contains("template=crash-bug-report.yml"), link); + assertTrue(link.contains("title=Crash%3A+NullPointerException"), link); + assertTrue(link.contains("terasology_version=5.4.0-SNAPSHOT"), link); + assertTrue(link.contains("operating_system=Mac+OS+X"), link); + } + + @Test + void formBuildOmitsFieldsWithNoValueInsteadOfPreFillingThemBlank() { + Map fields = new LinkedHashMap<>(); + fields.put("terasology_version", ""); + fields.put("java_version", null); + fields.put("operating_system", "Linux"); + + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "crash-bug-report.yml", "t", fields); + + assertFalse(link.contains("terasology_version="), link); + assertFalse(link.contains("java_version="), link); + assertTrue(link.contains("operating_system=Linux"), link); + } } diff --git a/cr-terasology/src/main/resources/crashreporter.properties b/cr-terasology/src/main/resources/crashreporter.properties index 47a7a74..45c4030 100644 --- a/cr-terasology/src/main/resources/crashreporter.properties +++ b/cr-terasology/src/main/resources/crashreporter.properties @@ -1,5 +1,6 @@ SUPPORT_FORUM_LINK=http://forum.terasology.org/forum/support.20/ REPORT_ISSUE_LINK=https://github.com/MovingBlocks/Terasology/issues/new +REPORT_ISSUE_TEMPLATE=crash-bug-report.yml JOIN_DISCORD_LINK=https://discord.gg/terasology RES_BANNER_IMAGE=icons/banner.jpg From fb909e2edb87c33566f433e09d5f34eba0abe0d9 Mon Sep 17 00:00:00 2001 From: soloturn Date: Wed, 26 Aug 2026 19:15:31 +0200 Subject: [PATCH 7/7] fix(reporter): cap the GitHub issue URL to GitHub's own byte limit GitHub rejects the whole URL past 8191 bytes (github/docs#5136). A crash with many exceptions/long traces blew past that even after CrashSummary's per-block caps. Now the builder budgets the whole URL, truncating (with a note) or dropping fields to fit, instead of sending an oversized link. Fixes #58. Co-Authored-By: soloturn --- .../pages/GitHubIssueLinkBuilder.java | 59 ++++++++++++++++--- .../pages/GitHubIssueLinkBuilderTest.java | 46 +++++++++++++++ 2 files changed, 97 insertions(+), 8 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java index cff644b..9c10bda 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilder.java @@ -17,6 +17,12 @@ */ public final class GitHubIssueLinkBuilder { + // GitHub rejects the whole URL past this. See github/docs#5136, crashreporter#58. + private static final int GITHUB_URL_BYTE_LIMIT = 8191; + // Safety margin. + private static final int URL_BYTE_BUDGET = GITHUB_URL_BYTE_LIMIT - 200; + private static final String TRUNCATED_SUFFIX = "\n... truncated, see the full log"; + private GitHubIssueLinkBuilder() { } @@ -24,30 +30,67 @@ public static String build(String baseUrl, String title, String body) { if (baseUrl == null) { return null; } - String query = "title=" + encode(title) + "&body=" + encode(body); - return baseUrl + (baseUrl.contains("?") ? "&" : "?") + query; + String separator = baseUrl.contains("?") ? "&" : "?"; + String encodedTitle = encode(title); + int budget = URL_BYTE_BUDGET - baseUrl.length() - separator.length() + - "title=".length() - encodedTitle.length() - "&body=".length(); + String encodedBody = fitToBudget(body, budget); + String query = "title=" + encodedTitle + "&body=" + (encodedBody != null ? encodedBody : ""); + return baseUrl + separator + query; } /** - * @param template the issue form's filename (e.g. {@code "crash-bug-report.yml"}), as it appears - * under {@code .github/ISSUE_TEMPLATE/} in the target repo - * @param fields field ID to value - entries with a {@code null}/empty value are omitted, leaving - * that field for the user to fill in themselves rather than pre-filling it blank + * @param template issue form filename under {@code .github/ISSUE_TEMPLATE/} + * @param fields field ID to value. Null/empty value: field omitted. Too long: truncated or + * dropped, whichever fits. */ public static String build(String baseUrl, String template, String title, Map fields) { if (baseUrl == null) { return null; } + String separator = baseUrl.contains("?") ? "&" : "?"; StringBuilder query = new StringBuilder("template=").append(encode(template)) .append("&title=").append(encode(title)); + int budget = URL_BYTE_BUDGET - baseUrl.length() - separator.length() - query.length(); + for (Map.Entry field : fields.entrySet()) { String value = field.getValue(); if (value == null || value.isEmpty()) { continue; } - query.append('&').append(field.getKey()).append('=').append(encode(value)); + String key = field.getKey(); + int overhead = key.length() + 2; // '&' + key + '=' + String encoded = fitToBudget(value, budget - overhead); + if (encoded == null) { + continue; + } + query.append('&').append(key).append('=').append(encoded); + budget -= overhead + encoded.length(); + } + return baseUrl + separator + query; + } + + /** + * URL-encodes {@code value}, truncating with {@link #TRUNCATED_SUFFIX} to fit {@code maxBytes}. + * Null if even the suffix doesn't fit. Truncates the raw text first, then encodes - never + * splits mid-escape. + */ + private static String fitToBudget(String value, int maxBytes) { + String encoded = encode(value); + if (encoded.length() <= maxBytes) { + return encoded; + } + String suffix = encode(TRUNCATED_SUFFIX); + if (suffix.length() > maxBytes) { + return null; } - return baseUrl + (baseUrl.contains("?") ? "&" : "?") + query; + String truncated = value; + String withSuffix; + do { + truncated = truncated.substring(0, truncated.length() - 1); + withSuffix = encode(truncated) + suffix; + } while (!truncated.isEmpty() && withSuffix.length() > maxBytes); + return truncated.isEmpty() ? null : withSuffix; } private static String encode(String value) { diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java index bbcb421..88d513b 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/GitHubIssueLinkBuilderTest.java @@ -76,4 +76,50 @@ void formBuildOmitsFieldsWithNoValueInsteadOfPreFillingThemBlank() { assertFalse(link.contains("java_version="), link); assertTrue(link.contains("operating_system=Linux"), link); } + + @Test + void bodyBuildStaysUnderGitHubsUrlByteLimit() { + // GitHub rejects the whole thing past 8191 bytes. Must truncate. + String hugeBody = "x".repeat(50_000); + + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "Crash: NullPointerException", hugeBody); + + assertTrue(link.length() < 8191, "link was " + link.length() + " bytes: " + link); + assertTrue(link.contains("truncated"), link); + } + + @Test + void formBuildStaysUnderGitHubsUrlByteLimit() { + Map fields = new LinkedHashMap<>(); + fields.put("terasology_version", "5.4.0-SNAPSHOT"); + fields.put("operating_system", "Mac OS X"); + fields.put("actual_behavior", "x".repeat(50_000)); + fields.put("additional_context", "y".repeat(50_000)); + + String link = GitHubIssueLinkBuilder.build("https://github.com/MovingBlocks/Terasology/issues/new", + "crash-bug-report.yml", "Crash: NullPointerException", fields); + + assertTrue(link.length() < 8191, "link was " + link.length() + " bytes: " + link); + // Early fields stay full, only the overflowing tail gets trimmed. + assertTrue(link.contains("terasology_version=5.4.0-SNAPSHOT"), link); + assertTrue(link.contains("operating_system=Mac+OS+X"), link); + } + + @Test + void truncationNeverSplitsAPercentEscape() { + Map fields = new LinkedHashMap<>(); + // Every char is a 3-byte "%XX" escape, so a mid-escape cut would hide here. + fields.put("actual_behavior", "&".repeat(50_000)); + + String link = GitHubIssueLinkBuilder.build("https://example.com/issues/new", "t.yml", "t", fields); + + int valueStart = link.indexOf("actual_behavior=") + "actual_behavior=".length(); + String value = link.substring(valueStart); + for (int i = 0; i < value.length(); i++) { + if (value.charAt(i) == '%') { + assertTrue(i + 2 < value.length(), "truncated escape at end of value: " + value); + } + } + } }