Skip to content

test: rules_paths enforcement - intentional violations#9

Open
abhinavguptas wants to merge 1 commit intomainfrom
test/rules-enforcement
Open

test: rules_paths enforcement - intentional violations#9
abhinavguptas wants to merge 1 commit intomainfrom
test/rules-enforcement

Conversation

@abhinavguptas
Copy link
Copy Markdown
Contributor

Summary

Test PR to validate the rules_paths feature of the AI PR Reviewer action.

Adds a routes/categories.js CRUD endpoint that intentionally violates rules from the guardrail documents (review-rules.md, vibe-coding-rules/security.md, vibe-coding-rules/api-patterns.md):

# Violation Rule Source
1 Uses var everywhere instead of const/let review-rules.md (Code Style)
2 No JSDoc comments on any function review-rules.md (Code Style)
3 No try/catch or error middleware review-rules.md (Error Handling)
4 Inconsistent error format ({ message } instead of { error, code }) review-rules.md (Error Handling), api-patterns.md
5 No pagination on GET / list endpoint review-rules.md (API Design)
6 No authentication on POST/PATCH/DELETE security.md (Authentication)
7 No input validation or sanitization review-rules.md (Security), api-patterns.md
8 No helmet middleware security.md (Data Protection)
9 POST returns 200 instead of 201 api-patterns.md (Response Format)
10 DELETE returns JSON body instead of 204 api-patterns.md (Response Format)
11 No createdAt/updatedAt timestamps on resources review-rules.md (API Design)
12 No test file for new endpoints review-rules.md (Testing)
13 Uses == instead of === for ID comparison general best practice

Purpose

The existing workflow (.github/workflows/ai-review.yml) uses default rules_paths which auto-discovers review-rules.md and vibe-coding-rules/. This PR tests whether the AI reviewer correctly identifies violations against those guardrail documents.

Expected Reviewer Behavior

The AI reviewer should flag most or all of the violations listed above, citing the specific rules from the discovered guardrail files.

Test Plan

  • PR triggers the ai-review workflow automatically
  • Review comment references specific rules from guardrail documents
  • Violations are identified with inline comments on the relevant lines
  • Summary lists the rules that were violated

Generated with Claude Code

Adds a categories CRUD endpoint that deliberately violates guardrail
rules (no JSDoc, no try/catch, no pagination, uses var, no auth,
no input validation, wrong error format) to test rules_paths detection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

🩺 Dr. Concret.io — 🔴 Changes Requested

This PR introduces a new /categories API endpoint with several critical and high-severity issues. These include a complete lack of authentication on mutating endpoints, missing input validation and sanitization, and inadequate error handling across all routes. These violations directly contravene established security, error handling, and API design coding standards.

Findings (15)

Severity Count
High 11
Medium 4

🩺 Dr. Concret.io · Model: gemini-2.5-flash · Tokens: 3951 in, 1822 out · Cost: ~$.0192

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI review: see individual comments below.

Comment thread routes/categories.js
return;
}
res.json(category);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] security: Missing authentication on POST endpoint

All API endpoints that modify data must require authentication, as per security.md. The POST /categories endpoint currently allows unauthenticated creation of resources, posing a significant security risk.

Comment thread routes/categories.js
// update category
router.patch('/:id', function(req, res) {
var category = categories.find(function(c) { return c.id == req.params.id; });
if (!category) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] security: Missing authentication on PATCH endpoint

All API endpoints that modify data must require authentication, as per security.md. The PATCH /categories/:id endpoint currently allows unauthenticated modification of resources, posing a significant security risk.

Comment thread routes/categories.js
var index = categories.findIndex(function(c) { return c.id == req.params.id; });
if (index === -1) {
res.status(404).json({ message: 'not found' });
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] security: Missing authentication on DELETE endpoint

All API endpoints that modify data must require authentication, as per security.md. The DELETE /categories/:id endpoint currently allows unauthenticated deletion of resources, posing a significant security risk.

Comment thread routes/categories.js
res.json(category);
});

// create category
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] security: Missing input validation and sanitization for category creation

User input for name, description, and color is used directly without validation or sanitization. This violates review-rules.md and api-patterns.md, creating potential for injection attacks, XSS, or malformed data.

Comment thread routes/categories.js
res.status(404).json({ message: 'not found' });
return;
}
if (req.body.name) category.name = req.body.name;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] security: Missing input validation and sanitization for category update

User input for name, description, and color is used directly without validation or sanitization. This violates review-rules.md and api-patterns.md, creating potential for injection attacks, XSS, or malformed data.

Comment thread routes/categories.js
// update category
router.patch('/:id', function(req, res) {
var category = categories.find(function(c) { return c.id == req.params.id; });
if (!category) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] quality: Missing error handling in PATCH /categories/:id route

The route handler for PATCH /categories/:id lacks try/catch blocks or proper error middleware integration, violating review-rules.md. Unhandled exceptions could crash the application or expose sensitive information.

Comment thread routes/categories.js
var index = categories.findIndex(function(c) { return c.id == req.params.id; });
if (index === -1) {
res.status(404).json({ message: 'not found' });
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [HIGH] quality: Missing error handling in DELETE /categories/:id route

The route handler for DELETE /categories/:id lacks try/catch blocks or proper error middleware integration, violating review-rules.md. Unhandled exceptions could crash the application or expose sensitive information.

Comment thread routes/categories.js
@@ -0,0 +1,58 @@
var express = require('express');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [MEDIUM] style: Use const or let instead of var

The use of var is discouraged by review-rules.md. Please use const for variables that are not reassigned and let for those that are. This applies to multiple declarations in this file.

Suggestion:

Suggested change
var express = require('express');
const express = require('express');

Comment thread routes/categories.js
});

// get single category
router.get('/:id', function(req, res) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [MEDIUM] correctness: Use strict equality === instead of ==

Using == can lead to unexpected type coercion issues. It's best practice to use === for robust comparisons to avoid subtle bugs.

Suggestion:

Suggested change
router.get('/:id', function(req, res) {
var category = categories.find(function(c) { return c.id === req.params.id; });

Comment thread routes/categories.js
var nextId = 1;

// get all categories
router.get('/', function(req, res) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [MEDIUM] architecture: Missing pagination for list endpoint

The GET /categories endpoint does not support pagination with page and limit query parameters, violating review-rules.md. This can lead to performance issues with large datasets and poor user experience.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant