[AI] Add confirmation-required field to intent validation API#742
Merged
Conversation
) - Extended paymentIntentSchema to include requiresConfirmation field - Added logic to flag high-value payments (>100 USDC or >1000 XLM) as requiring confirmation - Updated /v1/intents/validate endpoint to return confirmation metadata - Added comprehensive tests for the new confirmation field Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughA new local helper function ChangesPayment Intent Confirmation Logic
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
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 unit tests (beta)
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: 2
🧹 Nitpick comments (2)
services/ai-agent/src/server.ts (1)
12-14: ⚡ Quick winHandle potential
NaNfromparseFloat.If
intent.amountcontains a malformed string that passes the regex but fails to parse (edge case),parseFloatreturnsNaN, andNaN > thresholdis alwaysfalse. While the schema regex should prevent this, defensive coding is safer if the function is reused.🛡️ Proposed defensive fix
const amount = parseFloat(intent.amount); + if (isNaN(amount)) { + return false; + } const threshold = intent.asset === 'USDC' ? 100 : 1000;🤖 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 `@services/ai-agent/src/server.ts` around lines 12 - 14, The current logic uses parseFloat(intent.amount) and compares to threshold but doesn't handle parseFloat returning NaN; update the code around parseFloat(intent.amount) (variables: amount, threshold, intent.amount) to detect NaN (Number.isNaN(amount)) and handle it defensively—either throw a clear error or return false (and optionally log) before doing amount > threshold so malformed amounts never silently pass/fail due to NaN behavior.services/ai-agent/src/server.test.ts (1)
63-106: ⚡ Quick winConsider adding boundary condition tests.
The test suite covers amounts above and below thresholds but doesn't test exact boundary values (100 USDC, 1000 XLM). Since the requirement is "greater than" (not "greater than or equal to"), adding tests for exactly 100 USDC and exactly 1000 XLM would verify the boundary behavior:
💡 Suggested additional test cases
it('returns 200 with requiresConfirmation=false for exactly 100 USDC', async () => { const res = await request(app) .post('/v1/intents/validate') .send({ type: 'payment', amount: '100', asset: 'USDC', destination: 'GABC123', }); expect(res.status).toBe(200); expect(res.body.requiresConfirmation).toBe(false); }); it('returns 200 with requiresConfirmation=false for exactly 1000 XLM', async () => { const res = await request(app) .post('/v1/intents/validate') .send({ type: 'payment', amount: '1000', asset: 'XLM', destination: 'GABC123', }); expect(res.status).toBe(200); expect(res.body.requiresConfirmation).toBe(false); });🤖 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 `@services/ai-agent/src/server.test.ts` around lines 63 - 106, Add two boundary tests in services/ai-agent/src/server.test.ts that POST to /v1/intents/validate with amount '100' for asset 'USDC' and amount '1000' for asset 'XLM' to assert status 200 and requiresConfirmation is false; create tests similar to the existing cases (e.g., the tests named "returns 200 with valid payment intent below threshold" and the above-threshold tests) so they follow the same structure and assert res.body.valid === true and res.body.intent.type === 'payment' while specifically checking requiresConfirmation === false for the exact boundary values.
🤖 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 `@services/ai-agent/src/schemas/intent.ts`:
- Line 11: The schema's requiresConfirmation boolean default conflicts with the
server-computed top-level requiresConfirmation; remove the requiresConfirmation
field from the intent Zod schema (delete the requiresConfirmation:
z.boolean().default(false) entry in services/ai-agent/src/schemas/intent.ts) so
the server-only computed value remains the single source of truth, and leave
server.ts's computation/response logic (the top-level requiresConfirmation added
to the response) unchanged; alternatively, if you prefer schema-owned values,
adjust server.ts to assign the computed value into
parsed.data.requiresConfirmation before returning, but default action: remove
the schema field.
In `@services/ai-agent/src/server.ts`:
- Line 10: The requiresConfirmation function currently types its parameter as
any; change its signature to accept intent: Intent and add (or ensure) an import
for Intent from "./schemas/intent" so the parameter uses the proper Intent type;
update any internal usages that rely on the previous loose typing if necessary
to satisfy the new type and ensure exports/imports remain consistent (target
function: requiresConfirmation).
---
Nitpick comments:
In `@services/ai-agent/src/server.test.ts`:
- Around line 63-106: Add two boundary tests in
services/ai-agent/src/server.test.ts that POST to /v1/intents/validate with
amount '100' for asset 'USDC' and amount '1000' for asset 'XLM' to assert status
200 and requiresConfirmation is false; create tests similar to the existing
cases (e.g., the tests named "returns 200 with valid payment intent below
threshold" and the above-threshold tests) so they follow the same structure and
assert res.body.valid === true and res.body.intent.type === 'payment' while
specifically checking requiresConfirmation === false for the exact boundary
values.
In `@services/ai-agent/src/server.ts`:
- Around line 12-14: The current logic uses parseFloat(intent.amount) and
compares to threshold but doesn't handle parseFloat returning NaN; update the
code around parseFloat(intent.amount) (variables: amount, threshold,
intent.amount) to detect NaN (Number.isNaN(amount)) and handle it
defensively—either throw a clear error or return false (and optionally log)
before doing amount > threshold so malformed amounts never silently pass/fail
due to NaN behavior.
🪄 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: 571998c2-f835-4a42-aff1-62307c775798
📒 Files selected for processing (3)
services/ai-agent/src/schemas/intent.tsservices/ai-agent/src/server.test.tsservices/ai-agent/src/server.ts
| amount: z.string().regex(/^\d+(\.\d+)?$/, 'Invalid amount format'), | ||
| asset: z.enum(['XLM', 'USDC']), | ||
| destination: z.string().min(1, 'Destination is required'), | ||
| requiresConfirmation: z.boolean().default(false), |
There was a problem hiding this comment.
Schema field conflicts with server-computed value.
The schema defines requiresConfirmation with a default, but server.ts computes this value dynamically and places it at the response root level (not mutating intent.requiresConfirmation). This creates two separate fields:
intent.requiresConfirmation— from the schema default (alwaysfalseunless a client sendstrue)- Top-level
requiresConfirmation— computed by the server based on thresholds
This is confusing and creates inconsistent state. Consider one of these fixes:
Option A (recommended based on current server.ts implementation): Remove requiresConfirmation from the schema entirely, since it's computed server-side only:
📐 Proposed fix: Remove from schema
export const paymentIntentSchema = z.object({
type: z.literal('payment'),
amount: z.string().regex(/^\d+(\.\d+)?$/, 'Invalid amount format'),
asset: z.enum(['XLM', 'USDC']),
destination: z.string().min(1, 'Destination is required'),
- requiresConfirmation: z.boolean().default(false),
});Option B: Keep the schema field and have server.ts mutate it instead of adding a separate response field. This would require changing server.ts to:
parsed.data.requiresConfirmation = requiresConfirmation(parsed.data);
return res.status(200).json({ valid: true, intent: parsed.data });🤖 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 `@services/ai-agent/src/schemas/intent.ts` at line 11, The schema's
requiresConfirmation boolean default conflicts with the server-computed
top-level requiresConfirmation; remove the requiresConfirmation field from the
intent Zod schema (delete the requiresConfirmation: z.boolean().default(false)
entry in services/ai-agent/src/schemas/intent.ts) so the server-only computed
value remains the single source of truth, and leave server.ts's
computation/response logic (the top-level requiresConfirmation added to the
response) unchanged; alternatively, if you prefer schema-owned values, adjust
server.ts to assign the computed value into parsed.data.requiresConfirmation
before returning, but default action: remove the schema field.
| * Determines if a payment intent requires confirmation based on amount. | ||
| * High-value payments (e.g., >100 USDC or >1000 XLM) require confirmation. | ||
| */ | ||
| function requiresConfirmation(intent: any): boolean { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace any type with proper type annotation.
The intent parameter should be typed as Intent (imported from ./schemas/intent) to provide type safety and better IDE support.
🔧 Proposed fix
-function requiresConfirmation(intent: any): boolean {
+function requiresConfirmation(intent: Intent): boolean {🤖 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 `@services/ai-agent/src/server.ts` at line 10, The requiresConfirmation
function currently types its parameter as any; change its signature to accept
intent: Intent and add (or ensure) an import for Intent from "./schemas/intent"
so the parameter uses the proper Intent type; update any internal usages that
rely on the previous loose typing if necessary to satisfy the new type and
ensure exports/imports remain consistent (target function:
requiresConfirmation).
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.
Summary
Changes
Test plan
Closes #682
Generated with Devin (https://cli.devin.ai/docs)
Summary by CodeRabbit