Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/ai-review-related.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
name: AI Code Review (related context)
on:
pull_request:
types: [opened, synchronize, reopened]

jobs:
review:
runs-on: ubuntu-latest
permissions:
contents: read
pull-requests: write
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: concretios/ai-pr-reviewer@v1
with:
gemini_api_key: ${{ secrets.GEMINI_API_KEY }}
context_depth: related
submit_review_verdict: true
19 changes: 17 additions & 2 deletions routes/tasks.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const express = require('express');
const { validateTask } = require('../utils/task-validator');
const router = express.Router();

// In-memory task store
Expand All @@ -19,11 +20,25 @@ router.get('/:id', (req, res) => {

// POST /tasks

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 [CRITICAL] security: Missing Authentication on Mutating Endpoint

The POST /tasks endpoint, which modifies data by creating a new task, is not protected by any authentication middleware. This violates the security rule requiring all mutating endpoints to be authenticated, making it vulnerable to unauthorized task creation.

Suggestion:

Suggested change
// POST /tasks
router.post('/', authenticateUser, (req, res) => {

router.post('/', (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: Inline validation in route handler

The validateTask function is called directly within the route handler. The API Design Patterns rule specifies using middleware for request validation, not inline checks, to keep route handlers focused on business logic.

Suggestion:

Suggested change
router.post('/', (req, res) => {
router.post('/', validateTaskMiddleware, (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: Validation Not Implemented as Middleware

The validateTask function is called directly within the route handler. The API design patterns specify using middleware for request validation. Moving this logic into a dedicated middleware function would improve separation of concerns and reusability.

Suggestion:

Suggested change
router.post('/', (req, res) => {
router.post('/', validateTaskMiddleware, (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.

🩺 [HIGH] security: POST /tasks endpoint lacks authentication

The POST /tasks endpoint, which is a mutating operation, is not protected by any authentication middleware. This violates the review-rules.md and security.md standards, allowing unauthorized users to create tasks.

Suggestion:

Suggested change
router.post('/', (req, res) => {
router.post('/', authenticate, (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] quality: Missing try/catch in route handler

The router.post handler lacks a try/catch block or integration with an Express error middleware for its main logic. This violates the review-rules.md standard for robust error handling.

Suggestion:

Suggested change
router.post('/', (req, res) => {
router.post('/', (req, res, next) => {
try {
const result = validateTask(req.body);
if (!result.valid) {
return res.status(400).json({ errors: result.errors });
}
// ... rest of the handler logic
} catch (err) {
next(err); // Pass errors to the next error handling middleware
}
});

const result = validateTask(req.body);

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: Validation not implemented as middleware

The api-patterns.md rule states that request validation should use middleware, not inline checks in route handlers. Calling validateTask directly in the handler deviates from this architectural pattern.

Suggestion:

Suggested change
const result = validateTask(req.body);
router.post('/', validateTaskMiddleware, (req, res) => {

if (!result.valid) {

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] quality: Inconsistent error response format

The error response for validation failures uses { errors: result.errors }. This format is inconsistent with the vibe-coding-rules/api-patterns.md which requires all error responses to use { error: "Human readable message", code: "MACHINE_CODE" }.

Suggestion:

Suggested change
if (!result.valid) {
return res.status(400).json({ error: 'Validation failed', code: 'VALIDATION_ERROR', details: result.errors });

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] quality: Inconsistent Error Response Format

The validation error response { errors: result.errors } is inconsistent with the project's standard error format of { error: string, code?: string }. All error responses should adhere to the specified format for predictability and easier client-side handling.

Suggestion:

Suggested change
if (!result.valid) {
return res.status(400).json({ error: 'Validation failed', code: 'VALIDATION_ERROR', details: result.errors });

return res.status(400).json({ errors: result.errors });

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: Inconsistent error response format

The validation error response { errors: result.errors } deviates from the consistent error format { error: string, code?: string } specified in both review-rules.md and api-patterns.md.

Suggestion:

Suggested change
return res.status(400).json({ errors: result.errors });
return res.status(400).json({ error: 'Validation failed', code: 'VALIDATION_ERROR', details: result.errors });

}

// INCONSISTENCY: validator allows priority and dueDate fields,
// but this destructuring only extracts title and description.
// A user could send { title: "x", priority: "high", dueDate: "2026-12-01" }
// and the validator would pass it, but the created task silently drops

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] correctness: Validated fields are silently dropped

The validateTask function correctly validates priority and dueDate, but these fields are not destructured or used when creating the task. This leads to valid user input being silently discarded, which is a data loss issue.

Suggestion:

Suggested change
// and the validator would pass it, but the created task silently drops
const { title, description, priority, dueDate } = req.body;
const task = {
id: nextId++,
title: title.length > 100 ? title.slice(0, 100) : title,
description: description || '',
priority: priority || 'medium',
dueDate: dueDate || null,
completed: false,
createdAt: new Date().toISOString()
};

// priority and dueDate.

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] correctness: Validated fields silently dropped

The validateTask function correctly validates priority and dueDate fields, but the route handler only destructures title and description from req.body. This means any valid priority or dueDate provided by the user will be silently ignored and not stored, leading to data loss and an inconsistent API contract.

Suggestion:

Suggested change
// priority and dueDate.
const { title, description, priority, dueDate } = req.body;

const { title, description } = req.body;

// INCONSISTENCY: this hardcodes a 100-char title limit,

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] correctness: Validated Fields Silently Dropped

The validateTask function correctly validates priority and dueDate fields, but the route handler only destructures title and description from req.body. This means any valid priority or dueDate provided by the user will be silently ignored and not stored with the task, leading to data loss and an inconsistent API experience.

Suggestion:

Suggested change
// INCONSISTENCY: this hardcodes a 100-char title limit,
const { title, description, priority, dueDate } = req.body;

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 Sanitization

The title and description fields are validated for length and type, but there is no sanitization (e.g., stripping HTML tags or special characters) before storing them. This could expose the application to cross-site scripting (XSS) vulnerabilities if these fields are later rendered in a web interface without proper escaping.

Suggestion:

Suggested change
// INCONSISTENCY: this hardcodes a 100-char title limit,
const { title, description } = req.body;
const sanitizedTitle = sanitizeHtml(title);
const sanitizedDescription = sanitizeHtml(description || '');
const task = {
id: nextId++,
title: sanitizedTitle,
description: sanitizedDescription,

// but the validator uses MAX_TITLE_LENGTH = 200.

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] correctness: Validated fields are silently dropped

The validateTask function accepts priority and dueDate fields, but the routes/tasks.js handler only destructures title and description from req.body. This means any priority or dueDate provided by the user, even if valid, will be silently dropped from the created task, leading to data loss and a misleading API contract.

Suggestion:

Suggested change
// but the validator uses MAX_TITLE_LENGTH = 200.
const { title, description, priority, dueDate } = req.body;
const task = {
id: nextId++,
title: title.length > 100 ? title.slice(0, 100) : title,
description: description || '',
priority: priority || 'medium', // Or a suitable default
dueDate: dueDate || null, // Or a suitable default
completed: false,
createdAt: new Date().toISOString()
};

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] correctness: Title length truncated despite validator allowing more

The validateTask allows titles up to 200 characters, but the route handler explicitly truncates the title to 100 characters. This means valid user input is silently modified, causing an unexpected behavior for users.

Suggestion:

Suggested change
// but the validator uses MAX_TITLE_LENGTH = 200.
title: title,

// Titles between 101-200 chars pass validation but get truncated here.
const task = {

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] correctness: Title truncated after validation

The validateTask function allows titles up to MAX_TITLE_LENGTH (200 characters), but the POST /tasks handler truncates the title to 100 characters. This creates an inconsistency where valid input is unexpectedly modified, leading to data loss for longer titles.

Suggestion:

Suggested change
const task = {
title,

id: nextId++,

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] correctness: Title length mismatch and truncation

The validateTask function allows titles up to MAX_TITLE_LENGTH (200 characters), but the routes/tasks.js handler hardcodes a truncation to 100 characters. This results in data loss for titles between 101 and 200 characters, which pass validation but are then silently shortened.

Suggestion:

Suggested change
id: nextId++,
title: title.length > MAX_TITLE_LENGTH ? title.slice(0, MAX_TITLE_LENGTH) : title,

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] quality: Missing input sanitization after validation

Although the validator checks for empty or whitespace-only titles, the title and description are not explicitly trimmed before being stored. This could lead to leading or trailing whitespace being saved in the database, which might cause inconsistencies or display issues.

Suggestion:

Suggested change
id: nextId++,
title: title.trim().length > 100 ? title.trim().slice(0, 100) : title.trim(),
description: (description || '').trim(),

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] correctness: Title Length Mismatch and Truncation

The validateTask function allows titles up to MAX_TITLE_LENGTH = 200 characters. However, the route handler explicitly truncates the title to 100 characters. This creates an inconsistency where valid input according to the validator is unexpectedly modified, potentially leading to data corruption or user confusion.

Suggestion:

Suggested change
id: nextId++,
title,

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] quality: Missing 'updatedAt' timestamp

The review-rules.md requires createdAt and updatedAt timestamps on all resources. While createdAt is included, updatedAt is missing for newly created tasks.

Suggestion:

Suggested change
id: nextId++,
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString()

title,
description,
title: title.length > 100 ? title.slice(0, 100) : title,
description: description || '',
completed: false,
createdAt: new Date().toISOString()
};
Expand Down
72 changes: 72 additions & 0 deletions utils/task-validator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
* Task validation utilities.
* Used by routes/tasks.js to validate incoming task data.
*/

const MAX_TITLE_LENGTH = 200;
const MAX_DESCRIPTION_LENGTH = 2000;

// Valid priority levels accepted by the validator
const VALID_PRIORITIES = ['low', 'medium', 'high', 'critical'];

/**
* Validates a task object for creation.
*
* Returns { valid: true } or { valid: false, errors: [...] }
*

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: Function exceeds maximum line length

The validateTask function is 72 lines long, which exceeds the 'Maximum function length: 30 lines' rule from review-rules.md. Consider extracting smaller helper functions to improve readability and maintainability.

* Accepts: title (required, max 200 chars), description (optional, max 2000 chars),

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: Function exceeds maximum line length

The validateTask function is 58 lines long, which exceeds the maximum function length of 30 lines specified in review-rules.md. Consider extracting helper functions to improve readability and maintainability.

Suggestion:

Suggested change
* Accepts: title (required, max 200 chars), description (optional, max 2000 chars),
/**
* Helper to validate title field.
* @param {object} data - The task data.
* @param {Array<string>} errors - Array to accumulate errors.
*/
function validateTitle(data, errors) {
if (!data.title || typeof data.title !== 'string') {
errors.push('title is required and must be a string');
} else if (data.title.trim().length === 0) {
errors.push('title cannot be empty or whitespace-only');
} else if (data.title.length > MAX_TITLE_LENGTH) {
errors.push(`title must be ${MAX_TITLE_LENGTH} characters or fewer`);
}
}
/**
* Helper to validate description field.
* @param {object} data - The task data.
* @param {Array<string>} errors - Array to accumulate errors.
*/
function validateDescription(data, errors) {
if (data.description !== undefined) {
if (typeof data.description !== 'string') {
errors.push('description must be a string');
} else if (data.description.length > MAX_DESCRIPTION_LENGTH) {
errors.push(`description must be ${MAX_DESCRIPTION_LENGTH} characters or fewer`);
}
}
}
// ... similar helpers for priority and dueDate
function validateTask(data) {
const errors = [];
if (!data || typeof data !== 'object') {
return { valid: false, errors: ['Request body must be a JSON object'] };
}
validateTitle(data, errors);
validateDescription(data, errors);
// ... call other validation helpers
return errors.length > 0 ? { valid: false, errors } : { valid: true };
}

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] security: Missing input sanitization

The security.md rule requires sanitizing all user-provided strings (e.g., stripping HTML) before storing. The current validator only checks types and lengths but does not perform sanitization.

Suggestion:

Suggested change
* Accepts: title (required, max 200 chars), description (optional, max 2000 chars),
// Example: Add a sanitization step before validation checks
if (data.title) data.title = data.title.trim();
if (data.description) data.description = data.description.trim();
// Consider a library like 'sanitize-html' for stripping HTML tags

* priority (optional, one of low/medium/high/critical), dueDate (optional, ISO string)
*/
function validateTask(data) {
const errors = [];

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: Function exceeds maximum length

The validateTask function is 44 lines long, exceeding the project's maximum function length rule of 30 lines. Consider extracting individual field validation logic into smaller helper functions to improve readability and maintainability.

Suggestion:

Suggested change
function validateTitle(title, errors) { /* ... */ }
function validateDescription(description, errors) { /* ... */ }
// ... and so on

if (!data || typeof data !== 'object') {

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: Function Exceeds Maximum Length

The validateTask function is 43 lines long, which exceeds the project's maximum function length rule of 30 lines. This can make the function harder to read and maintain. Consider extracting parts of the validation logic into smaller, more focused helper functions.

Suggestion:

Suggested change
if (!data || typeof data !== 'object') {
function validateTitle(title, errors) {
if (!title || typeof title !== 'string') {
errors.push('title is required and must be a string');
} else if (title.trim().length === 0) {
errors.push('title cannot be empty or whitespace-only');
} else if (title.length > MAX_TITLE_LENGTH) {
errors.push(`title must be ${MAX_TITLE_LENGTH} characters or fewer`);
}
}
function validateDescription(description, errors) {
if (description !== undefined) {
if (typeof description !== 'string') {
errors.push('description must be a string');
} else if (description.length > MAX_DESCRIPTION_LENGTH) {
errors.push(`description must be ${MAX_DESCRIPTION_LENGTH} characters or fewer`);
}
}
}
// ... similar for priority and dueDate
function validateTask(data) {
const errors = [];
if (!data || typeof data !== 'object') {
return { valid: false, errors: ['Request body must be a JSON object'] };
}
validateTitle(data.title, errors);
validateDescription(data.description, errors);
validatePriority(data.priority, errors);
validateDueDate(data.dueDate, errors);
return errors.length > 0 ? { valid: false, errors } : { valid: true };
}

return { valid: false, errors: ['Request body must be a JSON object'] };
}

// Title validation
if (!data.title || typeof data.title !== 'string') {
errors.push('title is required and must be a string');
} else if (data.title.trim().length === 0) {
errors.push('title cannot be empty or whitespace-only');
} else if (data.title.length > MAX_TITLE_LENGTH) {
errors.push(`title must be ${MAX_TITLE_LENGTH} characters or fewer`);
}

// Description validation (optional)
if (data.description !== undefined) {
if (typeof data.description !== 'string') {
errors.push('description must be a string');
} else if (data.description.length > MAX_DESCRIPTION_LENGTH) {
errors.push(`description must be ${MAX_DESCRIPTION_LENGTH} characters or fewer`);
}
}

// Priority validation (optional, accepted by validator but ignored by route handler)
if (data.priority !== undefined) {
if (!VALID_PRIORITIES.includes(data.priority)) {
errors.push(`priority must be one of: ${VALID_PRIORITIES.join(', ')}`);
}
}

// Due date validation (optional, accepted by validator but ignored by route handler)
if (data.dueDate !== undefined) {
if (typeof data.dueDate !== 'string') {
errors.push('dueDate must be an ISO 8601 date string');
} else {
const parsed = new Date(data.dueDate);
if (isNaN(parsed.getTime())) {
errors.push('dueDate must be a valid ISO 8601 date string');
}
}
}

return errors.length > 0 ? { valid: false, errors } : { valid: true };
}

module.exports = {
validateTask,
MAX_TITLE_LENGTH,
MAX_DESCRIPTION_LENGTH,
VALID_PRIORITIES,
};
Loading