Skip to content

fix(logger): #630 escape Spectre markup on all console paths + non-throwing render fallback #655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jsboige merged 1 commit into from
Jul 3, 2026
Merged

fix(logger): #630 escape Spectre markup on all console paths + non-throwing render fallback #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions Generation/Converters/Argumentum.AssetConverter.Tests/Utility/LoggerMarkupSafetyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
using System;
using System.IO;
using Argumentum.AssetConverter;
using FluentAssertions;
using Spectre.Console;
using Xunit;

namespace Argumentum.AssetConverter.Tests.Utility
{
/// <summary>
/// Regression tests for issue #630: a message containing square brackets (e.g. the
/// <c>[HARVEST-FAILURE]</c> marker emitted by the #614 per-set resilience path) was fed
/// unescaped to Spectre.Console markup rendering. The StyleParser then threw
/// <c>InvalidOperationException("Could not find color or style 'HARVEST-FAILURE'")</c>
/// from inside the resilience catch block, killing the whole run instead of degrading
/// gracefully.
///
/// Two guarantees are pinned here:
/// (1) bracketed messages render literally (Markup.Escape on every console path), and
/// (2) Logger.Log never throws on any rendering failure (plain-text fallback), because
/// #614 calls it from catch paths where a throw is fatal to the run.
/// </summary>
public class LoggerMarkupSafetyTests
{
/// <summary>
/// Routes AnsiConsole to a plain StringWriter for the duration of a test so output can
/// be asserted, then restores the previous console.
/// </summary>
private static string CaptureConsole(Action action)
{
var writer = new StringWriter();
var console = AnsiConsole.Create(new AnsiConsoleSettings
{
Ansi = AnsiSupport.No,
ColorSystem = ColorSystemSupport.NoColors,
Interactive = InteractionSupport.No,
Out = new AnsiConsoleOutput(writer),
});
var previous = AnsiConsole.Console;
AnsiConsole.Console = console;
try
{
action();
}
finally
{
AnsiConsole.Console = previous;
}
return writer.ToString();
}

[Fact]
public void Log_Problem_WithHarvestFailureMarker_DoesNotThrow_AndRendersLiterally()
{
// Exact shape emitted by HarvestManager's #614 resilience path.
var message = "[HARVEST-FAILURE] Card set 'FallaciesTarot' / 'fr' failed and was skipped: boom";

string output = null;
var act = () => { output = CaptureConsole(() => Logger.Log(message, MessageType.Problem)); };

act.Should().NotThrow("a bracketed failure marker must not be parsed as Spectre style markup (#630)");
output.Should().Contain("[HARVEST-FAILURE]", "the marker must survive rendering literally for log greppability");
}

[Theory]
[InlineData(MessageType.Title)]
[InlineData(MessageType.Problem)]
[InlineData(MessageType.Instructions)]
[InlineData(MessageType.Explanations)]
[InlineData(MessageType.Warning)]
[InlineData(MessageType.Success)]
[InlineData(MessageType.Info)]
public void Log_AnyMessageType_WithBracketedContent_DoesNotThrow(MessageType messageType)
{
// Brackets show up in real messages: failure markers, file paths, exception text,
// Mustache/template fragments quoted in diagnostics.
var message = "path [C:\\x] marker [HARVEST-FAILURE] template {{field}} style [bold red]";

var act = () => CaptureConsole(() => Logger.Log(message, messageType));

act.Should().NotThrow($"no console rendering path may propagate for {messageType} (#630)");
}

[Fact]
public void Log_InvalidMessageType_StillThrowsArgumentOutOfRange()
{
// The render-failure fallback must not swallow the guard clause for invalid enums.
var act = () => CaptureConsole(() => Logger.Log("x", (MessageType)999));

act.Should().Throw<ArgumentOutOfRangeException>();
}

[Fact]
public void LogException_WithBracketsInExceptionMessage_DoesNotThrow()
{
var ex = new InvalidOperationException("outer [HARVEST-FAILURE]",
new IOException("inner [bold red] not-a-style"));

var act = () => CaptureConsole(() => Logger.LogException(ex));

act.Should().NotThrow("exception messages routinely contain brackets and are rendered on failure paths");
}
}
}
90 changes: 50 additions & 40 deletions Generation/Converters/Argumentum.AssetConverter/Logger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ static Logger()
}
catch (Exception ex)
{
AnsiConsole.MarkupLine($"[bold red]Error during logger initialization: {ex.Message}[/]");
AnsiConsole.MarkupLine($"[bold red]Error during logger initialization: {Markup.Escape(ex.Message)}[/]");
}
}

Expand Down Expand Up @@ -90,47 +90,57 @@ public static void Log(string message, MessageType messageType = MessageType.Inf
{
// Log initialization errors to the console to ensure they are visible
// as the file logger itself may be the source of the problem.
AnsiConsole.MarkupLine($"[bold red]Failed to write to log file '{LogFile}'. Exception: {ex.Message}[/]");
AnsiConsole.MarkupLine($"[bold red]Failed to write to log file '{Markup.Escape(LogFile)}'. Exception: {Markup.Escape(ex.Message)}[/]");
}

switch (messageType)
try
{
switch (messageType)
{
case MessageType.Info:
case MessageType.Success:
case MessageType.Warning:
var markup = messageType == MessageType.Info ? "dim" : messageType == MessageType.Warning ? "sandybrown" : "green3";
if (LogInfo || messageType != MessageType.Info)
{
AnsiConsole.MarkupLine($"{Stopwatch.Elapsed}: [{markup}]{Markup.Escape(message)}[/]");
}
break;
case MessageType.Title:
AnsiConsole.WriteLine();
AnsiConsole.WriteLine();
var rule = new Rule($"[bold]{Markup.Escape(message)}[/]");
AnsiConsole.Write(rule);
AnsiConsole.WriteLine();
break;
case MessageType.Problem:
AnsiConsole.WriteLine();
AnsiConsole.MarkupLine($"[bold red]{Markup.Escape(message)}[/]");
AnsiConsole.WriteLine();
break;
case MessageType.Instructions:
case MessageType.Explanations:
AnsiConsole.WriteLine();
var header = messageType.ToString();
var color = messageType==MessageType.Instructions ? Color.Yellow : Color.PaleGreen1;
AnsiConsole.Write(
new Panel(Markup.Escape(message))
.Header(header)
.Collapse()
.RoundedBorder()
.BorderColor(color));
AnsiConsole.WriteLine();
break;
default:
throw new ArgumentOutOfRangeException(nameof(messageType), messageType, null);
}
}
catch (Exception renderEx) when (renderEx is not ArgumentOutOfRangeException)
{
case MessageType.Info:
case MessageType.Success:
case MessageType.Warning:
var markup = messageType == MessageType.Info ? "dim" : messageType == MessageType.Warning ? "sandybrown" : "green3";
if (LogInfo || messageType != MessageType.Info)
{
AnsiConsole.MarkupLine($"{Stopwatch.Elapsed}: [{markup}]{Markup.Escape(message)}[/]");
}
break;
case MessageType.Title:
AnsiConsole.WriteLine();
AnsiConsole.WriteLine();
var rule = new Rule($"[bold]{message}[/]");
AnsiConsole.Write(rule);
AnsiConsole.WriteLine();
break;
case MessageType.Problem:
AnsiConsole.WriteLine();
AnsiConsole.MarkupLine($"[bold red]{message}[/]");
AnsiConsole.WriteLine();
break;
case MessageType.Instructions:
case MessageType.Explanations:
AnsiConsole.WriteLine();
var header = messageType.ToString();
var color = messageType==MessageType.Instructions ? Color.Yellow : Color.PaleGreen1;
AnsiConsole.Write(
new Panel(message)
.Header(header)
.Collapse()
.RoundedBorder()
.BorderColor(color));
AnsiConsole.WriteLine();
break;
default:
throw new ArgumentOutOfRangeException(nameof(messageType), messageType, null);
// Console rendering must never propagate (#630): the #614 per-set resilience calls
// Log() from its catch path, so a Spectre StyleParser failure here would kill the
// whole run instead of degrading gracefully. Fall back to plain (markup-free) output.
AnsiConsole.WriteLine($"{Stopwatch.Elapsed}: [{messageType}] {message} (console render failed: {renderEx.Message})");
}


Expand Down Expand Up @@ -193,7 +203,7 @@ public static void LogException(Exception ex)
}
catch (Exception logEx)
{
AnsiConsole.MarkupLine($"[bold red]Failed to write exception to log file '{LogFile}'. Exception: {logEx.Message}[/]");
AnsiConsole.MarkupLine($"[bold red]Failed to write exception to log file '{Markup.Escape(LogFile)}'. Exception: {Markup.Escape(logEx.Message)}[/]");
}
}

Expand Down
Loading