Add the nano-migrate stand-alone tool - #111
Open
danielmeza wants to merge 9 commits into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the NanoMigrate toolchain, docs, build and pipeline wiring, conversion and lifecycle support, CLI commands for migrate/clean/rollback/clone/fleet, and tests covering conversion, reporting, rollback, verification, and planning. ChangesNanoMigrate Toolchain
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)tools/migrate/src/NanoMigrate.Core/Common/Glob.cs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path tools/migrate/src/NanoMigrate.Core/Common/ProcessExec.cs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path tools/migrate/src/NanoMigrate.Core/Common/ProjectScanner.cs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path
🔧 markdownlint-cli2 (0.22.1)README.mdmarkdownlint-cli2 wrapper config was not available before execution README.zh-cn.mdmarkdownlint-cli2 wrapper config was not available before execution skills/nanoframework-sdk-migration/SKILL.mdmarkdownlint-cli2 wrapper config was not available before execution
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@azure-pipelines.yml`:
- Around line 522-524: The "Sign packages" task has continueOnError set to true,
which allows the pipeline to continue executing downstream tasks even if package
signing fails, creating a security vulnerability where unsigned packages could
reach NuGet. Change continueOnError from true to false on the "Sign packages"
task in both the Build_Nano job (around lines 522-524) and the Build_Migrate job
(around lines 652-654) to ensure the pipeline halts immediately if signing
fails, preventing unsigned artifact distribution.
In `@skills/nanoframework-sdk-migration/references/contributing-compliance.md`:
- Around line 123-150: The three consecutive bullet points in the C# coding
style section all begin with "Use" (regarding language keywords, var usage, and
nameof), creating repetitive sentence structure that impacts readability. Reword
one or more of these three bullets to vary the opening and improve flow—for
example, by combining related concepts into fewer, more varied sentences or
using alternative phrasings like "Prefer," "Apply," or restructuring as a single
compound statement that covers all three concepts together.
In `@skills/nanoframework-sdk-migration/SKILL.md`:
- Line 17: In the SKILL.md file, change the phrase "legacy flavored" to
"legacy-flavored" (with a hyphen) on the line that reads "Convert a
nanoFramework repo from the legacy flavored `.nfproj` project system onto
SDK-style". Using the hyphenated form "legacy-flavored" properly formats the
compound adjective for consistency and clarity.
- Line 28: The markdown file SKILL.md contains code blocks without language
identifiers (e.g., ```bash, ```shell), which violates the MD040 linter rule and
prevents proper syntax highlighting. Add appropriate language specifiers to all
code blocks throughout the file by modifying the opening backticks from ``` to
```bash for bash commands, ```shell for shell scripts, ```text for plain text
output, or ```markdown for markdown content, as applicable to each code block.
- Line 61: Add blank lines before and after all fenced code blocks (marked with
```) in the SKILL.md file to comply with markdown linting rule MD031. For each
code block at lines 61, 64, 68, 70, 72, and 74, insert a blank line immediately
before the opening fence and immediately after the closing fence to ensure
proper spacing around code block boundaries.
In `@tools/migrate/README.md`:
- Line 17: The fenced code block starting at line 17 in tools/migrate/README.md
is missing a language identifier after the opening triple backticks. Add `bash`
or `shell` as the language identifier immediately after the opening fence
(before the newline) to enable proper syntax highlighting and satisfy the MD040
markdown linting rule. The code block containing the tools/migrate directory
structure will then have the proper language specification.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cs`:
- Around line 103-117: The Execute method receives a cancellationToken parameter
that is never forwarded to the RunLoose or RunSolutionScoped method calls,
breaking the cancellation chain for long-running operations. Update the
signatures of both RunLoose and RunSolutionScoped methods to accept the
cancellationToken parameter, pass the cancellationToken argument when calling
these methods from the switch statement in Execute, and then propagate the token
through all downstream operations including ProcessProjects, _converter.Convert,
and builder.VerifyAll calls within those methods to enable graceful cancellation
support.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/MigrateRegistration.cs`:
- Around line 22-25: The XML documentation comment in the MigrateRegistration.cs
file contains an incorrect parameter reference. The doc comment references a
parameter named `appName` using `<paramref name="appName"/>`, but the actual
method parameter is named `migrateDescription`. Update the XML documentation to
replace the incorrect parameter reference from `appName` to `migrateDescription`
to ensure the generated API documentation accurately reflects the actual method
parameters.
In `@tools/migrate/src/NanoMigrate.Cli/Cli/CloneCommand.cs`:
- Around line 39-40: The outDir variable defaults to a relative path
"./nano-repos" which causes git clone to resolve destination paths incorrectly
when passed to ProcessRunner.Run(). Normalize outDir to an absolute path by
converting the relative default (and any user-provided relative path from
settings.OutDir) using Path.GetFullPath() before calling
Directory.CreateDirectory() and before passing it to ProcessRunner.Run() in
subsequent operations, ensuring repos are cloned to the correct directory and
the "already present" check works correctly.
In `@tools/migrate/src/NanoMigrate.Cli/ProcessRunner.cs`:
- Around line 30-34: The Process.Start method in this code block reads
StandardOutput and StandardError sequentially which can cause deadlocks, and
calls WaitForExit() without a timeout allowing indefinite hangs. Refactor this
to read both StandardOutput and StandardError concurrently using async
operations instead of sequential ReadToEnd calls, then add a timeout parameter
to the WaitForExit call to prevent the process from hanging indefinitely. This
ensures both streams are drained in parallel and the process doesn't wait
forever if it becomes unresponsive.
In `@tools/migrate/src/NanoMigrate.Core/Backup/BackupCleaner.cs`:
- Around line 89-105: The CleanResult is over-reporting deletions because
result.RemovedBackups.Add(bak) and result.RemovedFolders.Add(dir) are called
regardless of whether the files/directories actually exist. Move the Add() calls
inside the if statements so that result.RemovedBackups.Add(bak) only executes
when File.Exists(bak) is true and the deletion succeeds, and
result.RemovedFolders.Add(dir) only executes when Directory.Exists(dir) is true
and the deletion succeeds. This ensures the CleanResult only reports items that
were actually deleted, not those that didn't exist in the first place.
In `@tools/migrate/src/NanoMigrate.Core/Backup/RollbackJournal.cs`:
- Around line 233-241: The Apply method directly uses paths from the manifest
(e.OriginalPath and e.BackupPath) without validating they remain within an
allowed root directory, enabling arbitrary file operations outside the migration
scope. Add boundary validation by accepting an allowedRoot parameter in the
Apply method and verify both e.BackupPath and e.OriginalPath resolve to paths
within this allowed root directory before performing any File.Copy,
Directory.CreateDirectory, or File.Delete operations. This check should ensure
the full resolved paths (after Path.GetFullPath) start with the allowed root
path to prevent directory traversal attacks via modified manifests.
- Around line 279-285: The ApplyAndCleanup method unconditionally deletes the
backup set directory after calling Apply(manifest), regardless of whether the
apply operation succeeded or failed. If Apply encounters partial failures during
restore/delete operations, the directory cleanup still proceeds and discards
artifacts needed for recovery. Modify the code to only delete the setDir
directory when Apply(manifest) completes successfully, by checking the return
status or success condition of the Apply call before executing the
Directory.Delete operation, ensuring that backup artifacts are preserved when
rollback encounters problems.
In `@tools/migrate/src/NanoMigrate.Core/Common/Glob.cs`:
- Around line 42-55: The current implementation appends `.*` when handling `**`
patterns in the `*` case, which incorrectly matches arbitrary characters and can
cause patterns like `**/Foo.nfproj` to match `MyFoo.nfproj`. Replace the
`sb.Append(".*");` call in the `*` case (where `pattern[i + 1] == '*'`) with a
regex pattern that enforces directory boundaries, such that `**/` is treated as
an optional directory-prefix segment rather than matching any arbitrary
characters. This ensures unintended projects are not matched during migration.
In `@tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs`:
- Around line 153-156: The WriteReport method writes to o.Report without
ensuring parent directories exist, which will fail for nested paths like
reports/fleet/migration.md. Before calling File.WriteAllText in WriteReport,
extract the directory path from o.Report using Path.GetDirectoryName, then call
Directory.CreateDirectory on that path to ensure all parent directories exist
before attempting to write the file.
- Around line 114-115: The branch name from o.Branch is directly interpolated
into the git command string without validation, creating a security
vulnerability where malicious branch names could alter git argument parsing
through option smuggling. Before using o.Branch in the _git.Run method call for
the checkout command, validate that the branch name contains only safe
characters (alphanumeric, hyphens, underscores, slashes) and does not start with
a hyphen that could be interpreted as a git option. Alternatively, check if the
git runner supports passing arguments as a list instead of a concatenated string
to avoid string interpolation entirely.
In `@tools/migrate/src/NanoMigrate.Core/Projects/ProjectConverter.cs`:
- Around line 89-91: The code allows CentralPackageManagement to be active
(cpmActive = true) when no Directory.Packages.props file exists (cpmPropsPath is
null), but the package version additions are only applied when cpmPropsPath is
not null, resulting in invalid versionless PackageReference items. Fix this by
ensuring that whenever CentralPackageManagement is enabled and cpmPropsPath is
null, either create and journal the Directory.Packages.props file in the
migration, or prevent cpmActive from being set to true and instead fail or log a
warning about the missing central props file. Apply this fix consistently across
all three affected locations where cpmActive is set and where cpmAdditions are
populated.
- Around line 116-132: The SetProp method in the PropertyGroup iteration is
deduplicating properties solely by name, which loses conditional information
from legacy projects and flattens Debug/Release-specific settings into
unconditional properties. Modify the code to capture the Condition attribute
from each PropertyGroup element (via pg.Attribute("Condition")) and pass it to
SetProp alongside the property key and value. Update the SetProp method and the
props collection to store both the property name and its associated condition,
ensuring that properties with different conditions are not deduplicated by name
alone. This way, when properties are later emitted, the condition attributes can
be preserved on the generated property elements to maintain the correct
Debug/Release behavior during migration.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/SolutionRewriter.cs`:
- Around line 77-79: The bare catch block at Line 77 in the try-catch statement
around File.ReadAllText(solution.Path) silently suppresses all exceptions and
returns false, making I/O failures indistinguishable from intentional no-change
scenarios. Instead of catching all exceptions without distinction, either let
the exception propagate (remove the catch block entirely if returning false is
not necessary), or catch only specific exceptions if error handling is required,
and ensure that unexpected I/O failures are logged or re-thrown rather than
silently converted to a false return value that masks the actual problem.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cs`:
- Around line 22-24: The nested foreach loop in the SolutionScanner that calls
Directory.EnumerateFiles with SearchOption.AllDirectories can throw an exception
when encountering inaccessible folders, causing the entire scan to abort. Wrap
the Directory.EnumerateFiles call in a try-catch block to handle any exceptions
that occur during enumeration (such as unauthorized access), log or skip the
inaccessible directory, and continue scanning the remaining directories. This
ensures the scan completes even when some folders are inaccessible.
In `@tools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cs`:
- Around line 165-169: The sequential ReadToEnd() calls on both StandardOutput
and StandardError streams in the Process.Start block create a deadlock risk when
both pipes are redirected and buffer space fills. Replace the sequential reads
with asynchronous concurrent reads instead. Use Task.WaitAll() or similar
concurrency pattern combined with ReadToEndAsync() to read both streams
(StandardOutput and StandardError) in parallel before calling WaitForExit(),
ensuring neither stream buffer fills while the parent is blocked waiting on the
other stream.
In `@tools/migrate/tests/NanoMigrate.Tests/ReportingTests.cs`:
- Around line 202-205: The test in the ReportingTests.cs file does not assert
that the CLI command exited successfully, only that a report file was written.
Add an assertion to verify that the exit variable returned from app.Run() equals
zero (the success exit code) before asserting on the report file contents. This
ensures the test fails if the migrate command exits with a non-zero status even
when a report file is generated.
In `@tools/migrate/version.json`:
- Around line 11-13: The publicReleaseRefSpec field in
tools/migrate/version.json currently uses a permissive "^" pattern that marks
all refs as public-release candidates, which does not align with the stricter
best-practice patterns used in tools/VersionCop/version.json. Replace the loose
"^" pattern with more restrictive regex patterns that explicitly match only the
intended branches and tags, specifically patterns like those in the VersionCop
configuration that target only main branch, version tags, and release branches
using anchored patterns (e.g., ^refs/heads/main$,
^refs/heads/v\\d+(?:\\.\\d+)?$) to provide defense-in-depth control over what is
considered a public release candidate.
In `@tools/nano/nanoFramework.Tool/Commands/DeployCommand.cs`:
- Around line 88-95: The FlashFirmware method call in the DeployCommand class
does not validate that an explicit target port has been provided before
attempting to flash firmware. Add a validation check before calling
FlashFirmware to ensure that settings.Port is not null or empty when
settings.NoFlash is false. If no port is specified when flash is enabled, log an
appropriate error message and return a failure result. Apply the same validation
fix to the other location mentioned in the comment (around line 151-159) where
similar flash operations occur.
- Around line 356-365: The code currently only handles the case where exactly
one .csproj file is found in the directory, but does not properly handle the
case when multiple .csproj files exist. When Directory.GetFiles(searchRoot,
"*.csproj") returns multiple projects (found.Length > 1), the code should fail
fast and require the user to explicitly specify the --project argument instead
of falling back to heuristic selection. Add an else if condition after the
found.Length == 1 check to detect when multiple projects exist and throw an
error or log a message instructing the user to use the --project argument. This
ensures deployment targets are explicitly specified and prevents accidentally
deploying the wrong application.
- Around line 418-441: The RunStreaming method calls proc.WaitForExit() without
any timeout, which can cause indefinite hanging if an external process gets
stuck. Modify the WaitForExit call to include a bounded timeout parameter
instead of waiting indefinitely. If the process does not exit within the timeout
period, add a cancellation path that kills the process using proc.Kill() and
returns an appropriate error exit code to indicate the timeout failure. This
ensures the CLI does not hang when external processes hang.
In `@tools/nano/nanoFramework.Tool/Commands/FlashCommand.cs`:
- Around line 25-28: The Validate() method is rejecting requests when Target is
null, which blocks the passthrough flow where users can specify --target via
remaining arguments after --. Move the target validation logic from the
Validate() method to the Execute() method, and modify the validation to return
Success() unconditionally, allowing the command to proceed to Execute(). In
Execute(), check if Target is set from typed options OR if passthrough arguments
are available (via context.Remaining.Raw or the MapArgs() method at lines
86-94), and only then report an error if neither source provides the required
--target argument.
In `@tools/nano/nanoFramework.Tool/Commands/WifiCommand.cs`:
- Around line 51-57: The current validation in the auth parameter check only
verifies that the auth value is one of the allowed options (WPA2, WPA, OPEN),
but does not enforce that a password is provided when WPA or WPA2 authentication
is selected. Add an additional validation check after the existing auth type
validation that returns a ValidationResult.Error if the auth value is either
"WPA" or "WPA2" and the password field is empty or null. This ensures that a
password is required for WPA/WPA2 authentication modes and fails early with a
clear error message before the configuration is used later.
In `@tools/nano/nanoFramework.Tool/ExternalTools/ToolManifest.cs`:
- Around line 55-67: In the LoadEmbedded method, replace the silent fallback
behavior where it returns an empty ToolManifest() when the manifest resource
name is null or when the stream is null. Instead, throw an appropriate exception
(such as InvalidOperationException) with a descriptive message indicating that
the embedded manifest is missing or cannot be loaded. This ensures the
application fails explicitly rather than silently degrading to an empty
manifest, which masks packaging errors and weakens the trust model for external
tool pin metadata.
In `@tools/nano/NuGet.Config`:
- Around line 3-5: Add a `<clear />` element at the beginning of the
`<packageSources>` section in NuGet.Config, before the existing `<add
key="NuGet"...` element. This clears any inherited package sources from user or
machine-level configurations, ensuring only the explicitly defined nuget.org
source is used during restore operations for better determinism and supply-chain
security.
In `@tools/nano/README.md`:
- Around line 10-17: The fenced code block in tools/nano/README.md starting at
line 10 is missing a language label, which violates markdownlint rule MD040. Add
the language identifier "text" to the opening triple backticks of the code block
that shows the directory structure (containing nanoFramework.Tool/, Program.cs,
Commands/, etc.). Change the opening fence from ``` to ```text to properly label
this fenced code block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0597ebc5-8500-4fec-919e-5ae1cc0078fe
📒 Files selected for processing (83)
README.mdREADME.zh-cn.mdazure-pipelines.ymlskills/nanoframework-sdk-migration/SKILL.mdskills/nanoframework-sdk-migration/references/contributing-compliance.mdskills/nanoframework-sdk-migration/references/migration-rules.mdtools/migrate/NanoMigrate.slntools/migrate/README.mdtools/migrate/nuget.configtools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cstools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cstools/migrate/src/NanoMigrate.Cli.Commands/MigrateRegistration.cstools/migrate/src/NanoMigrate.Cli.Commands/NanoMigrate.Cli.Commands.csprojtools/migrate/src/NanoMigrate.Cli.Commands/Rendering/ConsoleSupport.cstools/migrate/src/NanoMigrate.Cli.Commands/Rendering/MigrateRenderer.cstools/migrate/src/NanoMigrate.Cli.Commands/Rendering/MigrationReportBuilder.cstools/migrate/src/NanoMigrate.Cli.Commands/RollbackCommand.cstools/migrate/src/NanoMigrate.Cli/Cli/CloneCommand.cstools/migrate/src/NanoMigrate.Cli/Cli/FleetCommand.cstools/migrate/src/NanoMigrate.Cli/GitHub.cstools/migrate/src/NanoMigrate.Cli/NanoMigrate.Cli.csprojtools/migrate/src/NanoMigrate.Cli/ProcessRunner.cstools/migrate/src/NanoMigrate.Cli/Program.cstools/migrate/src/NanoMigrate.Cli/Rendering/FleetRenderer.cstools/migrate/src/NanoMigrate.Cli/UserError.cstools/migrate/src/NanoMigrate.Core/Backup/BackupCleaner.cstools/migrate/src/NanoMigrate.Core/Backup/MigrationJournaling.cstools/migrate/src/NanoMigrate.Core/Backup/RollbackJournal.cstools/migrate/src/NanoMigrate.Core/Common/Glob.cstools/migrate/src/NanoMigrate.Core/Common/ProjectScanner.cstools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cstools/migrate/src/NanoMigrate.Core/Fleet/RepoReport.cstools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csprojtools/migrate/src/NanoMigrate.Core/Projects/ConversionOptions.cstools/migrate/src/NanoMigrate.Core/Projects/ConvertResult.cstools/migrate/src/NanoMigrate.Core/Projects/IProjectConverter.cstools/migrate/src/NanoMigrate.Core/Projects/ProjectConverter.cstools/migrate/src/NanoMigrate.Core/Reporting/HtmlReportWriter.cstools/migrate/src/NanoMigrate.Core/Reporting/MarkdownReportWriter.cstools/migrate/src/NanoMigrate.Core/Reporting/MigrationReport.cstools/migrate/src/NanoMigrate.Core/Solutions/MigrationPlan.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionDiscovery.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionFile.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionRewriter.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cstools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cstools/migrate/src/NanoMigrate.Core/Verification/Verification.cstools/migrate/tests/NanoMigrate.Tests/CleanTests.cstools/migrate/tests/NanoMigrate.Tests/ConverterTests.cstools/migrate/tests/NanoMigrate.Tests/CpmTests.cstools/migrate/tests/NanoMigrate.Tests/GlobTests.cstools/migrate/tests/NanoMigrate.Tests/HintPathTests.cstools/migrate/tests/NanoMigrate.Tests/ItemGlobTests.cstools/migrate/tests/NanoMigrate.Tests/NanoMigrate.Tests.csprojtools/migrate/tests/NanoMigrate.Tests/OutputTypeTests.cstools/migrate/tests/NanoMigrate.Tests/PackageResolutionTests.cstools/migrate/tests/NanoMigrate.Tests/ProjectScannerTests.cstools/migrate/tests/NanoMigrate.Tests/ReportingTests.cstools/migrate/tests/NanoMigrate.Tests/RollbackTests.cstools/migrate/tests/NanoMigrate.Tests/SolutionRewriteTests.cstools/migrate/tests/NanoMigrate.Tests/SolutionTests.cstools/migrate/tests/NanoMigrate.Tests/TempDir.cstools/migrate/tests/NanoMigrate.Tests/VerifyTests.cstools/migrate/version.jsontools/nano/NuGet.Configtools/nano/README.mdtools/nano/nanoFramework.Tool.Tests/ExternalToolResolverTests.cstools/nano/nanoFramework.Tool.Tests/nanoFramework.Tool.Tests.csprojtools/nano/nanoFramework.Tool/Commands/DeployCommand.cstools/nano/nanoFramework.Tool/Commands/FlashCommand.cstools/nano/nanoFramework.Tool/Commands/PlaceholderCommand.cstools/nano/nanoFramework.Tool/Commands/WifiCommand.cstools/nano/nanoFramework.Tool/ExternalTools/ExternalToolBase.cstools/nano/nanoFramework.Tool/ExternalTools/ExternalToolResolver.cstools/nano/nanoFramework.Tool/ExternalTools/IExternalTool.cstools/nano/nanoFramework.Tool/ExternalTools/NanoffTool.cstools/nano/nanoFramework.Tool/ExternalTools/ToolEnvironment.cstools/nano/nanoFramework.Tool/ExternalTools/ToolManifest.cstools/nano/nanoFramework.Tool/ExternalTools/nano-tools.jsontools/nano/nanoFramework.Tool/Program.cstools/nano/nanoFramework.Tool/nanoFramework.Tool.csprojtools/nano/version.jsontools/nanoFramework.Tool.slnx
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
danielmeza
marked this pull request as ready for review
June 23, 2026 19:50
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tools/nano/README.md (1)
14-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
wificommand in the layout and command table.Line 14 and Lines 56-64 omit
wifi, buttools/nano/nanoFramework.Tool/Commands/WifiCommand.csindicates it is part of the CLI surface. This creates a docs/help mismatch.Suggested doc patch
- Commands/ # flash (nanoff) + deploy/monitor/devices placeholders + Commands/ # flash (nanoff), wifi, and deploy/monitor/devices placeholders @@ | `flash` | external | `nanoff` (prebuilt release, version-pinned) | +| `wifi` | built-in (in-proc) | configure device Wi-Fi settings on target hardware | | `deploy` | built-in | *placeholder — not yet implemented in the CLI* |Also applies to: 56-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/nano/README.md` around lines 14 - 15, The README.md documentation is missing the `wifi` command from both the layout description and the command reference table, even though WifiCommand.cs exists in the codebase as an actual CLI command. Update the Commands section layout description around lines 14-15 to include the wifi command alongside the existing flash, deploy, monitor, and devices commands, and then add a corresponding entry for the wifi command in the command table section around lines 56-64 with its description and usage information to match the actual CLI surface.tools/nano/nanoFramework.Tool/Commands/WifiCommand.cs (1)
219-223: 🩺 Stability & Availability | 🟠 MajorEscape dynamic error text before writing Spectre markup.
AnsiConsole.MarkupLine($"[red]error:[/] {message}")treatsmessageas markup. If an exception message contains[or], error rendering can throw and hide the original failure. Other error handlers in the codebase already useMarkup.Escape()for this reason.Suggested fix
private static int Error(string message) { - AnsiConsole.MarkupLine($"[red]error:[/] {message}"); + AnsiConsole.MarkupLine($"[red]error:[/] {Markup.Escape(message)}"); return 1; }Note: A similar unescaped usage exists in
DeployCommand.cs:446with the same pattern—consider fixing both in this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/nano/nanoFramework.Tool/Commands/WifiCommand.cs` around lines 219 - 223, The Error method in WifiCommand.cs passes the message parameter directly to AnsiConsole.MarkupLine without escaping, which means if the message contains markup characters like [ or ], it will be interpreted as markup and break rendering. Fix this by wrapping the message parameter with Markup.Escape() before including it in the MarkupLine call. Additionally, apply the same fix to the similar error handler in DeployCommand.cs at line 446 which has the identical pattern of unescaped message text being passed to AnsiConsole.MarkupLine.tools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cs (1)
75-78: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire explicit consent for non-interactive cleanup.
Line 77 auto-approves deletion when the console is non-interactive. For a destructive command, this should require
--yes; otherwise cleanup can run unintentionally and remove recovery artifacts.Suggested fix
private static bool Confirm(string question, bool assumeYes) { - if (assumeYes || !IsInteractive()) return true; + if (assumeYes) return true; + if (!IsInteractive()) return false; return AnsiConsole.Confirm(question, defaultValue: false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cs` around lines 75 - 78, The Confirm method currently auto-approves cleanup when the console is non-interactive (!IsInteractive()), regardless of whether assumeYes is set to true. For this destructive cleanup command, non-interactive mode should require explicit user consent via the assumeYes parameter. Modify the logic in the Confirm method so that if the console is not interactive and assumeYes is false, the method returns false to prevent unintended cleanup. The method should only proceed with AnsiConsole.Confirm when in interactive mode.tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs (1)
151-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not swallow cancellation in the generic exception handler.
catch (Exception)currently capturesOperationCanceledException. If cancellation occurs on the last repo, the method can return a partial report instead of honoring cancellation.Proposed fix
- catch (Exception ex) + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) { rr.Error = ex.Message; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs` around lines 151 - 154, The generic catch block for Exception is currently swallowing OperationCanceledException, which prevents proper cancellation propagation. Add a separate catch block for OperationCanceledException before the generic catch (Exception ex) block in the same try-catch structure, and re-throw the OperationCanceledException in that handler to ensure cancellation requests are properly honored rather than treated as regular errors that result in partial reports being returned.tools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cs (1)
67-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor cancellation in the “tool missing” path too.
When
dotnetis unavailable, this path can still iterate all targets even after cancellation is requested. Add cancellation checks before and during skip-result generation.Suggested patch
{ + cancellationToken.ThrowIfCancellationRequested(); var results = new List<BuildOutcome>(); if (!_runner.IsAvailable) { onSkippedToolMissing?.Invoke(); foreach (var t in targets) + { + cancellationToken.ThrowIfCancellationRequested(); results.Add(new BuildOutcome { Target = Path.GetFullPath(t), Skipped = true, ExitCode = -1, Message = "dotnet not found on PATH; verification skipped", }); + } return results; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cs` around lines 67 - 79, In the SolutionBuilder.cs file where the tool is unavailable (the !_runner.IsAvailable condition), the code generates skip results for all targets without respecting cancellation requests. Add a cancellation check before the foreach loop that iterates over targets, and also add a cancellation check inside the foreach loop before each BuildOutcome is added to the results list. This ensures that if cancellation is requested during the skip-result generation, the operation stops promptly rather than processing all targets regardless.tools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cs (1)
41-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNarrow exception handling when loading solutions.
This unconditional
catchsuppresses every exception type and can hide real defects inSolutionFile.Load, leading to silent data loss in discovery results. Catch only expected parse/I/O exceptions and let unexpected failures surface.Suggested patch
- try { result.Add(SolutionFile.Load(path)); } - catch { /* skip a solution we cannot parse */ } + try { result.Add(SolutionFile.Load(path)); } + catch (Exception ex) when ( + ex is IOException || + ex is UnauthorizedAccessException || + ex is InvalidDataException) + { + // skip a solution we cannot parse/read + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cs` around lines 41 - 42, The bare catch block in the SolutionFile.Load call suppresses all exception types indiscriminately, masking unexpected errors and making debugging difficult. Replace the unconditional catch statement with specific exception handling that catches only expected parse and I/O related exceptions (such as IOException, XmlException, or similar). This allows unexpected failures to properly surface while still skipping solutions that cannot be parsed due to expected errors. Keep the comment about skipping unparseable solutions but narrow the exception scope.
♻️ Duplicate comments (1)
tools/migrate/src/NanoMigrate.Core/Common/Glob.cs (1)
42-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
/**/translation still permits cross-segment false positives.The
"/**"branch emits(/.*)?, which makes the separator optional. That allows scoped patterns such assrc/**/Foo.nfprojto matchsrcFoo.nfprojwhen the optional group is skipped. SinceProjectScanner.ResolveProjectsuses this matcher to pick migration targets, this can include unintended projects.🐛 Proposed fix
case '/' when i + 2 < pattern.Length && pattern[i + 1] == '*' && pattern[i + 2] == '*': i += 2; // consume '/' is current; skip both '*' - if (i + 1 < pattern.Length && pattern[i + 1] == '/') i++; - sb.Append("(/.*)?"); + if (i + 1 < pattern.Length && pattern[i + 1] == '/') + { + i++; + sb.Append("/(?:.*/)?"); + } + else + { + sb.Append("(?:/.*)?"); + } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/migrate/src/NanoMigrate.Core/Common/Glob.cs` around lines 42 - 46, The regex pattern emitted for the `/**/` wildcard in the case statement is `(/.*)?` which makes the separator optional, allowing false positive matches where the slash is skipped entirely. In the method handling the `'/'` case when followed by `**`, change the pattern from `(/.*)?` to `(/.*)` (removing the `?` quantifier) to make the path separator mandatory rather than optional, ensuring that patterns like `src/**/Foo.nfproj` only match paths with proper segment boundaries and not `srcFoo.nfproj`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cs`:
- Around line 263-264: The bare catch block and the catch (Exception ex) block
in the conversion loops are swallowing OperationCanceledException, which
prevents graceful termination when users cancel via Ctrl+C. In both catch blocks
(the one at line 264 and the one at line 499), add a check to detect if the
caught exception is an OperationCanceledException and re-throw it immediately,
allowing other exceptions to be handled normally with the existing error/skip
behavior. This ensures cancellation requests propagate up and terminate the
batch gracefully instead of continuing with normal error handling.
In `@tools/migrate/src/NanoMigrate.Cli/NanoMigrate.Cli.csproj`:
- Line 5: There is a version mismatch between the project files and
documentation. The TargetFramework in NanoMigrate.Cli.csproj (and related
projects NanoMigrate.Cli.Commands, NanoMigrate.Core, and NanoMigrate.Tests) is
set to net10.0, but the README files state the requirement is .NET 8 SDK with
example paths showing net8.0. Either revert the TargetFramework back to net8.0
in all the migrate CLI project files to match the documented baseline, or update
the README files (tools/migrate/README.md and tools/nano/README.md) to change
the .NET 8 SDK requirement references to .NET 10 SDK and update any net8.0
example paths to net10.0. Choose one approach to ensure consistency between code
and documentation.
In `@tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs`:
- Around line 136-137: The _git.Run("add -A", repo, cancellationToken) call's
result is not being checked for success, so if the git staging operation fails,
the code continues to attempt WriteCommitMessage and commit operations, which
masks the real issue. Capture the return value or exit code from the _git.Run
call for "add -A" and add error handling to validate that staging succeeded
before proceeding to WriteCommitMessage and subsequent commit operations. If the
git add fails, throw an appropriate exception or return an error to prevent
misleading error messages downstream.
In `@tools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csproj`:
- Line 4: The TargetFramework element in NanoMigrate.Core.csproj is set to
net10.0 but the README documentation specifies .NET 8 as the baseline
requirement. Update the TargetFramework element from net10.0 to net8.0 in all
three project files (NanoMigrate.Core.csproj, NanoMigrate.Cli.csproj, and
NanoMigrate.Cli.Commands.csproj) to match the documented baseline, or
alternatively update the README to specify .NET 10 as the requirement if that is
the intended baseline. Choose one approach to ensure consistency between the
code and documentation.
In `@tools/nano/nanoFramework.Tool.Tests/nanoFramework.Tool.Tests.csproj`:
- Line 4: The TargetFramework in nanoFramework.Tool.Tests.csproj is set to
net10.0, but the project documentation in tools/nano/README.md specifies that
the .NET 8 SDK is required. Update the TargetFramework element from net10.0 to
net8.0 in nanoFramework.Tool.Tests.csproj, and also update the same
TargetFramework element in nanoFramework.Tool.csproj to match. This will align
the project configuration with the documented SDK requirement and ensure
consistency across both project files.
In `@tools/nano/nanoFramework.Tool/nanoFramework.Tool.csproj`:
- Line 5: Update the TargetFramework element in the nanoFramework.Tool.csproj
file from net10.0 to net8.0 to align with the documented .NET 8 SDK requirement
in the README, or alternatively update the documentation to reflect the actual
net10.0 requirement. Choose one approach to ensure consistency between the code
requirement and user-facing documentation.
---
Outside diff comments:
In `@tools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cs`:
- Around line 75-78: The Confirm method currently auto-approves cleanup when the
console is non-interactive (!IsInteractive()), regardless of whether assumeYes
is set to true. For this destructive cleanup command, non-interactive mode
should require explicit user consent via the assumeYes parameter. Modify the
logic in the Confirm method so that if the console is not interactive and
assumeYes is false, the method returns false to prevent unintended cleanup. The
method should only proceed with AnsiConsole.Confirm when in interactive mode.
In `@tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs`:
- Around line 151-154: The generic catch block for Exception is currently
swallowing OperationCanceledException, which prevents proper cancellation
propagation. Add a separate catch block for OperationCanceledException before
the generic catch (Exception ex) block in the same try-catch structure, and
re-throw the OperationCanceledException in that handler to ensure cancellation
requests are properly honored rather than treated as regular errors that result
in partial reports being returned.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cs`:
- Around line 41-42: The bare catch block in the SolutionFile.Load call
suppresses all exception types indiscriminately, masking unexpected errors and
making debugging difficult. Replace the unconditional catch statement with
specific exception handling that catches only expected parse and I/O related
exceptions (such as IOException, XmlException, or similar). This allows
unexpected failures to properly surface while still skipping solutions that
cannot be parsed due to expected errors. Keep the comment about skipping
unparseable solutions but narrow the exception scope.
In `@tools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cs`:
- Around line 67-79: In the SolutionBuilder.cs file where the tool is
unavailable (the !_runner.IsAvailable condition), the code generates skip
results for all targets without respecting cancellation requests. Add a
cancellation check before the foreach loop that iterates over targets, and also
add a cancellation check inside the foreach loop before each BuildOutcome is
added to the results list. This ensures that if cancellation is requested during
the skip-result generation, the operation stops promptly rather than processing
all targets regardless.
In `@tools/nano/nanoFramework.Tool/Commands/WifiCommand.cs`:
- Around line 219-223: The Error method in WifiCommand.cs passes the message
parameter directly to AnsiConsole.MarkupLine without escaping, which means if
the message contains markup characters like [ or ], it will be interpreted as
markup and break rendering. Fix this by wrapping the message parameter with
Markup.Escape() before including it in the MarkupLine call. Additionally, apply
the same fix to the similar error handler in DeployCommand.cs at line 446 which
has the identical pattern of unescaped message text being passed to
AnsiConsole.MarkupLine.
In `@tools/nano/README.md`:
- Around line 14-15: The README.md documentation is missing the `wifi` command
from both the layout description and the command reference table, even though
WifiCommand.cs exists in the codebase as an actual CLI command. Update the
Commands section layout description around lines 14-15 to include the wifi
command alongside the existing flash, deploy, monitor, and devices commands, and
then add a corresponding entry for the wifi command in the command table section
around lines 56-64 with its description and usage information to match the
actual CLI surface.
---
Duplicate comments:
In `@tools/migrate/src/NanoMigrate.Core/Common/Glob.cs`:
- Around line 42-46: The regex pattern emitted for the `/**/` wildcard in the
case statement is `(/.*)?` which makes the separator optional, allowing false
positive matches where the slash is skipped entirely. In the method handling the
`'/'` case when followed by `**`, change the pattern from `(/.*)?` to `(/.*)`
(removing the `?` quantifier) to make the path separator mandatory rather than
optional, ensuring that patterns like `src/**/Foo.nfproj` only match paths with
proper segment boundaries and not `srcFoo.nfproj`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90112621-d4f4-4f2d-a400-cfc6eeee771e
📒 Files selected for processing (79)
azure-pipelines.ymlskills/nanoframework-sdk-migration/SKILL.mdtools/migrate/README.mdtools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cstools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cstools/migrate/src/NanoMigrate.Cli.Commands/MigrateRegistration.cstools/migrate/src/NanoMigrate.Cli.Commands/NanoMigrate.Cli.Commands.csprojtools/migrate/src/NanoMigrate.Cli.Commands/Rendering/ConsoleSupport.cstools/migrate/src/NanoMigrate.Cli.Commands/Rendering/MigrateRenderer.cstools/migrate/src/NanoMigrate.Cli.Commands/Rendering/MigrationReportBuilder.cstools/migrate/src/NanoMigrate.Cli.Commands/RollbackCommand.cstools/migrate/src/NanoMigrate.Cli/Cli/CloneCommand.cstools/migrate/src/NanoMigrate.Cli/Cli/FleetCommand.cstools/migrate/src/NanoMigrate.Cli/GitHub.cstools/migrate/src/NanoMigrate.Cli/NanoMigrate.Cli.csprojtools/migrate/src/NanoMigrate.Cli/ProcessRunner.cstools/migrate/src/NanoMigrate.Cli/Program.cstools/migrate/src/NanoMigrate.Cli/Rendering/FleetRenderer.cstools/migrate/src/NanoMigrate.Cli/UserError.cstools/migrate/src/NanoMigrate.Core/Backup/BackupCleaner.cstools/migrate/src/NanoMigrate.Core/Backup/MigrationJournaling.cstools/migrate/src/NanoMigrate.Core/Backup/RollbackJournal.cstools/migrate/src/NanoMigrate.Core/Common/Glob.cstools/migrate/src/NanoMigrate.Core/Common/ProcessExec.cstools/migrate/src/NanoMigrate.Core/Common/ProjectScanner.cstools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cstools/migrate/src/NanoMigrate.Core/Fleet/RepoReport.cstools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csprojtools/migrate/src/NanoMigrate.Core/Projects/ConversionOptions.cstools/migrate/src/NanoMigrate.Core/Projects/ConvertResult.cstools/migrate/src/NanoMigrate.Core/Projects/IProjectConverter.cstools/migrate/src/NanoMigrate.Core/Projects/ProjectConverter.cstools/migrate/src/NanoMigrate.Core/Reporting/HtmlReportWriter.cstools/migrate/src/NanoMigrate.Core/Reporting/MarkdownReportWriter.cstools/migrate/src/NanoMigrate.Core/Reporting/MigrationReport.cstools/migrate/src/NanoMigrate.Core/Solutions/MigrationPlan.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionDiscovery.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionFile.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionRewriter.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cstools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cstools/migrate/src/NanoMigrate.Core/Verification/Verification.cstools/migrate/tests/NanoMigrate.Tests/CleanTests.cstools/migrate/tests/NanoMigrate.Tests/ConverterTests.cstools/migrate/tests/NanoMigrate.Tests/CpmTests.cstools/migrate/tests/NanoMigrate.Tests/GlobTests.cstools/migrate/tests/NanoMigrate.Tests/HintPathTests.cstools/migrate/tests/NanoMigrate.Tests/ItemGlobTests.cstools/migrate/tests/NanoMigrate.Tests/NanoMigrate.Tests.csprojtools/migrate/tests/NanoMigrate.Tests/OutputTypeTests.cstools/migrate/tests/NanoMigrate.Tests/PackageResolutionTests.cstools/migrate/tests/NanoMigrate.Tests/ProjectScannerTests.cstools/migrate/tests/NanoMigrate.Tests/ReportingTests.cstools/migrate/tests/NanoMigrate.Tests/RollbackTests.cstools/migrate/tests/NanoMigrate.Tests/SolutionRewriteTests.cstools/migrate/tests/NanoMigrate.Tests/SolutionTests.cstools/migrate/tests/NanoMigrate.Tests/TempDir.cstools/migrate/tests/NanoMigrate.Tests/VerifyTests.cstools/migrate/version.jsontools/nano/NuGet.Configtools/nano/README.mdtools/nano/nanoFramework.Tool.Tests/AuthModeTests.cstools/nano/nanoFramework.Tool.Tests/ExternalToolResolverTests.cstools/nano/nanoFramework.Tool.Tests/nanoFramework.Tool.Tests.csprojtools/nano/nanoFramework.Tool/Cli/SmartEnumTypeConverter.cstools/nano/nanoFramework.Tool/Commands/AuthMode.cstools/nano/nanoFramework.Tool/Commands/DeployCommand.cstools/nano/nanoFramework.Tool/Commands/FlashCommand.cstools/nano/nanoFramework.Tool/Commands/PlaceholderCommand.cstools/nano/nanoFramework.Tool/Commands/WifiCommand.cstools/nano/nanoFramework.Tool/ExternalTools/ExternalToolBase.cstools/nano/nanoFramework.Tool/ExternalTools/ExternalToolResolver.cstools/nano/nanoFramework.Tool/ExternalTools/IExternalTool.cstools/nano/nanoFramework.Tool/ExternalTools/NanoffTool.cstools/nano/nanoFramework.Tool/ExternalTools/ToolEnvironment.cstools/nano/nanoFramework.Tool/ExternalTools/ToolManifest.cstools/nano/nanoFramework.Tool/Program.cstools/nano/nanoFramework.Tool/nanoFramework.Tool.csprojtools/nano/version.json
Comment on lines
+263
to
+264
| try { preview = _converter.Convert(nf, dryOpts, _cancellationToken); } | ||
| catch { continue; } // a project that fails to analyse is converted (and reported) normally; just not journaled |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the relevant code sections
cat -n tools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cs | sed -n '260,270p'Repository: nanoframework/nf-tools
Length of output: 733
🏁 Script executed:
# Also check the second location (lines 497-501)
cat -n tools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cs | sed -n '495,505p'Repository: nanoframework/nf-tools
Length of output: 615
Re-throw OperationCanceledException in conversion loops to respect user cancellation.
The bare catch at line 264 and the catch (Exception ex) at line 499 both swallow OperationCanceledException. When a user cancels via Ctrl+C, the cancellation token raises this exception, but these broad handlers convert it into normal error/skip behavior and allow the batch to continue instead of terminating gracefully.
Suggested fix
try { preview = _converter.Convert(nf, dryOpts, _cancellationToken); }
+catch (OperationCanceledException) { throw; }
catch { continue; } // a project that fails to analyse is converted (and reported) normally; just not journaled try
{
result = _converter.Convert(nf, o, _cancellationToken);
}
+catch (OperationCanceledException) { throw; }
catch (Exception ex)
{
result = new ConvertResult { OutputPath = nf, Error = ex.Message };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { preview = _converter.Convert(nf, dryOpts, _cancellationToken); } | |
| catch { continue; } // a project that fails to analyse is converted (and reported) normally; just not journaled | |
| try { preview = _converter.Convert(nf, dryOpts, _cancellationToken); } | |
| catch (OperationCanceledException) { throw; } | |
| catch { continue; } // a project that fails to analyse is converted (and reported) normally; just not journaled |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cs` around lines
263 - 264, The bare catch block and the catch (Exception ex) block in the
conversion loops are swallowing OperationCanceledException, which prevents
graceful termination when users cancel via Ctrl+C. In both catch blocks (the one
at line 264 and the one at line 499), add a check to detect if the caught
exception is an OperationCanceledException and re-throw it immediately, allowing
other exceptions to be handled normally with the existing error/skip behavior.
This ensures cancellation requests propagate up and terminate the batch
gracefully instead of continuing with normal error handling.
Comment on lines
+136
to
+137
| _git.Run("add -A", repo, cancellationToken); | ||
| var msgFile = WriteCommitMessage(o); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle git add failures before attempting commit.
The git add -A exit code is ignored. If staging fails, the flow still attempts commit and can report the wrong root cause.
Proposed fix
if (o.Commit && !conv.DryRun)
{
- _git.Run("add -A", repo, cancellationToken);
+ var add = _git.Run("add -A", repo, cancellationToken);
+ if (add.Code != 0)
+ {
+ rr.Error = $"git add failed: {add.Stderr.Trim()}";
+ report.Add(rr);
+ continue;
+ }
var msgFile = WriteCommitMessage(o);
var signOff = o.SignOff ? "-s " : "";
var commit = _git.Run($"commit {signOff}-F \"{msgFile}\"", repo, cancellationToken);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _git.Run("add -A", repo, cancellationToken); | |
| var msgFile = WriteCommitMessage(o); | |
| if (o.Commit && !conv.DryRun) | |
| { | |
| var add = _git.Run("add -A", repo, cancellationToken); | |
| if (add.Code != 0) | |
| { | |
| rr.Error = $"git add failed: {add.Stderr.Trim()}"; | |
| report.Add(rr); | |
| continue; | |
| } | |
| var msgFile = WriteCommitMessage(o); | |
| var signOff = o.SignOff ? "-s " : ""; | |
| var commit = _git.Run($"commit {signOff}-F \"{msgFile}\"", repo, cancellationToken); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs` around lines 136 -
137, The _git.Run("add -A", repo, cancellationToken) call's result is not being
checked for success, so if the git staging operation fails, the code continues
to attempt WriteCommitMessage and commit operations, which masks the real issue.
Capture the return value or exit code from the _git.Run call for "add -A" and
add error handling to validate that staging succeeded before proceeding to
WriteCommitMessage and subsequent commit operations. If the git add fails, throw
an appropriate exception or return an error to prevent misleading error messages
downstream.
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net10.0</TargetFramework> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify migrate target frameworks across projects
fd -e csproj tools/migrate | xargs rg -n "<TargetFramework>|<TargetFrameworks>"
# Verify docs/pipeline statements about .NET baseline
rg -n "target .*\\.NET 8|\\.NET 8|net8\\.0|net10\\.0" README.md README.zh-cn.md tools/migrate/README.md azure-pipelines.ymlRepository: nanoframework/nf-tools
Length of output: 3013
🏁 Script executed:
# Check for any PR or commit context
git log --oneline -20 -- tools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csproj
# Look for any configuration files that might specify a baseline
fd -type f -name "*.yml" -o -name "*.yaml" -o -name "*.json" | head -20
# Check azure-pipelines.yml for any baseline references
cat azure-pipelines.yml | grep -A5 -B5 "net8\|net10\|NET 8\|NET 10" | head -50
# Check if there are any global property configurations
fd -type f \( -name "Directory.Build.props" -o -name "global.json" -o -name "*.props" \) | head -10Repository: nanoframework/nf-tools
Length of output: 558
🏁 Script executed:
# Look at the migrate README more closely for context on the baseline
sed -n '50,80p' tools/migrate/README.mdRepository: nanoframework/nf-tools
Length of output: 1142
🏁 Script executed:
# Check git history for when net10.0 was set in these files
git log -p --follow -- tools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csproj | head -150
# Look for global baseline configuration
cat global.json 2>/dev/null || echo "No global.json found"
# Check if there's a Directory.Build.props that might have baseline
find . -name "Directory.Build.props" -exec cat {} \;Repository: nanoframework/nf-tools
Length of output: 181
🏁 Script executed:
# Search for any issue or PR description references in commits
git log --all --oneline | grep -i "migrate\|net8\|net10\|baseline" | head -20Repository: nanoframework/nf-tools
Length of output: 491
🏁 Script executed:
# Get details on the merge commit that brought in the migrate tools
git show d2125e4 --stat | head -100
# Check what was in the add-nano-migrate-tools branch
git log d2125e4 --not main --oneline 2>/dev/null | head -20
# Get the actual current state of the csproj file
cat tools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csprojRepository: nanoframework/nf-tools
Length of output: 8549
Update target framework or documentation to match the stated baseline.
Line 4 specifies net10.0, but the README states the tool requires the .NET 8 SDK with examples showing net8.0 output paths. This mismatch will cause users to use the wrong SDK version when building. Either update the target framework to net8.0 to match the documented baseline, or update the README to reflect the .NET 10 requirement.
This affects all migrate tool projects: NanoMigrate.Core, NanoMigrate.Cli, and NanoMigrate.Cli.Commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csproj` at line 4, The
TargetFramework element in NanoMigrate.Core.csproj is set to net10.0 but the
README documentation specifies .NET 8 as the baseline requirement. Update the
TargetFramework element from net10.0 to net8.0 in all three project files
(NanoMigrate.Core.csproj, NanoMigrate.Cli.csproj, and
NanoMigrate.Cli.Commands.csproj) to match the documented baseline, or
alternatively update the README to specify .NET 10 as the requirement if that is
the intended baseline. Choose one approach to ensure consistency between the
code and documentation.
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net10.0</TargetFramework> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Target frameworks under tools/nano =="
rg -n --glob 'tools/nano/**/*.csproj' '<TargetFramework>|<TargetFrameworks>' tools/nano
echo
echo "== SDK pinning/config files =="
fd -HI 'global.json|Directory.Build.props|Directory.Build.targets|azure-pipelines.yml' .
echo
echo "== Pipeline SDK selection hints =="
rg -n 'UseDotNet|dotnet-version|8\.0|10\.0|global\.json' azure-pipelines.ymlRepository: nanoframework/nf-tools
Length of output: 513
🏁 Script executed:
# Read the azure-pipelines.yml to see SDK configuration
cat azure-pipelines.ymlRepository: nanoframework/nf-tools
Length of output: 21886
🏁 Script executed:
# Search for documentation files that might describe SDK requirements
fd -HI 'README|CONTRIBUTING|\.md$' tools/nano | head -20Repository: nanoframework/nf-tools
Length of output: 181
🏁 Script executed:
# Check for global.json at repository root and in tools/nano
find . -maxdepth 3 -name 'global.json' -type fRepository: nanoframework/nf-tools
Length of output: 160
🏁 Script executed:
# Look for any .NET 8 references in documentation
rg -i 'net\s*8|\.net\s*8|sdk.*8' --type md --type txtRepository: nanoframework/nf-tools
Length of output: 160
🏁 Script executed:
# Read the README to check what SDK requirement it documents
cat tools/nano/README.mdRepository: nanoframework/nf-tools
Length of output: 5537
Align .NET target framework with documented SDK requirement.
The tools/nano/README.md explicitly states "Requires the .NET 8 SDK", but both nanoFramework.Tool.csproj and nanoFramework.Tool.Tests.csproj target net10.0. This creates a mismatch between documentation and implementation that can cause contributor friction and CI surprises. Either update the projects to target net8.0 (or use multi-targeting for broader compatibility), or update the documentation to reflect the net10.0 requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/nano/nanoFramework.Tool.Tests/nanoFramework.Tool.Tests.csproj` at line
4, The TargetFramework in nanoFramework.Tool.Tests.csproj is set to net10.0, but
the project documentation in tools/nano/README.md specifies that the .NET 8 SDK
is required. Update the TargetFramework element from net10.0 to net8.0 in
nanoFramework.Tool.Tests.csproj, and also update the same TargetFramework
element in nanoFramework.Tool.csproj to match. This will align the project
configuration with the documented SDK requirement and ensure consistency across
both project files.
josesimoes
requested changes
Jun 26, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
@danielmeza could you please split this? Let's start with the migration tool which, as discussed in Discord, should be offered as a stand-alone .NET tool.
Author
On it! |
danielmeza
changed the title
Author
|
Done — split as requested. This PR now carries only the standalone |
Move the `dotnet nano` umbrella CLI and the NanoMigrate (`nano-migrate`) project migrator out of the nanoFramework.Sdk repository and into this tools monorepo, so the SDK repository carries only the MSBuild project SDK. - Add tools/nano (nanoFramework.Tool) and tools/migrate (NanoMigrate.Core / .Cli.Commands / .Cli), with the combined nanoFramework.Tool.slnx. - Add the companion SDK-migration skill under skills/. - Register both tools in the Azure Pipelines build: change detection, build, pack, test, sign, and push jobs, plus the build-failure report. - Add per-tool version.json (Nerdbank.GitVersioning) and NuGet.Config to match the other tools, MIT license headers on all sources, and the new tool entries in the README (English and 简体中文).
Build, pack, test, and sign still run on every build; pushing the packages to NuGet now requires the pipeline variable PublishNanoTools=true, so the preview tool packages are not published until they are ready for release.
…ce casing, net10 - Make the GitHub repo enumeration async (ListOrgReposAsync) and turn the clone command into an AsyncCommand, replacing the blocking GetAwaiter().GetResult(). - Fix the process-stream deadlock in ProcessRunner and the build verifier: drain stdout/stderr concurrently and add a generous per-process timeout. Use named local functions for the data handlers so they are detached afterwards. - Recase the project namespaces from NanoFramework.* to nanoFramework.* to match the brand and the package ids (external NanoFrameworkDevices and the legacy NanoFrameworkProjectSystemPath MSBuild property are left untouched). - Target net10.0. - Correctness/doc fixes from the review: BackupCleaner no longer over-reports removals, the migrate registration doc references the right parameter, the clone output directory is resolved to an absolute path, version.json uses a proper publicReleaseRefSpec, the nano NuGet.Config clears inherited sources, and the README directory listings get a language hint. - Gate package publishing behind PublishNanoTools (already pushed earlier).
…ss spawn - Thread Spectre's CancellationToken from each command into the long-running Core operations: project conversion, build verification, the fleet repo/project loop and its git calls, clean, and rollback, plus the clone loop. Ctrl+C / console close now kills the in-flight git/dotnet child and stops the run promptly; cancellation maps to exit code 130. - Extract the process spawn/drain/cancel/timeout logic into a single ProcessExec helper (Core/Common). ProcessRunner, the dotnet build runner, and the dotnet availability probe now delegate to it, removing three copies of the concurrent-read + kill-on-cancel pattern.
…, wifi validation - Glob: "**/" now ends on a directory boundary, so "**/Foo.nfproj" no longer matches "MyFoo.nfproj" (it matched any characters before). Add regression cases. - FleetService: validate the branch name (reject option-smuggling / unsafe chars) before it is interpolated into `git checkout -B`, and create the report's parent directory so a nested --report path does not fail the write. - SolutionScanner: enumerate with IgnoreInaccessible so one permission-denied subtree does not abort solution discovery. - WifiCommand: require --password for WPA/WPA2. - ReportingTests: assert the migrate command exits 0, not just that a report exists. - SKILL.md: hyphenate "legacy-flavored" and tag the help code block as bash.
Previously ApplyAndCleanup deleted the backup set directory unconditionally, even when Apply reported per-file problems, discarding the originals needed for a retry or manual recovery. Only delete the set on a fully clean revert.
- Replace the raw `--auth` string with an AuthMode SmartEnum (OPEN / WPA / WPA2) whose members carry their own behaviour: RequiresPassword and the device-side authentication + encryption pair. The command no longer re-parses the string in Validate or Execute. - Add a reusable SmartEnumTypeConverter<TEnum> so Spectre.Console.Cli binds the option to the typed value at parse time (case-insensitive by name), rejecting unknown values with the list of allowed ones. - Add the Ardalis.SmartEnum dependency, and tests for the converter and behaviour.
Each ReportFormat value now owns its file extensions and its writer, so FromPath keys the format off the extension and Write renders through the value — replacing the two switches in MigrationReportBuilder. Adding a format (e.g. JSON) becomes a new value rather than another switch arm. Adjust the reporting test to compare by name (a SmartEnum instance is not a valid InlineData constant).
Per the maintainer request on the PR, the tools are split. This PR now carries only the standalone nano-migrate tool — NanoMigrate.Core (engine), NanoMigrate.Cli.Commands, and NanoMigrate.Cli (packed as the nanoFramework.Migrate global tool) — plus the companion migration skill. The dotnet nano umbrella (tools/nano) and the combined solution are removed, along with its Build_Nano pipeline job, the BUILD_NANO change detection, the README rows, and the umbrella-only dependencies. The umbrella work is preserved on the nano-umbrella-wip branch for a separate, later pull request.
josesimoes
force-pushed
the
add-nano-migrate-tools
branch
from
June 28, 2026 23:18
c7a45f3 to
d3f5854
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/nanoframework-sdk-migration/SKILL.md`:
- Around line 19-31: Update the NanoMigrate skill text to remove the obsolete
umbrella `dotnet nano` surface and `tools/nano/README.md` references, since this
branch only supports the standalone migrate tool. In the `SKILL.md` guidance
around the command reference and help examples, replace all `dotnet nano ...`
usage with `nano-migrate ...` or the direct source invocation via `dotnet run
--project tools/migrate/src/NanoMigrate.Cli -- ...`, and keep the
source-of-truth reference pointed at the migrate tool docs.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cs`:
- Around line 75-78: The Confirm method currently falls back to true when
IsInteractive() is false, which auto-approves destructive cleanup in
non-interactive/redirected runs. Update CleanCommand.Confirm to fail closed by
returning false unless assumeYes is explicitly set, and keep the interactive
AnsiConsole.Confirm path only for interactive sessions. Make the behavior in
Confirm and any callers of CleanCommand rely on --yes for non-interactive
execution.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/RollbackCommand.cs`:
- Around line 33-51: RollbackCommand currently mixes two different journal
lookups: it previews metadata from RollbackJournal.FindLatest(root) but applies
RollbackJournal.ApplyAndCleanup using manifestPath from
RollbackJournal.ManifestPaths(root).FirstOrDefault(), so the confirmed migration
may differ from the one executed. Update RollbackCommand.Execute to resolve a
single manifest once and reuse that same manifest for both the displayed summary
and the rollback application, keeping the selection path consistent through the
confirm flow and ApplyAndCleanup call.
In `@tools/migrate/src/NanoMigrate.Cli/GitHub.cs`:
- Around line 25-26: The GitHub org repository listing in GitHub.cs is
hard-coded to use public-only visibility, so authenticated runs still miss
private repos. Update the URL construction in the org repo fetch logic to make
visibility depend on authentication, using all accessible repositories when a
token is present (or expose visibility as an explicit option), and keep the
behavior clear in the GitHub repository discovery flow.
In `@tools/migrate/src/NanoMigrate.Cli/Program.cs`:
- Around line 66-68: The `Program` CLI example for `CloneCommand` is advertising
a GitHub PAT on the command line, which should be avoided. Update the
`WithExample` on the `clone` command to demonstrate a safer token flow, such as
using an environment variable or interactive prompt, and remove any `--token
...` usage from the example while keeping the rest of the `CloneCommand` help
text intact.
In `@tools/migrate/src/NanoMigrate.Cli/Rendering/FleetRenderer.cs`:
- Line 29: The error-line rendering in FleetRenderer should normalize Windows
CRLF before displaying the first line, since using rr.Error.Split('\n')[0] can
leave a trailing carriage return that breaks Spectre output. Update the note
construction in the FleetRenderer logic and the other affected rendering spot(s)
so the first error line is trimmed/normalized before Esc(...) is applied,
preserving clean table/tree formatting across newline styles.
In `@tools/migrate/src/NanoMigrate.Core/Backup/BackupCleaner.cs`:
- Around line 128-142: The SafeEnumerateFiles and SafeEnumerateDirectories
helpers in BackupCleaner currently swallow any recursive enumeration failure and
return an empty set, which can wipe out the entire Plan() scan. Update these
helpers to skip only inaccessible subtrees by using IgnoreInaccessible or
equivalent branch-level handling during the
Directory.EnumerateFiles/EnumerateDirectories traversal, while still returning
the accessible results. Keep the behavior localized to the SafeEnumerateFiles
and SafeEnumerateDirectories methods so one bad folder does not cancel discovery
of other backups or rollback directories.
In `@tools/migrate/src/NanoMigrate.Core/Backup/MigrationJournaling.cs`:
- Around line 54-67: The createsNewFile assignment in MigrationJournaling should
be simplified because the Any() check already returns a boolean and the current
? true : false ternary is redundant. Update the logic in the code path that
computes createsNewFile from preview.DeletedFiles so it directly assigns the
negated Any(...) result, preserving the existing Path.GetFullPath and
StringComparison.OrdinalIgnoreCase comparison while removing the unnecessary
conditional.
In `@tools/migrate/src/NanoMigrate.Core/Common/Glob.cs`:
- Around line 78-79: The glob-to-regex निर्माण in the regex-building helper
currently returns a Regex without any timeout, so user-supplied --glob input can
create unbounded matching. Update the Regex construction in the glob compiler
method to use the timeout overload with a reasonable fixed timeout, keeping the
existing IgnoreCase and CultureInvariant options intact. Reference the
regex-building path in Glob.cs so the timeout is applied wherever the
user-provided glob pattern is converted to a Regex.
In `@tools/migrate/src/NanoMigrate.Core/Common/ProcessExec.cs`:
- Around line 61-65: The timeout branch in ProcessExec.Execute kills the process
tree and returns immediately, which can miss trailing async output still being
flushed. Update the timeout path in Execute to mirror the normal completion flow
by waiting after p.Kill(entireProcessTree: true) before reading so and returning
the timeout tuple. Keep the change localized to the timeout handling around
WaitForExit/Kill so the method consistently drains output before exit.
In `@tools/migrate/src/NanoMigrate.Core/Common/ProjectScanner.cs`:
- Around line 38-46: NfprojUnder() is currently non-deterministic and throws
when repoDir is missing. Update ProjectScanner.NfprojUnder to mirror
ResolveProjects(): return an empty sequence if the directory does not exist, and
collect the matched .nfproj paths, sort them, then yield in stable order. Keep
the glob filtering behavior intact while using the existing NfprojUnder and
ResolveProjects patterns to locate the fix.
In `@tools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cs`:
- Around line 137-140: The temporary commit-message file created by
WriteCommitMessage in FleetService should always be cleaned up even if _git.Run
fails or cancellation occurs. Wrap the commit execution and File.Delete(msgFile)
in a try/finally so the cleanup runs regardless of success, and keep the fix
localized around the commit/sign-off flow in FleetService.
In `@tools/migrate/src/NanoMigrate.Core/Projects/ProjectConverter.cs`:
- Around line 566-570: The recursive directory scans in ProjectConverter’s
AddSolutionsIn and the other all-directories traversal should not abort on
unreadable subtrees; update the Directory.EnumerateFiles usage to ignore
inaccessible folders by using EnumerationOptions with IgnoreInaccessible
enabled. Keep the change localized to the scan helpers so both the solution
discovery path and the other recursive scan continue past permission errors
instead of failing the migration.
In `@tools/migrate/src/NanoMigrate.Core/Reporting/MarkdownReportWriter.cs`:
- Around line 67-72: The manual-review section in MarkdownReportWriter writes
RelativePath and Review items directly, so HTML-sensitive text is not escaped
there. Update the ManualReview rendering path in MarkdownReportWriter to escape
content before appending it, using the same protection approach as Cell() or
equivalent for both headings and bullet items. Make sure the fix covers the
flagged loop that builds the “Manual review” section and any other related
Markdown appenders in the same class that emit raw user content.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/MigrationPlan.cs`:
- Around line 187-200: The MigrationPlan branch in the selection logic can
produce DirectoryWithSolutions with an empty Candidates list after filtering
solutions via NanoProjects(), which leaves no valid choice and skips the
loose-directory fallback. Update the logic around MigrationPlan and its
Candidates construction so the filtered candidates are materialized first, then
return LooseDirectory when that list is empty; otherwise return
DirectoryWithSolutions with the non-empty candidates.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/SolutionRewriter.cs`:
- Around line 100-107: The SolutionRewriter logic in the project path matching
step is using the raw solution-relative value from project.FilePath, so
backslash-separated .sln entries can fail the converted.Contains(abs) check on
Unix-like systems. Normalize rel to OS separators before combining or comparing,
and make the matching in SolutionRewriter consistent so classic .sln entries
resolve correctly and still flip to .csproj.
In `@tools/migrate/tests/NanoMigrate.Tests/VerifyTests.cs`:
- Around line 123-127: The test in
Real_build_passes_for_a_buildable_project_and_fails_for_a_broken_one should not
silently return when DotnetBuildRunner.IsAvailable is false. Update the
VerifyTests path to use an explicit skip for the unavailable dotnet
prerequisite, with a clear skip reason, so the test is reported as skipped
rather than passing. Keep the logic centered around DotnetBuildRunner and the
existing Fact test method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11746b7f-5d02-4d93-892a-97ecb16deada
📒 Files selected for processing (65)
README.mdREADME.zh-cn.mdazure-pipelines.ymlskills/nanoframework-sdk-migration/SKILL.mdskills/nanoframework-sdk-migration/references/contributing-compliance.mdskills/nanoframework-sdk-migration/references/migration-rules.mdtools/migrate/NanoMigrate.slntools/migrate/README.mdtools/migrate/nuget.configtools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cstools/migrate/src/NanoMigrate.Cli.Commands/MigrateCommand.cstools/migrate/src/NanoMigrate.Cli.Commands/MigrateRegistration.cstools/migrate/src/NanoMigrate.Cli.Commands/NanoMigrate.Cli.Commands.csprojtools/migrate/src/NanoMigrate.Cli.Commands/Rendering/ConsoleSupport.cstools/migrate/src/NanoMigrate.Cli.Commands/Rendering/MigrateRenderer.cstools/migrate/src/NanoMigrate.Cli.Commands/Rendering/MigrationReportBuilder.cstools/migrate/src/NanoMigrate.Cli.Commands/RollbackCommand.cstools/migrate/src/NanoMigrate.Cli/Cli/CloneCommand.cstools/migrate/src/NanoMigrate.Cli/Cli/FleetCommand.cstools/migrate/src/NanoMigrate.Cli/GitHub.cstools/migrate/src/NanoMigrate.Cli/NanoMigrate.Cli.csprojtools/migrate/src/NanoMigrate.Cli/ProcessRunner.cstools/migrate/src/NanoMigrate.Cli/Program.cstools/migrate/src/NanoMigrate.Cli/Rendering/FleetRenderer.cstools/migrate/src/NanoMigrate.Cli/UserError.cstools/migrate/src/NanoMigrate.Core/Backup/BackupCleaner.cstools/migrate/src/NanoMigrate.Core/Backup/MigrationJournaling.cstools/migrate/src/NanoMigrate.Core/Backup/RollbackJournal.cstools/migrate/src/NanoMigrate.Core/Common/Glob.cstools/migrate/src/NanoMigrate.Core/Common/ProcessExec.cstools/migrate/src/NanoMigrate.Core/Common/ProjectScanner.cstools/migrate/src/NanoMigrate.Core/Fleet/FleetService.cstools/migrate/src/NanoMigrate.Core/Fleet/RepoReport.cstools/migrate/src/NanoMigrate.Core/NanoMigrate.Core.csprojtools/migrate/src/NanoMigrate.Core/Projects/ConversionOptions.cstools/migrate/src/NanoMigrate.Core/Projects/ConvertResult.cstools/migrate/src/NanoMigrate.Core/Projects/IProjectConverter.cstools/migrate/src/NanoMigrate.Core/Projects/ProjectConverter.cstools/migrate/src/NanoMigrate.Core/Reporting/HtmlReportWriter.cstools/migrate/src/NanoMigrate.Core/Reporting/MarkdownReportWriter.cstools/migrate/src/NanoMigrate.Core/Reporting/MigrationReport.cstools/migrate/src/NanoMigrate.Core/Solutions/MigrationPlan.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionDiscovery.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionFile.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionRewriter.cstools/migrate/src/NanoMigrate.Core/Solutions/SolutionScanner.cstools/migrate/src/NanoMigrate.Core/Verification/SolutionBuilder.cstools/migrate/src/NanoMigrate.Core/Verification/Verification.cstools/migrate/tests/NanoMigrate.Tests/CleanTests.cstools/migrate/tests/NanoMigrate.Tests/ConverterTests.cstools/migrate/tests/NanoMigrate.Tests/CpmTests.cstools/migrate/tests/NanoMigrate.Tests/GlobTests.cstools/migrate/tests/NanoMigrate.Tests/HintPathTests.cstools/migrate/tests/NanoMigrate.Tests/ItemGlobTests.cstools/migrate/tests/NanoMigrate.Tests/NanoMigrate.Tests.csprojtools/migrate/tests/NanoMigrate.Tests/OutputTypeTests.cstools/migrate/tests/NanoMigrate.Tests/PackageResolutionTests.cstools/migrate/tests/NanoMigrate.Tests/ProjectScannerTests.cstools/migrate/tests/NanoMigrate.Tests/ReportingTests.cstools/migrate/tests/NanoMigrate.Tests/RollbackTests.cstools/migrate/tests/NanoMigrate.Tests/SolutionRewriteTests.cstools/migrate/tests/NanoMigrate.Tests/SolutionTests.cstools/migrate/tests/NanoMigrate.Tests/TempDir.cstools/migrate/tests/NanoMigrate.Tests/VerifyTests.cstools/migrate/version.json
Comment on lines
+19
to
+31
| **NanoMigrate** tool (in this repo at `tools/migrate`, surfaced as `dotnet nano migrate`). The | ||
| tool is **idempotent + reentrant**: it skips already-SDK-style projects and re-running over a tree | ||
| is a safe no-op, so a partial or repeated migration is never destructive. | ||
|
|
||
| The full, as-built command/option reference is [`tools/migrate/README.md`](../../tools/migrate/README.md) | ||
| (and [`tools/nano/README.md`](../../tools/nano/README.md) for the umbrella). It is the source of | ||
| truth; this skill is the workflow guide. **Always confirm the live surface with `--help`** — it | ||
| matches the installed version exactly: | ||
|
|
||
| ```bash | ||
| dotnet nano --help # every command | ||
| dotnet nano migrate --help # options for one command | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stale dotnet nano umbrella surface from this skill.
This guide still tells readers to use dotnet nano ... and points them at tools/nano/README.md, but this PR was explicitly split down to the standalone nano-migrate tool. Following these instructions on this branch will fail before migration even starts. Update the text/examples to use only nano-migrate ... (or dotnet run --project tools/migrate/src/NanoMigrate.Cli -- ... from source).
Also applies to: 43-56
🧰 Tools
🪛 SkillSpector (2.2.3)
[error] 56: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 80: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/nanoframework-sdk-migration/SKILL.md` around lines 19 - 31, Update the
NanoMigrate skill text to remove the obsolete umbrella `dotnet nano` surface and
`tools/nano/README.md` references, since this branch only supports the
standalone migrate tool. In the `SKILL.md` guidance around the command reference
and help examples, replace all `dotnet nano ...` usage with `nano-migrate ...`
or the direct source invocation via `dotnet run --project
tools/migrate/src/NanoMigrate.Cli -- ...`, and keep the source-of-truth
reference pointed at the migrate tool docs.
Comment on lines
+75
to
+78
| private static bool Confirm(string question, bool assumeYes) | ||
| { | ||
| if (assumeYes || !IsInteractive()) return true; | ||
| return AnsiConsole.Confirm(question, defaultValue: false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed in non-interactive mode.
Line 77 auto-accepts destructive cleanup whenever prompting is unavailable. Because IsInteractive() drops to false on redirected output, even nano-migrate clean >clean.log will delete leftovers without confirmation. Require --yes instead of silently proceeding.
Proposed fix
private static bool Confirm(string question, bool assumeYes)
{
- if (assumeYes || !IsInteractive()) return true;
+ if (assumeYes) return true;
+ if (!IsInteractive())
+ throw new UserError("Refusing to delete in non-interactive mode without --yes.");
return AnsiConsole.Confirm(question, defaultValue: false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static bool Confirm(string question, bool assumeYes) | |
| { | |
| if (assumeYes || !IsInteractive()) return true; | |
| return AnsiConsole.Confirm(question, defaultValue: false); | |
| private static bool Confirm(string question, bool assumeYes) | |
| { | |
| if (assumeYes) return true; | |
| if (!IsInteractive()) | |
| throw new UserError("Refusing to delete in non-interactive mode without --yes."); | |
| return AnsiConsole.Confirm(question, defaultValue: false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/CleanCommand.cs` around lines 75 -
78, The Confirm method currently falls back to true when IsInteractive() is
false, which auto-approves destructive cleanup in non-interactive/redirected
runs. Update CleanCommand.Confirm to fail closed by returning false unless
assumeYes is explicitly set, and keep the interactive AnsiConsole.Confirm path
only for interactive sessions. Make the behavior in Confirm and any callers of
CleanCommand rely on --yes for non-interactive execution.
Comment on lines
+33
to
+51
| var manifestPath = RollbackJournal.ManifestPaths(root).FirstOrDefault(); | ||
| if (manifestPath is null) | ||
| { | ||
| AnsiConsole.MarkupLine($"[grey]nothing to roll back under '{Esc(root)}' (no migration journal found).[/]"); | ||
| return 0; | ||
| } | ||
|
|
||
| var manifest = RollbackJournal.FindLatest(root); | ||
| var count = manifest?.Entries.Count ?? 0; | ||
| AnsiConsole.MarkupLine( | ||
| $"Found a recorded migration ([blue]{Esc(manifest?.Id ?? "?")}[/]) with [bold]{count}[/] action(s)."); | ||
|
|
||
| if (!Confirm("Roll back the last recorded migration?", settings.AssumeYes)) | ||
| { | ||
| AnsiConsole.MarkupLine("[grey]aborted; nothing reverted.[/]"); | ||
| return 0; | ||
| } | ||
|
|
||
| var result = RollbackJournal.ApplyAndCleanup(manifestPath, cancellationToken); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one manifest selection path for both preview and apply.
Lines 33-51 check one journal path, display metadata from another lookup, then apply rollback to the original manifestPath. Those are not guaranteed to identify the same migration, so the command can confirm one rollback and execute another.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Cli.Commands/RollbackCommand.cs` around lines
33 - 51, RollbackCommand currently mixes two different journal lookups: it
previews metadata from RollbackJournal.FindLatest(root) but applies
RollbackJournal.ApplyAndCleanup using manifestPath from
RollbackJournal.ManifestPaths(root).FirstOrDefault(), so the confirmed migration
may differ from the one executed. Update RollbackCommand.Execute to resolve a
single manifest once and reuse that same manifest for both the displayed summary
and the rollback application, keeping the selection path consistent through the
confirm flow and ApplyAndCleanup call.
Comment on lines
+25
to
+26
| var url = $"https://api.github.com/orgs/{org}/repos?per_page=100&page={page}&type=public"; | ||
| using var resp = await http.GetAsync(url, ct); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--token still can't discover private repositories.
Line 25 hard-codes type=public, so authenticated org members will silently miss private repos during org-wide clone/fleet runs. If this tool is meant to cover the whole accessible fleet, switch to type=all when authenticated or make visibility explicit.
Proposed fix
- var url = $"https://api.github.com/orgs/{org}/repos?per_page=100&page={page}&type=public";
+ var visibility = string.IsNullOrEmpty(token) ? "public" : "all";
+ var url = $"https://api.github.com/orgs/{org}/repos?per_page=100&page={page}&type={visibility}";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var url = $"https://api.github.com/orgs/{org}/repos?per_page=100&page={page}&type=public"; | |
| using var resp = await http.GetAsync(url, ct); | |
| var visibility = string.IsNullOrEmpty(token) ? "public" : "all"; | |
| var url = $"https://api.github.com/orgs/{org}/repos?per_page=100&page={page}&type={visibility}"; | |
| using var resp = await http.GetAsync(url, ct); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Cli/GitHub.cs` around lines 25 - 26, The GitHub
org repository listing in GitHub.cs is hard-coded to use public-only visibility,
so authenticated runs still miss private repos. Update the URL construction in
the org repo fetch logic to make visibility depend on authentication, using all
accessible repositories when a token is present (or expose visibility as an
explicit option), and keep the behavior clear in the GitHub repository discovery
flow.
Comment on lines
+66
to
+68
| config.AddCommand<CloneCommand>("clone") | ||
| .WithDescription("Clone all matching repos from a GitHub org.") | ||
| .WithExample("clone", "./nano-repos", "--token", "$GITHUB_TOKEN"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not advertise PATs on the command line.
This help example encourages passing the GitHub token via argv. On most systems that exposes the secret through shell history, process listings, and sometimes CI logs. Prefer an env-var/prompt flow in the example instead of --token ....
Suggested change
config.AddCommand<CloneCommand>("clone")
.WithDescription("Clone all matching repos from a GitHub org.")
- .WithExample("clone", "./nano-repos", "--token", "$GITHUB_TOKEN");
+ .WithExample("clone", "./nano-repos");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| config.AddCommand<CloneCommand>("clone") | |
| .WithDescription("Clone all matching repos from a GitHub org.") | |
| .WithExample("clone", "./nano-repos", "--token", "$GITHUB_TOKEN"); | |
| config.AddCommand<CloneCommand>("clone") | |
| .WithDescription("Clone all matching repos from a GitHub org.") | |
| .WithExample("clone", "./nano-repos"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Cli/Program.cs` around lines 66 - 68, The
`Program` CLI example for `CloneCommand` is advertising a GitHub PAT on the
command line, which should be avoided. Update the `WithExample` on the `clone`
command to demonstrate a safer token flow, such as using an environment variable
or interactive prompt, and remove any `--token ...` usage from the example while
keeping the rest of the `CloneCommand` help text intact.
Comment on lines
+566
to
+570
| void AddSolutionsIn(string d, SearchOption option) | ||
| { | ||
| foreach (var pattern in new[] { "*.sln", "*.slnx" }) | ||
| foreach (var sln in Directory.EnumerateFiles(d, pattern, option)) | ||
| found.Add(Path.GetFullPath(sln)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Skip inaccessible directories in both recursive scans (ProjectConverter.cs:566-570, 683-688).
Directory.EnumerateFiles(..., SearchOption.AllDirectories) will abort the scan when it hits an unreadable subtree, so one bad directory can stop the migration. Use EnumerationOptions.IgnoreInaccessible = true to keep the repo traversal going.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Core/Projects/ProjectConverter.cs` around lines
566 - 570, The recursive directory scans in ProjectConverter’s AddSolutionsIn
and the other all-directories traversal should not abort on unreadable subtrees;
update the Directory.EnumerateFiles usage to ignore inaccessible folders by
using EnumerationOptions with IgnoreInaccessible enabled. Keep the change
localized to the scan helpers so both the solution discovery path and the other
recursive scan continue past permission errors instead of failing the migration.
Comment on lines
+67
to
+72
| sb.Append("## Manual review\n\n"); | ||
| foreach (var p in flagged) | ||
| { | ||
| sb.Append("### ").Append(p.RelativePath).Append('\n').Append('\n'); | ||
| foreach (var item in p.Review) | ||
| sb.Append("- ").Append(item).Append('\n'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escape HTML-sensitive text before writing Markdown.
Cell() only protects table structure, and the manual-review section bypasses it entirely. If a path or review item contains < or & (which your own ReportingTests fixture already does), many Markdown renderers will interpret that as HTML and the report will show the wrong content.
Proposed fix
- sb.Append("### ").Append(p.RelativePath).Append('\n').Append('\n');
+ sb.Append("### ").Append(EscapeText(p.RelativePath)).Append('\n').Append('\n');
foreach (var item in p.Review)
- sb.Append("- ").Append(item).Append('\n');
+ sb.Append("- ").Append(EscapeText(item)).Append('\n');
sb.Append('\n');
}
}
@@
- private static string Cell(string s) =>
- s.Replace("\\", "\\\\")
+ private static string Cell(string s) =>
+ EscapeText(s).Replace("\\", "\\\\")
.Replace("|", "\\|")
.Replace("\r\n", "<br>")
.Replace("\n", "<br>")
.Replace("\r", "<br>");
+
+ private static string EscapeText(string s) =>
+ s.Replace("&", "&")
+ .Replace("<", "<")
+ .Replace(">", ">");Also applies to: 126-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Core/Reporting/MarkdownReportWriter.cs` around
lines 67 - 72, The manual-review section in MarkdownReportWriter writes
RelativePath and Review items directly, so HTML-sensitive text is not escaped
there. Update the ManualReview rendering path in MarkdownReportWriter to escape
content before appending it, using the same protection approach as Cell() or
equivalent for both headings and bullet items. Make sure the fix covers the
flagged loop that builds the “Manual review” section and any other related
Markdown appenders in the same class that emit raw user content.
Comment on lines
+187
to
+200
| // NO GLOB: 0 solutions => loose directory; >=1 => the user chooses. | ||
| if (solutions.Count == 0) | ||
| { | ||
| var all = ProjectScanner.ResolveProjects(dir, glob: null); | ||
| return new MigrationPlan { Kind = PlanKind.LooseDirectory, LooseProjects = all }; | ||
| } | ||
|
|
||
| return new MigrationPlan | ||
| { | ||
| Kind = PlanKind.DirectoryWithSolutions, | ||
| Candidates = solutions | ||
| .Where(s => s.NanoProjects().Count > 0) // only solutions with .nfproj to convert | ||
| .Select(s => new SolutionCandidate { Solution = s, NanoProjects = s.NanoProjects() }) | ||
| .ToList(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not return DirectoryWithSolutions with zero candidates.
This branch keys off solutions.Count, but Candidates is filtered afterward. If a repo has solution files but none of them reference .nfproj, RequiresSelection becomes true with an empty list, so the CLI has no valid selection to present and loose projects in the directory are ignored.
Proposed fix
- return new MigrationPlan
- {
- Kind = PlanKind.DirectoryWithSolutions,
- Candidates = solutions
- .Where(s => s.NanoProjects().Count > 0) // only solutions with .nfproj to convert
- .Select(s => new SolutionCandidate { Solution = s, NanoProjects = s.NanoProjects() })
- .ToList(),
- };
+ var candidates = solutions
+ .Select(s => new SolutionCandidate { Solution = s, NanoProjects = s.NanoProjects() })
+ .Where(c => c.NanoProjects.Count > 0)
+ .ToList();
+
+ if (candidates.Count == 0)
+ {
+ var all = ProjectScanner.ResolveProjects(dir, glob: null);
+ return new MigrationPlan { Kind = PlanKind.LooseDirectory, LooseProjects = all };
+ }
+
+ return new MigrationPlan
+ {
+ Kind = PlanKind.DirectoryWithSolutions,
+ Candidates = candidates,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // NO GLOB: 0 solutions => loose directory; >=1 => the user chooses. | |
| if (solutions.Count == 0) | |
| { | |
| var all = ProjectScanner.ResolveProjects(dir, glob: null); | |
| return new MigrationPlan { Kind = PlanKind.LooseDirectory, LooseProjects = all }; | |
| } | |
| return new MigrationPlan | |
| { | |
| Kind = PlanKind.DirectoryWithSolutions, | |
| Candidates = solutions | |
| .Where(s => s.NanoProjects().Count > 0) // only solutions with .nfproj to convert | |
| .Select(s => new SolutionCandidate { Solution = s, NanoProjects = s.NanoProjects() }) | |
| .ToList(), | |
| // NO GLOB: 0 solutions => loose directory; >=1 => the user chooses. | |
| if (solutions.Count == 0) | |
| { | |
| var all = ProjectScanner.ResolveProjects(dir, glob: null); | |
| return new MigrationPlan { Kind = PlanKind.LooseDirectory, LooseProjects = all }; | |
| } | |
| var candidates = solutions | |
| .Select(s => new SolutionCandidate { Solution = s, NanoProjects = s.NanoProjects() }) | |
| .Where(c => c.NanoProjects.Count > 0) | |
| .ToList(); | |
| if (candidates.Count == 0) | |
| { | |
| var all = ProjectScanner.ResolveProjects(dir, glob: null); | |
| return new MigrationPlan { Kind = PlanKind.LooseDirectory, LooseProjects = all }; | |
| } | |
| return new MigrationPlan | |
| { | |
| Kind = PlanKind.DirectoryWithSolutions, | |
| Candidates = candidates, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/MigrationPlan.cs` around lines
187 - 200, The MigrationPlan branch in the selection logic can produce
DirectoryWithSolutions with an empty Candidates list after filtering solutions
via NanoProjects(), which leaves no valid choice and skips the loose-directory
fallback. Update the logic around MigrationPlan and its Candidates construction
so the filtered candidates are materialized first, then return LooseDirectory
when that list is empty; otherwise return DirectoryWithSolutions with the
non-empty candidates.
Comment on lines
+100
to
+107
| var rel = project.FilePath; | ||
| if (!rel.EndsWith(".nfproj", StringComparison.OrdinalIgnoreCase)) continue; | ||
|
|
||
| var abs = Path.GetFullPath(Path.Combine(solutionDir, rel)); | ||
| if (!converted.Contains(abs)) continue; | ||
|
|
||
| project.FilePath = Path.ChangeExtension(rel, ".csproj"); | ||
| try { project.Type = isXml ? string.Empty : CsprojTypeGuid; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize solution-relative separators before matching converted projects.
project.FilePath comes from the solution text, so classic .sln entries can still use backslashes on Linux/macOS. Path.Combine(solutionDir, rel) does not normalize those on Unix, which means the converted.Contains(abs) check misses and the solution entry never flips to .csproj.
Proposed fix
- var abs = Path.GetFullPath(Path.Combine(solutionDir, rel));
+ var normalizedRel = rel.Replace('\\', Path.DirectorySeparatorChar)
+ .Replace('/', Path.DirectorySeparatorChar);
+ var abs = Path.GetFullPath(Path.Combine(solutionDir, normalizedRel));
if (!converted.Contains(abs)) continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var rel = project.FilePath; | |
| if (!rel.EndsWith(".nfproj", StringComparison.OrdinalIgnoreCase)) continue; | |
| var abs = Path.GetFullPath(Path.Combine(solutionDir, rel)); | |
| if (!converted.Contains(abs)) continue; | |
| project.FilePath = Path.ChangeExtension(rel, ".csproj"); | |
| try { project.Type = isXml ? string.Empty : CsprojTypeGuid; } | |
| var rel = project.FilePath; | |
| if (!rel.EndsWith(".nfproj", StringComparison.OrdinalIgnoreCase)) continue; | |
| var normalizedRel = rel.Replace('\\', Path.DirectorySeparatorChar) | |
| .Replace('/', Path.DirectorySeparatorChar); | |
| var abs = Path.GetFullPath(Path.Combine(solutionDir, normalizedRel)); | |
| if (!converted.Contains(abs)) continue; | |
| project.FilePath = Path.ChangeExtension(rel, ".csproj"); | |
| try { project.Type = isXml ? string.Empty : CsprojTypeGuid; } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/src/NanoMigrate.Core/Solutions/SolutionRewriter.cs` around
lines 100 - 107, The SolutionRewriter logic in the project path matching step is
using the raw solution-relative value from project.FilePath, so
backslash-separated .sln entries can fail the converted.Contains(abs) check on
Unix-like systems. Normalize rel to OS separators before combining or comparing,
and make the matching in SolutionRewriter consistent so classic .sln entries
resolve correctly and still flip to .csproj.
Comment on lines
+123
to
+127
| [Fact] | ||
| public void Real_build_passes_for_a_buildable_project_and_fails_for_a_broken_one() | ||
| { | ||
| var runner = new DotnetBuildRunner(); | ||
| if (!runner.IsAvailable) return; // no dotnet → nothing to assert (tolerated) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an explicit skip when dotnet is unavailable. Returning here makes the test pass, so CI can’t tell the difference between a real pass and an unavailable prerequisite; mark it skipped with a reason instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/tests/NanoMigrate.Tests/VerifyTests.cs` around lines 123 - 127,
The test in Real_build_passes_for_a_buildable_project_and_fails_for_a_broken_one
should not silently return when DotnetBuildRunner.IsAvailable is false. Update
the VerifyTests path to use an explicit skip for the unavailable dotnet
prerequisite, with a clear skip reason, so the test is reported as skipped
rather than passing. Keep the logic centered around DotnetBuildRunner and the
existing Fact test method.
josesimoes
reviewed
Jun 28, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
This is rather complex.
I would have left out features like git commit.
As they are there, just better leave them there.
Reviewed most of the code. Skipped unit tests.
Please address coderabbitat commets.
| // deletions, .sln edits); otherwise it shows the count of review flags. | ||
| private static string BuildNotesCell(ConvertResult result, bool dryRun) | ||
| { | ||
| if (result.Status == ConvertStatus.Error) |
Member
There was a problem hiding this comment.
Per project code style:add open/close brakets
| if (dryRun) | ||
| { | ||
| lines.Add($"[grey]→[/] {Esc(Path.GetFileName(result.OutputPath))}"); | ||
| foreach (var d in result.DeletedFiles) |
| var tree = new Tree("[bold]Affected solution(s)[/]"); | ||
| foreach (var c in candidates) | ||
| { | ||
| var fmt = c.Solution.Format == SolutionFormat.Xml ? "slnx" : "sln"; |
Member
There was a problem hiding this comment.
Please use meaningful names for vars. No point using abreviations.
| AnsiConsole.WriteLine(); | ||
| } | ||
|
|
||
| // The solutions actually retargeted by a real run (grouped notice). Pure |
| } | ||
|
|
||
| // The solutions actually retargeted by a real run (grouped notice). Pure | ||
| // presentation over the rewrite results. |
Member
There was a problem hiding this comment.
If these are public they need an IntelliSense comment. If not please change visibility.
|
|
||
| // Named local functions (not lambdas) so the handlers can be detached afterwards, | ||
| // leaving no reference from the Process to the captured builders. | ||
| var so = new StringBuilder(); |
Member
There was a problem hiding this comment.
Please name variables to meaningful names.
| public string Ext { get; init; } = ".csproj"; | ||
|
|
||
| /// <summary>Target framework moniker written into the emitted project.</summary> | ||
| public string Tfm { get; init; } = "netnano1.0"; |
| /// to convert. Null means "all <c>.nfproj</c> recursively" (the default). | ||
| /// Supports <c>*</c>, <c>**</c> and <c>?</c>. | ||
| /// </summary> | ||
| public string? Glob { get; init; } |
| <PackageId>nanoFramework.Migrate.Core</PackageId> | ||
| <Title>nanoFramework Migrate (Core)</Title> | ||
| <Description>Conversion engine that migrates legacy nanoFramework .nfproj projects to the SDK-style MSBuild project system: folds packages.config into PackageReference and .nuspec metadata into Pack properties. Console-free library; see the nano-migrate CLI for an interactive front end.</Description> | ||
| <Authors>nanoFramework</Authors> |
Member
There was a problem hiding this comment.
Author is nanoframework all small caps. Required for proper package attribution and . NET Foundation checking.
| <Title>nanoFramework Migrate (Core)</Title> | ||
| <Description>Conversion engine that migrates legacy nanoFramework .nfproj projects to the SDK-style MSBuild project system: folds packages.config into PackageReference and .nuspec metadata into Pack properties. Console-free library; see the nano-migrate CLI for an interactive front end.</Description> | ||
| <Authors>nanoFramework</Authors> | ||
| <Company>nanoFramework</Company> |
Member
There was a problem hiding this comment.
No company needed
Suggested change
| <Company>nanoFramework</Company> |
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
3 participants
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.
Description
nano-migratestand-alone .NET tool (nanoFramework.Migrate) undertools/migrate:NanoMigrate.Core(the conversion engine),NanoMigrate.Cli.Commands(the shared Spectre commands), andNanoMigrate.Cli(thenano-migrateglobal tool). It converts legacy.nfprojprojects to SDK-style, for a single repository or across an entire cloned fleet.skills/.version.json(Nerdbank.GitVersioning) andNuGet.Config, MIT license headers on all sources, and a README entry in English and Simplified Chinese.Motivation and Context
dotnet nanoumbrella was split out of this pull request and will follow separately.How Has This Been Tested?
dotnet buildoftools/migrate/NanoMigrate.slncompletes with no warnings or errors (Release).dotnet packproduces thenanoFramework.Migrate,nanoFramework.Migrate.Cli.Commands, andnanoFramework.Migrate.Corepackages.Types of changes
Checklist: