feat: add site:shared push pull commands for shared files - #91
feat: add site:shared push pull commands for shared files#91loadinglucian wants to merge 3 commits into
Conversation
WalkthroughAdds two new console commands (site:shared:pull and site:shared:push) for downloading/uploading files to a site's shared directory, plus helper trait methods to resolve site root/shared paths and to resolve a server for a site; includes path validation, SSH transfer logic, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PullCmd as "SiteSharedPullCommand"
participant Resolver as "Site/Server Resolver"
participant SSH
User->>PullCmd: run site:shared:pull (options)
PullCmd->>PullCmd: validate options & normalize remote path
PullCmd->>Resolver: select site & resolve server
Resolver-->>PullCmd: SiteDTO, ServerDTO
PullCmd->>SSH: check remote file exists
alt file exists
PullCmd->>PullCmd: resolve local path & prompt overwrite
PullCmd->>SSH: download file
SSH-->>PullCmd: transfer result
PullCmd->>User: success / exit 0
else missing / error
PullCmd->>User: error message / non-zero exit
end
sequenceDiagram
participant User
participant PushCmd as "SiteSharedPushCommand"
participant Resolver as "Site/Server Resolver"
participant SSH
User->>PushCmd: run site:shared:push (options)
PushCmd->>PushCmd: validate options & expand local path
PushCmd->>Resolver: select site & resolve server
Resolver-->>PushCmd: SiteDTO, ServerDTO
PushCmd->>PushCmd: normalize remote relative path
PushCmd->>SSH: ensure remote directory exists
SSH-->>PushCmd: dir ready
PushCmd->>SSH: upload file
SSH-->>PushCmd: upload result
alt success
PushCmd->>SSH: set permissions & ownership
PushCmd->>User: success / exit 0
else failure
PushCmd->>User: error message / non-zero exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
app/Console/Site/SiteSharedPushCommand.php (1)
86-92: Replay hint usessitekey; keep it in sync with the chosen option nameOnce you decide whether the canonical option is
siteordomain, make sure the array passed intoshowCommandReplay()uses that same key so the generated replay command is actually runnable (e.g.--domain=example.comif you align withselectSite()).Right now it uses
'site' => $site->domain, which will need to be updated if you switch the option name as suggested earlier.app/Console/Site/SiteSharedPullCommand.php (4)
91-100: Download + error handling look good; remaining phpstan issue is$servertypeThe download block correctly logs what is happening, calls
SSHService::downloadFile(), and catches\RuntimeExceptionto report an error and returnCommand::FAILURE. This is consistent with the rest of the app’s SSH usage.The only outstanding issue here is phpstan’s “mixed given” for
$server, which should be resolved by typinggetServerForSite()and adding the inline/** @var ServerDTO|int $server */annotation as mentioned above.
104-108: Replay hint usessitekey; keep in sync with the canonical optionReplay currently uses
'site' => $site->domain. After you decide whether the canonical CLI flag is--siteor--domain, ensure this key matches so the generated replay is actually runnable.If you switch the option to
domainas suggested, you likely want:- 'site' => $site->domain, + 'domain' => $site->domain,
43-51:getServerForSite()missing and$servertyped asmixed(same root cause as push)Line 48 calls
$this->getServerForSite($site), but phpstan reports that method as undefined on this command. If not provided byServersTraitorBaseCommand, this will be a runtime fatal.Phpstan also reports
$serverasmixedwhen passed toremoteFileExists()andSSHService::downloadFile(). As in the push command, the flow assumesServerDTO|int.Suggested steps (same as in
SiteSharedPushCommand):
- Implement
getServerForSite(SiteDTO $site): ServerDTO|intin a shared place (trait/service), and- Add an inline type hint for phpstan:
- $server = $this->getServerForSite($site); + /** @var ServerDTO|int $server */ + $server = $this->getServerForSite($site);
179-188:buildSharedPath()relies on missing / loosely-typedgetSiteSharedPath()(same as push)As in
SiteSharedPushCommand, this method depends ongetSiteSharedPath($site)but phpstan reports that method as undefined, and then flagsrtrim($sharedRoot, '/')and the return type asmixed.To avoid runtime fatals and clean up phpstan:
- Implement
getSiteSharedPath(SiteDTO $site): stringin a shared place (trait/service), and- Optionally cast or document the type here:
$sharedRoot = (string) $this->getSiteSharedPath($site);This will also make the phpstan “rtrim expects string” and “returns mixed” errors go away.
🧹 Nitpick comments (5)
app/Console/Site/SiteSharedPushCommand.php (2)
53-124:resolveLocalPath()logic is solid; consider sharing/inlining per guidelinesThe local path resolution (option-or-prompt,
expandPath(), existence +is_file()check) is robust and user-friendly.Given the
*.phpguideline to avoid single-use helpers and the fact that similar logic exists inSiteSharedPullCommand::resolveLocalPath(), you might:
- Move shared local-path resolution into a dedicated service used by both push/pull commands, or
- If you don’t expect reuse, inline this method into
execute()to avoid another single-use helper.This is not a blocker, just a maintainability improvement.
126-170: Remote filename normalization is safe but duplicated between commands
resolveRemotePath()andnormalizeRelativePath()correctly:
- Default to the local basename,
- Normalize separators,
- Strip leading
/,- Reject empty strings and any path containing
..to keep paths safely undershared/.The same normalization logic is duplicated in
SiteSharedPullCommand. Per the architecture learning that commands should not duplicate orchestration logic but delegate it to shared services, consider extracting this into a small shared “SharedPathService” (or similar) that both commands call. That would also address the guideline about avoiding single-use methods in commands.app/Console/Site/SiteSharedPullCommand.php (3)
53-71: Remote existence check is robust; helper could be sharedThe
remoteFileExists()helper usestest -fand interprets exit codes (0= exists,1= missing, other = error) andexecute()wraps it in atry/catchthat reports failures cleanly. This is a good pattern.However, this is orchestration logic that could be reused elsewhere (e.g., future commands). Per the “commands should not duplicate orchestration logic; extract to shared services” learning, consider moving
remoteFileExists()into a shared SSH/files helper service and having both commands call that instead of each defining their own helpers.Also applies to: 190-209
72-89: Local overwrite handling is user-friendly; consider guarding directory overwrite edge caseThe overwrite prompt when
$this->fs->exists($localPath)is a nice UX touch, and returningCommand::SUCCESSwhen the user cancels is reasonable.One minor edge case: if
$localPathis an existing directory,downloadFile()(and ultimatelydumpFile()) will likely fail at runtime. You may want to special-caseis_dir($localPath)and treat it as an error early with a clearer message.
113-177: Remote path resolution mirrors push; good safety but duplicated
resolveRemotePath()andnormalizeRelativePath()mirror the push command and provide:
- Option-or-prompt input with sensible defaults,
- Separator normalization,
- Rejection of empty or traversal (
..) paths,- Stripping of leading
/to keep paths undershared/.This is correct and safe, but now you have effectively the same logic in both pull and push commands. To avoid duplicated orchestration in commands (per the architecture note) and to conform with the guideline against single-use helpers, consider extracting the normalization/building logic into a dedicated shared service used by both commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Console/Site/SiteSharedPullCommand.php(1 hunks)app/Console/Site/SiteSharedPushCommand.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Site/SiteSharedPullCommand.phpapp/Console/Site/SiteSharedPushCommand.php
🧠 Learnings (1)
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands must not duplicate orchestration logic—extract to shared Services
Applied to files:
app/Console/Site/SiteSharedPullCommand.php
🧬 Code graph analysis (1)
app/Console/Site/SiteSharedPullCommand.php (7)
app/Contracts/BaseCommand.php (5)
BaseCommand(30-244)heading(168-172)nay(186-190)yay(177-181)showCommandReplay(197-243)app/DTOs/ServerDTO.php (1)
ServerDTO(7-19)app/DTOs/SiteDTO.php (1)
SiteDTO(7-24)app/Traits/SitesTrait.php (1)
selectSite(70-106)app/Services/FilesystemService.php (2)
exists(42-45)expandPath(113-143)app/Services/IOService.php (3)
promptConfirm(261-275)writeln(463-469)getOptionOrPrompt(84-137)app/Services/SSHService.php (2)
downloadFile(139-158)executeCommand(69-102)
🪛 GitHub Check: phpstan
app/Console/Site/SiteSharedPullCommand.php
[failure] 187-187:
Parameter #1 $string of function rtrim expects string, mixed given.
[failure] 184-184:
Method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPullCommand::buildSharedPath() should return string but returns mixed.
[failure] 181-181:
Call to an undefined method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPullCommand::getSiteSharedPath().
[failure] 95-95:
Parameter #1 $server of method Bigpixelrocket\DeployerPHP\Services\SSHService::downloadFile() expects Bigpixelrocket\DeployerPHP\DTOs\ServerDTO, mixed given.
[failure] 61-61:
Parameter #1 $server of method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPullCommand::remoteFileExists() expects Bigpixelrocket\DeployerPHP\DTOs\ServerDTO, mixed given.
[failure] 48-48:
Call to an undefined method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPullCommand::getServerForSite().
app/Console/Site/SiteSharedPushCommand.php
[failure] 72-72:
Parameter #1 $server of method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPushCommand::runRemoteCommand() expects Bigpixelrocket\DeployerPHP\DTOs\ServerDTO, mixed given.
[failure] 71-71:
Parameter #1 $server of method Bigpixelrocket\DeployerPHP\Services\SSHService::uploadFile() expects Bigpixelrocket\DeployerPHP\DTOs\ServerDTO, mixed given.
[failure] 70-70:
Parameter #1 $server of method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPushCommand::runRemoteCommand() expects Bigpixelrocket\DeployerPHP\DTOs\ServerDTO, mixed given.
[failure] 48-48:
Call to an undefined method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPushCommand::getServerForSite().
🔇 Additional comments (1)
app/Console/Site/SiteSharedPushCommand.php (1)
69-80: Remote operations and error handling look goodThe pattern of:
- Wrapping remote shell calls in
runRemoteCommand()withexit_codechecking, and- Using
escapeshellarg()for all interpolated paths,is solid and protects against both silent failures and shell injection. The
try/catcharoundmkdir,uploadFile,chmod,chowncorrectly funnels all failures into a clear error message andCommand::FAILURE.Also applies to: 183-192
| $site = $this->selectSite(); | ||
| if (is_int($site)) { | ||
| return $site; | ||
| } | ||
|
|
||
| $server = $this->getServerForSite($site); | ||
| if (is_int($server)) { | ||
| return $server; | ||
| } | ||
|
|
There was a problem hiding this comment.
getServerForSite() is missing and $server is mixed (phpstan + runtime)
At Line 48 you call $this->getServerForSite($site), but phpstan reports this method as undefined on the command class. If it’s not provided by ServersTrait or BaseCommand, this will be a fatal error at runtime.
In addition, phpstan warns that $server is mixed when passed to methods expecting ServerDTO (runRemoteCommand(), SSHService::uploadFile()), even though the flow clearly assumes ServerDTO|int (int as error code):
$server = $this->getServerForSite($site);
if (is_int($server)) {
return $server;
}Consider:
- Ensuring
getServerForSite(SiteDTO $site): ServerDTO|intis implemented inServersTraitor a shared service, and - Documenting the type at the call site to help static analysis:
- $server = $this->getServerForSite($site);
+ /** @var ServerDTO|int $server */
+ $server = $this->getServerForSite($site);Once getServerForSite() exists and is typed, the phpstan “mixed given” errors at Lines 70–72 should also disappear.
🧰 Tools
🪛 GitHub Check: phpstan
[failure] 48-48:
Call to an undefined method Bigpixelrocket\DeployerPHP\Console\Site\SiteSharedPushCommand::getServerForSite().
🤖 Prompt for AI Agents
In app/Console/Site/SiteSharedPushCommand.php around lines 43 to 52, the call to
$this->getServerForSite($site) is to a missing/untagged method which makes
$server a mixed type for phpstan and can cause a runtime fatal error; implement
getServerForSite(SiteDTO $site): ServerDTO|int in a shared location (preferably
ServersTrait or BaseCommand) or add the method to this class, ensure it returns
a ServerDTO on success or an int error code, add the proper use/import for
ServerDTO, and then annotate the call site (docblock or inline phpcs/phpstan
type hint) so $server is recognized as ServerDTO|int to eliminate the “mixed
given” warnings and allow subsequent calls (runRemoteCommand,
SSHService::uploadFile) to accept a typed ServerDTO.
| private function buildSharedPath(SiteDTO $site, string $relative = ''): string | ||
| { | ||
| $sharedRoot = $this->getSiteSharedPath($site); | ||
|
|
||
| if ($relative === '') { | ||
| return $sharedRoot; | ||
| } | ||
|
|
||
| return rtrim($sharedRoot, '/').'/'.ltrim($relative, '/'); | ||
| } |
There was a problem hiding this comment.
buildSharedPath() depends on undefined / loosely-typed getSiteSharedPath()
buildSharedPath() assumes a getSiteSharedPath(SiteDTO $site) helper, but phpstan reports this method as undefined on the command class. If no trait or base class provides it, this will also be a fatal error at runtime when you build the remote path.
Additionally, phpstan flags:
rtrim($sharedRoot, '/')with$sharedRootasmixed, andbuildSharedPath()returningmixedinstead ofstring.
Once getSiteSharedPath() exists and is declared with a string return type (or at least documented as @return string), these type complaints should go away.
Action items:
- Ensure
getSiteSharedPath()is implemented in a shared place (likelySitesTraitor a dedicated service) and returnsstring. - Optionally add a cast to make intent explicit:
$sharedRoot = (string) $this->getSiteSharedPath($site);🤖 Prompt for AI Agents
In app/Console/Site/SiteSharedPushCommand.php around lines 172 to 181,
buildSharedPath() calls getSiteSharedPath($site) which phpstan reports as
undefined and returning mixed; implement getSiteSharedPath(SiteDTO $site):
string in an appropriate shared location (e.g. SitesTrait or a dedicated
service) so the method exists and has a string return type, update the class to
use that trait/service, and in buildSharedPath() explicitly cast the result to
string (e.g. $sharedRoot = (string) $this->getSiteSharedPath($site)) to satisfy
static analysis and ensure rtrim() and the return type are correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Console/Site/SiteSharedPullCommand.php (1)
37-40: Past concern about option naming has been resolved.The previous review flagged a mismatch between
siteanddomainoption names. This has been correctly addressed—the command now usesdomain(line 38), which aligns withSitesTrait::selectSite().
🧹 Nitpick comments (5)
app/Console/Site/SiteSharedPullCommand.php (5)
171-190: Consider inlining single-use method.
resolveRemotePath()is called only once (line 91). Per coding guidelines, consider inlining this logic directly into theexecute()method to eliminate method call overhead.As per coding guidelines.
192-213: Consider inlining single-use method.
resolveLocalPath()is called only once (line 114). Per coding guidelines, consider inlining this logic directly into theexecute()method to eliminate method call overhead.As per coding guidelines.
215-235: Consider inlining single-use path validation.
normalizeRelativePath()is called only once (line 183). Per coding guidelines, consider inlining this validation logic. However, note that this method contains security-critical path traversal prevention (line 228), which benefits from being isolated for testing and clarity.As per coding guidelines.
237-246: Consider inlining single-use method.
buildSharedPath()is called only once (line 96). Per coding guidelines, consider inlining this path construction logic directly into theexecute()method to eliminate method call overhead.As per coding guidelines.
248-267: Consider inlining single-use method.
remoteFileExists()is called only once (line 103). Per coding guidelines, consider inlining this SSH file existence check directly into theexecute()method to eliminate method call overhead.As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Console/Site/SiteSharedPullCommand.php(1 hunks)app/Console/Site/SiteSharedPushCommand.php(1 hunks)app/Traits/SitesTrait.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Console/Site/SiteSharedPushCommand.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Site/SiteSharedPullCommand.phpapp/Traits/SitesTrait.php
🧠 Learnings (1)
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands must not duplicate orchestration logic—extract to shared Services
Applied to files:
app/Console/Site/SiteSharedPullCommand.php
🧬 Code graph analysis (1)
app/Traits/SitesTrait.php (1)
app/DTOs/SiteDTO.php (1)
SiteDTO(7-24)
🔇 Additional comments (1)
app/Console/Site/SiteSharedPullCommand.php (1)
47-165: Extract duplicated path resolution logic to a shared service.Both
SiteSharedPullCommandandSiteSharedPushCommandduplicatenormalizeRelativePath()andbuildSharedPath()methods with identical implementations. Extract these to a dedicated service to comply with architectural guidelines and eliminate code duplication.⛔ Skipped due to learnings
Learnt from: CR Repo: deployer-plus/deployer-php PR: 0 File: .cursor/rules/01-architecture.mdc:0-0 Timestamp: 2025-09-22T11:10:21.459Z Learning: Applies to app/**/Command/**/*.php : Commands must not duplicate orchestration logic—extract to shared ServicesLearnt from: CR Repo: deployer-plus/deployer-php PR: 0 File: .cursor/rules/01-architecture.mdc:0-0 Timestamp: 2025-09-22T11:10:21.459Z Learning: Applies to app/**/@(Service|Services)/**/*.php : Extract complex orchestration shared by multiple Commands into dedicated Services
| /** | ||
| * Get the remote root path for a site. | ||
| */ | ||
| protected function getSiteRootPath(SiteDTO $site): string | ||
| { | ||
| return '/home/deployer/sites/'.$site->domain; | ||
| } | ||
|
|
||
| /** | ||
| * Get the remote shared directory path for a site. | ||
| */ | ||
| protected function getSiteSharedPath(SiteDTO $site): string | ||
| { | ||
| return $this->getSiteRootPath($site).'/shared'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify usage count of new path helper methods.
The coding guidelines require eliminating single-use methods. Please verify that both getSiteRootPath and getSiteSharedPath are used in multiple commands (likely both push and pull commands as suggested by the PR context).
Run the following script to verify usage:
🏁 Script executed:
#!/bin/bash
# Description: Verify usage count of new site path helper methods
echo "=== getSiteRootPath usage ==="
rg -n "getSiteRootPath" --type=php -C2
echo ""
echo "=== getSiteSharedPath usage ==="
rg -n "getSiteSharedPath" --type=php -C2Length of output: 1843
Inline getSiteRootPath into getSiteSharedPath per coding guidelines.
The script confirms that getSiteRootPath is called only once (within getSiteSharedPath), violating the guideline to eliminate single-use methods. Since getSiteSharedPath itself is correctly used in both SiteSharedPushCommand and SiteSharedPullCommand, it should remain; however, getSiteRootPath should be inlined.
Refactor getSiteSharedPath at lines 216–221 to:
protected function getSiteSharedPath(SiteDTO $site): string
{
return '/home/deployer/sites/'.$site->domain.'/shared';
}🤖 Prompt for AI Agents
In app/Traits/SitesTrait.php around lines 207 to 221, inline the single-use
getSiteRootPath into getSiteSharedPath: replace getSiteSharedPath to directly
return '/home/deployer/sites/'.$site->domain.'/shared' and remove the now-unused
getSiteRootPath method; ensure all callers still use getSiteSharedPath and run
tests/lint.
1 participant
Summary by CodeRabbit
New Features
Improvements