feat(spectre)!: Ploch.CommandLine.Spectre initial release-ready implementation - #11
Merged
Conversation
|
Skipping CodeAnt AI review — this PR changes more than 100 files, which usually means a migration, codemod, or vendored drop. Line-level review on diffs this large produces duplicate findings on the same rewrite pattern and drowns out anything that actually matters. If you still want a review, comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Bito Automatic Review Skipped - Large PR |
|
Important Review skippedToo many files! This PR contains 400 files, which is 300 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (400)
You can disable this status message by setting the Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cbf6cc142
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…warnings Finishing-touches pass over the Spectre.Console initial implementation. Build fixes (branch did not compile): - Stale IExceptionHandler<T> type arguments in the FluentValidation test commands, and a CommandContext? / CommandContext nullability mismatch between AppCommand and its AsyncAppCommand sibling. Both base classes guarantee a non-null context via NotNull(), so the nullable annotation was incorrect. Runtime defects identified by external review (Grok 4.6, Kimi K3): - ConvertibleMessageFormatter was registered in DI but threw NotImplementedException, crashing any output write of an IConvertible value (int, DateTime, bool). - CommandInfoFactory.CreateFromType threw NotImplementedException on its primary path; it now maps CommandAttribute metadata onto CommandInfo. - CancellationToken was accepted throughout but never honoured. It is now forwarded from Execute/ExecuteAsync into DoExecute/DoExecuteAsync and on to IUseCase.ExecuteAsync. - AnsiConsoleMarkupOutput.Write emitted every writer-handled message twice. - DefaultExceptionHandler printed Win32 exceptions twice and routed them through a markup-parsing path, so exception text containing '[' could fail inside the exception handler itself. - The startup banner rendered the FigletText application name twice. - The Release configuration set TreatWarningsAsErrors=false, dropping this repository's zero-warning bar in the published build. Quality: - 78 XML documentation warnings resolved across 26 files. Several existing doc comments described intended rather than actual behaviour and were corrected. - Remaining analyser warnings fixed rather than suppressed; removed a #pragma warning disable CS1591 and a block of commented-out code. - .editorconfig: dotnet_diagnostic.IDE0002.severity was "true:error", which is not a valid severity value and disabled the rule across all six projects. - qodana.yaml targeted Ploch.Common.sln, a different repository's solution, which was the cause of the failing Qodana check. Build is clean (0 errors, 0 warnings) and all tests pass. Known gap: the library has 9 tests covering 3 of 63 public types, and the repository has no coverage tooling. Tracked separately. BREAKING CHANGE: AppCommand<TSettings>.DoExecute and AsyncAppCommand<TSettings>.DoExecuteAsync now accept a CancellationToken parameter; implementations must update their signatures. IMessageFormatterProcessor.WriteMessage now returns bool, indicating whether a registered writer handled the message. Refs: #3
…I solution path Resolves the three unresolved review threads raised on PR #11. - AppBuilder: the single-argument AddJsonFile overload made appsettings.json mandatory, overriding the optional behaviour Host.CreateDefaultBuilder already provides. Consumers without an appsettings.json got a file-not-found exception when the host was built, even when their command needs no JSON configuration. Now registered as optional with reloadOnChange. - EnvironmentSettingsLoader: PauseBeforeExit defaulted to true when DEV_RUNTIME_CONSOLE_EXIT_PAUSE was unset, so CommandAppExecutor.RunAsync called Console.ReadLine() after every command. An ordinary interactive invocation appeared to hang until Enter was pressed. This is a development-oriented setting and now defaults to false. - build-dotnet.yml: the build action was passed ./Ploch.Common.sln, another repository's solution, which does not exist here. Points at ./Ploch.CommandLine.Spectre.slnx instead. Note the workflow still filters on branch 'master' while this repository's default branch is 'main', so it does not currently trigger; raised separately. Build remains clean (0 errors, 0 warnings) and all tests pass. Refs: #3
This was referenced Aug 19, 2026
Contributor
Author
Quality pass complete — summaryAutomated finishing-touches pass on this branch. Full detail is in the updated PR description. Build: was 3 errors + 116 warnings → now 0 errors, 0 warnings. All tests pass. Seven runtime defects fixed (found by external review, all pre-existing): two public members that threw 78 XML documentation warnings resolved across 26 files. All analyser warnings fixed rather than suppressed — a Two config bugs corrected: All three review threads addressed and resolved.
|
Resolves a conflict in qodana.yaml: main added `profile` and `include` (CheckDependencyLicenses) while this branch corrected `dotnet.solution` from Ploch.Common.sln — another repository's solution — to Ploch.CommandLine.Spectre.slnx and scoped `exclude` to tests/**. Both sides are additive and are combined. Refs: #3
|
Bito Automatic Review Skipped - Large PR |
…w findings Addresses #12 and #14, and the tooling half of #13. CI (#12) — this repository's CI did not run at all: - build-dotnet.yml filtered on branch 'master' while the default branch is 'main', so Build, tests and SonarCloud never executed on any pull request. - Removed code_quality.yml, a stale duplicate of qodana_code_quality.yml whose pull_request and push triggers were commented out. The version already on main has correct triggers. - Bumped actions/checkout and actions/upload-artifact from v3 to v4; upload-artifact@v3 is retired and now fails outright. - Gated the GitHub Pages deploy and all three NuGet publish steps to pushes on main. Previously every pull request build ran `dotnet nuget push`, publishing packages from unmerged branches. Coverage tooling (#13): - Injected coverlet.msbuild into every test project via Directory.Build.props, following the ploch-common pattern. Without it the CI coverage flag was a silent no-op and the Codacy coverage step had no report to upload. - Scoped collection to Ploch.CommandLine.* so the figure describes this repository rather than referenced sibling repos. Review findings (#14): - EnvironmentSettings.Current: check-then-act lazy init replaced with a synchronised one; Initialize now throws if Current has already been materialised instead of silently doing nothing; added Reset for test isolation. - EnvironmentSettingsLoader: removed the unreachable `result.Keys is null` guard, switched to an ordinal-ignore-case dictionary and indexer assignment so environment names differing only in case no longer throw, and filtered to the DEV_RUNTIME prefix the property name and docs describe. The full environment block, which routinely carries secrets, is no longer retained. - CommandAppExecutor: Run now honours PauseBeforeExit identically to RunAsync. - AppCommand/AsyncAppCommand: OperationCanceledException is no longer reported to the exception handler as a generic failure; it returns the new ExitCode.Cancelled (130, the conventional SIGINT code). - Serilog: the error log sink sat outside its filtered sub-logger, so the "errors" file received every event. AddSerilog registered Serilog twice and the second registration dropped the output template. Also removed a duplicated Enrich.FromLogContext, a second console sink that doubled every log line, a stale CS8604 pragma whose justification referenced a parameter the call no longer has, and replaced duplicated literals with the existing constants. - AppServicesBundle: dropped a duplicate AnsiConsole.Console singleton, an unused keyed TokensArgumentsProcessor registration, and AddConsole() alongside the Serilog console sink. - CommandSettingsPropertyTypeProcessor: Properties is cleared per invocation; it previously accumulated and would throw on a repeated property name. - ConsoleAppInfo: removed the parallel SysColor properties and the FromSysColor extension - three properties plus an extension method that added no capability and had no callers. - AnsiConsoleMarkupOutput: removed WriteMarkupLineInterpolated, which was absent from IOutput and silently discarded any non-FormattableString message. - CommandInfo: dropped the redundant IEqualityOperators interface. - Documentation corrected: the Validate message now says "null or empty" to match the check, and the token docs state that {date}/{datetime} are UTC. Build is clean (0 errors, 0 warnings) and all tests pass. BREAKING CHANGE: ConsoleAppInfo.AppNameColorSys, AppNameInfoColorSys and AppDescriptionColorSys are removed, along with the ConsoleAppInfoExtensions FromSysColor extension method. Use the Spectre Color properties instead. AnsiConsoleMarkupOutput.WriteMarkupLineInterpolated is removed; use MarkupLineInterpolated. EnvironmentSettings.Initialize now throws InvalidOperationException if called after Current has been read. EnvironmentSettings.DevRuntimeVariables now contains only DEV_RUNTIME-prefixed variables rather than the entire environment block. Refs: #3
Grows the Ploch.CommandLine.Spectre suite from 8 tests to 74 and raises line
coverage on the library from 3.33% to 42.25%.
Regression cover for the defects fixed earlier on this branch — each of these
would have caught the corresponding bug before it shipped:
- ConvertibleMessageFormatterTests: the formatter is registered in DI for every
IConvertible, and previously threw NotImplementedException, so writing an int
crashed the application.
- MessageFormatterProcessorTests: WriteMessage now reports whether a writer
handled the message, which is what stops the output path writing twice.
- OutputServicesBundleTests: resolves the bundle through a real ServiceProvider
and exercises the formatter pipeline end to end. This is the test that covers
both consumer-visible output defects at once.
- CommandInfoFactory tests: the attribute-carrying path — the entire purpose of
the factory — previously threw and the existing test file contained helper
types but no test methods at all.
- AppCommandTests / AsyncAppCommandTests: the cancellation token reaches the
implementation, cancellation returns ExitCode.Cancelled instead of being
reported to the exception handler as a fault, and exceptions still route to
the handler.
New behavioural cover:
- EnvironmentSettingsTests: lazy initialisation loads exactly once under
concurrent access, Initialize throws once Current has been materialised, and
Reset restores a clean slate.
- EnvironmentSettingsLoaderTests: only DEV_RUNTIME-prefixed variables are
captured, prefix matching and lookup are case-insensitive, and PauseBeforeExit
defaults to false.
- TokensArgumentsProcessorTests: {date}/{datetime} substitution, case-insensitive
token matching, path-safe values, untagged properties left alone, null values
tolerated, and repeated invocations on one instance.
- CommandSettingsPropertyTypeProcessorTests: required-attribute filtering and the
per-invocation reset.
- ConsoleAppInfoExtensionsTests: banner validation.
The validation test found a real gap: Validate used IsNullOrEmpty, so a
whitespace-only application name passed and then rendered a blank FigletText
banner. It now rejects whitespace, and the exception message describes all three
rejected states rather than only "null".
VSTHRD200 is disabled for test projects in tests/.editorconfig with a documented
rationale: the repository mandates `<TestedMember>_should_<behaviour>` test names,
which cannot also carry the "Async" suffix the rule requires.
Build is clean (0 errors, 0 warnings) and all 75 tests pass.
Refs: #3
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Bito Automatic Review Skipped - Large PR |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 399 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
.cursor/rules/naming.mdc:8
- This always-applied rule conflicts with the C# codebase's PascalCase public members, such as
AppBuilder.Create,ConfigureCommandApp, andConsoleAppInfo.AppNameColor. Cursor will be instructed to generate names that violate the repository's actual convention and analyzer expectations.
.claude/rules/naming.md:3 - This repository is C#, where the established public-member convention is PascalCase (
AppBuilder.Create,ConfigureCommandApp, andConsoleAppInfo.AppNameColor). Declaring camelCase for methods and properties will make Claude generate code that conflicts with the existing API and analyzers. Scope the rule by language and use PascalCase for C# members.
.cursor/skills/winui3-controls-layouts/SKILL.md:3 - This PR adds an entire WinUI-specific skill set under
.cursor/skills/winui3-*, but the repository contains a console library and sample with no WinUI project or dependency. These unrelated files expand an already oversized release PR and leave contributor guidance for a technology that cannot be used here. Remove the WinUI skill files from this change, consistent with the PR's cleanup of unrelated marketplace content.
.cursor/mcp.json:5
- This commits an API credential in plaintext. Anyone with repository access can reuse it, and removing the file later will not remove it from history. Remove this machine-specific config, ignore it or commit a credential-free example, and revoke/rotate the exposed key.
BuildFullLogPath combined logPath with a file name built from logName, a public parameter of AddSerilog. Path.Combine discards everything before a rooted later segment, so logName: "C:\app" produced "C:\app.log" and the configured logPath was silently dropped - the library ignoring the very parameter documented to control where logs are written. Confirmed by mutation rather than inference: reverting this line to Path.Combine makes the new test fail with the log file actually present at the rooted location, so Serilog really did write outside the configured directory. Path.Join concatenates unconditionally, so the file stays under logPath. For an ordinary logName the two produce an identical string, verified against .NET, so nothing changes for callers passing a bare name - only the case that was already violating the documented contract behaves differently now. The six Path.Combine calls in the Serilog tests are converted as well. Those were genuine false positives - the second segment is Path.GetTempPath(), a Guid or a literal, none of which can be rooted - but the analyser re-fires on every push and re-blocks the merge each cycle, so removing the construct settles the class instead of re-arguing it. Refs: #48
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 399 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.cursor/mcp.json:5
- This commits an apparent live ContextStream API key to the public repository. Anyone can reuse it with the workspace/project identifiers in this file; remove it from the tracked config, rotate/revoke it immediately, and load the replacement from a local secret or environment variable. Removing the line alone is insufficient because the value remains in Git history.
ConfigGetCommand and ConfigSetCommand had regression tests for their disclosure paths; ConfigShowCommand did not, so its recursive redaction could have regressed to printing a nested secret without any test failing. Both halves of its policy are now covered - the section allow-list and the per-leaf redaction - plus a control proving the redaction is driven by the key rather than applied to every value. Each is mutation-verified: replacing the IsSensitive check with `false`, and swapping the allow-list for configuration.GetChildren(), each fail exactly one test. Testing this command needed a different approach from its siblings. They render strings, so mocking MarkupLineInterpolated is enough; this one builds a Tree and hands it over, so the renderable is captured and rendered to plain text through Spectre's own console. That is deliberate rather than incidental: asserting on what a user would actually see, and without adding a Spectre.Console.Testing dependency to a sample that consumers copy. Worth recording, because the first version of these tests passed while verifying nothing: `output.Write(tree)` binds to the generic Write<TMessage> overload, not Write(IRenderable) - the generic is an exact match for Tree while the IRenderable overload needs a conversion. Mocking IRenderable captured nothing, and the assertions then ran against an empty string. Also converts the last Path.Combine, which the previous commit's own new test had introduced. Refs: #48
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 400 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.cursor/skills/winui3-mrploch-app/SKILL.md:3
- The release-readiness notes say the unrelated WinUI/agent content was removed, but this file and the other 14 newly added
.cursor/skills/winui3-*files are still present. They are unrelated to the Spectre CLI library, sample, documentation, or release path and continue to inflate an already review-limit-sized PR. Remove this subtree or move it to a separately described tooling change.
The previous commit stopped a rooted logName from replacing logPath, but
Path.Join preserves ".." segments - so logName: "../outside" still
produced logs/../outside.log, which the operating system resolves to a
sibling of the configured directory. Half the hole was closed.
logName names a file, not a path, so the directory portion is now
stripped rather than assumed absent. That closes both escapes at once and
leaves an ordinary name untouched. A value consisting only of a directory
part ("sub/") leaves nothing behind, so the process name stands in rather
than producing a file called ".log".
Covered by a Theory over both separators, mutation-verified: reverting to
the previous fix fails both cases with the log file present outside the
directory.
The two theory rows deliberately build a unique destination each. Without
that they resolve to the same sibling file on Windows, where '/' and '\'
are equivalent, and race for one path - the first version failed only one
of the two under mutation for exactly that reason.
Also puts EnvironmentSettingsTests in the GlobalConsoleState collection.
It had a private collection name while mutating process-wide
EnvironmentSettings.Current, and xUnit runs distinct collections in
parallel - so it could reset that state underneath AppBuilderTests,
CommandAppExecutorTests and CommandAppConfiguratorTests, all of which
read it. GlobalConsoleState sets DisableParallelization.
Refs: #48
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 400 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.cursor/skills/winui3-controls-layouts/SKILL.md:3
- This is one of 15 newly committed WinUI-specific skill files, but the repository contains no WinUI source or project references and this guidance is not mirrored in the other agent skill directories. These files are unrelated to the command-line release and add substantial repository/tooling clutter; remove the
winui3-*skill set from this PR.
.cursor/mcp.json:5
- This commits a live-looking ContextStream API credential to the public repository. Anyone can reuse it, and removing the file later will not remove it from Git history. Remove the credential from version control, load it from a local environment/secret store, and revoke/rotate this key before merging.
CI failed on the row that hard-coded a backslash. On Linux a backslash is an ordinary file-name character, not a separator, so "..\name" is a single valid file name that lands inside the log directory - the assertion that it escaped was asserting something untrue, and the path the test computed as a "sibling" was that same in-directory file. The theory now takes its rows from Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar, deduplicated: two rows on Windows, one on Unix where both are '/'. Each row therefore tests a character that really does delimit directories on the platform running it. Mutation-verified on Windows: reverting the production fix fails both rows. The underlying mistake was verifying platform-dependent behaviour only on the platform I was sitting on. Every Path API in this fix was checked against .NET on Windows; CI runs ubuntu-latest, where GetInvalidFileNameChars, the separator set and GetFileName all behave differently. Refs: #48
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 400 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.cursor/mcp.json:5
- This commits an API credential in a tracked editor configuration file. Anyone who can read the repository can reuse it, and removing it later will not remove it from history. Revoke/rotate the key immediately, remove it from the repository, and load it from an ignored local configuration or secret/environment injection; the current
/.mcp.jsonignore rule does not cover.cursor/mcp.json.
GitHub withholds repository secrets from workflow runs triggered by a pull request opened from a fork, so SONAR_TOKEN arrives empty and the scanner cannot authenticate. Because SonarScanner Begin precedes Build and Test, an external contributor's pull request would fail before a single line was compiled - with an error about a token they have no way of supplying. The step now runs for pushes and for same-repository pull requests only. That keeps it blocking everywhere it can actually run, which is the deliberate choice recorded above it, rather than weakening the gate. SonarScanner End needs no change: it already runs only when steps.sonar-begin.outcome == 'success', and a skipped step reports 'skipped', so it stands down with Begin. This repository is public, so fork pull requests are possible even though none has arrived yet. Refs: #48
|
Bito Automatic Review Skipped - Large PR |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 400 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
.claude/rules/naming.md:5
- This rule conflicts with the repository's established C# naming convention: public methods such as
AppBuilder.ConfigureCommandApp(src/Spectre/CommandLine.Spectre/AppBuilder.cs:52) and properties such asEnvironmentSettings.Current(EnvironmentSettings.cs:25) use PascalCase. Applying this always-on instruction would make generated code violate the codebase convention.
.cursor/rules/naming.mdc:10 - This always-applied Cursor rule also prescribes camelCase C# members, contradicting
AppBuilder.ConfigureCommandApp(src/Spectre/CommandLine.Spectre/AppBuilder.cs:52) andEnvironmentSettings.Current(EnvironmentSettings.cs:25). Keep the mirrored rule aligned with the repository's PascalCase member convention.
.cursor/skills/winui3-controls-layouts/SKILL.md:4 - This WinUI skill set is unrelated to a console-command-line library and is not part of the PR's stated release work. The PR adds multiple WinUI packs and reference files under
.cursor/skills, increasing an already review-limited change set and creating unrelated maintenance surface. Remove these files from this PR or move them to the repository where WinUI guidance is actually used.
.cursor/mcp.json:5
- This file commits a live API credential in plaintext. Anyone with repository access can reuse it, and deleting the file later will not remove it from Git history. Remove the credential from tracked configuration, load it from a local environment/secret store, and revoke/rotate the exposed key.
4 tasks
AppBuilder.Create attached an anonymous handler to the static Console.CancelKeyPress event and created a CancellationTokenSource, and released neither. The event is process-wide and the lambda captures the source, so every Create call left one more handler subscribed for the life of the process, pinning the source and its closure. GitHub Code Quality flags the undisposed source at warning severity, which is what blocks the merge under this repository's code_quality ruleset rule. AppBuilder now implements IDisposable and tears down exactly what it created. A source passed to the public constructor belongs to the caller and is deliberately left alone, so that constructor keeps its current semantics and only Create takes ownership. Configuring a disposed builder throws ObjectDisposedException instead of publishing a released source to the application's services, and a Ctrl+C that races Dispose is answered on the console thread rather than throwing there. The token has to outlive construction - the application runs after ConfigureCommandApp returns - so the documented pattern is a using declaration that falls out of scope once the run has returned, not one wrapped around the builder chain. The documentation and the getting started guide are updated to that form. The sample keeps its current shape: it builds against the published package, where AppBuilder is not yet disposable, so a using declaration there does not compile. It adopts one when the package ships. Issue #32 also records that e.Cancel = true leaves Ctrl+C unable to force-terminate the process. That half is unchanged and stays tracked there. Refs: #32 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KECFm7givf8zoQi2hJmCiN
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 400 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.cursor/mcp.json:5
- This commits an API credential in a tracked editor configuration file. Anyone with repository access can reuse it against the ContextStream endpoint, and deleting it later will not remove it from history. Remove the credential from the repository, load it from local secret/environment configuration, add the local config path to
.gitignore, and revoke/rotate the exposed key before merging.
SonarCloud raised IDE0039 against the lambda assigned to the handler variable - the one new finding the previous commit introduced. A named local function states what the handler is at its declaration and drops the deep indentation the lambda forced. Behaviour is unchanged. The delegate is still captured in a variable and that same instance is what Dispose unsubscribes. Converting the local function a second time would yield a second delegate instance and -= would still match it on method plus target, but holding the instance keeps the subscribe/unsubscribe pairing explicit rather than resting on structural equality. Refs: #32 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KECFm7givf8zoQi2hJmCiN
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 400 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.cursor/mcp.json:5
- This commits a live-looking ContextStream API key to the public repository. Anyone with read access can reuse it, and removing it later will not remove it from history. Revoke/rotate this key immediately and keep the editor-specific MCP configuration untracked or source the credential from an environment variable/secret store.
|
Bito Automatic Review Skipped - Large PR |
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
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.
Describe your changes
Initial implementation of
Ploch.CommandLine.Spectre— adopting Spectre.Console as the framework this library builds on — together with everything needed to make the repository releasable: the legacy projects retired, CI made functional, packaging modernised, a sample application, published documentation, and a test suite.+35,652 / −107,469 across 422 files. The large deletion is the retired McMaster-based library and 4.2 MB of committed analyser output.
This began as the library alone. It grew because the repository could not actually ship: it did not compile, its CI had never once executed, it had no licence file, no release path, and 3.33% test coverage. Each of those was tracked as its own issue and delivered as its own reviewed pull request, then merged here.
Why this is one large PR
The sub-PRs (#15, #16, #17, #18, #21) were each reviewed independently before merging in. They had to converge here rather than land separately, for a structural reason: SonarCloud measures new code, and this branch introduces the entire library, so every file in the repository counts as new. Any finding anywhere therefore fails this PR's gate. #16's deletion of
DocumentationSite/Dockerfilewas literally required to clear the security rating. Splitting the work further would not have produced independently mergeable PRs.The cost is real and should be stated: at 422 files this PR is past CodeRabbit's 100-file review limit, so it has had no CodeRabbit review. SonarCloud, Qodana, CodeAnt and Codex all reviewed the constituent PRs.
Changes
The framework
AppBuilder, wrappingMicrosoft.Extensions.HostingwithSpectre.Console.Cli, plusCommandAppExecutor/ICommandAppExecutor.AppCommand<TSettings>andAsyncAppCommand<TSettings>base classes with validation, exception handling and cancellation.ICommandSettingsProcessor,CommandArgumentsRootProcessor,CommandSettingsPropertyTypeProcessor<T>, andTokensArgumentsProcessorfor{date}/{datetime}substitution via[SupportsTokens].IOutput/AnsiConsoleMarkupOutput,IMessageFormatterProcessor, type-based formatters and writers.UseCaseAsyncCommand, bridging commands toIResultUseCase(Ardalis.Result).Runtime defects fixed
The branch did not compile when this work started (3 errors, 116 warnings). External review then surfaced seven defects on consumer-visible paths:
ConvertibleMessageFormatterwas DI-registered but threwNotImplementedExceptionoutput.Write(42)crashed —int,DateTime,boolall implementIConvertibleCommandInfoFactory.CreateFromTypethrew on its primary pathCancellationTokenaccepted everywhere, honoured nowhereAnsiConsoleMarkupOutput.Writedouble-wroteDefaultExceptionHandlermarkup-unsafe[could throw inside the exception handlerFigletTexttwiceTreatWarningsAsErrors=falsein ReleasePlus the review findings (#14):
EnvironmentSettings.Currentcheck-then-act lazy init replaced with a synchronised one;EnvironmentSettingsLoaderno longer retains the entire environment block, which routinely carries secrets; the Serilog error sink sat outside its filtered sub-logger so the "errors" file received every event;AddSerilogregistered Serilog twice.Legacy retired (#10)
src/CommandLine{,.Autofac,.Hosting,.Serilog},src/DemoApp, their tests and samples, plus the old solutions. Nothing here was ever published to NuGet, so no consumer is affected and no deprecation shims were added.Repository clutter removed with evidence for each:
azure-pipelines.ymlbuilt another repository's solution (Ploch.Common.sln/ploch_common);run-sonar-build-test.ps1scannedmrploch_ploch-data;build.cmd/.ps1/.shwere NUKE bootstraps for abuild/_build.csprojthat does not exist;qodana.sarif.jsonwas 4.2 MB of generated output.CI made functional (#12)
CI had never run.
build-dotnet.ymlfiltered on branchmasterwhile the default branch ismain.Once enabled, a chain of failures surfaced, each hidden by the previous one:
MSB3202for every../ploch-commonproject — the solution references sibling repositories by relative path, and CI checked out only this one. Siblings are now cloned to reproduce the workspace layout.fetch-depth: 0.Not authorizedon every run. Root cause was not permissions, despite the message naming the token, key, organisation and permissions. The sharedmrploch/ploch-github-actions/build-test-sonaraction passes the token only assonar.login, which is deprecated and no longer honoured by the current scanner engine — so it authenticated anonymously. Proved by re-running the old workflow and the new one minutes apart against the same token, key and organisation: old fails, new passes. The inline workflow setsSONAR_TOKENas an environment variable, which the scanner reads natively. Filed upstream as build-test-sonar passes the Sonar token only via the removed sonar.login property, so every analysis fails 'Not authorized' ploch-github-actions#1, since every repository still using that action is silently unanalysed.dotnet nuget add sourcefailed on a duplicate URL —NuGet.Configalready registers that feed asgithub, and the command rejects duplicate URLs as well as duplicate names. Packages are now pushed to the feed URL directly.Also:
pull_requestwas filtered tobranches: [main], which matches the PR's base branch, so stacked PRs got no build at all; the filter is gone.code_quality.ymlandtest_report.ymlwere stale duplicates and are removed. Third-party actions are pinned to commit SHAs. The Pages deploy and all package publishing are gated so they cannot fire from an arbitrary branch.Packaging and versioning (#7)
Legacy
VersionPrefix 0.0.1+RELEASEVERSIONreplaced with Nerdbank.GitVersioning (version.jsonat1.0-prerelease),.config/dotnet-tools.jsonpinningnbgvanddocfx, and aglobal.jsonpinning the SDK — the build previously worked only because the runner image happened to ship a .NET 10 SDK and nothing pinned it, while$(TargetFrameworkVersion)resolves tonet10.0.PlochCommandLine.Spectre.FluentValidationwas renamed toPloch.CommandLine.Spectre.FluentValidation— the package id was missing a dot. The C# namespace was renamed with it, since shipping aPloch.*package containing aPlochCommandLine.*namespace would have been permanent.SourceLink,
.snupkgsymbols and portable PDBs added. Four packages now pack cleanly.Licence, documentation and release (#6)
LICENSEadded.Directory.Build.propshas assertedPackageLicenseExpression = Apache-2.0since the repository was created, with no licence file present — every package would have shipped claiming a licence the repository did not carry.index.mddocumented the retired McMaster API, andtoc.ymllinked toploch-data. Rewritten against the current public surface, verified against the source. Builds with 0 errors and 2 warnings, both benign cross-repository references.publish-docs.ymldeploys to GitHub Pages via the official Pages actions.release.yml— manually dispatched, tags before publishing (orphaned NuGet packages cannot be deleted, only unlisted), publishes to NuGet.org, creates the GitHub Release fromchange-log/entries, then bumps to the next development version.RELEASE_NOTES.mdand thechange-log/convention.Sample application (#9)
samples/SampleApp/— a complete multi-level CLI (config,file,project,userbranches) demonstrating DI, configuration, FluentValidation, token expansion, Serilog, use cases and exit codes, with 28 tests of its own and adocs/GETTING_STARTED.mdwalkthrough. It consumes the libraries asPackageReferencewith a-p:UsePlochProjectReferences=trueswitch for in-repo validation. CI builds it, so it cannot silently rot against a library change again.Running it found five further defects, four of which only manual execution could surface — including
config showprinting the entire process environment, API keys included, because it enumeratedconfiguration.GetChildren()and the host adds an environment-variable provider.Tests and coverage (#13)
new_coverageTwo packages had no test project at all (
Serilog,UseCases). Regression cover was added for every defect above — including a test that writes real events through a real Serilog pipeline and asserts that only Warning and above reach the errors file.Post-review fixes
Codacy raised 16 threads on this pull request. All are resolved: two were already fixed by #28, eight were fixed in code across #35, #36, #37 and #39, three were declined with evidence, and three were deferred to new issues.
The substantive one was a release blocker.
output.WriteError("Value [archive] is invalid")threwInvalidOperationException: Could not find color or style 'archive'— arbitrary caller data was going through Spectre's markup parser, in the one method whose purpose is printing exception text. Fixed in #35 and #37 by escaping content where this library adds the tag, while leaving markup the caller writes untouched. The same change recovered format specifiers:$"total: {1234.5:N2}"had been rendering astotal: 1234.5.Two defects were also found in the fixes themselves, both caught by review before merge: a trailing blank line after writer-handled collections, and a first attempt at that fix which contradicted its own contract by suppressing the line break for inline writers.
IMessageWriter.WritesLineTerminatorsettles it.Design decisions
ploch-common, which marks its Sonar stepscontinue-on-error. Recorded in a comment in the workflow so it is not "aligned" away later.PackageReference, preserving the documented workspace model and requiring no change to the eight cross-repository project references.IDE0058disabled repo-wide in.editorconfig, per.claude/rules/code-quality.md, which prescribes this exact remedy and forbids the_ =discard alternative. Matchesploch-commonandploch-data. This cleared 357 INFO findings.VSTHRD200disabled for tests, because this repository mandates<TestedMember>_should_<behaviour>names which cannot also carry theAsyncsuffix the rule wants.Breaking changes
Pre-release; nothing published depends on these yet.
AppCommand<TSettings>.DoExecuteandAsyncAppCommand<TSettings>.DoExecuteAsyncnow accept aCancellationToken.IMessageFormatterProcessor.WriteMessagereturnsboolinstead ofvoid.PlochCommandLine.Spectre.FluentValidation→Ploch.CommandLine.Spectre.FluentValidation.Ploch.Common.CommandLinepackages and their Autofac, Hosting and Serilog companions are removed.ConsoleAppInfo.AppNameColorSys,AppNameInfoColorSys,AppDescriptionColorSysandConsoleAppInfoExtensions.FromSysColorremoved — use the SpectreColorproperties.AnsiConsoleMarkupOutput.WriteMarkupLineInterpolatedremoved — useMarkupLineInterpolated.EnvironmentSettings.Initializethrows if called afterCurrenthas been read;DevRuntimeVariablescontains onlyDEV_RUNTIME-prefixed variables;PauseBeforeExitdefaults tofalse.Testing
Local:
dotnet build -c Release— 0 errors, 0 warnings.dotnet test— 195 tests, 0 failures. Sample builds and runs; real console output captured in #18.CI on the head commit:
buildpass,Test Resultspass,qodanapass,SonarCloud Code Analysispass.SonarCloud quality gate OK — every condition green:
new_coveragenew_reliability_ratingnew_security_ratingnew_maintainability_ratingnew_duplicated_lines_densitynew_security_hotspots_reviewedZero security hotspots to review; zero unresolved review threads. 45 platform findings remain, all pre-existing INFO plus two MAJOR code smells that predate this work — enumerated and triaged in #24.
Related
IOutput.Write(exception)threwInvalidCastExceptionthrough the writer pipelineAppBuilderconfiguration delegates were last-wins instead of additiverelease.ymlconcatenatedchange-log/README.mdinto the release notesWriteErrorthrew on any message containing a[release.ymlis actually dispatched to cut v1.Console.CancelKeyPresshandler leak, and Ctrl+C unable to terminate. Deferred: both halves change application lifecycle behaviour and want a deliberate decision.AppCommandperforms no settings argument processing whileAsyncAppCommanddoes. Deferred: needs a call on which base class is right.Follow-ups filed: #19 (sample tests built but not run), #20 (
IOutput.Write(exception)throwsInvalidCastException), #22 (AppBuilder.ConfigureServicesdiscards all but the last delegate), #23 (Codacy coverage upload unauthenticated), #24 (remaining analyser findings), mrploch/ploch-github-actions#1 (shared Sonar action broken org-wide).Checklist before requesting a review
Ploch.CommandLine.SpectreRelease-readiness review pass (2026-08-26)
Twelve findings from the Copilot reviewer were each verified against source, fixed, and
re-reviewed by an external panel (Codex, Gemini, GitHub Copilot CLI/Grok 4.6). Commits
06864bband46c2884.Fixed
PrintAppInfoescapes consumer name/description. Reproduced theCould not find color or style 'Dev'crash before fixing.Win32ExceptionMessageFormatterunreachableIFormatProviderignored for scalarsIFormattable; null results still coalesce to empty. Default behaviour unchanged.volatiledouble-checked lockEnvironmentSettings._currentisvolatile.DbContext.AddCommandLineSettingsFluentValidationregisters the mapping once.EchoSettings, default off. Breaking.<redacted>.AddSerilogremarks.claude/local-marketplace/contentBreaking changes
UseCaseAsyncCommand<...>no longer echoes settings; overrideEchoSettingsto restore.FluentCommandSettingsValidator<TSettings>takesIServiceScopeFactoryinstead of anoptional
IValidator<TSettings>(resolved from DI, so direct construction only).Verification — build clean in Debug and Release, 0 warnings under
TreatWarningsAsErrors. 210 tests in the Spectre suite (was 202), 8 in FluentValidation(was 4), 9 in UseCases (was 7). Every fix has a regression test, and each was
mutation-verified by reverting the fix and confirming the test fails.
Known blockers
S2360/S2339in pre-existingsrc/code — see Codacy: scope analysis to project deliverables and settle the C# API-shape findings #38.Ploch.Commonpinned to2.0.1 vs the required
>= 4.0.20-prerelease) — pre-existing, filed as SampleApp does not build against the published packages (Ploch.Common pinned to 2.0.1) #46.