Fix Spectre markup crashes and anchor mapping paths to projectPath - #197
Merged
Merged
Conversation
added 6 commits
September 9, 2026 11:04
A NuGet version range ("[2.9.0,3.0.0)") or an exact pin ("[2.1.4]") starts with
'[', which the console sink reads as an opening markup tag. The version was
interpolated unescaped, so the log call itself threw and aborted the run for
that area with "An error occurred while writing to logger(s). (Encountered
malformed markup tag at position 81.)".
Only the version needed escaping; a package id cannot contain '['.
The existing DirectoryBuildPropsHelperPackageVersionTests already covers this
method, but every version it uses is bracket-free and it never renders the
message, so the path was green throughout. The markup-safety assertion lives in
its own class rather than as extra cases there: that class asserts the message
contains the raw version, which escaping necessarily breaks, so folding the two
together would mean loosening a check that is currently exact.
Same defect class as the package-version skip log: log messages are rendered as Spectre markup, so a '[' in interpolated text is read as an opening tag and the log call throws. Sweeps the 14 sites that interpolated exception text unescaped; the four that already used Markup.Escape are unchanged and were the precedent. These are not theoretical. AtcApiNugetClientHelper's System.Text.Json failures carry paths like "$.items[0]", and GetMessage() concatenates inner exceptions, so that site has a live path to "Could not find color or style '0'". File paths and MSBuild diagnostics reach the ProjectHelper sites the same way. Atc.CodingRules gains a direct Spectre.Console reference for this. It emits no markup of its own, but it logs into the same markup-rendered sink as everything else, so it was already bound by that contract implicitly - the dependency only makes it explicit. Pinned to 0.57.2, the version already resolving there transitively via Atc.Console.Spectre, so nothing new reaches the output graph. No tests: these are catch-block diagnostics with no seam to trigger, and a test would end up asserting Markup.Escape's own behaviour. The regression test for the reproducible crash rides with the previous commit.
…directory
Relative mapping paths were only resolved against the project when the process
current directory happened to sit inside a folder named
"Atc.CodingRules.Updater.CLI":
var di = new DirectoryInfo(orgPath);
if (di.FullName.Contains("Atc.CodingRules.Updater.CLI", ...))
DirectoryInfo resolves a relative path against the current directory, so that
guard reads as leftover from running the tool out of its own build output.
Anywhere else it is false, the path is returned unresolved, and ProjectHelper
later turns it into a DirectoryInfo - against the current directory again.
The effect is that "atc-coding-rules-updater run -p <repo>" from any directory
other than the repo itself writes .editorconfig and Directory.Build.props into
<cwd>/sample, <cwd>/src and <cwd>/test, and bumps NuGet versions there, while
reporting the target repo's areas. Reproduced from an empty directory: sample,
src and test were reported as "would create" while the target repo already had
them; after the fix all three report "nothing to update".
Three separate shapes were wrong, and the new tests cover each:
sample, ./sample, src/nested left unresolved -> resolved against cwd
\server\share\src leading separator stripped, then combined onto
the project path, silently retargeting a UNC
share into the project tree
\sample already correct, and still is
The guard is IsPathFullyQualified rather than IsPathRooted on purpose: on
Windows IsPathRooted also accepts the drive-relative "\sample", which is one of
the forms that does need anchoring.
CreateDefaultOptions is the second half. It discovers bare folder names under
the project path and never called ResolvePaths, so the no-options-file run was
broken the same way regardless of the guard above.
The common invocation - cd into the repo, then run - is unaffected, because
there the current directory already is the project path. Verified: identical
output before and after.
Two log messages interpolate values that come straight from the command line, so
a '[' in either is read as an opening markup tag and throws out of the log call:
--includeProviders / --excludeProviders echoed back when a name is unknown
--organizationName / --repositoryName echoed back when written into the
props file
Values a user types are the likeliest place for a stray bracket to arrive, and
the failure lands on the path that was already reporting a mistake.
Stopping here rather than escaping every interpolation. Package ids, MSBuild
property names and elapsed times cannot carry a bracket. File paths and feed
version strings can in principle, but escaping every path in the codebase is a
large diff against a near-zero probability. HttpClientHelper's "[link={url}]"
is left alone deliberately: that is intentional markup with an interpolated tag
attribute, where a naive escape would break the link rather than protect it.
CI runs ubuntu, macos and windows. The tests added with the path-anchoring fix
were written against Windows path semantics and failed all five non-UNC cases on
the Unix legs.
Two distinct causes:
The project path was built as Path.Combine("D:", "Code", "MyRepo"). On Unix that
is not rooted, so DirectoryInfo.FullName resolved it against the current
directory and every expectation drifted. It is now built from Path.GetTempPath(),
which is absolute everywhere. The absolute-path case had the same problem:
"D:\Elsewhere\test" is a relative path on Unix, so it was anchored rather than
left alone.
Two inputs are genuinely platform-divergent rather than badly written, and are
now separate tests that skip off Windows:
/sample on Windows drive-relative, so it means "project root";
on Unix an absolute path, correctly left alone
\server\share\src a UNC share only exists on Windows; the same string is
an ordinary relative path elsewhere
Resolution itself now accepts both separators on every platform and normalizes
to the local one. An options file is checked in and shared, so a path written on
Windows as "src\nested" has to keep working for someone running the tool on
Linux or macOS - previously only "/" was normalized, leaving "src\nested" as a
single literal filename there.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--projectPathbeing ignored for mapping pathsChanges
🐛 Fixes
[2.9.0,3.0.0)) aborting a run[2.1.4]) failing the same way--projectPath💥 Breaking Changes
♻️ Refactoring
IsPathFullyQualifiedPath.DirectorySeparatorCharinstead of hardcoded\🔧 Configuration
Spectre.ConsoletoAtc.CodingRules, pinned to 0.57.2Spectre.ConsoletoAtc.CodingRules.AnalyzerProviders📦 Dependencies
Breaking Changes
--projectPathrather than the current working directory.inside a folder named
Atc.CodingRules.Updater.CLI.cd <repo> && atc-coding-rules-updater .is unaffected,since there the current directory already is the project path.
invoking with
-p <repo>from a different directory, will nowwrite to the target repo instead of the current one.