fix(graph): report trigger failures separately#341
Conversation
|
@wyh0626 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR separates error handling for knowledge graph API endpoints. Graph-query, graph-stats, and graph-extract endpoints now return 503 when graph extraction is disabled, but return 500 with diagnostic details when the underlying SDK trigger fails. A helper function standardizes failure responses. Startup logging guides users to enable the feature. Tests validate all three scenarios. ChangesKnowledge Graph Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/triggers/api.ts (1)
1064-1079:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWhitelist and validate graph request payloads before calling
sdk.trigger.Line 1078 and Line 1127 forward raw request bodies, which bypasses boundary whitelisting and lets unvalidated fields flow into backend functions.
🔧 Suggested fix
sdk.registerFunction("api::graph-query", async ( req: ApiRequest<{ startNodeId?: string; nodeType?: string; maxDepth?: number; query?: string; }>, ): Promise<Response> => { const authErr = checkAuth(req, secret); if (authErr) return authErr; if (!isGraphExtractionEnabled()) return graphDisabledResponse(); const functionId = "mem::graph-query"; + const body = (req.body ?? {}) as Record<string, unknown>; + const maxDepth = parseOptionalPositiveInt(body.maxDepth); + if (maxDepth === null) { + return { status_code: 400, body: { error: "maxDepth must be a positive integer" } }; + } + const payload: { + startNodeId?: string; + nodeType?: string; + maxDepth?: number; + query?: string; + } = {}; + if (typeof body.startNodeId === "string") payload.startNodeId = body.startNodeId; + if (typeof body.nodeType === "string") payload.nodeType = body.nodeType; + if (typeof body.query === "string") payload.query = body.query; + if (maxDepth !== undefined) payload.maxDepth = maxDepth; try { - const result = await sdk.trigger({ function_id: functionId, payload: req.body || {} }); + const result = await sdk.trigger({ function_id: functionId, payload }); return { status_code: 200, body: result }; } catch (err) { return graphRequestFailedResponse(functionId, err); } }, ); @@ sdk.registerFunction("api::graph-extract", async (req: ApiRequest<{ observations: unknown[] }>): Promise<Response> => { @@ const functionId = "mem::graph-extract"; try { - const result = await sdk.trigger({ function_id: functionId, payload: req.body }); + const result = await sdk.trigger({ + function_id: functionId, + payload: { observations: req.body.observations }, + }); return { status_code: 200, body: result }; } catch (err) { return graphRequestFailedResponse(functionId, err); } }, );As per coding guidelines, "REST endpoints must whitelist fields — never pass raw request body to sdk.trigger()" and "Input validation must occur at system boundaries (MCP handlers, REST endpoints)".
Also applies to: 1111-1128
🤖 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/triggers/api.ts` around lines 1064 - 1079, The handler registered via sdk.registerFunction("api::graph-query") currently forwards req.body directly into sdk.trigger (function_id "mem::graph-query"), bypassing boundary validation; replace that by whitelisting and validating only the allowed fields (startNodeId:string?, nodeType:string?, maxDepth:number? with sensible bounds/default, query:string?) at the REST boundary (inside the async handler after checkAuth and isGraphExtractionEnabled checks) and build a new payload object with those validated/coerced values before calling sdk.trigger; apply the same whitelist/validation pattern to the other api handler that also forwards req.body to sdk.trigger so no raw request body is passed through.
🤖 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/triggers/api.ts`:
- Around line 1064-1079: The handler registered via
sdk.registerFunction("api::graph-query") currently forwards req.body directly
into sdk.trigger (function_id "mem::graph-query"), bypassing boundary
validation; replace that by whitelisting and validating only the allowed fields
(startNodeId:string?, nodeType:string?, maxDepth:number? with sensible
bounds/default, query:string?) at the REST boundary (inside the async handler
after checkAuth and isGraphExtractionEnabled checks) and build a new payload
object with those validated/coerced values before calling sdk.trigger; apply the
same whitelist/validation pattern to the other api handler that also forwards
req.body to sdk.trigger so no raw request body is passed through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5e426e0-718b-4882-8c84-0de7b69396af
📒 Files selected for processing (3)
src/index.tssrc/triggers/api.tstest/api-graph.test.ts
1 participant
Summary
Refs #338.
The graph REST endpoints were treating every graph trigger failure as if graph extraction was disabled. That made a real backend error, for example
iii::engine Function not found: mem::graph-stats, show up as503 Knowledge graph not enabled.This keeps the disabled-flag response, but only uses it when
GRAPH_EXTRACTION_ENABLEDis actually off. If the flag is on and the underlying graph function fails, the REST endpoint now returns a 500 with the function id and the original error message. That should make reports like #338 much easier to diagnose.I also added a startup log when graph extraction is off. The log calls out that
GRAPH_EXTRACTION_ENABLED=truehas to be set in the process running agentmemory, or in~/.agentmemory/.env; setting it only inside the iii Docker container will not enable graph registration for the host worker.#308 looks related from the user's point of view because graph extraction is skipped there too, but it is a different problem: SessionEnd is unreliable, so the extraction path may never run. This PR only fixes the misleading REST error handling around #338.
Tests
npm test -- test/api-graph.test.ts --reporter=dot npm run buildBoth pass locally.
I also ran
git diff --check.Note: the full suite still has existing failures in
test/mcp-standalone.test.tsin this workspace. Those failures are on the standalone MCP/proxy path and are unrelated to the graph REST changes here.Summary by CodeRabbit
Bug Fixes
Tests