chore: reorganize cursor rules and add review workflow - #12
Conversation
WalkthroughAdds Cursor CLI config and command stubs, restructures rule documents (adds/edits main, architecture, and tests docs; removes legacy code-quality/tests docs), and adds a GitHub Actions workflow that runs an automated PR review via cursor-agent. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant WF as Review Workflow
participant CO as Checkout
participant CLI as Cursor CLI
participant AG as cursor-agent
participant GHCL as gh CLI
Dev->>GH: Open or update PR
GH-->>WF: Trigger workflow on pull_request
activate WF
WF->>CO: actions/checkout@v4
WF->>CLI: Install Cursor CLI (curl installer, add to PATH)
WF->>AG: Run cursor-agent (fetch diff, fetch PR comments, generate inline feedback + summary)
AG-->>WF: Inline comments + summary
WF->>GHCL: gh pr review --comment (post feedback)
note over WF: Concurrency control (cancel in-progress)<br/>Job timeout: 9 minutes
deactivate WF
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Code Review Summary
This PR introduces comprehensive Cursor AI configuration and development workflow rules. The changes are well-structured overall but have several areas for improvement:
✨ Positive Changes:
- Comprehensive development rules with minimalist philosophy
- Strong architecture patterns with dependency injection
- Automated code review workflow integration
- Clear testing guidelines with Pest framework
⚠️ Issues Found:
Critical Issues:
- 🚨 Missing tests directory structure - no test files exist despite comprehensive testing rules
- 🚨 No Container/DI implementation referenced in architecture rules
- ⚡ Security concern: direct shell execution in Deployer.php without proper sanitization
Architecture Violations:
- 🔒 EnvService uses manual file operations instead of Symfony Filesystem
⚠️ Missing explicit return type documentation in some methods- ✨ Inconsistent comment formatting - some sections don't follow the librarian organization style
Rule Consistency:
- 📝 Quality gates reference files that may not exist ($CHANGED_PHP_FILES)
- 🔄 GitHub workflow timeout is very short (3 minutes) for thorough reviews
📊 Compliance Score: 7/10
The codebase shows good structure and follows most PSR-12 standards, but needs work on dependency injection implementation and testing infrastructure.
🎯 Next Steps:
- Implement the Container/DI system referenced in rules
- Add test files to match the comprehensive testing guidelines
- Replace direct file operations with Symfony components
- Add proper input sanitization for shell commands
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
.cursor/commands/refactor.md (1)
1-1: Tone and clarity: remove hype, add a clear imperative.Consider a neutral, actionable instruction instead of conversational/hype language.
-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++ 🚀 +Refactor to the minimal viable implementation, then organize and catalog code consistently (grouped sections, clear names). Prioritize uniformity and clarity across the codebase..cursor/rules/00-main.mdc (1)
23-26: Alphabetical ordering: scope or soften to avoid harming logical proximity.Strict alphabetical ordering can degrade readability when execution flow or cohesion matters; recommend “prefer alphabetical when it doesn’t fight logical grouping” rather than unconditional.
.cursor/rules/02-tests.mdc (2)
9-15: Enforce coverage in CI.You require Pest and 60%+ coverage but the repo lacks a CI job to run tests/coverage. Add a workflow (or extend an existing one) to run “composer pest -- --coverage” and fail below threshold.
I can draft a GitHub Actions job that runs Pest with coverage and uploads the report.
18-29: Automation for the 1.8x size target.The ratio is useful but non‑actionable without tooling. Consider a lightweight check that compares LOC of tests vs. sources and emits a warning when >1.8x.
I can provide a script to compute LOC ratios per class/namespace and report offenders.
.cursor/rules/01-architecture.mdc (1)
2-3: Avoid applying architecture rules to tests.Including tests in globs may force service/command layering and comments onto test files. Limit globs to app/**/*.php, and keep test rules in 02-tests.mdc.
-globs: app/**/*.php,tests/**/*.php +globs: app/**/*.php.cursor/cli.json (1)
1-6: Clarify allowlist to avoid accidental denials.With an empty allow list, behavior may default‑deny. Explicitly allow the gh commands used by the workflow to prevent breakage.
- "allow": [], + "allow": ["Shell(gh pr diff)", "Shell(gh pr view --json comments)", "Shell(gh pr review --comment)"], "deny": ["Shell(git push)", "Shell(gh pr create)", "Write(**)"]Please confirm the CLI permission semantics; if default‑deny, this change is required for the review job to function.
.cursor/commands/review.md (1)
1-1: Remove “uncommitted changes” phrasing for CI context.Actions runners check out a clean ref; there are no uncommitted changes. Rephrase to avoid unreachable asks.
-Analyze and meticulously catalog all the changes in this branch, including all the changes that haven't been committed yet. +Analyze and meticulously catalog all changes in this branch (PR diff and current head)..github/workflows/review.yml (3)
13-17: Timeout likely too strict.3 minutes is brittle for large PRs or slow API responses. Recommend 10–15 minutes.
- timeout-minutes: 3 + timeout-minutes: 10
27-67: Skip forks and drafts to prevent secret-related failures.Guard the job to run only on same‑repo PRs and non‑drafts; secrets are unavailable on forks and the run will fail.
jobs: review: + if: ${{ github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository }} runs-on: ubuntu-latest
32-32: Externalize the model as a variable.Avoid hardcoding the model; make it configurable via a repo variable.
- cursor-agent --force --model "claude-4-sonnet" --output-format=text --print << 'EOF' + cursor-agent --force --model "${CURSOR_MODEL:-claude-4-sonnet}" --output-format=text --print << 'EOF'Add at step/env or job/env:
env: CURSOR_MODEL: ${{ vars.CURSOR_MODEL }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.cursor/cli.json(1 hunks).cursor/commands/refactor.md(1 hunks).cursor/commands/review.md(1 hunks).cursor/rules/00-main.mdc(2 hunks).cursor/rules/01-architecture.mdc(2 hunks).cursor/rules/02-code-quality.mdc(0 hunks).cursor/rules/02-tests.mdc(1 hunks).cursor/rules/03-tests.mdc(0 hunks).github/workflows/review.yml(1 hunks)
💤 Files with no reviewable changes (2)
- .cursor/rules/03-tests.mdc
- .cursor/rules/02-code-quality.mdc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: review
🔇 Additional comments (1)
.cursor/rules/01-architecture.mdc (1)
85-95: No change needed — required dev tools are present.composer.json (require-dev) includes laravel/pint, phpstan/phpstan, and rector/rector.
| **🤖 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. | ||
|
|
There was a problem hiding this comment.
🛠️ 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:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Listing .cursor directory (if present)"
ls -la .cursor || true
echo
echo "Listing .cursor/rules directory (if present)"
ls -la .cursor/rules || true
echo
echo "Show .cursor/rules/00-main.mdc (up to 300 lines):"
if [ -f .cursor/rules/00-main.mdc ]; then
nl -ba -w3 -s': ' .cursor/rules/00-main.mdc | sed -n '1,300p'
else
echo "File .cursor/rules/00-main.mdc not found"
fi
echo
echo "Show .cursor/rules/02-tests.mdc (up to 300 lines):"
if [ -f .cursor/rules/02-tests.mdc ]; then
nl -ba -w3 -s': ' .cursor/rules/02-tests.mdc | sed -n '1,300p'
else
echo "File .cursor/rules/02-tests.mdc not found"
fi
echo
echo "Search repo for mentions of Pest, coverage, and 'don't worry about tests' phrases:"
rg -n --hidden --no-ignore-vcs -S "Pest" || true
rg -n --hidden --no-ignore-vcs -S "coverage" || true
rg -n --hidden --no-ignore-vcs -S "don't worry about tests" || true
rg -n --hidden --no-ignore-vcs -S "don’t worry" || true
rg -n --hidden --no-ignore-vcs -S "When to write tests" || trueLength of output: 782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "Files in .cursor/rules:"
ls -la .cursor/rules || true
echo
echo "----- .cursor/rules/00-main.mdc (numbered) -----"
if [ -f .cursor/rules/00-main.mdc ]; then
awk '{printf "%4d: %s\n", NR, $0}' .cursor/rules/00-main.mdc | sed -n '1,300p'
else
echo "missing: .cursor/rules/00-main.mdc"
fi
echo
echo "----- Lines 27-34 of .cursor/rules/00-main.mdc -----"
if [ -f .cursor/rules/00-main.mdc ]; then
awk 'NR>=27 && NR<=34 {printf "%4d: %s\n", NR, $0}' .cursor/rules/00-main.mdc || true
fi
echo
echo "----- .cursor/rules/02-tests.mdc (numbered) -----"
if [ -f .cursor/rules/02-tests.mdc ]; then
awk '{printf "%4d: %s\n", NR, $0}' .cursor/rules/02-tests.mdc | sed -n '1,400p'
else
echo "missing: .cursor/rules/02-tests.mdc"
fi
echo
echo "----- Search for key phrases in both files -----"
grep -nhiE "Pest|coverage|don't worry about tests|don't worry|When to write tests|Don’t worry|write tests" .cursor/rules/*.mdc || true
echo "done"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.
- .cursor/rules/00-main.mdc — line 35: "✔️ Don't worry about tests: Write or run tests ONLY if specifically instructed"
- .cursor/rules/02-tests.mdc — line 9: "Framework: Pest exclusively with
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
.cursor/rules/00-main.mdc lines 27-34: the file contains a line that contradicts
the test policy in .cursor/rules/02-tests.mdc (saying tests are optional vs.
Pest + 60% coverage required); remove or replace the contradictory sentence so
there is a single source of truth — either delete the "Don't worry about tests"
line or change it to a short pointer that delegates to
.cursor/rules/02-tests.mdc (e.g., "See 02-tests.mdc for testing policy"), and
run a quick scan of both files to ensure no other test-policy discrepancies
remain.
| ### 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.) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Symfony 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
In .cursor/rules/01-architecture.mdc around lines 14 to 18, the rule mandates
using Symfony wrappers (e.g., Process::) but the project lacks symfony/process;
either add symfony/process to composer.json (require "symfony/process":
"^version" and run composer install) so the rule can remain strict, or change
the rule text to a relaxed phrasing such as "prefer Symfony wrappers when
available (use Symfony classes like Filesystem::, Console::, Process:: where the
corresponding Symfony package is installed) " so it no longer mandates packages
not present.
| - 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 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Container 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.
- Option A: Add a concrete Container at app/Container.php (namespace Bigpixelrocket\DeployerPHP), implement/wire DI (PSR-11 or project contract) and update bootstrap to use it.
- Option B: Remove the hard reference to app/Container and reword the rule to require a DI contract (e.g., PSR-11) or the actual DI mechanism used.
There was a problem hiding this comment.
Code Review Analysis
I've analyzed all the changes in this PR against our development, architecture, and testing rules. Here's my assessment:
✨ Positive Changes
Rules Organization: The consolidation and restructuring of rules files from scattered documents into well-organized sections improves maintainability and discoverability.
Enhanced Standards: The expanded architecture rules with stronger emphasis on dependency injection patterns and Symfony best practices align with modern PHP development.
Automation Infrastructure: Adding the automated review workflow demonstrates commitment to consistent code quality.
🚨 Critical Issues Identified
Workflow Security: The GitHub Actions workflow downloads and executes external code via curl without checksum verification - this creates a potential supply chain attack vector.
Documentation Consistency: Several rule sections have inconsistent formatting and could benefit from the same "librarian organization" philosophy applied to the code.
⚠️ Areas for Improvement
Rule Clarity: Some rules could be more specific about enforcement mechanisms and examples of violations vs. compliance.
Command Organization: The new command stubs are minimal but could follow the same detailed structure as the main rules.
Summary
Overall, this PR represents a solid improvement in project organization and development standards. The rule consolidation and architectural guidance will help maintain consistency across the codebase. However, the security concern with the workflow should be addressed before merging.
The changes align well with the minimalist philosophy and emphasis on consistency, though some areas could benefit from applying the same "obsessive consistency" standards to the documentation itself.
There was a problem hiding this comment.
📋 Code Review Summary
Based on my analysis of this PR that adds Cursor CLI configuration and a GitHub Actions review workflow, here are my findings:
✅ Strengths
- 🤓 Good Organization: The structure follows the librarian cataloging principle with clear numerical prefixes
- 🫥 Minimalist Approach: New command files are appropriately minimal without unnecessary complexity
- ⚡ Efficient Workflow: The review workflow has proper concurrency control and timeout limits
⚠️ Areas for Improvement
Security & Configuration:
- The
.cursor/cli.jsonpermissions are appropriately restrictive, preventing dangerous operations
Documentation Consistency:
- The new test rules file follows consistent structure with the main rules
- Command stubs are appropriately minimal
Workflow Design:
- The GitHub Actions workflow correctly implements the automated review pattern
- Proper use of environment variables and secrets management
🎯 Overall Assessment
This PR successfully implements the automated review infrastructure while maintaining consistency with the established development philosophy. The changes demonstrate good adherence to the minimalist code principles and organize the configuration like a proper librarian would.
The removal of legacy rule files and consolidation into a cleaner structure aligns well with the "only write the minimum amount of code necessary" principle.
Verdict: The changes align well with the established development rules and architecture patterns. The implementation is clean, minimal, and well-organized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/review.yml (3)
13-13: Consider a higher timeout for large diffs/model latency.Nine minutes is tight for busy runners or big PRs; 15 minutes is safer.
- timeout-minutes: 9 + timeout-minutes: 15
28-31: Surface missing secrets early with a guard step.Fail fast with a clear message when CURSOR_API_KEY is unset to avoid opaque downstream errors.
- name: Code review env: CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} GH_TOKEN: ${{ github.token }} + if: ${{ env.CURSOR_API_KEY != '' }} run: |Add a preceding echo step for the else path if desired.
32-67: Non-enforceable instruction; consider hard guards if enforcement is required.Telling the agent to “Use only gh pr review --comment” is advisory. If strict enforcement is required, wrap gh or provide a minimal permission token limited to PR comments only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.cursor/rules/00-main.mdc(2 hunks).cursor/rules/02-tests.mdc(1 hunks).github/workflows/review.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .cursor/rules/02-tests.mdc
- .cursor/rules/00-main.mdc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: review
🔇 Additional comments (2)
.github/workflows/review.yml (2)
22-26: Replace curl|bash installer with a pinned, checksum‑verified install (and add pipefail).Piping a remote script to bash is a supply‑chain risk and can mask failures without pipefail. Download a specific version, verify checksum/signature, and avoid the pipe. Example patch (fill in the version and sha256 from the vendor):
- - name: Install Cursor CLI - run: | - curl https://cursor.com/install -fsS | bash - echo "$HOME/.cursor/bin" >> $GITHUB_PATH + - name: Install Cursor CLI (pinned + verified) + run: | + set -euo pipefail + VERSION="vX.Y.Z" # TODO: pin exact version + OS="$(uname -s | tr '[:upper:]' '[:lower:]')" + ARCH="$(uname -m)" + ARTIFACT="cursor-agent_${OS}_${ARCH}.tar.gz" + URL="https://example.vendor.tld/cursor-agent/releases/download/${VERSION}/${ARTIFACT}" + curl -fsSL -o "/tmp/${ARTIFACT}" "$URL" + echo "<sha256sum> /tmp/${ARTIFACT}" | sha256sum -c - + sudo tar -xzf "/tmp/${ARTIFACT}" -C /usr/local/bin cursor-agent + echo "/usr/local/bin" >> "$GITHUB_PATH"If an official GitHub Action exists for installing the Cursor CLI, prefer that and pin the action to a commit SHA.
19-21: Pin actions/checkout@v4 to its v4 commit SHA (08eba0b).github/workflows/review.yml (lines 19-21) — replace:
uses: actions/checkout@v4
uses: actions/checkout@08eba0b
| review: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 9 | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
Gate the job to avoid failing on forked PRs (secrets and write perms are unavailable).
PRs from forks won't have access to secrets.CURSOR_API_KEY and GITHUB_TOKEN is read‑only, so commenting will fail. Gate execution for non‑forks (or handle gracefully).
review:
+ if: ${{ github.event.pull_request.head.repo.fork == false && secrets.CURSOR_API_KEY != '' }}
runs-on: ubuntu-latest
timeout-minutes: 9
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: trueOptionally add a separate, no‑op step for forks that logs why the job is skipped.
📝 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.
| review: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 9 | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true | |
| review: | |
| if: ${{ github.event.pull_request.head.repo.fork == false && secrets.CURSOR_API_KEY != '' }} | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 9 | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref }} | |
| cancel-in-progress: true |
🤖 Prompt for AI Agents
.github/workflows/review.yml around lines 11-16: the job currently runs for PRs
from forks which lack secrets and a writable GITHUB_TOKEN; update the workflow
to skip running the job for forked PRs by adding a condition at the job level
(e.g., if: github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.fork == false) so the job only runs when the
PR source is not a fork, and optionally add a separate no-op step that runs when
the repo is a fork to log a clear message stating the job was skipped due to
missing secrets.
1 participant
Summary by CodeRabbit
Chores
Documentation
Tests