feat: add agent config #371
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces HTTP endpoint exposure for LangChain agent graphs via a new lifecycle hook, enabling dynamic controller registration with support for both invoke and streaming response modes. Core graph objects are refactored to carry application context, type definitions are extended to support agent configuration, and Node.js engine requirements are updated to version 20. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EggContainer as Egg Container
participant LifecycleHook as AgentHttpLoadUnitLifecycleHook
participant GraphHttpController as GraphHttpController
participant CompiledStateGraph as CompiledStateGraph
participant SSETransform as SSE Transform
rect rgb(200, 220, 240)
note over LifecycleHook: Lifecycle: preCreate Phase
EggContainer->>LifecycleHook: preCreate(loadUnit)
LifecycleHook->>LifecycleHook: getModuleConfig(loadUnit)
LifecycleHook->>GraphHttpController: createGraphHttpControllerClass()
LifecycleHook->>EggContainer: register prototype
end
rect rgb(240, 220, 200)
note over Client: HTTP Request Phase
Client->>GraphHttpController: POST /graph/stream {payload}
GraphHttpController->>GraphHttpController: transform array elements to Messages
end
alt Streaming Enabled
rect rgb(220, 240, 200)
note over GraphHttpController: Streaming Flow
GraphHttpController->>CompiledStateGraph: stream(input)
CompiledStateGraph-->>GraphHttpController: async chunk iterator
GraphHttpController->>SSETransform: pipe chunks
SSETransform->>SSETransform: JSON.stringify + "data: " prefix
SSETransform-->>Client: Server-Sent Events
end
else Non-Streaming
rect rgb(240, 240, 200)
note over GraphHttpController: Direct Invoke
GraphHttpController->>CompiledStateGraph: invoke(input)
CompiledStateGraph-->>GraphHttpController: result
GraphHttpController-->>Client: JSON response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (16)
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 |
Summary of ChangesHello @akitaSummer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the LangChain plugin by enabling the configuration and exposure of LangChain agents as HTTP endpoints. It provides a flexible way to integrate compiled state graphs into web applications, supporting streaming capabilities and configurable timeouts. The changes involve updating the object lifecycle to provide application context, introducing new configuration schemas, and implementing the necessary routing and request handling logic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new feature to configure agents and expose them as HTTP endpoints. The implementation is mostly solid, but there are a few critical issues to address. I've found a potential race condition due to incorrect thread_id generation, a bug in config handling, and a potential crash from an unchecked optional property. I've also included suggestions to improve encapsulation and remove reliance on private framework APIs.
|
|
||
| const defaultConfig = { | ||
| configurable: { | ||
| thread_id: process.pid.toString(), |
There was a problem hiding this comment.
Using process.pid for thread_id is incorrect in a web server context. All concurrent requests handled by the same process will share the same thread_id, leading to mixed-up conversation states and data corruption. A unique ID must be generated for each request. Using crypto.randomUUID() is a robust solution. Note: you'll need to add import crypto from 'node:crypto'; at the top of the file.
| thread_id: process.pid.toString(), | |
| thread_id: crypto.randomUUID(), |
|
|
||
| async boundByConfig() { | ||
| const config = ModuleConfigUtil.loadModuleConfigSync(this.proto.unitPath) as ModuleConfig | undefined; | ||
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; |
There was a problem hiding this comment.
The default value for agents is [], but its type LangChainConfigSchemaType['agents'] is Record<string, ...>, which is an object. The correct default value should be an empty object {} to match the type definition. This will prevent potential issues and align the code with its declared type.
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; | |
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? {}; |
| if ((type ?? '').toLocaleLowerCase() === 'http') { | ||
| const router = this.app.router; | ||
| const regExp = pathToRegexp(methodRealPath!, { | ||
| sensitive: true, | ||
| }); | ||
| const handler = this.createHttpHandler(stream, timeout); | ||
| Reflect.apply(router.post, router, | ||
| [ `${this.graphName}.Invoke`, methodRealPath, ...[], handler ]); | ||
| this.app.rootProtoManager.registerRootProto('AgentControllerInvoke', (ctx: Context) => { | ||
| if (regExp.test(ctx.path)) { | ||
| return this.proto; | ||
| } | ||
| }, ''); | ||
| } |
There was a problem hiding this comment.
The path property in the agent configuration is optional, so methodRealPath can be undefined. Using the non-null assertion operator (!) on methodRealPath will cause a runtime error if the path is not provided in the configuration. It's safer to check for the existence of methodRealPath in the conditional statement.
| if ((type ?? '').toLocaleLowerCase() === 'http') { | |
| const router = this.app.router; | |
| const regExp = pathToRegexp(methodRealPath!, { | |
| sensitive: true, | |
| }); | |
| const handler = this.createHttpHandler(stream, timeout); | |
| Reflect.apply(router.post, router, | |
| [ `${this.graphName}.Invoke`, methodRealPath, ...[], handler ]); | |
| this.app.rootProtoManager.registerRootProto('AgentControllerInvoke', (ctx: Context) => { | |
| if (regExp.test(ctx.path)) { | |
| return this.proto; | |
| } | |
| }, ''); | |
| } | |
| if ((type ?? '').toLocaleLowerCase() === 'http' && methodRealPath) { | |
| const router = this.app.router; | |
| const regExp = pathToRegexp(methodRealPath, { | |
| sensitive: true, | |
| }); | |
| const handler = this.createHttpHandler(stream, timeout); | |
| Reflect.apply(router.post, router, | |
| [ `${this.graphName}.Invoke`, methodRealPath, ...[], handler ]); | |
| this.app.rootProtoManager.registerRootProto('AgentControllerInvoke', (ctx: Context) => { | |
| if (regExp.test(ctx.path)) { | |
| return this.proto; | |
| } | |
| }, ''); | |
| } |
| readonly ctx: EggContext; | ||
| readonly daoName: string; | ||
| private _obj: object; | ||
| _obj: object; |
There was a problem hiding this comment.
The _obj property was changed from public (default access modifier) to private. This is a good practice for encapsulation. However, in this PR, it has been changed back to public. Since it's only accessed within the class's methods (via a self closure in createHttpHandler), it can and should remain private to maintain proper encapsulation.
| _obj: object; | |
| private _obj: object; |
| const explicitStatus = (ctx.response as any)._explicitStatus; | ||
|
|
||
| if ( | ||
| // has body | ||
| body != null || | ||
| // status is not set and has no body | ||
| // code should by 204 | ||
| // https://github.com/koajs/koa/blob/master/lib/response.js#L140 | ||
| !explicitStatus | ||
| ) { |
There was a problem hiding this comment.
Relying on the private property _explicitStatus from Koa's response object is fragile and may break with future updates to the framework. This logic can be simplified by just setting ctx.body = body and letting Koa handle the response status correctly (e.g., setting status 204 for an empty body). This would make the code more robust and easier to maintain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
plugin/langchain/test/llm.test.ts (1)
120-125: Remove redundant try-catch block.The catch block simply rethrows the error without any transformation or logging, making it unnecessary.
if (line.startsWith('data: ')) { const data = line.slice(6); - try { - const parsed = JSON.parse(data); - messages.push(parsed); - } catch (e) { - throw e; - } + const parsed = JSON.parse(data); + messages.push(parsed); }plugin/langchain/lib/graph/CompiledStateGraphObject.ts (1)
162-204: Duplicatectx.bodyassignment when not streaming.Line 170 sets
ctx.body = body, then whenstreamis false, line 202 sets it again. The first assignment is redundant.if ( body != null || !explicitStatus ) { - ctx.body = body; if (stream) { ctx.set({ 'content-type': 'text/event-stream', 'cache-control': 'no-cache', 'transfer-encoding': 'chunked', 'X-Accel-Buffering': 'no', }); // ... transform stream logic ... ctx.body = Readable.fromWeb(body as any, { objectMode: true }).pipe(transformStream); } else { ctx.body = body; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
plugin/langchain/app.ts(1 hunks)plugin/langchain/lib/graph/CompiledStateGraphObject.ts(5 hunks)plugin/langchain/lib/graph/CompiledStateGraphProto.ts(1 hunks)plugin/langchain/package.json(1 hunks)plugin/langchain/test/fixtures/apps/langchain/app/modules/bar/module.yml(1 hunks)plugin/langchain/test/fixtures/apps/langchain/config/config.default.js(0 hunks)plugin/langchain/test/llm.test.ts(1 hunks)plugin/langchain/typings/index.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- plugin/langchain/test/fixtures/apps/langchain/config/config.default.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (20)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
🔇 Additional comments (7)
plugin/langchain/test/fixtures/apps/langchain/app/modules/bar/module.yml (1)
21-26: Test fixture configuration looks correct.The agent configuration for
FooGraphaligns with the newLangChainConfigSchemaand will exercise the streaming HTTP endpoint binding in tests.plugin/langchain/lib/graph/CompiledStateGraphProto.ts (1)
27-27: LGTM!The
unitPathproperty is correctly initialized from the load unit and enables module config loading inCompiledStateGraphObject.boundByConfig().Also applies to: 35-35
plugin/langchain/app.ts (1)
30-30: LGTM!Correctly passes the application instance to the factory, enabling HTTP route registration in
CompiledStateGraphObject.plugin/langchain/typings/index.d.ts (1)
112-130: Schema definition looks good.The agent configuration schema correctly defines the optional fields for HTTP binding with streaming support.
plugin/langchain/lib/graph/CompiledStateGraphObject.ts (1)
254-260: Factory pattern update looks correct.The static factory now accepts the application context and returns a properly bound factory function for object creation.
plugin/langchain/test/llm.test.ts (1)
80-133: Test coverage for streaming endpoint looks good.The test correctly validates SSE streaming behavior with proper resource cleanup via
reader.releaseLock()in the finally block. The hardcoded assertionmessages.length === 3is well-founded: the test uses a controlled fixture with the deterministicFooGraphthat has a predictable execution path (START → ACTION producing AIMessage with tool_calls → TOOLS → ACTION processing ToolMessage → END), with each step producing one streamed message. The test setup properly mocks the application via egg-mock with fixtures and starts an SSE server for the MCP client integration.plugin/langchain/package.json (1)
54-56: Node.js version bump to >=20.0.0 is required by @langchain/langgraph.@langchain/langgraph v1.0.0 requires Node.js 20 or higher, which justifies this engine constraint change. The test file confirms this requirement with a runtime check for Node versions above 19.
| async boundByConfig() { | ||
| const config = ModuleConfigUtil.loadModuleConfigSync(this.proto.unitPath) as ModuleConfig | undefined; | ||
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; | ||
| const configName = `${this.graphName.slice(0, 1).toUpperCase()}${this.graphName.slice(1)}`; | ||
| if (Object.keys(agents).includes(configName)) { | ||
| const { path: methodRealPath, type, stream, timeout } = agents[configName]; | ||
|
|
||
| if ((type ?? '').toLocaleLowerCase() === 'http') { | ||
| const router = this.app.router; | ||
| const regExp = pathToRegexp(methodRealPath!, { | ||
| sensitive: true, | ||
| }); | ||
| const handler = this.createHttpHandler(stream, timeout); | ||
| Reflect.apply(router.post, router, | ||
| [ `${this.graphName}.Invoke`, methodRealPath, ...[], handler ]); | ||
| this.app.rootProtoManager.registerRootProto('AgentControllerInvoke', (ctx: Context) => { | ||
| if (regExp.test(ctx.path)) { | ||
| return this.proto; | ||
| } | ||
| }, ''); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation for required path field.
If path is undefined in the config, pathToRegexp(methodRealPath!) will throw a runtime error. Add validation before using the path.
if ((type ?? '').toLocaleLowerCase() === 'http') {
+ if (!methodRealPath) {
+ throw new Error(`Agent ${configName} is configured as HTTP but missing 'path' field`);
+ }
const router = this.app.router;
const regExp = pathToRegexp(methodRealPath!, {📝 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.
| async boundByConfig() { | |
| const config = ModuleConfigUtil.loadModuleConfigSync(this.proto.unitPath) as ModuleConfig | undefined; | |
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; | |
| const configName = `${this.graphName.slice(0, 1).toUpperCase()}${this.graphName.slice(1)}`; | |
| if (Object.keys(agents).includes(configName)) { | |
| const { path: methodRealPath, type, stream, timeout } = agents[configName]; | |
| if ((type ?? '').toLocaleLowerCase() === 'http') { | |
| const router = this.app.router; | |
| const regExp = pathToRegexp(methodRealPath!, { | |
| sensitive: true, | |
| }); | |
| const handler = this.createHttpHandler(stream, timeout); | |
| Reflect.apply(router.post, router, | |
| [ `${this.graphName}.Invoke`, methodRealPath, ...[], handler ]); | |
| this.app.rootProtoManager.registerRootProto('AgentControllerInvoke', (ctx: Context) => { | |
| if (regExp.test(ctx.path)) { | |
| return this.proto; | |
| } | |
| }, ''); | |
| } | |
| } | |
| } | |
| async boundByConfig() { | |
| const config = ModuleConfigUtil.loadModuleConfigSync(this.proto.unitPath) as ModuleConfig | undefined; | |
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; | |
| const configName = `${this.graphName.slice(0, 1).toUpperCase()}${this.graphName.slice(1)}`; | |
| if (Object.keys(agents).includes(configName)) { | |
| const { path: methodRealPath, type, stream, timeout } = agents[configName]; | |
| if ((type ?? '').toLocaleLowerCase() === 'http') { | |
| if (!methodRealPath) { | |
| throw new Error(`Agent ${configName} is configured as HTTP but missing 'path' field`); | |
| } | |
| const router = this.app.router; | |
| const regExp = pathToRegexp(methodRealPath!, { | |
| sensitive: true, | |
| }); | |
| const handler = this.createHttpHandler(stream, timeout); | |
| Reflect.apply(router.post, router, | |
| [ `${this.graphName}.Invoke`, methodRealPath, ...[], handler ]); | |
| this.app.rootProtoManager.registerRootProto('AgentControllerInvoke', (ctx: Context) => { | |
| if (regExp.test(ctx.path)) { | |
| return this.proto; | |
| } | |
| }, ''); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugin/langchain/lib/graph/CompiledStateGraphObject.ts around lines 89 to
111, the code assumes agents[configName].path (methodRealPath) is defined and
passes it to pathToRegexp which will throw if undefined; add an explicit check
that methodRealPath is a non-empty string before using it, and if it's
missing/invalid either log a clear warning/error and skip registering the
route/handler (i.e., return or continue) so runtime crashes are avoided; remove
the non-null assertion usage and guard all subsequent uses (regExp creation,
router.post call, and proto registration) behind this validation.
|
|
||
| async boundByConfig() { | ||
| const config = ModuleConfigUtil.loadModuleConfigSync(this.proto.unitPath) as ModuleConfig | undefined; | ||
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; |
There was a problem hiding this comment.
Incorrect default value type for agents.
agents is typed as a Record<string, ...> but defaults to [] (an array). This should default to {}.
- const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? [];
+ const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? {};📝 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.
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; | |
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? {}; |
🤖 Prompt for AI Agents
In plugin/langchain/lib/graph/CompiledStateGraphObject.ts around line 91, the
variable `agents` is declared with type LangChainConfigSchemaType['agents'] (a
Record<string,...>) but is defaulting to an empty array ([]); change the default
to an empty object ({}) so its runtime value matches its declared Record type
(e.g., const agents = config?.langchain?.agents ?? {}), ensuring
type-compatibility and avoiding runtime type mismatches.
| const defaultConfig = { | ||
| configurable: { | ||
| thread_id: process.pid.toString(), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Thread ID using process.pid is not unique per request.
Using process.pid as thread_id means all concurrent requests share the same conversation thread, which can cause data corruption or unexpected behavior when multiple requests run simultaneously. Consider using a request-specific identifier (e.g., from request headers or a generated UUID).
const defaultConfig = {
configurable: {
- thread_id: process.pid.toString(),
+ thread_id: ctx.request.get('x-thread-id') || ctx.traceId || crypto.randomUUID(),
},
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/langchain/lib/graph/CompiledStateGraphObject.ts around lines 141 to
145, the default thread_id is set to process.pid which is global to the process
and not unique per request; change the code to accept a request-scoped
identifier (e.g., pass thread_id into the constructor or method from request
headers/context) and fall back to a generated UUID (use a UUID v4 generator)
only when no request id is provided, ensuring each request/conversation gets a
unique thread_id.
| } catch (error) { | ||
| callback(error); |
There was a problem hiding this comment.
TypeScript type error in error callback.
The error variable is typed as unknown in the catch block, but callback expects Error | null | undefined. Add a type assertion.
} catch (error) {
- callback(error);
+ callback(error instanceof Error ? error : new Error(String(error)));
}🤖 Prompt for AI Agents
In plugin/langchain/lib/graph/CompiledStateGraphObject.ts around lines 195-196,
the catch block passes the caught variable typed as unknown into callback which
expects Error | null | undefined; change the call to assert or convert the
unknown to an Error before passing it (for example use a type assertion like
casting to Error or wrap non-Error values in new Error(String(error)) so
callback receives Error | null | undefined).
| declare module '@eggjs/tegg' { | ||
| export type LangChainModuleConfig = { | ||
| ChatModel?: ChatModelConfigModuleConfigType; | ||
| langchain?: LangChainConfigSchema; |
There was a problem hiding this comment.
Type error: Use the type alias instead of the schema constant.
LangChainConfigSchema is a runtime schema object, not a type. The property should use LangChainConfigSchemaType to represent the inferred TypeScript type.
export type LangChainModuleConfig = {
ChatModel?: ChatModelConfigModuleConfigType;
- langchain?: LangChainConfigSchema;
+ langchain?: LangChainConfigSchemaType;
};📝 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.
| langchain?: LangChainConfigSchema; | |
| langchain?: LangChainConfigSchemaType; |
🤖 Prompt for AI Agents
In plugin/langchain/typings/index.d.ts around line 138, the declaration uses the
runtime schema constant LangChainConfigSchema where a TypeScript type is
required; replace LangChainConfigSchema with the inferred type alias
LangChainConfigSchemaType so the langchain? property is typed correctly,
updating the type annotation to use LangChainConfigSchemaType instead of the
schema constant.
| const config = ModuleConfigUtil.loadModuleConfigSync(this.proto.unitPath) as ModuleConfig | undefined; | ||
| const agents: LangChainConfigSchemaType['agents'] = config?.langchain?.agents ?? []; | ||
| const configName = `${this.graphName.slice(0, 1).toUpperCase()}${this.graphName.slice(1)}`; | ||
| if (Object.keys(agents).includes(configName)) { |
There was a problem hiding this comment.
- 能不能用 http controller 的方式去包一层？ 不要手写一遍 http 的逻辑
- http 要和 graph 解耦
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
plugin/langchain/lib/graph/CompiledStateGraphObject.ts (1)
23-23: Consider keeping_objprivate for encapsulation.The
_objproperty was previously private and is only accessed via the publicobjgetter (line 124-126). Changing it to public exposes internal state unnecessarily. Based on past review comments, this should remainprivate.- _obj: object; + private _obj: object;plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (2)
63-83: Consider extracting message conversion logic to a separate helper.The message type conversion logic is complex and could be extracted to improve readability and testability. Also, consider handling
undefinedroles more gracefully instead of throwing.function convertToLangChainMessage(obj: { role: string; [key: string]: any }) { switch (obj.role) { case 'human': return new HumanMessage(obj); case 'ai': return new AIMessage(obj); case 'system': return new SystemMessage(obj); case 'tool': return new ToolMessage(obj); default: throw new Error(`Unknown message type: ${obj.role}`); } }
59-59: Add type annotations to method parameters.The
invokemethod parametersctxandargslack type annotations, which reduces type safety.- async invoke(@Context() ctx, @HTTPBody() args) { + async invoke(@Context() ctx: EggContext, @HTTPBody() args: Record<string, unknown>) {You'll need to import
EggContextfrom the appropriate module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/controller-decorator/package.json(1 hunks)plugin/controller/package.json(1 hunks)plugin/controller/test/http/request.test.ts(1 hunks)plugin/langchain/app.ts(3 hunks)plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts(1 hunks)plugin/langchain/lib/graph/CompiledStateGraphObject.ts(3 hunks)plugin/mcp-proxy/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/langchain/app.ts
🧰 Additional context used
🧬 Code graph analysis (2)
plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (7)
core/types/common/ModuleConfig.ts (1)
ModuleReference(1-6)core/types/lifecycle/LifecycleHook.ts (1)
LifecycleHook(12-19)core/metadata/src/model/ProtoDescriptorHelper.ts (1)
ProtoDescriptorHelper(21-181)core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)
EggPrototypeCreatorFactory(13-115)plugin/langchain/typings/index.d.ts (1)
LangChainConfigSchemaType(133-133)core/controller-decorator/src/decorator/http/HTTPMethod.ts (1)
HTTPMethod(7-23)core/core-decorator/src/decorator/ConfigSource.ts (1)
ConfigSourceQualifier(5-9)
plugin/langchain/lib/graph/CompiledStateGraphObject.ts (4)
plugin/langchain/typings/index.d.ts (1)
Application(152-154)core/types/core-decorator/model/InjectObjectInfo.ts (1)
EggObjectName(1-1)core/metadata/src/model/graph/GlobalGraph.ts (1)
proto(143-159)core/runtime/src/model/ContextHandler.ts (1)
ContextHandler(6-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (7)
plugin/mcp-proxy/package.json (1)
63-63: Consistent zod v4 upgrade.Same major version upgrade as other packages in this PR. Ensure all zod schema definitions and validations in this package are compatible with v4 APIs.
core/controller-decorator/package.json (1)
51-51: Core package zod upgrade aligns with repo-wide update.As a core package, this upgrade will propagate to dependent packages. Verify compatibility across the codebase.
plugin/controller/test/http/request.test.ts (1)
33-33: Good fix: removing.onlyfrom test.Removing
.onlyensures the full test suite runs, not just this isolated test.plugin/langchain/lib/graph/CompiledStateGraphObject.ts (2)
26-36: LGTM: App injection pattern.The constructor and property additions for
app: Applicationare well-structured, enabling the graph object to access application context for HTTP controller registration.
128-134: Clean factory function pattern.The curried
createObject(app)returning an async factory function is a good approach for injecting the application instance while maintaining the existing object creation signature.plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (1)
34-50: LGTM: Lifecycle hook structure.The
preCreatehook properly iterates through module configurations and dynamically registers HTTP controllers for agents withtype: 'http'. The prototype descriptor creation and registration flow is correct.plugin/controller/package.json (1)
67-67: zod v4 is already correctly integrated in this package.The codebase is using the v4 subpath import (
import * as z from 'zod/v4') with v4-compatible APIs (e.g.,z.string().describe()). No v3-style breaking change patterns were detected in the codebase. The migration to zod v4 is complete and compatible.
| @HTTPMethod({ | ||
| path: config.path!, | ||
| method: HTTPMethodEnum.POST, | ||
| timeout: config.timeout, | ||
| }) |
There was a problem hiding this comment.
Add validation for config.path before using non-null assertion.
The path property in the agent configuration is optional, so config.path can be undefined. Using the non-null assertion (!) will cause a runtime error if the path is not provided. This should be validated before reaching this point.
The validation should occur in preCreate before calling #createGraphHttpControllerClass:
for (const [ graphName, config ] of moduleConfigs) {
- if (config?.type === 'http') {
+ if (config?.type === 'http' && config.path) {
const GraphHttpController = this.#createGraphHttpControllerClass(loadUnit, graphName, config);Or add an explicit error if path is missing:
#createGraphHttpControllerClass(loadUnit: LoadUnit, graphName: string, config: ValueOf<LangChainConfigSchemaType['agents']>) {
+ if (!config.path) {
+ throw new Error(`Agent ${graphName} is configured as HTTP but missing 'path' field`);
+ }
class GraphHttpController {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts around lines 54
to 58, the code uses config.path! with a non-null assertion but config.path is
optional; add validation in preCreate (before calling
#createGraphHttpControllerClass) to check that config.path is defined and is a
non-empty string, and if not either supply a default path or throw a clear,
descriptive error (e.g., throw new Error("Agent HTTP config.path is required"))
so that #createGraphHttpControllerClass never receives an undefined path; ensure
the validated path is passed into the decorator call instead of using the
non-null assertion.
| const defaultConfig = { | ||
| configurable: { | ||
| thread_id: process.pid.toString(), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Critical: thread_id using process.pid is not unique per request.
Using process.pid as thread_id means all concurrent requests within the same process share the same conversation thread. This causes conversation state corruption when multiple requests run simultaneously. Use a request-specific identifier instead.
const defaultConfig = {
configurable: {
- thread_id: process.pid.toString(),
+ thread_id: ctx.request.get('x-thread-id') || ctx.traceId || crypto.randomUUID(),
},
};Add the import at the top of the file:
import crypto from 'node:crypto';🤖 Prompt for AI Agents
In plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts around lines 85
to 89, the defaultConfig sets configurable.thread_id to process.pid which is
shared across all requests; replace this with a request-unique identifier by
importing crypto (import crypto from 'node:crypto') at the top and setting
configurable.thread_id = crypto.randomUUID() (or
crypto.randomBytes(16).toString('hex')) when building the config per request
rather than using process.pid, ensuring the config is created inside the request
lifecycle so each request gets its own thread_id.
| } catch (error) { | ||
| callback(error); | ||
| } |
There was a problem hiding this comment.
TypeScript type error: error is unknown in catch block.
The error variable is typed as unknown in the catch block, but callback expects Error | null | undefined. Add proper type handling.
} catch (error) {
- callback(error);
+ callback(error instanceof Error ? error : new Error(String(error)));
}📝 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.
| } catch (error) { | |
| callback(error); | |
| } | |
| } catch (error) { | |
| callback(error instanceof Error ? error : new Error(String(error))); | |
| } |
🤖 Prompt for AI Agents
In plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts around lines 117
to 119, the catch block receives an unknown-typed error but calls callback with
that value; change the handling to narrow the type before calling callback: test
if error is an instance of Error and pass it directly, otherwise create a new
Error(String(error)) (or wrap with a descriptive message) and pass that to
callback so the callback receives Error | null | undefined as expected.
akitaSummer
force-pushed
the
feat/agent_path
branch
from
e6a57e1 to
e773e9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
plugin/langchain/lib/graph/CompiledStateGraphObject.ts (1)
23-23:_objvisibility changed from private - consider keeping it private.The
_objproperty was previously private. The public getterobj(line 124-126) already provides access to this property. Making_objpublic breaks encapsulation without apparent necessity.This was flagged in a past review. Keep
_objprivate:- _obj: object; + private _obj: object;plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (4)
34-49: Add validation forconfig.pathbefore creating controller.The
config.pathproperty is optional, but the code usesconfig.path!(non-null assertion) in the decorator. This will cause a runtime error if path is undefined.This was flagged in past reviews. Add validation in
preCreate:for (const [ graphName, config ] of moduleConfigs) { - if (config?.type === 'http') { + if (config?.type === 'http' && config.path) { const GraphHttpController = this.#createGraphHttpControllerClass(loadUnit, graphName, config);
85-89: Critical:thread_idusingprocess.pidcauses conversation state corruption.Using
process.pidasthread_idmeans all concurrent requests within the same process share the same conversation thread. This causes conversation state corruption when multiple users or requests run simultaneously.This was flagged as critical in past reviews. Use a request-unique identifier:
const defaultConfig = { configurable: { - thread_id: process.pid.toString(), + thread_id: ctx.request.get('x-thread-id') || ctx.traceId || crypto.randomUUID(), }, };
117-119: TypeScript type error:errorisunknownin catch block.The
errorvariable caught in the try-catch is typed asunknown, butcallbackexpectsError | null | undefined. This causes a TypeScript compilation error.This was flagged in past reviews. Add type handling:
} catch (error) { - callback(error); + callback(error instanceof Error ? error : new Error(String(error))); }
1-16: Missingcryptoimport for UUID generation.The file will need
cryptoimported to fix thethread_idissue flagged below.Add import at top of file:
+import crypto from 'node:crypto'; import { ConfigSourceQualifier,
🧹 Nitpick comments (2)
plugin/langchain/app.ts (1)
18-18: Consider making the field type explicit for clarity.The
#agentHttpLoadUnitHookfield lacks thereadonlymodifier unlike sibling hooks, which is correct since it's assigned inconfigWillLoad. However, adding an explicit type annotation would improve readability.- #agentHttpLoadUnitHook: AgentHttpLoadUnitLifecycleHook; + #agentHttpLoadUnitHook: AgentHttpLoadUnitLifecycleHook | undefined;This makes the potentially-undefined state explicit before
configWillLoadruns.plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (1)
63-83: Potential runtime error when accessing empty array.Line 64 accesses
value[0]without checking if the array is non-empty. Ifvalueis an empty array,value[0]isundefined, andtypeof undefined === 'object'isfalse, so it would skip the block. However, this edge case handling is implicit and could be made clearer.Make the intent explicit:
- if (Array.isArray(value) && typeof value[0] === 'object') { + if (Array.isArray(value) && value.length > 0 && typeof value[0] === 'object') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/controller-decorator/package.json(1 hunks)core/langchain-decorator/package.json(1 hunks)plugin/controller/package.json(1 hunks)plugin/controller/test/http/request.test.ts(1 hunks)plugin/langchain/app.ts(3 hunks)plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts(1 hunks)plugin/langchain/lib/graph/CompiledStateGraphObject.ts(3 hunks)plugin/mcp-proxy/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/controller/test/http/request.test.ts
- plugin/mcp-proxy/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
plugin/langchain/lib/graph/CompiledStateGraphObject.ts (6)
plugin/langchain/typings/index.d.ts (1)
Application(152-154)core/types/core-decorator/model/InjectObjectInfo.ts (1)
EggObjectName(1-1)plugin/langchain/lib/graph/CompiledStateGraphProto.ts (1)
CompiledStateGraphProto(17-66)core/runtime/src/model/ContextHandler.ts (1)
ContextHandler(6-19)core/lifecycle/src/IdenticalObject.ts (1)
IdenticalUtil(3-39)core/types/metadata/model/EggPrototype.ts (1)
EggPrototype(117-154)
plugin/langchain/app.ts (2)
plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (1)
AgentHttpLoadUnitLifecycleHook(27-140)plugin/langchain/lib/graph/CompiledStateGraphProto.ts (1)
CompiledStateGraphProto(17-66)
plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (4)
core/types/common/ModuleConfig.ts (1)
ModuleReference(1-6)plugin/langchain/typings/index.d.ts (1)
LangChainConfigSchemaType(133-133)core/controller-decorator/src/decorator/http/HTTPMethod.ts (1)
HTTPMethod(7-23)core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (10)
core/controller-decorator/package.json (1)
51-51: Verify zod v4 compatibility and Node engine version consistency.Same concern as plugin/controller: zod upgraded to ^4 (line 51, production dependency), but Node engine remains at ">=14.0.0" (lines 36-37). Confirm zod v4 compatibility and align Node engine requirements across the PR if moving to >=20.0.0.
Also applies to: 36-37
core/langchain-decorator/package.json (1)
60-61: Clarify zod placement as devDependency and verify Node engine consistency.Zod is added as a devDependency (lines 60-61), whereas in plugin/controller (line 67) and core/controller-decorator (line 51) it's a production dependency. Clarify whether zod should be in dependencies or devDependencies for this package. Additionally, Node engine remains at ">=14.0.0" (lines 35-36); align with the broader PR if moving to >=20.0.0.
Also applies to: 35-36
plugin/controller/package.json (1)
67-67: Verify zod v4 compatibility and Node engine version alignment.The zod dependency is upgraded from ^3.24.4 to ^4 (line 67), a major version bump. Review the zod v4 migration guide to confirm compatibility with existing validation schemas. Additionally, this file declares Node engine ">=14.0.0" (lines 47-48), but the PR appears to update Node requirements to >=20.0.0 elsewhere. Align the Node engine version if the broader PR is standardizing to >=20.0.0.
plugin/langchain/app.ts (2)
29-35: LGTM! Lifecycle registration and factory wiring look correct.The hook is registered before other object lifecycle hooks, which ensures the HTTP controllers are created during load unit initialization. The
createObject(this.#app)call correctly passes the application instance to the factory.
43-45: Good defensive check, but cleanup order may not match registration order.The conditional check for
#agentHttpLoadUnitHookis good. However, lifecycles are typically cleaned up in reverse order of registration (LIFO). Currently,#agentHttpLoadUnitHookis registered second inconfigWillLoad(after constructor registers#graphLoadUnitHook), but it's deleted first inbeforeClose.Consider whether the cleanup order matters for this lifecycle. If the order is significant, ensure it follows LIFO:
async beforeClose() { - if (this.#agentHttpLoadUnitHook) { - this.#app.loadUnitLifecycleUtil.deleteLifecycle(this.#agentHttpLoadUnitHook); - } this.#app.eggObjectLifecycleUtil.deleteLifecycle(this.#graphObjectHook); this.#app.eggObjectLifecycleUtil.deleteLifecycle(this.#boundModelObjectHook); this.#app.loadUnitLifecycleUtil.deleteLifecycle(this.#graphLoadUnitHook); this.#app.eggPrototypeLifecycleUtil.deleteLifecycle(this.#graphPrototypeHook); + if (this.#agentHttpLoadUnitHook) { + this.#app.loadUnitLifecycleUtil.deleteLifecycle(this.#agentHttpLoadUnitHook); + } }plugin/langchain/lib/graph/CompiledStateGraphObject.ts (2)
26-35: LGTM! Constructor updated to accept Application instance.The constructor correctly accepts and stores the
appparameter. Theappfield is markedreadonly, which is appropriate since it shouldn't change after construction.
128-134: Good curried factory pattern implementation.The
createObjectstatic method now returns a factory function that closes over theappinstance. This cleanly separates the application-level configuration from per-object creation, allowing the factory to be registered once with the app context.plugin/langchain/lib/agent/AgentHttpLoadUnitLifecycleHook.ts (3)
122-122: Type escape withas anyon stream result.
Readable.fromWeb(res as any, ...)uses a type assertion to work around potential type incompatibility between LangGraph's stream result and Web Streams API.Verify that
resfromstreamFuncis actually a web-compatibleReadableStream. If it's a different stream type, this could cause runtime issues. Consider adding a runtime check or documenting the expected type.
127-128: LGTM! Controller decoration approach.Applying
HTTPControllerandConfigSourceQualifierdecorators dynamically is a clean pattern for runtime controller generation.
133-139: LGTM! Module config extraction logic.The
#getModuleConfigmethod correctly navigates the nested config structure and returns agent entries only when present.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
akitaSummer
force-pushed
the
feat/agent_path
branch
from
e773e9f to
68d44d8
Compare
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.