Add maui ai command group for AI-assisted development bootstrapping#98
Add maui ai command group for AI-assisted development bootstrapping#98jfversluis wants to merge 32 commits into
maui ai command group for AI-assisted development bootstrapping#98Conversation
New top-level `maui ai` command group with 5 subcommands: - `maui ai detect agent environments, fetch skills frominit` marketplace, install + configure MCP in one command - `maui ai show available skills from marketplacelist` - `maui ai show installed vs available skillsstatus` - `maui ai sync installed skills to latest versionsupdate` - `maui ai add < install a specific skill by nameskill>` Architecture: - MarketplaceClient: HTTP fetch marketplace.json/plugin.json, enumerate skills via GitHub Trees API, download files - AgentEnvironmentDetector: detect Claude Code, VS Code, Copilot CLI, OpenCode environments - McpConfigurator: merge maui-devflow MCP server entry into each environment's config file - SkillInstaller + SkillVersionStore: download skills and track versions via .skill-version files - AiJsonContext: AOT-compatible source-generated JSON serialization All commands support --json, --ci, --dry-run, --force modes. Supports GITHUB_TOKEN env var for authenticated API access. Closes #97 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new top-level maui ai command group to bootstrap and manage AI-agent “skills” for MAUI development by fetching skill definitions from the repo marketplace and installing them into detected agent environments, plus wiring up MCP configuration.
Changes:
- Register new
maui aicommand group in the CLI root command. - Implement
init/list/status/update/addcommands plus supporting service layer (MarketplaceClient, environment detection, skill install/version tracking, MCP config merge). - Add unit test coverage for command construction, marketplace frontmatter parsing, version store, environment detection, and MCP config behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/Microsoft.Maui.Cli/Program.cs | Registers maui ai in the root command. |
| src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.cs | Defines the ai command group and shared options/HTTP client creation. |
| src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.Init.cs | Implements maui ai init (fetch marketplace, detect envs, select/install skills, configure MCP). |
| src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.List.cs | Implements maui ai list (marketplace listing). |
| src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.Status.cs | Implements maui ai status (installed skills + optional update check). |
| src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.Update.cs | Implements maui ai update (update installed skills to latest). |
| src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.Add.cs | Implements maui ai add <skill> (install one skill). |
| src/Cli/Microsoft.Maui.Cli/Ai/AgentEnvironmentDetector.cs | Detects Claude/VS Code/OpenCode in repo + Copilot CLI in user profile. |
| src/Cli/Microsoft.Maui.Cli/Ai/MarketplaceClient.cs | Fetches marketplace/plugin manifests, enumerates skills, downloads files, parses SKILL.md frontmatter. |
| src/Cli/Microsoft.Maui.Cli/Ai/McpConfigurator.cs | Merges maui-devflow MCP server entry into environment config JSON. |
| src/Cli/Microsoft.Maui.Cli/Ai/SkillInstaller.cs | Downloads skill files and writes .skill-version metadata. |
| src/Cli/Microsoft.Maui.Cli/Ai/SkillVersionStore.cs | Reads/writes .skill-version JSON (including legacy format). |
| src/Cli/Microsoft.Maui.Cli/Ai/AiJsonContext.cs | Source-generated JSON serialization context for AOT friendliness. |
| src/Cli/Microsoft.Maui.Cli/Ai/Models/*.cs | POCO models for manifests, skill info, environment info, version metadata. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/AiCommandsTests.cs | Tests AI command construction and option/alias wiring. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/AgentEnvironmentDetectorTests.cs | Tests environment detection behavior and paths. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/MarketplaceClientTests.cs | Tests SKILL.md frontmatter parsing. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/McpConfiguratorTests.cs | Tests MCP config creation/merge/idempotency across environments. |
| src/Cli/Microsoft.Maui.Cli.UnitTests/SkillVersionStoreTests.cs | Tests .skill-version read/write/legacy behavior. |
Security fixes: - Path traversal validation in DownloadSkillFilesAsync - Skill name sanitization (reject path chars and '..') 'as JsonObject' (prevents crash on non-object JSON) - Guard .skill-version write when 0 files downloaded Spec compliance: - Add Owner field to MarketplaceManifest (per marketplace.json spec) - Add Agents and LspServers fields to PluginManifest (per plugin.json spec) - Add StringOrArrayConverter for skills/agents (spec allows both) - Register MarketplaceOwner in AiJsonContext for AOT UX improvements: - Next-steps guidance after init completes - 'Installed' column in 'maui ai list' output - Proper singular/plural for 'environment' - Hide --repo/--branch options (developer-only) - MCP restart reminder after configuration - --force now skips env creation prompt (same as --ci) - Safe DateTime.TryParse for version timestamps Robustness: - CancellationToken propagated through all async service methods - Removed unused DefaultMarketplacePath constant Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- FetchStringAsync/FetchBytesAsync: only swallow real HTTP timeouts, let user cancellation (Ctrl+C) propagate correctly - StringOrArrayConverter: add reader.Skip() for non-string array elements (nested objects/arrays/nulls) to prevent reader corruption - SkillInstaller: return -1 for invalid skill names; callers now distinguish failure from 'already installed' skip proper singular/plural - McpConfigurator: catch UnauthorizedAccessException - Add StringOrArrayConverterTests (9 tests) and path traversal tests (3) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Cache GitHub tree fetch across plugins (single API call per branch) - Use projectRoot to anchor relative skill directories - De-duplicate updates when VS Code and Copilot CLI share .github/skills - Don't trigger reinstall when remote SHA is unavailable (null != outdated) - FetchStringAsync/FetchBytesAsync: return null only for 404, propagate other HTTP errors for meaningful caller messages Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix tab indentation in Init.cs and Update.cs - SkillInstaller returns -2 for download failures (distinct from -1 invalid name, 0 already installed) - Callers show 'check your network connection' for -2 - Add SkillInstallerTests (3 tests: invalid names, valid name) - Add McpConfigurator corrupted JSON test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Move dry-run check before confirmation prompt in Init.cs - Remove no-op AddChoices(Array.Empty) call - Fix misleading comment in FetchStringAsync Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- NormalizePath: collapse middle './' segments (e.g. 'plugins/dotnet-maui/./skills') which prevented skill discovery when plugin.json uses './skills/' paths - ParseFrontmatter: handle YAML block scalar indicators (>-, >, |, etc.) by reading indented continuation lines for multi-line descriptions Both bugs found during local end-to-end testing against the live marketplace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lity - AgentEnvironmentDetector: git root comparison now case-insensitive (Windows paths are case-insensitive, C:\Users vs c:\users) - MarketplaceClient: path traversal check now case-insensitive (prevents false rejections on Windows with mixed-case paths) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
End-to-end tests without network calls using MockHttpMessageHandler: - Environment detection (Claude + VsCode) with path verification - SkillVersionStore round-trip with all fields - MCP config: Claude schema, OpenCode schema, existing entries preserved, idempotent re-runs - YAML block scalar parsing (>- multi-line descriptions) - Real SKILL.md frontmatter parsing - NormalizePath middle './' handling - Full install simulation with mock HTTP - Dry-run skip when version exists - Invalid skill name rejection (../malicious) All 12 tests pass offline in <0.6s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- FetchStringAsync/FetchBytesAsync: remove HttpRequestException swallow, only return null for 404; other errors propagate to command handlers - Status --check-updates: show 'Unknown' instead of 'Up to date' when remote SHA unavailable - Update: track uncheckable skills, warn user, return exit 1 in CI mode - McpConfigurator: return false when existing config keys are wrong type instead of silently overwriting - SkillInstallerTests: replace real HttpClient with mock handler Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Code Review: 8 findings posted inline (1 critical, 5 moderate, 2 minor). See the lean summary comment for methodology and discarded findings.
Generated by Expert Code Review · ● 36.3M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 3 findings posted inline. See the lean summary comment for full details.
Generated by Expert Code Review · ● 34.2M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 4 findings posted inline (1 critical, 2 moderate, 1 minor). See the lean summary comment for full methodology and discarded findings.
Generated by Expert Code Review · ● 29.5M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review — maui ai command group
Reviewed all 30 files in this PR for regressions, security issues, bugs, data loss, and race conditions. Overall this is well-structured code with strong security defenses (path traversal guards, atomic writes, symlink detection). Below are the findings.
🟡 MODERATE — TOCTOU race in RepositoryAssetInstaller.InstallAssetAsync
File: src/Cli/Microsoft.Maui.Cli/Ai/RepositoryAssetInstaller.cs, lines ~78-88 in InstallAssetAsync
Scenario: The code checks IsReparsePoint(fullDestinationPath) during the file-list loop, then checks again after downloading content, then finally calls WriteFileAtomicallyWithinRootAsync. Between the second check and the write, an attacker with local access could replace the destination directory with a symlink (TOCTOU). The WriteFileAtomicallyWithinRootAsync re-checks via IsSafeDestination which mitigates the risk for the actual write, but the intermediate Directory.CreateDirectory(destinationRoot) (line ~74) occurs before any check, and could follow a symlink that was just created.
Recommendation: The risk is mitigated by the post-creation IsPathWithinRoot check right after Directory.CreateDirectory. This is defense-in-depth and acceptable for a CLI tool (not a privileged daemon). Low practical risk but worth noting.
🟡 MODERATE — ContainsJsonComments can throw JsonException which is unhandled in ConfigureAsync
File: src/Cli/Microsoft.Maui.Cli/Ai/McpConfigurator.cs, line ~110 (backupExistingConfig = ContainsJsonComments(existingJson))
Scenario: ContainsJsonComments uses a Utf8JsonReader with CommentHandling = Allow. If the existing file is parseable by JsonNode.Parse (which uses AllowTrailingCommas + Skip for comments) but contains certain edge cases that cause Utf8JsonReader.Read() to throw (e.g., extremely large nesting depth), the JsonException would propagate. However — looking at the outer catch (JsonException) in ConfigureAsync, this IS caught.
Verdict: False alarm on review — the outer catch handles it. No action needed.
🟡 MODERATE — SkillInstaller.InstallSkillAsync downloads ALL files before checking count, wasting bandwidth on partial failures
File: src/Cli/Microsoft.Maui.Cli/Ai/SkillInstaller.cs, lines ~87-93
Scenario: DownloadSkillFilesAsync downloads files one by one and returns the count of successfully written files. If the expected count is, say, 5 and the 3rd file fails to download (returns null from FetchRawBytesAsync), the method still continues and counts only the successful ones. Then InstallSkillAsync compares filesInstalled != expectedFileCount → returns (-2, ""). The files were already downloaded to the temp dir and written to disk before the count mismatch is detected. This is wasteful but not a bug — the finally block cleans up.
Verdict: Not a bug. Behavior is correct — temp directory is cleaned up. Minor efficiency concern only.
🟡 MODERATE — AgentEnvironmentDetector.Detect loop termination has dead/confusing code
File: src/Cli/Microsoft.Maui.Cli/Ai/AgentEnvironmentDetector.cs, lines ~83-88
Scenario: The loop has:
if (rootFullPath is null)
break;
if (rootFullPath is not null && ...)
break;The second rootFullPath is not null check is always true at that point because the first if already breaks when null. This is dead code that reduces readability but does not affect correctness.
Recommendation: Remove the redundant rootFullPath is not null && condition from the second check for clarity.
🟡 MODERATE — McpConfigurator strips JSON comments without warning when modifying files
File: src/Cli/Microsoft.Maui.Cli/Ai/McpConfigurator.cs, lines ~106-110, ~131-133
Scenario: If a user's mcp.json contains JSON5 comments (common for VS Code config files), the code correctly backs up the file before overwriting. However, the re-serialization via root.ToJsonString(options) always strips comments from the output. Users who rely on inline comments in their MCP config will lose them permanently (backup only goes to .bak). This is documented behavior (backup is created) but could surprise users.
Recommendation: Consider emitting a console warning when comments are detected and stripped, so users know to check the .bak file. The current backup mechanism is adequate, but visibility helps.
🟡 MODERATE — RepositoryAssetInstaller.InstallAssetAsync returns (0, destinationRoot) when all files are skipped due to !force
File: src/Cli/Microsoft.Maui.Cli/Ai/RepositoryAssetInstaller.cs, end of method
Scenario: When force=false and all files already exist, filesToInstall is empty, fileContents is empty, and the method returns (0, destinationRoot). This is returned to callers like the update command which checks files <= 0 as a failure condition (HasUpdateInstallFailures returns true for files == 0). This means a skill that is already up-to-date but gets processed by the update path will be reported as a failure.
Recommendation: This is mitigated because the update command only calls InstallAssetAsync with force: true for items that NeedsUpdate already confirmed need updating. For the init and add flows, 0 means "skipped" and is handled correctly with appropriate user messaging. No functional bug, but the return value semantics are subtle.
🟢 MINOR — CreateGitHubHttpClient uses hardcoded User-Agent version "1.0"
File: src/Cli/Microsoft.Maui.Cli/Commands/AiCommands.cs, line ~74
Scenario: The User-Agent is Microsoft.Maui.Cli/1.0 regardless of actual CLI version. GitHub rate-limits by UA and a static version string makes debugging API issues harder.
Recommendation: Use the actual assembly version or Program.Version if available.
🟢 MINOR — ParseFrontmatter does not handle > (folded without chomp) correctly
File: src/Cli/Microsoft.Maui.Cli/Ai/MarketplaceClient.cs, ParseFrontmatter method
Scenario: The YAML block scalar detection checks for >-, >, |, |-, |+, >+. For the > indicator (folded, clip), YAML spec says a trailing newline should be preserved. The current implementation joins with spaces and doesn't append a newline. For SKILL.md descriptions this is harmless (descriptions don't need trailing newlines), but it's technically not YAML-compliant.
Recommendation: Acceptable for the use case. No action needed.
🟢 MINOR — NormalizePath allows paths with leading . segments (e.g., .hidden/path)
File: src/Cli/Microsoft.Maui.Cli/Ai/MarketplaceClient.cs, NormalizePath method
Scenario: A path like .github/skills/..hidden-dir/SKILL.md passes validation. The .. check only looks for exact .. segments (split by /), so .hidden or ...triple pass through. This is correct behavior — Unix allows dotfiles. Not a bug.
Summary
No 🔴 CRITICAL issues found. The security model is well-designed with layered defenses (path normalization, symlink detection, atomic writes, path-within-root guards). The code is production-ready with the minor observations noted above.
Generated by Expert Code Review · ● 41.8M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 6 findings posted inline (all 🟡 moderate). See the lean summary comment for methodology and details.
Generated by Expert Code Review · ● 32.2M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 5 findings posted inline (1 critical, 3 moderate, 1 minor). See the lean summary comment for methodology and discarded findings.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
learn.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "learn.microsoft.com"See Network Configuration for more information.
Generated by Expert Code Review · ● 26M
| Directory.Move(destinationDirectory, backupDirectory); | ||
|
|
||
| try | ||
| { | ||
| Directory.Move(sourceDirectory, destinationDirectory); |
There was a problem hiding this comment.
🟢 MINOR · Consensus: 2/3
Non-atomic directory replacement has a crash-unsafe window. If the process is killed between line 145 (Directory.Move to backup) and line 149 (Directory.Move source → destination), the skill directory disappears: the old install is in a .bak path and the new one hasn't landed yet. RestoreBackupDirectory handles the IOException path, but kill -9 / power loss bypasses it.
Recommendation: Document this limitation. Consider detecting orphaned .bak directories on next run and recovering automatically, or writing a sentinel file before the swap to enable recovery.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review — maui ai init Bootstrap Command
I reviewed the full implementation (all 30 files) for regressions, security issues, bugs, data loss, and race conditions. The security posture is strong — FileSystemPathGuard, NormalizePath with traversal rejection, reparse-point checks, and atomic writes are well-implemented. No critical issues found.
🟡 MODERATE (4 findings)
| # | File | Line | Issue |
|---|---|---|---|
| 1 | AiCommands.List.cs |
~65 | Uses Directory.GetCurrentDirectory() directly instead of GetAiCommandWorkingDirectory() like all other commands. Running from a subdirectory will miss environments detected at the Git root. |
| 2 | AiCommands.Update.cs |
~125 | Installed skills with PluginPath == null are silently excluded from updatable even with --force. User sees "all up to date" for stale skills. |
| 3 | McpConfigurator.cs |
~220 | GetConfigWriteRoot unconditionally returns projectRoot for non-CopilotCli kinds. If config path is outside project root (e.g., user-scoped OpenCode), the write will silently fail. |
| 4 | SkillInstaller.cs |
~72–100 | TOCTOU between ReadAsync check and ReplaceDirectory — concurrent invocations can race. Acceptable for a CLI tool but worth documenting. |
🟢 MINOR (5 findings)
| # | File | Line | Issue |
|---|---|---|---|
| 5 | RepositoryAssetInstaller.cs |
~213 | GetAssetRelativePath can produce empty string for edge-case inputs (malformed tree entry matching destination prefix exactly). |
| 6 | FileSystemPathGuard.cs |
~68 | Temp dirs (.maui-ai-write.*.tmp) created in project root may appear in git status if process is interrupted. |
| 7 | StringOrArrayConverter.cs |
~44 | Returns [] silently for unexpected JSON token types (number, object, null) — masks plugin.json configuration errors. |
| 8 | AgentEnvironmentDetector.cs |
~76 | When no Git root exists, only the immediate working directory is scanned (loop breaks immediately). Behavior differs from doc saying "scans up to Git root." |
| 9 | MarketplaceClient.cs |
~287 | ParseFrontmatter handles block-scalar description: but not name: — a name: >- entry will parse as literal ">-". |
Overall: solid implementation with good security hardening. The moderate issues are correctness edge cases, not vulnerabilities.
Generated by Expert Code Review · ● 30.7M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 3 findings posted inline (2 moderate, 1 minor). See the summary comment for full methodology and discarded findings.
Generated by Expert Code Review · ● 44.6M
| } | ||
|
|
||
| internal static bool NeedsUpdate(AiAssetStatusRow row, bool force) | ||
| => force || row.Status is "Missing" or "missing" or "Update available" or "update-available-from-current-cli" or "installed-from-different-cli-same-version" or "installed-from-newer-cli" or "dirty" or "unknown-or-unmanaged"; |
There was a problem hiding this comment.
🟢 MINOR · Consensus: 2/3 reviewers agree
Fragile hardcoded DevFlow status strings + misleading status for uncheckable skills
NeedsUpdate pattern-matches on raw DevFlow-internal status strings ("installed-from-different-cli-same-version", "dirty", "unknown-or-unmanaged", etc.). This creates a fragile coupling: if DevFlow renames any of these values, the update command silently stops working for affected skills. Additionally, "unknown-or-unmanaged" means ANY skill DevFlow doesn't recognize will unconditionally trigger an update attempt on every maui ai update run.
Separately, skills with legacy .skill-version entries (no PluginPath) display "Installed" even when --check-updates is set, giving a false sense of freshness.
Recommendation: Consider moving the "needs update" predicate into DevFlowSkillManager (which owns the status schema) and exposing it as a typed method. For uncheckable skills, surface a distinct status like "Unknown" when --check-updates is requested but verification is impossible.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 6 findings posted inline (3 moderate, 3 minor). See the lean summary comment for methodology and discarded findings.
Generated by Expert Code Review · ● 39.6M
| projectRoot, | ||
| content, | ||
| ct).ConfigureAwait(false)) | ||
| { |
There was a problem hiding this comment.
🟢 MINOR · Consensus: 2/3 reviewers
Partial write with no rollback: If WriteFileAtomicallyWithinRootAsync fails mid-loop (line 129–135), earlier files already written are left on disk — leaving the asset in an inconsistent state. The SkillInstaller uses a staging directory + atomic move pattern to avoid this.
Currently low impact since repository assets are typically single-file (Copilot agents), but worth noting for future multi-file asset support.
Recommendation: Consider downloading all content to a temp staging directory first (mirroring SkillInstaller.ReplaceDirectory), then atomically swapping into place.
| skill.Name.IndexOfAny(Path.GetInvalidFileNameChars()) >= 0 || | ||
| skill.Name.Contains("..") || | ||
| skill.Name.Contains('/') || | ||
| skill.Name.Contains('\\')) |
There was a problem hiding this comment.
🟢 MINOR · Consensus: 2/3 reviewers
Windows reserved device names not rejected: Path.GetInvalidFileNameChars() does not include Windows reserved names (CON, PRN, AUX, NUL, COM1–COM9, LPT1–LPT9). A skill with such a name would pass validation but cause Directory.CreateDirectory / File.WriteAllBytes to fail unpredictably on Windows.
Recommendation: Add a check after line 48:
|| (OperatingSystem.IsWindows() && IsWindowsReservedName(skill.Name))Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 4 findings posted inline. See the lean summary comment for full details and methodology.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
Generated by Expert Code Review · ● 33.6M
| { | ||
| if (FileSystemPathGuard.IsReparsePoint(env.SkillsDirectory) || | ||
| !Directory.Exists(env.SkillsDirectory) || | ||
| FileSystemPathGuard.IsReparsePoint(env.SkillsDirectory)) |
There was a problem hiding this comment.
🟡 MODERATE · Consensus: 2/3 reviewers
Duplicate IsReparsePoint check. Line 117 and line 119 both call FileSystemPathGuard.IsReparsePoint(env.SkillsDirectory) — the third condition is an exact copy of the first and provides no additional protection.
The intent may have been to re-check after Directory.Exists (TOCTOU guard), but since no directory is created between the checks, the second call is redundant.
Suggestion: Remove the duplicate third condition:
if (FileSystemPathGuard.IsReparsePoint(env.SkillsDirectory) ||
!Directory.Exists(env.SkillsDirectory))Or, if TOCTOU protection is desired, check each returned skillDir inside the foreach (which is already done at line 125/126).
| var skill = allSkills.FirstOrDefault(s => | ||
| string.Equals(s.Name, skillName, StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (skill is null) | ||
| { | ||
| formatter.WriteError(new Exception( | ||
| $"Skill '{skillName}' not found in the marketplace. Run 'maui ai list' to see available skills.")); | ||
| return 1; | ||
| } | ||
|
|
||
| // Detect environments | ||
| var currentDir = Directory.GetCurrentDirectory(); | ||
| var workingDir = GetAiCommandWorkingDirectory(currentDir); | ||
| var environments = AgentEnvironmentDetector.Detect(currentDir); | ||
|
|
||
| environments = FilterEnvironments(environments, envFilter); | ||
|
|
||
| if (environments.Count == 0) | ||
| { | ||
| formatter.WriteWarning("No agent environments detected. Run 'maui ai init' to set up environments first."); | ||
| return 1; | ||
| } | ||
|
|
||
| if (dryRun) | ||
| { | ||
| formatter.WriteInfo($"[Dry run] Would install skill '{skill.Name}' to:"); | ||
| formatter.WriteTable( | ||
| environments, | ||
| ("Environment", e => e.Kind.ToString()), | ||
| ("Path", e => Path.Combine(e.SkillsDirectory, skill.Name))); | ||
| return 0; | ||
| } | ||
|
|
||
| // Confirm unless --force, --ci, or --json | ||
| if (!force && !isCi && !useJson) | ||
| { | ||
| formatter.WriteInfo($"Will install '{skill.Name}' ({skill.Files.Count} files) to {environments.Count} {(environments.Count == 1 ? "environment" : "environments")}."); | ||
| if (!AnsiConsole.Confirm("Proceed?", defaultValue: true)) | ||
| { | ||
| formatter.WriteInfo("Installation cancelled."); | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| // Install | ||
| var results = new List<(string Env, int Files, string Path)>(); | ||
|
|
||
| foreach (var env in environments) | ||
| { | ||
| var (filesInstalled, installPath) = await SkillInstaller.InstallSkillAsync( |
There was a problem hiding this comment.
🟡 MODERATE · Consensus: 3/3 reviewers
maui ai add bypasses DevFlow installer for DevFlow-managed skills. Unlike init and update (which check IsDevFlowManagedSkillName() and route to DevFlowSkillManager), the add command installs any marketplace skill — including DevFlow-managed ones like maui-devflow-debug — directly through SkillInstaller, bypassing the intended ownership/version flow.
Suggestion: Add a guard before installation:
if (IsDevFlowManagedSkillName(skill.Name))
{
formatter.WriteWarning(
$"'{skill.Name}' is managed by DevFlow. Use 'maui ai init' or 'maui ai update' instead.");
return 1;
}Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expert Code Review — PR #98Methodology: 3 independent reviewers with adversarial consensus 6 findings posted as inline comments (4 🟡 moderate, 2 🟢 minor)
Discarded findings (single reviewer only, did not reach consensus)
CI Status
Test CoveragePR includes comprehensive unit tests (8 new test files:
|
There was a problem hiding this comment.
Expert Code Review: 6 findings posted inline (4 moderate, 2 minor). See the lean summary comment for methodology and discarded findings.
Generated by Expert Code Review · ● 26.2M
| if (!IsSafeDestination(fullDestinationPath, destinationDirectory, root)) | ||
| return false; | ||
|
|
||
| File.Move(tempPath, fullDestinationPath, overwrite: true); |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 consensus
TOCTOU race between final safety check and File.Move.
The IsSafeDestination check at line 83 verifies the destination isn't a symlink, but File.Move at line 86 re-resolves the path. A local attacker with write access to the destination directory could swap fullDestinationPath to a symlink between these two operations, redirecting the write outside the intended root.
The window is small and requires local filesystem access, but this method is explicitly designed as a security boundary (FileSystemPathGuard).
Recommendation: After the final IsSafeDestination check, additionally verify that fullDestinationPath is not a reparse point, or use handle-based open with O_NOFOLLOW / FILE_FLAG_OPEN_REPARSE_POINT semantics to prevent symlink dereference on the final rename.
| } | ||
|
|
||
| internal static bool NeedsUpdate(AiAssetStatusRow row, bool force) | ||
| => force || row.Status is "Missing" or "missing" or "Update available" or "update-available-from-current-cli" or "installed-from-different-cli-same-version" or "installed-from-newer-cli" or "dirty" or "unknown-or-unmanaged"; |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 consensus
NeedsUpdate is overly aggressive — treats "Missing" and "unknown-or-unmanaged" as update triggers.
Two concerns:
"Missing"causesmaui ai updateto install assets that were never installed, rather than just updating existing ones. Users expectupdateto refresh what's already present, not add new things."unknown-or-unmanaged"causes silently replacing hand-placed customized skill files with bundled versions — no warning that an unmanaged file is being overwritten.
Recommendation: Exclude "Missing" and "unknown-or-unmanaged" from the non-force update path. Either require --force for these statuses, or add an explicit --install-missing flag. Emit a warning when encountering unmanaged files.
| var count = 0; | ||
| foreach (var (destinationPath, content) in fileContents) | ||
| { | ||
| if (!await FileSystemPathGuard.WriteFileAtomicallyWithinRootAsync( |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 consensus (disputed, then confirmed)
Partial-write inconsistency on multi-file assets.
If WriteFileAtomicallyWithinRootAsync fails for file N after files 1..N-1 succeeded, the method returns an error. On the next non-forced run, already-written files are skipped (via File.Exists at line 98), and only remaining files are retried — reporting "Installed (1 file)" even though the asset has multiple files.
Currently assets are single-file (mitigating this), but the code structure allows multi-file assets. Unlike SkillInstaller which stages atomically, this writes directly to the destination.
Recommendation: Consider staging all files into a temp directory within projectRoot before moving atomically into place (matching the SkillInstaller pattern), or document the single-file invariant with an assertion.
| if (FileSystemPathGuard.IsReparsePoint(env.SkillsDirectory) || | ||
| !Directory.Exists(env.SkillsDirectory) || | ||
| FileSystemPathGuard.IsReparsePoint(env.SkillsDirectory)) |
There was a problem hiding this comment.
🟢 MINOR · 2/3 consensus
Duplicate IsReparsePoint check (line 117 and 119 are identical).
The third condition is a copy-paste of the first — it incurs an extra File.GetAttributes syscall and creates the false impression of a deliberate double-check pattern. Additionally, the gap between the reparse-point check and Directory.GetDirectories (line 124) constitutes a narrow TOCTOU window, though exploitation is impractical for this read-only enumeration.
Recommendation: Remove the duplicate IsReparsePoint call.
| } | ||
|
|
||
| static string GetBackupPath(string configPath) | ||
| => $"{configPath}.bak"; |
There was a problem hiding this comment.
🟢 MINOR · 2/3 consensus
Backup file is always <config>.bak — subsequent runs silently overwrite prior backups.
If a user modifies mcp.json between runs, the previous .bak recovery point is lost without warning. This makes it impossible to recover from a bad maui ai init run if update has already overwritten the backup.
Recommendation: Use timestamped backup names (e.g., $"{configPath}.{timestamp}.bak") or check for an existing .bak and warn before overwriting.
| static string BuildRawUrl(string repo, string branch, string path) | ||
| { | ||
| var encodedRepo = EncodeRepoPath(repo); | ||
| var encodedBranch = string.Join("/", branch.Split('/').Select(Uri.EscapeDataString)); |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 consensus
Branch parameter not validated for path-traversal segments.
Uri.EscapeDataString("..") returns ".." unmodified (dots are not percent-encoded). .NET's Uri class normalizes .. segments before sending the HTTP request. A --branch value like "main/../../../attacker/evil-repo/main" would resolve to a different GitHub repository's raw content after URI normalization.
While --branch is a hidden option and GitHub may reject malformed paths server-side, client-side defense-in-depth is warranted since skills downloaded from the resolved URL are written to disk.
Recommendation: Add validation that rejects "." or ".." segments:
var segments = branch.Split('/');
if (segments.Any(s => s is "." or ".."))
throw new InvalidOperationException($"Branch '{branch}' must not contain '.' or '..' segments.");
Summary
Updates this PR with current
mainand expandsmaui ai initinto a one-stop bootstrap command for AI-powered .NET MAUI development.The top-level bootstrap now orchestrates all relevant MAUI AI assets without duplicating DevFlow logic:
DevFlowSkillManager.InstallRecommendedAsync) for recommended DevFlow skills.plugins/dotnet-maui/skills..github/skillssuch as Comet Go and MAUI platform backend guidance..github/agents/*.agent.md..github/skillstargets.--no-mcpis specified.Implements #97.
Commands
maui ai initmaui ai listmaui ai statusmaui ai updatemaui ai add <skill>Architecture
Service layer (
Ai/):MarketplaceClientfetches marketplace/plugin manifests, enumerates skills, and downloads skill files.RepositoryAssetInstallerdiscovers, installs, and inspects repository-hosted AI assets such as Copilot agent definitions.AgentEnvironmentDetectordetects agent environments and handles normal repos plus worktree.gitfiles.McpConfiguratormerges themaui-devflowMCP server entry into agent config files.SkillInstallerandSkillVersionStoreinstall non-DevFlow skills and track versions.AiJsonContextprovides AOT-compatible source-generated JSON serialization.Commands (
Commands/AiCommands*.cs):maui aicommand group follows the existing partial command pattern.maui ai initdelegates DevFlow skill installation to the existing DevFlow implementation instead of duplicating it.maui ai statusnow reports DevFlow bundled skill state, marketplace/repository skills, and Copilot agent definitions.maui ai updatenow calls the DevFlow updater, refreshes installed marketplace/repository skills, and updates repo-hosted Copilot agents.Docs
maui ai init --dry-run,maui ai init,maui ai status --check-updates, andmaui ai update.Testing
./.dotnet/dotnet test src/Cli/Microsoft.Maui.Cli.UnitTests/ --no-restore --logger "console;verbosity=minimal"git diff --check/cc @Redth