Test review testing rules workflow - #11
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds CI review workflow and Cursor config/docs; introduces a reflection-based DI Container, inventory/SSH/Env services, ServerItem and ServerDTO, three Symfony console server commands, container-based command registration and version resolution improvements, a composer constraint change, and unit tests for Container, EnvService, and InventoryService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Console as Symfony Console
participant Container
participant AddCmd as ServerAddCommand
participant SSH as SSHService
participant Item as ServerItem
participant Inv as InventoryService
User->>Console: run "server:add <name> <host> [--port --user --key]"
Console->>Container: build(ServerAddCommand)
Container-->>Console: ServerAddCommand instance
Console->>AddCmd: execute()
AddCmd->>SSH: assertCanConnect(host, port, user, key?)
alt SSH success
SSH-->>AddCmd: success
AddCmd->>Item: create(name, ServerDTO)
Item->>Inv: set(collection="servers", key=name, value=...)
Inv-->>Item: persisted
AddCmd-->>Console: SUCCESS
else SSH failure
SSH-->>AddCmd: throws
AddCmd-->>Console: FAILURE (error)
end
sequenceDiagram
autonumber
participant App as Deployer
participant Container
participant Cmds as Server Commands
participant IV as InstalledVersions
participant Git as Process (git)
App->>Container: new Container()
App->>Container: build(ServerAdd/ServerList/ServerDelete)
Container-->>App: command instances
App->>Cmds: add() to application
note over App: startup version resolution
App->>IV: getPrettyVersion(package)?
alt InstalledVersions available
IV-->>App: version
else
App->>Git: git describe --tags --exact-match
alt exact tag found
Git-->>App: tag
else
App->>Git: git describe --tags --always
App->>Git: git rev-parse --abbrev-ref HEAD
App->>Git: git rev-parse --short HEAD
Git-->>App: tag/branch/commit info
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (39)
.cursor/commands/review.md (1)
1-1: Clarify CI scope: cannot see uncommitted changesIn CI, there are no “uncommitted” local changes to analyze. Reword to target the PR diff only.
-Analyze and meticulously catalog all the changes in this branch, including all the changes that haven't been committed yet. Report back on where the changes fall short of our development, architecture and testing rules. +Analyze and meticulously catalog all changes in this PR (diff between base and head). Report where they fall short of our development, architecture, and testing rules..cursor/cli.json (1)
3-5: Verify default policy semantics; explicitly allow required shell calls if neededIf empty "allow" implies default‑deny, the workflow’s gh calls will be blocked. Explicitly allow the minimal commands the workflow needs.
- "allow": [], + "allow": ["Shell(gh pr view)", "Shell(gh pr diff)", "Shell(gh pr review --comment)"], "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"].cursor/commands/refactor.md (1)
1-1: Make guidance actionableConsider adding concrete steps (checklist) and references to rules files to turn this into actionable refactor criteria.
.github/workflows/review.yml (2)
12-16: Bump timeout for reliability3 minutes can be tight for diff retrieval and review; suggest 5–10 minutes.
- timeout-minutes: 3 + timeout-minutes: 5
22-26: Avoid curl | bash without pinningPin the installer or verify checksum to reduce supply‑chain risk.
- curl https://cursor.com/install -fsS | bash + curl -fsSL https://cursor.com/install | bash + # TODO: Prefer a pinned version or checksum verification if supported by the installer. echo "$HOME/.cursor/bin" >> $GITHUB_PATH.cursor/rules/02-tests.mdc (1)
1-103: Resolve conflict with 00-main “Don’t worry about tests”This file mandates 60%+ coverage and detailed testing rules; 00-main says “Don’t worry about tests.” Unify the guidance.
app/Services/EnvService.php (3)
18-22: Avoid side effects in constructor; consider lazy load or explicit initLoading files in the constructor reduces testability and violates “stateless services” intent. Prefer lazy loading on first get() or an explicit initialize()/reload() method invoked by the caller.
68-72: Support alternate env filenames/locationsConsider checking .env.local and allowing an injected base path to better align with varied environments.
50-57: Import exceptions to match "use imports" ruleReplace \RuntimeException with an imported RuntimeException and import Throwable (replace \Throwable) for consistency — add
use RuntimeException;anduse Throwable;to app/Services/EnvService.php (throw in get() and catch in loadDotenvFile()).tests/Unit/EnvServiceTest.php (2)
104-113: Strengthen the “custom .env path” assertionThe stub ignores the filename, so this test does not prove the custom path is used. Track the last read filename in the stub and assert it.
- $service = new EnvService(mockFilesystem(true, 'CUSTOM_KEY=custom_value'), '/custom/.env'); + $fs = mockFilesystem(true, 'CUSTOM_KEY=custom_value'); + $service = new EnvService($fs, '/custom/.env');Also augment the stub:
class extends Filesystem { + public string $lastReadFile = ''; // ... public function readFile(string $filename): string { + $this->lastReadFile = $filename; if ($this->error) { throw new \RuntimeException('Permission denied'); } return $this->content; } }Then add an assertion after ACT:
expect($fs->lastReadFile)->toBe('/custom/.env');
12-13: Add minimal DocBlocks for helper functions (per project rules)Add short DocBlocks describing purpose, params, and returns.
+/** + * Create a mockable Symfony Filesystem instance with controllable behavior. + * + * @param bool $exists Whether the path should be reported as existing + * @param string $content Content returned by readFile + * @param bool $throwError Whether readFile should throw + * @return Filesystem + */ function mockFilesystem(bool $exists = true, string $content = '', bool $throwError = false): Filesystem { // ... } +/** + * Set or unset an environment variable in both $_ENV and process env. + * + * @param string $key + * @param string|null $value Null will unset + * @return void + */ function setEnv(string $key, ?string $value): void { // ... }Also applies to: 32-41
app/DTOs/ServerDTO.php (1)
10-10: Consider making the DTO finalDTOs are typically not extended. Making it final avoids accidental inheritance.
-class ServerDTO +final class ServerDTOtests/Unit/InventoryServiceTest.php (1)
36-39: Align stub method signatures with parent for LSP and clarityMatch Symfony Filesystem signatures to avoid surprises and improve static analysis (even if tests are excluded).
- public function exists($files): bool + public function exists(string|iterable $files): bool { return $this->fileExists; } - public function readFile(string $filename): string + public function readFile(string $filename): string { return $this->fileContent; } - public function mkdir($dirs, int $mode = 0777): void + public function mkdir(string|iterable $dirs, int $mode = 0777): void { if ($this->shouldThrowOnMkdir) { - throw new Exception('Permission denied'); + throw new \Exception('Permission denied'); } } - public function dumpFile(string $filename, $content): void + public function dumpFile(string $filename, string $content): void { if ($this->shouldThrowOnDump) { - throw new Exception('Write failed'); + throw new \Exception('Write failed'); } }Also applies to: 41-44, 46-51, 53-58
app/Console/Server/ServerListCommand.php (2)
14-21: Add DocBlocks for class and constructor (per app rules)Add minimal DocBlocks for the command and constructor-injected dependency.
+/** + * List all configured servers. + */ #[AsCommand(name: 'server:list', description: 'List all configured servers')] class ServerListCommand extends Command { - public function __construct( + /** + * @param ServerItem $servers Server inventory accessor + */ + public function __construct( private readonly ServerItem $servers, ) { parent::__construct(); }
23-65: Add DocBlock for execute() and consider injecting SymfonyStyle factory
- Add a DocBlock to execute() for params/return.
- To align with “no new in methods” guidance, consider injecting a SymfonyStyle factory/creator.
- protected function execute(InputInterface $input, OutputInterface $output): int + /** + * @param InputInterface $input + * @param OutputInterface $output + * @return int + */ + protected function execute(InputInterface $input, OutputInterface $output): int { $io = new SymfonyStyle($input, $output); // ... }Optional refactor: inject a callable like
fn(InputInterface, OutputInterface) => new SymfonyStyle(...)via constructor.app/Console/Server/ServerDeleteCommand.php (2)
15-22: Add DocBlocks for class and constructor (per app rules)+/** + * Delete a server entry from inventory. + */ #[AsCommand(name: 'server:delete', description: 'Delete a server entry from inventory')] class ServerDeleteCommand extends Command { - public function __construct( + /** + * @param ServerItem $servers Server inventory accessor + */ + public function __construct( private readonly ServerItem $servers, ) { parent::__construct(); }
24-54: DocBlock for configure()/execute() and optional SymfonyStyle factory
- Add DocBlocks for configure() and execute().
- Consider injecting a SymfonyStyle factory rather than
newin execute() to align with DI guidance.- protected function configure(): void + /** + * @return void + */ + protected function configure(): void { $this ->addArgument('name', InputArgument::REQUIRED, 'Server name to delete'); } - protected function execute(InputInterface $input, OutputInterface $output): int + /** + * @param InputInterface $input + * @param OutputInterface $output + * @return int + */ + protected function execute(InputInterface $input, OutputInterface $output): int { $io = new SymfonyStyle($input, $output); // ... }app/Services/SSHService.php (1)
72-79: Doc vs code mismatch on key preference order; unify and clarify messagingThe docblock says id_ed25519 before id_rsa, but the implementation tries id_rsa first. Align them (either order is fine; just be consistent). If keeping the current implementation, update docs and message:
- * 2) ~/.ssh/id_ed25519 - * 3) ~/.ssh/id_rsa + * 2) ~/.ssh/id_rsa + * 3) ~/.ssh/id_ed25519- // Default to id_rsa first, then try id_ed25519 + // Prefer id_rsa, then fallback to id_ed25519 $candidates[] = $home.'/.ssh/id_rsa'; $candidates[] = $home.'/.ssh/id_ed25519';- 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_rsa or ~/.ssh/id_ed25519.');Also applies to: 86-91, 29-34
app/Container.php (2)
83-92: Handle union types in constructor parametersCurrently, union types (e.g., Foo|Bar or Foo|null) are treated as non-class and will throw if no default is provided. Add support to pick the first non-builtin class from the union.
private function buildParameter(ReflectionParameter $parameter): mixed { $type = $parameter->getType(); - if (!$type instanceof ReflectionNamedType || $type->isBuiltin()) { - return $this->resolveNonClassParameter($parameter); - } - - return $this->resolveClassParameter($parameter, $type->getName()); + if ($type instanceof ReflectionNamedType) { + return $type->isBuiltin() + ? $this->resolveNonClassParameter($parameter) + : $this->resolveClassParameter($parameter, $type->getName()); + } + + if ($type instanceof \ReflectionUnionType) { + foreach ($type->getTypes() as $t) { + if ($t instanceof ReflectionNamedType && !$t->isBuiltin()) { + return $this->resolveClassParameter($parameter, $t->getName()); + } + } + return $this->resolveNonClassParameter($parameter); + } + + return $this->resolveNonClassParameter($parameter); }Also add the missing import:
use ReflectionClass; use ReflectionNamedType; +use ReflectionUnionType; use ReflectionParameter;
7-10: Prefer imported exceptions over fully qualified namesPer repo rules, use imports instead of FQCN for exceptions. This also improves consistency across files.
use ReflectionClass; use ReflectionNamedType; +use ReflectionUnionType; use ReflectionParameter; +use RuntimeException; +use Throwable; @@ - } catch (\RuntimeException $e) { + } catch (RuntimeException $e) { @@ - throw new \RuntimeException( + throw new RuntimeException( "Cannot resolve dependency [{$className}] for parameter [{$parameter->getName()}]", previous: $e ); @@ - throw new \RuntimeException( + throw new RuntimeException( "Cannot resolve parameter [{$parameter->getName()}] in class [{$parameter->getDeclaringClass()?->getName()}]" ); @@ - throw new \RuntimeException("Circular dependency detected: {$chain}"); + throw new RuntimeException("Circular dependency detected: {$chain}"); @@ - if (!class_exists($className)) { - throw new \RuntimeException("Class [{$className}] does not exist"); + if (!class_exists($className)) { + throw new RuntimeException("Class [{$className}] does not exist"); } @@ - if (!$reflector->isInstantiable()) { - throw new \RuntimeException("Class [{$className}] is not instantiable"); + if (!$reflector->isInstantiable()) { + throw new RuntimeException("Class [{$className}] is not instantiable"); }Also applies to: 100-197
app/Items/ServerItem.php (4)
7-9: Import exceptions and drop FQCN usageAlign with “use imports” rule; import exceptions and remove leading backslashes.
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 (!$this->exists($name)) { - throw new \RuntimeException("Server '{$name}' does not exist."); + if (!$this->exists($name)) { + throw new RuntimeException("Server '{$name}' does not exist."); } @@ - 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, 59-64, 71-82, 87-89
45-50: Tighten the return type docblock for list()Values have a known shape; document it for better tooling.
- * @return array<string, mixed> + * @return array<string, array{host:string, port:int, user:string, key:?string}>
47-50: Avoid reserved-word method name; add a non-breaking aliaslist is a language construct; having an alias improves readability and avoids edge cases.
public function list(): array { return $this->inventory->list('servers'); } + + /** + * Alias of list() to avoid reserved-word method name in call sites. + * + * @return array<string, array{host:string, port:int, user:string, key:?string}> + */ + public function all(): array + { + return $this->list(); + }
85-90: Add a docblock to assertValidName()Keep docblocks consistent for private validators.
- private function assertValidName(string $name): void + /** + * Validate the server name against allowed characters. + */ + private function assertValidName(string $name): void.cursor/rules/01-architecture.mdc (2)
16-18: Add a note to enforce timeouts for external processesYour rules encourage using Symfony Process; explicitly require timeouts to prevent hangs in CLI workflows.
- **Symfony Classes:** instead of native PHP functions for easier mocking during testing (eg. `Filesystem::`, `Process::`, etc.) + - **Symfony Classes:** instead of native PHP functions for easier mocking during testing (eg. `Filesystem::`, `Process::`, etc.) + - Always set reasonable timeouts on external processes (e.g., 3–10s) to avoid hanging commands.
89-95: Reinforce running quality gates on touched files in CIConsider adding an example CI snippet here to enforce Rector, Pint, and PHPStan on changed files, matching the guidance.
If you want, I can propose a GitHub Actions job that computes changed PHP files and runs these tools only on those paths.
app/Deployer.php (5)
21-34: Inject Container instead of instantiating itRules prohibit manual instantiation; accept Container via constructor to keep DI pure.
- private readonly 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(); }Follow-up: adjust the bootstrap to pass a Container instance, e.g., new Deployer(new Container()).
87-100: Add a docblock to registerCommands()Keep private APIs documented; helps readers and static analyzers.
- private function registerCommands(): void + /** + * Instantiate and register console commands via the container. + */ + private function registerCommands(): void
114-123: Import OutOfBoundsException and drop FQCNConsistency with import rules.
use Composer\InstalledVersions; +use OutOfBoundsException; @@ - } catch (\OutOfBoundsException) { + } catch (OutOfBoundsException) { // Package not found in installed.json, continue to fallbacks }
140-147: Use git to detect repository presence instead of is_dir()Rely on git itself and keep everything mockable via Process. Also aligns with your “prefer Symfony classes” rule.
- // Check if we're in a git repository - if (!is_dir($projectRoot.'/.git')) { - return null; - } + // Check if we're in a git repository + $checkRepo = new Process(['git', 'rev-parse', '--is-inside-work-tree'], $projectRoot); + $checkRepo->setTimeout(3.0)->run(); + if (!$checkRepo->isSuccessful() || trim($checkRepo->getOutput()) !== 'true') { + return null; + }
149-173: Set timeouts for external processesAvoid hangs during version detection; set short, explicit timeouts.
- $tagProcess = new Process(['git', 'describe', '--tags', '--exact-match'], $projectRoot); - $tagProcess->run(); + $tagProcess = new Process(['git', 'describe', '--tags', '--exact-match'], $projectRoot); + $tagProcess->setTimeout(5.0)->run(); @@ - $describeProcess = new Process(['git', 'describe', '--tags', '--always'], $projectRoot); - $describeProcess->run(); + $describeProcess = new Process(['git', 'describe', '--tags', '--always'], $projectRoot); + $describeProcess->setTimeout(5.0)->run(); @@ - $branchProcess = new Process(['git', 'rev-parse', '--abbrev-ref', 'HEAD'], $projectRoot); - $branchProcess->run(); - $commitProcess = new Process(['git', 'rev-parse', '--short', 'HEAD'], $projectRoot); - $commitProcess->run(); + $branchProcess = new Process(['git', 'rev-parse', '--abbrev-ref', 'HEAD'], $projectRoot); + $branchProcess->setTimeout(5.0)->run(); + $commitProcess = new Process(['git', 'rev-parse', '--short', 'HEAD'], $projectRoot); + $commitProcess->setTimeout(5.0)->run();app/Console/Server/ServerAddCommand.php (3)
18-27: Add a class-level docblockAttributes are great; add a minimal docblock per repo rules.
#[AsCommand(name: 'server:add', description: 'Add a server entry and verify SSH connectivity')] +/** + * Add a server entry after verifying SSH connectivity. + */ class ServerAddCommand extends Command
28-36: Add method docblock for configure()Keep public/protected methods documented.
- protected function configure(): void + /** + * Configure command arguments and options. + */ + protected function configure(): void
38-78: Import Throwable and document execute()Minor consistency update; also add a brief docblock.
use Symfony\Component\Console\Style\SymfonyStyle; +use Throwable; @@ - protected function execute(InputInterface $input, OutputInterface $output): int + /** + * Execute the command: verify SSH and persist the server entry. + */ + protected function execute(InputInterface $input, OutputInterface $output): int @@ - } catch (\Throwable $e) { + } catch (Throwable $e) { $io->error($e->getMessage()); return Command::FAILURE; }app/Services/InventoryService.php (4)
7-9: Import exceptions and avoid FQCN; also standardize exception types.Use imports for RuntimeException and Throwable (per guidelines) and reference them unqualified.
use Symfony\Component\Filesystem\Filesystem; +use Symfony\Component\Filesystem\Path; use Symfony\Component\Yaml\Yaml; +use RuntimeException; +use Throwable; @@ - private function throwKeyNotFound(string $collection, string $key): never + private function throwKeyNotFound(string $collection, string $key): never { - throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + throw new RuntimeException("Key '{$key}' not found in collection '{$collection}'."); } @@ - } catch (\Throwable $e) { - throw new \RuntimeException("Unable to create inventory directory: {$dir}", 0, $e); + } catch (Throwable $e) { + throw new RuntimeException("Unable to create inventory directory: {$dir}", 0, $e); } } @@ - } catch (\Throwable $e) { - throw new \RuntimeException("Failed to write inventory file at {$path}", 0, $e); + } catch (Throwable $e) { + throw new RuntimeException("Failed to write inventory file at {$path}", 0, $e); }Also applies to: 162-165, 220-224, 229-231
182-186: Use Path::join and handle getcwd() failure.Safer cross‑platform path building and explicit error on missing CWD.
- private function getInventoryPath(): string - { - return rtrim((string) getcwd(), '/').'/.deployer/inventory.yml'; - } + private function getInventoryPath(): string + { + $cwd = getcwd(); + if ($cwd === false) { + throw new RuntimeException('Cannot determine current working directory'); + } + return Path::join($cwd, '.deployer', 'inventory.yml'); + }
94-102: Avoid naming a method list().list is a language construct; while allowed as a method name, it’s confusing. Prefer a clearer name like listCollection() or getCollection().
213-232: Consider basic concurrency protection.Concurrent set()/delete() from multiple processes can drop updates (read‑modify‑write without locking). If this file is written from parallel CLI invocations, add a simple file lock (flock) or use a lock service around read/write.
tests/Unit/ContainerTest.php (1)
164-165: Avoid type-only assertions per testing rules.Replace toBeInstanceOf with a concrete behavior check.
- expect($result)->toBeInstanceOf(SimpleService::class); + expect($result->getName())->toBe('simple');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.cursor/cli.json(1 hunks).cursor/commands/refactor.md(1 hunks).cursor/commands/review.md(1 hunks).cursor/rules/00-main.mdc(2 hunks).cursor/rules/01-architecture.mdc(2 hunks).cursor/rules/02-code-quality.mdc(0 hunks).cursor/rules/02-tests.mdc(1 hunks).cursor/rules/03-tests.mdc(0 hunks).github/workflows/review.yml(1 hunks)app/Console/Server/ServerAddCommand.php(1 hunks)app/Console/Server/ServerDeleteCommand.php(1 hunks)app/Console/Server/ServerListCommand.php(1 hunks)app/Container.php(1 hunks)app/DTOs/ServerDTO.php(1 hunks)app/Deployer.php(5 hunks)app/Items/ServerItem.php(1 hunks)app/Services/EnvService.php(2 hunks)app/Services/InventoryService.php(1 hunks)app/Services/SSHService.php(1 hunks)composer.json(1 hunks)tests/Unit/ContainerTest.php(1 hunks)tests/Unit/EnvServiceTest.php(1 hunks)tests/Unit/InventoryServiceTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- .cursor/rules/02-code-quality.mdc
- .cursor/rules/03-tests.mdc
🧰 Additional context used
📓 Path-based instructions (6)
**/{composer.json,package.json}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/{composer.json,package.json}: Check composer.json and package.json for installed packages before starting any task
Plan implementation using features supported by the major versions specified in composer.json and package.json
Files:
composer.json
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/Console/Server/ServerListCommand.phpapp/Console/Server/ServerAddCommand.phpapp/Deployer.phpapp/Services/SSHService.phpapp/Console/Server/ServerDeleteCommand.phpapp/Services/InventoryService.phpapp/Services/EnvService.phpapp/DTOs/ServerDTO.phpapp/Container.phpapp/Items/ServerItem.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/Console/Server/ServerListCommand.phpapp/Console/Server/ServerAddCommand.phpapp/Deployer.phpapp/Services/SSHService.phpapp/Console/Server/ServerDeleteCommand.phptests/Unit/EnvServiceTest.phptests/Unit/ContainerTest.phpapp/Services/InventoryService.phpapp/Services/EnvService.phpapp/DTOs/ServerDTO.phpapp/Container.phptests/Unit/InventoryServiceTest.phpapp/Items/ServerItem.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.phptests/Unit/InventoryServiceTest.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.phptests/Unit/InventoryServiceTest.php
🧬 Code graph analysis (11)
app/Console/Server/ServerListCommand.php (3)
app/Items/ServerItem.php (3)
ServerItem(13-91)__construct(15-17)list(47-50)app/Services/InventoryService.php (2)
__construct(15-18)list(94-102)app/DTOs/ServerDTO.php (1)
__construct(12-18)
app/Console/Server/ServerAddCommand.php (3)
app/Items/ServerItem.php (3)
ServerItem(13-91)__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/Deployer.php (1)
app/Container.php (2)
Container(21-198)build(40-60)
app/Console/Server/ServerDeleteCommand.php (2)
app/Items/ServerItem.php (3)
ServerItem(13-91)__construct(15-17)delete(55-64)app/Services/InventoryService.php (2)
__construct(15-18)delete(110-126)
tests/Unit/EnvServiceTest.php (1)
app/Services/EnvService.php (3)
EnvService(13-89)__construct(18-22)get(33-57)
tests/Unit/ContainerTest.php (1)
app/Container.php (2)
Container(21-198)build(40-60)
app/Services/InventoryService.php (2)
app/Items/ServerItem.php (4)
__construct(15-17)list(47-50)delete(55-64)exists(37-40)tests/Unit/InventoryServiceTest.php (4)
exists(36-39)readFile(41-44)mkdir(46-51)dumpFile(53-58)
app/Services/EnvService.php (3)
app/Services/InventoryService.php (1)
__construct(15-18)tests/Unit/EnvServiceTest.php (3)
__construct(15-17)exists(18-21)readFile(22-28)tests/Unit/InventoryServiceTest.php (2)
exists(36-39)readFile(41-44)
app/DTOs/ServerDTO.php (5)
app/Services/InventoryService.php (1)
__construct(15-18)app/Console/Server/ServerAddCommand.php (1)
__construct(21-26)app/Console/Server/ServerDeleteCommand.php (1)
__construct(18-22)app/Console/Server/ServerListCommand.php (1)
__construct(17-21)app/Items/ServerItem.php (1)
__construct(15-17)
tests/Unit/InventoryServiceTest.php (1)
app/Services/InventoryService.php (8)
InventoryService(13-233)getAll(75-78)list(94-102)has(83-87)get(60-68)set(30-40)setCollection(47-52)delete(110-126)
app/Items/ServerItem.php (5)
app/Services/InventoryService.php (6)
InventoryService(13-233)__construct(15-18)set(30-40)has(83-87)list(94-102)delete(110-126)app/DTOs/ServerDTO.php (3)
ServerDTO(10-34)__construct(12-18)toArray(25-33)app/Console/Server/ServerAddCommand.php (1)
__construct(21-26)app/Console/Server/ServerDeleteCommand.php (1)
__construct(18-22)app/Console/Server/ServerListCommand.php (1)
__construct(17-21)
🪛 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)
app/Container.php
83-92: Avoid unused private methods such as 'buildParameter'. (undefined)
(UnusedPrivateMethod)
tests/Unit/InventoryServiceTest.php
36-36: Avoid unused parameters such as '$files'. (undefined)
(UnusedFormalParameter)
41-41: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
46-46: Avoid unused parameters such as '$dirs'. (undefined)
(UnusedFormalParameter)
46-46: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
53-53: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
53-53: Avoid unused parameters such as '$content'. (undefined)
(UnusedFormalParameter)
🪛 GitHub Actions: Rector
tests/Unit/EnvServiceTest.php
[error] 12-12: Rector dry-run would modify constructor property visibility to 'readonly'. Applied rule: ReadOnlyPropertyRector. Process exited with code 2.
🔇 Additional comments (9)
composer.json (1)
24-24: LGTM: constrain to symfony/filesystem ^7.1This aligns with usage of Filesystem::readFile() added in 7.1. Please confirm no consumers require 7.0 support.
.github/workflows/review.yml (1)
27-59: Confirm permissions/policy allow PR commentingThe agent will run gh pr view/diff/review; ensure repo permissions and .cursor/cli.json policy allow these.
app/Services/EnvService.php (1)
38-48: Behavior LGTMENV takes precedence over .env; empty strings ignored; return type enforced. Good.
To ensure DI compliance elsewhere, scan for manual service instantiation:
app/Services/SSHService.php (1)
48-52: Avoid new inside methods; inject factories (project rule)Per app rules, dependencies should be created via DI, not with
newin methods. Inject factories/callables forSSH2creation and key loading.Sketch of a compliant refactor:
final class SSHService { /** * @param callable(string,int): SSH2 $sshFactory * @param callable(string): PrivateKey $keyLoader */ public function __construct( private readonly \Closure $sshFactory, private readonly \Closure $keyLoader, ) {} public function assertCanConnect(string $host, int $port, string $username, ?string $privateKeyPath = null): void { // ... $key = ($this->keyLoader)($keyContents); // loads PrivateKey $ssh = ($this->sshFactory)($host, $port); // creates SSH2 $loggedIn = $ssh->login($username, $key); // ... } }Container wiring example:
// In your container config/builder $container->set(SSHService::class, new SSHService( fn (string $h, int $p) => new SSH2($h, $p), fn (string $k): PrivateKey => PublicKeyLoader::load($k), ));Also applies to: 55-58
app/Container.php (1)
75-79: PHPMD “unused private method” is a false positive
buildParameteris referenced via array_map callback and is used. No action needed; consider suppressing PHPMD for this occurrence if it keeps flagging.If it keeps surfacing in CI, I can add a targeted @SuppressWarnings or rewrite the map to a foreach to appease the tool. Want a patch?
app/Services/InventoryService.php (2)
1-4: Process hygiene: Rector, Pint, PHPStan.Please confirm Rector and Pint have been run and that PHPStan passes for this file as required by the guidelines.
200-205: Confirm Symfony Filesystem::readFile() availability (introduced in Symfony 7.1)Filesystem::readFile() was added in Symfony 7.1; if the project uses an older Symfony version this call will fatal — replace with file_get_contents() + error handling if needed:
- $raw = $this->filesystem->readFile($path); + $raw = @file_get_contents($path); + if ($raw === false) { + throw new RuntimeException("Failed to read inventory file at {$path}"); + }tests/Unit/ContainerTest.php (2)
134-138: Good: circular dependency surfaces as a dependency resolution error.Assertion aligns with Container’s cascade to “Cannot resolve dependency …”.
If Container’s message changes, loosen the match to just 'Circular' or 'Cannot resolve'.
101-116: Solid AAA usage and Pest style.Inline fixtures and AAA headers are clear; new instances check is good.
| **✔️ Don't worry about tests:** Write or run tests ONLY if specifically instructed | ||
|
|
There was a problem hiding this comment.
Contradiction: tests discouraged here but required elsewhere
“Don’t worry about tests” conflicts with tests/coverage requirements in 02-tests.mdc and composer scripts. Align these rules.
🤖 Prompt for AI Agents
In .cursor/rules/00-main.mdc around lines 35-36, the single-line rule "Don't
worry about tests: Write or run tests ONLY if specifically instructed"
contradicts the tests/coverage requirements in 02-tests.mdc and the composer
scripts; change this sentence to explicitly align with the project-wide policy
by either (A) rewording it to: "Do not add or modify tests only when a task
explicitly states tests are not required; otherwise follow the tests/coverage
rules in 02-tests.mdc and the composer scripts," or (B) remove the line and add
a short pointer sentence that directs readers to follow 02-tests.mdc for test
requirements so the guidance is consistent across the repository.
| /** | ||
| * Load and parse .env file if it exists. | ||
| */ | ||
| private function loadDotenvFile(): void | ||
| { | ||
| $envPath = rtrim((string) getcwd(), '/') . '/.env'; | ||
|
|
||
| if (!$this->filesystem->exists($envPath)) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| $content = $this->filesystem->readFile($envPath); | ||
| $dotenv = new Dotenv(); | ||
| $parsed = $dotenv->parse($content, $envPath); | ||
|
|
||
| foreach ($parsed as $k => $v) { | ||
| if (is_string($k) && is_string($v)) { | ||
| $this->dotenv[$k] = $v; | ||
| } | ||
| } | ||
| } catch (\Throwable) { | ||
| // Silently ignore file reading errors | ||
| $this->dotenv = []; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inject Dotenv; don’t instantiate services inside methods
Per rules, avoid new inside services. Inject Dotenv via constructor and use it here. Also import exceptions instead of FQCN.
- try {
- $content = $this->filesystem->readFile($envPath);
- $dotenv = new Dotenv();
- $parsed = $dotenv->parse($content, $envPath);
+ try {
+ $content = $this->filesystem->readFile($envPath);
+ $parsed = $this->dotenvParser->parse($content, $envPath);
@@
- } catch (\Throwable) {
+ } catch (Throwable) {
// Silently ignore file reading errors
$this->dotenv = [];
}Apply outside this range as well:
use Symfony\Component\Dotenv\Dotenv;
use Symfony\Component\Filesystem\Filesystem;
+use RuntimeException;
+use Throwable;
@@
- public function __construct(
- private readonly Filesystem $filesystem
- ) {
+ public function __construct(
+ private readonly Filesystem $filesystem,
+ private readonly Dotenv $dotenvParser,
+ private readonly ?string $envPath = null,
+ ) {
- $this->loadDotenvFile();
+ $this->loadDotenvFile();
}Optionally support an override path:
- $envPath = rtrim((string) getcwd(), '/') . '/.env';
+ $envPath = $this->envPath ?? (rtrim((string) getcwd(), '/') . '/.env');| ['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.
Fix invalid expectation for interfaces.
Interfaces exist but are not instantiable. Update expected error to match Container’s message.
- [TestInterface::class, 'does not exist'], // Interfaces don't exist as classes
+ [TestInterface::class, 'not instantiable'], // Interfaces are 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'], // Interfaces are 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 currently asserts 'does not exist' but interfaces do exist
and should produce the container's "not instantiable" message; update the
expected message for TestInterface::class to 'not instantiable' so the test
matches the Container's actual error for interfaces.
| 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) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Satisfy Rector: make ctor-promoted props readonly
Rector flagged this. Mark the anonymous class constructor-promoted props as readonly.
- 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)📝 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.
| 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) |
🧰 Tools
🪛 PHPMD (2.15.0)
15-15: Avoid unused parameters such as '$error'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In tests/Unit/EnvServiceTest.php around line 15, the anonymous class
constructor-promoted properties are not marked readonly which Rector flagged;
update the constructor signature to mark each promoted property readonly (e.g.
change "private bool $exists, private string $content, private bool $error" to
use "private readonly" for each promoted property) so the props are immutable
and satisfy Rector.
| $service = new EnvService(mockFilesystem(!empty($fileContent), $fileContent, $fileError), '.env'); | ||
|
|
There was a problem hiding this comment.
Fix constructor mismatch: tests pass an extra EnvService argument
EnvService currently accepts only Filesystem (see app/Services/EnvService.php). These tests construct EnvService with a second argument (custom path), which will cause a fatal error. Either:
- Update EnvService to accept an optional .env path, or
- Remove the extra constructor argument from tests and drop the “custom path” test.
Preferred: add optional path support to EnvService to keep the test. Example change outside this file:
// app/Services/EnvService.php (sketch)
public function __construct(
private readonly Filesystem $filesystem,
private readonly ?string $envPath = null,
) {
$this->loadDotenvFile();
}
private function loadDotenvFile(): void
{
$envPath = $this->envPath ?? rtrim((string) getcwd(), '/') . '/.env';
if (!$this->filesystem->exists($envPath)) {
return;
}
// ...
}Also applies to: 86-89, 106-107
🤖 Prompt for AI Agents
In app/Services/EnvService.php (affects tests referencing lines 59, 86-89,
106-107), the constructor currently only accepts a Filesystem but tests pass a
second argument for a custom .env path; update the constructor to accept an
optional string envPath (nullable) as a second parameter, store it on the
instance, and change the dotenv loading logic to compute the path as the
provided envPath or default to rtrim(getcwd(), '/') . '/.env'; use the
Filesystem->exists($envPath) check and continue loading from that resolved path
so existing behavior is preserved when envPath is null.
| // ASSERT | ||
| // No exception should be thrown - test passes if no error | ||
| expect(true)->toBeTrue(); | ||
| }); |
There was a problem hiding this comment.
Replace “expect(true)->toBeTrue()” with behavioral assertions (forbidden pattern)
Project rules forbid generic assertions. Assert real behavior: that write/delete occurred and content changed. Enhance the stub to capture dump calls and assert against it.
Stub improvements:
class FilesystemStub extends Filesystem
{
+ public bool $dumpCalled = false;
+ public ?string $lastDumpFilename = null;
+ public ?string $lastDumpContent = null;
// ...
public function dumpFile(string $filename, $content): void
{
if ($this->shouldThrowOnDump) {
throw new Exception('Write failed');
}
+ $this->dumpCalled = true;
+ $this->lastDumpFilename = $filename;
+ $this->lastDumpContent = (string) $content;
}
}Example assertion change (apply similarly in other tests):
-// ASSERT
-// No exception should be thrown - test passes if no error
-expect(true)->toBeTrue();
+// ASSERT
+expect($this->filesystem->dumpCalled)->toBeTrue();
+expect($this->filesystem->lastDumpContent)->toContain('servers:', 'web1:', 'host: example.com');Also applies to: 219-221, 232-234, 244-246, 262-266, 277-279
🤖 Prompt for AI Agents
In tests/Unit/InventoryServiceTest.php around lines 205-208 (and similarly for
219-221, 232-234, 244-246, 262-266, 277-279), replace the forbidden generic
assertion expect(true)->toBeTrue() with concrete behavioral assertions: enhance
the stub for the storage/dump dependency to capture the calls and payloads, then
assert that the expected write or delete method was invoked with the correct
arguments and that the repository/state reflects the content change (e.g., item
added/removed or updated), and when applicable assert the number of calls and
the exact dumped content matches the expected JSON/array structure; update each
test block to assert these behaviors instead of a noop success assertion.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review Summary
🔍 Overall Assessment
This PR introduces significant architectural changes including a DI container, new services (Env, Inventory, SSH), server management commands, and comprehensive testing. The code demonstrates good understanding of PHP best practices but has several areas that violate our development rules.
📋 Changes Analyzed
- New DI Container: Reflection-based autowiring with circular dependency detection
- Services: EnvService (.env handling), InventoryService (YAML CRUD), SSHService (connectivity checks)
- Commands: server:add, server:delete, server:list with proper Symfony console integration
- DTOs/Items: ServerDTO and ServerItem with validation
- Tests: Unit tests for Container, EnvService, InventoryService with good coverage
- Config: Cursor config, rules documentation, GitHub Actions workflow
⚠️ Rule Violations Found
🚨 Critical Issues
- Missing DocBlocks: Several classes lack proper class-level docblocks
- Constructor Parameters: Some constructors have inconsistent trailing commas
- Comment Formatting: Section headers don't consistently use the mandated format
✨ Improvements Needed
- Code Organization: Some files could benefit from better alphabetical organization of methods
- Type Safety: A few places could use more specific return types
- Error Handling: Some exception messages could be more specific
✅ Strengths
- Excellent use of strict types and modern PHP 8.x features
- Proper dependency injection throughout
- Good separation of concerns between Commands/Services/Items
- Comprehensive test coverage with proper mocking
- Consistent use of Symfony components instead of native PHP functions
🎯 Recommendations
- Run quality gates:
composer pallto fix style and static analysis issues - Add missing class docblocks following the minimalist description pattern
- Standardize section header comment formatting
- Consider alphabetical method organization in service classes
The architecture is solid and follows our DI principles well. Most issues are minor formatting/documentation concerns that can be quickly addressed.
There was a problem hiding this comment.
📋 Detailed Code Review
This PR introduces significant architectural improvements with proper DI container, services, and testing. Here are specific areas for improvement:
🚨 Critical Issues
- Missing class-level docblocks on several classes
- Inconsistent constructor parameter formatting
- Comment format violations in rules documentation
✨ Strengths
- Excellent dependency injection implementation
- Comprehensive test coverage
- Proper separation of concerns
- Modern PHP 8.x feature usage
| * $service = $container->build(MyService::class); | ||
| * ``` | ||
| */ | ||
| class Container |
There was a problem hiding this comment.
🚨 Missing class-level docblock. According to our architecture rules, all classes must have minimalist descriptions explaining their purpose.
| { | ||
| public function __construct( | ||
| private readonly Filesystem $filesystem, | ||
| ) { |
There was a problem hiding this comment.
⚡ Missing trailing comma after $filesystem parameter for consistent formatting.
1 participant
Summary by CodeRabbit