feat(api): add runtime request validation for all API endpoints#65
Merged
Flamki merged 4 commits intoJun 8, 2026
Merged
Conversation
|
@supremeproton01 is attempting to deploy a commit to the flamki's projects Team on Vercel. A member of the Team first needs to authorize it. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds request validators and Express middleware, wires them into premium/orchestrate/wallet routes, updates the error handler to include validation details, and adds unit tests and package.json test scripts for the validation suite. ChangesRequest Validation Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant validatePremiumQuery
participant validateOrchestrate
participant validateWalletTransactions
participant RequestValidator
participant ErrorHandler
Client->>Server: HTTP request to premium/orchestrate/wallet
Server->>validatePremiumQuery: invoke (premium routes)
Server->>validateOrchestrate: invoke (orchestrate)
Server->>validateWalletTransactions: invoke (wallet)
validatePremiumQuery->>RequestValidator: validate topic/text/prompt
validateOrchestrate->>RequestValidator: validate task and budget
validateWalletTransactions->>RequestValidator: validate address
RequestValidator-->>validatePremiumQuery: {valid, value|error}
RequestValidator-->>validateOrchestrate: {valid, value|error}
RequestValidator-->>validateWalletTransactions: {valid, value|error}
validatePremiumQuery->>Server: attach req.validated or next(err)
validateOrchestrate->>Server: attach req.validated or next(err)
validateWalletTransactions->>Server: attach req.validated or next(err)
Server->>ErrorHandler: next(err) when validation fails
ErrorHandler-->>Client: JSON error response (includes details)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
src/server.jsParsing error: Identifier 'task' has already been declared 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 |
|
@supremeproton01 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.js (1)
164-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
summarizeignores the validated value and reads rawreq.query.text.Unlike the other premium endpoints (research/analyze/code) which now consume
req.validated, this handler still readsreq.query.texton Line 166. That bypasses the trimming/normalization done byvalidatePremiumQueryand is inconsistent with the PR's stated behavior of reading fromreq.validated.🐛 Proposed fix
- const text = req.query.text || 'Please provide text to summarize via ?text= parameter'; + const text = req.validated.text;🤖 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 `@src/server.js` around lines 164 - 172, The summarize route handler is still reading raw req.query.text instead of using the validated value from the validatePremiumQuery middleware; update the GET /api/premium/summarize handler to use req.validated.text (and fall back to the existing placeholder only if undefined) for the variables passed to broadcast and runSummary and for any cost/logging; ensure all references to the input in this handler (broadcast input, runSummary call, resultPreview) use req.validated.text to match the other premium endpoints and preserve trimming/normalization performed by validatePremiumQuery.
🧹 Nitpick comments (1)
src/requestValidation.js (1)
147-150: ⚡ Quick win
collectValidationErroris unused dead code.None of the middleware call this helper (they invoke
assertStringField/assertBudgetdirectly), and it isn't exported. It looks like a leftover from an intended refactor. Either route the validators through it for consistency or remove it.♻️ Proposed removal
-function collectValidationError(name, validator, value, options) { - const result = validator(name, value, options); - return result.valid ? { valid: true, value: result.value } : { valid: false, error: result.error }; -} -🤖 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 `@src/requestValidation.js` around lines 147 - 150, The function collectValidationError is dead code; remove its declaration to avoid unused code bloat and either (A) delete the collectValidationError function entirely from src/requestValidation.js, or (B) if you prefer the refactor, update middleware to call collectValidationError from assertStringField and assertBudget (replace their direct validation calls with collectValidationError(name, validator, value, options) and adapt return shapes accordingly); after making the change, run tests/linters to ensure no remaining references to collectValidationError remain and adjust any callsites to match the returned {valid, value, error} shape if you chose the refactor path.
🤖 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.
Outside diff comments:
In `@src/server.js`:
- Around line 164-172: The summarize route handler is still reading raw
req.query.text instead of using the validated value from the
validatePremiumQuery middleware; update the GET /api/premium/summarize handler
to use req.validated.text (and fall back to the existing placeholder only if
undefined) for the variables passed to broadcast and runSummary and for any
cost/logging; ensure all references to the input in this handler (broadcast
input, runSummary call, resultPreview) use req.validated.text to match the other
premium endpoints and preserve trimming/normalization performed by
validatePremiumQuery.
---
Nitpick comments:
In `@src/requestValidation.js`:
- Around line 147-150: The function collectValidationError is dead code; remove
its declaration to avoid unused code bloat and either (A) delete the
collectValidationError function entirely from src/requestValidation.js, or (B)
if you prefer the refactor, update middleware to call collectValidationError
from assertStringField and assertBudget (replace their direct validation calls
with collectValidationError(name, validator, value, options) and adapt return
shapes accordingly); after making the change, run tests/linters to ensure no
remaining references to collectValidationError remain and adjust any callsites
to match the returned {valid, value, error} shape if you chose the refactor
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55fa3ed9-4e10-4412-9ae1-3f7824179071
📒 Files selected for processing (5)
package.jsonsrc/middleware/errorHandler.jssrc/requestValidation.jssrc/server.jstests/api.validation.test.js
Owner
|
@supremeproton01 can you plz resolve conflict |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server.js (3)
398-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInline API-key check bypasses the structured validator and can 500 on non-string input.
This route replaces
validateConfigApiKey(still imported on Line 14, now unused) with an inlinestartsWithcheck. Two concerns:
- It diverges from the PR's goal of consistent structured 400 validation payloads —
validateConfigApiKeyemitsvalidationError(..., 'INVALID_API_KEY')with structured details, while this path emits a plainError.- If
apiKeyis a truthy non-string (e.g. a JSON number),apiKey.startsWiththrowsTypeError, surfacing as a 500 rather than a structured 400.Also note the AI summary claims
POST /api/config/apikeyis unchanged in this diff, which is inconsistent with these added lines.🛡️ Proposed fix (reuse the validator)
-app.post('/api/config/apikey', apikeyLimiter, (req, res, next) => { - const { apiKey } = req.body; - if (!apiKey || !apiKey.startsWith('sk-ant-')) { - const err = new Error('Invalid API key. Must start with sk-ant-'); - err.status = 400; - err.code = 'INVALID_API_KEY'; - return next(err); - } +app.post('/api/config/apikey', apikeyLimiter, validateConfigApiKey, (req, res, next) => { + const { apiKey } = req.validated; // Security: Log only that a key was updated, never log the key itself console.log(' 🔑 API key updated (ephemeral, session-only)'); setApiKey(apiKey); res.json({ success: true, masked: `sk-ant-...${apiKey.slice(-6)}` }); });🤖 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 `@src/server.js` around lines 398 - 410, Replace the inline startsWith check in the POST /api/config/apikey handler with the existing validateConfigApiKey validator so you emit the same structured 400 responses (use validationError with code 'INVALID_API_KEY') and avoid TypeError on non-strings; specifically, call validateConfigApiKey (or run its validation logic) against req.body.apiKey before calling setApiKey, ensure any validation failure is passed to next(...) as the structured validation error, never log the raw key (keep the existing console message), and return the masked key in the same response shape after successful setApiKey.
165-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRead
textfromreq.validated, notreq.query.Unlike the sibling routes (research/analyze/code read
req.validated.topic/prompt), this handler reads the rawreq.query.texton Line 167, bypassing the sanitized value produced byvalidatePremiumQuery. This also contradicts the AI summary, which states summarize readstextfromreq.validated.🐛 Proposed fix
- const text = req.query.text || 'Please provide text to summarize via ?text= parameter'; + const text = req.validated.text;🤖 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 `@src/server.js` around lines 165 - 177, The route handler for GET /api/premium/summarize is reading raw req.query.text instead of the sanitized value set by validatePremiumQuery; update the handler to use req.validated.text (with the same fallback message if necessary) wherever text is used — i.e., assign const text = req.validated.text || 'Please provide text to summarize via ?text= parameter', use that text for broadcast input truncation, pass it to runSummary, and include it in response/broadcast logic; reference the validatePremiumQuery middleware, the route handler function (app.get('/api/premium/summarize', ...)), and runSummary/priceInfo usage to locate and modify the code.
249-268:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add
orchestrateLimiterto/api/orchestrateroutes
orchestrateLimiteris imported and defined, but it isn’t attached to eitherPOSTorGET /api/orchestrate(both currently use onlyvalidateOrchestrate), leaving the expensiveorchestrate()endpoint unrate-limited and the import effectively unused.🛡️ Proposed fix (re-add limiter)
-app.post('/api/orchestrate', validateOrchestrate, async (req, res, next) => { +app.post('/api/orchestrate', orchestrateLimiter, validateOrchestrate, async (req, res, next) => { try { const { task, budget } = req.validated; const result = await orchestrate(task, budget, broadcast); res.json(result); } catch (err) { next(err); } }); // Also support GET for easy testing -app.get('/api/orchestrate', validateOrchestrate, async (req, res, next) => { +app.get('/api/orchestrate', orchestrateLimiter, validateOrchestrate, async (req, res, next) => { try { const { task, budget } = req.validated; const result = await orchestrate(task, budget, broadcast); res.json(result); } catch (err) { next(err); } });🤖 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 `@src/server.js` around lines 249 - 268, The POST and GET /api/orchestrate handlers are missing the imported orchestrateLimiter middleware, leaving the expensive orchestrate() endpoint unrate-limited; update both route registrations to include orchestrateLimiter (e.g., add orchestrateLimiter before validateOrchestrate) so that requests pass through the limiter before validateOrchestrate and the orchestrate(task, budget, broadcast) call; ensure you reference the existing symbols orchestrateLimiter, validateOrchestrate, and orchestrate when making the change.
🤖 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.
Outside diff comments:
In `@src/server.js`:
- Around line 398-410: Replace the inline startsWith check in the POST
/api/config/apikey handler with the existing validateConfigApiKey validator so
you emit the same structured 400 responses (use validationError with code
'INVALID_API_KEY') and avoid TypeError on non-strings; specifically, call
validateConfigApiKey (or run its validation logic) against req.body.apiKey
before calling setApiKey, ensure any validation failure is passed to next(...)
as the structured validation error, never log the raw key (keep the existing
console message), and return the masked key in the same response shape after
successful setApiKey.
- Around line 165-177: The route handler for GET /api/premium/summarize is
reading raw req.query.text instead of the sanitized value set by
validatePremiumQuery; update the handler to use req.validated.text (with the
same fallback message if necessary) wherever text is used — i.e., assign const
text = req.validated.text || 'Please provide text to summarize via ?text=
parameter', use that text for broadcast input truncation, pass it to runSummary,
and include it in response/broadcast logic; reference the validatePremiumQuery
middleware, the route handler function (app.get('/api/premium/summarize', ...)),
and runSummary/priceInfo usage to locate and modify the code.
- Around line 249-268: The POST and GET /api/orchestrate handlers are missing
the imported orchestrateLimiter middleware, leaving the expensive orchestrate()
endpoint unrate-limited; update both route registrations to include
orchestrateLimiter (e.g., add orchestrateLimiter before validateOrchestrate) so
that requests pass through the limiter before validateOrchestrate and the
orchestrate(task, budget, broadcast) call; ensure you reference the existing
symbols orchestrateLimiter, validateOrchestrate, and orchestrate when making the
change.
Author
done chief @Flamki |
Owner
|
@supremeproton01 its look like there is still some conflicts |
Author
fixed conflicts @Flamki |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.js (1)
179-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandler bypasses validated
textvalue.
validatePremiumQuerysanitizes and validatestextintoreq.validated.text, but this handler reads fromreq.query.textdirectly. This defeats the length guards and trimming applied by the validator, and is inconsistent with the other premium endpoints.🐛 Proposed fix
app.get('/api/premium/summarize', validatePremiumQuery, async (req, res, next) => { try { - const text = req.query.text || 'Please provide text to summarize via ?text= parameter'; + const text = req.validated.text; const priceInfo = pricingConfig.getEndpointInfo('GET /api/premium/summarize');🤖 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 `@src/server.js` around lines 179 - 181, The route handler registered with app.get('/api/premium/summarize', validatePremiumQuery, ...) is incorrectly reading raw input from req.query.text and bypasses the validator; change the handler to use the validated and sanitized value req.validated.text (instead of req.query.text) so the length trimming and guards applied by validatePremiumQuery are respected—remove the fallback default text or keep only as a debug fallback after checking req.validated.text is missing, and update any downstream uses of the local text variable accordingly.
🤖 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 15-21: There are duplicate "test" script keys in package.json so
the first one (running tests/api.validation.test.js) is ignored; update the
single "test" script (the "test" key) to run both desired tests (e.g., chain
node src/agents/settlement-header.test.js && node tests/api.validation.test.js
&& node tests/premium-endpoints.integration.test.js) or keep "test" for the
default suite and ensure "test:validation" is referenced/used in CI; modify the
"test" entry (script name "test") to include api.validation.test.js so
validation tests are executed.
In `@src/middleware/errorHandler.js`:
- Around line 79-81: The statement assigning err.details to body.details in the
errorHandler (the block that checks Array.isArray(err.details) &&
err.details.length > 0) contains a trailing semicolon that violates Prettier;
remove the semicolon after the body.details assignment so the line ends without
a semicolon and run the project's formatter/linter to ensure style compliance.
In `@src/server.js`:
- Around line 423-428: In the '/api/wallet/transactions' route handler (the
async function registered with app.get and middleware
validateWalletTransactions) remove the duplicate declaration "const address"
that comes from req.query and keep the single declaration that reads from
req.validated?.address || config.orchestratorAddress || config.serverAddress;
ensure you delete the second "const address = req.query.address..." line and its
redundant null-check so the handler consistently uses the validated address
fallback (req.validated?.address then config.orchestratorAddress then
config.serverAddress) and returns an empty array if that single address is
falsy.
- Around line 20-25: Add missing imports for the validation middleware functions
used in route definitions: import validatePremiumQuery, validateOrchestrate, and
validateWalletTransactions into the top of src/server.js alongside other
middleware imports (so the route handlers referencing validatePremiumQuery,
validateOrchestrate, and validateWalletTransactions no longer throw
ReferenceError). Ensure you import the exact exported names from the modules
that define them (the validation middleware file(s)) and include them in the
same import block as requestId/requestLogger/errorHandler or the appropriate
middleware import so the symbols are available where routes are registered.
- Around line 326-329: The route handler for app.get('/api/orchestrate') is
ignoring the middleware-validated values by reading directly from req.query with
fallbacks; update the handler to use the values attached by validateOrchestrate
(e.g., use req.validated.task and req.validated.budget or whatever property
validateOrchestrate populates) instead of using req.query.task || 'Research AI
payments' and parseFloat(req.query.budget) || 0.15, and remove the fallback
defaults so the handler relies on the middleware-validated task and budget (use
the budget value as-is or coerce safely with Number() if needed).
- Around line 284-319: In the app.post('/api/orchestrate' ...) handler remove
the stale duplicate extraction and manual validation that pull { task, budget }
from req.body and re-parse budget (the redeclared "{ task, budget } = req.body",
the missing-task check and the parseFloat(... || 0.15)" fallback), and also
remove the premature orchestrate call that runs before the run is created;
instead use the values from req.validated (const { task, budget } =
req.validated), compute budgetNum from that validated budget (or rely on the
middleware) and call orchestrate only once with runBroadcast so
runHistoryStore.createRun, runBroadcast, runHistoryStore.completeRun/failRun and
result.runId all operate on the validated inputs.
---
Outside diff comments:
In `@src/server.js`:
- Around line 179-181: The route handler registered with
app.get('/api/premium/summarize', validatePremiumQuery, ...) is incorrectly
reading raw input from req.query.text and bypasses the validator; change the
handler to use the validated and sanitized value req.validated.text (instead of
req.query.text) so the length trimming and guards applied by
validatePremiumQuery are respected—remove the fallback default text or keep only
as a debug fallback after checking req.validated.text is missing, and update any
downstream uses of the local text variable accordingly.
🪄 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: 263c6fbd-dd48-4e2e-b208-68c49fd785a8
📒 Files selected for processing (3)
package.jsonsrc/middleware/errorHandler.jssrc/server.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.js (1)
179-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandler bypasses validated
textvalue.
validatePremiumQuerysanitizes and validatestextintoreq.validated.text, but this handler reads fromreq.query.textdirectly. This defeats the length guards and trimming applied by the validator, and is inconsistent with the other premium endpoints.🐛 Proposed fix
app.get('/api/premium/summarize', validatePremiumQuery, async (req, res, next) => { try { - const text = req.query.text || 'Please provide text to summarize via ?text= parameter'; + const text = req.validated.text; const priceInfo = pricingConfig.getEndpointInfo('GET /api/premium/summarize');🤖 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 `@src/server.js` around lines 179 - 181, The route handler registered with app.get('/api/premium/summarize', validatePremiumQuery, ...) is incorrectly reading raw input from req.query.text and bypasses the validator; change the handler to use the validated and sanitized value req.validated.text (instead of req.query.text) so the length trimming and guards applied by validatePremiumQuery are respected—remove the fallback default text or keep only as a debug fallback after checking req.validated.text is missing, and update any downstream uses of the local text variable accordingly.
🤖 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 15-21: There are duplicate "test" script keys in package.json so
the first one (running tests/api.validation.test.js) is ignored; update the
single "test" script (the "test" key) to run both desired tests (e.g., chain
node src/agents/settlement-header.test.js && node tests/api.validation.test.js
&& node tests/premium-endpoints.integration.test.js) or keep "test" for the
default suite and ensure "test:validation" is referenced/used in CI; modify the
"test" entry (script name "test") to include api.validation.test.js so
validation tests are executed.
In `@src/middleware/errorHandler.js`:
- Around line 79-81: The statement assigning err.details to body.details in the
errorHandler (the block that checks Array.isArray(err.details) &&
err.details.length > 0) contains a trailing semicolon that violates Prettier;
remove the semicolon after the body.details assignment so the line ends without
a semicolon and run the project's formatter/linter to ensure style compliance.
In `@src/server.js`:
- Around line 423-428: In the '/api/wallet/transactions' route handler (the
async function registered with app.get and middleware
validateWalletTransactions) remove the duplicate declaration "const address"
that comes from req.query and keep the single declaration that reads from
req.validated?.address || config.orchestratorAddress || config.serverAddress;
ensure you delete the second "const address = req.query.address..." line and its
redundant null-check so the handler consistently uses the validated address
fallback (req.validated?.address then config.orchestratorAddress then
config.serverAddress) and returns an empty array if that single address is
falsy.
- Around line 20-25: Add missing imports for the validation middleware functions
used in route definitions: import validatePremiumQuery, validateOrchestrate, and
validateWalletTransactions into the top of src/server.js alongside other
middleware imports (so the route handlers referencing validatePremiumQuery,
validateOrchestrate, and validateWalletTransactions no longer throw
ReferenceError). Ensure you import the exact exported names from the modules
that define them (the validation middleware file(s)) and include them in the
same import block as requestId/requestLogger/errorHandler or the appropriate
middleware import so the symbols are available where routes are registered.
- Around line 326-329: The route handler for app.get('/api/orchestrate') is
ignoring the middleware-validated values by reading directly from req.query with
fallbacks; update the handler to use the values attached by validateOrchestrate
(e.g., use req.validated.task and req.validated.budget or whatever property
validateOrchestrate populates) instead of using req.query.task || 'Research AI
payments' and parseFloat(req.query.budget) || 0.15, and remove the fallback
defaults so the handler relies on the middleware-validated task and budget (use
the budget value as-is or coerce safely with Number() if needed).
- Around line 284-319: In the app.post('/api/orchestrate' ...) handler remove
the stale duplicate extraction and manual validation that pull { task, budget }
from req.body and re-parse budget (the redeclared "{ task, budget } = req.body",
the missing-task check and the parseFloat(... || 0.15)" fallback), and also
remove the premature orchestrate call that runs before the run is created;
instead use the values from req.validated (const { task, budget } =
req.validated), compute budgetNum from that validated budget (or rely on the
middleware) and call orchestrate only once with runBroadcast so
runHistoryStore.createRun, runBroadcast, runHistoryStore.completeRun/failRun and
result.runId all operate on the validated inputs.
---
Outside diff comments:
In `@src/server.js`:
- Around line 179-181: The route handler registered with
app.get('/api/premium/summarize', validatePremiumQuery, ...) is incorrectly
reading raw input from req.query.text and bypasses the validator; change the
handler to use the validated and sanitized value req.validated.text (instead of
req.query.text) so the length trimming and guards applied by
validatePremiumQuery are respected—remove the fallback default text or keep only
as a debug fallback after checking req.validated.text is missing, and update any
downstream uses of the local text variable accordingly.
🪄 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: 263c6fbd-dd48-4e2e-b208-68c49fd785a8
📒 Files selected for processing (3)
package.jsonsrc/middleware/errorHandler.jssrc/server.js
🛑 Comments failed to post (6)
package.json (1)
15-21:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate "test" script key causes validation tests to be skipped.
The
scriptsobject contains two"test"keys (lines 15 and 21). In JSON, duplicate keys are invalid, and parsers will use only the last occurrence. As a result,npm testwill executepremium-endpoints.integration.test.js(line 21) instead ofapi.validation.test.js(line 15), which contradicts the PR objective of adding runtime request validation test coverage.🔧 Proposed fix to remove the duplicate and keep the validation tests
"test": "node src/agents/settlement-header.test.js && node tests/api.validation.test.js", "test:validation": "node tests/api.validation.test.js", "lint": "eslint . --ext .js,.mjs,.cjs", "lint:fix": "eslint . --ext .js,.mjs,.cjs --fix", "format": "prettier --write \"**/*.{js,md,html,yaml,yml}\"", "format:check": "prettier --check \"**/*.{js,md,html,yaml,yml}\"", - "test": "node src/agents/settlement-header.test.js && node tests/premium-endpoints.integration.test.js", "test:parser": "node src/agents/settlement-header.test.js", "test:premium": "node tests/premium-endpoints.integration.test.js",📝 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."test": "node src/agents/settlement-header.test.js && node tests/api.validation.test.js", "test:validation": "node tests/api.validation.test.js", "lint": "eslint . --ext .js,.mjs,.cjs", "lint:fix": "eslint . --ext .js,.mjs,.cjs --fix", "format": "prettier --write \"**/*.{js,md,html,yaml,yml}\"", "format:check": "prettier --check \"**/*.{js,md,html,yaml,yml}\"",🧰 Tools
🪛 Biome (2.4.16)
[error] 15-15: The key test was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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 15 - 21, There are duplicate "test" script keys in package.json so the first one (running tests/api.validation.test.js) is ignored; update the single "test" script (the "test" key) to run both desired tests (e.g., chain node src/agents/settlement-header.test.js && node tests/api.validation.test.js && node tests/premium-endpoints.integration.test.js) or keep "test" for the default suite and ensure "test:validation" is referenced/used in CI; modify the "test" entry (script name "test") to include api.validation.test.js so validation tests are executed.src/middleware/errorHandler.js (1)
79-81:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove trailing semicolon on line 80.
The semicolon violates the Prettier code style configured for this project.
🎨 Proposed fix
if (Array.isArray(err.details) && err.details.length > 0) { - body.details = err.details; + body.details = err.details }📝 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.if (Array.isArray(err.details) && err.details.length > 0) { body.details = err.details }🧰 Tools
🪛 ESLint
[error] 80-80: Delete
;(prettier/prettier)
🤖 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 `@src/middleware/errorHandler.js` around lines 79 - 81, The statement assigning err.details to body.details in the errorHandler (the block that checks Array.isArray(err.details) && err.details.length > 0) contains a trailing semicolon that violates Prettier; remove the semicolon after the body.details assignment so the line ends without a semicolon and run the project's formatter/linter to ensure style compliance.src/server.js (4)
20-25:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing imports for validation middleware functions.
validatePremiumQuery,validateOrchestrate, andvalidateWalletTransactionsare used in route definitions but are never imported. This will cause aReferenceErrorat runtime.🐛 Proposed fix to add missing imports
import { requestId, requestLogger, errorHandler } from './middleware/errorHandler.js' import { orchestrateLimiter, apikeyLimiter } from './middleware/rateLimiter.js' import { logger } from './logger.js' import { adminAuth } from './middleware/auth.js' +import { + validatePremiumQuery, + validateOrchestrate, + validateWalletTransactions, +} from './requestValidation.js' import { registerPremiumRoutes } from './routes/premium-routes.js'📝 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.import { requestId, requestLogger, errorHandler } from './middleware/errorHandler.js' import { orchestrateLimiter, apikeyLimiter } from './middleware/rateLimiter.js' import { logger } from './logger.js' import { adminAuth } from './middleware/auth.js' import { validatePremiumQuery, validateOrchestrate, validateWalletTransactions, } from './requestValidation.js' import { registerPremiumRoutes } from './routes/premium-routes.js' import { createRunHistoryStore } from './storage/run-history.js'🤖 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 `@src/server.js` around lines 20 - 25, Add missing imports for the validation middleware functions used in route definitions: import validatePremiumQuery, validateOrchestrate, and validateWalletTransactions into the top of src/server.js alongside other middleware imports (so the route handlers referencing validatePremiumQuery, validateOrchestrate, and validateWalletTransactions no longer throw ReferenceError). Ensure you import the exact exported names from the modules that define them (the validation middleware file(s)) and include them in the same import block as requestId/requestLogger/errorHandler or the appropriate middleware import so the symbols are available where routes are registered.
284-319:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate extraction overwrites validated values and bypasses middleware validation.
Lines 286-287 correctly extract from
req.validatedand callorchestrate, but lines 288-295 then shadow those variables by re-extracting fromreq.body. This completely bypasses the validation middleware:
validateOrchestraterejects negative/NaN budgets and missing tasks- Line 288 re-declares
{ task, budget }from rawreq.body, shadowing line 286- Line 295 re-parses budget with
|| 0.15fallback that accepts negative values the middleware would rejectThis appears to be leftover code from before the middleware was added. Remove the redundant extraction and manual validation.
🐛 Proposed fix: remove duplicate extraction and manual validation
app.post('/api/orchestrate', validateOrchestrate, async (req, res, next) => { try { const { task, budget } = req.validated; - const result = await orchestrate(task, budget, broadcast); - const { task, budget } = req.body; - if (!task) { - const err = new Error('Missing "task" in request body') - err.status = 400 - err.code = 'MISSING_FIELD' - return next(err) - } - const budgetNum = parseFloat(budget) || 0.15 const run = await runHistoryStore.createRun({ task, - budget: budgetNum, + budget, source: 'POST /api/orchestrate', }) const runBroadcast = (event) => { const eventWithRun = { ...event, runId: run.id } broadcast(eventWithRun) runHistoryStore.appendEvent(run.id, eventWithRun).catch((persistErr) => { logger.warn('run_history_append_failed', { runId: run.id, error: persistErr.message }) }) } let result try { - result = await orchestrate(task, budgetNum, runBroadcast, { correlationId: req.requestId }) + result = await orchestrate(task, budget, runBroadcast, { correlationId: req.requestId }) await runHistoryStore.completeRun(run.id, result) } catch (err) { await runHistoryStore.failRun(run.id, err) throw err } result.runId = run.id res.json(result) } catch (err) { next(err) } })🤖 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 `@src/server.js` around lines 284 - 319, In the app.post('/api/orchestrate' ...) handler remove the stale duplicate extraction and manual validation that pull { task, budget } from req.body and re-parse budget (the redeclared "{ task, budget } = req.body", the missing-task check and the parseFloat(... || 0.15)" fallback), and also remove the premature orchestrate call that runs before the run is created; instead use the values from req.validated (const { task, budget } = req.validated), compute budgetNum from that validated budget (or rely on the middleware) and call orchestrate only once with runBroadcast so runHistoryStore.createRun, runBroadcast, runHistoryStore.completeRun/failRun and result.runId all operate on the validated inputs.
326-329:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandler bypasses validated values from middleware.
validateOrchestratemiddleware requirestaskand rejects negative/NaN budgets, but the handler reads fromreq.querywith fallbacks that defeat this validation:
- Line 328: Falls back to
'Research AI payments'if task missing (middleware already rejects missing task)- Line 329: Uses
parseFloat(...) || 0.15which would accept negative budgets the middleware rejects🐛 Proposed fix
app.get('/api/orchestrate', validateOrchestrate, async (req, res, next) => { try { - const task = req.query.task || 'Research AI payments' - const budget = parseFloat(req.query.budget) || 0.15 + const { task, budget } = req.validated; const run = await runHistoryStore.createRun({ task, budget,🤖 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 `@src/server.js` around lines 326 - 329, The route handler for app.get('/api/orchestrate') is ignoring the middleware-validated values by reading directly from req.query with fallbacks; update the handler to use the values attached by validateOrchestrate (e.g., use req.validated.task and req.validated.budget or whatever property validateOrchestrate populates) instead of using req.query.task || 'Research AI payments' and parseFloat(req.query.budget) || 0.15, and remove the fallback defaults so the handler relies on the middleware-validated task and budget (use the budget value as-is or coerce safely with Number() if needed).
423-428:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
const addressdeclarations will cause a syntax error.Lines 425 and 427 both declare
const addressin the same scope. This is a syntax error that will crash at startup. This appears to be an incomplete merge where both the validated and non-validated versions of the code were left in.Remove lines 427-428 and use the validated address from line 425.
🐛 Proposed fix
app.get('/api/wallet/transactions', validateWalletTransactions, async (req, res, next) => { try { const address = req.validated?.address || config.orchestratorAddress || config.serverAddress; if (!address) return res.json([]); - const address = req.query.address || config.orchestratorAddress || config.serverAddress - if (!address) return res.json([]) // 1. Validate & clamp "limit"📝 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.app.get('/api/wallet/transactions', validateWalletTransactions, async (req, res, next) => { try { const address = req.validated?.address || config.orchestratorAddress || config.serverAddress; if (!address) return res.json([]);🤖 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 `@src/server.js` around lines 423 - 428, In the '/api/wallet/transactions' route handler (the async function registered with app.get and middleware validateWalletTransactions) remove the duplicate declaration "const address" that comes from req.query and keep the single declaration that reads from req.validated?.address || config.orchestratorAddress || config.serverAddress; ensure you delete the second "const address = req.query.address..." line and its redundant null-check so the handler consistently uses the validated address fallback (req.validated?.address then config.orchestratorAddress then config.serverAddress) and returns an empty array if that single address is falsy.
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
2 participants
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.
closes #9
Summary
hooked into API routes so invalid input returns structured 400 errors.
validated request data and enforce non-negative/finite budgets plus text length limits.
cover validation failures for major routes.
Validation
Checklist
.envor private keys committedREADME.mdand examples still accurateSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores