-
Notifications
You must be signed in to change notification settings - Fork 0
chore: reorganize cursor rules and add review workflow #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "permissions": { | ||
| "allow": [], | ||
| "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| 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++ 🚀 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,27 @@ | ||
| --- | ||
| 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; | ||
| - **Type Safety:** All methods must have explicit return types with proper generics (e.g., `Collection<int, User>`). | ||
| - **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<int, User>`) | ||
|
|
||
| **🚨 Architecture rules are IMMUTABLE - fix violating code, not the architecture rules** | ||
|
|
||
| ### 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.) | ||
|
|
||
|
Comment on lines
+14
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainSymfony dependencies: verify availability or scope the rule. Mandating Symfony wrappers over natives requires symfony/console, symfony/filesystem, symfony/process in composer.json. Either verify they exist or soften the rule to “prefer when available.” 🏁 Script executed: #!/bin/bash
# Verify required packages exist
jq -r '.require, .["require-dev"] | keys[]' composer.json 2>/dev/null | rg -n '^(symfony/(console|filesystem|process))$' -n -C0 || trueLength of output: 38 Require symfony/process or relax the "Symfony Patterns" rule composer.json contains symfony/console and symfony/filesystem but not symfony/process — either add symfony/process to composer.json or change .cursor/rules/01-architecture.mdc (lines 14–18) to "prefer Symfony wrappers when available" instead of mandating them. 🤖 Prompt for AI Agents |
||
| ### Dependency Injection | ||
|
|
||
| - 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 | ||
|
|
||
|
Comment on lines
+21
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainContainer existence and DI feasibility. The rule depends on an app/Container class. Verify it exists and is wired; otherwise this becomes aspirational and will cause drift. 🏁 Script executed: #!/bin/bash
# Look for a Container class in app/
fd -a 'Container.php' app/ | xargs -r -I{} rg -nP '^\s*class\s+Container\b' {}Length of output: 0 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Search: class Container definitions ==="
rg -nP --hidden --follow -S '\bclass\s+Container\b' || true
echo "=== Search: files named Container.php ==="
find . -type f -iname 'Container.php' -print || true
echo "=== Search: literal 'app/Container' occurrences ==="
rg -n --hidden --follow -S 'app/Container' || true
echo "=== Search: namespaced App\\Container occurrences ==="
rg -n --hidden --follow -S 'App\\\\Container' || true
echo "=== composer.json (if present) ==="
if [ -f composer.json ]; then sed -n '1,240p' composer.json; else echo "no composer.json"; fi
echo "=== composer.lock (scan for DI packages) ==="
if [ -f composer.lock ]; then rg -n --hidden --follow -S 'php-di|illuminate/container|symfony/component/dependency-injection|pimple' composer.lock || true; else echo "no composer.lock"; fiLength of output: 2170 app/Container referenced in rule but missing — implement it or update the rule The rule (.cursor/rules/01-architecture.mdc:21-24) claims dependencies are injected by app/Container, but the repo contains no app/Container class or Container.php; this makes the rule aspirational and will cause drift.
|
||
| ### Command Layer | ||
|
|
||
| - Commands handle user interaction (input/output) and orchestrate Services | ||
|
|
@@ -26,31 +38,58 @@ 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. | ||
|
|
||
| ### 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 | ||
|
|
||
| **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:** | ||
|
|
||
| ```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 | ||
| ``` | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| --- | ||
| globs: app/**/*.php,tests/**/*.php | ||
| --- | ||
|
|
||
| ## Testing Rules | ||
|
|
||
| **Philosophy:** "A test that never fails is not a test, it's a lie." | ||
|
|
||
| **Framework:** Pest exclusively with `it()` syntax, 80%+ coverage | ||
|
|
||
| ### Running Tests | ||
|
|
||
| - `composer pest` - run entire test suite in parallel, with coverage | ||
| - `vendor/bin/pest $TEST_FILE` - run specific test file | ||
|
|
||
| ### Test Minimalism | ||
|
|
||
| **Target:** Keep test files under 1.8x the size of source code they test. | ||
|
|
||
| **Rules:** | ||
|
|
||
| - 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 | ||
|
|
||
| **Don't consolidate when:** | ||
|
|
||
| - Different public methods | ||
| - Exception vs normal flow tests | ||
| - Different setup requirements | ||
| - Distinct business logic | ||
|
|
||
| ### AAA Pattern (MANDATORY) | ||
|
|
||
| ```php | ||
| it('does something specific', function () { | ||
| // ARRANGE | ||
| $service = new Service(mock(Dependency::class)); | ||
|
|
||
| // ACT | ||
| $result = $service->performAction(); | ||
|
|
||
| // ASSERT | ||
| expect($result)->toBe('expected'); | ||
|
|
||
| // CLEANUP (when needed) | ||
| $this->resetTimeState(); | ||
| unlink($tempFile); | ||
| }); | ||
| ``` | ||
|
|
||
| **Exception tests:** Use `// ACT & ASSERT` when act triggers assertion. | ||
|
|
||
| **Organization:** Use `describe()` blocks, `beforeEach()` setup, extract helpers/traits for DRY tests. | ||
|
|
||
| ### Testing Patterns | ||
|
|
||
| **❌ FORBIDDEN:** | ||
|
|
||
| ```php | ||
| expect($x)->toBeInstanceOf(Class::class); // Type-only testing | ||
| expect($x)->toBeArray(); // Generic assertions | ||
| expect($x)->not->toBeNull(); // Meaningless | ||
| expect(true)->toBeTrue(); // Literally meaningless | ||
| sleep(...); // Use time mocking | ||
| ``` | ||
|
|
||
| **✅ REQUIRED:** | ||
|
|
||
| ```php | ||
| expect($config->getValue('host'))->toBe('example.com'); | ||
| expect($this->validator->isValid($input))->toBe($expected); | ||
| $mock->shouldReceive('method')->with('param')->andReturn('result'); | ||
| ``` | ||
|
|
||
| ### Test Types | ||
|
|
||
| **Unit Tests:** | ||
|
|
||
| - Mock all external dependencies (filesystem, HTTP, processes) | ||
| - Test single units in isolation | ||
| - Complete in milliseconds | ||
|
|
||
| **Integration Tests:** | ||
|
|
||
| - Real file operations and external processes | ||
| - CLI commands and full workflows | ||
|
|
||
| **Layer Strategy:** | ||
|
|
||
| - CLI Commands → Integration tests | ||
| - Business Services → Unit tests (mocked dependencies) | ||
| - Utilities/Helpers → Unit tests | ||
|
|
||
| ### Static Analysis | ||
|
|
||
| - Ignore PHPStan issues in tests - focus on test functionality over compliance | ||
| - Avoid excessive phpdoc just to appease types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
🧩 Analysis chain
Rules conflict: testing stance here vs. tests rules file.
This doc says “Don’t worry about tests,” while .cursor/rules/02-tests.mdc mandates Pest and 60%+ coverage. Please reconcile and make a single source of truth.
Would you like me to propose a unified “When to write tests” section that aligns both files?
🏁 Script executed:
Length of output: 782
🏁 Script executed:
Length of output: 7024
Conflict: testing guidance is inconsistent — reconcile test policy
00-main.mdc says tests are optional while 02-tests.mdc mandates Pest + 60%+ coverage. Consolidate to a single source of truth.
it()syntax, 60%+ coverage"Fix: remove or clarify the contradictory line in 00-main.mdc or update it to reference 02-tests.mdc (or move all testing policy into a single file). I can draft a unified "When to write tests" section that aligns both files.
🤖 Prompt for AI Agents