feat: server crud commands - #27
Conversation
WalkthroughAdds three new server CLI commands (add, delete, list), introduces shared server helper/validation traits, injects SSHService into BaseCommand, adjusts SSHService exception handling, and registers commands in SymfonyApp. Updates docs to exclude tests from PHPStan. Expands tests with new integration and unit coverage and updates fixtures/helpers for SSH wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Symfony Console
participant Cmd as ServerAddCommand
participant Repo as ServerRepository
participant SSH as SSHService
User->>CLI: server:add [--name --host --port --user --key --skip]
CLI->>Cmd: execute()
Cmd->>Cmd: validateHost/validatePort
alt skip SSH test
Note over Cmd: SSH check skipped
else test SSH
Cmd->>SSH: connect(host, port, user, key)
SSH-->>Cmd: success/failure
alt failure
Cmd-->>User: show troubleshooting + hint
Cmd-->>CLI: return FAILURE
end
end
Cmd->>User: prompt confirm save
alt confirmed
Cmd->>Repo: save(ServerDTO)
Repo-->>Cmd: ok
Cmd-->>User: success + usage hint
else declined
Cmd-->>User: cancelled
end
Cmd-->>CLI: return SUCCESS
sequenceDiagram
autonumber
actor User
participant CLI as Symfony Console
participant Cmd as ServerDeleteCommand
participant Repo as ServerRepository
User->>CLI: server:delete [--name] [-y]
CLI->>Cmd: execute()
Cmd->>Repo: all()
alt none found
Cmd-->>User: no servers guidance
Cmd-->>CLI: SUCCESS
else servers exist
alt name not provided
Cmd->>User: select server
end
Cmd->>Repo: find by name
alt not found
Cmd-->>User: error
Cmd-->>CLI: FAILURE
else found
Cmd-->>User: show server info
alt -y provided
else prompt confirm
end
alt confirmed
Cmd->>Repo: delete(name)
Cmd-->>User: deleted + hint
Cmd-->>CLI: SUCCESS
else declined
Cmd-->>User: cancelled
Cmd-->>CLI: SUCCESS
end
end
end
sequenceDiagram
autonumber
actor User
participant CLI as Symfony Console
participant Cmd as ServerListCommand
participant Repo as ServerRepository
User->>CLI: server:list
CLI->>Cmd: execute()
Cmd->>Repo: all()
alt empty
Cmd-->>User: warning + add hint
else non-empty
loop each server
Cmd-->>User: displayServerInfo
end
end
Cmd-->>CLI: SUCCESS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
🧹 Nitpick comments (3)
.cursor/rules/01-architecture.mdc (1)
144-145: Consider removing extra space in Markdown formatting.There's an extra space before the closing
**in "Important: **". For consistency with Markdown conventions, consider removing it.Apply this diff:
-**Important: ** Don't run PHPStan on test files; tests are excluded from static analysis. +**Important:** Don't run PHPStan on test files; tests are excluded from static analysis.app/Traits/ServerHelpersTrait.php (1)
7-33: Drop the unuseduseimports
ServerRepositoryandSSHServiceare never referenced in this trait. Please remove the unused imports so the file stays PSR‑12 compliant.-use Bigpixelrocket\DeployerPHP\Repositories\ServerRepository; -use Bigpixelrocket\DeployerPHP\Services\SSHService;tests/Integration/Console/Server/ServerAddCommandTest.php (1)
48-57: Inconsistent output buffering and repetitive pattern.Output buffering with
ob_start()/ob_end_clean()is repeated across multiple tests to suppress Laravel Prompts output. However:
- The test at line 306-314 ("displays complete server information before saving") omits this pattern despite also using
--yes.- The repetition creates maintenance overhead.
Consider extracting this pattern:
describe('ServerAddCommand', function () { beforeEach(function () { ob_start(); }); afterEach(function () { ob_end_clean(); }); it('adds server with all options provided non-interactively', function () { // ... test without explicit ob_start/ob_end_clean }); });Alternatively, create a helper function:
function executeWithOutputSuppressed(CommandTester $tester, array $input): int { ob_start(); try { return $tester->execute($input); } finally { ob_end_clean(); } }Then update line 306 to add output buffering for consistency:
it('displays complete server information before saving', function () { // ARRANGE $sshService = mockSSHServiceWithBehavior(true); $tester = createServerAddCommandTester($sshService); // ACT + ob_start(); $tester->execute([ '--name' => 'display-test', '--host' => 'example.com', '--port' => '22', '--username' => 'deployer', '--private-key-path' => '~/.ssh/key', '--skip' => true, '--yes' => true, ]); + ob_end_clean();Also applies to: 76-82, 100-107, 123-130, 186-192, 217-224, 243-250, 278-287, 336-343
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.cursor/rules/01-architecture.mdc(1 hunks).cursor/rules/02-tests.mdc(1 hunks)app/Console/Server/ServerAddCommand.php(1 hunks)app/Console/Server/ServerDeleteCommand.php(1 hunks)app/Console/Server/ServerListCommand.php(1 hunks)app/Contracts/BaseCommand.php(2 hunks)app/Services/SSHService.php(1 hunks)app/SymfonyApp.php(2 hunks)app/Traits/ServerHelpersTrait.php(1 hunks)app/Traits/ServerValidationTrait.php(1 hunks)tests/Fixtures/TestConsoleCommand.php(4 hunks)tests/Integration/Console/Server/ServerAddCommandTest.php(1 hunks)tests/Integration/Console/Server/ServerDeleteCommandTest.php(1 hunks)tests/Integration/Console/Server/ServerListCommandTest.php(1 hunks)tests/TestHelpers.php(3 hunks)tests/Unit/Contracts/BaseCommandTest.php(4 hunks)tests/Unit/Traits/ServerHelpersTraitTest.php(1 hunks)tests/Unit/Traits/ServerValidationTraitTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use private methods by inlining them directly
Cache expensive computed values by initializing them in the constructor instead of recomputing
Prefer direct property access over method calls when appropriate to avoid call overhead
Organize code into comment-separated sections; prefer alphabetical ordering when it does not conflict with logical grouping
**/*.php: Follow PSR-12, declare strict types, and use PHP 8.x features (unions, match, attributes, readonly)
Use import statements (use) instead of fully qualified class names
All methods must have explicit return types; use proper generics in phpdoc (e.g.,Collection<int, User>)
Use dependency injection rather than manually resolving/instantiating classes
Prefer Symfony component classes (e.g., Filesystem, Process) over native PHP functions for mockability
All object creation must use$container->build(ClassName::class)except for value objects, DTOs, and pure data structures
In production code, access the container through constructor injection (no service locator pattern)
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments as visual separators for sections/subsections; avoid obvious or stale comments
Files:
app/SymfonyApp.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/TestHelpers.phpapp/Console/Server/ServerAddCommand.phptests/Unit/Traits/ServerValidationTraitTest.phptests/Integration/Console/Server/ServerListCommandTest.phpapp/Console/Server/ServerListCommand.phpapp/Traits/ServerValidationTrait.phpapp/Console/Server/ServerDeleteCommand.phptests/Fixtures/TestConsoleCommand.phpapp/Traits/ServerHelpersTrait.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.phpapp/Services/SSHService.phpapp/Contracts/BaseCommand.php
{tests/**,test/**,**/*@(Test|Spec).php}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not run or edit tests unless explicitly instructed
Files:
tests/Unit/Contracts/BaseCommandTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/TestHelpers.phptests/Unit/Traits/ServerValidationTraitTest.phptests/Integration/Console/Server/ServerListCommandTest.phptests/Fixtures/TestConsoleCommand.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively with it() syntax for tests
In unit tests, instantiate classes manually with explicit mocks; use the container only for integration tests
Keep individual test files under ~1.8x the size of the source they cover
Test core business logic; avoid testing the framework itself
Prefer dataset-driven testing with ->with([...]) for multiple scenarios
Eliminate overlapping tests; avoid two tests covering the same functionality
Consolidate assertions with expect(...)->and(...) when appropriate
Mock external dependencies only; do not mock internal behavior
Avoid performance tests unless performance is the primary concern
Do not sacrifice readability to hit size/ratio targets
Follow the AAA pattern (Arrange, Act, Assert) in tests; use ACT & ASSERT for exception tests when act triggers assertion
Organize tests with describe() blocks, beforeEach() setup, and extract helpers/traits for DRY
Forbid meaningless assertions (e.g., toBeTrue, not->toBeNull, type-only checks, sleep(...)); use time mocking instead of sleep
Prefer meaningful assertions on behavior and results, and proper mocks (e.g., expect domain outputs; mock->shouldReceive(...))
Unit tests: mock all external dependencies; test single units in isolation; execute in milliseconds
Integration tests: perform real file operations/external processes; cover CLI commands and full workflows
Ignore PHPStan issues in tests; avoid excessive PHPDoc added only to appease types
Files:
tests/Unit/Contracts/BaseCommandTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/TestHelpers.phptests/Unit/Traits/ServerValidationTraitTest.phptests/Integration/Console/Server/ServerListCommandTest.phptests/Fixtures/TestConsoleCommand.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.php
{test,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
In tests, you may instantiate the container directly and build services via
$container->build()
Files:
tests/Unit/Contracts/BaseCommandTest.phptests/Unit/Traits/ServerHelpersTraitTest.phptests/TestHelpers.phptests/Unit/Traits/ServerValidationTraitTest.phptests/Integration/Console/Server/ServerListCommandTest.phptests/Fixtures/TestConsoleCommand.phptests/Integration/Console/Server/ServerDeleteCommandTest.phptests/Integration/Console/Server/ServerAddCommandTest.php
**/*Command.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*Command.php: Commands handle user interaction (input/output) and orchestrate Services
Commands must not contain business logic—delegate to Services
Commands must not duplicate orchestration logic—extract to shared Services
Commands are responsible for console styling, error formatting, and user prompts (use SymfonyStyle)
Commands should not invoke other commands (no proxy commands)
Use SymfonyStyle consistently for all user-facing console output
Files:
app/Console/Server/ServerAddCommand.phpapp/Console/Server/ServerListCommand.phpapp/Console/Server/ServerDeleteCommand.phptests/Fixtures/TestConsoleCommand.phpapp/Contracts/BaseCommand.php
**/*{Command,Service}.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*{Command,Service}.php: Declare all dependencies in constructor signatures (no hidden/global dependencies)
No circular dependencies between classes
Files:
app/Console/Server/ServerAddCommand.phpapp/Console/Server/ServerListCommand.phpapp/Console/Server/ServerDeleteCommand.phptests/Fixtures/TestConsoleCommand.phpapp/Services/SSHService.phpapp/Contracts/BaseCommand.php
**/*Service.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*Service.php: Services provide atomic, reusable functionality with no console I/O
Services accept and return plain PHP data types (no console I/O objects)
Services must be dependency-injected via constructor
Services implement core business logic, external API calls, and file operations (no user I/O)
Extract complex orchestration shared by multiple Commands into dedicated Services
Stateless services perform pure operations; stateful services manage config/caches
Stateful services should use lazy loading when initialization is expensive or path-dependent
State must be initialized explicitly via public methods (e.g., load(), initialize()) before use
Services should document stateful nature and initialization requirements
Only Commands perform console I/O; Services must not perform console input/output
Services return exceptions or structured data; commands handle display
Validation errors and business exceptions should bubble up to Commands for display
Files:
app/Services/SSHService.php
app/Contracts/BaseCommand.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Keep BaseCommand limited to shared initialization, configuration, and orchestration logic; do not implement individual I/O operations here
Files:
app/Contracts/BaseCommand.php
🧠 Learnings (3)
📚 Learning: 2025-10-04T12:08:00.471Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-10-04T12:08:00.471Z
Learning: Run phpstan static analysis on changed PHP files excluding tests; never run static analysis against tests
Applied to files:
.cursor/rules/01-architecture.mdc.cursor/rules/02-tests.mdc
📚 Learning: 2025-10-02T19:48:48.339Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-02T19:48:48.339Z
Learning: Applies to tests/**/*.php : Ignore PHPStan issues in tests; avoid excessive PHPDoc added only to appease types
Applied to files:
.cursor/rules/01-architecture.mdc.cursor/rules/02-tests.mdc
📚 Learning: 2025-10-02T19:48:48.339Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-02T19:48:48.339Z
Learning: Applies to tests/**/*.php : Avoid performance tests unless performance is the primary concern
Applied to files:
.cursor/rules/02-tests.mdc
🧬 Code graph analysis (13)
tests/Unit/Contracts/BaseCommandTest.php (4)
app/Services/SSHService.php (1)
SSHService(40-327)tests/Fixtures/TestConsoleCommand.php (1)
__construct(31-39)tests/TestHelpers.php (3)
__construct(52-64)__construct(279-282)mockSSHService(261-267)app/Contracts/BaseCommand.php (1)
__construct(35-43)
tests/Unit/Traits/ServerHelpersTraitTest.php (4)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)tests/TestHelpers.php (1)
mockTestConsoleCommand(345-358)tests/Fixtures/TestConsoleCommand.php (2)
setTestMethod(44-48)execute(59-90)tests/Unit/Contracts/BaseCommandTest.php (1)
execute(43-48)
tests/TestHelpers.php (4)
app/Services/SSHService.php (6)
SSHService(40-327)assertCanConnect(57-61)executeCommand(70-87)executeScript(96-121)uploadFile(128-148)downloadFile(155-171)tests/Fixtures/TestConsoleCommand.php (2)
__construct(31-39)TestConsoleCommand(24-234)tests/Unit/Contracts/BaseCommandTest.php (1)
__construct(26-35)app/Contracts/BaseCommand.php (1)
__construct(35-43)
app/Console/Server/ServerAddCommand.php (7)
app/Contracts/BaseCommand.php (3)
BaseCommand(26-133)configure(52-69)execute(111-132)app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Traits/ConsoleOutputTrait.php (3)
hr(85-91)writeln(23-29)showCommandHint(102-138)app/Traits/ConsoleInputTrait.php (4)
getOptionOrPrompt(58-76)promptText(106-124)promptConfirm(166-182)promptSpin(345-353)app/Traits/ServerValidationTrait.php (2)
validateHost(17-28)validatePort(35-43)app/Traits/ServerHelpersTrait.php (1)
displayServerInfo(23-33)app/Services/SSHService.php (1)
assertCanConnect(57-61)
tests/Unit/Traits/ServerValidationTraitTest.php (1)
app/Traits/ServerValidationTrait.php (2)
validateHost(17-28)validatePort(35-43)
tests/Integration/Console/Server/ServerListCommandTest.php (4)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)tests/TestHelpers.php (3)
mockEnvService(144-152)mockInventoryService(160-177)mockSSHService(261-267)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)loadInventory(31-43)
app/Console/Server/ServerListCommand.php (4)
app/Contracts/BaseCommand.php (2)
BaseCommand(26-133)execute(111-132)app/Traits/ConsoleOutputTrait.php (4)
hr(85-91)warning(54-57)writeln(23-29)h1(74-80)app/Repositories/ServerRepository.php (1)
all(83-93)app/Traits/ServerHelpersTrait.php (1)
displayServerInfo(23-33)
app/Console/Server/ServerDeleteCommand.php (6)
app/Contracts/BaseCommand.php (3)
BaseCommand(26-133)configure(52-69)execute(111-132)app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Traits/ConsoleOutputTrait.php (7)
hr(85-91)warning(54-57)writeln(23-29)h1(74-80)error(62-65)success(46-49)showCommandHint(102-138)app/Repositories/ServerRepository.php (1)
all(83-93)app/Traits/ConsoleInputTrait.php (3)
getOptionOrPrompt(58-76)promptSelect(210-228)promptConfirm(166-182)app/Traits/ServerHelpersTrait.php (1)
displayServerInfo(23-33)
tests/Fixtures/TestConsoleCommand.php (2)
app/Contracts/BaseCommand.php (2)
BaseCommand(26-133)__construct(35-43)app/Traits/ServerHelpersTrait.php (1)
displayServerInfo(23-33)
app/Traits/ServerHelpersTrait.php (2)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Traits/ConsoleOutputTrait.php (1)
writeln(23-29)
tests/Integration/Console/Server/ServerDeleteCommandTest.php (5)
app/Container.php (1)
Container(23-227)app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)tests/TestHelpers.php (3)
mockEnvService(144-152)mockInventoryService(160-177)mockSSHService(261-267)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)loadInventory(31-43)
tests/Integration/Console/Server/ServerAddCommandTest.php (6)
app/Container.php (1)
Container(23-227)app/Services/SSHService.php (1)
SSHService(40-327)tests/TestHelpers.php (4)
mockEnvService(144-152)mockInventoryService(160-177)mockSSHService(261-267)mockSSHServiceWithBehavior(276-338)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)app/Repositories/ServerRepository.php (3)
ServerRepository(15-168)loadInventory(31-43)findByName(65-76)tests/Fixtures/TestConsoleCommand.php (1)
execute(59-90)
app/Contracts/BaseCommand.php (1)
app/Services/SSHService.php (1)
SSHService(40-327)
🪛 PHPMD (2.15.0)
tests/TestHelpers.php
300-300: Avoid unused parameters such as '$command'. (undefined)
(UnusedFormalParameter)
310-310: Avoid unused parameters such as '$scriptPath'. (undefined)
(UnusedFormalParameter)
328-328: Avoid unused parameters such as '$host'. (undefined)
(UnusedFormalParameter)
329-329: Avoid unused parameters such as '$port'. (undefined)
(UnusedFormalParameter)
330-330: Avoid unused parameters such as '$username'. (undefined)
(UnusedFormalParameter)
331-331: Avoid unused parameters such as '$remotePath'. (undefined)
(UnusedFormalParameter)
332-332: Avoid unused parameters such as '$localPath'. (undefined)
(UnusedFormalParameter)
333-333: Avoid unused parameters such as '$privateKeyPath'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (12)
.cursor/rules/02-tests.mdc (1)
126-127: LGTM!The clarification that PHPStan applies to production code, not tests, is helpful and aligns with the project's testing philosophy.
app/SymfonyApp.php (1)
8-10: LGTM!The new server commands are properly imported and registered using the existing DI container pattern. The implementation is consistent with the established architecture.
Also applies to: 120-122
tests/Unit/Traits/ServerHelpersTraitTest.php (1)
1-84: LGTM!The test suite provides good coverage of the ServerHelpersTrait functionality with clear scenarios for all fields, default SSH key display, and default values. The tests follow the AAA pattern and use appropriate assertions.
tests/Integration/Console/Server/ServerDeleteCommandTest.php (1)
1-229: LGTM!The integration test suite provides comprehensive coverage of ServerDeleteCommand functionality including success scenarios, error handling, and display verification. The tests are well-structured and follow the AAA pattern consistently.
app/Console/Server/ServerListCommand.php (1)
1-56: LGTM!The ServerListCommand implementation is clean and follows established patterns. It provides good user experience with helpful guidance when the inventory is empty and uses the shared ServerHelpersTrait for consistent display formatting.
app/Contracts/BaseCommand.php (1)
11-11: Verified: all BaseCommand subclasses updated with SSHService TestableBaseCommand and TestConsoleCommand now accept SSHService in their constructors.tests/Integration/Console/Server/ServerAddCommandTest.php (3)
17-31: LGTM!The helper function correctly instantiates the command with mocked dependencies. The pattern follows the coding guidelines that permit direct container instantiation in tests.
263-298: Excellent integration test for persistence.This test properly verifies that server data persists to the inventory by checking all fields of the retrieved server. The comprehensive assertions ensure data integrity across the full stack.
144-178: Good use of dataset-driven testing.The validation tests properly leverage Pest's
->with()syntax to cover multiple invalid inputs without duplication. This approach aligns with the coding guidelines.app/Console/Server/ServerDeleteCommand.php (3)
1-35: LGTM!The command class follows best practices:
- Uses PHP 8 attributes for command metadata per Symfony 7.3 guidelines
- Extends BaseCommand with proper dependency injection via constructor promotion
- Uses ServerHelpersTrait for consistent display formatting
- Options are properly configured
50-76: LGTM!The server selection logic is well-implemented:
- Handles empty inventory gracefully with helpful guidance
- Properly extracts server names from DTOs for the selection prompt
- Uses
getOptionOrPromptpattern consistently with other commandsReturning
Command::SUCCESSwhen no servers exist (line 59) is appropriate for idempotent CLI operations.
99-132: LGTM!The confirmation and deletion flow is well-implemented:
- Respects user cancellation by returning
Command::SUCCESS(appropriate for user choice)- Uses repository for deletion (proper separation of concerns)
- Provides helpful command hint for non-interactive usage
| $privateKeyPathRaw = $this->getOptionOrPrompt( | ||
| 'private-key-path', | ||
| fn (): string => $this->promptText( | ||
| label: 'SSH private key path (leave empty for default ~/.ssh/id_ed25519 or ~/.ssh/id_rsa):', | ||
| default: '', | ||
| required: false | ||
| ) | ||
| ); | ||
|
|
||
| /** @var ?string $privateKeyPath */ | ||
| $privateKeyPath = $privateKeyPathRaw !== '' ? $privateKeyPathRaw : null; | ||
|
|
||
| // | ||
| // Create DTO and display server info | ||
|
|
||
| $server = new ServerDTO( | ||
| name: $name, | ||
| host: $host, | ||
| port: $port, | ||
| username: $username, | ||
| privateKeyPath: $privateKeyPath | ||
| ); | ||
|
|
||
| $this->hr(); | ||
|
|
||
| $this->displayServerInfo($server); | ||
|
|
||
| // | ||
| // Verify connectivity | ||
|
|
||
| /** @var bool $skipCheck */ | ||
| $skipCheck = $this->getOptionOrPrompt( | ||
| 'skip', | ||
| fn (): bool => !$this->promptConfirm( | ||
| label: 'Test SSH connection before saving?', | ||
| default: true | ||
| ) | ||
| ); | ||
|
|
||
| if ($skipCheck) { | ||
| $this->warning('Skipping SSH connection check'); | ||
| $this->writeln(''); | ||
| } else { | ||
| if (!$this->testConnection($server)) { | ||
| return Command::FAILURE; | ||
| } | ||
| } |
There was a problem hiding this comment.
Allow non-interactive runs to skip the key prompt
Calling getOptionOrPrompt('private-key-path', …) always triggers a prompt when the option is omitted. In non-interactive workflows (CI, automation) the command hangs/fails even with --yes/--skip, so users cannot rely on the default key lookup. Please fall back to the prompt only when the input is interactive; otherwise treat the missing option as null.
- /** @var string $privateKeyPathRaw */
- $privateKeyPathRaw = $this->getOptionOrPrompt(
- 'private-key-path',
- fn (): string => $this->promptText(
- label: 'SSH private key path (leave empty for default ~/.ssh/id_ed25519 or ~/.ssh/id_rsa):',
- default: '',
- required: false
- )
- );
-
- /** @var ?string $privateKeyPath */
- $privateKeyPath = $privateKeyPathRaw !== '' ? $privateKeyPathRaw : null;
+ /** @var string|null $privateKeyPathOption */
+ $privateKeyPathOption = $this->input->getOption('private-key-path');
+
+ if (is_string($privateKeyPathOption) && $privateKeyPathOption !== '') {
+ $privateKeyPath = $privateKeyPathOption;
+ } elseif ($this->input->isInteractive()) {
+ $privateKeyPathRaw = $this->promptText(
+ label: 'SSH private key path (leave empty for default ~/.ssh/id_ed25519 or ~/.ssh/id_rsa):',
+ default: '',
+ required: false
+ );
+ $privateKeyPath = $privateKeyPathRaw !== '' ? $privateKeyPathRaw : null;
+ } else {
+ $privateKeyPath = null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $privateKeyPathRaw = $this->getOptionOrPrompt( | |
| 'private-key-path', | |
| fn (): string => $this->promptText( | |
| label: 'SSH private key path (leave empty for default ~/.ssh/id_ed25519 or ~/.ssh/id_rsa):', | |
| default: '', | |
| required: false | |
| ) | |
| ); | |
| /** @var ?string $privateKeyPath */ | |
| $privateKeyPath = $privateKeyPathRaw !== '' ? $privateKeyPathRaw : null; | |
| // | |
| // Create DTO and display server info | |
| $server = new ServerDTO( | |
| name: $name, | |
| host: $host, | |
| port: $port, | |
| username: $username, | |
| privateKeyPath: $privateKeyPath | |
| ); | |
| $this->hr(); | |
| $this->displayServerInfo($server); | |
| // | |
| // Verify connectivity | |
| /** @var bool $skipCheck */ | |
| $skipCheck = $this->getOptionOrPrompt( | |
| 'skip', | |
| fn (): bool => !$this->promptConfirm( | |
| label: 'Test SSH connection before saving?', | |
| default: true | |
| ) | |
| ); | |
| if ($skipCheck) { | |
| $this->warning('Skipping SSH connection check'); | |
| $this->writeln(''); | |
| } else { | |
| if (!$this->testConnection($server)) { | |
| return Command::FAILURE; | |
| } | |
| } | |
| /** @var string|null $privateKeyPathOption */ | |
| $privateKeyPathOption = $this->input->getOption('private-key-path'); | |
| if (is_string($privateKeyPathOption) && $privateKeyPathOption !== '') { | |
| $privateKeyPath = $privateKeyPathOption; | |
| } elseif ($this->input->isInteractive()) { | |
| $privateKeyPathRaw = $this->promptText( | |
| label: 'SSH private key path (leave empty for default ~/.ssh/id_ed25519 or ~/.ssh/id_rsa):', | |
| default: '', | |
| required: false | |
| ); | |
| $privateKeyPath = $privateKeyPathRaw !== '' ? $privateKeyPathRaw : null; | |
| } else { | |
| $privateKeyPath = null; | |
| } | |
| // | |
| // Create DTO and display server info | |
| $server = new ServerDTO( | |
| name: $name, | |
| host: $host, | |
| port: $port, | |
| username: $username, | |
| privateKeyPath: $privateKeyPath | |
| ); |
🤖 Prompt for AI Agents
In app/Console/Server/ServerAddCommand.php around lines 107-153, the private-key
prompt is always invoked when the option is omitted, blocking non-interactive
runs; change the logic to first read the option directly and only call the
interactive prompt when the input is actually interactive: retrieve the raw
option value (null/empty if not provided), and if it's null and
$this->input->isInteractive() is true then call getOptionOrPrompt to prompt the
user; otherwise treat missing/empty option as null and proceed (preserving the
existing conversion from empty string to null and the rest of the flow).
| $server = null; | ||
| foreach ($allServers as $s) { | ||
| if ($s->name === $name) { | ||
| $server = $s; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ($server === null) { | ||
| $this->error("Server '{$name}' not found in inventory"); | ||
| return Command::FAILURE; | ||
| } | ||
|
|
||
| $this->displayServerInfo($server); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use repository method instead of manual lookup.
Lines 82-87 manually loop through servers to find a match by name, duplicating logic that already exists in ServerRepository::findByName().
Apply this diff to use the repository method:
- //
- // Find server and display info
-
- $server = null;
- foreach ($allServers as $s) {
- if ($s->name === $name) {
- $server = $s;
- break;
- }
- }
+ //
+ // Find server and display info
+
+ $server = $this->servers->findByName($name);
if ($server === null) {
$this->error("Server '{$name}' not found in inventory");
return Command::FAILURE;
}This reduces duplication and improves maintainability by relying on the repository's existing method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $server = null; | |
| foreach ($allServers as $s) { | |
| if ($s->name === $name) { | |
| $server = $s; | |
| break; | |
| } | |
| } | |
| if ($server === null) { | |
| $this->error("Server '{$name}' not found in inventory"); | |
| return Command::FAILURE; | |
| } | |
| $this->displayServerInfo($server); | |
| // | |
| // Find server and display info | |
| $server = $this->servers->findByName($name); | |
| if ($server === null) { | |
| $this->error("Server '{$name}' not found in inventory"); | |
| return Command::FAILURE; | |
| } | |
| $this->displayServerInfo($server); |
🤖 Prompt for AI Agents
In app/Console/Server/ServerDeleteCommand.php around lines 81 to 94, replace the
manual foreach lookup for a server by name with a call to the repository's
findByName() method; call $this->serverRepository->findByName($name), assign the
result to $server, then keep the existing null check and error return and the
subsequent $this->displayServerInfo($server) so behavior remains identical but
avoids duplicating lookup logic.
| try { | ||
| $ssh = new SSH2($host, $port); | ||
| } catch (\Throwable $e) { | ||
| throw new \RuntimeException("Error initiating SSH connection to {$host}:{$port}: " . $e->getMessage()); | ||
| } | ||
|
|
||
| try { | ||
| $loggedIn = $ssh->login($username, $key); | ||
| } catch (\Throwable $e) { | ||
| throw new \RuntimeException("Error authenticating SSH for {$username}@{$host}: " . $e->getMessage()); | ||
| throw new \RuntimeException($e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Consider retaining connection context in error messages.
Removing the outer try-catch around the SSH2 constructor means connection errors will no longer include which host/port failed. This could make debugging harder when working with multiple servers.
Consider retaining contextual information while avoiding double-wrapping:
try {
$ssh = new SSH2($host, $port);
$loggedIn = $ssh->login($username, $key);
} catch (\Throwable $e) {
- throw new \RuntimeException($e->getMessage());
+ throw new \RuntimeException("Failed to connect or authenticate with {$username}@{$host}:{$port}: " . $e->getMessage(), previous: $e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| $ssh = new SSH2($host, $port); | |
| } catch (\Throwable $e) { | |
| throw new \RuntimeException("Error initiating SSH connection to {$host}:{$port}: " . $e->getMessage()); | |
| } | |
| try { | |
| $loggedIn = $ssh->login($username, $key); | |
| } catch (\Throwable $e) { | |
| throw new \RuntimeException("Error authenticating SSH for {$username}@{$host}: " . $e->getMessage()); | |
| throw new \RuntimeException($e->getMessage()); | |
| } | |
| try { | |
| $ssh = new SSH2($host, $port); | |
| $loggedIn = $ssh->login($username, $key); | |
| } catch (\Throwable $e) { | |
| throw new \RuntimeException( | |
| "Failed to connect or authenticate with {$username}@{$host}:{$port}: " . $e->getMessage(), | |
| previous: $e | |
| ); | |
| } |
🤖 Prompt for AI Agents
In app/Services/SSHService.php around lines 186 to 191, the current try/catch
strips connection context when SSH2 construction or login fails; update the
catch to include the host and port in the thrown message and preserve the
original exception as the previous exception (e.g. throw new
\RuntimeException("Failed to connect to {$host}:{$port}: ".$e->getMessage(), 0,
$e)) so debugging shows which server failed while retaining the original stack.
| throw new \InvalidArgumentException( | ||
| "Invalid host '{$host}'. Must be a valid IP address or domain name.\n". | ||
| 'Examples: 192.168.1.100, example.com, server.example.com' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Validate port is in valid range. | ||
| * | ||
| * @throws \InvalidArgumentException When port is out of range | ||
| */ | ||
| protected function validatePort(int $port): void | ||
| { | ||
| if ($port < 1 || $port > 65535) { | ||
| throw new \InvalidArgumentException( | ||
| "Invalid port {$port}. Port must be between 1 and 65535.\n". | ||
| 'Common SSH ports: 22 (default), 2222, 22000' | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import InvalidArgumentException instead of using the FQN
The coding guidelines require use imports for classes. Please add use InvalidArgumentException; (and adjust the throw statements accordingly) so the trait follows the standard. As per coding guidelines.
+use InvalidArgumentException;
@@
- throw new \InvalidArgumentException(
+ throw new InvalidArgumentException(
@@
- throw new \InvalidArgumentException(
+ throw new InvalidArgumentException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new \InvalidArgumentException( | |
| "Invalid host '{$host}'. Must be a valid IP address or domain name.\n". | |
| 'Examples: 192.168.1.100, example.com, server.example.com' | |
| ); | |
| } | |
| } | |
| /** | |
| * Validate port is in valid range. | |
| * | |
| * @throws \InvalidArgumentException When port is out of range | |
| */ | |
| protected function validatePort(int $port): void | |
| { | |
| if ($port < 1 || $port > 65535) { | |
| throw new \InvalidArgumentException( | |
| "Invalid port {$port}. Port must be between 1 and 65535.\n". | |
| 'Common SSH ports: 22 (default), 2222, 22000' | |
| ); | |
| <?php | |
| declare(strict_types=1); | |
| namespace App\Traits; | |
| use InvalidArgumentException; | |
| trait ServerValidationTrait | |
| { | |
| // … | |
| protected function validateHost(string $host): void | |
| { | |
| if (! $this->isValidHost($host)) { | |
| throw new InvalidArgumentException( | |
| "Invalid host '{$host}'. Must be a valid IP address or domain name.\n" . | |
| 'Examples: 192.168.1.100, example.com, server.example.com' | |
| ); | |
| } | |
| } | |
| /** | |
| * Validate port is in valid range. | |
| * | |
| * @throws \InvalidArgumentException When port is out of range | |
| */ | |
| protected function validatePort(int $port): void | |
| { | |
| if ($port < 1 || $port > 65535) { | |
| throw new InvalidArgumentException( | |
| "Invalid port {$port}. Port must be between 1 and 65535.\n" . | |
| 'Common SSH ports: 22 (default), 2222, 22000' | |
| ); | |
| } | |
| } | |
| // … | |
| } |
🤖 Prompt for AI Agents
In app/Traits/ServerValidationTrait.php around lines 23 to 41, the trait uses
the fully-qualified exception class (\InvalidArgumentException) instead of
importing it; add a top-level import "use InvalidArgumentException;" and update
the throw statements in this block to use the short class name (throw new
InvalidArgumentException(...)) for both host and port validation so the file
follows the project's import conventions.
1 participant
Summary by CodeRabbit