feat: Allow Add-PatServer -Force to overwrite an existing entry#42
Conversation
Closes #40. Matches PowerShell convention for -Force on creation cmdlets (New-Item, Copy-Item, Register-PSRepository): on name collision, replace the entry wholesale rather than throwing. The existing vault-stored token is removed before the new entry is written. Without -Force the duplicate-name error now points at both -Force and Update-PatServerToken as concrete recovery paths. Replace, not merge: fields not supplied on the new call (localUri, preferLocal, default) are not preserved. Use Update-PatServerToken for in-place token rotation that preserves other fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change makes Add-PatServer obey PowerShell-style -Force by overwriting an existing server entry when names collide, removing any prior vault-stored token and writing the new entry; without -Force it still errors with guidance to use -Force or Update-PatServerToken. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddPatServer
participant ConfigStore
participant Vault
User->>AddPatServer: Invoke Add-PatServer -Name X -Token ... [-Force]
AddPatServer->>ConfigStore: Check for existing server named X
alt server not found
AddPatServer->>ConfigStore: Append new server entry
AddPatServer->>ConfigStore: Persist config
else server found and -Force not set
AddPatServer-->>User: Throw duplicate-name error with guidance
else server found and -Force set
AddPatServer->>AddPatServer: Enter ShouldProcess
AddPatServer->>Vault: If old token in vault -> Remove-PatServerToken (delete)
AddPatServer->>ConfigStore: Remove old server entry
AddPatServer->>ConfigStore: Append new server entry (wholesale)
AddPatServer->>ConfigStore: Persist config
AddPatServer-->>User: Return success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates Add-PatServer -Force to follow common PowerShell -Force semantics by allowing an existing server entry with the same -Name to be replaced, and documents/tests that overwrite behavior (including vault token cleanup).
Changes:
- Allow
Add-PatServer -Forceto replace an existing server entry (wholesale replacement) and remove any prior vault token before writing the new entry. - Improve the duplicate-name error message to explicitly suggest
-Force(replace) andUpdate-PatServerToken(token rotation). - Add Pester tests for overwrite behavior and update changelog/help documentation accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
PlexAutomationToolkit/Public/Add-PatServer.ps1 |
Implements -Force overwrite behavior, updates help text and duplicate-name error message. |
tests/Unit/Public/Add-PatServer.tests.ps1 |
Adds unit tests covering overwrite behavior and vault token cleanup invocation. |
CHANGELOG.md |
Documents the new -Force overwrite semantics and guidance to use Update-PatServerToken for token-only rotation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PlexAutomationToolkit/Public/Add-PatServer.ps1 (1)
192-335:⚠️ Potential issue | 🔴 CriticalMove all state changes behind
ShouldProcess.
Remove-PatServerToken, token storage, and$configuration.servers += $newServerall happen before theShouldProcesscheck, so-WhatIfor a cancelled confirmation still mutates the in-memory config and can delete the existing vault token. That makes the new overwrite path non-transactional and can break the previously working server entry even when the add is never committed.Proposed fix
- $configuration = Get-PatServerConfiguration -ErrorAction Stop - ... - if ($existingServer) { - ... - Remove-PatServerToken -ServerName $Name - $configuration.servers = @($configuration.servers | Where-Object { $_.name -ne $Name }) - } - ... - $configuration.servers += $newServer - - if ($PSCmdlet.ShouldProcess($Name, 'Add server to configuration')) { - Set-PatServerConfiguration -Configuration $configuration -ErrorAction Stop - Write-Verbose "Added server '$Name' to configuration" - } + $configuration = Get-PatServerConfiguration -ErrorAction Stop + if ($PSCmdlet.ShouldProcess($Name, 'Add server to configuration')) { + ... + if ($existingServer) { + Remove-PatServerToken -ServerName $Name + $configuration.servers = @($configuration.servers | Where-Object { $_.name -ne $Name }) + } + ... + $configuration.servers += $newServer + Set-PatServerConfiguration -Configuration $configuration -ErrorAction Stop + Write-Verbose "Added server '$Name' to configuration" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PlexAutomationToolkit/Public/Add-PatServer.ps1` around lines 192 - 335, The code mutates state (calls Remove-PatServerToken, Set-PatServerToken, adds token members to $newServer, and appends to $configuration.servers) before honoring $PSCmdlet.ShouldProcess; change to prepare all data in temporaries (e.g., $pendingServer, $pendingTokenStorage) and defer any destructive or persistent actions — Remove-PatServerToken, Set-PatServerToken, adding token/tokenInVault members to the server object, and $configuration.servers += $newServer — until inside the ShouldProcess block where you then perform those operations and call Set-PatServerConfiguration; keep read-only validation (Get-PatDetectedLocalUri, Invoke-PatApi) outside but ensure all writes occur only after $PSCmdlet.ShouldProcess($Name, ...) returns true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@PlexAutomationToolkit/Public/Add-PatServer.ps1`:
- Around line 192-335: The code mutates state (calls Remove-PatServerToken,
Set-PatServerToken, adds token members to $newServer, and appends to
$configuration.servers) before honoring $PSCmdlet.ShouldProcess; change to
prepare all data in temporaries (e.g., $pendingServer, $pendingTokenStorage) and
defer any destructive or persistent actions — Remove-PatServerToken,
Set-PatServerToken, adding token/tokenInVault members to the server object, and
$configuration.servers += $newServer — until inside the ShouldProcess block
where you then perform those operations and call Set-PatServerConfiguration;
keep read-only validation (Get-PatDetectedLocalUri, Invoke-PatApi) outside but
ensure all writes occur only after $PSCmdlet.ShouldProcess($Name, ...) returns
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a86567a4-c3fc-4835-b0ab-d08307144972
📒 Files selected for processing (3)
CHANGELOG.mdPlexAutomationToolkit/Public/Add-PatServer.ps1tests/Unit/Public/Add-PatServer.tests.ps1
Addresses review feedback from Copilot and CodeRabbit on PR #42. The duplicate-name detection still fires early so -Force-less callers get fast feedback, but the destructive overwrite operations -- removing the existing entry from the in-memory configuration and clearing its vault token -- now run inside the existing ShouldProcess block. Under -WhatIf or a declined -Confirm, these no longer execute. Remove-PatServerToken is also gated on $newServer.tokenInVault -ne $true, so when the new -Token was stored in the vault it doesn't clobber the just-written entry. Tests: - Renamed the existing overwrite vault-cleanup test to reflect that the cleanup is conditional on the new entry not being vault-stored. - Added a test asserting Remove-PatServerToken is NOT called when the new -Token is vault-stored. - Added a test asserting -Force overwrite under -WhatIf invokes neither Remove-PatServerToken nor Set-PatServerConfiguration, and leaves the prior in-memory config intact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Re: CodeRabbit's outside-diff finding on The destructive ops introduced by this PR (the in-memory removal and the The proposed full restructure (pulling Verified by a new test that asserts no |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PlexAutomationToolkit/Public/Add-PatServer.ps1`:
- Around line 325-336: The vault writes are occurring before the ShouldProcess
check (Set-PatServerToken calls at the token handling/validation paths and
Remove-PatServerToken in the Add-PatServer flow), so move any calls that mutate
the vault (Set-PatServerToken, Remove-PatServerToken or similar) into the
ShouldProcess block where the server is actually added; keep token validation
and any in-memory token parsing/verification earlier but do not persist to the
vault until after PSCmdlet.ShouldProcess approves; add a regression test
covering the -Force -WhatIf (and -Confirm declined) case to ensure no vault
writes happen during WhatIf/preview.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c98f926-489a-4065-82e0-8ed61e7f8b55
📒 Files selected for processing (2)
PlexAutomationToolkit/Public/Add-PatServer.ps1tests/Unit/Public/Add-PatServer.tests.ps1
Closes #40.
Summary
Add-PatServer -Forcenow replaces an existing entry with the same-Nameinstead of throwing, matching PowerShell convention for-Forceon creation cmdlets (New-Item,Copy-Item,Register-PSRepository).localUri,preferLocal,default) are not preserved. Any vault-stored token for the prior entry is removed before the new entry is written, so a fresh-Tokencleanly takes over.-Forcenow names both recovery paths concretely:-Forcefor replace,Update-PatServerTokenfor in-place token rotation that preserves other fields.Why replace, not merge
Microsoft's cmdlet design guidance treats
-Forceas "override a restriction that would normally block the command" — not as a way to change the cmdlet's fundamental semantics. Field-level updates belong in a separateSet-*/Update-*cmdlet. The module already follows this split (Update-PatServerToken,Set-PatDefaultServer); a futureSet-PatServercould fill the partial-update gap if it's needed.Test plan
Invoke-Pester tests/Unit/Public/Add-PatServer.tests.ps1— 51/51 pass, including 6 new tests covering: throw without-Force, the new error message naming both-ForceandUpdate-PatServerToken, overwrite replaces uri/token, vault token cleanup is invoked, replace is wholesale (no field merge), other servers are preserved, and-Forceon a brand-new name does not invokeRemove-PatServerToken.pwsh -File ./build.ps1 -Task Testpasses.Add-PatServer -Name 'plex' -ServerUri 'https://example:32400' -Token $newToken -Forceafter a prior add now succeeds.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Add-PatServer -Forcenow overwrites an existing server entry instead of erroring on duplicate names.Bug Fixes
-Force) now include clearer guidance on recovery options.Documentation
-Forceoverwrite behavior and recommend token-rotation workflow.Tests