Align the Cmd integration with shell APIs - #4278
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Cmd integration now exposes ChangesCmd v4 integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR aligns the Cmd APIs, relocates options, and adds typed file execution while preserving direct batch-file execution; the supplied build, test, formatting, and API checks pass, leaving no actionable merge-blocking risk. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 38.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR aligns the Cmd integration with the shared shell API conventions and documents the v4 migration.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines.Cmd/CmdContext.cs | Renames the Cmd service implementation and adds typed batch-file execution while preserving the string overload. |
| src/ModularPipelines.Cmd/ICmdContext.cs | Defines the renamed public Cmd context with typed and string overloads for scripts and batch files. |
| src/ModularPipelines.Cmd/Extensions/CmdExtensions.cs | Updates dependency-injection registration and context resolution to consistently use ICmdContext. |
| src/ModularPipelines.Cmd/Options/CmdFileOptions.cs | Introduces the strongly typed options record for direct batch-file execution. |
| src/ModularPipelines.Cmd/Options/CmdOptions.cs | Extracts common Command Prompt flags into a shared options base record. |
| src/ModularPipelines.Cmd/Options/CmdScriptOptions.cs | Moves inline script options to ModularPipelines.Options and derives them from the shared Cmd options. |
| test/ModularPipelines.UnitTests/Helpers/CmdTests.cs | Adds coverage for typed and string batch-file overloads and explicit Tool preservation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
M[Pipeline module] --> T[context.Tools.Cmd]
T --> C[ICmdContext]
C -->|RunAsync| S[CmdScriptOptions]
C -->|RunFileAsync| F[CmdFileOptions]
S --> E[Shared shell execution]
F --> E
E --> R[CommandResult]
Reviews (6): Last reviewed commit: "fix(cmd): preserve file tool overrides" | Re-trigger Greptile
GitHub Actions
Inactive
![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: abe65160bc
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Summary of changes: renames ICmd→ICmdContext / Cmd→CmdContext, moves the Cmd options out of ModularPipelines.Cmd.Models into ModularPipelines.Options (splitting into CmdOptions, CmdScriptOptions, CmdFileOptions), and adds a typed RunFileAsync(CmdFileOptions, ...) overload for parity with IBashContext/IPowerShellContext. PublicAPI.Shipped/Unshipped.txt, tests, docs, and release notes are all updated consistently with the new shape, and the rename itself is clean — no leftover references to the old ICmd/Cmd/ModularPipelines.Cmd.Models.CmdScriptOptions names anywhere in src or test.
Finding: CmdFileOptions.FilePath can silently desync from the executable that actually runs
src/ModularPipelines.Cmd/Options/CmdFileOptions.cs:10
public record CmdFileOptions(string FilePath) : CommandLineToolOptions(FilePath);FilePath is a normal record positional property — a store separate from the base Tool property, which is what ToolResolver.ResolveTool (src/ModularPipelines/Helpers/Internal/ToolResolver.cs:32, options.Tool ?? ResolveTool(options.GetType())) and CommandLineBuilder.Build (src/ModularPipelines/Context/CommandLineBuilder.cs:57) actually read to pick the executable that gets launched. The constructor happens to set both to the same value, but nothing keeps them in sync afterward.
Because CmdFileOptions is an immutable record, the idiomatic way to change the path is options with { FilePath = "b.cmd" } — and that's exactly where it breaks: the record copy constructor carries the old Tool value forward untouched, so the resulting record's FilePath property reports "b.cmd" while the process that actually launches is still "a.cmd".
Compare with BashFileOptions/PowerShellFileOptions elsewhere in the codebase, where FilePath is a real [CliArgument]-attributed argument that the command builder re-reads each time, so with-mutations stay correct. CmdFileOptions diverges from that established pattern by piggy-backing on Tool directly (understandable, since a batch file is invoked directly rather than via a wrapping cmd/bash tool), but that's what introduces the duplicate, unsynchronized state.
Since this PR's own docs/release-notes changes are actively pointing users at these typed option records for their with-friendly ergonomics, this is a realistic footgun rather than a theoretical one — an automated review bot (Codex) also independently flagged this exact issue, and tracing it through ToolResolver/CommandLineBuilder confirms it's real.
Suggested fix: don't let Tool and FilePath drift independently. The smallest, self-contained fix is to resolve the executable from the current FilePath at the call site in CmdContext.RunFileAsync, right before forwarding to the shell:
public virtual Task<CommandResult> RunFileAsync(
CmdFileOptions options,
CommandExecutionOptions? executionOptions = null,
CancellationToken cancellationToken = default)
{
return _context.Shell.RunAsync(options with { Tool = options.FilePath }, executionOptions, cancellationToken);
}This keeps the current public API/property shape intact (no breaking change) while guaranteeing whatever FilePath currently holds — including after any with mutation — is what actually executes.
1275f66 to
8b9b79b
Compare
|
@claude review |
GitHub Actions
Inactive
There was a problem hiding this comment.
Review: Align the Cmd integration with shell APIs
Reviewed the full diff (src/ModularPipelines.Cmd/*, docs/docs/mp-packages/cmd.md, RELEASE_NOTES_V4.md, and 3 test files). The ICmd → ICmdContext rename is complete and clean — traced every reference across the repo and found no stale usages. The CmdOptions/CmdScriptOptions split preserves the original [CliFlag] behavior exactly, and the existing RunAsync(CmdScriptOptions) code path is unchanged.
Two findings posted inline, both worth addressing before merge:
-
src/ModularPipelines.Cmd/CmdContext.cs:50—RunFileAsync(CmdFileOptions options, ...)unconditionally overwritesoptions.Toolwithoptions.FilePathviaoptions with { Tool = options.FilePath }. SinceToolis a publicinitproperty thatToolResolver.ResolveTooluses to pick the executable, any caller who explicitly setsToolon the options (e.g. to run the file throughcmd.exerather than directly) has that value silently discarded. Either honor a caller-suppliedTool, or throw/document that it's not supported. -
test/ModularPipelines.UnitTests/Helpers/CmdTests.cs:34— The test for file execution was migrated from the plainRunFileAsync(string path, ...)overload to the newRunFileAsync(CmdFileOptions options, ...)overload. Good coverage of the new API, but the original string-path overload is still public onICmdContextand now has no test coverage anywhere in the repo. Consider keeping (or adding back) a test for that overload so it isn't shipped untested.
No architectural concerns beyond these — the rename and options split are a reasonable, low-risk refactor.
9db570f to
558f7d6
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Summary: This PR renames ICmd→ICmdContext/Cmd→CmdContext, moves the Cmd option records from ModularPipelines.Cmd.Models into ModularPipelines.Options (splitting into CmdOptions/CmdScriptOptions/CmdFileOptions), and adds a typed RunFileAsync(CmdFileOptions, ...) overload for parity with IBashContext/IPowerShellContext. Docs, release notes, PublicAPI.* baselines, and tests are updated consistently, and I traced the rename across src/test — no stale ICmd/Cmd/ModularPipelines.Cmd.Models.CmdScriptOptions references remain.
Comparing against the prior review rounds on this PR:
- Round 1 (
abe65160) flagged thatCmdFileOptions.FilePathcould desync from theToolactually executed after awith { FilePath = ... }mutation. That was fixed in9d29c2fcby recomputingToolfromFilePathat theRunFileAsynccall site. - Round 2 flagged that this fix has a side effect: it unconditionally discards a caller-supplied
Tool. That finding is still present on the current head (558f7d6f) — the only commit since round 2 (558f7d6f) is a whitespace-only test change.
Still open: RunFileAsync silently discards a caller-supplied Tool
src/ModularPipelines.Cmd/CmdContext.cs:44-53
public virtual Task<CommandResult> RunFileAsync(
CmdFileOptions options,
CommandExecutionOptions? executionOptions = null,
CancellationToken cancellationToken = default)
{
return _context.Shell.RunAsync(
options with { Tool = options.FilePath },
executionOptions,
cancellationToken);
}Tool (src/ModularPipelines/Options/CommandLineToolOptions.cs:30) is documented as a general-purpose init override that every other CommandLineToolOptions-derived record honors when a caller sets it (e.g. to route execution through a different executable). This override unconditionally replaces whatever Tool the caller set with options.FilePath, so a caller who does new CmdFileOptions(path) with { Tool = "cmd.exe" } (e.g. to run the batch file through cmd.exe /c instead of launching it directly — useful for paths cmd needs to resolve via PATHEXT/PATH, or to combine with the CmdOptions flags) has that choice silently thrown away. That's a real inconsistency: every other options type in this codebase treats Tool as "last write wins" via with; CmdFileOptions alone makes it unwritable through the public API.
Since Tool is only ever meant to default to FilePath here (batch files are launched directly rather than through a wrapping shell, unlike Bash/PowerShell), the fix is to only fill it in when the caller hasn't already set one, preserving with-style overrides for everything else:
return _context.Shell.RunAsync(
options.Tool is null ? options with { Tool = options.FilePath } : options,
executionOptions,
cancellationToken);This keeps the round-1 desync fix (no explicit Tool ⇒ default to the current FilePath) while restoring the override contract that the rest of the options hierarchy relies on.
Still open: no coverage for the RunFileAsync(string path, ...) overload
test/ModularPipelines.UnitTests/Helpers/CmdTests.cs:25-36
The file-execution test was migrated to exercise RunFileAsync(CmdFileOptions, ...) only; the plain RunFileAsync(string path, ...) overload (still public on ICmdContext, still forwards into the code above via RunFileAsync(new CmdFileOptions(path), ...)) has no test anywhere in the repo. This was raised in round 2 and is still true on 558f7d6f. Worth adding a small test (or parameterizing the existing one) so that overload doesn't ship without any coverage.
No other concerns — the rename, options split, and doc/release-notes updates are a clean, low-risk refactor.
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Summary: This PR renames ICmd→ICmdContext/Cmd→CmdContext, moves the Cmd option records into ModularPipelines.Options (split into CmdOptions/CmdScriptOptions/CmdFileOptions), and adds a typed RunFileAsync(CmdFileOptions, ...) overload for parity with IBashContext/IPowerShellContext.
Comparing against prior review rounds: the only commit since the last review (558f7d6f) is 1107de39 ("fix(cmd): preserve file tool overrides"), which addresses both outstanding findings:
Tool/FilePathdesync (round 1) — previously fixed by computingToolfromFilePathat theRunFileAsynccall site.- Caller-supplied
Toolsilently discarded (round 2) — now fixed properly:CmdFileOptions(src/ModularPipelines.Cmd/Options/CmdFileOptions.cs) no longer forwardsFilePathinto the baseToolvia the constructor (it derives from the parameterlessCommandLineToolOptionsctor, soTooldefaults tonull), andCmdContext.RunFileAsync(src/ModularPipelines.Cmd/CmdContext.cs:50) now only defaultsToolfromFilePathwhen the caller hasn't already set one:options.Tool is null ? options with { Tool = options.FilePath } : options. This restores the "last write wins"with-override contract that every otherCommandLineToolOptions-derived record in the codebase honors, while keeping the round-1 desync fix intact. - Missing coverage for the
RunFileAsync(string path, ...)overload (round 2) — now covered byFile_String_Overload_Produces_Expected_Output(CmdStringFileModule). A new test,File_Options_Preserve_Explicit_Tool(CmdToolOverrideFileModule), also directly exercises the override-preservation fix by settingToolexplicitly to a valid file path whileFilePathpoints at a nonexistent file, asserting the explicitToolwins.
I re-traced the rename across src/test again and confirmed no stale ICmd/Cmd/ModularPipelines.Cmd.Models.CmdScriptOptions references remain, and the docs/RELEASE_NOTES_V4.md don't reference the Tool/FilePath internals so nothing there needed updating for this fix.
No further concerns — the rename, options split, and the RunFileAsync fix are consistent with the established CommandLineToolOptions conventions elsewhere in the codebase.
Rename the context contract, move typed options into the shared options namespace, and add file-option parity while preserving direct batch-file execution.
1107de3 to
ed7147e
Compare
There was a problem hiding this comment.
Code review
Summary: No new content to review. I diffed the current head (ed7147ebd) against the previously-reviewed CLEAR commit (1107de39, itself equal in content to the prior head) restricted to every file this PR actually touches (src/ModularPipelines.Cmd/**, test/ModularPipelines.UnitTests/Helpers/CmdTests.cs, docs/docs/mp-packages/cmd.md, and the Cmd section of RELEASE_NOTES_V4.md) — the diff is empty. The only change since the last review is a rebase onto a newer main that picked up unrelated merged PRs (hashing/ZIP API unification, module-result metadata renames, installer flattening, etc.); the Cmd section of RELEASE_NOTES_V4.md is byte-identical, just relocated further down the file.
Recap of this PR's history for context: the ICmd→ICmdContext rename, the CmdOptions/CmdScriptOptions/CmdFileOptions split into ModularPipelines.Options, and the typed RunFileAsync overload were reviewed across four rounds. Two real issues were found and fixed along the way — a Tool/FilePath desync after with { FilePath = ... } mutation, and RunFileAsync silently discarding a caller-supplied Tool override — both resolved in 1107de39 ("fix(cmd): preserve file tool overrides"), which also added test coverage for the previously-untested RunFileAsync(string path, ...) overload. That commit's content carries forward unchanged to the current head.
No new findings; nothing further to address before merge.
GitHub Actions
Inactive
1 participant
Summary
Validation
Closes #4234
Summary by CodeRabbit
New Features
context.Tools.Cmd.Documentation