Impertio-Studio nextcloud-agents-review
Purpose
This skill guides Claude through a systematic code review of Nextcloud app code. Run this checklist BEFORE merging, deploying, or accepting generated code. Each check specifies WHAT to verify, the EXPECTED state, and the COMMON failure.
Review Process
ALWAYS follow this sequence:
- Gather scope -- Identify all files changed or generated
- Run area checklists -- Apply each relevant section below
- Report findings -- List every violation with file, line, and fix
- Block on critical -- NEVER approve code with critical violations
Area 1: Controller Security Attributes
ALWAYS verify every public controller method has explicit security attributes.
| Check | Expected State | Common Failure |
|---|---|---|
| Security attributes present | Every public method has at least one of #[NoAdminRequired], #[PublicPage], or is intentionally admin-only |
Missing attributes -- method silently defaults to admin-only |
| No unprotected state-changing endpoints | #[PublicPage] + #[NoCSRFRequired] NEVER appear together on POST/PUT/DELETE methods |
CSRF bypass on public write endpoints |
| Brute force protection on auth endpoints | #[BruteForceProtection] present on login/token/password methods |
Missing throttle on authentication |
| Throttle only on failure | $response->throttle() called ONLY in failure branches |
Throttle on success degrades legitimate users |
| Rate limiting on sensitive endpoints | #[UserRateLimit] or #[AnonRateLimit] on expensive operations |
Unlimited calls to resource-heavy endpoints |
Decision tree:
Method is public?
├── YES: Has security attributes?
│ ├── NO → CRITICAL: Add explicit attributes
│ └── YES: Is state-changing (POST/PUT/DELETE)?
│ ├── YES: Has #[PublicPage] + #[NoCSRFRequired]?
│ │ ├── YES → CRITICAL: Remove one or add alternative auth
│ │ └── NO → OK
│ └── NO → OK
└── NO → SKIP (private/protected methods)
Area 2: OCS Endpoint Validation
ALWAYS verify OCS controllers and routes follow the v2 pattern.
| Check | Expected State | Common Failure |
|---|---|---|
| Extends OCSController | OCS route controllers extend OCSController, NOT Controller |
Using Controller breaks response envelope |
Routes in ocs array |
OCS endpoints listed in the ocs array of routes.php |
Listed in routes array -- wrong URL prefix |
| Returns DataResponse | OCS methods return DataResponse, not JSONResponse |
JSONResponse bypasses OCS envelope |
| OCS-APIRequest header | API documentation/tests include OCS-APIRequest: true |
Requests rejected without header |
| v2 endpoints only | All OCS URLs use /ocs/v2.php/ prefix |
Using v1 with wrong status code assumptions |
Area 3: Database Migration Integrity
ALWAYS verify migrations follow Nextcloud conventions.
| Check | Expected State | Common Failure |
|---|---|---|
| Naming convention | Class: Version{MajorMinor}Date{Timestamp} |
Wrong naming breaks migration order |
| No modified migrations | Existing migration files NEVER changed | Modified migration skipped on instances that already ran it |
| Primary key exists | Every table has setPrimaryKey(['id']) |
Galera Cluster replication fails |
| Auto-increment id | Every table has id BIGINT autoincrement |
Missing primary key column |
| Table name length | Table name max 23 chars (27 with oc_ prefix) |
Oracle compatibility failure |
| Column name length | Column/index names max 30 chars | Oracle compatibility failure |
| Uses query builder | No raw SQL strings in migrations postSchemaChange |
Database portability broken |
| Index names explicit | All indices have named identifiers | Cannot reference index for future modification |
Decision tree:
Is this a schema change?
├── Existing migration file modified?
│ └── YES → CRITICAL: Create new migration instead
├── New table without primary key?
│ └── YES → CRITICAL: Add setPrimaryKey(['id'])
├── Table name > 23 chars?
│ └── YES → CRITICAL: Shorten name
└── All checks pass → OK
Area 4: Dependency Injection
ALWAYS verify services use constructor injection.
| Check | Expected State | Common Failure |
|---|---|---|
| No Server::get() | Zero occurrences of \OCP\Server::get() |
Service locator breaks testability |
| Constructor injection | All dependencies passed via constructor | Runtime resolution via container |
| No ILogger | Zero imports of OCP\ILogger |
Deprecated since NC 24 -- use Psr\Log\LoggerInterface |
| Namespace in info.xml | <namespace> element present |
Auto-wiring silently fails |
| No service queries in register() | register() only calls IRegistrationContext methods |
Queries fail -- other apps not yet registered |
| No side effects in constructors | Constructors only assign dependencies | I/O in constructor breaks lazy loading |
Area 5: Frontend Validation
ALWAYS verify frontend code uses @nextcloud/* packages.
| Check | Expected State | Common Failure |
|---|---|---|
| No OC.generateUrl() | Zero uses of OC.generateUrl |
Legacy global -- use @nextcloud/router |
| No OC.requestToken | Zero direct uses of OC.requestToken |
Use @nextcloud/axios which handles CSRF |
| No raw fetch/axios | Zero imports of plain axios or fetch() |
Missing auth headers -- use @nextcloud/axios |
| @nextcloud/vue imports | Components from @nextcloud/vue, not custom UI |
Inconsistent look, no dark mode support |
| No hardcoded colors | CSS uses var(--color-*) variables |
Breaks dark mode and custom themes |
| Dialog styles imported | @nextcloud/dialogs/style.css imported when using toasts |
Toasts render without styling |
| loadState has fallback | loadState() calls include third argument |
Throws on missing key without fallback |
Area 6: File API Validation
ALWAYS verify file operations use the Node API correctly.
| Check | Expected State | Common Failure |
|---|---|---|
| NotFoundException handled | Every get() call wrapped in try/catch for NotFoundException |
Unhandled exception crashes request |
| getById() array check | getById() result checked as array before access |
Assumes single result -- returns array |
| No hardcoded paths | File access via getUserFolder(), not string paths |
Bypasses storage abstraction |
| IRootFolder injected | IRootFolder as constructor dependency, not IUserFolder |
Cannot access other users' files when needed |
| instanceof check | get() result checked with instanceof File or instanceof Folder |
Type confusion between files and folders |
Area 7: Event System Validation
ALWAYS verify events use the modern typed system.
| Check | Expected State | Common Failure |
|---|---|---|
| Typed events only | All events extend OCP\EventDispatcher\Event |
Using deprecated GenericEvent |
| No deprecated hooks | Zero uses of $manager->listen() |
Legacy hook system -- use IEventDispatcher |
| instanceof check in handle() | handle() method checks $event instanceof SpecificEvent |
Base Event type causes runtime errors |
| Listeners in register() | Listeners registered via IRegistrationContext::registerEventListener() |
Registered in boot() -- may miss early events |
| parent::__construct() called | Custom event constructors call parent::__construct() |
Event base class not initialized |
| No dispatch in constructors | Events dispatched in service methods, not constructors | Services may not be fully initialized |
Area 8: Security Cross-Checks
ALWAYS run these cross-cutting security checks.
| Check | Expected State | Common Failure |
|---|---|---|
| CSRF on state change | POST/PUT/DELETE endpoints have CSRF protection OR OCS-APIRequest |
State changes without CSRF protection |
| No PublicPage on admin endpoints | Admin-settings controllers have NO #[PublicPage] |
Settings exposed to anonymous users |
| Password not logged | No $password or credential values in logger calls |
Credentials in log files |
| SQL injection prevention | All query builder params use createNamedParameter() |
Raw string concatenation in queries |
| Input validation | Controller params have type hints | Untyped string injection |
Severity Levels
| Level | Action | Examples |
|---|---|---|
| CRITICAL | MUST fix before merge | Missing CSRF, modified migration, no primary key, PublicPage+NoCSRFRequired on POST |
| WARNING | SHOULD fix before merge | Missing rate limiting, no NotFoundException handling, deprecated ILogger |
| INFO | Consider fixing | Missing explicit security attributes on admin-only method, barrel imports |
Review Report Template
## Nextcloud Code Review
### Summary
- Files reviewed: {count}
- Critical: {count}
- Warnings: {count}
- Info: {count}
### Critical Issues
- [{file}:{line}] {area}: {description} → Fix: {action}
### Warnings
- [{file}:{line}] {area}: {description} → Fix: {action}
### Info
- [{file}:{line}] {area}: {description} → Fix: {action}
### Verdict: {PASS | BLOCK}
Reference Links
- references/methods.md -- Complete validation checklist with code patterns
- references/examples.md -- Review scenarios: good code, bad code, fixes
- references/anti-patterns.md -- All anti-patterns consolidated by area
Official Sources
- https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/controllers.html
- https://docs.nextcloud.com/server/latest/developer_manual/basics/routing.html
- https://docs.nextcloud.com/server/latest/developer_manual/basics/storage/database.html
- https://docs.nextcloud.com/server/latest/developer_manual/basics/dependency_injection.html
- https://docs.nextcloud.com/server/latest/developer_manual/basics/events.html