wip - #8
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds configuration/docs for Cursor and testing rules, a GitHub Actions workflow for automated PR test-rule reviews, introduces a reflection-based DI Container, rewrites EnvService to use injected Filesystem, implements YAML-backed InventoryService, SSH connectivity checks, server CRUD item/DTO, a new server:create console command wired via Deployer, and unit tests for Container and EnvService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Symfony Console
participant Deployer as Deployer
participant Container as Container
participant Cmd as ServerCreateCommand
participant SSH as SSHService
participant Inv as InventoryService
participant Item as ServerItem
User->>CLI: run "server:create <name> --host ... [--port --user --key]"
CLI->>Deployer: boot
Deployer->>Container: build(ServerCreateCommand)
Container-->>Deployer: instance
Deployer->>CLI: add command
User->>Cmd: execute()
Cmd->>SSH: assertCanConnect(host, port, user, key?)
SSH-->>Cmd: success or throws
alt success
Cmd->>Item: create(name, ServerDTO)
Item->>Inv: set('servers', name, server array)
Inv-->>Item: ok
Cmd-->>User: success exit 0
else error
Cmd-->>User: error message
Cmd-->>CLI: exit 1
end
sequenceDiagram
autonumber
actor GH as GitHub PR
participant GHA as Actions Runner
participant Cursor as cursor-agent
participant gh as gh CLI
GH->>GHA: pull_request event
GHA->>GHA: checkout repo
GHA->>Cursor: run review with prompt
Cursor->>gh: fetch PR diff/comments
Cursor-->>GHA: generate review text
GHA->>gh: pr review --comment
gh-->>GH: post single PR comment
sequenceDiagram
autonumber
participant App as Deployer
participant C as Container
participant R as Reflection API
App->>C: build(FQCN)
C->>R: inspect class and ctor
alt no ctor
C-->>App: new instance()
else ctor with params
loop each param
C->>C: resolveParameter(param)
alt class-typed
C->>C: build(dependency)
else builtin/union
C-->>C: use default or fail
end
end
C-->>App: new instance(...deps)
end
note right of C: Detects and errors on circular chains
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Pull Request Overview
This is a work-in-progress PR adding comprehensive functionality to the PHP deployer including SSH connectivity, inventory management, and dependency injection with corresponding unit tests.
- Adds SSH service for connection validation, inventory service for YAML-based data management, and dependency injection container
- Implements server management functionality with validation and persistence
- Creates comprehensive unit tests for all services with proper mocking and AAA patterns
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/EnvServiceTest.php | Comprehensive test coverage for environment variable resolution with dataset-driven scenarios |
| tests/Unit/ContainerTest.php | Unit tests for dependency injection container covering edge cases and error handling |
| app/Services/SSHService.php | SSH connectivity service with private key resolution and authentication |
| app/Services/InventoryService.php | YAML-based inventory management service for CRUD operations |
| app/Services/EnvService.php | Environment variable service with Symfony Filesystem integration |
| app/Items/ServerItem.php | Server item class with validation and inventory integration |
| app/Deployer.php | Main application class with dependency injection and command registration |
| app/DTOs/ServerDTO.php | Data transfer object for server configuration |
| app/Container.php | Dependency injection container with reflection-based auto-wiring |
| app/Console/Server/ServerCreateCommand.php | Command for creating server entries with SSH verification |
| .github/workflows/testing-rules.yml | GitHub Actions workflow for automated testing rule validation |
| .cursor/rules/03-tests.mdc | Updated testing guidelines emphasizing minimalism and efficiency |
| .cursor/rules/01-architecture.mdc | Updated architecture rules emphasizing Symfony patterns and DI |
| .cursor/commands/review-testing-rules.md | Command for reviewing testing rule compliance |
| .cursor/cli.json | CLI permissions configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Default to id_rsa first, then try id_ed25519 | ||
| $candidates[] = $home.'/.ssh/id_rsa'; | ||
| $candidates[] = $home.'/.ssh/id_ed25519'; |
There was a problem hiding this comment.
The comment on line 88 doesn't match the implementation - ed25519 is generally preferred over RSA for security reasons, so the order should prioritize id_ed25519 first, then id_rsa as fallback.
| // Default to id_rsa first, then try id_ed25519 | |
| $candidates[] = $home.'/.ssh/id_rsa'; | |
| $candidates[] = $home.'/.ssh/id_ed25519'; | |
| // Default to id_ed25519 first, then try id_rsa | |
| $candidates[] = $home.'/.ssh/id_ed25519'; | |
| $candidates[] = $home.'/.ssh/id_rsa'; |
| /** | ||
| * @return array<string, mixed> | ||
| */ |
| { | ||
| $dir = $this->getInventoryDir(); | ||
| if (!is_dir($dir)) { | ||
| if (!@mkdir($dir, 0775, true) && !is_dir($dir)) { |
There was a problem hiding this comment.
The directory permissions 0775 allow group write access which could be a security concern. Consider using 0755 for better security, allowing only owner write access.
| if (!@mkdir($dir, 0775, true) && !is_dir($dir)) { | |
| if (!@mkdir($dir, 0755, true) && !is_dir($dir)) { |
| - name: Testing rules review | ||
| env: | ||
| CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
The $MODEL environment variable is referenced but never defined in the workflow. Either define the MODEL environment variable or replace with a specific model name.
| GH_TOKEN: ${{ github.token }} | |
| GH_TOKEN: ${{ github.token }} | |
| MODEL: gpt-4o |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧪 Test Coverage Analysis
Missing Tests ⚠️
Critical gaps found:
SSHService- 0% coverage for SSH connection validation logicServerItem- 0% coverage for server CRUD and validationInventoryService- 0% coverage for YAML file operationsServerDTO- 0% coverage for data transfer objectDeployer- 0% coverage for main application logic
Test Quality Issues 🚨
The existing tests follow good patterns but significant functionality remains untested. Per minimalist testing rules, core business logic should be covered.
Recommendations:
- Add
SSHServiceTestfocusing on key resolution and connection validation - Add
ServerItemTestfor validation rules and CRUD operations - Add
InventoryServiceTestfor YAML persistence and collection operations - Consider
ServerDTOTestfor array conversion logic
Priority: SSH and validation logic are critical paths that should have test coverage before production deployment.
There was a problem hiding this comment.
🧪 Test Coverage Analysis
Missing Tests ⚠️
Critical gaps found:
SSHService- 0% coverage for SSH connection validation logicServerItem- 0% coverage for server CRUD and validationInventoryService- 0% coverage for YAML file operationsServerDTO- 0% coverage for data transfer objectDeployer- 0% coverage for main application logic
Test Quality Issues 🚨
The existing tests follow good patterns but significant functionality remains untested. Per minimalist testing rules, core business logic should be covered.
Recommendations:
- Add
SSHServiceTestfocusing on key resolution and connection validation - Add
ServerItemTestfor validation rules and CRUD operations - Add
InventoryServiceTestfor YAML persistence and collection operations - Consider
ServerDTOTestfor array conversion logic
Priority: SSH and validation logic are critical paths that should have test coverage before production deployment.
| * | ||
| * @throws \RuntimeException When connection or authentication fails | ||
| */ | ||
| public function assertCanConnect(string $host, int $port, string $username, ?string $privateKeyPath = null): void |
There was a problem hiding this comment.
🚨 Critical: This SSH connection logic has no test coverage. Consider testing key resolution paths, validation failures, and connection errors to ensure reliability.
![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 (24)
.cursor/commands/review-testing-rules.md (1)
1-2: Make the directive explicit (scope, rules source, and output format).Spell out what to scan, what rules to enforce, and how to report so the agent’s behavior is deterministic.
-Analyze the changes in this Git working tree and report back on tests that fall short of our testing rules. +Analyze the current PR diff and report any violations of our testing rules. + +Scope: +- Files: tests/**/*.php only +- Rules source: .cursor/rules/03-tests.mdc + +Report format: +- For each issue: file:line → short rule name → 1‑line fix +- End with a brief summary of top 3 actions.cursor/cli.json (1)
1-6: Harden CLI permissions; explicitly deny merges/releases, and document intent.Prevents unintended repo‑level changes during reviews; keep surface minimal and self‑documented.
{ "permissions": { - "allow": [], - "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"] + "allow": [ + "Shell(gh pr view)", + "Shell(gh pr diff)", + "Shell(gh pr review --comment)" + ], + "deny": [ + "Shell(git push)", + "Shell(git tag*)", + "Shell(gh pr create)", + "Shell(gh pr merge*)", + "Shell(gh release create*)", + "Shell(gh release delete*)", + "Write(**)" + ] } }Please confirm Cursor respects wildcards here; if not, replace
*with explicit subcommands supported by your version..cursor/rules/01-architecture.mdc (2)
13-17: Clarify IO rule: prefer Symfony Filesystem/Finder/Process over native functions.This helps avoid confusion seen in Services using
file_*/mkdir.### Symfony Patterns (MANDATORY) -- **Dependency Injection:** instead of manually resolving and instantiating classes -- **Symfony Classes:** instead of native PHP functions for easier mocking during testing (eg. `Filesystem::`, `Process::`, etc.) +- **Dependency Injection:** instead of manually resolving and instantiating classes +- **Symfony Classes for IO/OS:** prefer `Symfony\Component\Filesystem\Filesystem`, `Finder`, and `Process` + over native `file_*`, `mkdir`, `exec`, etc., to improve testability and consistency.
20-23: Call out scalar/env dependencies (paths, time) must be injected.Avoids hidden globals like
getcwd()/time()in Services.- Dependencies are automatically injected by the `app/Container` class - ALL dependencies MUST be injected through constructors - NO manual instantiation - **Exceptions:** Only value objects, DTOs, and pure data structures can be manually instantiated +- Inject scalars (e.g., base path, timezone, now-provider) instead of calling globals (`getcwd()`, `time()`).cursor/rules/03-tests.mdc (2)
9-15: Enforce the 60%+ coverage target in CI.Document the command so the workflow can gate on it.
**Framework:** Pest exclusively with `it()` syntax, 60%+ coverage ### Running Tests -- `composer pest` - run entire test suite in parallel, with coverage +- `composer pest` - run entire test suite in parallel, with coverage - `vendor/bin/pest $TEST_FILE` - run specific test file +> CI gate: `vendor/bin/pest --coverage-clover=build/coverage.xml --min=60`
37-53: Tighten AAA example with explicit cleanup guidance.Prevents fragile tests from stray temp files.
// ASSERT expect($result)->toBe('expected'); - // CLEANUP (when needed) - $this->resetTimeState(); - unlink($tempFile); + // CLEANUP (only when the test created resources) + // e.g. Clock::reset(); + // Filesystem::remove($tempFile);.github/workflows/testing-rules.yml (1)
21-25: Pin installer and verify checksum for supply‑chain safety.Curl‑to‑bash is convenient but brittle; pin or checksum the installer.
- curl https://cursor.com/install -fsS | bash + curl -fsSLo /tmp/cursor-install.sh https://cursor.com/install + # echo "<sha256sum> /tmp/cursor-install.sh" | sha256sum -c - + bash /tmp/cursor-install.shapp/Container.php (4)
7-10: Import missing types; avoid FQCNs per guidelines.Bring
RuntimeException(and union support) into scope.use ReflectionClass; +use ReflectionUnionType; +use RuntimeException; use ReflectionNamedType; use ReflectionParameter;
41-56: Replace FQCN\RuntimeExceptionwith importedRuntimeException.Aligns with “Always use use import statements”.
- throw new \RuntimeException("Circular dependency detected: {$chain}"); + throw new RuntimeException("Circular dependency detected: {$chain}"); @@ - throw new \RuntimeException("Class [{$className}] does not exist"); + throw new RuntimeException("Class [{$className}] does not exist"); @@ - throw new \RuntimeException("Class [{$className}] is not instantiable"); + throw new RuntimeException("Class [{$className}] is not instantiable"); @@ - throw new \RuntimeException( + throw new RuntimeException( "Cannot resolve parameter [{$parameter->getName()}] in class [{$parameter->getDeclaringClass()?->getName()}]" ); @@ - throw new \RuntimeException( + throw new RuntimeException( "Cannot resolve dependency [{$className}] for parameter [{$parameter->getName()}]", previous: $e );Also applies to: 132-135, 148-151
124-153: Handle union/nullable parameters and prefer defaults when available.Prevents over‑instantiation and supports
Foo|null/?Foo.private function resolveParameter(ReflectionParameter $parameter): mixed { $type = $parameter->getType(); - // Handle union types and built-in types - if (!$type instanceof ReflectionNamedType || $type->isBuiltin()) { + // Builtins or no type → use default if available + if ($type instanceof ReflectionNamedType && $type->allowsNull() && $parameter->isDefaultValueAvailable()) { + return $parameter->getDefaultValue(); + } + + // Handle non-classy or built-in types + if (!$type instanceof ReflectionNamedType || $type->isBuiltin()) { if ($parameter->isDefaultValueAvailable()) { return $parameter->getDefaultValue(); } - throw new RuntimeException( + throw new RuntimeException( "Cannot resolve parameter [{$parameter->getName()}] in class [{$parameter->getDeclaringClass()?->getName()}]" ); } $className = $type->getName(); try { /** @var class-string $className */ return $this->build($className); - } catch (\RuntimeException $e) { + } catch (RuntimeException $e) { // If dependency resolution fails and parameter has default, use it if ($parameter->isDefaultValueAvailable()) { return $parameter->getDefaultValue(); } - throw new RuntimeException( + throw new RuntimeException( "Cannot resolve dependency [{$className}] for parameter [{$parameter->getName()}]", previous: $e ); } }
36-77: Consider interface bindings/factories (future).You currently error on interfaces/abstracts; adding a simple binding map or factory hook would unblock common cases.
If you want, I can sketch a minimal bindings API compatible with existing tests.
app/Services/InventoryService.php (2)
157-165: Derive base dir from injected scalar, not global CWD.Improves testability and avoids hidden env coupling.
private function getInventoryDir(): string { - return rtrim((string) getcwd(), '/').'/.deployer'; + $root = $this->baseDir ?? (string) getcwd(); + return rtrim($root, '/').'/.deployer'; }
1-16: Add class imports and doc header updates accordingly.Bring Filesystem into scope and document constructor.
Please run Rector/Pint after applying changes and ensure PHPStan passes for app/.
tests/Unit/ContainerTest.php (1)
160-165: Avoid forbidden type‑only assertion in tests.Guidelines forbid type-only expectations. Assert behavior instead.
- expect($result)->toBeInstanceOf(SimpleService::class); + expect($result->getName())->toBe('simple');app/Deployer.php (2)
83-94: Add array type hint for command class list.Improves static analysis; no behavior change.
- $commands = [ + /** @var list<class-string<Command>> $commands */ + $commands = [ ServerCreateCommand::class, ];
143-161: Shell calls: add a short timeout guard.git invocations via shell_exec can hang in odd environments. Consider a timeout wrapper or suppress this path behind an env flag.
app/Services/EnvService.php (1)
26-47: Optional: inject Dotenv parser for full DI purity.If you want zero “new” in methods, accept a Dotenv in constructor and use it here.
app/Console/Server/ServerCreateCommand.php (2)
28-36: Align key default/help text with SSH preference.If we prefer ed25519 first, reflect that in help.
- ->addOption('key', null, InputOption::VALUE_OPTIONAL, 'Path to SSH private key (default: ~/.ssh/id_rsa)'); + ->addOption('key', null, InputOption::VALUE_OPTIONAL, 'Path to SSH private key (default: ~/.ssh/id_ed25519 or ~/.ssh/id_rsa)');
18-26: Add minimal DocBlocks for class and constructor.Keeps app/ code aligned with “DocBlock classes/methods” guideline.
app/Items/ServerItem.php (2)
45-59: Validation reads well; consider normalizing host/user whitespace.Trim inputs before validation to avoid surprising rejections.
- if ($server->host === '') { + if (trim($server->host) === '') { throw new \InvalidArgumentException('Invalid host.'); } @@ - if ($server->user === '') { + if (trim($server->user) === '') { throw new \InvalidArgumentException('Invalid user.'); }
61-66: Name validation: clarify allowed charset in message.Current message is fine; optional: include regex in docs or constants for reuse.
app/Services/SSHService.php (3)
28-31: Update guidance message to match preferred key order.If ed25519 is preferred, say so here.
- throw new \RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_rsa (or ~/.ssh/id_ed25519).'); + throw new \RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_ed25519 (or ~/.ssh/id_rsa).');
48-56: Add a network timeout to avoid hanging logins.phpseclib SSH2 supports setTimeout(); set a sane default before login.
try { - $ssh = new SSH2($host, $port); + $ssh = new SSH2($host, $port); + $ssh->setTimeout(10); // seconds } catch (\Throwable $e) {
86-91: HOME on Windows.Also consider USERPROFILE/HOMEDRIVE+HOMEPATH fallback to locate ~/.ssh on Windows.
- $home = rtrim((string) getenv('HOME'), '/'); + $home = rtrim((string) (getenv('HOME') ?: getenv('USERPROFILE') ?: (getenv('HOMEDRIVE').getenv('HOMEPATH'))), "/\\");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.cursor/cli.json(1 hunks).cursor/commands/review-testing-rules.md(1 hunks).cursor/rules/01-architecture.mdc(2 hunks).cursor/rules/03-tests.mdc(1 hunks).github/workflows/testing-rules.yml(1 hunks)app/Console/Server/ServerCreateCommand.php(1 hunks)app/Container.php(1 hunks)app/DTOs/ServerDTO.php(1 hunks)app/Deployer.php(3 hunks)app/Items/ServerItem.php(1 hunks)app/Services/EnvService.php(1 hunks)app/Services/InventoryService.php(1 hunks)app/Services/SSHService.php(1 hunks)tests/Unit/ContainerTest.php(1 hunks)tests/Unit/EnvServiceTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/*.php: Follow PSR-12, declare strict_types, and leverage PHP 8.x features (unions, match, attributes, readonly)
Always useuseimport statements instead of fully qualified class names in code
All methods must declare explicit return types and use proper generics annotations (e.g., Collection<int, User>)
All dependencies must be injected via constructors; no manual instantiation
Use a ServiceContainer/DI container for all object creation
Never usenew ClassName()inside methods—always inject dependencies
Only value objects/DTOs/pure data structures may be manually instantiated
No circular dependencies
Declare all dependencies in constructor signaturesRun PHPStan static analysis on changed PHP files, excluding tests
Files:
app/Container.phpapp/Deployer.phpapp/Services/SSHService.phpapp/Items/ServerItem.phpapp/Services/InventoryService.phpapp/Services/EnvService.phpapp/DTOs/ServerDTO.phpapp/Console/Server/ServerCreateCommand.php
{app,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-code-quality.mdc)
{app,tests}/**/*.php: Add DocBlock comments with minimalist descriptions, parameters, and return types for PHP classes and functions
Use comments to separate sections of code and to explain or summarize complex logic; avoid commenting the obvious
Format comment sections as headers/subheaders/paragraphs and separate them with a single newline
Do not leave stale comments behind when removing code
Run Rector on all changed PHP files before completing a task
Run Pint (code style fixer) on all changed PHP files before completing a task
Files:
app/Container.phpapp/Deployer.phpapp/Services/SSHService.phpapp/Items/ServerItem.phpapp/Services/InventoryService.phpapp/Services/EnvService.phpapp/DTOs/ServerDTO.phpapp/Console/Server/ServerCreateCommand.phptests/Unit/EnvServiceTest.phptests/Unit/ContainerTest.php
app/**/@(Service|Services)/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/@(Service|Services)/**/*.php: Services provide atomic, reusable functionality and must not perform console I/O
Services accept and return plain PHP data types
Services must be stateless and use dependency injection
Services handle core business logic, external API calls, and file operations
Extract complex orchestration shared by multiple Commands into dedicated Services
Services return exceptions or structured data for Commands to handle
Validation errors and business exceptions should bubble up to Commands for display
Services receive other Services/utilities via constructor injection
Services may depend on other Services or utilities
Files:
app/Services/SSHService.phpapp/Services/InventoryService.phpapp/Services/EnvService.php
{tests/**,**/*Test.php,**/*.test.php,phpunit.xml,phpunit.xml.dist}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not write or run tests unless specifically instructed
Files:
tests/Unit/EnvServiceTest.phptests/Unit/ContainerTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-code-quality.mdc)
Never run static analysis against tests
tests/**/*.php: Use Pest exclusively with it() syntax for all tests
Follow the AAA pattern with explicit headers in every test: // ARRANGE, // ACT, // ASSERT, and // CLEANUP when needed
For exception-focused tests, use a combined // ACT & ASSERT section header
Focus tests on core business logic only; do not test the framework itself
Use minimal test data and simplest possible setup
Prefer built-in expect() assertions over custom assertions
Only include essential edge cases that matter to business behavior
Do not write performance tests unless performance is a primary concern
Ignore PHPStan issues in tests
Avoid excessive phpdoc in tests; add only when necessary for clarity
Organize tests with logical describe() groupings
Extract repeated mocking into reusable helper methods
Create test traits for shared behavior across tests
Use beforeEach() for common setup within groups
Ensure proper cleanup in tests to maintain isolation (temp files, time state, etc.)
Build helper functions for creating test configurations and mock data
Forbidden assertions/patterns: type-only or meaningless expectations (toBeInstanceOf, toBeArray, not->toBeNull, toBeTrue, expect(true)->toBeTrue), property type tests, and sleep(...)
Assert specific values and behaviors rather than generic types
Use datasets for input variations in Pest tests
Mock only external dependencies; do not mock internal behavior under test
Unit tests must be isolated: test single units, mock/fake all external dependencies, run without external systems, and complete quickly
Unit tests must not use real filesystem, network, shell commands, external services, or actual HTTP/process executions
Use integration tests (not unit tests) for filesystem operations and external processes; unit tests for pure business logic with mocks
Layer testing strategy: CLI commands as integration tests (mock external services/processes), business services as unit tests (mock all externals), utilities/he...
Files:
tests/Unit/EnvServiceTest.phptests/Unit/ContainerTest.php
🧬 Code graph analysis (7)
app/Container.php (1)
tests/Unit/ContainerTest.php (3)
getType(21-24)getName(13-16)getName(58-61)
app/Deployer.php (1)
app/Container.php (2)
Container(21-154)build(36-77)
app/Items/ServerItem.php (3)
app/Services/InventoryService.php (3)
InventoryService(14-166)set(72-81)has(47-54)app/DTOs/ServerDTO.php (3)
ServerDTO(10-34)__construct(12-18)toArray(25-33)app/Console/Server/ServerCreateCommand.php (1)
__construct(21-26)
app/Services/EnvService.php (1)
tests/Unit/EnvServiceTest.php (3)
__construct(15-17)exists(18-21)readFile(22-28)
app/Console/Server/ServerCreateCommand.php (3)
app/Items/ServerItem.php (3)
ServerItem(13-67)__construct(15-17)create(22-32)app/Services/SSHService.php (2)
SSHService(17-118)assertCanConnect(24-70)app/DTOs/ServerDTO.php (2)
ServerDTO(10-34)__construct(12-18)
tests/Unit/EnvServiceTest.php (1)
app/Services/EnvService.php (3)
EnvService(14-77)__construct(19-24)get(54-75)
tests/Unit/ContainerTest.php (1)
app/Container.php (2)
Container(21-154)build(36-77)
🪛 GitHub Actions: Rector
app/Deployer.php
[error] 1-1: Rector 'ReadOnlyPropertyRector' would modify private property declarations (e.g., 'private Container $container' to 'private readonly Container $container').
tests/Unit/EnvServiceTest.php
[error] 1-1: Rector 'ReadOnlyPropertyRector' would modify constructor property promotion to include 'readonly' for private properties.
🪛 PHPMD (2.15.0)
tests/Unit/EnvServiceTest.php
15-15: Avoid unused parameters such as '$error'. (undefined)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$files'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing-rules
🔇 Additional comments (12)
.github/workflows/testing-rules.yml (2)
29-31: Past concern resolved: MODEL is defined here.No action needed.
26-59: Switch to summary-only or post inline comments via the APIFile: .github/workflows/testing-rules.yml (lines 26–59)
gh pr review --comment posts a single review body and cannot attach per‑line (inline) comments.
Minimal change (summary‑only):
- 3. leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end + 3. leave a brief summary (1–2 paragraphs) at the endOr create inline comments via the API (POST a review with comments[]):
gh api -X POST repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/reviews \ -f event=COMMENT \ -f body@"$RUNNER_TEMP/review.md" \ -F comments[]@"$RUNNER_TEMP/comments.json"Verify CURSOR_API_KEY is configured in repo/org secrets and that API/rate limits for forks are acceptable.
app/Services/InventoryService.php (4)
26-33: Remove duplicate @return annotation.DocBlock is duplicated above
list().- /** - * @return array<string, mixed> - */
140-155: Avoid@suppression and nativemkdir/file_put_contents; use Filesystem and tighten perms.Also switches 0775 → 0755 as previously noted.
- $dir = $this->getInventoryDir(); - if (!is_dir($dir)) { - if (!@mkdir($dir, 0775, true) && !is_dir($dir)) { - throw new \RuntimeException("Unable to create inventory directory: {$dir}"); - } - } - - $path = $this->getInventoryPath(); - $yaml = Yaml::dump($inventory, 4, 2, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE); - $bytes = @file_put_contents($path, $yaml); - if ($bytes === false) { - throw new \RuntimeException("Failed to write inventory file at {$path}"); - } + $dir = $this->getInventoryDir(); + if (!$this->fs->exists($dir)) { + $this->fs->mkdir($dir, 0755); + } + + $path = $this->getInventoryPath(); + $yaml = Yaml::dump($inventory, 4, 2, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE); + $this->fs->dumpFile($path, $yaml);
119-133: Use Symfony Filesystem for existence checks; wrap file reads.Aligns with architecture rules and simplifies testing.
- if (!is_file($path)) { + if (!$this->fs->exists($path)) { return []; } - $raw = (string) file_get_contents($path); + // Consider injecting a FileReader to avoid native calls; interim keep native read. + $raw = (string) file_get_contents($path); $parsed = Yaml::parse($raw);
7-15: Inject Filesystem and base path; avoidgetcwd()in Services.Constructor injection keeps Services pure/stateless per rules.
use Symfony\Component\Yaml\Yaml; +use Symfony\Component\Filesystem\Filesystem; @@ class InventoryService { + public function __construct( + private readonly Filesystem $fs, + private readonly ?string $baseDir = null, // null → fallback to CWD + ) {}app/Deployer.php (1)
26-29: Composition root note.Instantiating the DI container here is acceptable as the composition root. Add a brief comment to document this exception to the “no new in methods” rule.
app/DTOs/ServerDTO.php (1)
7-34: LGTM.Immutable DTO with clear shape and toArray(). Conforms to guidelines.
tests/Unit/EnvServiceTest.php (2)
12-30: Test doubles are fine; ignore static-analysis nits.Anonymous subclass correctly fakes Filesystem::exists/readFile. PHPMD "unused parameter" hints in tests can be ignored per guidelines.
47-85: Great coverage and AAA structure.Precedence and error-path datasets are clear; no external I/O used. Nice.
app/Services/SSHService.php (1)
86-91: Prefer ed25519 before rsa; fix code to match doc.Code currently checks id_rsa first; swap order.
- if ($home !== '') { - // Default to id_rsa first, then try id_ed25519 - $candidates[] = $home.'/.ssh/id_rsa'; - $candidates[] = $home.'/.ssh/id_ed25519'; + if ($home !== '') { + // Default to id_ed25519 first, then try id_rsa + $candidates[] = $home.'/.ssh/id_ed25519'; + $candidates[] = $home.'/.ssh/id_rsa';app/Services/EnvService.php (1)
19-24: Don’t instantiate Filesystem in parameter default (violates DI rule).Make Filesystem a required injected dependency; keep default only for $envPath.
- public function __construct( - private readonly Filesystem $filesystem = new Filesystem(), - private readonly string $envPath = '.env' - ) { + public function __construct( + private readonly Filesystem $filesystem, + private readonly string $envPath = '.env' + ) { $this->loadDotenvFile(); }
| class Deployer extends Application | ||
| { | ||
| private SymfonyStyle $io; | ||
| private Container $container; |
There was a problem hiding this comment.
Mark container property readonly (Rector failure).
Matches Rector ReadOnlyPropertyRector suggestion and immutability intent.
- private Container $container;
+ private readonly Container $container;📝 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.
| private Container $container; | |
| private readonly Container $container; |
🤖 Prompt for AI Agents
In app/Deployer.php around line 17, the private Container $container property
should be marked readonly to match Rector's ReadOnlyPropertyRector and the
immutability intent; update the property declaration to include the readonly
modifier, ensure the class is using a PHP version that supports readonly
properties (PHP 8.1+), and verify the container is only assigned once (typically
in the constructor) with no subsequent reassignment.
| if ($this->filesystem->exists($this->envPath)) { | ||
| try { | ||
| $content = $this->filesystem->readFile($this->envPath); | ||
| $dotenv = new Dotenv(); | ||
| $parsed = $dotenv->parse($content, $this->envPath); | ||
|
|
||
| foreach ($parsed as $k => $v) { | ||
| if (is_string($k) && is_string($v)) { | ||
| $this->dotenv[$k] = $v; | ||
| } | ||
| } | ||
| } catch (\Throwable) { | ||
| // Silently ignore file reading errors, similar to original behavior | ||
| $this->dotenv = []; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Filesystem::readFile requires Symfony 7.1+. Add fallback or bump dependency.
If the project isn’t on symfony/filesystem >= 7.1, this will fatally error. Either require ^7.1 in composer or add a safe fallback.
- try {
- $content = $this->filesystem->readFile($this->envPath);
+ try {
+ $content = method_exists($this->filesystem, 'readFile')
+ ? $this->filesystem->readFile($this->envPath)
+ : (string) file_get_contents($this->envPath);
$dotenv = new Dotenv();
$parsed = $dotenv->parse($content, $this->envPath);Run to confirm composer constraint:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
jq -r '.require["symfony/filesystem"] // "MISSING"' composer.json 2>/dev/null || echo "composer.json missing"Length of output: 9
Require symfony/filesystem >=7.1 or add a safe fallback for readFile
composer.json allows ^6.0|^7.0, and Filesystem::readFile was introduced in Symfony 7.1 — calling readFile will fatal on older installs; either bump to ^7.1 or use a method_exists fallback. (symfony.com)
File: app/Services/EnvService.php (lines 31–47)
- try {
- $content = $this->filesystem->readFile($this->envPath);
+ try {
+ $content = method_exists($this->filesystem, 'readFile')
+ ? $this->filesystem->readFile($this->envPath)
+ : (string) file_get_contents($this->envPath);
$dotenv = new Dotenv();
$parsed = $dotenv->parse($content, $this->envPath);📝 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.
| if ($this->filesystem->exists($this->envPath)) { | |
| try { | |
| $content = $this->filesystem->readFile($this->envPath); | |
| $dotenv = new Dotenv(); | |
| $parsed = $dotenv->parse($content, $this->envPath); | |
| foreach ($parsed as $k => $v) { | |
| if (is_string($k) && is_string($v)) { | |
| $this->dotenv[$k] = $v; | |
| } | |
| } | |
| } catch (\Throwable) { | |
| // Silently ignore file reading errors, similar to original behavior | |
| $this->dotenv = []; | |
| } | |
| } | |
| } | |
| if ($this->filesystem->exists($this->envPath)) { | |
| try { | |
| $content = method_exists($this->filesystem, 'readFile') | |
| ? $this->filesystem->readFile($this->envPath) | |
| : (string) file_get_contents($this->envPath); | |
| $dotenv = new Dotenv(); | |
| $parsed = $dotenv->parse($content, $this->envPath); | |
| foreach ($parsed as $k => $v) { | |
| if (is_string($k) && is_string($v)) { | |
| $this->dotenv[$k] = $v; | |
| } | |
| } | |
| } catch (\Throwable) { | |
| // Silently ignore file reading errors, similar to original behavior | |
| $this->dotenv = []; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/Services/EnvService.php around lines 31 to 47, the code calls
$this->filesystem->readFile which only exists in symfony/filesystem >=7.1 and
will fatal on older installs; either update composer to require
symfony/filesystem ^7.1 or add a safe runtime fallback: check method_exists or
is_callable for readFile and use it when present, otherwise fallback to a safe
alternative such as file_get_contents($this->envPath) (wrapped in the same
try/catch) so behavior remains the same across Symfony versions.
| it('detects circular dependencies', function () { | ||
| // ARRANGE & ACT & ASSERT | ||
| expect(fn () => $this->container->build(CircularA::class)) | ||
| ->toThrow(RuntimeException::class, 'Cannot resolve dependency'); | ||
| }); |
There was a problem hiding this comment.
Fix expected error message for circular dependency.
Container::build() throws "Circular dependency detected: ..." but the test expects "Cannot resolve dependency", which will fail.
Apply:
- expect(fn () => $this->container->build(CircularA::class))
- ->toThrow(RuntimeException::class, 'Cannot resolve dependency');
+ expect(fn () => $this->container->build(CircularA::class))
+ ->toThrow(RuntimeException::class, 'Circular dependency detected');📝 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.
| it('detects circular dependencies', function () { | |
| // ARRANGE & ACT & ASSERT | |
| expect(fn () => $this->container->build(CircularA::class)) | |
| ->toThrow(RuntimeException::class, 'Cannot resolve dependency'); | |
| }); | |
| it('detects circular dependencies', function () { | |
| // ARRANGE & ACT & ASSERT | |
| expect(fn () => $this->container->build(CircularA::class)) | |
| ->toThrow(RuntimeException::class, 'Circular dependency detected'); | |
| }); |
🤖 Prompt for AI Agents
In tests/Unit/ContainerTest.php around lines 134 to 138, the test expects the
wrong exception message for circular dependency; update the expectation to match
the actual message thrown by Container::build() by changing the toThrow second
argument from 'Cannot resolve dependency' to 'Circular dependency detected' (or
use a partial match/regex that asserts the message starts with 'Circular
dependency detected') so the test matches the real error text.
| ['NonExistentClass', 'does not exist'], | ||
| [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes | ||
| [AbstractClass::class, 'not instantiable'], | ||
| [PrivateConstructor::class, 'not instantiable'], | ||
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | ||
| ]); |
There was a problem hiding this comment.
Adjust interface case: message should be "not instantiable", not "does not exist".
Interfaces satisfy class_exists() and are non‑instantiable; Container emits "is not instantiable".
Apply:
- [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes
+ [TestInterface::class, 'not instantiable'],📝 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.
| ['NonExistentClass', 'does not exist'], | |
| [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes | |
| [AbstractClass::class, 'not instantiable'], | |
| [PrivateConstructor::class, 'not instantiable'], | |
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | |
| ]); | |
| ['NonExistentClass', 'does not exist'], | |
| [TestInterface::class, 'not instantiable'], | |
| [AbstractClass::class, 'not instantiable'], | |
| [PrivateConstructor::class, 'not instantiable'], | |
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | |
| ]); |
🤖 Prompt for AI Agents
In tests/Unit/ContainerTest.php around lines 145 to 150, the expectation for
TestInterface::class incorrectly expects the message "does not exist";
interfaces return as existing but non-instantiable, so change the expected
message for TestInterface::class from 'does not exist' to 'not instantiable' so
the test matches the Container's actual error wording.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Deployer.php (2)
134-164: Replace shell_exec (with @ suppression) by Symfony Process with cwd + error handlingEliminates error suppression, improves testability, and follows the “Symfony Classes” rule.
Apply:
private function getVersionFromGit(): ?string { $projectRoot = dirname(__DIR__, 1); // Check if we're in a git repository if (!is_dir($projectRoot.'/.git')) { return null; } - // Try to get the current tag - $tag = @shell_exec('cd '.escapeshellarg($projectRoot).' && git describe --tags --exact-match 2>/dev/null'); - if ($tag) { - return trim($tag); - } - - // Get the latest tag + commit info - $describe = @shell_exec('cd '.escapeshellarg($projectRoot).' && git describe --tags --always 2>/dev/null'); - if ($describe) { - return trim($describe); - } - - // Get current branch + short commit hash - $branch = @shell_exec('cd '.escapeshellarg($projectRoot).' && git rev-parse --abbrev-ref HEAD 2>/dev/null'); - $commit = @shell_exec('cd '.escapeshellarg($projectRoot).' && git rev-parse --short HEAD 2>/dev/null'); - - if ($branch && $commit) { - return trim($branch).'@'.trim($commit); - } - - return null; + // Try current tag + $try = function (array $cmd) use ($projectRoot): ?string { + $p = new Process($cmd, $projectRoot); + $p->run(); + if ($p->isSuccessful()) { + $out = trim($p->getOutput()); + return $out !== '' ? $out : null; + } + return null; + }; + + foreach ([['git','describe','--tags','--exact-match'], ['git','describe','--tags','--always']] as $cmd) { + if ($val = $try($cmd)) { + return $val; + } + } + + $branch = $try(['git','rev-parse','--abbrev-ref','HEAD']); + $commit = $try(['git','rev-parse','--short','HEAD']); + return ($branch && $commit) ? $branch.'@'.$commit : null; }
16-30: Inject the Container via constructor and make the property readonlyPHP requirement is ^8.2, so readonly properties are supported.
class Deployer extends Application { private SymfonyStyle $io; - private Container $container; + private readonly Container $container; - public function __construct() + public function __construct(Container $container) { $version = $this->getVersionFromComposer(); parent::__construct('Deployer', $version); $this->setDefaultCommand('list'); - $this->container = new Container(); + $this->container = $container; // Register commands $this->registerCommands(); }Update bootstrap: change bin/deployer from (new Deployer())->run(); to (new Deployer(new Container()))->run();
🧹 Nitpick comments (21)
.cursor/commands/review-testing-rules.md (1)
1-2: Make the command actionable and self-containedExpand this into a short checklist so runs don’t rely on external prompt context.
Apply:
-Analyze the changes in this Git working tree and report back on tests that fall short of our testing rules. +Analyze the PR diff and: +1) Flag tests violating rules in .cursor/rules/03-tests.mdc (cite rule + file:line). +2) Identify missing/duplicate/overlapping tests and propose minimal cases. +3) Summarize gaps by layer (CLI/Services/Helpers) with a 3–5 bullet list. +Output: terse inline comments (1–2 sentences) + a final summary block..github/workflows/testing-rules.yml (1)
31-59: Ensure comments actually get posted (don’t rely on the agent’s heuristics)Emit to a file and submit via gh explicitly to avoid silent no‑ops.
Apply:
- cursor-agent --force --model "$MODEL" --output-format=text --print "You are operating in a GitHub Actions runner performing automated code review. The gh CLI is available and authenticated via GH_TOKEN. You may comment on pull requests. + cursor-agent --force --model "$MODEL" --output-format=markdown --output-file review-testing-rules.md --print "You are operating in a GitHub Actions runner performing automated code review. The gh CLI is available and authenticated via GH_TOKEN. You may comment on pull requests. ... - - Use only: gh pr review --comment - - Do not use: gh pr review --approve or --request-changes" + - Use only: gh pr review --comment + - Do not use: gh pr review --approve or --request-changes" + + # Submit the generated review + gh pr review ${{ github.event.pull_request.number }} --comment -F review-testing-rules.mdIf cursor-agent can post comments itself, keep this as a fallback path and skip when it already posted (idempotency).
.cursor/rules/03-tests.mdc (1)
80-103: Add an explicit ban on real FS/network in unit testsYou imply it via “Mock all external dependencies”; make it explicit to avoid ambiguity.
Apply:
**Unit Tests:** - - Mock all external dependencies (filesystem, HTTP, processes) + - Mock all external dependencies (filesystem, HTTP, processes) + - Do not touch real filesystem, network, shells, or external processes.cursor/cli.json (1)
1-6: Tighten permissions with an allow‑list and scoped writesPermit only the exact gh commands needed and a bounded output file pattern.
Apply:
{ "permissions": { - "allow": [], - "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"] + "allow": [ + "Shell(gh pr view *)", + "Shell(gh pr diff *)", + "Shell(gh pr review --comment *)", + "Write(review-*.md)" + ], + "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"] } }Confirm cursor-agent doesn’t require additional commands; expand the allow‑list only as needed.
.cursor/rules/01-architecture.mdc (1)
18-23: Ban constructing the container inside app/Console codeCodify that the bootstrap is responsible for creating and injecting the container; app classes must not call
new Container().Apply:
### Dependency Injection (MANDATORY) - Dependencies are automatically injected by the `app/Container` class - ALL dependencies MUST be injected through constructors - NO manual instantiation - **Exceptions:** Only value objects, DTOs, and pure data structures can be manually instantiated + - Application/Console classes MUST NOT instantiate the DI container. Bootstrap constructs it and injects it.app/Deployer.php (1)
100-117: Use the import instead of FQCN for InstalledVersionsSmall consistency fix.
Apply:
- if (class_exists(\Composer\InstalledVersions::class)) { + if (class_exists(InstalledVersions::class)) { try { - $version = \Composer\InstalledVersions::getPrettyVersion('bigpixelrocket/deployer-php'); + $version = InstalledVersions::getPrettyVersion('bigpixelrocket/deployer-php');tests/Unit/EnvServiceTest.php (1)
15-16: Minor: mark captured ctor params as readonly to appease RectorPurely optional in tests.
Apply:
- public function __construct(private bool $exists, private string $content, private bool $error) + public function __construct(private readonly bool $exists, private readonly string $content, private readonly bool $error)app/Services/SSHService.php (1)
7-11: Import exceptions per guideline; drop FQCN slashes.Follow “Always use use import statements instead of fully qualified class names.”
Apply this diff:
use phpseclib3\Crypt\Common\PrivateKey; use phpseclib3\Crypt\PublicKeyLoader; use phpseclib3\Net\SSH2; +use RuntimeException; +use Throwable; @@ - throw new \RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_ed25519 (or ~/.ssh/id_rsa).'); + throw new RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_ed25519 (or ~/.ssh/id_rsa).'); @@ - throw new \RuntimeException("SSH key is not readable: {$resolvedKeyPath}"); + throw new RuntimeException("SSH key is not readable: {$resolvedKeyPath}"); @@ - } catch (\Throwable $e) { - throw new \RuntimeException('Failed to load SSH private key: '.$e->getMessage(), previous: $e); + } catch (Throwable $e) { + throw new RuntimeException('Failed to load SSH private key: '.$e->getMessage(), previous: $e); @@ - throw new \RuntimeException('Provided key is not a valid private key.'); + throw new RuntimeException('Provided key is not a valid private key.'); @@ - } catch (\Throwable $e) { - throw new \RuntimeException("Failed to initiate SSH connection to {$host}: {$e->getMessage()}", previous: $e); + } catch (Throwable $e) { + throw new RuntimeException("Failed to initiate SSH connection to {$host}: {$e->getMessage()}", previous: $e); @@ - } catch (\Throwable $e) { - throw new \RuntimeException('SSH authentication error: '.$e->getMessage(), previous: $e); + } catch (Throwable $e) { + throw new RuntimeException('SSH authentication error: '.$e->getMessage(), previous: $e); @@ - throw new \RuntimeException('SSH authentication failed. Check username and key permissions.'); + throw new RuntimeException('SSH authentication failed. Check username and key permissions.'); @@ - } catch (\Throwable) { + } catch (Throwable) {Also applies to: 28-46, 50-62, 66-67
app/Items/ServerItem.php (1)
7-9: Import exceptions; avoid root-qualified throws.Conform to the import guideline and keep messages unchanged.
Apply this diff:
use Bigpixelrocket\DeployerPHP\Services\InventoryService; use Bigpixelrocket\DeployerPHP\DTOs\ServerDTO; +use InvalidArgumentException; +use RuntimeException; @@ - if ($this->exists($name)) { - throw new \RuntimeException("Server '{$name}' already exists."); + if ($this->exists($name)) { + throw new RuntimeException("Server '{$name}' already exists."); @@ - if ($server->host === '') { - throw new \InvalidArgumentException('Invalid host.'); + if ($server->host === '') { + throw new InvalidArgumentException('Invalid host.'); @@ - if ($port < 1 || $port > 65535) { - throw new \InvalidArgumentException('Invalid port.'); + if ($port < 1 || $port > 65535) { + throw new InvalidArgumentException('Invalid port.'); @@ - if ($server->user === '') { - throw new \InvalidArgumentException('Invalid user.'); + if ($server->user === '') { + throw new InvalidArgumentException('Invalid user.'); @@ - if ($name === '' || !preg_match('/^[a-zA-Z0-9_.-]+$/', $name)) { - throw new \InvalidArgumentException('Invalid server name. Use letters, numbers, dots, dashes, underscores.'); + if ($name === '' || !preg_match('/^[a-zA-Z0-9_.-]+$/', $name)) { + throw new InvalidArgumentException('Invalid server name. Use letters, numbers, dots, dashes, underscores.');Also applies to: 27-31, 47-58, 63-66
app/Services/EnvService.php (2)
7-13: Import exceptions per guideline.Use imports instead of root-qualified names.
Apply this diff:
use Symfony\Component\Dotenv\Dotenv; use Symfony\Component\Filesystem\Filesystem; +use RuntimeException; +use Throwable; @@ - throw new \RuntimeException("Missing environment {$label}: {$list}"); + throw new RuntimeException("Missing environment {$label}: {$list}");Also applies to: 68-72
26-47: Optional: keep Filesystem-only approach consistent.If you want zero native I/O in services, introduce a tiny FilesystemReader interface with read() and exists() and inject an implementation. I can draft it if you want.
app/Container.php (2)
7-10: Import exceptions; avoid root-qualified throws/catches.Align with import guideline.
Apply this diff:
use ReflectionClass; use ReflectionNamedType; use ReflectionParameter; +use RuntimeException; @@ - throw new \RuntimeException("Circular dependency detected: {$chain}"); + throw new RuntimeException("Circular dependency detected: {$chain}"); @@ - throw new \RuntimeException("Class [{$className}] does not exist"); + throw new RuntimeException("Class [{$className}] does not exist"); @@ - throw new \RuntimeException("Class [{$className}] is not instantiable"); + throw new RuntimeException("Class [{$className}] is not instantiable"); @@ - throw new \RuntimeException( + throw new RuntimeException( "Cannot resolve parameter [{$parameter->getName()}] in class [{$parameter->getDeclaringClass()?->getName()}]" ); @@ - } catch (\RuntimeException $e) { + } catch (RuntimeException $e) { @@ - throw new \RuntimeException( + throw new RuntimeException( "Cannot resolve dependency [{$className}] for parameter [{$parameter->getName()}]", previous: $e );Also applies to: 41-46, 54-56, 128-135, 142-152
122-153: Consider basic union-type support.If a parameter has a union, try the first class-resolvable arm before falling back to default.
Example patch (conceptual):
$type = $parameter->getType(); if ($type instanceof \ReflectionUnionType) { foreach ($type->getTypes() as $arm) { if ($arm instanceof ReflectionNamedType && !$arm->isBuiltin()) { try { return $this->build($arm->getName()); } catch (RuntimeException) { /* try next */ } } } if ($parameter->isDefaultValueAvailable()) { return $parameter->getDefaultValue(); } throw new RuntimeException("Cannot resolve union parameter [{$parameter->getName()}]"); }app/Services/InventoryService.php (3)
149-154: Use LOCK_EX when writing YAML to avoid concurrent write corruption.Apply this diff:
- $bytes = @file_put_contents($path, $yaml); + $bytes = @file_put_contents($path, $yaml, LOCK_EX);
7-9: Import exceptions; avoid root-qualified throws.Align with import guideline across the file.
Apply this diff:
use Symfony\Component\Yaml\Yaml; +use RuntimeException; @@ - throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + throw new RuntimeException("Key '{$key}' not found in collection '{$collection}'."); @@ - throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + throw new RuntimeException("Key '{$key}' not found in collection '{$collection}'."); @@ - throw new \RuntimeException("Unable to create inventory directory: {$dir}"); + throw new RuntimeException("Unable to create inventory directory: {$dir}"); @@ - if ($bytes === false) { - throw new \RuntimeException("Failed to write inventory file at {$path}"); + if ($bytes === false) { + throw new RuntimeException("Failed to write inventory file at {$path}");Also applies to: 61-67, 100-106, 140-155
119-133: Optional: migrate to injected Filesystem for testability.For consistency with EnvService and easier mocking, inject Filesystem and replace native I/O. I can draft a scoped patch if desired.
app/Console/Server/ServerCreateCommand.php (4)
7-16: Avoid fully qualified global classes; import Throwable.Per guidelines, prefer use imports over \Throwable. Also keeps consistency with the rest of the file.
Apply this diff:
use Symfony\Component\Console\Style\SymfonyStyle; +use Throwable; @@ - } catch (\Throwable $e) { + } catch (Throwable $e) {Also applies to: 73-77
33-36: Clarify key default to match SSHService fallbacks.Help text mentions only id_rsa; SSHService also tries id_ed25519. Reflect both to reduce confusion.
- ->addOption('key', null, InputOption::VALUE_OPTIONAL, 'Path to SSH private key (default: ~/.ssh/id_rsa)'); + ->addOption('key', null, InputOption::VALUE_OPTIONAL, 'Path to SSH private key (default: ~/.ssh/id_rsa or ~/.ssh/id_ed25519)');
55-59: Fail fast on duplicate server name before network calls.Avoid an unnecessary SSH attempt if the name already exists.
- $keyPath = $input->getOption('key'); + $keyPath = $input->getOption('key'); $keyPath = $keyPath !== null && $keyPath !== '' ? $keyPath : null; + // Fail fast if name is already taken + if ($this->servers->exists($name)) { + $io->error("Server '{$name}' already exists."); + return Command::FAILURE; + } + try {
18-26: Add minimal DocBlocks per repo rules.Short class/method DocBlocks with params/returns are required for app/**/*.php.
#[AsCommand(name: 'server:create', description: 'Create a server entry and verify SSH connectivity')] +/** + * Console command to create a server entry after verifying SSH connectivity. + * + * @internal CLI surface + */ class ServerCreateCommand extends Command { public function __construct( private readonly SSHService $sshService, private readonly ServerItem $servers, ) { parent::__construct(); } + /** + * Configure command arguments and options. + */ protected function configure(): void { $this ->addArgument('name', InputArgument::REQUIRED, 'Server name (unique identifier)') ->addArgument('host', InputArgument::REQUIRED, 'Server host (IP or FQDN)') ->addOption('port', null, InputOption::VALUE_REQUIRED, 'SSH port', '22') ->addOption('user', null, InputOption::VALUE_REQUIRED, 'SSH username', 'root') ->addOption('key', null, InputOption::VALUE_OPTIONAL, 'Path to SSH private key (default: ~/.ssh/id_rsa or ~/.ssh/id_ed25519)'); } + /** + * Execute the command. + * + * @param InputInterface $input + * @param OutputInterface $output + * @return int Command::SUCCESS on success, Command::FAILURE otherwise + */ protected function execute(InputInterface $input, OutputInterface $output): int { $io = new SymfonyStyle($input, $output); @@ } catch (\Throwable $e) { $io->error($e->getMessage()); return Command::FAILURE; } } }Also applies to: 28-36, 38-78
tests/Unit/ContainerTest.php (1)
163-165: Replace forbidden type-only assertion.Repo rules forbid toBeInstanceOf; assert behavior instead.
- expect($result)->toBeInstanceOf(SimpleService::class); + expect($result->getName())->toBe('simple');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.cursor/cli.json(1 hunks).cursor/commands/review-testing-rules.md(1 hunks).cursor/rules/01-architecture.mdc(2 hunks).cursor/rules/03-tests.mdc(1 hunks).github/workflows/testing-rules.yml(1 hunks)app/Console/Server/ServerCreateCommand.php(1 hunks)app/Container.php(1 hunks)app/DTOs/ServerDTO.php(1 hunks)app/Deployer.php(3 hunks)app/Items/ServerItem.php(1 hunks)app/Services/EnvService.php(1 hunks)app/Services/InventoryService.php(1 hunks)app/Services/SSHService.php(1 hunks)tests/Unit/ContainerTest.php(1 hunks)tests/Unit/EnvServiceTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
{tests/**,**/*Test.php,**/*.test.php,phpunit.xml,phpunit.xml.dist}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not write or run tests unless specifically instructed
Files:
tests/Unit/EnvServiceTest.phptests/Unit/ContainerTest.php
{app,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-code-quality.mdc)
{app,tests}/**/*.php: Add DocBlock comments with minimalist descriptions, parameters, and return types for PHP classes and functions
Use comments to separate sections of code and to explain or summarize complex logic; avoid commenting the obvious
Format comment sections as headers/subheaders/paragraphs and separate them with a single newline
Do not leave stale comments behind when removing code
Run Rector on all changed PHP files before completing a task
Run Pint (code style fixer) on all changed PHP files before completing a task
Files:
tests/Unit/EnvServiceTest.phpapp/Services/InventoryService.phpapp/DTOs/ServerDTO.phpapp/Items/ServerItem.phpapp/Container.phpapp/Services/SSHService.phpapp/Console/Server/ServerCreateCommand.phpapp/Services/EnvService.phpapp/Deployer.phptests/Unit/ContainerTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-code-quality.mdc)
Never run static analysis against tests
tests/**/*.php: Use Pest exclusively with it() syntax for all tests
Follow the AAA pattern with explicit headers in every test: // ARRANGE, // ACT, // ASSERT, and // CLEANUP when needed
For exception-focused tests, use a combined // ACT & ASSERT section header
Focus tests on core business logic only; do not test the framework itself
Use minimal test data and simplest possible setup
Prefer built-in expect() assertions over custom assertions
Only include essential edge cases that matter to business behavior
Do not write performance tests unless performance is a primary concern
Ignore PHPStan issues in tests
Avoid excessive phpdoc in tests; add only when necessary for clarity
Organize tests with logical describe() groupings
Extract repeated mocking into reusable helper methods
Create test traits for shared behavior across tests
Use beforeEach() for common setup within groups
Ensure proper cleanup in tests to maintain isolation (temp files, time state, etc.)
Build helper functions for creating test configurations and mock data
Forbidden assertions/patterns: type-only or meaningless expectations (toBeInstanceOf, toBeArray, not->toBeNull, toBeTrue, expect(true)->toBeTrue), property type tests, and sleep(...)
Assert specific values and behaviors rather than generic types
Use datasets for input variations in Pest tests
Mock only external dependencies; do not mock internal behavior under test
Unit tests must be isolated: test single units, mock/fake all external dependencies, run without external systems, and complete quickly
Unit tests must not use real filesystem, network, shell commands, external services, or actual HTTP/process executions
Use integration tests (not unit tests) for filesystem operations and external processes; unit tests for pure business logic with mocks
Layer testing strategy: CLI commands as integration tests (mock external services/processes), business services as unit tests (mock all externals), utilities/he...
Files:
tests/Unit/EnvServiceTest.phptests/Unit/ContainerTest.php
app/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/*.php: Follow PSR-12, declare strict_types, and leverage PHP 8.x features (unions, match, attributes, readonly)
Always useuseimport statements instead of fully qualified class names in code
All methods must declare explicit return types and use proper generics annotations (e.g., Collection<int, User>)
All dependencies must be injected via constructors; no manual instantiation
Use a ServiceContainer/DI container for all object creation
Never usenew ClassName()inside methods—always inject dependencies
Only value objects/DTOs/pure data structures may be manually instantiated
No circular dependencies
Declare all dependencies in constructor signaturesRun PHPStan static analysis on changed PHP files, excluding tests
Files:
app/Services/InventoryService.phpapp/DTOs/ServerDTO.phpapp/Items/ServerItem.phpapp/Container.phpapp/Services/SSHService.phpapp/Console/Server/ServerCreateCommand.phpapp/Services/EnvService.phpapp/Deployer.php
app/**/@(Service|Services)/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/@(Service|Services)/**/*.php: Services provide atomic, reusable functionality and must not perform console I/O
Services accept and return plain PHP data types
Services must be stateless and use dependency injection
Services handle core business logic, external API calls, and file operations
Extract complex orchestration shared by multiple Commands into dedicated Services
Services return exceptions or structured data for Commands to handle
Validation errors and business exceptions should bubble up to Commands for display
Services receive other Services/utilities via constructor injection
Services may depend on other Services or utilities
Files:
app/Services/InventoryService.phpapp/Services/SSHService.phpapp/Services/EnvService.php
🧬 Code graph analysis (6)
tests/Unit/EnvServiceTest.php (1)
app/Services/EnvService.php (3)
EnvService(14-77)__construct(19-24)get(54-75)
app/Items/ServerItem.php (2)
app/Services/InventoryService.php (3)
InventoryService(14-166)set(72-81)has(47-54)app/DTOs/ServerDTO.php (3)
ServerDTO(10-34)__construct(12-18)toArray(25-33)
app/Console/Server/ServerCreateCommand.php (3)
app/Items/ServerItem.php (3)
ServerItem(13-67)__construct(15-17)create(22-32)app/Services/SSHService.php (2)
SSHService(17-118)assertCanConnect(24-70)app/DTOs/ServerDTO.php (2)
ServerDTO(10-34)__construct(12-18)
app/Services/EnvService.php (1)
tests/Unit/EnvServiceTest.php (3)
__construct(15-17)exists(18-21)readFile(22-28)
app/Deployer.php (1)
app/Container.php (2)
Container(21-154)build(36-77)
tests/Unit/ContainerTest.php (1)
app/Container.php (2)
Container(21-154)build(36-77)
🪛 PHPMD (2.15.0)
tests/Unit/EnvServiceTest.php
15-15: Avoid unused parameters such as '$error'. (undefined)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$files'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
🪛 GitHub Actions: Rector
tests/Unit/EnvServiceTest.php
[warning] 12-12: ReadOnlyPropertyRector suggestion: mark constructor properties as readonly in the test stub.
app/Deployer.php
[warning] 14-14: ReadOnlyPropertyRector suggestion: make the $container property readonly.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing-rules
🔇 Additional comments (17)
.github/workflows/testing-rules.yml (1)
26-31: MODEL env now defined — goodThis addresses the prior feedback about an undefined variable.
.cursor/rules/03-tests.mdc (1)
16-36: Balanced minimalism guidance looks goodTargets/readability carve-outs are clear and pragmatic.
.cursor/rules/01-architecture.mdc (1)
13-17: Good call-out to prefer Symfony abstractionsThis will make mocking/timeouts consistent across the codebase.
app/Deployer.php (1)
79-95: Command registration looks goodContainer-based wiring keeps constructors clean and testable.
app/DTOs/ServerDTO.php (1)
20-33: LGTM: immutable DTO with shaped-array serializerClean use of promoted readonly props and explicit return types.
tests/Unit/EnvServiceTest.php (2)
48-85: Great AAA structure and dataset coverageClear arrange/act/assert sections, env cleanup, and precedence cases.
18-28: Ignore PHPMD “unused parameter” in testsPer rules, static analysis in tests is out of scope; no change needed.
app/Services/SSHService.php (3)
28-33: Prefer ed25519 over RSA; fix mismatch between comment, error text, and implementation.ed25519 should be tried before RSA, and the error text should reflect the same order.
Apply this diff:
- throw new \RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_rsa (or ~/.ssh/id_ed25519).'); + throw new RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_ed25519 (or ~/.ssh/id_rsa).'); @@ - // Default to id_rsa first, then try id_ed25519 - $candidates[] = $home.'/.ssh/id_rsa'; - $candidates[] = $home.'/.ssh/id_ed25519'; + // Default to id_ed25519 first, then try id_rsa + $candidates[] = $home.'/.ssh/id_ed25519'; + $candidates[] = $home.'/.ssh/id_rsa';Also applies to: 88-91
24-24: Add unit tests for key resolution and auth paths.Missing coverage for: explicit key path, tilde expansion, default key resolution, bad key parse, connection/auth failures.
Script to check if tests exist:
#!/bin/bash rg -nP --type=php 'SSHService|assertCanConnect|resolvePrivateKeyPath' tests -C2 || true
49-56: Add a timeout to SSH2 to avoid indefinite hangs.Set a timeout on the SSH2 instance after construction (10s is a sane default).
- $ssh = new SSH2($host, $port); + $ssh = new SSH2($host, $port); + $ssh->setTimeout(10);Vendor/lock files weren't available in the sandbox; confirm the target phpseclib version supports SSH2::setTimeout by checking composer.lock/composer.json or running:
rg -n 'phpseclib' composer.lock composer.json || composer show phpseclib/phpseclibapp/Items/ServerItem.php (1)
31-32: LGTM: Creation persists validated DTO and existence check is atomic via InventoryService.Also applies to: 37-40
app/Container.php (1)
21-77: LGTM: Reflection-based auto-wiring with circular detection and cleanup is solid.app/Services/InventoryService.php (2)
31-34: Duplicate @return annotations; remove the redundant block.Apply this diff:
- /** - * @return array<string, mixed> - */
142-147: Tighten directory perms; prefer 0755 over 0775.Reduces group write exposure by default.
Apply this diff:
- if (!@mkdir($dir, 0775, true) && !is_dir($dir)) { + if (!@mkdir($dir, 0755, true) && !is_dir($dir)) {app/Services/EnvService.php (1)
19-24: Enforce DI: removenew Filesystem()default and avoidnew Dotenv()inside methods. Also avoid$dotenvname collision.Follow the DI rule and improve clarity by renaming the in‑memory store.
Apply this diff:
- /** @var array<string, string> */ - private array $dotenv = []; + /** @var array<string, string> */ + private array $store = []; @@ - public function __construct( - private readonly Filesystem $filesystem = new Filesystem(), - private readonly string $envPath = '.env' - ) { + public function __construct( + private readonly Filesystem $filesystem, + private readonly Dotenv $parser, + private readonly string $envPath = '.env' + ) { $this->loadDotenvFile(); } @@ - $raw = @file_get_contents($this->envPath); - if ($raw === false) { - return; // unreadable; keep behavior silent - } - $parsed = (new Dotenv())->parse($raw, $this->envPath); + $raw = @file_get_contents($this->envPath); + if ($raw === false) { + return; // unreadable; keep behavior silent + } + $parsed = $this->parser->parse($raw, $this->envPath); @@ - $this->dotenv[$k] = $v; + $this->store[$k] = $v; @@ - // Silently ignore file reading errors, similar to original behavior - $this->dotenv = []; + // Silently ignore file reading errors, similar to original behavior + $this->store = []; @@ - if (isset($this->dotenv[$key]) && $this->dotenv[$key] !== '') { - return $this->dotenv[$key]; + if (isset($this->store[$key]) && $this->store[$key] !== '') { + return $this->store[$key];Also applies to: 31-45, 63-65
app/Console/Server/ServerCreateCommand.php (1)
63-71: LGTM: happy path flow and persistence.Connectivity check, DTO creation (allowed), and ServerItem->create integration look coherent.
Please confirm: InventoryService writes to .deployer/inventory.yml atomically or with fsync to avoid partial writes on interruption.
tests/Unit/ContainerTest.php (1)
101-116: Solid use of Pest, AAA headers, and isolation.Fixtures are inline, tests are fast, and expectations focus on behavior. Nice.
| jobs: | ||
| testing-rules: | ||
| runs-on: ubuntu-latest | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
There was a problem hiding this comment.
Gate on forks/secrets to prevent noisy failures
Skip when secrets aren’t available (PRs from forks).
Apply:
jobs:
testing-rules:
runs-on: ubuntu-latest
+ if: ${{ github.event.pull_request.head.repo.fork == false && secrets.CURSOR_API_KEY != '' }}
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true📝 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.
| jobs: | |
| testing-rules: | |
| runs-on: ubuntu-latest | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| jobs: | |
| testing-rules: | |
| runs-on: ubuntu-latest | |
| if: ${{ github.event.pull_request.head.repo.fork == false && secrets.CURSOR_API_KEY != '' }} | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true |
🤖 Prompt for AI Agents
In .github/workflows/testing-rules.yml around lines 10-16, the workflow needs to
skip the testing-rules job for PRs from forks where repository secrets are
unavailable; add a job-level if conditional that only runs the job when either
the event is not a pull_request or the pull_request head repo is not a fork
(e.g. if: ${{ github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.fork == false }}), placing this directly
under the job name so the job is gated and won’t run for forked PRs without
secrets.
| - name: Install Cursor CLI | ||
| run: | | ||
| curl https://cursor.com/install -fsS | bash | ||
| echo "$HOME/.cursor/bin" >> $GITHUB_PATH | ||
|
|
There was a problem hiding this comment.
❓ Verification inconclusive
Pin and verify the installer to reduce supply‑chain risk
Avoid curl|bash of a moving script; pin a version and verify checksum.
Example hardening:
- - name: Install Cursor CLI
- run: |
- curl https://cursor.com/install -fsS | bash
- echo "$HOME/.cursor/bin" >> $GITHUB_PATH
+ - name: Install Cursor CLI (pinned)
+ run: |
+ set -euo pipefail
+ VERSION="vX.Y.Z" # pin a known-good version
+ curl -fsSL "https://cursor.com/install?version=${VERSION}" -o /tmp/cursor-install.sh
+ sha256sum /tmp/cursor-install.sh # TODO: compare against a checked-in expected sum
+ bash /tmp/cursor-install.sh
+ echo "$HOME/.cursor/bin" >> "$GITHUB_PATH"Please confirm Cursor provides a versioned installer and published checksums; if not, consider packaging or a custom action.
Pin and verify the Cursor installer (supply‑chain risk)
The workflow runs the upstream installer via curl | bash. (docs.cursor.com)
The installer at https://cursor.com/install downloads a versioned package (e.g. 2025.09.18-7ae6800) but the script does not verify checksums. (cursor.com)
No official checksums were found on Cursor’s downloads page. (cursor.com)
- Update .github/workflows/testing-rules.yml (Install Cursor CLI step) to fetch a specific release artifact (downloads.cursor.com/.../agent-cli-package.tar.gz) and verify its SHA256 before extracting, or use a pinned, audited GitHub Action that installs the CLI.
- If Cursor does not publish checksums, vendor the artifact or pin a reviewed installer script and keep the expected checksum in the repo or a secret for CI verification.
| $io = new SymfonyStyle($input, $output); | ||
|
|
There was a problem hiding this comment.
Do not instantiate services in methods; inject a style factory.
"Never use new inside methods" (except DTOs). Create a ConsoleStyleFactory (or equivalent) and inject it, e.g., $io = $this->styleFactory->make($input, $output).
If you want, I can draft a minimal ConsoleStyleFactory and wire-up changes across Deployer registration.
🤖 Prompt for AI Agents
In app/Console/Server/ServerCreateCommand.php around lines 40-41, avoid
instantiating SymfonyStyle with new inside the execute method; instead inject a
ConsoleStyleFactory (or SymfonyStyleFactory) via the command constructor, store
it as a typed property, and replace the inline instantiation with $io =
$this->styleFactory->make($input, $output). Update the service wiring (service
definition or autowiring) to provide the new factory to this command, and ensure
the factory exposes a make(InputInterface, OutputInterface): SymfonyStyle
method; adjust imports and constructor signature accordingly.
| /** @var string|int $portOption */ | ||
| $portOption = $input->getOption('port'); | ||
| $port = (int) $portOption; | ||
|
|
There was a problem hiding this comment.
Validate --port is numeric and within 1..65535.
Casting a non-numeric string yields 0; SSH connect will fail with a misleading error. Add explicit validation before use.
- /** @var string|int $portOption */
- $portOption = $input->getOption('port');
- $port = (int) $portOption;
+ /** @var string|int $portOption */
+ $portOption = $input->getOption('port');
+ $portStr = (string) $portOption;
+ if (filter_var(
+ $portStr,
+ FILTER_VALIDATE_INT,
+ ['options' => ['min_range' => 1, 'max_range' => 65535]]
+ ) === false) {
+ $io->error('Invalid --port. Must be an integer between 1 and 65535.');
+ return Command::FAILURE;
+ }
+ $port = (int) $portStr;📝 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.
| /** @var string|int $portOption */ | |
| $portOption = $input->getOption('port'); | |
| $port = (int) $portOption; | |
| /** @var string|int $portOption */ | |
| $portOption = $input->getOption('port'); | |
| $portStr = (string) $portOption; | |
| if (filter_var( | |
| $portStr, | |
| FILTER_VALIDATE_INT, | |
| ['options' => ['min_range' => 1, 'max_range' => 65535]] | |
| ) === false) { | |
| $io->error('Invalid --port. Must be an integer between 1 and 65535.'); | |
| return Command::FAILURE; | |
| } | |
| $port = (int) $portStr; |
🤖 Prompt for AI Agents
In app/Console/Server/ServerCreateCommand.php around lines 48 to 51, the code
casts the port option directly to int which turns non-numeric input into 0; add
explicit validation: check that $portOption is numeric (ctype_digit or
is_numeric) and that the parsed integer is between 1 and 65535, and if not,
write an error message to the console (or throw an InvalidArgumentException) and
return/exit with a non-zero status so the command fails fast with a clear
message.
| use Symfony\Component\Console\Application; | ||
| use Symfony\Component\Console\Command\Command; | ||
| use Symfony\Component\Console\Input\InputInterface; | ||
| use Symfony\Component\Console\Output\OutputInterface; | ||
| use Symfony\Component\Console\Style\SymfonyStyle; | ||
| use Bigpixelrocket\DeployerPHP\Console\Server\ServerCreateCommand; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import InstalledVersions and Symfony Process instead of using FQCNs and shell_exec
Aligns with import rules and enables safer process execution.
Apply:
use Symfony\Component\Console\Application;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Style\SymfonyStyle;
+use Symfony\Component\Process\Process;
+use Composer\InstalledVersions;
use Bigpixelrocket\DeployerPHP\Console\Server\ServerCreateCommand;📝 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.
| use Symfony\Component\Console\Application; | |
| use Symfony\Component\Console\Command\Command; | |
| use Symfony\Component\Console\Input\InputInterface; | |
| use Symfony\Component\Console\Output\OutputInterface; | |
| use Symfony\Component\Console\Style\SymfonyStyle; | |
| use Bigpixelrocket\DeployerPHP\Console\Server\ServerCreateCommand; | |
| use Symfony\Component\Console\Application; | |
| use Symfony\Component\Console\Command\Command; | |
| use Symfony\Component\Console\Input\InputInterface; | |
| use Symfony\Component\Console\Output\OutputInterface; | |
| use Symfony\Component\Console\Style\SymfonyStyle; | |
| use Symfony\Component\Process\Process; | |
| use Composer\InstalledVersions; | |
| use Bigpixelrocket\DeployerPHP\Console\Server\ServerCreateCommand; |
🤖 Prompt for AI Agents
In app/Deployer.php around lines 7 to 13, the file currently references classes
via fully-qualified class names and calls shell_exec directly; instead import
Composer\InstalledVersions and Symfony\Component\Process\Process at the top and
replace FQCN usages with those imports, then replace any shell_exec calls with a
Symfony Process instantiation (Process::fromShellCommandline(...) or new
Process([...]) as appropriate), run the process, check/handle its exit code and
output via $process->run() and $process->getOutput()/getErrorOutput(), and throw
or log on failure; ensure to update use statements and adjust code paths to use
the imported class names rather than FQCNs.
| class SSHService | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid new inside services; inject an SSH2 factory.
Architecture rule: no manual instantiation in services. Inject a callable factory to create SSH2.
Apply this diff:
class SSHService
{
+ /** @var callable(string,int): SSH2 */
+ private readonly $sshFactory;
+
+ public function __construct(callable $sshFactory)
+ {
+ $this->sshFactory = $sshFactory;
+ }
@@
- try {
- $ssh = new SSH2($host, $port);
+ try {
+ /** @var SSH2 $ssh */
+ $ssh = ($this->sshFactory)($host, $port);Outside this file, wire the factory via your container/bootstrap:
$sshFactory = static fn (string $host, int $port) => new SSH2($host, $port);
$container->bind(SSHService::class, fn() => new SSHService($sshFactory));Also applies to: 48-52
🤖 Prompt for AI Agents
In app/Services/SSHService.php around lines 17-18 (and also affecting lines
48-52), the class currently performs manual instantiation of SSH2 inside the
service which violates the architecture rule; change the constructor to accept
an injected callable/factory (callable(string $host, int $port): SSH2) and
replace any internal "new SSH2(...)" usages with calls to that factory; update
property types and usages accordingly so the service calls the injected factory
to create SSH2 instances, and wire the factory in the container/bootstrap as
described in the review comment.
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| use Bigpixelrocket\DeployerPHP\Container; | ||
|
|
||
| // | ||
| // Inline Test Fixtures | ||
| // ------------------------------------------------------------------------------- | ||
|
|
||
| class SimpleService | ||
| { | ||
| public function getName(): string | ||
| { | ||
| return 'simple'; | ||
| } | ||
| } | ||
|
|
||
| class NoConstructorService | ||
| { | ||
| public function getType(): string | ||
| { | ||
| return 'no-constructor'; | ||
| } | ||
| } | ||
|
|
||
| class ServiceWithDependency | ||
| { | ||
| public function __construct(private readonly SimpleService $service) | ||
| { | ||
| } | ||
| public function getDependency(): SimpleService | ||
| { | ||
| return $this->service; | ||
| } | ||
| } | ||
|
|
||
| class ServiceWithMultipleDeps | ||
| { | ||
| public function __construct(private readonly SimpleService $s1, private readonly ServiceWithDependency $s2) | ||
| { | ||
| } | ||
| public function getSimple(): SimpleService | ||
| { | ||
| return $this->s1; | ||
| } | ||
| public function getComplex(): ServiceWithDependency | ||
| { | ||
| return $this->s2; | ||
| } | ||
| } | ||
|
|
||
| class ServiceWithDefaults | ||
| { | ||
| public function __construct(private readonly SimpleService $service, private readonly string $name = 'default') | ||
| { | ||
| } | ||
| public function getName(): string | ||
| { | ||
| return $this->name; | ||
| } | ||
| } | ||
|
|
||
| class CircularA | ||
| { | ||
| public function __construct(private readonly CircularB $b) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| class CircularB | ||
| { | ||
| public function __construct(private readonly CircularA $a) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| class ServiceWithScalarParam | ||
| { | ||
| public function __construct(private readonly string $required) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| interface TestInterface | ||
| { | ||
| } | ||
|
|
||
| abstract class AbstractClass | ||
| { | ||
| } | ||
|
|
||
| class PrivateConstructor | ||
| { | ||
| private function __construct() | ||
| { | ||
| } | ||
| } | ||
|
|
||
|
|
||
| describe('Container', function () { | ||
| beforeEach(function () { | ||
| $this->container = new Container(); | ||
| }); | ||
|
|
||
| it('builds classes without dependencies', function () { | ||
| // ARRANGE & ACT | ||
| $simple = $this->container->build(SimpleService::class); | ||
| $noConstructor = $this->container->build(NoConstructorService::class); | ||
|
|
||
| // ASSERT | ||
| expect($simple->getName())->toBe('simple') | ||
| ->and($noConstructor->getType())->toBe('no-constructor') | ||
| ->and($this->container->build(SimpleService::class))->not->toBe($simple); // New instances | ||
| }); | ||
|
|
||
| it('resolves dependencies recursively', function () { | ||
| // ARRANGE & ACT | ||
| $service = $this->container->build(ServiceWithMultipleDeps::class); | ||
|
|
||
| // ASSERT | ||
| expect($service->getSimple()->getName())->toBe('simple') | ||
| ->and($service->getComplex()->getDependency()->getName())->toBe('simple'); | ||
| }); | ||
|
|
||
| it('uses default parameter values', function () { | ||
| // ARRANGE & ACT | ||
| $service = $this->container->build(ServiceWithDefaults::class); | ||
|
|
||
| // ASSERT | ||
| expect($service->getName())->toBe('default'); | ||
| }); | ||
|
|
||
| it('detects circular dependencies', function () { | ||
| // ARRANGE & ACT & ASSERT | ||
| expect(fn () => $this->container->build(CircularA::class)) | ||
| ->toThrow(RuntimeException::class, 'Cannot resolve dependency'); | ||
| }); | ||
|
|
||
| it('throws exceptions for invalid classes', function (string $className, string $errorPattern) { | ||
| // ARRANGE & ACT & ASSERT | ||
| expect(fn () => $this->container->build($className)) | ||
| ->toThrow(RuntimeException::class, $errorPattern); | ||
| })->with([ | ||
| ['NonExistentClass', 'does not exist'], | ||
| [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes | ||
| [AbstractClass::class, 'not instantiable'], | ||
| [PrivateConstructor::class, 'not instantiable'], | ||
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | ||
| ]); | ||
|
|
||
| it('cleans up state after errors', function () { | ||
| // ARRANGE | ||
| try { | ||
| $this->container->build(CircularA::class); | ||
| } catch (RuntimeException) { | ||
| // Expected | ||
| } | ||
|
|
||
| // ACT - Should work fine after error | ||
| $result = $this->container->build(SimpleService::class); | ||
|
|
||
| // ASSERT | ||
| expect($result)->toBeInstanceOf(SimpleService::class); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Optional: verify messages against Container source.
Quick grep to confirm error strings in app/Container.php and reduce flaky message assertions.
🏁 Script executed:
#!/bin/bash
# Check exception messages used by Container
rg -n -C2 -e 'Circular dependency detected' -e 'not instantiable' -e 'does not exist' -e 'Cannot resolve parameter' app/Container.phpLength of output: 768
Update test to match Container exception message
app/Container.php throws "Circular dependency detected: {chain}" (app/Container.php:41). In tests/Unit/ContainerTest.php change the expectation in the "detects circular dependencies" spec from 'Cannot resolve dependency' to 'Circular dependency detected' (or assert only RuntimeException::class). Other expected messages in the data provider ('does not exist', 'not instantiable', 'Cannot resolve parameter') already match.
🤖 Prompt for AI Agents
In tests/Unit/ContainerTest.php lines 1-166 update the "detects circular
dependencies" spec to match the Container exception message: change the
expectation string from 'Cannot resolve dependency' to 'Circular dependency
detected' (or remove the message assertion and only assert
RuntimeException::class) so the test matches app/Container.php's thrown message;
leave the other data-provider expectations unchanged.
| it('detects circular dependencies', function () { | ||
| // ARRANGE & ACT & ASSERT | ||
| expect(fn () => $this->container->build(CircularA::class)) | ||
| ->toThrow(RuntimeException::class, 'Cannot resolve dependency'); | ||
| }); |
There was a problem hiding this comment.
Fix expected message for circular dependency.
Container throws "Circular dependency detected: …", not "Cannot resolve dependency".
- it('detects circular dependencies', function () {
+ it('detects circular dependencies', function () {
// ARRANGE & ACT & ASSERT
expect(fn () => $this->container->build(CircularA::class))
- ->toThrow(RuntimeException::class, 'Cannot resolve dependency');
+ ->toThrow(RuntimeException::class, 'Circular dependency detected');
});📝 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.
| it('detects circular dependencies', function () { | |
| // ARRANGE & ACT & ASSERT | |
| expect(fn () => $this->container->build(CircularA::class)) | |
| ->toThrow(RuntimeException::class, 'Cannot resolve dependency'); | |
| }); | |
| it('detects circular dependencies', function () { | |
| // ARRANGE & ACT & ASSERT | |
| expect(fn () => $this->container->build(CircularA::class)) | |
| ->toThrow(RuntimeException::class, 'Circular dependency detected'); | |
| }); |
🤖 Prompt for AI Agents
In tests/Unit/ContainerTest.php around lines 134 to 138, the test asserts the
wrong exception message for circular dependency; update the expectation to match
the container's actual message by changing the toThrow assertion to expect
RuntimeException::class with the message starting "Circular dependency detected"
(e.g. 'Circular dependency detected' or the exact prefix used by the container)
so the test matches the real error text.
| ['NonExistentClass', 'does not exist'], | ||
| [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes | ||
| [AbstractClass::class, 'not instantiable'], | ||
| [PrivateConstructor::class, 'not instantiable'], | ||
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | ||
| ]); |
There was a problem hiding this comment.
Adjust dataset expectations to match Container errors.
Interfaces/privately-constructible classes are “not instantiable”; only unknown classes “do not exist”.
})->with([
['NonExistentClass', 'does not exist'],
- [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes
+ [TestInterface::class, 'not instantiable'],
[AbstractClass::class, 'not instantiable'],
[PrivateConstructor::class, 'not instantiable'],
[ServiceWithScalarParam::class, 'Cannot resolve parameter'],
]);📝 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.
| ['NonExistentClass', 'does not exist'], | |
| [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes | |
| [AbstractClass::class, 'not instantiable'], | |
| [PrivateConstructor::class, 'not instantiable'], | |
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | |
| ]); | |
| })->with([ | |
| ['NonExistentClass', 'does not exist'], | |
| [TestInterface::class, 'not instantiable'], | |
| [AbstractClass::class, 'not instantiable'], | |
| [PrivateConstructor::class, 'not instantiable'], | |
| [ServiceWithScalarParam::class, 'Cannot resolve parameter'], | |
| ]); |
🤖 Prompt for AI Agents
In tests/Unit/ContainerTest.php around lines 145 to 150, the dataset
expectations are incorrect: interfaces and classes with private constructors
should expect an error message of "not instantiable" rather than "does not
exist". Update the dataset rows so that TestInterface::class and
PrivateConstructor::class use 'not instantiable', keep 'NonExistentClass' as
'does not exist', and leave ServiceWithScalarParam::class as 'Cannot resolve
parameter'.
| function mockFilesystem(bool $exists = true, string $content = '', bool $throwError = false): Filesystem | ||
| { | ||
| return new class ($exists, $content, $throwError) extends Filesystem { | ||
| public function __construct(private bool $exists, private string $content, private bool $error) | ||
| { | ||
| } | ||
| public function exists(string|iterable $files): bool | ||
| { | ||
| return $this->exists; | ||
| } | ||
| public function readFile(string $filename): string | ||
| { | ||
| if ($this->error) { | ||
| throw new \RuntimeException('Permission denied'); | ||
| } | ||
| return $this->content; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Test stub hides a production bug: Filesystem::readFile doesn’t exist in Symfony
EnvService calls $filesystem->readFile(), which the real Symfony Filesystem doesn’t provide; tests pass only because the stub adds it.
Refactor EnvService to depend on a tiny FileReader interface (or use native file_get_contents behind a wrapper), inject it, and update tests to mock that interface. Example interface:
interface FileReader { public function read(string $path): string; }Then replace the Filesystem call with the reader.
🧰 Tools
🪛 PHPMD (2.15.0)
15-15: Avoid unused parameters such as '$error'. (undefined)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$files'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
🪛 GitHub Actions: Rector
[warning] 12-12: ReadOnlyPropertyRector suggestion: mark constructor properties as readonly in the test stub.
🤖 Prompt for AI Agents
In tests/Unit/EnvServiceTest.php around lines 12 to 30, the test stub adds a
readFile method to Symfony's Filesystem which masks a production bug because
Filesystem does not provide readFile; refactor EnvService to depend on a small
FileReader abstraction and update tests to mock that interface: create a
FileReader interface with a read(string $path): string method, inject it into
EnvService (replace any $filesystem->readFile(...) calls with
$fileReader->read(...)), provide a concrete implementation that wraps
file_get_contents for production, and modify unit tests to mock the FileReader
instead of extending Filesystem so the test no longer hides the missing method.
Summary by CodeRabbit