feat: inventory crud - #16
Conversation
- Remove monolithic analyze.md command - Add _analyze.md for general analysis functionality - Add _in-branch.md for branch-specific analysis - Add _in-diff.md for working tree analysis - Add report.md for reporting without changes This modular approach allows for more targeted analysis commands and better separation of concerns in cursor automation.
- Add 'meticulously catalog and analyze' prefix to create-branch.md - Add 'meticulously catalog and analyze' prefix to create-commits.md - Clarify that commands should analyze both staged and unstaged changes This provides clearer instructions for more thorough analysis when creating branches and commits.
Update testing exception rule to allow direct instantiation in tests rather than only Container instantiation. This provides clearer guidance that tests can use direct instantiation for better mocking and isolation while production code must use App::build().
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (7)
Tip CodeRabbit can use your project's PHP CodeSniffer (phpcs) configuration to improve the quality of PHP code reviews.Add a WalkthroughAdds a new YAML-backed InventoryService with dot-path CRUD, accompanying comprehensive unit tests. Updates Cursor command docs and architecture rule wording. Minor whitespace tweak in VersionService test. Introduces a new report command doc and small instruction refinements in several Cursor command files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant InventoryService
participant Filesystem
participant YAML as Yaml Parser
rect rgb(240,248,255)
note over Client,InventoryService: Get value by dot-path
Client->>InventoryService: get(path)
InventoryService->>Filesystem: read ~/.deployer/inventory.yml
alt file exists
Filesystem-->>InventoryService: contents
InventoryService->>YAML: parse(contents)
YAML-->>InventoryService: inventory array
InventoryService-->>Client: value or null
else missing file
InventoryService-->>Client: null or []
end
end
rect rgb(245,255,240)
note over Client,InventoryService: Set value by dot-path
Client->>InventoryService: set(path, value)
InventoryService->>Filesystem: read ~/.deployer/inventory.yml (optional)
InventoryService->>YAML: parse(existing or empty)
YAML-->>InventoryService: inventory array
InventoryService->>InventoryService: upsert nested value
InventoryService->>Filesystem: ensure ~/.deployer exists
InventoryService->>YAML: dump(inventory)
YAML-->>InventoryService: yaml string
InventoryService->>Filesystem: write inventory.yml
InventoryService-->>Client: void
end
rect rgb(255,245,245)
note over InventoryService,Filesystem: Error handling
Filesystem--x InventoryService: mkdir/write failure
InventoryService-->>Client: throw RuntimeException
YAML--x InventoryService: parse failure
InventoryService-->>Client: throw ParseException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.cursor/commands/_analyze.md(1 hunks).cursor/commands/_in-branch.md(1 hunks).cursor/commands/_in-diff.md(1 hunks).cursor/commands/analyze.md(0 hunks).cursor/commands/create-branch.md(1 hunks).cursor/commands/create-commits.md(1 hunks).cursor/commands/report.md(1 hunks).cursor/rules/01-architecture.mdc(1 hunks)app/Services/InventoryService.php(1 hunks)tests/Unit/InventoryServiceTest.php(1 hunks)tests/Unit/VersionServiceTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- tests/Unit/VersionServiceTest.php
- .cursor/commands/analyze.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Organize code by grouping related functions into comment-separated sections; prefer alphabetical ordering when it doesn’t conflict with logical grouping
Files:
tests/Unit/InventoryServiceTest.phpapp/Services/InventoryService.php
tests/**
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not write or run tests unless specifically instructed
Files:
tests/Unit/InventoryServiceTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively for PHP tests with it() syntax.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic only; do not test the framework.
Use dataset-driven testing (->with([...])) for multiple scenarios.
Consolidate related assertions using expect(...)->and(...).
Mock only external dependencies; avoid mocking internal logic.
Do not write performance tests unless performance is the primary concern.
Do not consolidate tests when covering different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate when setup requirements differ.
Do not consolidate when testing distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in tests.
For exception tests, use a combined // ACT & ASSERT section when the act triggers the assertion.
Organize tests using describe() blocks, beforeEach() setup, and extracted helpers/traits for DRY.
Avoid type-only and generic assertions (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)).
Avoid sleep(...); use time mocking instead.
Write meaningful assertions tied to behavior and outcomes (e.g., checking config values, validator results, and mock expectations).
Unit tests must mock all external dependencies, test single units in isolation, and complete in milliseconds.
Integration tests should use real file operations and external processes; cover CLI commands and full workflows.
Ignore PHPStan issues in tests; prioritize test functionality over static analysis compliance.
Avoid excessive PHPDoc in tests added solely to appease types.In tests, direct Container instantiation is allowed to isolate state (new Container())
Files:
tests/Unit/InventoryServiceTest.php
{app,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
{app,tests}/**/*.php: Follow PSR-12, declare strict_types, and target PHP 8.x features (unions, match, attributes, readonly) in all PHP files
Use import statements (use Foo\Bar;) instead of fully qualified class names in code
All methods must declare explicit return types, using proper generics in docblocks (e.g., Collection<int, User>)
Add concise DocBlock comments for classes and functions including descriptions, parameters, and return types
Use comments as visual separators for sections/subsections/paragraphs with the prescribed formatting and spacing
Always use the full section header comment format with the correct dash line length (do not use simplified form)
Files:
tests/Unit/InventoryServiceTest.phpapp/Services/InventoryService.php
app/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/*.php: Use App::build(ClassName::class) for object creation instead of new, except for value objects/DTOs/pure data structures
Prefer Symfony helper classes (e.g., Filesystem, Process) over native PHP functions for easier mocking
Commands handle user interaction and orchestration; they must not contain business logic
Commands must not duplicate orchestration logic; extract shared orchestration into Services
Commands should not invoke other commands (no proxy commands)
Use SymfonyStyle consistently for all user-facing console output
Services provide atomic, reusable functionality and must be stateless and dependency-injected
Services perform business logic, external API calls, and file operations with no console I/O
Services accept and return plain PHP data types
Only Commands perform console input/output operations; Services must not perform I/O
Declare all dependencies in constructor signatures (constructor injection)
No circular dependencies between services/commands
Files:
app/Services/InventoryService.php
🧠 Learnings (2)
📚 Learning: 2025-09-24T19:26:21.229Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-24T19:26:21.229Z
Learning: Applies to tests/**/*.php : In tests, direct Container instantiation is allowed to isolate state (new Container())
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-24T19:26:21.229Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-24T19:26:21.229Z
Learning: Applies to app/**/*.php : Use App::build(ClassName::class) for object creation instead of new, except for value objects/DTOs/pure data structures
Applied to files:
.cursor/rules/01-architecture.mdc
🧬 Code graph analysis (2)
tests/Unit/InventoryServiceTest.php (1)
app/Services/InventoryService.php (5)
InventoryService(37-270)set(56-63)has(89-95)delete(100-107)getAll(81-84)
app/Services/InventoryService.php (1)
tests/Unit/InventoryServiceTest.php (2)
mkdir(37-42)dumpFile(44-49)
🪛 PHPMD (2.15.0)
tests/Unit/InventoryServiceTest.php
27-27: Avoid unused parameters such as '$files'. (undefined)
(UnusedFormalParameter)
37-37: Avoid unused parameters such as '$dirs'. (undefined)
(UnusedFormalParameter)
37-37: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
44-44: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
44-44: Avoid unused parameters such as '$content'. (undefined)
(UnusedFormalParameter)
🪛 GitHub Actions: Pest
tests/Unit/InventoryServiceTest.php
[error] 16-16: PHP Fatal error: Cannot redeclare function mockFilesystem() (previously declared in EnvServiceTest.php:15) during 'vendor/bin/pest --parallel --coverage'.
🪛 GitHub Actions: Rector
tests/Unit/InventoryServiceTest.php
[error] 17-17: vendor/bin/rector --dry-run: ReadOnlyPropertyRector would modify property declarations to 'readonly' in tests/Unit/InventoryServiceTest.php (line 17) — private to private readonly. Exit code 2.
| /** | ||
| * Set a value using dot notation path. | ||
| */ | ||
| public function set(string $path, mixed $value): void | ||
| { | ||
| $inventory = $this->readInventory(); | ||
| $segments = $this->parsePath($path); | ||
|
|
||
| $this->setByPath($inventory, $segments, $value); | ||
| $this->writeInventory($inventory); | ||
| } | ||
|
|
||
| /** | ||
| * Get a value using dot notation path. | ||
| */ | ||
| public function get(string $path): mixed | ||
| { | ||
| $inventory = $this->readInventory(); | ||
| $segments = $this->parsePath($path); | ||
|
|
||
| return $this->getByPath($inventory, $segments); | ||
| } | ||
|
|
||
| /** | ||
| * Get the entire inventory structure. | ||
| * | ||
| * @return array<string, mixed> | ||
| */ | ||
| public function getAll(): array | ||
| { | ||
| return $this->readInventory(); | ||
| } | ||
|
|
||
| /** | ||
| * Check if a path exists using dot notation. | ||
| */ | ||
| public function has(string $path): bool | ||
| { | ||
| $inventory = $this->readInventory(); | ||
| $segments = $this->parsePath($path); | ||
|
|
||
| return $this->hasByPath($inventory, $segments); | ||
| } | ||
|
|
||
| /** | ||
| * Delete a value using dot notation path. | ||
| */ | ||
| public function delete(string $path): void | ||
| { | ||
| $inventory = $this->readInventory(); | ||
| $segments = $this->parsePath($path); | ||
|
|
||
| $this->unsetByPath($inventory, $segments); | ||
| $this->writeInventory($inventory); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Docblocks need parameter/return tags per repo rules.
Per the repository’s PHP guidelines, every method docblock must include the parameter and return annotations. All of the public methods here are missing those tags. Please add the required annotations across the class.
As per coding guidelines
Here’s an example of the expected format:
/**
* Set a value using dot notation path.
+ *
+ * @param string $path
+ * @param mixed $value
+ *
+ * @return void
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Set a value using dot notation path. | |
| */ | |
| public function set(string $path, mixed $value): void | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| $this->setByPath($inventory, $segments, $value); | |
| $this->writeInventory($inventory); | |
| } | |
| /** | |
| * Get a value using dot notation path. | |
| */ | |
| public function get(string $path): mixed | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| return $this->getByPath($inventory, $segments); | |
| } | |
| /** | |
| * Get the entire inventory structure. | |
| * | |
| * @return array<string, mixed> | |
| */ | |
| public function getAll(): array | |
| { | |
| return $this->readInventory(); | |
| } | |
| /** | |
| * Check if a path exists using dot notation. | |
| */ | |
| public function has(string $path): bool | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| return $this->hasByPath($inventory, $segments); | |
| } | |
| /** | |
| * Delete a value using dot notation path. | |
| */ | |
| public function delete(string $path): void | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| $this->unsetByPath($inventory, $segments); | |
| $this->writeInventory($inventory); | |
| } | |
| /** | |
| * Set a value using dot notation path. | |
| * | |
| * @param string $path | |
| * @param mixed $value | |
| * | |
| * @return void | |
| */ | |
| public function set(string $path, mixed $value): void | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| $this->setByPath($inventory, $segments, $value); | |
| $this->writeInventory($inventory); | |
| } | |
| /** | |
| * Get a value using dot notation path. | |
| */ | |
| public function get(string $path): mixed | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| return $this->getByPath($inventory, $segments); | |
| } | |
| /** | |
| * Get the entire inventory structure. | |
| * | |
| * @return array<string, mixed> | |
| */ | |
| public function getAll(): array | |
| { | |
| return $this->readInventory(); | |
| } | |
| /** | |
| * Check if a path exists using dot notation. | |
| */ | |
| public function has(string $path): bool | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| return $this->hasByPath($inventory, $segments); | |
| } | |
| /** | |
| * Delete a value using dot notation path. | |
| */ | |
| public function delete(string $path): void | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| $this->unsetByPath($inventory, $segments); | |
| $this->writeInventory($inventory); | |
| } |
🤖 Prompt for AI Agents
In app/Services/InventoryService.php around lines 53 to 107, the public method
docblocks for set, get, getAll, has, and delete are missing @param/@return
annotations; update each docblock to include the parameter and return tags per
repo rules (set: @param string $path, @param mixed $value, @return void; get:
@param string $path, @return mixed; getAll: @return array<string,mixed>; has:
@param string $path, @return bool; delete: @param string $path, @return void),
keeping descriptions concise and matching existing docblock style.
| $raw = $this->filesystem->readFile($path); | ||
| $parsed = Yaml::parse($raw); | ||
|
|
||
| /** @var array<string, mixed> $result */ | ||
| $result = is_array($parsed) ? $parsed : []; | ||
| return $result; | ||
| } |
There was a problem hiding this comment.
Undefined Filesystem::readFile call will fatal at runtime.
Symfony\Component\Filesystem\Filesystem does not provide a readFile() method, so this call will immediately trigger “Call to undefined method Filesystem::readFile()” in production. Please switch to a supported way of reading the file (e.g., wrap file_get_contents with proper error handling) before parsing the YAML.
Apply this diff to fix the problem:
- $raw = $this->filesystem->readFile($path);
+ $raw = @file_get_contents($path);
+ if ($raw === false) {
+ throw new \RuntimeException("Failed to read inventory file at {$path}");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $raw = $this->filesystem->readFile($path); | |
| $parsed = Yaml::parse($raw); | |
| /** @var array<string, mixed> $result */ | |
| $result = is_array($parsed) ? $parsed : []; | |
| return $result; | |
| } | |
| $raw = @file_get_contents($path); | |
| if ($raw === false) { | |
| throw new \RuntimeException("Failed to read inventory file at {$path}"); | |
| } | |
| $parsed = Yaml::parse($raw); | |
| /** @var array<string, mixed> $result */ | |
| $result = is_array($parsed) ? $parsed : []; | |
| return $result; | |
| } |
🤖 Prompt for AI Agents
In app/Services/InventoryService.php around lines 238 to 244, the code calls
$this->filesystem->readFile($path) which does not exist on Symfony's Filesystem
and will fatal; replace that call with a safe file read: check file_exists and
is_readable($path) (or use @file_get_contents with error check), retrieve the
file contents into a variable, throw or log a descriptive error if reading
fails, only pass non-false content to Yaml::parse and guard that parse result is
an array before returning it (otherwise return an empty array); ensure any
exceptions from YAML parsing are caught or allowed to bubble with context so
runtime failures are handled cleanly.
| return new class ($exists, $yamlContent, $throwOnMkdir, $throwOnDump) extends Filesystem { | ||
| public function __construct( | ||
| private bool $exists, | ||
| private string $yamlContent, | ||
| private bool $throwOnMkdir, | ||
| private bool $throwOnDump | ||
| ) { |
There was a problem hiding this comment.
Mark mock filesystem properties as readonly
Rector fails because these constructor-promoted properties are never mutated after instantiation. Declare them readonly to satisfy the code-style rule and unblock the Rector check.
- private bool $exists,
- private string $yamlContent,
- private bool $throwOnMkdir,
- private bool $throwOnDump
+ private readonly bool $exists,
+ private readonly string $yamlContent,
+ private readonly bool $throwOnMkdir,
+ private readonly bool $throwOnDump📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new class ($exists, $yamlContent, $throwOnMkdir, $throwOnDump) extends Filesystem { | |
| public function __construct( | |
| private bool $exists, | |
| private string $yamlContent, | |
| private bool $throwOnMkdir, | |
| private bool $throwOnDump | |
| ) { | |
| return new class ($exists, $yamlContent, $throwOnMkdir, $throwOnDump) extends Filesystem { | |
| public function __construct( | |
| private readonly bool $exists, | |
| private readonly string $yamlContent, | |
| private readonly bool $throwOnMkdir, | |
| private readonly bool $throwOnDump | |
| ) { |
🤖 Prompt for AI Agents
In tests/Unit/InventoryServiceTest.php around lines 18 to 24, the anonymous
class constructor promotes properties that are never mutated; update the
constructor-promoted properties to be readonly by adding the readonly modifier
to each promoted parameter (bool $exists, string $yamlContent, bool
$throwOnMkdir, bool $throwOnDump) so they become readonly properties and satisfy
the Rector code-style rule.
Eliminates fatal 'Cannot redeclare function mockFilesystem()' error by: - Creating unified mockFilesystem() in tests/TestHelpers.php with comprehensive error simulation (throwOnRead, throwOnMkdir, throwOnDump parameters) - Removing duplicate implementations from EnvServiceTest.php and InventoryServiceTest.php - Maintaining backward compatibility for EnvServiceTest calls - Adding throwOnRead=false parameter to InventoryServiceTest calls - Supporting all required Filesystem methods: exists(), readFile(), mkdir(), dumpFile() All 112 tests now pass with 92.8% coverage.
- Replace report.md/review.md with _report.md/_review.md - Update _analyze.md capitalization for consistency - Refine command descriptions for clearer intent
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
Chores