[opencode] Implement backend validation for goal creation endpoints#21
Open
heodongun wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete Goal Management API server built with Express. It includes API documentation defining CRUD endpoints for goals with validation on title and priority fields, a server implementation with in-memory storage and request validation, project configuration, and console-based validation tests. ChangesGoal API Server
Sequence DiagramsequenceDiagram
participant Client
participant ExpressServer
participant ValidateGoal
participant GoalsStore
Client->>ExpressServer: POST /api/goals with goal data
ExpressServer->>ValidateGoal: validateGoal(goalData)
ValidateGoal-->>ExpressServer: errors array or empty
alt validation errors
ExpressServer-->>Client: 400 Bad Request
else valid goal
ExpressServer->>GoalsStore: Add goal with auto ID and timestamps
GoalsStore-->>ExpressServer: Stored goal object
ExpressServer-->>Client: 201 Created with goal
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 1-17: server.js imports express but package.json has no
dependencies entry for express; add an "express" entry under the "dependencies"
object in package.json (or run npm install --save express) so installs won't
fail. Update package.json "dependencies": { "express": "<appropriate-semver>" }
and regenerate lockfile (npm install or yarn) so server.js can
require('express') successfully.
- Around line 10-11: Replace the placeholder "test" npm script so it actually
runs the validation test file instead of exiting with an error: update the
"test" script entry in package.json (the "test" npm script) to invoke the
project's test runner against test/goal-validation.test.js (so the command runs
the test file and returns the runner's exit code) and ensure the script uses the
repo's configured test tool (mocha/jest/etc.) rather than the current echo/exit
stub.
In `@server.js`:
- Around line 9-10: The current goals array uses id: goals.length + 1 which
reuses IDs after deletes (variables: goals and the create/add handler that
assigns id), causing collisions; replace this with a monotonic ID generator such
as a module-level counter (e.g., declare let nextGoalId = 1) or a UUID generator
and use that when creating new goal objects (update all creation sites that
currently use goals.length + 1, including the handlers referenced around where
id is assigned and at lines noted), increment the counter after assignment (or
generate a fresh UUID) so deletes do not affect future IDs and ensure
update/delete logic continues to match by the new id type.
- Around line 12-17: The validateGoal function currently assumes its input is an
object and directly reads goal.title, which throws on null or non-object
payloads; update validateGoal to first check that goal is a non-null object
(e.g., if (typeof goal !== 'object' || goal === null) { errors.push('Payload
must be a JSON object'); return errors; }) before accessing properties, and
apply the same guard pattern to the other validators that access
goal.title/other properties (the validator functions referenced around the other
occurrences) so they return a 400-style validation error instead of throwing.
- Around line 133-137: The file starts the HTTP server on import
(app.listen(...)) which causes side effects in tests and also does not export
the validateGoal function; modify the module so the server only starts when the
file is run directly (e.g., guard app.listen with a check like require.main ===
module or export a startServer function) and update exports to export both app
and validateGoal (export validateGoal by name along with app or as part of
module.exports) so tests can import validateGoal without launching the server.
In `@test/goal-validation.test.js`:
- Line 2: The test imports the wrong module path; update the require in
test/goal-validation.test.js so it loads the repository-root server module (the
exported validateGoal) instead of test/server. Locate the line that requires
validateGoal (currently "require('./server')") and change it to reference the
root server module (so validateGoal from the module that exports the function,
e.g., require('../server')). Ensure the import still references the validateGoal
symbol used in the test.
- Around line 5-55: Replace the soft console PASS/FAIL checks with hard
assertions so failures exit non-zero: for each test that calls validateGoal
(errors1, errors2, errors3, errors4, errors5) replace the console.log boolean
checks with assert-style checks (or throw new Error) that validate the same
condition and include the errors array in the message; ensure the validGoal case
asserts errors.length === 0 and each negative case asserts errors.length > 0 and
specific message inclusion (e.g., includes 'Title', 'Priority', 'exceed') so a
failed expectation will throw and fail the test run.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: df61340c-4b8d-466b-8f64-89d5e2498da4
📒 Files selected for processing (4)
API_DOCUMENTATION.mdpackage.jsonserver.jstest/goal-validation.test.js
Comment on lines
+1
to
+17
| { | ||
| "name": "opencode", | ||
| "version": "1.0.0", | ||
| "description": "Initial sandbox for Cotor Gemma runtime smoke.", | ||
| "main": "index.js", | ||
| "directories": { | ||
| "doc": "docs" | ||
| }, | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "start": "node server.js" | ||
| }, | ||
| "keywords": [], | ||
| "author": "", | ||
| "license": "ISC", | ||
| "type": "commonjs" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking express import in server.js:"
rg -n "require\\('express'\\)" server.js
echo "Checking declared dependencies:"
jq '.dependencies // {}' package.jsonRepository: bssm-oss/cotor-testv2
Length of output: 176
Declare express as a dependency in package.json.
server.js imports express but it's not listed in the dependencies object, so a fresh install will fail to provide the necessary module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 1 - 17, server.js imports express but package.json
has no dependencies entry for express; add an "express" entry under the
"dependencies" object in package.json (or run npm install --save express) so
installs won't fail. Update package.json "dependencies": { "express":
"<appropriate-semver>" } and regenerate lockfile (npm install or yarn) so
server.js can require('express') successfully.
Comment on lines
+10
to
+11
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "start": "node server.js" |
There was a problem hiding this comment.
Replace the placeholder test script with the actual validation test command.
The current script always exits with failure and does not execute test/goal-validation.test.js.
Suggested fix
"scripts": {
- "test": "echo \"Error: no test specified\" && exit 1",
+ "test": "node test/goal-validation.test.js",
"start": "node server.js"
},
+ "dependencies": {
+ "express": "^4.21.2"
+ },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 10 - 11, Replace the placeholder "test" npm script
so it actually runs the validation test file instead of exiting with an error:
update the "test" script entry in package.json (the "test" npm script) to invoke
the project's test runner against test/goal-validation.test.js (so the command
runs the test file and returns the runner's exit code) and ensure the script
uses the repo's configured test tool (mocha/jest/etc.) rather than the current
echo/exit stub.
Comment on lines
+9
to
+10
| let goals = []; | ||
|
|
There was a problem hiding this comment.
Use a monotonic ID generator to prevent ID collisions after delete.
id: goals.length + 1 reuses IDs when items are deleted, which can corrupt update/delete targeting.
Suggested fix
let goals = [];
+let nextGoalId = 1;
@@
const newGoal = {
- id: goals.length + 1,
+ id: nextGoalId++,Also applies to: 75-75, 123-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 9 - 10, The current goals array uses id: goals.length
+ 1 which reuses IDs after deletes (variables: goals and the create/add handler
that assigns id), causing collisions; replace this with a monotonic ID generator
such as a module-level counter (e.g., declare let nextGoalId = 1) or a UUID
generator and use that when creating new goal objects (update all creation sites
that currently use goals.length + 1, including the handlers referenced around
where id is assigned and at lines noted), increment the counter after assignment
(or generate a fresh UUID) so deletes do not affect future IDs and ensure
update/delete logic continues to match by the new id type.
Comment on lines
+12
to
+17
| function validateGoal(goal) { | ||
| const errors = []; | ||
|
|
||
| // Validate title | ||
| if (!goal.title || typeof goal.title !== 'string') { | ||
| errors.push('Title is required and must be a string'); |
There was a problem hiding this comment.
Guard validateGoal against non-object payloads.
POST/PUT can throw on null or non-object JSON bodies because goal.title is accessed unguarded, returning 500 instead of a 400 validation error.
Suggested fix
function validateGoal(goal) {
const errors = [];
+ if (!goal || typeof goal !== 'object' || Array.isArray(goal)) {
+ return ['Request body must be a JSON object'];
+ }
// Validate title
if (!goal.title || typeof goal.title !== 'string') {Also applies to: 65-66, 94-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 12 - 17, The validateGoal function currently assumes
its input is an object and directly reads goal.title, which throws on null or
non-object payloads; update validateGoal to first check that goal is a non-null
object (e.g., if (typeof goal !== 'object' || goal === null) {
errors.push('Payload must be a JSON object'); return errors; }) before accessing
properties, and apply the same guard pattern to the other validators that access
goal.title/other properties (the validator functions referenced around the other
occurrences) so they return a 400-style validation error instead of throwing.
Comment on lines
+133
to
+137
| app.listen(port, () => { | ||
| console.log(`Goal API server running at http://localhost:${port}`); | ||
| }); | ||
|
|
||
| module.exports = app; No newline at end of file |
There was a problem hiding this comment.
Avoid server startup side effects on import and export validateGoal for tests.
Importing this file in tests currently starts the HTTP server and does not expose validateGoal.
Suggested fix
-app.listen(port, () => {
- console.log(`Goal API server running at http://localhost:${port}`);
-});
-
-module.exports = app;
+if (require.main === module) {
+ app.listen(port, () => {
+ console.log(`Goal API server running at http://localhost:${port}`);
+ });
+}
+
+module.exports = { app, validateGoal };📝 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.
Suggested change
| app.listen(port, () => { | |
| console.log(`Goal API server running at http://localhost:${port}`); | |
| }); | |
| module.exports = app; | |
| if (require.main === module) { | |
| app.listen(port, () => { | |
| console.log(`Goal API server running at http://localhost:${port}`); | |
| }); | |
| } | |
| module.exports = { app, validateGoal }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 133 - 137, The file starts the HTTP server on import
(app.listen(...)) which causes side effects in tests and also does not export
the validateGoal function; modify the module so the server only starts when the
file is run directly (e.g., guard app.listen with a check like require.main ===
module or export a startServer function) and update exports to export both app
and validateGoal (export validateGoal by name along with app or as part of
module.exports) so tests can import validateGoal without launching the server.
| @@ -0,0 +1,55 @@ | |||
| // Mock test for goal validation - would normally use Jest and Supertest | |||
| const { validateGoal } = require('./server'); // This won't work without proper setup, but shows intent | |||
There was a problem hiding this comment.
Fix the module path for server.js.
From test/goal-validation.test.js, require('./server') resolves to test/server and fails; this should point to the repository-root server module.
Suggested fix
-const { validateGoal } = require('./server');
+const { validateGoal } = require('../server');📝 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.
Suggested change
| const { validateGoal } = require('./server'); // This won't work without proper setup, but shows intent | |
| const { validateGoal } = require('../server'); // This won't work without proper setup, but shows intent |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/goal-validation.test.js` at line 2, The test imports the wrong module
path; update the require in test/goal-validation.test.js so it loads the
repository-root server module (the exported validateGoal) instead of
test/server. Locate the line that requires validateGoal (currently
"require('./server')") and change it to reference the root server module (so
validateGoal from the module that exports the function, e.g.,
require('../server')). Ensure the import still references the validateGoal
symbol used in the test.
Comment on lines
+5
to
+55
| console.log('Running goal validation tests...'); | ||
|
|
||
| // Test 1: Valid goal | ||
| const validGoal = { | ||
| title: 'Learn JavaScript', | ||
| description: 'Study ES6 features and async/await', | ||
| priority: 'medium', | ||
| targetDate: '2026-12-31' | ||
| }; | ||
|
|
||
| const errors1 = validateGoal(validGoal); | ||
| console.log('Test 1 - Valid goal:', errors1.length === 0 ? 'PASS' : 'FAIL', errors1); | ||
|
|
||
| // Test 2: Missing title | ||
| const missingTitle = { | ||
| description: 'Some description', | ||
| priority: 'high' | ||
| }; | ||
|
|
||
| const errors2 = validateGoal(missingTitle); | ||
| console.log('Test 2 - Missing title:', errors2.length > 0 && errors2.some(e => e.includes('Title')) ? 'PASS' : 'FAIL', errors2); | ||
|
|
||
| // Test 3: Invalid priority | ||
| const invalidPriority = { | ||
| title: 'Test Goal', | ||
| priority: 'invalid' | ||
| }; | ||
|
|
||
| const errors3 = validateGoal(invalidPriority); | ||
| console.log('Test 3 - Invalid priority:', errors3.length > 0 && errors3.some(e => e.includes('Priority')) ? 'PASS' : 'FAIL', errors3); | ||
|
|
||
| // Test 4: Title too long | ||
| const longTitle = { | ||
| title: 'A'.repeat(101), // 101 characters | ||
| priority: 'low' | ||
| }; | ||
|
|
||
| const errors4 = validateGoal(longTitle); | ||
| console.log('Test 4 - Title too long:', errors4.length > 0 && errors4.some(e => e.includes('exceed')) ? 'PASS' : 'FAIL', errors4); | ||
|
|
||
| // Test 5: Description too long | ||
| const longDescription = { | ||
| title: 'Test Goal', | ||
| description: 'A'.repeat(501), // 501 characters | ||
| priority: 'medium' | ||
| }; | ||
|
|
||
| const errors5 = validateGoal(longDescription); | ||
| console.log('Test 5 - Description too long:', errors5.length > 0 && errors5.some(e => e.includes('exceed')) ? 'PASS' : 'FAIL', errors5); | ||
|
|
||
| console.log('Validation tests completed.'); No newline at end of file |
There was a problem hiding this comment.
Make tests fail hard on broken expectations (not just log).
These checks only print PASS/FAIL and can exit 0 even when behavior regresses.
Suggested fix
+const assert = require('assert');
@@
-const errors1 = validateGoal(validGoal);
-console.log('Test 1 - Valid goal:', errors1.length === 0 ? 'PASS' : 'FAIL', errors1);
+const errors1 = validateGoal(validGoal);
+assert.strictEqual(errors1.length, 0, `Test 1 failed: ${JSON.stringify(errors1)}`);
@@
-const errors2 = validateGoal(missingTitle);
-console.log('Test 2 - Missing title:', errors2.length > 0 && errors2.some(e => e.includes('Title')) ? 'PASS' : 'FAIL', errors2);
+const errors2 = validateGoal(missingTitle);
+assert.ok(errors2.some(e => e.includes('Title')), `Test 2 failed: ${JSON.stringify(errors2)}`);
@@
-const errors3 = validateGoal(invalidPriority);
-console.log('Test 3 - Invalid priority:', errors3.length > 0 && errors3.some(e => e.includes('Priority')) ? 'PASS' : 'FAIL', errors3);
+const errors3 = validateGoal(invalidPriority);
+assert.ok(errors3.some(e => e.includes('Priority')), `Test 3 failed: ${JSON.stringify(errors3)}`);
@@
-const errors4 = validateGoal(longTitle);
-console.log('Test 4 - Title too long:', errors4.length > 0 && errors4.some(e => e.includes('exceed')) ? 'PASS' : 'FAIL', errors4);
+const errors4 = validateGoal(longTitle);
+assert.ok(errors4.some(e => e.includes('exceed')), `Test 4 failed: ${JSON.stringify(errors4)}`);
@@
-const errors5 = validateGoal(longDescription);
-console.log('Test 5 - Description too long:', errors5.length > 0 && errors5.some(e => e.includes('exceed')) ? 'PASS' : 'FAIL', errors5);
+const errors5 = validateGoal(longDescription);
+assert.ok(errors5.some(e => e.includes('exceed')), `Test 5 failed: ${JSON.stringify(errors5)}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/goal-validation.test.js` around lines 5 - 55, Replace the soft console
PASS/FAIL checks with hard assertions so failures exit non-zero: for each test
that calls validateGoal (errors1, errors2, errors3, errors4, errors5) replace
the console.log boolean checks with assert-style checks (or throw new Error)
that validate the same condition and include the errors array in the message;
ensure the validGoal case asserts errors.length === 0 and each negative case
asserts errors.length > 0 and specific message inclusion (e.g., includes
'Title', 'Priority', 'exceed') so a failed expectation will throw and fail the
test run.
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
Contributor
Author
|
CEO_VERDICT: APPROVE CEO summary from Cotor:
|
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
1 similar comment
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
1 similar comment
Contributor
Author
|
QA_VERDICT: PASS QA summary from Cotor:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prompt
Summary by CodeRabbit
New Features
Documentation
Tests