Extract CLI skills into dedicated project#188
Conversation
Move the CLI-bundled DevFlow skill resources into a dedicated Microsoft.Maui.Cli.Skills project so the skills can be built and versioned independently while still being consumed by the maui CLI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expert Code Review — PR #188Methodology: 3 independent reviewers with adversarial consensus 3 findings posted as inline comments (2 🟡 moderate, 1 🟢 minor)
Discarded findings (did not reach consensus)
CI Status✅ All checks passing (macOS build ✅, Windows build ✅, package ✅, license/CLA ✅) Test Coverage✅ PR includes a new unit test (
|
![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: 3 findings posted inline (2 moderate, 1 minor). See the lean summary comment for full details.
Generated by Expert Code Review (auto) for issue #188 · ● 13.4M
|
|
||
| public static Assembly Assembly => typeof(MauiCliSkillResources).Assembly; | ||
|
|
||
| public static IReadOnlyList<MauiCliSkillDefinition> BundledSkills { get; } = |
There was a problem hiding this comment.
🟡 MODERATE · consensus 3/3
Silent failure if BundledSkills list and .csproj embedded resource globs diverge.
These two manually-maintained lists must stay in sync: the BundledSkills entries here and the EmbeddedResource includes in the .csproj. If a future skill directory is added to the .csproj glob but not to this list (or vice versa):
- Missing from
BundledSkills→ resources are embedded but never discovered (silent dead weight) - Missing from
.csproj→LoadSkillBundleAsyncfinds zero resources and throwsInvalidOperationException("No embedded DevFlow skill resources found...")at runtime
The existing unit test hardcodes the two known skill names and wouldn't catch divergence when a third skill is added.
Recommendation: Add a test (or adjust the existing one) that dynamically iterates MauiCliSkillResources.BundledSkills and asserts each entry has at least one embedded resource with prefix {ResourceRoot}/{skill.Id}/, and conversely that every embedded resource prefix maps to a BundledSkills entry.
| } | ||
|
|
||
| return new SkillBundle(skill.Id, GetCurrentCliVersion(), files, HashBundle(files)); | ||
| return new SkillBundle(skill.Id, GetBundledSkillVersion(), files, HashBundle(files)); |
There was a problem hiding this comment.
🟡 MODERATE · consensus 2/3
Version decoupling incomplete — update safety logic still coupled to CLI version.
This line correctly records GetBundledSkillVersion() in the bundle, but the update/downgrade safety logic elsewhere (UpsertStateEntry at ~line 809) still writes installedByCliVersion = GetCurrentCliVersion(), and the comparison at ~line 360 evaluates against CLI version. If the Skills assembly is ever versioned independently (a stated goal of this extraction), a skills-only hotfix (e.g., Skills v1.0.1 with CLI still at v1.0) would record installedByCliVersion = "1.0" even though the installed content is from v1.0.1. A subsequent older Skills bundle would bypass downgrade protection since installedByCliVersion == GetCurrentCliVersion().
Recommendation: Write installedByBundleVersion = GetBundledSkillVersion() separately in UpsertStateEntry, and use it (not installedByCliVersion) for the newer/older classification when deciding whether to allow overwrites. Keep installedByCliVersion as diagnostic metadata only.
| => MauiCliSkillResources.Assembly | ||
| .GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion | ||
| ?? MauiCliSkillResources.Assembly.GetName().Version?.ToString() | ||
| ?? GetCurrentCliVersion(); |
There was a problem hiding this comment.
🟢 MINOR · consensus 2/3
Fallback to GetCurrentCliVersion() silently re-couples the assemblies.
If the Skills assembly has no AssemblyInformationalVersionAttribute and no AssemblyVersion (e.g., a malformed build), this silently falls back to the CLI's version. The skillVersion recorded in state would then be the CLI version rather than signaling something is wrong. Future diagnostics comparing bundledVersion vs installedVersion would appear identical, masking the issue.
Recommendation: Consider using "unknown" as the final fallback (consistent with GetCurrentCliVersion()'s own ultimate fallback) to make the failure mode visible rather than silently aliased to the CLI version. This is low-risk given IsPackable=false keeps versions in lockstep today.
1 participant
DevFlow skills were embedded directly by
Microsoft.Maui.Cli, which made them harder to treat as their own buildable and versioned component. This extracts the bundled skills into a dedicated project while keeping the existing CLI install flow intact.Changes
Microsoft.Maui.Cli.Skills, anet9.0;net10.0class library that embeds the existing DevFlow skill assets fromplugins/dotnet-maui/skills.DevFlowSkillManagerto load bundled skill resources from the referenced skills assembly instead of themauiassembly.MauiLabs.slnx,src/Cli/Cli.slnf, CLI CI path filters, docs, and unit tests.Validation
.dotnet/dotnet build src/Cli/Cli.slnf.dotnet/dotnet test src/Cli/Microsoft.Maui.Cli.UnitTests/Microsoft.Maui.Cli.UnitTests.csproj --no-buildMicrosoft.Maui.Cli.Skills.dllis copied with the CLI andmaui devflow initinstalls both bundled skills.