From a96ebb98f71536b7c389435d09c522dc25929306 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Mon, 22 Sep 2025 22:17:24 +0300 Subject: [PATCH 1/7] wip --- .cursor/cli.json | 6 + .cursor/commands/review-testing-rules.md | 1 + .cursor/rules/01-architecture.mdc | 37 ++- .cursor/rules/03-tests.mdc | 215 ++++----------- .github/workflows/testing-rules.yml | 58 ++++ app/Console/Server/ServerAddCommand.php | 79 ++++++ app/Console/Server/ServerDeleteCommand.php | 55 ++++ app/Console/Server/ServerListCommand.php | 66 +++++ app/Container.php | 154 +++++++++++ app/DTOs/ServerDTO.php | 34 +++ app/Deployer.php | 62 ++++- app/Items/ServerItem.php | 91 ++++++ app/Services/EnvService.php | 36 ++- app/Services/InventoryService.php | 182 ++++++++++++ app/Services/SSHService.php | 118 ++++++++ composer.json | 2 +- composer.lock | 28 +- tests/Unit/ContainerTest.php | 166 +++++++++++ tests/Unit/EnvServiceTest.php | 114 ++++++++ tests/Unit/InventoryServiceTest.php | 304 +++++++++++++++++++++ 20 files changed, 1591 insertions(+), 217 deletions(-) create mode 100644 .cursor/cli.json create mode 100644 .cursor/commands/review-testing-rules.md create mode 100644 .github/workflows/testing-rules.yml create mode 100644 app/Console/Server/ServerAddCommand.php create mode 100644 app/Console/Server/ServerDeleteCommand.php create mode 100644 app/Console/Server/ServerListCommand.php create mode 100644 app/Container.php create mode 100644 app/DTOs/ServerDTO.php create mode 100644 app/Items/ServerItem.php create mode 100644 app/Services/InventoryService.php create mode 100644 app/Services/SSHService.php create mode 100644 tests/Unit/ContainerTest.php create mode 100644 tests/Unit/EnvServiceTest.php create mode 100644 tests/Unit/InventoryServiceTest.php diff --git a/.cursor/cli.json b/.cursor/cli.json new file mode 100644 index 00000000..aa2a1431 --- /dev/null +++ b/.cursor/cli.json @@ -0,0 +1,6 @@ +{ + "permissions": { + "allow": [], + "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"] + } +} diff --git a/.cursor/commands/review-testing-rules.md b/.cursor/commands/review-testing-rules.md new file mode 100644 index 00000000..2205e791 --- /dev/null +++ b/.cursor/commands/review-testing-rules.md @@ -0,0 +1 @@ +Analyze the changes in this Git working tree and report back on tests that fall short of our testing rules. diff --git a/.cursor/rules/01-architecture.mdc b/.cursor/rules/01-architecture.mdc index eeb85f4e..457d2a7b 100644 --- a/.cursor/rules/01-architecture.mdc +++ b/.cursor/rules/01-architecture.mdc @@ -4,12 +4,23 @@ globs: app/**/*.php,templates/**/*.yaml ### Architecture (MANDATORY) -- **PHP:** PSR-12, strict types, PHP 8.x features (unions, match, attributes, readonly); -- **Imports:** Always add `use` statements instead of fully qualified class names; -- **Type Safety:** All methods must have explicit return types with proper generics (e.g., `Collection`). +- **PHP:** PSR-12, strict types, PHP 8.x features (unions, match, attributes, readonly) +- **Imports:** Always add `use` statements instead of fully qualified class names +- **Type Safety:** All methods must have explicit return types with proper generics (e.g., `Collection`) **🚨 Architecture rules are IMMUTABLE - fix violating code, not the architecture rules** +### 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 (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 + ### Command Layer - Commands handle user interaction (input/output) and orchestrate Services @@ -26,31 +37,17 @@ globs: app/**/*.php,templates/**/*.yaml - Services handle core business logic, external API calls, file operations - Complex orchestration shared by multiple Commands should be extracted to dedicated Services -### Shared Utilities - -- Common path resolution, configuration loading goes in dedicated utility classes -- Utility classes are static or singleton patterns for simple operations -- No business logic or state in utilities - pure functions only - ### Console I/O Rules - Only Commands perform console input/output operations -- Services return exceptions or structured data for Commands to handle - Use SymfonyStyle consistently for all user-facing output +- Services return exceptions or structured data for Commands to handle - Validation errors and business exceptions bubble up to Commands for display -### Dependency Injection (Mandatory) - -- ALL dependencies MUST be injected through constructors - NO manual instantiation -- Commands receive Services via constructor injection -- Services receive other Services/utilities via constructor injection -- Use ServiceContainer or dependency injection container for all object creation -- NEVER use `new ClassName()` inside methods - always inject dependencies -- Exceptions: Only value objects, DTOs, and pure data structures can be manually instantiated - ### Dependency Rules - Commands depend on Services - Services depend on other Services or utilities -- No circular dependencies allowed - All dependencies declared in constructor signatures + +**IMPORTANT:** No circular dependencies allowed. diff --git a/.cursor/rules/03-tests.mdc b/.cursor/rules/03-tests.mdc index a5747e8d..dff36d50 100644 --- a/.cursor/rules/03-tests.mdc +++ b/.cursor/rules/03-tests.mdc @@ -10,212 +10,93 @@ globs: app/**/*.php,tests/**/*.php ### Running Tests -- `composer pest` run the entire test suite in parallel, with coverage -- `vendor/bin/pest $TEST_FILE` run a specific test file +- `composer pest` - run entire test suite in parallel, with coverage +- `vendor/bin/pest $TEST_FILE` - run specific test file -### Test Minimalism Rules +### Test Minimalism -**Write only essential tests that would break if business logic fails:** +**Target:** Keep test files under 1.8x the size of source code they test. -- **Core Business Logic Only:** Test critical paths, skip framework testing -- **Minimal Test Data:** Use simplest possible setup, avoid complex scenarios -- **Standard Assertions:** Prefer built-in `expect()` over custom assertions -- **Essential Edge Cases:** Only test failure modes that actually matter -- **No Performance Tests:** Unless performance is the primary concern +**Rules:** -### Static Analysis in Tests +- Test core business logic only, skip framework testing +- Use dataset-driven testing: `->with([])` for multiple scenarios +- Eliminate test overlap: no two tests covering same functionality +- Consolidate assertions: `expect($x)->toBe(1)->and($y)->toBe(2)` +- Mock external dependencies only +- No performance tests unless performance is the primary concern +- Don't sacrifice readability for ratio targets -- Ignore PHPStan issues in tests. -- Avoid excessive phpdoc just to appease types; add only when necessary. +**Don't consolidate when:** -### AAA Pattern Requirements (MANDATORY) +- Different public methods +- Exception vs normal flow tests +- Different setup requirements +- Distinct business logic -**All tests MUST follow the AAA pattern with explicit section headers:** +### AAA Pattern (MANDATORY) ```php it('does something specific', function () { // ARRANGE - $testData = ['key' => 'value']; - $mockService = mock(SomeService::class); + $service = new Service(mock(Dependency::class)); // ACT - $result = $this->service->performAction($testData); + $result = $service->performAction(); // ASSERT - expect($result)->toBe($expectedValue); - $mockService->shouldHaveReceived('method'); + expect($result)->toBe('expected'); // CLEANUP (when needed) $this->resetTimeState(); - $this->cleanupTempFiles(); + unlink($tempFile); }); ``` -**Required Section Headers:** +**Exception tests:** Use `// ACT & ASSERT` when act triggers assertion. -- `// ARRANGE` - Setup test data, mocks, and dependencies -- `// ACT` - Execute the code under test (single action) -- `// ASSERT` - Verify expected outcomes and behaviors -- `// CLEANUP` - Reset state when necessary (time state, temp files, etc.) +**Organization:** Use `describe()` blocks, `beforeEach()` setup, extract helpers/traits for DRY tests. -**Exception Pattern:** +### Testing Patterns -- `// ACT & ASSERT` - For exception tests where the act triggers the assertion - -### Organizing Tests & Helpers - -**DRY Principle:** Tests should be as DRY and streamlined as the code they're testing. - -**Best Practices:** - -- Logical grouping with `describe()` blocks -- Extract repeated mocking into reusable helper methods -- Create test traits for shared behavior across test classes -- Use `beforeEach()` blocks for common setup within test groups -- Use proper cleanup in tests (temp files, reset state) for test isolation -- Build helper functions for creating test configurations and mock data - -#### Test Helpers - -- Evolution-friendly, loosely coupled -- Support interface changes - -**Helper Examples:** +**❌ FORBIDDEN:** ```php -// Test trait for common mocking -trait MocksExternalServices -{ - protected function mockSuccessfulProcess(): void - { - // Mock external process calls - $this->processRunner = mock(ProcessRunner::class); - $this->processRunner->shouldReceive('run') - ->andReturn(['output' => 'Success', 'exitCode' => 0]); - } -} - -// Reusable test data builders -function createCommandOptions(array $overrides = []): array -{ - return array_merge([ - 'composer' => true, - 'npm' => false, - 'force' => false, - ], $overrides); -} - -// beforeEach for common setup -describe('package installation', function () { - beforeEach(function () { - $this->mockSuccessfulProcess(); - $this->baseOptions = createCommandOptions(); - }); -}); -``` - -**Benefits:** - -- Reduces test maintenance burden -- Ensures consistent mocking patterns -- Makes tests more readable and focused -- Easier to update when dependencies change - -### FORBIDDEN Patterns (Auto-Reject) - -```php -expect($x)->toBeInstanceOf(Class::class); // Type-only -expect($x)->toBeArray(); // Generic +expect($x)->toBeInstanceOf(Class::class); // Type-only testing +expect($x)->toBeArray(); // Generic assertions expect($x)->not->toBeNull(); // Meaningless -expect($x)->toBeTrue(); // No context -expect(true)->toBeTrue(); // This is literally meaningless -expect($object->property())->toBeInstanceOf(); // Property type testing -sleep(...); // Use proper time mocking instead +expect(true)->toBeTrue(); // Literally meaningless +sleep(...); // Use time mocking ``` -### REQUIRED Patterns +**✅ REQUIRED:** ```php -// Test specific values and behavior -expect($config->getValue('host'))->toBe('example.com') - ->and($config->getValue('port'))->toBe(22); - -// Test with datasets -it('validates server hostnames', function (string $hostname, bool $valid) { - expect($this->validator->isValidHostname($hostname))->toBe($valid); -})->with([ - ['server.example.com', true], - ['invalid_hostname!', false], -]); - -// Mock only external dependencies -$processRunner = mock(ProcessRunner::class); -$processRunner->shouldReceive('run')->with('ls *')->andReturn('file1.txt'); - -$httpClient = mock(HttpClient::class); -$httpClient->shouldReceive('get')->with('api/endpoint')->andReturn(['status' => 'ok']); +expect($config->getValue('host'))->toBe('example.com'); +expect($this->validator->isValid($input))->toBe($expected); +$mock->shouldReceive('method')->with('param')->andReturn('result'); ``` -### Unit Test Isolation - -**Core Principle:** True unit tests must be isolated from external dependencies. +### Test Types -**Unit tests should:** +**Unit Tests:** -- Test single units of code in isolation -- Use mocks/fakes for all external dependencies -- Run without external dependencies +- Mock all external dependencies (filesystem, HTTP, processes) +- Test single units in isolation - Complete in milliseconds -**Unit tests MUST NOT:** +**Integration Tests:** -- Use real file system, network calls, or shell commands -- Test service integrations with external systems -- Make actual HTTP requests or process executions -- Depend on external services, APIs, or system processes +- Real file operations and external processes +- CLI commands and full workflows -**Integration vs Unit:** Use integration tests for file system operations and external processes, unit tests (with mocks) for pure business logic. +**Layer Strategy:** -### Layer Testing Strategy +- CLI Commands → Integration tests +- Business Services → Unit tests (mocked dependencies) +- Utilities/Helpers → Unit tests -- **CLI Commands:** Integration tests, mock external processes and services -- **Business Services:** Unit tests, mock all external calls and file operations -- **Utilities/Helpers:** Unit tests with isolated scenarios +### Static Analysis -```php -// ✅ Service unit test (mocked dependencies) -$this->fileSystem->shouldReceive('exists')->andReturn(true); -$this->processRunner->shouldReceive('execute')->andReturn(['output' => 'success']); - -// ✅ Command integration test (real file operations) -$testFile = '/tmp/test-config.yml'; -file_put_contents($testFile, 'host: example.com'); -$result = $this->command->execute(['--config' => $testFile]); -expect($result->exitCode)->toBe(0); -unlink($testFile); // cleanup -``` - -### Performance Testing - -**When testing CLI operations:** - -- Monitor execution time for file operations and external processes -- Test with realistic server counts and configuration sizes to expose performance issues -- Verify efficient processing of multiple servers or long-running deployment operations - -```php -// ✅ CLI performance monitoring pattern -it('processes server provisioning efficiently', function () { - // ARRANGE - $serverConfigs = createTestServerConfigs(5); - - // ACT - $startTime = microtime(true); - $result = $this->provisioningService->processServers($serverConfigs); - $executionTime = microtime(true) - $startTime; - - // ASSERT - expect($result->getSuccessCount())->toBe(5) - ->and($executionTime)->toBeLessThan(2.0); // Should complete provisioning in under 2 seconds -}); -``` +- Ignore PHPStan issues in tests - focus on test functionality over compliance +- Avoid excessive phpdoc just to appease types diff --git a/.github/workflows/testing-rules.yml b/.github/workflows/testing-rules.yml new file mode 100644 index 00000000..483df391 --- /dev/null +++ b/.github/workflows/testing-rules.yml @@ -0,0 +1,58 @@ +name: Testing Rules + +permissions: + contents: read + pull-requests: write + +on: + pull_request: + +jobs: + testing-rules: + runs-on: ubuntu-latest + timeout-minutes: 3 + concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install Cursor CLI + run: | + curl https://cursor.com/install -fsS | bash + echo "$HOME/.cursor/bin" >> $GITHUB_PATH + + - name: Testing rules review + env: + CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} + GH_TOKEN: ${{ github.token }} + run: | + cursor-agent --force --model "claude-4-sonnet" --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. + + Context: + - Repo: ${{ github.repository }} + - PR Number: ${{ github.event.pull_request.number }} + - PR Head SHA: ${{ github.event.pull_request.head.sha }} + - PR Base SHA: ${{ github.event.pull_request.base.sha }} + + Review the current PR diff and... + 1. report back on tests that fall short of our testing rules + 2. identify gaps in our tests or missing tests + 3. leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end + + Procedure: + - Get existing comments: gh pr view --json comments + - Get diff: gh pr diff + - If a previously reported issue appears fixed by nearby changes, reply: ✅ This issue appears to be resolved by the recent changes + - Avoid duplicates: skip if similar feedback already exists on or near the same lines + + Commenting rules: + - Natural tone, specific and actionable; do not mention automated or high-confidence + - Use emojis: 🚨 Critical 🔒 Security ⚡ Performance ⚠️ Logic ✅ Resolved ✨ Improvement + + Submission: + - Submit one review containing inline comments plus a concise summary + - Use only: gh pr review --comment + - Do not use: gh pr review --approve or --request-changes" diff --git a/app/Console/Server/ServerAddCommand.php b/app/Console/Server/ServerAddCommand.php new file mode 100644 index 00000000..bb8a5073 --- /dev/null +++ b/app/Console/Server/ServerAddCommand.php @@ -0,0 +1,79 @@ +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)'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + + /** @var string $name */ + $name = $input->getArgument('name'); + + /** @var string $host */ + $host = $input->getArgument('host'); + + /** @var string|int $portOption */ + $portOption = $input->getOption('port'); + $port = (int) $portOption; + + /** @var string $user */ + $user = $input->getOption('user'); + + /** @var string|null $keyPath */ + $keyPath = $input->getOption('key'); + $keyPath = $keyPath !== null && $keyPath !== '' ? $keyPath : null; + + try { + $io->section('Verifying SSH connectivity'); + $io->writeln(sprintf('Connecting to %s@%s:%d', $user, $host, $port)); + + $this->sshService->assertCanConnect($host, $port, $user, $keyPath); + + $io->success('SSH connectivity verified.'); + + $dto = new ServerDTO(host: $host, port: $port, user: $user, key: $keyPath); + $this->servers->create($name, $dto); + + $io->success(sprintf("Server '%s' saved to .deployer/inventory.yml", $name)); + + return Command::SUCCESS; + } catch (\Throwable $e) { + $io->error($e->getMessage()); + + return Command::FAILURE; + } + } +} diff --git a/app/Console/Server/ServerDeleteCommand.php b/app/Console/Server/ServerDeleteCommand.php new file mode 100644 index 00000000..a43ffbce --- /dev/null +++ b/app/Console/Server/ServerDeleteCommand.php @@ -0,0 +1,55 @@ +addArgument('name', InputArgument::REQUIRED, 'Server name to delete'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + + /** @var string $name */ + $name = $input->getArgument('name'); + + try { + // Confirm deletion + if (!$io->confirm(sprintf("Are you sure you want to delete server '%s'?", $name))) { + $io->info('Deletion cancelled.'); + return Command::SUCCESS; + } + + $this->servers->delete($name); + + $io->success(sprintf("Server '%s' deleted from .deployer/inventory.yml", $name)); + + return Command::SUCCESS; + } catch (\Throwable $e) { + $io->error($e->getMessage()); + + return Command::FAILURE; + } + } +} diff --git a/app/Console/Server/ServerListCommand.php b/app/Console/Server/ServerListCommand.php new file mode 100644 index 00000000..17f5fca3 --- /dev/null +++ b/app/Console/Server/ServerListCommand.php @@ -0,0 +1,66 @@ +servers->list(); + + if (empty($servers)) { + $io->info('No servers configured.'); + return Command::SUCCESS; + } + + $io->section('Configured Servers'); + + // Prepare table data + $tableData = []; + foreach ($servers as $name => $server) { + if (!is_array($server)) { + continue; + } + + $host = $server['host'] ?? 'N/A'; + $port = $server['port'] ?? 'N/A'; + $user = $server['user'] ?? 'N/A'; + $key = !empty($server['key']) ? 'Yes' : 'No'; + + $tableData[] = [$name, $host, $port, $user, $key]; + } + + $io->table( + ['Name', 'Host', 'Port', 'User', 'Private Key'], + $tableData + ); + + $io->info(sprintf('Total servers: %d', count($tableData))); + + return Command::SUCCESS; + } catch (\Throwable $e) { + $io->error($e->getMessage()); + + return Command::FAILURE; + } + } +} diff --git a/app/Container.php b/app/Container.php new file mode 100644 index 00000000..ca74a36d --- /dev/null +++ b/app/Container.php @@ -0,0 +1,154 @@ +build(MyService::class); + * ``` + */ +class Container +{ + /** @var array Currently resolving classes (circular dependency detection) */ + private array $resolving = []; + + /** @var array, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]}> Reflection data cache */ + private array $reflectionCache = []; + + /** + * Build a class instance using reflection and auto-wire dependencies. + * + * @template T of object + * @param class-string $className + * @return T + */ + public function build(string $className): object + { + // Circular dependency detection + if (isset($this->resolving[$className])) { + $chain = implode(' -> ', array_keys($this->resolving)) . " -> {$className}"; + throw new \RuntimeException("Circular dependency detected: {$chain}"); + } + + if (!class_exists($className)) { + throw new \RuntimeException("Class [{$className}] does not exist"); + } + + // Get cached reflection data + $reflectionData = $this->getReflectionData($className); + $reflector = $reflectionData['reflector']; + $constructor = $reflectionData['constructor']; + $parameters = $reflectionData['parameters']; + + if (!$reflector->isInstantiable()) { + throw new \RuntimeException("Class [{$className}] is not instantiable"); + } + + // Mark as currently resolving + $this->resolving[$className] = true; + + try { + // If no constructor, return new instance + if ($constructor === null) { + /** @var T */ + return $reflector->newInstance(); + } + + // Resolve constructor dependencies + $dependencies = $this->resolveDependencies($parameters); + + /** @var T */ + return $reflector->newInstanceArgs($dependencies); + } finally { + // Always clean up resolving state + unset($this->resolving[$className]); + } + } + + /** + * Get cached reflection data for a class. + * + * @param class-string $className + * @return array{reflector: ReflectionClass, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]} + */ + private function getReflectionData(string $className): array + { + if (!isset($this->reflectionCache[$className])) { + $reflector = new ReflectionClass($className); + $constructor = $reflector->getConstructor(); + $parameters = $constructor?->getParameters() ?? []; + + $this->reflectionCache[$className] = [ + 'reflector' => $reflector, + 'constructor' => $constructor, + 'parameters' => $parameters, + ]; + } + + return $this->reflectionCache[$className]; + } + + /** + * Resolve all dependencies for constructor parameters. + * + * @param ReflectionParameter[] $parameters + * @return array + */ + private function resolveDependencies(array $parameters): array + { + $dependencies = []; + + foreach ($parameters as $parameter) { + $dependencies[] = $this->resolveParameter($parameter); + } + + return $dependencies; + } + + /** + * Resolve a single constructor parameter. + */ + private function resolveParameter(ReflectionParameter $parameter): mixed + { + $type = $parameter->getType(); + + // Handle union types and built-in types + if (!$type instanceof ReflectionNamedType || $type->isBuiltin()) { + if ($parameter->isDefaultValueAvailable()) { + return $parameter->getDefaultValue(); + } + + 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) { + // If dependency resolution fails and parameter has default, use it + if ($parameter->isDefaultValueAvailable()) { + return $parameter->getDefaultValue(); + } + + throw new \RuntimeException( + "Cannot resolve dependency [{$className}] for parameter [{$parameter->getName()}]", + previous: $e + ); + } + } +} diff --git a/app/DTOs/ServerDTO.php b/app/DTOs/ServerDTO.php new file mode 100644 index 00000000..d41514c6 --- /dev/null +++ b/app/DTOs/ServerDTO.php @@ -0,0 +1,34 @@ + $this->host, + 'port' => $this->port, + 'user' => $this->user, + 'key' => $this->key, + ]; + } +} diff --git a/app/Deployer.php b/app/Deployer.php index 87b65aa2..b826c4d4 100644 --- a/app/Deployer.php +++ b/app/Deployer.php @@ -4,14 +4,21 @@ namespace Bigpixelrocket\DeployerPHP; +use Composer\InstalledVersions; 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 Bigpixelrocket\DeployerPHP\Console\Server\ServerAddCommand; +use Bigpixelrocket\DeployerPHP\Console\Server\ServerDeleteCommand; +use Bigpixelrocket\DeployerPHP\Console\Server\ServerListCommand; class Deployer extends Application { private SymfonyStyle $io; + private readonly Container $container; public function __construct() { @@ -20,6 +27,10 @@ public function __construct() parent::__construct('Deployer', $version); $this->setDefaultCommand('list'); + $this->container = new Container(); + + // Register commands + $this->registerCommands(); } /** @@ -69,6 +80,25 @@ private function displayBanner(): void } + // + // Command registration / DI wiring + // ------------------------------------------------------------------------------- + + private function registerCommands(): void + { + $commands = [ + ServerAddCommand::class, + ServerDeleteCommand::class, + ServerListCommand::class, + ]; + + foreach ($commands as $command) { + /** @var Command $commandInstance */ + $commandInstance = $this->container->build($command); + $this->add($commandInstance); + } + } + // // Version functions // ------------------------------------------------------------------------------- @@ -81,9 +111,9 @@ private function displayBanner(): void private function getVersionFromComposer(): string { // Try Composer's InstalledVersions API first - 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'); if (null !== $version) { return $version; } @@ -117,23 +147,29 @@ private function getVersionFromGit(): ?string } // 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); + $tagProcess = new Process(['git', 'describe', '--tags', '--exact-match'], $projectRoot); + $tagProcess->run(); + if ($tagProcess->isSuccessful()) { + return trim($tagProcess->getOutput()); } // Get the latest tag + commit info - $describe = @shell_exec('cd '.escapeshellarg($projectRoot).' && git describe --tags --always 2>/dev/null'); - if ($describe) { - return trim($describe); + $describeProcess = new Process(['git', 'describe', '--tags', '--always'], $projectRoot); + $describeProcess->run(); + if ($describeProcess->isSuccessful()) { + return trim($describeProcess->getOutput()); } // 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); + $branchProcess = new Process(['git', 'rev-parse', '--abbrev-ref', 'HEAD'], $projectRoot); + $branchProcess->run(); + $commitProcess = new Process(['git', 'rev-parse', '--short', 'HEAD'], $projectRoot); + $commitProcess->run(); + + if ($branchProcess->isSuccessful() && $commitProcess->isSuccessful()) { + $branch = trim($branchProcess->getOutput()); + $commit = trim($commitProcess->getOutput()); + return $branch.'@'.$commit; } return null; diff --git a/app/Items/ServerItem.php b/app/Items/ServerItem.php new file mode 100644 index 00000000..d5b8323b --- /dev/null +++ b/app/Items/ServerItem.php @@ -0,0 +1,91 @@ +assertValidName($name); + $this->assertValidServer($server); + + if ($this->exists($name)) { + throw new \RuntimeException("Server '{$name}' already exists."); + } + + $this->inventory->set('servers', $name, $server->toArray()); + } + + /** + * Check if a server exists. + */ + public function exists(string $name): bool + { + return $this->inventory->has('servers', $name); + } + + /** + * Get all servers. + * + * @return array + */ + public function list(): array + { + return $this->inventory->list('servers'); + } + + /** + * Delete a server record after validation. + */ + public function delete(string $name): void + { + $this->assertValidName($name); + + if (!$this->exists($name)) { + throw new \RuntimeException("Server '{$name}' does not exist."); + } + + $this->inventory->delete('servers', $name); + } + + /** + * Validate server payload shape and values. + */ + private function assertValidServer(ServerDTO $server): void + { + if ($server->host === '') { + throw new \InvalidArgumentException('Invalid host.'); + } + + $port = $server->port; + if ($port < 1 || $port > 65535) { + throw new \InvalidArgumentException('Invalid port.'); + } + + if ($server->user === '') { + throw new \InvalidArgumentException('Invalid user.'); + } + } + + private function assertValidName(string $name): void + { + if ($name === '' || !preg_match('/^[a-zA-Z0-9_.-]+$/', $name)) { + throw new \InvalidArgumentException('Invalid server name. Use letters, numbers, dots, dashes, underscores.'); + } + } +} diff --git a/app/Services/EnvService.php b/app/Services/EnvService.php index d5a5ef80..f5848179 100644 --- a/app/Services/EnvService.php +++ b/app/Services/EnvService.php @@ -5,25 +5,45 @@ namespace Bigpixelrocket\DeployerPHP\Services; use Symfony\Component\Dotenv\Dotenv; +use Symfony\Component\Filesystem\Filesystem; /** * Environment variable reader with fallback to .env file if value not found in environment variables. + * Defaults to looking for .env file in the current working directory. + * Uses Symfony Filesystem component for better testability and mocking capabilities. */ class EnvService { /** @var array */ private array $dotenv = []; - public function __construct() + public function __construct( + private readonly Filesystem $filesystem = new Filesystem() + ) { + $this->loadDotenvFile(); + } + + /** + * Load and parse the .env file if it exists and is readable. + */ + private function loadDotenvFile(): void { - $path = getcwd().'/.env'; - if (is_file($path) && is_readable($path)) { - $dotenv = new Dotenv(); - $parsed = $dotenv->parse((string) file_get_contents($path), $path); - foreach ($parsed as $k => $v) { - if (is_string($k) && is_string($v)) { - $this->dotenv[$k] = $v; + $envPath = rtrim((string) getcwd(), '/') . '/.env'; + + if ($this->filesystem->exists($envPath)) { + 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, similar to original behavior + $this->dotenv = []; } } } diff --git a/app/Services/InventoryService.php b/app/Services/InventoryService.php new file mode 100644 index 00000000..cf4aedf3 --- /dev/null +++ b/app/Services/InventoryService.php @@ -0,0 +1,182 @@ +readInventory(); + if (!isset($inventory[$collection]) || !is_array($inventory[$collection])) { + $inventory[$collection] = []; + } + + $inventory[$collection][$key] = $value; + $this->writeInventory($inventory); + } + + /** + * Replace an entire collection. + * + * @param array $items + */ + public function setCollection(string $collection, array $items): void + { + $inventory = $this->readInventory(); + $inventory[$collection] = $items; + $this->writeInventory($inventory); + } + + // + // READ Operations + // ---- + + /** + * Get the entire inventory structure. + * + * @return array + */ + public function getAll(): array + { + return $this->readInventory(); + } + + /** + * Get all records from a collection. + * + * @return array + */ + public function list(string $collection): array + { + $inventory = $this->readInventory(); + + $records = $inventory[$collection] ?? []; + /** @var array $safe */ + $safe = is_array($records) ? $records : []; + return $safe; + } + + /** + * Get a single record from a collection. + */ + public function get(string $collection, string $key): mixed + { + $inventory = $this->readInventory(); + if (!isset($inventory[$collection]) || !is_array($inventory[$collection]) || !array_key_exists($key, $inventory[$collection])) { + throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + } + + return $inventory[$collection][$key]; + } + + /** + * Check if a key exists in a collection. + */ + public function has(string $collection, string $key): bool + { + $inventory = $this->readInventory(); + + return isset($inventory[$collection]) + && is_array($inventory[$collection]) + && array_key_exists($key, $inventory[$collection]); + } + + // + // DELETE Operations + // ---- + + /** + * Delete a single record from a collection. + */ + public function delete(string $collection, string $key, bool $mustExist = true): void + { + $inventory = $this->readInventory(); + + if (!isset($inventory[$collection]) || !is_array($inventory[$collection]) || !array_key_exists($key, $inventory[$collection])) { + if ($mustExist) { + throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + } + + return; + } + + unset($inventory[$collection][$key]); + $this->writeInventory($inventory); + } + + // + // Private Helper Methods + // ---- + + /** + * Read inventory YAML into a structured array. + * + * @return array + */ + private function readInventory(): array + { + $path = $this->getInventoryPath(); + + if (!$this->filesystem->exists($path)) { + return []; + } + + $raw = $this->filesystem->readFile($path); + $parsed = Yaml::parse($raw); + + /** @var array $result */ + $result = is_array($parsed) ? $parsed : []; + return $result; + } + + /** + * Persist inventory data to YAML file. + * + * @param array $inventory + */ + private function writeInventory(array $inventory): void + { + $path = $this->getInventoryPath(); + $dir = dirname($path); + + if (!$this->filesystem->exists($dir)) { + try { + $this->filesystem->mkdir($dir, 0775); + } catch (\Throwable $e) { + throw new \RuntimeException("Unable to create inventory directory: {$dir}", 0, $e); + } + } + + $yaml = Yaml::dump($inventory, 4, 2, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE); + try { + $this->filesystem->dumpFile($path, $yaml); + } catch (\Throwable $e) { + throw new \RuntimeException("Failed to write inventory file at {$path}", 0, $e); + } + } + + private function getInventoryPath(): string + { + return rtrim((string) getcwd(), '/').'/.deployer/inventory.yml'; + } +} diff --git a/app/Services/SSHService.php b/app/Services/SSHService.php new file mode 100644 index 00000000..1696e08c --- /dev/null +++ b/app/Services/SSHService.php @@ -0,0 +1,118 @@ +resolvePrivateKeyPath($privateKeyPath); + + if ($resolvedKeyPath === null) { + throw new \RuntimeException('No SSH private key found. Provide --key or place a key at ~/.ssh/id_rsa (or ~/.ssh/id_ed25519).'); + } + + if (!is_file($resolvedKeyPath) || !is_readable($resolvedKeyPath)) { + throw new \RuntimeException("SSH key is not readable: {$resolvedKeyPath}"); + } + + $keyContents = (string) file_get_contents($resolvedKeyPath); + + try { + $key = PublicKeyLoader::load($keyContents); + } catch (\Throwable $e) { + throw new \RuntimeException('Failed to load SSH private key: '.$e->getMessage(), previous: $e); + } + + if (!$key instanceof PrivateKey) { + throw new \RuntimeException('Provided key is not a valid private key.'); + } + + try { + $ssh = new SSH2($host, $port); + } catch (\Throwable $e) { + throw new \RuntimeException("Failed to initiate SSH connection to {$host}: {$e->getMessage()}", previous: $e); + } + + try { + $loggedIn = $ssh->login($username, $key); + } catch (\Throwable $e) { + throw new \RuntimeException('SSH authentication error: '.$e->getMessage(), previous: $e); + } + + if ($loggedIn !== true) { + throw new \RuntimeException('SSH authentication failed. Check username and key permissions.'); + } + + // Best-effort disconnect; ignore errors + try { + $ssh->disconnect(); + } catch (\Throwable) { + // no-op + } + } + + /** + * Resolve a usable private key path. Preference order: + * 1) Provided path (supports ~ expansion) + * 2) ~/.ssh/id_ed25519 + * 3) ~/.ssh/id_rsa + */ + private function resolvePrivateKeyPath(?string $path): ?string + { + $candidates = []; + + if (is_string($path) && $path !== '') { + $candidates[] = $this->expandHomePath($path); + } + + $home = rtrim((string) getenv('HOME'), '/'); + if ($home !== '') { + // Default to id_rsa first, then try id_ed25519 + $candidates[] = $home.'/.ssh/id_rsa'; + $candidates[] = $home.'/.ssh/id_ed25519'; + } + + foreach ($candidates as $candidate) { + if (is_file($candidate)) { + return $candidate; + } + } + + return null; + } + + /** + * Expand a leading tilde in a filesystem path to the user's HOME. + */ + private function expandHomePath(string $path): string + { + if ($path === '' || $path[0] !== '~') { + return $path; + } + + $home = (string) getenv('HOME'); + if ($home === '') { + return $path; // best effort + } + + return $home.substr($path, 1); + } +} diff --git a/composer.json b/composer.json index 4a9e449e..f811c775 100644 --- a/composer.json +++ b/composer.json @@ -21,7 +21,7 @@ "phpseclib/phpseclib": "^3.0", "symfony/console": "^6.0|^7.0", "symfony/dotenv": "^7.0", - "symfony/filesystem": "^6.0|^7.0", + "symfony/filesystem": "^7.1", "symfony/yaml": "^7.0" }, "require-dev": { diff --git a/composer.lock b/composer.lock index 447ed5e4..254f0154 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "7bf31386d2477629a768d85e61b8b315", + "content-hash": "7d5eeba1a728149305ebf84f8bb251da", "packages": [ { "name": "guzzlehttp/guzzle", @@ -4044,16 +4044,16 @@ }, { "name": "sebastian/exporter", - "version": "5.1.2", + "version": "5.1.3", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/exporter.git", - "reference": "955288482d97c19a372d3f31006ab3f37da47adf" + "reference": "9e7e86260de48e405ec3086bcb62e677ef192e7f" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/exporter/zipball/955288482d97c19a372d3f31006ab3f37da47adf", - "reference": "955288482d97c19a372d3f31006ab3f37da47adf", + "url": "https://api.github.com/repos/sebastianbergmann/exporter/zipball/9e7e86260de48e405ec3086bcb62e677ef192e7f", + "reference": "9e7e86260de48e405ec3086bcb62e677ef192e7f", "shasum": "" }, "require": { @@ -4062,7 +4062,7 @@ "sebastian/recursion-context": "^5.0" }, "require-dev": { - "phpunit/phpunit": "^10.0" + "phpunit/phpunit": "^10.5" }, "type": "library", "extra": { @@ -4110,15 +4110,27 @@ "support": { "issues": "https://github.com/sebastianbergmann/exporter/issues", "security": "https://github.com/sebastianbergmann/exporter/security/policy", - "source": "https://github.com/sebastianbergmann/exporter/tree/5.1.2" + "source": "https://github.com/sebastianbergmann/exporter/tree/5.1.3" }, "funding": [ { "url": "https://github.com/sebastianbergmann", "type": "github" + }, + { + "url": "https://liberapay.com/sebastianbergmann", + "type": "liberapay" + }, + { + "url": "https://thanks.dev/u/gh/sebastianbergmann", + "type": "thanks_dev" + }, + { + "url": "https://tidelift.com/funding/github/packagist/sebastian/exporter", + "type": "tidelift" } ], - "time": "2024-03-02T07:17:12+00:00" + "time": "2025-09-22T05:25:48+00:00" }, { "name": "sebastian/global-state", diff --git a/tests/Unit/ContainerTest.php b/tests/Unit/ContainerTest.php new file mode 100644 index 00000000..80f10f2f --- /dev/null +++ b/tests/Unit/ContainerTest.php @@ -0,0 +1,166 @@ +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); + }); +}); diff --git a/tests/Unit/EnvServiceTest.php b/tests/Unit/EnvServiceTest.php new file mode 100644 index 00000000..692c8323 --- /dev/null +++ b/tests/Unit/EnvServiceTest.php @@ -0,0 +1,114 @@ +exists; + } + public function readFile(string $filename): string + { + if ($this->error) { + throw new \RuntimeException('Permission denied'); + } + return $this->content; + } + }; +} + +function setEnv(string $key, ?string $value): void +{ + if ($value === null) { + unset($_ENV[$key]); + putenv("{$key}"); + } else { + $_ENV[$key] = $value; + putenv("{$key}={$value}"); + } +} + +// +// EnvService Unit Tests +// ------------------------------------------------------------------------------- + +describe('EnvService', function () { + beforeEach(function () { + foreach (['TEST_KEY', 'API_KEY', 'KEY1', 'KEY2', 'MISSING_KEY', 'CUSTOM_KEY'] as $key) { + setEnv($key, null); + } + }); + + it('resolves environment variables from multiple sources with correct precedence', function ($env, $fileContent, $fileError, $keys, $expected) { + // ARRANGE + foreach ($env as $key => $value) { + setEnv($key, $value); + } + $service = new EnvService(mockFilesystem(!empty($fileContent), $fileContent, $fileError), '.env'); + + // ACT + $result = $service->get($keys, false); + + // ASSERT + expect($result)->toBe($expected); + + // CLEANUP + foreach (array_keys($env) as $key) { + setEnv($key, null); + } + })->with([ + // Single key scenarios + [[], 'API_KEY=from_file', false, 'API_KEY', 'from_file'], // File only + [['API_KEY' => 'from_env'], 'API_KEY=from_file', false, 'API_KEY', 'from_env'], // Env wins + [['API_KEY' => ''], 'API_KEY=from_file', false, 'API_KEY', 'from_file'], // Empty env ignored + [[], 'API_KEY=', false, 'API_KEY', null], // Empty file ignored + [[], '', false, 'API_KEY', null], // File missing + [[], 'API_KEY=value', true, 'API_KEY', null], // File read error + + // Multiple key scenarios (iterates in order, returns first found) + [['KEY1' => 'from_env'], 'KEY2=file_val', false, ['KEY1', 'KEY2'], 'from_env'], // First key in env + [[], "KEY1=file_val\nKEY2=other", false, ['KEY1', 'KEY2'], 'file_val'], // First key in file + [[], '', false, ['KEY1', 'KEY2'], null], // No keys found + ]); + + it('handles required vs optional parameters', function ($keys, $required, $expectsException, $expectedMessage) { + // ARRANGE + $service = new EnvService(mockFilesystem(false), '.env'); + + // ACT & ASSERT + if ($expectsException) { + expect(fn () => $service->get($keys, $required)) + ->toThrow(\RuntimeException::class, $expectedMessage); + } else { + expect($service->get($keys, $required))->toBeNull(); + } + })->with([ + ['MISSING_KEY', true, true, 'Missing environment variable: MISSING_KEY'], + [['KEY1', 'KEY2'], true, true, 'Missing environment variables: KEY1, KEY2'], + ['MISSING_KEY', false, false, null], + ['MISSING_KEY', true, true, 'Missing environment variable: MISSING_KEY'], // Default required=true + ]); + + it('loads from custom .env path and returns correct values', function () { + // ARRANGE + $service = new EnvService(mockFilesystem(true, 'CUSTOM_KEY=custom_value'), '/custom/.env'); + + // ACT + $result = $service->get('CUSTOM_KEY', false); + + // ASSERT + expect($result)->toBe('custom_value'); + }); +}); diff --git a/tests/Unit/InventoryServiceTest.php b/tests/Unit/InventoryServiceTest.php new file mode 100644 index 00000000..824ec2e5 --- /dev/null +++ b/tests/Unit/InventoryServiceTest.php @@ -0,0 +1,304 @@ +fileExists = $exists; + } + + public function setFileContent(string $content): void + { + $this->fileContent = $content; + } + + public function setShouldThrowOnMkdir(bool $throw): void + { + $this->shouldThrowOnMkdir = $throw; + } + + public function setShouldThrowOnDump(bool $throw): void + { + $this->shouldThrowOnDump = $throw; + } + + public function exists($files): bool + { + return $this->fileExists; + } + + public function readFile(string $filename): string + { + return $this->fileContent; + } + + public function mkdir($dirs, int $mode = 0777): void + { + if ($this->shouldThrowOnMkdir) { + throw new Exception('Permission denied'); + } + } + + public function dumpFile(string $filename, $content): void + { + if ($this->shouldThrowOnDump) { + throw new Exception('Write failed'); + } + } +} + +describe('InventoryService', function () { + beforeEach(function () { + $this->filesystem = new FilesystemStub(); + $this->service = new InventoryService($this->filesystem); + }); + + // + // Read Operations + // ---- + + it('returns empty array when inventory file does not exist', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + + // ACT + $result = $this->service->getAll(); + + // ASSERT + expect($result)->toBe([]); + }); + + it('returns parsed inventory when file exists', function () { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com"; + $expected = ['servers' => ['web1' => ['host' => 'example.com']]]; + + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $result = $this->service->getAll(); + + // ASSERT + expect($result)->toBe($expected); + }); + + it('returns empty array for non-existent collection', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + + // ACT + $result = $this->service->list('servers'); + + // ASSERT + expect($result)->toBe([]); + }); + + it('returns collection items when collection exists', function () { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com\n web2:\n host: test.com"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $result = $this->service->list('servers'); + + // ASSERT + expect($result)->toBe([ + 'web1' => ['host' => 'example.com'], + 'web2' => ['host' => 'test.com'], + ]); + }); + + it('safely handles non-array collection values', function () { + // ARRANGE + $yamlContent = "servers: invalid_string_value"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $result = $this->service->list('servers'); + + // ASSERT + expect($result)->toBe([]); + }); + + it('returns false for non-existent collection', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + + // ACT + $result = $this->service->has('servers', 'web1'); + + // ASSERT + expect($result)->toBeFalse(); + }); + + it('checks key existence in collection', function (string $collection, string $key, bool $expected) { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com\ndatabases:\n db1:\n host: db.com"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $result = $this->service->has($collection, $key); + + // ASSERT + expect($result)->toBe($expected); + })->with([ + ['servers', 'web1', true], + ['servers', 'web2', false], + ['databases', 'db1', true], + ['nonexistent', 'key', false], + ]); + + it('throws exception when key not found', function (string $collection, string $key) { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT & ASSERT + expect(fn () => $this->service->get($collection, $key)) + ->toThrow(RuntimeException::class, "Key '{$key}' not found in collection '{$collection}'."); + })->with([ + ['servers', 'web2'], + ['databases', 'db1'], + ['nonexistent', 'key'], + ]); + + it('returns value when key exists', function () { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com\n port: 22"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $result = $this->service->get('servers', 'web1'); + + // ASSERT + expect($result)->toBe(['host' => 'example.com', 'port' => 22]); + }); + + // + // Write Operations + // ---- + + it('creates new collection and key', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + + // ACT + $this->service->set('servers', 'web1', ['host' => 'example.com']); + + // ASSERT + // No exception should be thrown - test passes if no error + expect(true)->toBeTrue(); + }); + + it('adds key to existing collection', function () { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $this->service->set('servers', 'web2', ['host' => 'test.com']); + + // ASSERT + expect(true)->toBeTrue(); // No exception should be thrown + }); + + it('overwrites existing key', function () { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: old.com"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $this->service->set('servers', 'web1', ['host' => 'new.com']); + + // ASSERT + expect(true)->toBeTrue(); // No exception should be thrown + }); + + it('replaces entire collection', function () { + // ARRANGE + $items = ['web1' => ['host' => 'example.com'], 'web2' => ['host' => 'test.com']]; + $this->filesystem->setFileExists(false); + + // ACT + $this->service->setCollection('servers', $items); + + // ASSERT + expect(true)->toBeTrue(); // No exception should be thrown + }); + + it('throws exception when key not found and mustExist is true', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + + // ACT & ASSERT + expect(fn () => $this->service->delete('servers', 'web1', true)) + ->toThrow(RuntimeException::class, "Key 'web1' not found in collection 'servers'."); + }); + + it('does nothing when key not found and mustExist is false', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + + // ACT + $this->service->delete('servers', 'web1', false); + + // ASSERT + expect(true)->toBeTrue(); // No exception should be thrown + }); + + it('removes key and updates file', function () { + // ARRANGE + $yamlContent = "servers:\n web1:\n host: example.com\n web2:\n host: test.com"; + $this->filesystem->setFileExists(true); + $this->filesystem->setFileContent($yamlContent); + + // ACT + $this->service->delete('servers', 'web1'); + + // ASSERT + expect(true)->toBeTrue(); // No exception should be thrown + }); + + // + // Error Handling + // ---- + + it('handles directory creation failure', function () { + // ARRANGE + $this->filesystem->setFileExists(false); + $this->filesystem->setShouldThrowOnMkdir(true); + + // ACT & ASSERT + expect(fn () => $this->service->set('servers', 'web1', ['host' => 'example.com'])) + ->toThrow(RuntimeException::class, 'Unable to create inventory directory'); + }); + + it('handles file write failure', function () { + // ARRANGE + $this->filesystem->setFileExists(true); + $this->filesystem->setShouldThrowOnDump(true); + + // ACT & ASSERT + expect(fn () => $this->service->set('servers', 'web1', ['host' => 'example.com'])) + ->toThrow(RuntimeException::class, 'Failed to write inventory file'); + }); +}); From b58f5bc7b4bf487be437761a3e222f3ecb97666f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Tue, 23 Sep 2025 19:33:07 +0300 Subject: [PATCH 2/7] fixup --- .cursor/commands/review-consistency.md | 1 + ...eview-testing-rules.md => review-tests.md} | 0 .github/workflows/review-consistency.yml | 57 +++++++++++++++++++ .../{testing-rules.yml => review-tests.yml} | 6 +- 4 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 .cursor/commands/review-consistency.md rename .cursor/commands/{review-testing-rules.md => review-tests.md} (100%) create mode 100644 .github/workflows/review-consistency.yml rename .github/workflows/{testing-rules.yml => review-tests.yml} (96%) diff --git a/.cursor/commands/review-consistency.md b/.cursor/commands/review-consistency.md new file mode 100644 index 00000000..3cef1bdf --- /dev/null +++ b/.cursor/commands/review-consistency.md @@ -0,0 +1 @@ +Analyze the changes in this Git working tree with a focus on finding inconsistencies in logic or implementation patterns with other similar areas of the codebase. All code in this repository should look and feel like it was written by a single person with a consistent set of easthetic principles: everything from how classes and variables are named to how code is organized and flows logically. diff --git a/.cursor/commands/review-testing-rules.md b/.cursor/commands/review-tests.md similarity index 100% rename from .cursor/commands/review-testing-rules.md rename to .cursor/commands/review-tests.md diff --git a/.github/workflows/review-consistency.yml b/.github/workflows/review-consistency.yml new file mode 100644 index 00000000..de36b01b --- /dev/null +++ b/.github/workflows/review-consistency.yml @@ -0,0 +1,57 @@ +name: Review Consistency + +permissions: + contents: read + pull-requests: write + +on: + pull_request: + +jobs: + review-consistency: + runs-on: ubuntu-latest + timeout-minutes: 3 + concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install Cursor CLI + run: | + curl https://cursor.com/install -fsS | bash + echo "$HOME/.cursor/bin" >> $GITHUB_PATH + + - name: Review consistency + env: + CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} + GH_TOKEN: ${{ github.token }} + run: | + cursor-agent --force --model "claude-4-sonnet" --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. + + Context: + - Repo: ${{ github.repository }} + - PR Number: ${{ github.event.pull_request.number }} + - PR Head SHA: ${{ github.event.pull_request.head.sha }} + - PR Base SHA: ${{ github.event.pull_request.base.sha }} + + Review the current PR diff with a focus on finding inconsistencies in logic or implementation patterns with other similar areas of the codebase. All code in this repository should look and feel like it was written by a single person with a consistent set of easthetic principles: everything from how classes and variables are named to how code is organized and flows logically. + + Leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end. + + Procedure: + - Get existing comments: gh pr view --json comments + - Get diff: gh pr diff + - If a previously reported issue appears fixed by nearby changes, reply: ✅ This issue appears to be resolved by the recent changes + - Avoid duplicates: skip if similar feedback already exists on or near the same lines + + Commenting rules: + - Natural tone, specific and actionable; do not mention automated or high-confidence + - Use emojis: 🚨 Critical 🔒 Security ⚡ Performance ⚠️ Logic ✅ Resolved ✨ Improvement + + Submission: + - Submit one review containing inline comments plus a concise summary + - Use only: gh pr review --comment + - Do not use: gh pr review --approve or --request-changes" diff --git a/.github/workflows/testing-rules.yml b/.github/workflows/review-tests.yml similarity index 96% rename from .github/workflows/testing-rules.yml rename to .github/workflows/review-tests.yml index 483df391..97dba25d 100644 --- a/.github/workflows/testing-rules.yml +++ b/.github/workflows/review-tests.yml @@ -1,4 +1,4 @@ -name: Testing Rules +name: Review Tests permissions: contents: read @@ -8,7 +8,7 @@ on: pull_request: jobs: - testing-rules: + review-tests: runs-on: ubuntu-latest timeout-minutes: 3 concurrency: @@ -24,7 +24,7 @@ jobs: curl https://cursor.com/install -fsS | bash echo "$HOME/.cursor/bin" >> $GITHUB_PATH - - name: Testing rules review + - name: Review tests env: CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} GH_TOKEN: ${{ github.token }} From b27a4c8c60e912ffafb7cffcc176209447add17f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Wed, 24 Sep 2025 00:10:44 +0300 Subject: [PATCH 3/7] fixup --- .cursor/commands/refactor.md | 1 + .cursor/commands/review-consistency.md | 1 - .cursor/commands/review-tests.md | 1 - .cursor/commands/review.md | 1 + .cursor/rules/00-main.mdc | 14 +++++++ .cursor/rules/01-architecture.mdc | 42 ++++++++++++++++++-- .cursor/rules/02-code-quality.mdc | 35 ---------------- .cursor/rules/{03-tests.mdc => 02-tests.mdc} | 2 +- .github/workflows/review-consistency.yml | 2 +- app/Services/EnvService.php | 2 +- 10 files changed, 57 insertions(+), 44 deletions(-) create mode 100644 .cursor/commands/refactor.md delete mode 100644 .cursor/commands/review-consistency.md delete mode 100644 .cursor/commands/review-tests.md create mode 100644 .cursor/commands/review.md delete mode 100644 .cursor/rules/02-code-quality.mdc rename .cursor/rules/{03-tests.mdc => 02-tests.mdc} (99%) diff --git a/.cursor/commands/refactor.md b/.cursor/commands/refactor.md new file mode 100644 index 00000000..0ded97ed --- /dev/null +++ b/.cursor/commands/refactor.md @@ -0,0 +1 @@ +Refactor following our minimalist code philosophy then organize and catalog like a librerian and obsess over code consistency. Let's take this code from an A+ to an A++ 🚀 diff --git a/.cursor/commands/review-consistency.md b/.cursor/commands/review-consistency.md deleted file mode 100644 index 3cef1bdf..00000000 --- a/.cursor/commands/review-consistency.md +++ /dev/null @@ -1 +0,0 @@ -Analyze the changes in this Git working tree with a focus on finding inconsistencies in logic or implementation patterns with other similar areas of the codebase. All code in this repository should look and feel like it was written by a single person with a consistent set of easthetic principles: everything from how classes and variables are named to how code is organized and flows logically. diff --git a/.cursor/commands/review-tests.md b/.cursor/commands/review-tests.md deleted file mode 100644 index 2205e791..00000000 --- a/.cursor/commands/review-tests.md +++ /dev/null @@ -1 +0,0 @@ -Analyze the changes in this Git working tree and report back on tests that fall short of our testing rules. diff --git a/.cursor/commands/review.md b/.cursor/commands/review.md new file mode 100644 index 00000000..988ab9b8 --- /dev/null +++ b/.cursor/commands/review.md @@ -0,0 +1 @@ +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. diff --git a/.cursor/rules/00-main.mdc b/.cursor/rules/00-main.mdc index 42c9ca29..58968260 100644 --- a/.cursor/rules/00-main.mdc +++ b/.cursor/rules/00-main.mdc @@ -6,6 +6,8 @@ alwaysApply: true We're developing Deployer PHP, a Composer package and CLI tool that simplifies provisioning and setting up servers across multiple Cloud providers, enabling you to deploy your PHP projects effortlessly. +**🚨 All our rules are MANDATORY and IMPORTANT!** + **🔎 Before ANY Task:** - Check `composer.json` and `package.json` for installed packages @@ -18,6 +20,18 @@ We're developing Deployer PHP, a Composer package and CLI tool that simplifies p - Always ask yourself if less code can achieve the same result - Refactor relentlessly for code clarity and necessity +**🤓 Organize & Catalog Like A Librarian:** + +Organize and catalog code like a librarian. Group related functions into comment-separated sections for visual clarity, and arrange them alphabetically by name. Code should be functional and visually appealing; there is beauty in order. + +**🤖 Obsessive Code Consistency:** + +Be rigorous to the point of perfectionism in maintaining implementation consistency and logical coherence across similar areas of the codebase. Additionally, make it a habit to regularly review other code in and around the files you are working on to identify reusable patterns. + +The goal is for all the code in this repository to appear as if it were written by a single individual, adhering to a uniform style, a consistent set of standards, and aesthetic principles in software development. + +This can include everything from naming files, classes, variables, or array keys to the precedence and type of parameters passed to a function, to how logic flows and how the code is organized or commented. + **✔️ Don't worry about tests:** Write or run tests ONLY if specifically instructed **🧠 AI Agent Protocol:** ULTRATHINK → STEP BY STEP → ACT diff --git a/.cursor/rules/01-architecture.mdc b/.cursor/rules/01-architecture.mdc index 457d2a7b..78b1ea87 100644 --- a/.cursor/rules/01-architecture.mdc +++ b/.cursor/rules/01-architecture.mdc @@ -1,8 +1,9 @@ --- -globs: app/**/*.php,templates/**/*.yaml +globs: app/**/*.php,tests/**/*.php +alwaysApply: false --- -### Architecture (MANDATORY) +## Architecture Rules - **PHP:** PSR-12, strict types, PHP 8.x features (unions, match, attributes, readonly) - **Imports:** Always add `use` statements instead of fully qualified class names @@ -10,12 +11,12 @@ globs: app/**/*.php,templates/**/*.yaml **🚨 Architecture rules are IMMUTABLE - fix violating code, not the architecture rules** -### Symfony Patterns (MANDATORY) +### Symfony Patterns - **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 (MANDATORY) +### Dependency Injection - Dependencies are automatically injected by the `app/Container` class - ALL dependencies MUST be injected through constructors - NO manual instantiation @@ -51,3 +52,36 @@ globs: app/**/*.php,templates/**/*.yaml - All dependencies declared in constructor signatures **IMPORTANT:** No circular dependencies allowed. + +### Comments + +**DocBlock:** Add docblock comments with minimalist descriptions, parameters and return types for classes and functions; + +**Comment everything:** Use comments to separate sections and explain or summarize complex logic; + +``` +// +// {Section Header} +// ------------------------------------------------------------------------------- + +// +// {Section Subheader} + +// {Paragraph} +``` + +- Use comments as visual separators to help separate different sections +- Separate section headers, subheaders and paragraphs with a single newline +- Avoid commenting the obvious or leaving comments behind when removing code + +### Quality Gates + +**ALWAYS run these commands against the files you have touched and fix any issues BEFORE considering a task complete:** + +```bash +vendor/bin/rector $CHANGED_PHP_FILES # Code improvements (changed files only) +vendor/bin/pint $CHANGED_PHP_FILES # Fix code style (changed files only) + +# Static analysis excluding tests (never do static analysis against tests) +vendor/bin/phpstan analyze $CHANGED_PHP_FILES_EXCEPT_TESTS +``` diff --git a/.cursor/rules/02-code-quality.mdc b/.cursor/rules/02-code-quality.mdc deleted file mode 100644 index e36e9152..00000000 --- a/.cursor/rules/02-code-quality.mdc +++ /dev/null @@ -1,35 +0,0 @@ ---- -globs: app/**/*.php,tests/**/*.php,templates/**/*.yaml ---- - -## Code Quality Rules - -**DocBlock:** Add docblock comments with minimalist descriptions, parameters and return types for classes and functions; - -**Comment everything:** Use comments to separate sections of code and explain or summarize complex logic; avoid commenting the obvious: - -``` -// -// {Section Header} -// ------------------------------------------------------------------------------- - -// -// {Section Subheader} - -// {Paragraph} -``` - -- Separate section headers, subheaders and paragraphs with a single newline; -- Avoid commenting the obvious or leaving comments behind when removing code. - -## Quality Gates - -**ALWAYS run these commands against the files you have touched and fix any issues BEFORE considering a task complete:** - -```bash -vendor/bin/rector $CHANGED_PHP_FILES # Code improvements (changed files only) -vendor/bin/pint $CHANGED_PHP_FILES # Fix code style (changed files only) - -# Static analysis excluding tests (never do static analysis against tests) -vendor/bin/phpstan analyze $CHANGED_PHP_FILES_EXCEPT_TESTS -``` diff --git a/.cursor/rules/03-tests.mdc b/.cursor/rules/02-tests.mdc similarity index 99% rename from .cursor/rules/03-tests.mdc rename to .cursor/rules/02-tests.mdc index dff36d50..9725aa19 100644 --- a/.cursor/rules/03-tests.mdc +++ b/.cursor/rules/02-tests.mdc @@ -2,7 +2,7 @@ globs: app/**/*.php,tests/**/*.php --- -## PHP CLI Testing Rules +## Testing Rules **Philosophy:** "A test that never fails is not a test, it's a lie." diff --git a/.github/workflows/review-consistency.yml b/.github/workflows/review-consistency.yml index de36b01b..b0a41dcb 100644 --- a/.github/workflows/review-consistency.yml +++ b/.github/workflows/review-consistency.yml @@ -37,7 +37,7 @@ jobs: - PR Head SHA: ${{ github.event.pull_request.head.sha }} - PR Base SHA: ${{ github.event.pull_request.base.sha }} - Review the current PR diff with a focus on finding inconsistencies in logic or implementation patterns with other similar areas of the codebase. All code in this repository should look and feel like it was written by a single person with a consistent set of easthetic principles: everything from how classes and variables are named to how code is organized and flows logically. + Review the current PR diff with a focus on finding inconsistencies in logic or implementation patterns with other similar areas of the codebase. All code in this repository should look and feel like it was written by a single person with a consistent set of aesthetic principles: everything from how classes and variables are named to how code is organized and flows logically. Leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end. diff --git a/app/Services/EnvService.php b/app/Services/EnvService.php index f5848179..07ba72c9 100644 --- a/app/Services/EnvService.php +++ b/app/Services/EnvService.php @@ -18,7 +18,7 @@ class EnvService private array $dotenv = []; public function __construct( - private readonly Filesystem $filesystem = new Filesystem() + private readonly Filesystem $filesystem ) { $this->loadDotenvFile(); } From ed7e52b79e27cb95d275f0e6c517f2ea936fc4f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Wed, 24 Sep 2025 08:59:29 +0300 Subject: [PATCH 4/7] fixup --- .cursor/commands/refactor.md | 2 +- .cursor/rules/01-architecture.mdc | 8 + .github/workflows/review-consistency.yml | 57 ----- .../{review-tests.yml => review.yml} | 21 +- app/Container.php | 196 +++++++++++------- app/Services/EnvService.php | 68 +++--- app/Services/InventoryService.php | 133 ++++++++---- 7 files changed, 271 insertions(+), 214 deletions(-) delete mode 100644 .github/workflows/review-consistency.yml rename .github/workflows/{review-tests.yml => review.yml} (71%) diff --git a/.cursor/commands/refactor.md b/.cursor/commands/refactor.md index 0ded97ed..fec65c22 100644 --- a/.cursor/commands/refactor.md +++ b/.cursor/commands/refactor.md @@ -1 +1 @@ -Refactor following our minimalist code philosophy then organize and catalog like a librerian and obsess over code consistency. Let's take this code from an A+ to an A++ 🚀 +Refactor following our minimalist code philosophy then organize and catalog like a librarian and obsess over code consistency. Let's take this code from an A+ to an A++ 🚀 diff --git a/.cursor/rules/01-architecture.mdc b/.cursor/rules/01-architecture.mdc index 78b1ea87..0963792a 100644 --- a/.cursor/rules/01-architecture.mdc +++ b/.cursor/rules/01-architecture.mdc @@ -74,6 +74,14 @@ alwaysApply: false - Separate section headers, subheaders and paragraphs with a single newline - Avoid commenting the obvious or leaving comments behind when removing code +**ALWAYS use the correct section header comment format and not the simplified one:** + +``` + // + // {Section Header} + // ---- ❌ Too few dashes +``` + ### Quality Gates **ALWAYS run these commands against the files you have touched and fix any issues BEFORE considering a task complete:** diff --git a/.github/workflows/review-consistency.yml b/.github/workflows/review-consistency.yml deleted file mode 100644 index b0a41dcb..00000000 --- a/.github/workflows/review-consistency.yml +++ /dev/null @@ -1,57 +0,0 @@ -name: Review Consistency - -permissions: - contents: read - pull-requests: write - -on: - pull_request: - -jobs: - review-consistency: - runs-on: ubuntu-latest - timeout-minutes: 3 - concurrency: - group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true - - steps: - - name: Checkout code - uses: actions/checkout@v4 - - - name: Install Cursor CLI - run: | - curl https://cursor.com/install -fsS | bash - echo "$HOME/.cursor/bin" >> $GITHUB_PATH - - - name: Review consistency - env: - CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} - GH_TOKEN: ${{ github.token }} - run: | - cursor-agent --force --model "claude-4-sonnet" --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. - - Context: - - Repo: ${{ github.repository }} - - PR Number: ${{ github.event.pull_request.number }} - - PR Head SHA: ${{ github.event.pull_request.head.sha }} - - PR Base SHA: ${{ github.event.pull_request.base.sha }} - - Review the current PR diff with a focus on finding inconsistencies in logic or implementation patterns with other similar areas of the codebase. All code in this repository should look and feel like it was written by a single person with a consistent set of aesthetic principles: everything from how classes and variables are named to how code is organized and flows logically. - - Leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end. - - Procedure: - - Get existing comments: gh pr view --json comments - - Get diff: gh pr diff - - If a previously reported issue appears fixed by nearby changes, reply: ✅ This issue appears to be resolved by the recent changes - - Avoid duplicates: skip if similar feedback already exists on or near the same lines - - Commenting rules: - - Natural tone, specific and actionable; do not mention automated or high-confidence - - Use emojis: 🚨 Critical 🔒 Security ⚡ Performance ⚠️ Logic ✅ Resolved ✨ Improvement - - Submission: - - Submit one review containing inline comments plus a concise summary - - Use only: gh pr review --comment - - Do not use: gh pr review --approve or --request-changes" diff --git a/.github/workflows/review-tests.yml b/.github/workflows/review.yml similarity index 71% rename from .github/workflows/review-tests.yml rename to .github/workflows/review.yml index 97dba25d..0da4415e 100644 --- a/.github/workflows/review-tests.yml +++ b/.github/workflows/review.yml @@ -1,4 +1,4 @@ -name: Review Tests +name: Review permissions: contents: read @@ -8,7 +8,7 @@ on: pull_request: jobs: - review-tests: + review: runs-on: ubuntu-latest timeout-minutes: 3 concurrency: @@ -24,12 +24,12 @@ jobs: curl https://cursor.com/install -fsS | bash echo "$HOME/.cursor/bin" >> $GITHUB_PATH - - name: Review tests + - name: Code review env: CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} GH_TOKEN: ${{ github.token }} run: | - cursor-agent --force --model "claude-4-sonnet" --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 "x-ai/grok-4-fast:free" --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. Context: - Repo: ${{ github.repository }} @@ -37,10 +37,11 @@ jobs: - PR Head SHA: ${{ github.event.pull_request.head.sha }} - PR Base SHA: ${{ github.event.pull_request.base.sha }} - Review the current PR diff and... - 1. report back on tests that fall short of our testing rules - 2. identify gaps in our tests or missing tests - 3. leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end + Review the current PR diff and meticulously catalog all the changes in this branch. + + Report back on where the changes fall short of our development, architecture and testing rules. + + Leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end. Procedure: - Get existing comments: gh pr view --json comments @@ -53,6 +54,6 @@ jobs: - Use emojis: 🚨 Critical 🔒 Security ⚡ Performance ⚠️ Logic ✅ Resolved ✨ Improvement Submission: - - Submit one review containing inline comments plus a concise summary - Use only: gh pr review --comment - - Do not use: gh pr review --approve or --request-changes" + - Do not use: gh pr review --approve or --request-changes + - Submit one review containing inline comments plus a concise summary" diff --git a/app/Container.php b/app/Container.php index ca74a36d..cd1b98ea 100644 --- a/app/Container.php +++ b/app/Container.php @@ -20,14 +20,18 @@ */ class Container { - /** @var array Currently resolving classes (circular dependency detection) */ + /** @var array, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]}> */ + private array $reflectionCache = []; + + /** @var array */ private array $resolving = []; - /** @var array, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]}> Reflection data cache */ - private array $reflectionCache = []; + // + // Public + // ------------------------------------------------------------------------------- /** - * Build a class instance using reflection and auto-wire dependencies. + * Build a class instance with auto-wired dependencies. * * @template T of object * @param class-string $className @@ -35,112 +39,70 @@ class Container */ public function build(string $className): object { - // Circular dependency detection - if (isset($this->resolving[$className])) { - $chain = implode(' -> ', array_keys($this->resolving)) . " -> {$className}"; - throw new \RuntimeException("Circular dependency detected: {$chain}"); - } - - if (!class_exists($className)) { - throw new \RuntimeException("Class [{$className}] does not exist"); - } - - // Get cached reflection data - $reflectionData = $this->getReflectionData($className); - $reflector = $reflectionData['reflector']; - $constructor = $reflectionData['constructor']; - $parameters = $reflectionData['parameters']; + $this->guardAgainstCircularDependency($className); + $this->guardAgainstInvalidClass($className); - if (!$reflector->isInstantiable()) { - throw new \RuntimeException("Class [{$className}] is not instantiable"); - } + $reflectionData = $this->getCachedReflectionData($className); + $this->guardAgainstNonInstantiableClass($className, $reflectionData['reflector']); - // Mark as currently resolving $this->resolving[$className] = true; try { - // If no constructor, return new instance - if ($constructor === null) { - /** @var T */ - return $reflector->newInstance(); - } - - // Resolve constructor dependencies - $dependencies = $this->resolveDependencies($parameters); - /** @var T */ - return $reflector->newInstanceArgs($dependencies); + return $reflectionData['constructor'] === null + ? $reflectionData['reflector']->newInstance() + : $reflectionData['reflector']->newInstanceArgs( + $this->buildDependencies($reflectionData['parameters']) + ); } finally { - // Always clean up resolving state unset($this->resolving[$className]); } } - /** - * Get cached reflection data for a class. - * - * @param class-string $className - * @return array{reflector: ReflectionClass, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]} - */ - private function getReflectionData(string $className): array - { - if (!isset($this->reflectionCache[$className])) { - $reflector = new ReflectionClass($className); - $constructor = $reflector->getConstructor(); - $parameters = $constructor?->getParameters() ?? []; - - $this->reflectionCache[$className] = [ - 'reflector' => $reflector, - 'constructor' => $constructor, - 'parameters' => $parameters, - ]; - } + // + // Private + // ------------------------------------------------------------------------------- - return $this->reflectionCache[$className]; - } + // + // Dependency Resolution /** - * Resolve all dependencies for constructor parameters. + * Build dependencies for constructor parameters. * * @param ReflectionParameter[] $parameters * @return array */ - private function resolveDependencies(array $parameters): array + private function buildDependencies(array $parameters): array { - $dependencies = []; - - foreach ($parameters as $parameter) { - $dependencies[] = $this->resolveParameter($parameter); - } - - return $dependencies; + return array_values(array_map([$this, 'buildParameter'], $parameters)); } /** - * Resolve a single constructor parameter. + * Build a single constructor parameter dependency. */ - private function resolveParameter(ReflectionParameter $parameter): mixed + private function buildParameter(ReflectionParameter $parameter): mixed { $type = $parameter->getType(); - // Handle union types and built-in types if (!$type instanceof ReflectionNamedType || $type->isBuiltin()) { - if ($parameter->isDefaultValueAvailable()) { - return $parameter->getDefaultValue(); - } - - throw new \RuntimeException( - "Cannot resolve parameter [{$parameter->getName()}] in class [{$parameter->getDeclaringClass()?->getName()}]" - ); + return $this->resolveNonClassParameter($parameter); } - $className = $type->getName(); + return $this->resolveClassParameter($parameter, $type->getName()); + } + + // + // Parameter Resolution + /** + * Resolve a class-type parameter by building its dependency. + */ + private function resolveClassParameter(ReflectionParameter $parameter, string $className): mixed + { try { /** @var class-string $className */ return $this->build($className); } catch (\RuntimeException $e) { - // If dependency resolution fails and parameter has default, use it if ($parameter->isDefaultValueAvailable()) { return $parameter->getDefaultValue(); } @@ -151,4 +113,86 @@ private function resolveParameter(ReflectionParameter $parameter): mixed ); } } + + /** + * Resolve a non-class parameter by using its default value. + */ + private function resolveNonClassParameter(ReflectionParameter $parameter): mixed + { + if ($parameter->isDefaultValueAvailable()) { + return $parameter->getDefaultValue(); + } + + throw new \RuntimeException( + "Cannot resolve parameter [{$parameter->getName()}] in class [{$parameter->getDeclaringClass()?->getName()}]" + ); + } + + // + // Reflection Caching + + /** + * Get cached reflection data for a class. + * + * @param class-string $className + * @return array{reflector: ReflectionClass, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]} + */ + private function getCachedReflectionData(string $className): array + { + return $this->reflectionCache[$className] ??= $this->buildReflectionData($className); + } + + /** + * Build reflection data for a class. + * + * @param class-string $className + * @return array{reflector: ReflectionClass, constructor: ?\ReflectionMethod, parameters: ReflectionParameter[]} + */ + private function buildReflectionData(string $className): array + { + $reflector = new ReflectionClass($className); + $constructor = $reflector->getConstructor(); + + return [ + 'reflector' => $reflector, + 'constructor' => $constructor, + 'parameters' => $constructor?->getParameters() ?? [], + ]; + } + + // + // Guard Methods + + /** + * Guard against circular dependencies. + */ + private function guardAgainstCircularDependency(string $className): void + { + if (isset($this->resolving[$className])) { + $chain = implode(' -> ', array_keys($this->resolving)) . " -> {$className}"; + throw new \RuntimeException("Circular dependency detected: {$chain}"); + } + } + + /** + * Guard against invalid class names. + */ + private function guardAgainstInvalidClass(string $className): void + { + if (!class_exists($className)) { + throw new \RuntimeException("Class [{$className}] does not exist"); + } + } + + /** + * Guard against non-instantiable classes. + * + * @param ReflectionClass $reflector + */ + private function guardAgainstNonInstantiableClass(string $className, ReflectionClass $reflector): void + { + if (!$reflector->isInstantiable()) { + throw new \RuntimeException("Class [{$className}] is not instantiable"); + } + } } diff --git a/app/Services/EnvService.php b/app/Services/EnvService.php index 07ba72c9..aa5c219d 100644 --- a/app/Services/EnvService.php +++ b/app/Services/EnvService.php @@ -8,9 +8,7 @@ use Symfony\Component\Filesystem\Filesystem; /** - * Environment variable reader with fallback to .env file if value not found in environment variables. - * Defaults to looking for .env file in the current working directory. - * Uses Symfony Filesystem component for better testability and mocking capabilities. + * Environment variable reader with .env file fallback. */ class EnvService { @@ -23,45 +21,27 @@ public function __construct( $this->loadDotenvFile(); } - /** - * Load and parse the .env file if it exists and is readable. - */ - private function loadDotenvFile(): void - { - $envPath = rtrim((string) getcwd(), '/') . '/.env'; - - if ($this->filesystem->exists($envPath)) { - 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, similar to original behavior - $this->dotenv = []; - } - } - } + // + // Public + // ------------------------------------------------------------------------------- /** - * Get the first non-empty value for the given key(s). + * Get first non-empty value for given key(s). * - * @param array|string $keys + * @param array|string $keys */ public function get(array|string $keys, bool $required = true): ?string { $keysList = is_array($keys) ? $keys : [$keys]; + foreach ($keysList as $key) { + // Check environment variables first $value = $_ENV[$key] ?? getenv($key); if (is_string($value) && $value !== '') { return $value; } + // Check .env file fallback if (isset($this->dotenv[$key]) && $this->dotenv[$key] !== '') { return $this->dotenv[$key]; } @@ -76,4 +56,34 @@ public function get(array|string $keys, bool $required = true): ?string return null; } + // + // Private + // ------------------------------------------------------------------------------- + + /** + * 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 = []; + } + } } diff --git a/app/Services/InventoryService.php b/app/Services/InventoryService.php index cf4aedf3..ccfbea14 100644 --- a/app/Services/InventoryService.php +++ b/app/Services/InventoryService.php @@ -17,9 +17,12 @@ public function __construct( ) { } + // + // Public + // ------------------------------------------------------------------------------- + // // CREATE Operations - // ---- /** * Set a single record in a collection (create or overwrite). @@ -27,11 +30,12 @@ public function __construct( public function set(string $collection, string $key, mixed $value): void { $inventory = $this->readInventory(); - if (!isset($inventory[$collection]) || !is_array($inventory[$collection])) { - $inventory[$collection] = []; - } + $this->ensureCollection($inventory, $collection); - $inventory[$collection][$key] = $value; + /** @var array $collectionData */ + $collectionData = $inventory[$collection]; + $collectionData[$key] = $value; + $inventory[$collection] = $collectionData; $this->writeInventory($inventory); } @@ -49,7 +53,19 @@ public function setCollection(string $collection, array $items): void // // READ Operations - // ---- + + /** + * Get a single record from a collection. + */ + public function get(string $collection, string $key): mixed + { + $inventory = $this->readInventory(); + $this->validateCollectionKey($inventory, $collection, $key); + + /** @var array $collectionData */ + $collectionData = $inventory[$collection]; + return $collectionData[$key]; + } /** * Get the entire inventory structure. @@ -61,6 +77,15 @@ public function getAll(): array return $this->readInventory(); } + /** + * Check if a key exists in a collection. + */ + public function has(string $collection, string $key): bool + { + $inventory = $this->readInventory(); + return $this->collectionKeyExists($inventory, $collection, $key); + } + /** * Get all records from a collection. * @@ -69,64 +94,95 @@ public function getAll(): array public function list(string $collection): array { $inventory = $this->readInventory(); - $records = $inventory[$collection] ?? []; - /** @var array $safe */ - $safe = is_array($records) ? $records : []; - return $safe; + + /** @var array $result */ + $result = is_array($records) ? $records : []; + return $result; } + // + // DELETE Operations + /** - * Get a single record from a collection. + * Delete a single record from a collection. */ - public function get(string $collection, string $key): mixed + public function delete(string $collection, string $key, bool $mustExist = true): void { $inventory = $this->readInventory(); - if (!isset($inventory[$collection]) || !is_array($inventory[$collection]) || !array_key_exists($key, $inventory[$collection])) { - throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + + if (!$this->collectionKeyExists($inventory, $collection, $key)) { + if ($mustExist) { + $this->throwKeyNotFound($collection, $key); + } + return; } - return $inventory[$collection][$key]; + /** @var array $collectionData */ + $collectionData = $inventory[$collection]; + unset($collectionData[$key]); + $inventory[$collection] = $collectionData; + $this->writeInventory($inventory); } + // + // Private + // ------------------------------------------------------------------------------- + + // + // Collection Validation + /** - * Check if a key exists in a collection. + * Check if a collection key exists. + * + * @param array $inventory */ - public function has(string $collection, string $key): bool + private function collectionKeyExists(array $inventory, string $collection, string $key): bool { - $inventory = $this->readInventory(); - return isset($inventory[$collection]) && is_array($inventory[$collection]) && array_key_exists($key, $inventory[$collection]); } - // - // DELETE Operations - // ---- - /** - * Delete a single record from a collection. + * Ensure a collection exists and is properly initialized. + * + * @param array $inventory */ - public function delete(string $collection, string $key, bool $mustExist = true): void + private function ensureCollection(array &$inventory, string $collection): void { - $inventory = $this->readInventory(); + if (!isset($inventory[$collection]) || !is_array($inventory[$collection])) { + $inventory[$collection] = []; + } + } - if (!isset($inventory[$collection]) || !is_array($inventory[$collection]) || !array_key_exists($key, $inventory[$collection])) { - if ($mustExist) { - throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); - } + /** + * Throw consistent key not found exception. + */ + private function throwKeyNotFound(string $collection, string $key): never + { + throw new \RuntimeException("Key '{$key}' not found in collection '{$collection}'."); + } - return; + /** + * Validate that a collection key exists, throw exception if not. + * + * @param array $inventory + */ + private function validateCollectionKey(array $inventory, string $collection, string $key): void + { + if (!$this->collectionKeyExists($inventory, $collection, $key)) { + $this->throwKeyNotFound($collection, $key); } - - unset($inventory[$collection][$key]); - $this->writeInventory($inventory); } // - // Private Helper Methods - // ---- + // File Operations + + private function getInventoryPath(): string + { + return rtrim((string) getcwd(), '/').'/.deployer/inventory.yml'; + } /** * Read inventory YAML into a structured array. @@ -174,9 +230,4 @@ private function writeInventory(array $inventory): void throw new \RuntimeException("Failed to write inventory file at {$path}", 0, $e); } } - - private function getInventoryPath(): string - { - return rtrim((string) getcwd(), '/').'/.deployer/inventory.yml'; - } } From cbc351f6406a48c0bfe16f7c42f3d9443c659b55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Wed, 24 Sep 2025 09:13:27 +0300 Subject: [PATCH 5/7] fixup --- .github/workflows/review.yml | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 0da4415e..c8f871a1 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -19,17 +19,21 @@ jobs: - name: Checkout code uses: actions/checkout@v4 - - name: Install Cursor CLI + - name: Install opencode CLI run: | - curl https://cursor.com/install -fsS | bash - echo "$HOME/.cursor/bin" >> $GITHUB_PATH + curl -fsSL https://opencode.ai/install | bash + echo "$HOME/.opencode/bin" >> $GITHUB_PATH - name: Code review env: - CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} + OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} GH_TOKEN: ${{ github.token }} run: | - cursor-agent --force --model "x-ai/grok-4-fast:free" --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. + cd ${{ github.workspace }} + opencode --model "x-ai/grok-4-fast:free" --output-format=text << 'EOF' + 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. Context: - Repo: ${{ github.repository }} @@ -56,4 +60,5 @@ jobs: Submission: - Use only: gh pr review --comment - Do not use: gh pr review --approve or --request-changes - - Submit one review containing inline comments plus a concise summary" + - Submit one review containing inline comments plus a concise summary + EOF From bbefd914bb3ad0d1f2b28fe3ddafbe17cbbe9ab2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Wed, 24 Sep 2025 09:25:03 +0300 Subject: [PATCH 6/7] fixup --- .github/workflows/review.yml | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index c8f871a1..06bea556 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -19,21 +19,17 @@ jobs: - name: Checkout code uses: actions/checkout@v4 - - name: Install opencode CLI + - name: Install Cursor CLI run: | - curl -fsSL https://opencode.ai/install | bash - echo "$HOME/.opencode/bin" >> $GITHUB_PATH + curl https://cursor.com/install -fsS | bash + echo "$HOME/.cursor/bin" >> $GITHUB_PATH - name: Code review env: - OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} + CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} GH_TOKEN: ${{ github.token }} run: | - cd ${{ github.workspace }} - opencode --model "x-ai/grok-4-fast:free" --output-format=text << 'EOF' - 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 "claude-4-sonnet" --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. Context: - Repo: ${{ github.repository }} @@ -60,5 +56,4 @@ jobs: Submission: - Use only: gh pr review --comment - Do not use: gh pr review --approve or --request-changes - - Submit one review containing inline comments plus a concise summary - EOF + - Submit one review containing inline comments plus a concise summary" From 0a24c8e6a2499a3c9970ea148e12bdc278f0fc6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucian=20V=C4=83c=C4=83roiu?= Date: Wed, 24 Sep 2025 09:34:51 +0300 Subject: [PATCH 7/7] fixup --- .github/workflows/review.yml | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 06bea556..28100137 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -29,7 +29,10 @@ jobs: CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} GH_TOKEN: ${{ github.token }} run: | - cursor-agent --force --model "claude-4-sonnet" --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 "claude-4-sonnet" --output-format=text --print << 'EOF' + You are operating in a GitHub Actions runner performing a very thorough automated code review. + + The gh CLI is available and authenticated via GH_TOKEN. You may comment on pull requests. Context: - Repo: ${{ github.repository }} @@ -37,23 +40,28 @@ jobs: - PR Head SHA: ${{ github.event.pull_request.head.sha }} - PR Base SHA: ${{ github.event.pull_request.base.sha }} + # Instructions: + Review the current PR diff and meticulously catalog all the changes in this branch. Report back on where the changes fall short of our development, architecture and testing rules. - Leave very short inline comments (1-2 sentences) on suggested changes and a brief summary at the end. + # Procedure: + + Leave short inline comments (1-2 sentences) on suggested changes and a brief summary at the end. - Procedure: - - Get existing comments: gh pr view --json comments - Get diff: gh pr diff + - Get existing comments: gh pr view --json comments - If a previously reported issue appears fixed by nearby changes, reply: ✅ This issue appears to be resolved by the recent changes - Avoid duplicates: skip if similar feedback already exists on or near the same lines - Commenting rules: + **Commenting rules:** + - Natural tone, specific and actionable; do not mention automated or high-confidence - Use emojis: 🚨 Critical 🔒 Security ⚡ Performance ⚠️ Logic ✅ Resolved ✨ Improvement Submission: - Use only: gh pr review --comment - Do not use: gh pr review --approve or --request-changes - - Submit one review containing inline comments plus a concise summary" + - Submit one review containing inline comments plus a concise summary + EOF