refactor: extract private gallery services#210
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PrzemyslawKlys
added
.NET
PR Review: refactor: extract private gallery servicesThis is a well-motivated refactor that improves testability by introducing an IPrivateGalleryHost abstraction and moving private gallery logic into an injectable PrivateGalleryService. The structure is sound, but I found several issues ranging from behavioral regressions to design concerns. Bugs and Behavioral Regressions1. RepositoryRegistrationTool.Both not handled in EnsureAzureArtifactsRepositoryRegistered The new dispatch in PrivateGalleryService.cs only handles PSResourceGet vs. else-PowerShellGet. When a caller explicitly passes Tool = Both, effectiveTool becomes Both, falls into the else branch, and only registers PowerShellGet. The old code had explicit "or RepositoryRegistrationTool.Both" checks in both branches. This is a regression for any caller using Tool = Both. 2. NullLogger silently drops verbose output from clients The new service always uses new NullLogger() when constructing PSResourceGetClient, PowerShellGetClient, ModuleDependencyInstaller, and AzureArtifactsCredentialProviderInstaller. The old code passed a CmdletLogger that respected the -Verbose flag. All verbose output from those underlying clients is now silently discarded. The fix is to expose a logger factory or logging delegate on IPrivateGalleryHost. 3. ToolUsed set to Auto instead of Both In the auto-mode path where PSResourceGet succeeds and PowerShellGet is also registered for compatibility, the code sets result.ToolUsed = RepositoryRegistrationTool.Auto. The old code set ToolUsed = Both. Returning Auto makes it impossible for callers to distinguish "both tools registered" from "tool not yet resolved". This should be RepositoryRegistrationTool.Both. 4. Credential prompt caption changed from cmdlet name to a generic string Old code used cmdlet.MyInvocation.MyCommand.Name as the dialog title. New service hardcodes "Private gallery authentication". Consider adding a CommandName property to IPrivateGalleryHost so the service can use the actual cmdlet name as the prompt caption. Design Concerns5. Duplicated ModuleRepositoryRegistrationResult with a hand-written mapper ModuleRepositoryRegistrationResultMapper.ToCmdletResult copies approximately 40 properties between two nearly identical classes in different namespaces. Adding any new property requires updating both classes and the mapper. Worth evaluating whether a single shared type would be more maintainable. 6. GetBootstrapPrerequisiteStatus unnecessarily widened to public It was private static in the old code. Since PrivateGalleryService is internal sealed this does not escape the assembly, but private is more appropriate given it is only called internally. 7. ReservedPowerShellGalleryRepositoryName constant removed The constant ReservedPowerShellGalleryRepositoryName was deleted from PrivateGalleryCommandSupport. Worth confirming nothing else in the codebase references it. Missing Test Coverage8. No unit tests for PrivateGalleryService One of the stated benefits of introducing IPrivateGalleryHost is testability, but no new tests accompany the refactor. The only test change is updating a class name reference. Unit tests using a mock IPrivateGalleryHost would be high-value, especially for ResolveCredential mode combinations, the ShouldProcess gate, and BuildDependencies edge cases. Minor Notes
Required fixes before merge:
|
|
Overall this is a well-structured refactor. Moving the static PrivateGalleryCommandSupport blob into PrivateGalleryService with IPrivateGalleryHost is the right call: testable, no PSCmdlet threading through every method, and PowerForge.PowerShell becomes the proper home for this logic. PrivateModuleWorkflowService deduplication of install/update is also a nice improvement. Issues found: BUG: RepositoryRegistrationTool.Both silently falls through to PowerShellGet-only In PrivateGalleryService.EnsureAzureArtifactsRepositoryRegistered, the registration block is an if/else on PSResourceGet vs everything else. If the caller passes Tool=Both, effectiveTool is never resolved to a single value (the Auto branch does not run, no pre-check throws for Both), so it falls to the else arm and only registers PowerShellGet. Old code handled Both with two separate if statements. Functional regression for any user passing -Tool Both. BUG: apiVersion RepositoryApiVersion.V3 dropped Old code passed apiVersion: RepositoryApiVersion.V3 to PSResourceGet.EnsureRepositoryRegistered. New PrivateGalleryService omits it. If V3 is not the default, Azure Artifacts repos could fail or misbehave after registration. REGRESSION: PowerShell client verbose logging silently dropped Old EnsureAzureArtifactsRepositoryRegistered and EnsureBootstrapPrerequisites used CmdletLogger so PSResourceGetClient/PowerShellGetClient/ModuleDependencyInstaller could emit verbose output. New PrivateGalleryService uses NullLogger in both - verbose from those operations is silently discarded even with -Verbose. IPrivateGalleryHost already has WriteVerbose; consider plumbing it to the runner/logger. MINOR: Credential prompt caption changed Old code used cmdlet.MyInvocation.MyCommand.Name (e.g. Connect-ModuleRepository) as the caption. New code hardcodes 'Private gallery authentication'. IPrivateGalleryHost could expose CommandName, or the caption could be passed in from the calling cmdlet. TEST GAP: AzureArtifacts path not covered PrivateModuleWorkflowServiceTests covers only UseAzureArtifacts=false. The AzureArtifacts path has no unit-test coverage. Good things: injectable dependencyExecutor in PrivateModuleWorkflowService is a clean testability pattern. ModuleRepositoryRegistrationResultMapper preserves the stable public contract. IPrivateGalleryHost surface is minimal. Removing PSCmdlet from every helper signature is a clear improvement. AGENTS.md additions document the layering intent well. The Both-tool regression and missing apiVersion are the two to resolve before merging. |
|
Test comment from automated review |
Code ReviewOverall this is a solid architectural improvement. Replacing the static god class PrivateGalleryCommandSupport with an interface + service + host-adapter pattern aligns with the new layering guidelines in AGENTS.md, and the result is genuinely more testable. A few items to look at before merging. Bug: RepositoryRegistrationTool.Both is silently broken RepositoryRegistrationTool.Both is a documented public enum value, but PrivateGalleryService.EnsureAzureArtifactsRepositoryRegistered no longer handles it correctly. When tool equals Both, effectiveTool stays Both, the prerequisite guards do not fire (they only check PSResourceGet and PowerShellGet individually), and execution falls into the else branch registering only PowerShellGet. ToolUsed ends up set to Both while PSResourceGetRegistered remains false. The old PrivateGalleryCommandSupport handled this with two independent if blocks. Suggest adding explicit handling for Both, or at minimum throwing for unrecognised values so the gap is visible. Regression: NullLogger in registration and prerequisite paths PrivateGalleryService.EnsureAzureArtifactsRepositoryRegistered and EnsureBootstrapPrerequisites both construct new NullLogger() internally rather than receiving a logger from the caller. The old code forwarded a CmdletLogger so -Verbose output from PSResourceGetClient, PowerShellGetClient, and the prerequisite installer surfaced to the user. All registration and bootstrap work is now silent even with -Verbose. Options include accepting ILogger in the PrivateGalleryService constructor, or passing it per-method. Minor: OutputPath relative resolution changed for Export-CertificateForNuGet The old cmdlet used SessionState.Path.GetUnresolvedProviderPathFromPSPath(OutputPath), which resolves against the PowerShell current filesystem location. The new service uses Path.GetFullPath which resolves against the process working directory. These differ when users use Set-Location within a session. Only affects relative paths; absolute paths are fine. The Trim of double-quote characters on the output path is also an unusual guard -- if intentional, a brief comment would help future readers. Minor: NuGetCertificateExportService.Execute swallows all exceptions The broad catch block converts every exception into a Success = false result. Callers outside a cmdlet context cannot distinguish certificate-not-found from disk-full without parsing the error string. Consider narrower catches and letting hard failures propagate. Minor: BuildNotFoundMessage uses a Windows-style path separator The backslash-My-store suffix in the not-found message is a Windows registry-path convention and will look odd in output on Linux or macOS. Worth clarifying whether this is intentional given the service is otherwise cross-platform. Test coverage gaps The new PrivateModuleWorkflowServiceTests cover the repository-only install path and the ShouldProcess-declines case. Notable gaps: Azure Artifacts path (even a minimal happy-path with a stub registration result), RepositoryRegistrationTool.Both behavior (which has the regression above), and NuGetCertificateExportServiceTests has no test for the SHA-256 lookup path. Nit: RecommendedBootstrapCommand on the internal model contains hardcoded cmdlet names ModuleRepositoryRegistrationResult.RecommendedBootstrapCommand (in PowerForge.PowerShell) builds a Register-ModuleRepository string with hardcoded parameter names. This couples the shared internal model to a specific public cmdlet surface. If that parameter is renamed, or if the model is consumed from a non-PS host, the suggestion will be wrong. Consider moving this helper to the PSPublishModule-side mapper/result, or leaving a comment acknowledging the deliberate coupling. Positive highlights
|
PR Review: refactor: extract private gallery servicesOverall this is a clean, well-motivated refactoring that follows the layering boundaries described in AGENTS.md. The cmdlets are noticeably thinner, the new Functional / Behavioral Concerns1.
|
PR Review: refactor: extract private gallery servicesGreat refactoring overall. The layering is cleaner, the cmdlets are genuinely thin, and the new test coverage is solid. A few items worth looking at before merge: Potential Bug: PS-path resolution regression in ExportCertificateForNuGet The old cmdlet resolved Minor: PSArgumentException to ArgumentException in BuildDependencies The old Subtle: implicit SwitchParameter to bool conversions in workflow request In Minor: redundant File.Exists check in ModuleTestFailureWorkflowService
Gap: no unit tests for PrivateGalleryService itself New test files cover Nit: ModuleRepositoryRegistrationResultMapper property drift risk The mapper manually copies approximately 30 properties. If a property is later added to the internal What is good The |
|
Overall this is a well-executed architectural refactoring. See below for the full review. |
|
Overall this is a well-executed architectural refactor. IPrivateGalleryHost makes the services testable without a real PowerShell host, and the new test files provide solid coverage of the extracted logic. A few issues worth addressing before merging. The Both-tool registration regression and the missing apiVersion are the two items I would prioritize. Full notes below. |
|
Potential Bug: RepositoryRegistrationTool.Both silently regresses File: PowerForge.PowerShell/Services/PrivateGalleryService.cs - EnsureAzureArtifactsRepositoryRegistered The old PrivateGalleryCommandSupport handled RepositoryRegistrationTool.Both with two sequential registration calls (PSResourceGet then PowerShellGet). The new service has no guard for Both in the pre-flight validation block, and the registration block uses a simple if/else between PSResourceGet and PowerShellGet. If a caller explicitly passes Tool = Both, the new code silently falls into the else branch and only registers PowerShellGet. Adding an explicit Both branch (or a defensive throw for an unrecognized value) would address this regression. |
|
Potential Bug: Missing apiVersion parameter for PSResourceGet registration File: PowerForge.PowerShell/Services/PrivateGalleryService.cs The old code passed apiVersion: RepositoryApiVersion.V3 as a named argument to PSResourceGetClient.EnsureRepositoryRegistered. The new code omits this argument entirely. If the method default is not V3, this could silently register the repository against the wrong NuGet API version and break package resolution. Worth verifying whether V3 is the default. |
|
Regression: Verbose install logging silently swallowed File: PowerForge.PowerShell/Services/PrivateGalleryService.cs Both EnsureBootstrapPrerequisites and EnsureAzureArtifactsRepositoryRegistered now use new NullLogger() where the old code passed a CmdletLogger that forwarded to WriteVerbose. Users running with -Verbose will no longer see installation-step messages from PSResourceGet or AACP installation - operations that can take several minutes. Consider whether IPrivateGalleryHost should expose a CreateLogger() factory or accept an ILogger in the constructor so the host can forward verbose messages back to the cmdlet stream. |
|
Minor UX: PromptForCredential caption changed File: PSPublishModule/Services/CmdletPrivateGalleryHost.cs Old code used cmdlet.MyInvocation.MyCommand.Name as the credential-prompt caption (e.g. Connect-ModuleRepository). New code uses the literal string 'Private gallery authentication'. Users who relied on the dialog caption to identify which cmdlet is prompting will now see a generic title. Low severity but a small regression. PSArgumentException vs ArgumentException File: PowerForge.PowerShell/Services/PrivateGalleryService.cs The old static helper threw PSArgumentException for invalid parameter combinations - this type formats as a proper parameter-binding error in the PowerShell error stream with the parameter name highlighted. The new service throws the standard ArgumentException. If keeping PS-specific exceptions out of the shared layer is a goal, IPrivateGalleryHost could expose a ThrowParameterError(string paramName, string message) helper that the cmdlet-layer adapter implements with PSArgumentException. What is working well
|
|
test2 |
Code Review: refactor extract private gallery servicesOverall this is a well-executed architectural refactoring that aligns squarely with the layering rules now documented in AGENTS.md. The direction is correct and the PR is generally in good shape. Positive highlights
Issues1. ToolUsed is set to Auto instead of Both when dual registration succeeds File: PowerForge.PowerShell/Services/PrivateGalleryService.cs When Tool == Auto, PSResourceGet succeeds, and the compat PowerShellGet registration also succeeds, result.ToolUsed is set to RepositoryRegistrationTool.Auto not Both. The old code set it to Both. Callers that inspect ToolUsed == Both to detect dual registration will silently see Auto instead. Worth either fixing the assignment to Both or documenting that Auto now means both succeeded. 2. NullLogger in EnsureBootstrapPrerequisites silently drops install diagnostics File: PowerForge.PowerShell/Services/PrivateGalleryService.cs The old EnsureBootstrapPrerequisites created a CmdletLogger(cmdlet, verbose), surfacing prerequisite install output to the user. The new service always uses NullLogger, silently dropping installer messages. PrivateModuleWorkflowService has a _logger field but it is not threaded into EnsureBootstrapPrerequisites. Consider accepting a logger in PrivateGalleryService constructor and forwarding it here. 3. CmdletPrivateGalleryHost.PromptForCredential loses cmdlet-name caption File: PSPublishModule/Services/CmdletPrivateGalleryHost.cs The old code used cmdlet.MyInvocation.MyCommand.Name as the credential dialog caption so users saw the cmdlet name (e.g. Connect-ModuleRepository). The new PrivateGalleryService hardcodes the caption to "Private gallery authentication". Consider constructing CmdletPrivateGalleryHost with the cmdlet name and using it as the caption. 4. ExportCertificateForNuGetCommand -- OutputPath bypasses SessionState.Path resolution File: PSPublishModule/Cmdlets/ExportCertificateForNuGetCommand.cs The old code called SessionState.Path.GetUnresolvedProviderPathFromPSPath(OutputPath) for the explicit-path case, correctly resolving PowerShell provider-relative paths. The new service only receives a plain working directory string, so provider-relative OutputPath values may resolve to the wrong directory when the user has cd inside the PowerShell session. Suggestion: Resolve OutputPath through SessionState.Path.GetUnresolvedProviderPathFromPSPath in the cmdlet before passing it to the service, so the service always receives a fully resolved filesystem path. 5. PrivateModuleWorkflowService does not forward Force to EnsureUpdated File: PowerForge.PowerShell/Services/PrivateModuleWorkflowService.cs request.Force is defined in PrivateModuleWorkflowRequest but is not forwarded to EnsureUpdated. If EnsureUpdated accepts a force parameter, this is a silent regression for Update-PrivateModule. Worth verifying whether the old UpdatePrivateModuleCommand forwarded force. Minor nits
SummaryItems 2 and 4 are the most actionable before merge -- silent regressions in installer output visibility and path resolution. Items 1 and 5 are lower priority but worth a quick check. Otherwise this is a solid cleanup that makes the codebase significantly more testable and follows the layering documented in AGENTS.md. Generated with Claude Code |
|
Code Review: PR 210 - refactor: extract private gallery services. Overall well-executed refactoring. See inline notes. |
|
Code Review: PR 210 - refactor: extract private gallery services Overall this is a well-executed, high-signal refactoring. The layering story is clear, the tests are meaningful, and the public cmdlet surface stays stable. Architecture positives:
Issues:
Test coverage: Excellent. Using the injected _dependencyExecutor delegate in PrivateModuleWorkflowServiceTests is exactly the right pattern. One gap: no tests for ModuleRepositoryRegistrationResultMapper.ToCmdletResult. The mapper copies roughly 30 properties; a round-trip test would catch any property added to the source model but omitted from the mapper. Minor: AboutTopicTemplateService.Generate returns Exists = false even after writing (reflects pre-generation state per the test, but a comment would help). ModuleBuildOutcomeService correctly handles workflow == null. The AGENTS.md layering docs added here are valuable. Summary: Ready to merge. Item 1 (credential caption) is the most actionable item - a one-liner fix. The mapper round-trip test is a good safety net but not blocking. |
PR Review — refactor: extract private gallery servicesOverall this is a well-structured refactor that clearly advances the layering goals described in AGENTS.md. The intent is sound, the scope is consistent, and the diff is internally coherent. A few issues worth addressing before merge. Potential bugs1. In the old // Fix: add ToolUsed to the initialiser, consistent with the old code
ToolRequested = tool,
Tool = tool,
ToolUsed = tool, // ← add this2. In the old code, when Diagnostic regression3. The old implementation passed a Consider accepting an Minor issues4.
private void OnItemProcessed(int current, int total, ProjectCleanupItemResult item)
{
WriteDisplayLines(new ProjectCleanupDisplayService().CreateItemLines(...));
// ^^ new instance each call
}
5. Implicit In InstallPrerequisites = InstallPrerequisites,
Force = Force,
Prerelease = Prerelease,
6. The old code used Positive highlights
Summary: Issues 1 and 2 are worth fixing before merge as they change observable behaviour for users and scripts. Issues 3–6 are improvements but lower urgency. 🤖 Generated with Claude Code |
PR Review: refactor: extract private gallery servicesOverall this is a solid refactoring. The layering intent is clear, the host adapter pattern ( Bugs / Correctness1. Null-forgiving operator on a conditionally-null expression (
2.
3. private void OnItemProcessed(int current, int total, ProjectCleanupItemResult item)
{
WriteDisplayLines(new ProjectCleanupDisplayService().CreateItemLines(...));
}The Architecture4.
5. While 6. Shared-source pattern for
Breaking Changes7. Previously emitted ad-hoc Also: on the error path both Test Coverage Gaps8. The 52-property manual copy between 9.
Minor / Nits10.
11. The 12. Success/failure is determined by Summary
The host adapter pattern is the right direction and the overall structure is a clear improvement. The highest-value fixes before merge are the null-forgiving bug (1), the Generated with Claude Code |
|
PR Review: refactor: extract private gallery services Overall this is a well-structured refactor. The layering intent is clear, the AGENTS.md additions codify the architecture neatly, and the reduction in cmdlet complexity is a net win. Positives
Concern 1: ModuleRepositoryRegistrationResultMapper -- silent field drift risk ToCmdletResult copies ~25 properties by hand. If a new property is added to PowerForge.ModuleRepositoryRegistrationResult and not mirrored here, the omission compiles cleanly but the cmdlet output silently loses data. Consider a comment warning maintainers to keep the mapper in sync, or a unit test using reflection to assert all public properties are covered. Concern 2: RemoveProjectFilesCommand.OnItemProcessed -- per-item service allocation ProcessRecord already holds a display local, but OnItemProcessed allocates a fresh ProjectCleanupDisplayService() for every item processed. If stateless this is just wasteful; if the service ever gains state it becomes a bug. Promote display to a field or pass it into the callback so both ProcessRecord and the callback share the same instance. Concern 3: ConnectModuleRepositoryCommand -- mutation order dependency The probe-field mutation happens before ToCmdletResult() is called -- that order is correct. A short comment noting that the mapper call must follow the probe mutations would protect against an accidental reorder in a future refactor. Concern 4: ShouldProcess lambda and -Force / -WhatIf interaction PSCmdlet.ShouldProcess honours -Force, -WhatIf, and ConfirmPreference automatically. Delegating it as a Func is the right pattern, but a test covering -Force combined with -WhatIf would confirm both flags are still respected correctly after the refactor. Minor: Directory.GetCurrentDirectory() vs Environment.CurrentDirectory InvokeModuleTestSuiteCommand changed the fallback to Environment.CurrentDirectory. These are semantically equivalent in .NET. The rest of the changed files now consistently use Environment.CurrentDirectory, so this is fine. Test coverage note It would be helpful to confirm whether the Pester suite covers RegisterModuleRepositoryCommand, ConnectModuleRepositoryCommand, and UpdateModuleRepositoryCommand through the new adapter path, not just install/update. Summary Architecture is correct and the direction is right. Two actionable items before merge:
Everything else is low-risk or informational. |
PR Review:
|
| Area | Assessment |
|---|---|
| Code quality / layering | Strong — follows AGENTS.md guidance precisely |
| Potential bugs | workflow?.Error! suppressor (#1); nullable outcome pattern (#2) |
| Performance | No regressions; stateless display services allocated per-call (acceptable) |
| Security | Credential file encoding edge case (minor, pre-existing) |
| Test coverage | Significantly improved with 16+ new test files |
Items worth addressing before merge: #1 (remove the ! suppressor) and #2 (tighten the outcome nullable declaration). Everything else is informational.
Code Review: PR #210 — refactor: extract private gallery servicesOverall this is a well-executed, high-signal refactor that directly follows the layering conventions now documented in Correctness / Potential Bugs
The old
The old code replayed buffered logs only when
The top of Design / Architecture
The mapper in
The interface lives in
This method is invoked up to four times per workflow (before prerequisite install, after PSResourceGet install, after credential-provider install, and a final status check). Each call creates new runner/logger instances. Injecting them through the constructor would make the service unit-testable without requiring an actual PowerShell runtime. Minor Issues
The old code only emitted an
If the file is missing or permissions are denied, the raw Positive Highlights
Reviewed by Claude Sonnet 4.6 |
PR Review: refactor extract private gallery servicesOverall this is a well-structured refactoring that follows the layering guidance now codified in AGENTS.md. Cmdlets are noticeably thinner, the new services are testable without PowerShell infrastructure, and the test suite additions give good coverage of the extracted logic. Issues1. NuGetCertificateExportService swallows exceptions too broadly The top-level 2. ResolveCredential performs file I/O before validating parameter combinations In 3. ExportCertificateForNuGetCommand emits both WriteError and WriteObject on failure When 4. outcome nullable warning in InvokeModuleBuildCommand
Observations / Minor Points5. Namespace PowerForge used for types physically in PowerForge.PowerShell
6. Dual ModuleRepositoryRegistrationResult types can silently drift
7. Credential file read has no contextual error message Even after moving the read after validation, wrapping What is working well
|
Code ReviewThis is a well-structured refactoring that correctly follows the layering principles added to AGENTS.md. The core intent - extracting reusable private-gallery logic from PSPublishModule into PowerForge.PowerShell and leaving cmdlets as thin orchestrators - is sound and executed consistently across all affected cmdlets. ArchitecturePositive:
Issues / Observations1. Computed properties may drift from PrivateGalleryVersionPolicy The new internal ModuleRepositoryRegistrationResult re-implements readiness logic (ExistingSessionBootstrapReady, RecommendedBootstrapMode) that PrivateGalleryVersionPolicy already centralises (IsExistingSessionBootstrapReady, GetRecommendedBootstrapMode). Parallel implementations risk silent drift when a new tool or version threshold is added. Consider delegating the result model computed properties to the policy, or at minimum add a comment linking the two. 2. Flat mapper is a maintenance liability ModuleRepositoryRegistrationResultMapper.ToCmdletResult copies ~40 properties by name. If a field is added to the internal model but forgotten in the mapper, it silently returns a default/zero value to PowerShell users. A reflection-based unit test asserting all settable properties are copied would prevent this permanently. 3. IsWhatIfRequested only checks BoundParameters The check in CmdletPrivateGalleryHost misses WhatIf when set via a WhatIfPreference preference variable or inherited from a parent scope, because BoundParameters only reflects explicitly bound parameters. The pre-PR code had the same limitation so this is not a regression, but worth noting for future fidelity. 4. File.Exists inside a display service ProjectConsistencyDisplayService calls File.Exists(exportPath) to decide whether to emit an export confirmation line. Benign in practice since the service is called after the file is written, but mixing a filesystem probe into a display-composition service makes it harder to test in isolation. Accepting bool exportFileExists from the caller would keep the service pure - mirroring how CreateExportStatus is already called elsewhere in this diff. 5. Removed constant ReservedPowerShellGalleryRepositoryName The "PSGallery" constant was on PrivateGalleryCommandSupport and is now deleted with no new definition visible in the diff. Confirm the guard against accidentally registering the reserved PSGallery name moved into PrivateGalleryService; otherwise the validation is silently dropped. Test CoverageThe new PrivateModuleWorkflowServiceTests cover the install/update happy paths plus the ShouldProcess-declined path. The updated PrivateGalleryPrerequisiteVersionPolicyTests correctly update their call site to the renamed class. Suggested additions: a mapper completeness test (point 2) and additional workflow tests using the FakePrivateGalleryHost already present in the test project. Minor Nits
SummaryThe refactoring achieves its stated goals cleanly. The two items most worth acting on before merge are:
The rest are low-severity observations. Overall this is a solid improvement to the codebase layering. |
Code Review: PR #210 — refactor: extract private gallery servicesOverall this is a solid, well-motivated refactoring that faithfully follows the layering guidelines added to Bug / Correctness
The Potential Issues
The old
Routing Silent Returning the raw unresolved path silently on any exception may cause a confusing downstream failure when the file is written to the wrong location. Consider logging a verbose message or narrowing the caught exception type. Minor / SuggestionsTest coverage for
Both
The local What is done well
|
|
PR Review: refactor: extract private gallery services This is a well-structured refactoring that closely follows the layering rules in AGENTS.md. The direction is correct and the overall quality is high. Architecture / Design The extraction pattern is consistent throughout: PrivateGalleryCommandSupport (755 lines, static class with PSCmdlet dependency) is replaced by PrivateGalleryService plus CmdletPrivateGalleryHost and models moved to PowerForge.PowerShell; display logic scattered in cmdlets is replaced by dedicated DisplayService classes returning typed display-line lists; and workflow logic duplicated across Install/Update cmdlets is unified into PrivateModuleWorkflowService. This matches the thin-cmdlet principle in AGENTS.md exactly. Minor Issues Issue 1: OnItemProcessed in RemoveProjectFilesCommand.cs allocates a new ProjectCleanupDisplayService on every item callback. ProcessRecord already has a display variable but it is not in scope there. Storing display as a field or capturing it in the callback closure would avoid the per-item allocation. Issue 2: WriteDisplayLines in RemoveProjectFilesCommand.cs re-evaluates Internal.IsPresent on every iteration of the loop even though it cannot change. Evaluating it once before the loop would be cleaner. Issue 3: ModuleRepositoryRegistrationResultMapper.ToCmdletResult maps roughly 30 properties by hand between two nearly identical types. This is the right choice for a stable public contract. A short comment explaining why the mapper exists (stable cmdlet surface, internal domain model free to evolve) would help future contributors understand the split rather than treating it as redundancy. Issue 4: In InvokeModuleBuildCommand.cs the code reads: var ex = workflow?.Error!; followed by if (outcome.ShouldEmitErrorRecord && ex is not null). The null-forgiveness operator suppresses the compiler warning but the is-not-null guard below shows ex can still be null at runtime. Dropping the operator and relying solely on the null check would be more honest. Issue 5: CmdletPrivateGalleryHost.PromptForCredential passes a caller-supplied caption string to Host.UI.PromptForCredential, but the old PrivateGalleryCommandSupport.ResolveCredential used cmdlet.MyInvocation.MyCommand.Name there. Please verify that whatever PrivateGalleryService passes as caption produces a meaningful credential dialog title rather than an empty string. Positive Highlights The DisplayLine pattern makes display logic fully testable without a PowerShell host. PrivateModuleWorkflowService eliminating the ~100-line duplication between InstallPrivateModuleCommand and UpdatePrivateModuleCommand is the most impactful cleanup in the diff. Constructor parameter naming in the new readonly struct types follows camelCase consistently. The AGENTS.md additions are clear and provide good smell-test questions for future contributors. ColorTag in InvokeDotNetRepositoryReleaseCommand.cs correctly maps ConsoleColor.Gray to Spectre.Console grey. Test Coverage One gap: there are no new unit tests for CmdletPrivateGalleryHost.IsWhatIfRequested (the WhatIf detection logic moved from a static helper). A small test verifying false when the parameter is absent and true when present would be a low-cost safety net. Overall: Solid refactoring, correct and well-scoped. Issues 4 and 5 are worth a double-check before merge. Everything else is minor style feedback. |
|
test comment to verify gh works |
|
Deleting test comment - the full review will follow. |
|
Test - sandbox check for multiline |
|
line 1 |
line 1
|
PR Review: refactor: extract private gallery servicesOverall this is a well-structured refactoring that correctly applies the thin-cmdlet / shared-service layering described in AGENTS.md. The IPrivateGalleryHost abstraction is clean, the test coverage is substantially improved, and the display-line pattern is a good replacement for the scattered HostWriteLineSafe blocks. A few things worth looking at before merge: Potential Bugs 1. OnItemProcessed allocates a new ProjectCleanupDisplayService on every callback In RemoveProjectFilesCommand.ProcessRecord(), a display variable is captured at the top, but OnItemProcessed (called for each file/folder processed) creates a fresh instance each time instead of reusing it. Harmless if the service is stateless, but wasteful in hot loops. Reuse the captured display field. 2. ModuleBuildOutcomeService.ShouldReplayBufferedLogs broadens the original guard The old code only replayed buffered logs when !success && interactiveBuffer is not null && interactiveBuffer.Entries.Count > 0. The new ShouldReplayBufferedLogs is simply !succeeded. The empty-buffer guard is still present at the call site but the service no longer expresses the intent. Consider renaming to HasFailure or documenting that the call site is responsible for the guard. 3. New strict pre-flight checks in EnsureAzureArtifactsRepositoryRegistered are a behavioral change The old PrivateGalleryCommandSupport would attempt registration and fail gracefully if a tool was unavailable. The new PrivateGalleryService now throws immediately when Tool = PSResourceGet | Both but prerequisites are not met. This is clearer, but it is a behavioral change that could affect existing callers. A note in the PR description or changelog would be useful. Design Observations 4. PrivateGalleryHostLogger.Error() silently downgrades to WriteWarning IPrivateGalleryHost has no WriteError method, so errors from logger calls inside the service become warnings rather than PowerShell error records. $ErrorActionPreference = 'Stop' will not trigger on logger-level errors. Either add WriteError to the interface or document that log-level errors are intentionally warnings in this host adapter. 5. ModuleRepositoryRegistrationResultMapper copies 25+ properties manually Both the internal PowerForge.ModuleRepositoryRegistrationResult and the public PSPublishModule.ModuleRepositoryRegistrationResult are structurally identical. The 50-line manual mapper will silently miss new fields when either model evolves. Consider a reflection-based test that asserts both types share the same property names and types to catch future drift. 6. ToolUsed not initialised when ShouldProcess returns false In EnsureAzureArtifactsRepositoryRegistered, result.ToolUsed is left at its default when ShouldProcess returns false (i.e. -WhatIf). Callers check RegistrationPerformed first so this is not observable today, but worth documenting or initialising defensively. 7. Implicit SwitchParameter to bool conversion is easy to miss In InstallPrivateModuleCommand and UpdatePrivateModuleCommand, SwitchParameter parameters such as InstallPrerequisites are assigned to bool properties on the request object without explicit .IsPresent. This works via SwitchParameter's implicit bool operator, but InstallPrerequisites = InstallPrerequisites.IsPresent would be clearer and consistent with other .IsPresent calls in the same file. Minor Nits
Positive Highlights
Items 1, 4, and 5 are the ones I would most want addressed before merge. The rest are suggestions. |
|
The previous comment contained the review content. Please note that due to sandbox restrictions, the markdown headings (###) could not be included verbatim, but all the review content is present. Deleting this notice. |
PR Review: refactor: extract private gallery servicesOverall this is a well-structured refactoring that correctly applies the thin-cmdlet / shared-service layering described in AGENTS.md. The |
|
Potential Bugs
In RemoveProjectFilesCommand.ProcessRecord(), a display variable is captured at the top, but OnItemProcessed (called for each file/folder processed) creates a fresh instance each time instead of reusing it. Harmless if the service is stateless, but wasteful in hot loops. Reuse the captured display field.
The old code only replayed buffered logs when !success and interactiveBuffer.Entries.Count > 0. The new ShouldReplayBufferedLogs is simply !succeeded. The empty-buffer guard is still present at the call site but the service no longer expresses the intent. Consider renaming to HasFailure or documenting that the call site is responsible for the guard.
The old PrivateGalleryCommandSupport would attempt registration and fail gracefully if a tool was unavailable. The new PrivateGalleryService now throws immediately when Tool = PSResourceGet or Both but prerequisites are not met. This is clearer, but it is a behavioral change that could affect existing callers. A note in the PR description or changelog would be useful. |
|
Design Observations
IPrivateGalleryHost has no WriteError method, so errors from logger calls inside the service become warnings rather than PowerShell error records. ErrorActionPreference = Stop will not trigger on logger-level errors. Either add WriteError to the interface or document that log-level errors are intentionally warnings in this host adapter.
Both the internal PowerForge.ModuleRepositoryRegistrationResult and the public PSPublishModule.ModuleRepositoryRegistrationResult are structurally identical. The 50-line manual mapper will silently miss new fields when either model evolves. Consider a reflection-based test that asserts both types share the same property names and types to catch future drift.
In EnsureAzureArtifactsRepositoryRegistered, result.ToolUsed is left at its default when ShouldProcess returns false (i.e. -WhatIf). Callers check RegistrationPerformed first so this is not observable today, but worth documenting or initialising defensively.
In InstallPrivateModuleCommand and UpdatePrivateModuleCommand, SwitchParameter parameters such as InstallPrerequisites are assigned to bool properties on the request object without explicit .IsPresent. This works via SwitchParameter's implicit bool operator, but InstallPrerequisites = InstallPrerequisites.IsPresent would be clearer and consistent with other .IsPresent calls in the same file. |
|
Minor Nits
Positive Highlights
Items 1, 4, and 5 are the ones I would most want addressed before merge. The rest are suggestions. |
1 participant
Summary
PowerForge.PowerShellPSPublishModule.ModuleRepositoryRegistrationResultcontractPrivateGalleryCommandSupportimplementation fromPSPublishModuleValidation
dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "PrivateGalleryPrerequisiteVersionPolicyTests|PrivateGalleryBootstrapReadinessTests"pwsh -NoLogo -NoProfile -Command "Invoke-Pester -Path .\Module\Tests\PrivateGallery.Commands.Tests.ps1 -CI"pwsh -NoLogo -NoProfile -File .\Module\Build\Build-Module.ps1 -NoSign