feat: ssh sftp service - #21
Conversation
Implements SSHService with comprehensive remote server management: - Connection testing with authentication validation - Command execution with exit code capture - Script execution via bash heredoc - File upload/download via SFTP - Private key resolution with tilde expansion and fallback locations - Stateless operations (connections per operation) Includes complete unit test coverage for key resolution, validation, and error handling using phpseclib3 for SSH/SFTP operations.
Updated docblock examples to demonstrate default value usage when paths don't exist, improving clarity on how the get() method handles missing configuration keys.
Added development guidelines for performance optimization: - Eliminate single-use private methods (inline directly) - Cache computed values in constructors - Prefer direct property access over method calls These patterns complement the existing minimalist code philosophy.
WalkthroughAdds a new SSHService providing stateless SSH/SFTP operations with robust error handling and private key resolution, a comprehensive unit test suite for that service, docblock clarifications in InventoryService about default-return behavior, and a new “Performance & Efficiency Patterns” subsection in development rules documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SSHService
participant Env as EnvService
participant FS as Filesystem
participant SSH2 as phpseclib::SSH2
participant SFTP as phpseclib::SFTP
rect rgba(230,245,255,0.5)
note right of SSHService: executeCommand / assertCanConnect
Client->>SSHService: executeCommand(host,port,user,command)
SSHService->>Env: resolve HOME (for key paths)
SSHService->>FS: check/read private key
SSHService->>SSH2: connect(host,port) & authenticate(key)
alt authenticated
SSHService->>SSH2: run command
SSH2-->>SSHService: stdout/stderr/exit
SSHService-->>Client: result array
else auth/error
SSH2-->>SSHService: error
SSHService-->>Client: RuntimeException (host/context)
end
SSHService->>SSH2: disconnect
end
rect rgba(230,255,230,0.5)
note right of SSHService: uploadFile / downloadFile
Client->>SSHService: uploadFile(host,port,user,local,remote)
SSHService->>FS: assert exists(local)
SSHService->>Env: resolve HOME
SSHService->>FS: read private key
SSHService->>SFTP: connect & authenticate
alt transfer success
SSHService->>SFTP: put/get file
SFTP-->>SSHService: ok
SSHService-->>Client: void / success
else transfer error
SFTP-->>SSHService: error
SSHService-->>Client: RuntimeException (includes paths)
end
SSHService->>SFTP: disconnect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cursor/rules/00-main.mdc(1 hunks)app/Services/InventoryService.php(1 hunks)app/Services/SSHService.php(1 hunks)tests/Unit/Services/SSHServiceTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Organize code into comment-separated sections grouping related functions
Prefer alphabetical ordering when it doesn’t conflict with logical grouping
Maintain obsessive implementation consistency and logical coherence across similar areas (naming, parameters, flow, comments)
Refactor relentlessly for clarity and necessity
**/*.php: Follow PSR-12 coding standard in all PHP files
Declare strict types in all PHP files (declare(strict_types=1);)
Use PHP 8.x features where appropriate (unions, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must declare explicit return types with proper generics (e.g., Collection<int, User>)
Prefer Dependency Injection over manual class resolution/instantiation
Use Symfony component classes (e.g., Filesystem, Process) instead of native PHP functions for easier mocking
All object creation must use $container->build(ClassName::class) except for value objects, DTOs, and pure data structures
Access the container via constructor injection in production code; tests may instantiate Container directly
Services must receive dependencies via constructor injection
Use SymfonyStyle consistently for all user-facing console output
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments to separate sections and explain complex logic; avoid obvious or stale comments
Use the exact section header comment format with a single newline between headers/subheaders/paragraphs
Files:
app/Services/InventoryService.phpapp/Services/SSHService.phptests/Unit/Services/SSHServiceTest.php
app/Services/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Services return plain data and must not perform any console I/O
Files:
app/Services/InventoryService.phpapp/Services/SSHService.php
tests/**
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not run or edit tests unless explicitly instructed
Files:
tests/Unit/Services/SSHServiceTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively with it() syntax for all tests.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic; do not test the framework itself.
Use dataset-driven testing with ->with([]) for multiple scenarios.
Eliminate overlap: avoid multiple tests covering the same functionality.
Consolidate assertions using chained expectations (e.g., expect(...)->toBe(...)->and(...)).
Mock only external dependencies; do not mock internal implementation details.
Do not add performance tests unless performance is the primary concern.
Do not sacrifice readability to meet size/ratio targets.
Do not consolidate tests when they target different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate tests with different setup requirements.
Do not consolidate tests covering distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in all tests; include cleanup when needed.
For exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion.
Organize tests with describe() blocks, beforeEach() setup, and extract helpers/traits for DRY tests.
Do not assert only on type using toBeInstanceOf(Class::class).
Do not use generic assertions like toBeArray() that don’t validate behavior.
Avoid meaningless assertions such as not->toBeNull() without behavioral relevance.
Do not write literally meaningless assertions like expect(true)->toBeTrue().
Do not use sleep(...); use time mocking instead.
Write assertions against concrete outcomes, e.g., expect($config->getValue('host'))->toBe('example.com').
Assert validator outcomes directly, e.g., expect($this->validator->isValid($input))->toBe($expected).
Define mock expectations explicitly, e.g., $mock->shouldReceive('method')->with('param')->andReturn('result').
In unit tests, mock all external dependencies (filesystem, HTTP, processes).
Unit tests should target a single unit in isolation.
Unit tests should complete i...
Files:
tests/Unit/Services/SSHServiceTest.php
🧬 Code graph analysis (2)
app/Services/SSHService.php (2)
app/Services/EnvService.php (1)
EnvService(13-135)tests/TestHelpers.php (3)
exists(72-91)readFile(93-105)dumpFile(117-124)
tests/Unit/Services/SSHServiceTest.php (2)
app/Services/SSHService.php (4)
SSHService(41-333)assertCanConnect(58-62)executeScript(97-122)uploadFile(129-149)tests/TestHelpers.php (4)
setEnv(15-25)mockFilesystem(32-126)mockEnvService(133-137)dumpFile(117-124)
🪛 GitHub Actions: Rector
tests/Unit/Services/SSHServiceTest.php
[error] 50-50: Rector dry-run would modify 1 file using RemoveReflectionSetAccessibleCallsRector; Rector reported: '1 file would have been changed (dry-run)'. Command: vendor/bin/rector --dry-run. Process exited with code 2.
1 participant
Summary by CodeRabbit