Skip to content

Add code review prevention checklist#1836

Open
arifulhoque7 wants to merge 1 commit intoweDevsOfficial:developfrom
arifulhoque7:docs/improve-code-review-prevention-rules
Open

Add code review prevention checklist#1836
arifulhoque7 wants to merge 1 commit intoweDevsOfficial:developfrom
arifulhoque7:docs/improve-code-review-prevention-rules

Conversation

@arifulhoque7
Copy link
Copy Markdown
Contributor

@arifulhoque7 arifulhoque7 commented Mar 27, 2026

Add a mandatory Code Review Prevention Checklist to developer docs. The detailed checklist in .claude/skills/wpuf-backend-dev/SKILL.md covers top rejection causes and required practices (strict ===/!== comparisons, in_array/array_search with strict true, wp_unslash() + sanitization for superglobals, output escaping, $wpdb->prepare() for SQL, nonce verification, permission checks, snake_case method naming, @SInCE docblocks, translator comments, hook prefixes, spacing rules, and a pre-PR mental checklist). A condensed summary of these rules was also added to CLAUDE.md with guidance to run composer phpcs before submitting changes.

Summary by CodeRabbit

  • Documentation
    • Enhanced internal coding standards documentation with comprehensive guidelines covering best practices for code quality, security, and WordPress-specific conventions.

Add a mandatory Code Review Prevention Checklist to developer docs. The detailed checklist in .claude/skills/wpuf-backend-dev/SKILL.md covers top rejection causes and required practices (strict ===/!== comparisons, in_array/array_search with strict true, wp_unslash() + sanitization for superglobals, output escaping, $wpdb->prepare() for SQL, nonce verification, permission checks, snake_case method naming, @SInCE docblocks, translator comments, hook prefixes, spacing rules, and a pre-PR mental checklist). A condensed summary of these rules was also added to CLAUDE.md with guidance to run composer phpcs before submitting changes.
@arifulhoque7 arifulhoque7 requested a review from sapayth March 27, 2026 09:39
@arifulhoque7 arifulhoque7 self-assigned this Mar 27, 2026
@arifulhoque7 arifulhoque7 added the needs: dev review This PR needs review by a developer label Mar 27, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Walkthrough

Documentation files updated to establish code review prevention standards. Added comprehensive checklists and mandatory coding conventions covering strict comparisons, sanitization, SQL safety, security verification, naming patterns, and WordPress standards.

Changes

Cohort / File(s) Summary
Code Review Standards Documentation
.claude/skills/wpuf-backend-dev/SKILL.md, CLAUDE.md
Added "Code Review Prevention (MANDATORY)" sections with 12 mandatory rules including strict comparison operators, input sanitization via wp_unslash(), output escaping, SQL safety via $wpdb->prepare(), nonce/capability verification, snake_case naming, DocBlock requirements with @since, translator comments, and hook naming conventions. Includes Quick Self-Review and Pre-PR checklists.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Possibly related PRs

  • Chore/add claude ai skills #1829 — The primary PR that introduced the same documentation files (.claude/skills/wpuf-backend-dev/SKILL.md and CLAUDE.md) with foundational content that this PR augments with stricter code review standards.

Poem

🐰 A checklist of rules, both mighty and grand,
Strict comparisons reign throughout the land,
No SQL concatenation, just prepare() please,
With escapes and sanitize, code flows with ease,
Self-review thrice before the merge bell rings! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change—adding a code review prevention checklist to the developer documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
.claude/skills/wpuf-backend-dev/SKILL.md (1)

391-405: Clarify SQL rule wording to avoid unsafe interpretation.

“Use prepare or allowlist” can be read as either/or for dynamic SQL. Safer wording: allowlist identifiers (e.g., ORDER BY columns/direction) and use $wpdb->prepare() for values.

✏️ Suggested doc wording tweak
-**Even for ORDER BY / LIMIT — use prepare or allowlist:**
+**Even for ORDER BY / LIMIT — allowlist identifiers, and use `$wpdb->prepare()` for values:**
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.claude/skills/wpuf-backend-dev/SKILL.md around lines 391 - 405, Update the
guidance text to avoid "prepare or allowlist" ambiguity by explicitly
instructing developers to allowlist SQL identifiers (e.g., ORDER BY column names
and sort direction using the example variables $allowed_orderby, $orderby,
$order) and separately use $wpdb->prepare() for user-supplied values (e.g.,
LIMIT/offset parameters such as $args['offset'] and $args['number']); make the
example and wording state that identifiers must be validated against the
allowlist and only values should be passed through $wpdb->prepare().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CLAUDE.md`:
- Line 193: The rule is too broad—don't require check_ajax_referer() +
current_user_can(wpuf_admin_role()) for every form/AJAX; instead, scope checks
by context: use check_ajax_referer() for AJAX endpoints and wp_verify_nonce()
(or form-specific nonce verification) for standard form handlers, and only call
current_user_can(...) / wpuf_admin_role() when the operation is privileged
(e.g., modifying other users, admin-only settings); ensure guest/frontend
submissions use appropriate nonce-only validation or alternate flow instead of
admin capability checks, and consider adding a small helper (e.g.,
verify_form_nonce_and_capability) or documenting the pattern to enforce
context-aware checks.

---

Nitpick comments:
In @.claude/skills/wpuf-backend-dev/SKILL.md:
- Around line 391-405: Update the guidance text to avoid "prepare or allowlist"
ambiguity by explicitly instructing developers to allowlist SQL identifiers
(e.g., ORDER BY column names and sort direction using the example variables
$allowed_orderby, $orderby, $order) and separately use $wpdb->prepare() for
user-supplied values (e.g., LIMIT/offset parameters such as $args['offset'] and
$args['number']); make the example and wording state that identifiers must be
validated against the allowlist and only values should be passed through
$wpdb->prepare().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d7a57d05-f538-4b62-8e04-41701680489e

📥 Commits

Reviewing files that changed from the base of the PR and between 48eb6c2 and f0c43e4.

📒 Files selected for processing (2)
  • .claude/skills/wpuf-backend-dev/SKILL.md
  • CLAUDE.md

Comment thread CLAUDE.md
3. **Superglobals** — `wp_unslash()` + sanitize every `$_POST/$_GET/$_REQUEST/$_SERVER` access. Use `sanitize_text_field(wp_unslash(...))`, `absint()`, `sanitize_email(wp_unslash(...))`, `esc_url_raw(wp_unslash(...))`, `sanitize_key(wp_unslash(...))`
4. **Escape all output** — `esc_html()` for text, `esc_attr()` for attributes, `esc_url()` for URLs, `wp_kses_post()` for HTML
5. **SQL safety** — `$wpdb->prepare()` for ALL dynamic values. Allowlist column names for ORDER BY
6. **Nonce + permission** — every form/AJAX: `check_ajax_referer()` + `current_user_can(wpuf_admin_role())`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This rule is too broad and may cause incorrect auth handling.

Requiring check_ajax_referer() + current_user_can( wpuf_admin_role() ) for every form/AJAX is not universally correct (e.g., non-AJAX form handlers, frontend/guest submissions). Recommend scoping it to: nonce verification per context (form vs AJAX), plus capability checks for sensitive operations only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` at line 193, The rule is too broad—don't require
check_ajax_referer() + current_user_can(wpuf_admin_role()) for every form/AJAX;
instead, scope checks by context: use check_ajax_referer() for AJAX endpoints
and wp_verify_nonce() (or form-specific nonce verification) for standard form
handlers, and only call current_user_can(...) / wpuf_admin_role() when the
operation is privileged (e.g., modifying other users, admin-only settings);
ensure guest/frontend submissions use appropriate nonce-only validation or
alternate flow instead of admin capability checks, and consider adding a small
helper (e.g., verify_form_nonce_and_capability) or documenting the pattern to
enforce context-aware checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: dev review This PR needs review by a developer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant