[WIP] add agent run controllers #380
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. 📝 WalkthroughWalkthroughAdds a LangGraph streaming API and supporting infrastructure: a streaming RunsController (SSE), comprehensive Zod schemas and TypeScript types, SSE utilities, Zod error middleware, in-memory Runs/Assistants repositories and services, graph/tool test fixtures, package updates, and unit/e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RunsController
participant Zod as ZodValidator
participant RunsService
participant SSE as SSEStreamWriter
participant Repo as RunsRepository
Note over Client,RunsController: POST /api/runs/stream (RunCreateDTO)
Client->>RunsController: POST /api/runs/stream
RunsController->>Zod: RunCreate.parse(payload)
alt validation fails
Zod-->>RunsController: ZodError
RunsController->>Client: 422 JSON (issues)
else validation succeeds
Zod-->>RunsController: validated payload
RunsController->>RunsService: createValidRun(payload)
RunsService->>Repo: put(run)
Repo-->>RunsService: persisted run
RunsService-->>RunsController: run metadata
RunsController->>SSE: streamSSE(init response)
RunsController->>SSE: writeSSE(event: metadata)
SSE-->>Client: "event: metadata\n data: {...}\n\n"
RunsController->>SSE: writeSSE(event: values - assistant)
SSE-->>Client: assistant chunks...
RunsController->>SSE: writeSSE(event: end)
SSE-->>Client: end event and close
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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 @RaoHai, 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 lays the foundational groundwork for integrating LangGraph run management within the application. It introduces a dedicated controller for handling LangGraph runs, specifically focusing on streaming stateless run data using Server-Sent Events. The changes also include the adoption of Zod for defining and validating API request schemas, ensuring data integrity, and the creation of comprehensive TypeScript types for LangGraph API entities. This setup is crucial for enabling real-time interaction and robust data handling for LangGraph functionalities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
这个 PR 引入了 agent run controller 的基本框架，包括了 controller、schemas、types 和 SSE 工具函数，并添加了初步的测试。这是一个很好的开端，为后续功能的实现奠定了基础。
我的主要反馈如下：
- 测试代码: 测试文件中使用了
describe.only，这会阻止其他测试用例的执行，需要移除。同时，建议将require改为import以保持代码风格一致。 - 日志记录: 在 controller 中，建议使用
ctx.logger代替console.error进行错误记录，以便于统一管理日志。 - 类型安全: 在
schemas.ts文件中，建议使用z.unknown()代替z.any()来增强 Zod schema 的类型安全性。
这些是针对当前 WIP 状态的建议，期待后续的完整实现。
| import path from 'path'; | ||
| import assert from 'assert'; | ||
|
|
||
| describe.only('plugin/langchain/test/agent.test.ts', () => { |
There was a problem hiding this comment.
describe.only 会导致 CI 环境中只运行这个测试文件，而跳过其他所有测试。请将其修改为 describe，以确保所有测试都能被执行。
| describe.only('plugin/langchain/test/agent.test.ts', () => { | |
| describe('plugin/langchain/test/agent.test.ts', () => { |
| }); | ||
| } catch (error) { | ||
| // 错误处理 | ||
| console.error('Error streaming run:', error); |
There was a problem hiding this comment.
建议使用 ctx.logger.error 代替 console.error 来记录错误。这样可以利用应用统一的日志配置，方便后续的日志收集和分析。
| console.error('Error streaming run:', error); | |
| ctx.logger.error('Error streaming run:', error); |
| export const Config = z.object({ | ||
| tags: z.array(z.string()).optional(), | ||
| recursion_limit: z.number().int().optional(), | ||
| configurable: z.object({}).catchall(z.any()).optional(), |
There was a problem hiding this comment.
在 Zod schema 中，使用 z.unknown() 通常比 z.any() 更安全。z.unknown() 会强制使用者在访问数据之前进行类型检查，从而减少潜在的运行时错误。文件中多处使用了 catchall(z.any())，建议统一修改为 catchall(z.unknown()) 以增强类型安全性。例如，此处的 AssistantConfigurable (line 23) 就正确地使用了 z.unknown()。
| configurable: z.object({}).catchall(z.any()).optional(), | |
| configurable: z.object({}).catchall(z.unknown()).optional(), |
plugin/langchain/test/agent.test.ts
Outdated
| // https://github.com/langchain-ai/langchainjs/blob/main/libs/langchain/package.json#L9 | ||
| if (parseInt(process.version.slice(1, 3)) > 19) { | ||
| let app; | ||
| const { startSSEServer, stopSSEServer } = require('./fixtures/sse-mcp-server/http'); |
There was a problem hiding this comment.
在 TypeScript 项目中，建议优先使用 ES6 的 import 语法来导入模块，而不是 require。这样可以获得更好的类型支持和静态分析。请将 require 修改为 import。
import { startSSEServer, stopSSEServer } from './fixtures/sse-mcp-server/http';![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
plugin/langchain/package.json (1)
30-33: Redundant file patterns in the published package.Lines 32-33 (
app/controller/**/*.jsandapp/controller/**/*.d.ts) are redundant because they're already covered by the more general patterns on lines 30-31 (app/**/*.jsandapp/**/*.d.ts). The broader patterns will include all controller files.🔎 Proposed fix
"files": [ "app.js", "app.d.ts", "index.js", "index.d.ts", "lib/**/*.js", "lib/**/*.d.ts", "app/**/*.js", "app/**/*.d.ts", - "app/controller/**/*.js", - "app/controller/**/*.d.ts", "typings/*.d.ts" ],plugin/langchain/app/controller/RunsController.ts (1)
62-65: Replace console.error with framework logger.Using
console.errorfor error logging bypasses the application's logging infrastructure, which may handle log levels, formatting, and transport differently.Consider using the Egg.js logger or injecting a logger service:
// If EggContext provides a logger ctx.logger.error('Error streaming run:', error);plugin/langchain/test/agent.test.ts (1)
55-217: Excellent test coverage for payload variations.The test suite thoroughly exercises various RunCreateDTO configurations including stream modes, checkpoints, metadata, interrupts, and tracing options. This provides good validation of the API surface.
However, consider adding negative test cases once validation is implemented:
- Invalid assistant_id format
- Missing required fields
- Invalid enum values for stream_mode, multitask_strategy, etc.
- Oversized payloads
plugin/langchain/app/controller/types.ts (1)
243-250: Consider making scopes optional in AuthContext.The
scopesfield is currently required (line 249), but in many authentication systems, scopes are optional or may be empty. Consider whether this should bescopes?: string[]to allow for auth contexts without explicit scopes.🔎 Proposed change
export interface AuthContext { user: { id: string; identity?: string; [key: string]: unknown; }; - scopes: string[]; + scopes?: string[]; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignoreplugin/langchain/app/controller/RunsController.tsplugin/langchain/app/controller/schemas.tsplugin/langchain/app/controller/sse-utils.tsplugin/langchain/app/controller/types.tsplugin/langchain/package.jsonplugin/langchain/test/agent.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
plugin/langchain/test/agent.test.ts (1)
benchmark/http/app/router.js (1)
app(4-4)
plugin/langchain/app/controller/RunsController.ts (5)
core/controller-decorator/src/decorator/http/HTTPMethod.ts (1)
HTTPMethod(7-23)core/controller-decorator/src/decorator/http/HTTPParam.ts (1)
HTTPBody(11-19)plugin/langchain/app/controller/types.ts (1)
RunCreateDTO(190-212)core/types/controller-decorator/model/types.ts (1)
EggContext(3-3)plugin/langchain/app/controller/sse-utils.ts (1)
streamSSE(52-88)
plugin/langchain/app/controller/schemas.ts (1)
plugin/langchain/app/controller/types.ts (5)
CheckpointSchema(90-94)LangsmithTracer(96-99)Run(117-127)Thread(142-151)Assistant(129-140)
plugin/langchain/app/controller/sse-utils.ts (1)
core/types/controller-decorator/model/types.ts (1)
EggContext(3-3)
plugin/langchain/app/controller/types.ts (1)
plugin/langchain/app/controller/schemas.ts (5)
CheckpointSchema(41-45)LangsmithTracer(70-73)Run(76-93)Assistant(295-302)Thread(244-250)
⏰ 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). (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (16)
.gitignore (2)
14-14: Summary mismatch: AI describes LangGraph API scaffolding, but only.gitignoreis provided for review.The AI-generated summary claims this PR introduces a LangGraph API controller with SSE streaming, Zod schemas, TypeScript types, RunsController, and E2E tests. However, the only file provided for review is
.gitignorewith a single benign entry added (.claudeeditor cache directory).Please verify that all affected files are included in the review.
14-14: LGTM!The
.claudeentry is appropriately placed and ignores Claude editor cache/state directories alongside other editor configs (.vscode,.idea).plugin/langchain/app/controller/RunsController.ts (3)
1-11: LGTM - Clean imports and type usage.The imports are well-organized and properly typed. Good use of type-only imports for EggContext and RunCreateDTO.
16-27: LGTM - Controller and endpoint setup.The HTTPController decorator and HTTPMethod configuration are correctly structured for the /api/runs/stream endpoint.
28-38: Payload validation not implemented yet.The
_payloadparameter is currently unused (indicated by the underscore prefix), and the TODO comment on line 29 confirms validation is pending. Since this is WIP, ensure validation is implemented before removing the WIP tag to prevent accepting malformed payloads.The mock run ID on line 34 will cause all runs to have the same ID. Consider using the
uuidpackage (already added to dependencies) for unique IDs:import { v4 as uuidv4 } from 'uuid'; // ... const mockRunId = uuidv4();plugin/langchain/test/agent.test.ts (2)
36-53: LGTM - Good coverage of SSE headers and Content-Location.The test correctly validates the SSE response headers (Content-Type, Cache-Control, Connection) and verifies the Content-Location header format.
7-7: Verify the Node.js version check against current LangChain requirements.The version check
parseInt(process.version.slice(1, 3)) > 19requires Node.js 20+. Confirm this aligns with the actual minimum Node.js version required for @langchain/langgraph 1.0.2. LangChain documentation typically specifies Node.js >=18, which would make this check overly restrictive.plugin/langchain/app/controller/sse-utils.ts (3)
11-15: LGTM - Clean SSE event interface.The SSEEvent interface properly defines the structure with optional id/event fields and required data field, aligning with SSE specifications.
52-88: SSE headers are correctly configured.The
streamSSEfunction properly sets all required SSE headers including Content-Type, Cache-Control, Connection, and X-Accel-Buffering for nginx compatibility.Note that the actual streaming implementation needs completion (the writeSSE calls won't write data until the TODO in writeSSE is resolved).
118-129: LGTM - Sensible data normalization.The
serialiseAsDictfunction provides reasonable fallback behavior for non-object data, wrapping primitives and arrays in a{ value: ... }structure for consistent SSE serialization.plugin/langchain/app/controller/schemas.ts (2)
12-15: LGTM - Sensible boolean coercion.The
coercedBooleanschema correctly handles common string representations of boolean values (true/1/yes).
41-45: Checkpoint schema requires UUID format.The
checkpoint_idfield requires UUID format (line 42), which is stricter than theassistant_idvalidation in RunCreate. Ensure this aligns with your checkpoint ID generation strategy.plugin/langchain/app/controller/types.ts (4)
1-60: LGTM - Well-organized type definitions.The primitive types, string literal unions, and field enums are clearly defined and provide strong typing for the API surface.
62-99: LGTM - Configuration and command interfaces.The RunnableConfig, RunCommand, CheckpointSchema, and LangsmithTracer interfaces are well-structured with appropriate optional fields and flexible
configurableobject.
117-151: LGTM - Core domain entities.The Run, Assistant, and Thread interfaces properly model the domain entities with appropriate fields and types.
190-212: LGTM - RunCreateDTO matches schema definition.The RunCreateDTO interface aligns with the RunCreate Zod schema in schemas.ts, providing type safety for the controller payload.
| export const RunCreate = z | ||
| .object({ | ||
| assistant_id: z.union([z.string().uuid(), z.string()]), | ||
| checkpoint_id: z.string().optional(), | ||
| checkpoint: CheckpointSchema.optional(), | ||
| input: z.union([z.unknown(), z.null()]).optional(), | ||
| command: CommandSchema.optional(), | ||
| metadata: z | ||
| .object({}) | ||
| .catchall(z.any()) | ||
| .describe('Metadata for the run.') | ||
| .optional(), | ||
| context: z.unknown().optional(), | ||
| config: AssistantConfig.optional(), | ||
| webhook: z.string().optional(), | ||
| interrupt_before: z.union([z.enum(['*']), z.array(z.string())]).optional(), | ||
| interrupt_after: z.union([z.enum(['*']), z.array(z.string())]).optional(), | ||
| on_disconnect: z | ||
| .enum(['cancel', 'continue']) | ||
| .optional() | ||
| .default('continue'), | ||
| multitask_strategy: z | ||
| .enum(['reject', 'rollback', 'interrupt', 'enqueue']) | ||
| .optional(), | ||
| stream_mode: z | ||
| .union([ | ||
| z.array( | ||
| z.enum([ | ||
| 'values', | ||
| 'messages', | ||
| 'messages-tuple', | ||
| 'updates', | ||
| 'events', | ||
| 'tasks', | ||
| 'checkpoints', | ||
| 'debug', | ||
| 'custom', | ||
| ]) | ||
| ), | ||
| z.enum([ | ||
| 'values', | ||
| 'messages', | ||
| 'messages-tuple', | ||
| 'updates', | ||
| 'events', | ||
| 'tasks', | ||
| 'checkpoints', | ||
| 'debug', | ||
| 'custom', | ||
| ]), | ||
| ]) | ||
| .optional(), | ||
| stream_subgraphs: z.boolean().optional(), | ||
| stream_resumable: z.boolean().optional(), | ||
| after_seconds: z.number().optional(), | ||
| if_not_exists: z.enum(['reject', 'create']).optional(), | ||
| on_completion: z.enum(['delete', 'keep']).optional(), | ||
| feedback_keys: z.array(z.string()).optional(), | ||
| langsmith_tracer: LangsmithTracer.optional(), | ||
| }) | ||
| .describe('Payload for creating a stateful run.'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "assistant_id" --type=ts -C 2 | head -100Repository: eggjs/tegg
Length of output: 6803
Document the intentional assistant_id flexibility in RunCreate schema.
RunCreate (line 97) accepts both UUID and arbitrary strings for assistant_id via z.union([z.string().uuid(), z.string()]), whereas all other schemas (RunResponse, Assistant, CronSearch, AssistantCreate) enforce UUID-only validation. Tests confirm this flexibility is used with non-UUID identifiers like 'test-assistant-id'. Add a comment or documentation explaining this is intentional to support slugs or names, clarifying why RunCreate differs from the stricter validation elsewhere.
🤖 Prompt for AI Agents
plugin/langchain/app/controller/schemas.ts lines 95-155: the RunCreate schema
intentionally allows assistant_id to be either a UUID or any string
(z.union([z.string().uuid(), z.string()])) to support slugs or human-friendly
names used in tests (e.g., 'test-assistant-id'), which differs from other
schemas that enforce UUID-only; add a concise inline comment above the
assistant_id field (or a short JSDoc on the RunCreate describe) stating this is
intentional to support non-UUID identifiers/slugs and referencing that other
schemas enforce UUIDs so reviewers understand the difference.
| export function writeSSE(_ctx: EggContext, event: SSEEvent): void { | ||
| let message = ''; | ||
|
|
||
| if (event.id) { | ||
| message += `id: ${event.id}\n`; | ||
| } | ||
|
|
||
| if (event.event) { | ||
| message += `event: ${event.event}\n`; | ||
| } | ||
|
|
||
| // 序列化数据为 JSON | ||
| const data = typeof event.data === 'string' | ||
| ? event.data | ||
| : JSON.stringify(event.data); | ||
|
|
||
| message += `data: ${data}\n\n`; | ||
|
|
||
| // 写入响应流 | ||
| // TODO: 需要使用 ctx.res.write() 或类似方法 | ||
| // ctx.res.write(message); | ||
| } |
There was a problem hiding this comment.
SSE messages are not actually written to the response stream.
The writeSSE function correctly formats SSE messages but doesn't write them to the response (lines 42-43). The tests will pass because they only check headers, but no actual SSE data will be sent to clients.
🔎 Proposed implementation
-export function writeSSE(_ctx: EggContext, event: SSEEvent): void {
+export function writeSSE(ctx: EggContext, event: SSEEvent): void {
let message = '';
if (event.id) {
message += `id: ${event.id}\n`;
}
if (event.event) {
message += `event: ${event.event}\n`;
}
// 序列化数据为 JSON
const data = typeof event.data === 'string'
? event.data
: JSON.stringify(event.data);
message += `data: ${data}\n\n`;
// 写入响应流
- // TODO: 需要使用 ctx.res.write() 或类似方法
- // ctx.res.write(message);
+ ctx.res.write(message);
}🤖 Prompt for AI Agents
In plugin/langchain/app/controller/sse-utils.ts around lines 23-44 the function
formats SSE messages but never writes them to the HTTP response; update the
function to actually write the serialized message to the response by using the
context's response stream (either rename the parameter from _ctx to ctx or
reference _ctx.res) and call res.write(message) (do not call res.end); handle
backpressure by checking the boolean return of res.write and, if false, wait for
the 'drain' event before continuing; wrap the write in a try/catch and
surface/log errors so failures are visible.
| } catch (error) { | ||
| // 发送错误事件 | ||
| stream.writeSSE({ | ||
| event: 'error', | ||
| data: { | ||
| message: error instanceof Error ? error.message : 'Unknown error', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Error event emission may not work with incomplete writeSSE.
The error handling attempts to send an error event via stream.writeSSE, but since writeSSE doesn't actually write to the response stream yet, errors won't be communicated to clients.
This will be resolved once the writeSSE implementation is completed, but be aware that error handling is currently non-functional.
🤖 Prompt for AI Agents
In plugin/langchain/app/controller/sse-utils.ts around lines 79–87, the catch
block calls stream.writeSSE but that helper currently doesn't send anything to
the client, so errors never reach consumers; fix by implementing writeSSE to
write directly to the underlying response (write the SSE-formatted string
"event: error\ndata: <json>\n\n" using res.write and flush/flushHeaders as
needed) and ensure it handles stringifying the error object and optionally ends
the stream on fatal errors, or as a quick fix replace the call in the catch with
a direct write to the raw response using the SSE format and then end/flush the
response.
| export function getDisconnectAbortSignal(_ctx: EggContext): AbortSignal { | ||
| const controller = new AbortController(); | ||
|
|
||
| // 监听请求关闭事件 | ||
| // const onClose = () => { | ||
| // controller.abort(); | ||
| // }; | ||
|
|
||
| // TODO: 监听 ctx.req 的 close 事件 | ||
| // ctx.req.on('close', onClose); | ||
|
|
||
| return controller.signal; | ||
| } |
There was a problem hiding this comment.
AbortSignal doesn't monitor client disconnections.
The getDisconnectAbortSignal function creates an AbortController but doesn't attach the close listener (lines 105-106). This means the signal won't abort when clients disconnect, potentially leaving operations running unnecessarily.
🔎 Proposed implementation
-export function getDisconnectAbortSignal(_ctx: EggContext): AbortSignal {
+export function getDisconnectAbortSignal(ctx: EggContext): AbortSignal {
const controller = new AbortController();
// 监听请求关闭事件
- // const onClose = () => {
- // controller.abort();
- // };
+ const onClose = () => {
+ controller.abort();
+ };
- // TODO: 监听 ctx.req 的 close 事件
- // ctx.req.on('close', onClose);
+ // 监听 ctx.req 的 close 事件
+ ctx.req.on('close', onClose);
return controller.signal;
}📝 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.
| export function getDisconnectAbortSignal(_ctx: EggContext): AbortSignal { | |
| const controller = new AbortController(); | |
| // 监听请求关闭事件 | |
| // const onClose = () => { | |
| // controller.abort(); | |
| // }; | |
| // TODO: 监听 ctx.req 的 close 事件 | |
| // ctx.req.on('close', onClose); | |
| return controller.signal; | |
| } | |
| export function getDisconnectAbortSignal(ctx: EggContext): AbortSignal { | |
| const controller = new AbortController(); | |
| // 监听请求关闭事件 | |
| const onClose = () => { | |
| controller.abort(); | |
| }; | |
| // 监听 ctx.req 的 close 事件 | |
| ctx.req.on('close', onClose); | |
| return controller.signal; | |
| } |
🤖 Prompt for AI Agents
In plugin/langchain/app/controller/sse-utils.ts around lines 97 to 109, the
AbortController is created but never wired to the request lifecycle so the
signal never aborts on client disconnect; update the function to use the
provided EggContext (rename _ctx to ctx or reference it), attach listeners to
ctx.req for 'close' and 'aborted' that call controller.abort(), and ensure you
remove those listeners when the controller is aborted (or when response
finishes) to avoid leaks (use controller.signal.addEventListener('abort', ...)
to cleanup and remove the request listeners).
| "uuid": "^11.0.3", | ||
| "zod": "^3.24.1" |
There was a problem hiding this comment.
Update package versions: uuid and zod are outdated.
uuid is at version 13.0.0, but the package.json specifies ^11.0.3. zod is at version 4.2.1, while ^3.24.1 is specified. Consider upgrading both packages: uuid v12 and later dropped CommonJS support, so review compatibility before upgrading. Zod 4 fixes long-standing design limitations of Zod 3 and provides significantly improved performance. Neither package has known active security vulnerabilities in their current versions, but staying current with stable releases is recommended.
🤖 Prompt for AI Agents
In plugin/langchain/package.json around lines 80-81, the dependency versions for
uuid (^11.0.3) and zod (^3.24.1) are outdated; update uuid to a modern stable
release (e.g., ^13.x or ^12.x) and zod to ^4.x, but before upgrading uuid verify
and adjust for ESM-only changes (imports: switch from require('uuid') to ESM
import or use the compatible build), run npm install to update
package-lock.json/yarn.lock, run the test suite and fix any import/type issues
introduced by zod v4 (adjust Zod APIs that changed) and ensure CI passes.
| import path from 'path'; | ||
| import assert from 'assert'; | ||
|
|
||
| describe.only('plugin/langchain/test/agent.test.ts', () => { |
There was a problem hiding this comment.
CRITICAL: Remove .only from describe block.
The describe.only on line 5 will cause all other test suites in the project to be skipped. This is a critical issue that will hide test failures in other parts of the codebase.
🔎 Proposed fix
-describe.only('plugin/langchain/test/agent.test.ts', () => {
+describe('plugin/langchain/test/agent.test.ts', () => {📝 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.
| describe.only('plugin/langchain/test/agent.test.ts', () => { | |
| describe('plugin/langchain/test/agent.test.ts', () => { |
🤖 Prompt for AI Agents
In plugin/langchain/test/agent.test.ts around line 5 there is a describe.only
which will cause all other test suites to be skipped; remove the `.only`
modifier so the line reads `describe('plugin/langchain/test/agent.test.ts', ()
=> {` (or restore it to the original describe call), then run the test suite to
confirm all tests run normally and commit the change.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
plugin/langchain/app/controller/RunsController.ts (1)
123-123: Consider using structured logging instead of console.error.Using
ctx.logger.errorinstead ofconsole.errorenables better log management and aggregation.🔎 Proposed change
- console.error('Error streaming run:', error); + ctx.logger.error('Error streaming run:', error);plugin/langchain/test/agent.test.ts (1)
5-5: CRITICAL: Remove.onlyfrom describe block.The
describe.onlywill cause all other test suites in the project to be skipped, potentially hiding test failures elsewhere in the codebase. This is a critical issue that must be fixed before merging.🔎 Proposed fix
-describe.only('plugin/langchain/test/agent.test.ts', () => { +describe('plugin/langchain/test/agent.test.ts', () => {
🧹 Nitpick comments (11)
plugin/langchain/app/middleware/ZodErrorMiddleware.ts (1)
13-13: Consider using structured logging instead of console.log.For consistency with application logging standards and to facilitate log aggregation, consider replacing
console.logwithctx.logger.debugorctx.logger.errordepending on the severity.🔎 Proposed change
- console.log('ZodErrorMiddleware Catch Error', error); + ctx.logger.error('ZodErrorMiddleware caught error:', error);plugin/langchain/test/agent.test.ts (1)
86-352: Extensive test scaffolding indicates work-in-progress status.The commented-out tests cover important scenarios including various RunCreateDTO options, stream modes, and SSE content parsing. Consider prioritizing which tests should be implemented before removing the WIP status from this PR.
plugin/langchain/app/controller/RunsController.ts (1)
36-36: Consider using structured logging instead of console.log.For consistency with application logging standards and to facilitate debugging in production, consider using
ctx.logger.infoorctx.logger.debug.🔎 Proposed change
- console.log('streamStatelessRun', validated); + ctx.logger.debug('streamStatelessRun', validated);plugin/langchain/test/sse-utils.test.ts (3)
88-97: Timing test has generous tolerances.The sleep timing test allows 10% variance on the lower bound (90ms vs 100ms) and 100% on the upper bound (200ms vs 100ms). While this accommodates CI/scheduling jitter, consider whether the upper bound is too permissive for catching real timing bugs.
217-239: Non-null assertion is unsafe.Line 238 uses
closeHandler!()with a non-null assertion operator, but there's no verification thatcloseHandlerwas actually assigned. If the implementation changes and no longer registers a'close'event handler, this test will throw a runtime error rather than failing gracefully.🔎 Recommended fix with explicit check
// 模拟请求关闭 - closeHandler!(); + if (!closeHandler) { + throw new Error('close handler was not registered'); + } + closeHandler();
241-266: Same non-null assertion issue.Line 259 uses
closeHandler!()without verifying it was assigned. Apply the same fix as the previous test.plugin/langchain/lib/sse/index.ts (3)
36-72: Consider handling write backpressure.The
writeSSEmethod callsthis.writable.write()(line 64) but ignores its boolean return value, which indicates whether the internal buffer is full. For high-throughput scenarios, this could lead to unbounded memory growth. Consider checking the return value and usingdrainevents for proper backpressure handling.🔎 Example with backpressure handling
return new Promise<void>((resolve, reject) => { const canContinue = this.writable.write(sseData, (error) => { if (error) { reject(error); } else { resolve(); } }); if (!canContinue) { // Buffer is full, wait for drain this.writable.once('drain', () => { // Ready to continue }); } });However, this adds complexity and may not be necessary for typical SSE use cases where events are not sent at extremely high rates.
139-195: Function signature is misleading.The function is declared
asyncbut returns immediately after launchingrunStream()asynchronously (line 194). This makes it impossible for callers to await completion or handle errors. Consider either:
- Removing
asyncand returningvoidto be explicit about fire-and-forget behavior- Returning the
runStream()promise so callers can optionally await:return runStream();Option 2 is recommended for better testability and error propagation.
🔎 Recommended refactor
export async function streamSSE( ctx: EggContext, cb: (stream: SSEStreamWriter) => Promise<void>, onError?: (e: Error, stream: SSEStreamWriter) => Promise<void> ): Promise<void> { // ... setup code ... // 启动流处理（不阻塞） - runStream(); + return runStream(); }This allows tests to properly await completion without setTimeout hacks.
171-181: Error is swallowed during error event writing.When writing an error event fails (line 178), the error is logged but not propagated. This could mask issues with stream handling. Consider propagating the error or at least ensuring it's logged with adequate context.
plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/Graph.ts (2)
8-18: Consider removing unused enum members.The enum defines
AGENT,NODE_A,NODE_B,NODE_C, andNODE_D(lines 12, 14-17) but these don't appear to be used in the graph definition below. If these are not planned for immediate use, consider removing them to reduce clutter.
55-66: Useinstanceoffor type checking.Line 60 uses
ToolMessage.prototype.isPrototypeOf(lastMessage)which is non-idiomatic. While functionally equivalent,instanceofis the standard and more readable approach:🔎 Recommended fix
- if (ToolMessage.prototype.isPrototypeOf(lastMessage)) { + if (lastMessage instanceof ToolMessage) { return { messages: [ new AIMessage(lastMessage!.text!),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
plugin/langchain/app/controller/RunsController.tsplugin/langchain/app/middleware/ZodErrorMiddleware.tsplugin/langchain/lib/sse/index.tsplugin/langchain/test/agent.test.tsplugin/langchain/test/fixtures/apps/agent/app/modules/bar/controller/AppController.tsplugin/langchain/test/fixtures/apps/agent/app/modules/bar/module.ymlplugin/langchain/test/fixtures/apps/agent/app/modules/bar/package.jsonplugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/BoundChatModel.tsplugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/Graph.tsplugin/langchain/test/fixtures/apps/agent/config/config.default.jsplugin/langchain/test/fixtures/apps/agent/config/module.jsonplugin/langchain/test/fixtures/apps/agent/config/plugin.jsplugin/langchain/test/fixtures/apps/agent/package.jsonplugin/langchain/test/fixtures/apps/agent/tsconfig.jsonplugin/langchain/test/sse-utils.test.ts
✅ Files skipped from review due to trivial changes (2)
- plugin/langchain/test/fixtures/apps/agent/config/module.json
- plugin/langchain/test/fixtures/apps/agent/package.json
🧰 Additional context used
🧬 Code graph analysis (6)
plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/BoundChatModel.ts (1)
plugin/langchain/test/fixtures/apps/langchain/app/modules/bar/service/BoundChatModel.ts (1)
BoundChatModel(9-9)
plugin/langchain/app/controller/RunsController.ts (5)
plugin/langchain/app/middleware/ZodErrorMiddleware.ts (1)
ZodErrorMiddleware(9-31)core/types/controller-decorator/model/types.ts (1)
EggContext(3-3)plugin/langchain/app/controller/types.ts (1)
RunCreateDTO(190-212)plugin/langchain/app/controller/schemas.ts (1)
RunCreate(95-155)plugin/langchain/lib/sse/index.ts (1)
streamSSE(139-195)
plugin/langchain/app/middleware/ZodErrorMiddleware.ts (1)
core/types/controller-decorator/model/types.ts (2)
EggContext(3-3)Next(4-4)
plugin/langchain/test/sse-utils.test.ts (1)
plugin/langchain/lib/sse/index.ts (4)
SSEStreamWriter(24-116)streamSSE(139-195)getDisconnectAbortSignal(205-235)serialiseAsDict(244-255)
plugin/langchain/test/agent.test.ts (2)
benchmark/http/app/router.js (1)
app(4-4)plugin/langchain/test/fixtures/apps/agent/config/plugin.js (1)
path(4-4)
plugin/langchain/lib/sse/index.ts (2)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
message(188-199)core/types/controller-decorator/model/types.ts (1)
EggContext(3-3)
🔇 Additional comments (19)
plugin/langchain/test/fixtures/apps/agent/config/config.default.js (1)
1-16: LGTM! Test configuration is appropriate.The configuration correctly disables CSRF protection and enables bodyParser for the test fixture, which aligns with testing the POST streaming endpoint introduced in this PR.
plugin/langchain/test/fixtures/apps/agent/tsconfig.json (1)
1-15: LGTM! TypeScript configuration is appropriate for the test fixture.The configuration correctly enables experimental decorators and decorator metadata, which are required for the @eggjs/tegg framework decorators used throughout this PR. The module settings align with Node.js 18+ requirements.
plugin/langchain/test/fixtures/apps/agent/app/modules/bar/package.json (1)
1-6: LGTM! Module definition follows Egg.js conventions.The package.json correctly defines the test module with appropriate eggModule metadata for framework integration.
plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/BoundChatModel.ts (1)
1-8: LGTM! Empty class body is consistent with the test fixture pattern.The
BoundChatModelclass serves as a declarative configuration via the@BoundModeldecorator. This pattern matches similar test fixtures in the codebase where the decorator provides all necessary metadata and behavior.plugin/langchain/test/fixtures/apps/agent/app/modules/bar/module.yml (1)
1-19: LGTM! Test fixture configuration is appropriate.The YAML configuration properly defines both ChatModel and mcp clients with appropriate mock credentials and endpoints for testing. The SSE transport configuration aligns with the streaming functionality introduced in this PR.
plugin/langchain/app/middleware/ZodErrorMiddleware.ts (1)
9-31: Validation error handling is well-structured.The middleware correctly intercepts
ZodErrorexceptions and returns a consistent 422 response with detailed validation errors. The error format provides clear path, message, and code fields that align with the test assertions in agent.test.ts.plugin/langchain/test/agent.test.ts (1)
30-84: Test assertions are comprehensive and well-structured.Both validation and streaming tests properly verify:
- 422 responses with structured error details for invalid input
- SSE headers (content-type, cache-control, connection)
- Content-Location header format
These tests align well with the RunsController implementation and validation middleware.
plugin/langchain/app/controller/RunsController.ts (1)
34-127: Controller implementation is well-structured for WIP status.The
streamStatelessRunmethod properly:
- Validates input using Zod schema (errors caught by ZodErrorMiddleware)
- Sets appropriate Content-Location header
- Uses SSE streaming utility correctly
- Provides mock data that demonstrates the expected response format
The TODO comments clearly indicate where real service integration is needed. The mock implementation is appropriate for testing the API surface.
plugin/langchain/test/fixtures/apps/agent/app/modules/bar/controller/AppController.ts (1)
1-62: LGTM! Test fixture controller is well-structured.The controller properly demonstrates integration patterns for:
- Direct ChatModel invocation
- Bound model usage
- Compiled state graph execution with configuration
The dependency injection and endpoint implementations are appropriate for a test fixture.
plugin/langchain/test/sse-utils.test.ts (1)
269-297: LGTM!The
serialiseAsDicttests are comprehensive and correctly cover all edge cases: null/undefined, objects, primitives, and arrays. The use ofdeepStrictEqualis appropriate for object comparison.plugin/langchain/test/fixtures/apps/agent/config/plugin.js (1)
1-26: LGTM!The plugin configuration is well-structured and follows Egg.js conventions. The use of
node:pathprotocol (line 4) and the computed path forteggLangChain(line 18) are correct. Disabling the watcher (line 26) is appropriate for test fixtures.plugin/langchain/lib/sse/index.ts (6)
13-18: LGTM!The
SSEEventinterface correctly models the SSE specification with appropriate optional fields and support for async data viaPromise<string>.
103-108: LGTM with minor note.The
close()method properly guards against double-close with theclosedflag. Whilewritable.end()could theoretically throw if the stream is already ended externally, the idempotent guard makes this unlikely in practice.
77-91: LGTM!The
writeCommentmethod correctly implements SSE comment format (: comment\n\n) and includes proper closed-stream checks.
96-98: LGTM!Simple sleep utility using
setTimeoutis appropriate for pacing SSE events.
205-235: LGTM!The
getDisconnectAbortSignalfunction correctly handles client disconnection by monitoring'close'and'error'events on the request object. The cleanup logic prevents memory leaks, and the optional stream closure is a nice convenience.
244-255: LGTM!The
serialiseAsDictutility correctly normalizes various data types into dictionary format. The null check (line 245) properly precedes the object type check to avoid thetypeof null === 'object'gotcha.plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/Graph.ts (2)
20-49: LGTM!The checkpoint saver, state definition, and tool implementation are appropriate for a test fixture. The tool schema and implementation are simple and clear.
125-135: LGTM!The
FooGraphclass correctly extendsAbstractStateGraphand properly wires up the nodes, edges, and checkpoint saver. The constructor appropriately passes the state definition to the parent class.
| import { AccessLevel, SingletonProto, ToolArgs, ToolArgsSchema } from '@eggjs/tegg'; | ||
| import { Graph, GraphEdge, IGraphEdge, AbstractStateGraph, GraphNode, IGraphNode, GraphStateType, GraphTool, IGraphTool, TeggToolNode } from '@eggjs/tegg-langchain-decorator'; | ||
| import { Annotation, MemorySaver } from '@langchain/langgraph'; | ||
| // import { AIMessage, BaseMessage, ToolMessage } from '@langchain/core/messages'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out import.
Line 4 contains a commented-out import that should be removed to keep the codebase clean. The active import from langchain (line 6) already provides these types.
🔎 Proposed fix
-// import { AIMessage, BaseMessage, ToolMessage } from '@langchain/core/messages';
import * as z from 'zod/v4';📝 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 { AIMessage, BaseMessage, ToolMessage } from '@langchain/core/messages'; | |
| import * as z from 'zod/v4'; |
🤖 Prompt for AI Agents
In plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/Graph.ts
around line 4, there is a commented-out import ("// import { AIMessage,
BaseMessage, ToolMessage } from '@langchain/core/messages';") that should be
removed; delete this commented line so only the active import from 'langchain'
remains and keep the file clean without changing any other imports or code.
| return { | ||
| messages: [ | ||
| new AIMessage(lastMessage!.text!), | ||
| ], |
There was a problem hiding this comment.
Unsafe non-null assertions.
Line 63 uses lastMessage!.text! with double non-null assertions. If lastMessage is nullish or text is undefined, this will throw a runtime error rather than being caught by type checking. Consider adding proper guards or using optional chaining.
🔎 Proposed fix with proper guards
- return {
- messages: [
- new AIMessage(lastMessage!.text!),
- ],
- };
+ const text = lastMessage?.text ?? '';
+ return {
+ messages: [
+ new AIMessage(text),
+ ],
+ };Or add explicit type guard:
if (lastMessage instanceof ToolMessage && lastMessage.text) {
return {
messages: [new AIMessage(lastMessage.text)],
};
}🤖 Prompt for AI Agents
In plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/Graph.ts
around lines 61 to 64, the code uses double non-null assertions
`lastMessage!.text!` which can throw at runtime; replace that with a safe guard:
check that lastMessage is defined and has a text property (for example ensure
lastMessage is an instance of the expected message class and text is truthy) and
only then construct the AIMessage with lastMessage.text, otherwise return a
sensible fallback (empty messages array or throw a controlled error) so no
non-null assertions are used.
| async execute( | ||
| state: GraphStateType<fooAnnotationStateDefinitionType>, | ||
| ): Promise<FooGraphNodeName> { | ||
| console.log('response: ', state.messages); | ||
| const messages = state.messages; | ||
| const lastMessage = messages[messages.length - 1] as AIMessage; | ||
| if (lastMessage?.tool_calls?.length) { | ||
| return FooGraphNodeName.TOOLS; | ||
| } | ||
| return FooGraphNodeName.END; | ||
| } |
There was a problem hiding this comment.
Unsafe type assertion.
Line 104 casts lastMessage to AIMessage without verification. If the last message is not actually an AIMessage, this could lead to unexpected behavior. Consider adding a proper type guard or using optional chaining throughout.
🔎 Recommended fix with type guard
- const messages = state.messages;
- const lastMessage = messages[messages.length - 1] as AIMessage;
- if (lastMessage?.tool_calls?.length) {
+ const messages = state.messages;
+ const lastMessage = messages[messages.length - 1];
+ if (lastMessage instanceof AIMessage && lastMessage.tool_calls?.length) {
return FooGraphNodeName.TOOLS;
}📝 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 execute( | |
| state: GraphStateType<fooAnnotationStateDefinitionType>, | |
| ): Promise<FooGraphNodeName> { | |
| console.log('response: ', state.messages); | |
| const messages = state.messages; | |
| const lastMessage = messages[messages.length - 1] as AIMessage; | |
| if (lastMessage?.tool_calls?.length) { | |
| return FooGraphNodeName.TOOLS; | |
| } | |
| return FooGraphNodeName.END; | |
| } | |
| async execute( | |
| state: GraphStateType<fooAnnotationStateDefinitionType>, | |
| ): Promise<FooGraphNodeName> { | |
| console.log('response: ', state.messages); | |
| const messages = state.messages; | |
| const lastMessage = messages[messages.length - 1]; | |
| if (lastMessage instanceof AIMessage && lastMessage.tool_calls?.length) { | |
| return FooGraphNodeName.TOOLS; | |
| } | |
| return FooGraphNodeName.END; | |
| } |
🤖 Prompt for AI Agents
plugin/langchain/test/fixtures/apps/agent/app/modules/bar/service/Graph.ts lines
99-109: the code unsafely casts lastMessage to AIMessage on line 104; add a
runtime type guard instead of a direct assertion—check that messages.length > 0,
verify lastMessage has the expected AIMessage discriminator (e.g., role ===
'assistant' or a type field) and that lastMessage.tool_calls is an array
(Array.isArray), then use optional chaining to read its length and return TOOLS
only when that guard passes; otherwise return END.
| it('should set correct SSE headers', async () => { | ||
| const ctx: any = { | ||
| set: (key: string, value: string) => { | ||
| ctx.headers = ctx.headers || {}; | ||
| ctx.headers[key.toLowerCase()] = value; | ||
| }, | ||
| headers: {}, | ||
| }; | ||
|
|
||
| streamSSE(ctx, async (stream) => { | ||
| await stream.writeSSE({ data: 'test' }); | ||
| }); | ||
|
|
||
| // 等待一下让 streamSSE 设置 headers | ||
| await new Promise(resolve => setTimeout(resolve, 10)); | ||
|
|
||
| assert.strictEqual(ctx.headers['content-type'], 'text/event-stream'); | ||
| assert.strictEqual(ctx.headers['cache-control'], 'no-cache'); | ||
| assert.strictEqual(ctx.headers['connection'], 'keep-alive'); | ||
| assert.strictEqual(ctx.headers['transfer-encoding'], 'chunked'); | ||
| assert.strictEqual(ctx.headers['x-accel-buffering'], 'no'); | ||
| }); |
There was a problem hiding this comment.
Timing-based waits are brittle.
The test uses setTimeout with a fixed 10ms delay to wait for header setup (line 151). This is fragile and may fail in slower CI environments or under load. Since streamSSE launches runStream() asynchronously without blocking, consider:
- Refactoring
streamSSEto return a promise that resolves when setup is complete - Using event-based synchronization instead of arbitrary delays
- Using a test-specific helper that provides deterministic timing
🔎 Alternative approach using event-based synchronization
Consider refactoring the test to wait for actual stream events rather than arbitrary timeouts:
- // 等待一下让 streamSSE 设置 headers
- await new Promise(resolve => setTimeout(resolve, 10));
+ // Wait for stream to be ready by listening to the body assignment
+ await new Promise(resolve => {
+ const checkInterval = setInterval(() => {
+ if (ctx.body) {
+ clearInterval(checkInterval);
+ resolve();
+ }
+ }, 1);
+ });Or better yet, have streamSSE return a promise that resolves after initial setup.
📝 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.
| it('should set correct SSE headers', async () => { | |
| const ctx: any = { | |
| set: (key: string, value: string) => { | |
| ctx.headers = ctx.headers || {}; | |
| ctx.headers[key.toLowerCase()] = value; | |
| }, | |
| headers: {}, | |
| }; | |
| streamSSE(ctx, async (stream) => { | |
| await stream.writeSSE({ data: 'test' }); | |
| }); | |
| // 等待一下让 streamSSE 设置 headers | |
| await new Promise(resolve => setTimeout(resolve, 10)); | |
| assert.strictEqual(ctx.headers['content-type'], 'text/event-stream'); | |
| assert.strictEqual(ctx.headers['cache-control'], 'no-cache'); | |
| assert.strictEqual(ctx.headers['connection'], 'keep-alive'); | |
| assert.strictEqual(ctx.headers['transfer-encoding'], 'chunked'); | |
| assert.strictEqual(ctx.headers['x-accel-buffering'], 'no'); | |
| }); | |
| it('should set correct SSE headers', async () => { | |
| const ctx: any = { | |
| set: (key: string, value: string) => { | |
| ctx.headers = ctx.headers || {}; | |
| ctx.headers[key.toLowerCase()] = value; | |
| }, | |
| headers: {}, | |
| }; | |
| streamSSE(ctx, async (stream) => { | |
| await stream.writeSSE({ data: 'test' }); | |
| }); | |
| // Wait for stream to be ready by listening to the body assignment | |
| await new Promise(resolve => { | |
| const checkInterval = setInterval(() => { | |
| if (ctx.body) { | |
| clearInterval(checkInterval); | |
| resolve(); | |
| } | |
| }, 1); | |
| }); | |
| assert.strictEqual(ctx.headers['content-type'], 'text/event-stream'); | |
| assert.strictEqual(ctx.headers['cache-control'], 'no-cache'); | |
| assert.strictEqual(ctx.headers['connection'], 'keep-alive'); | |
| assert.strictEqual(ctx.headers['transfer-encoding'], 'chunked'); | |
| assert.strictEqual(ctx.headers['x-accel-buffering'], 'no'); | |
| }); |
🤖 Prompt for AI Agents
In plugin/langchain/test/sse-utils.test.ts around lines 137 to 158, the test
uses a brittle setTimeout to wait for streamSSE to set headers; instead update
streamSSE to provide a deterministic synchronization point (e.g., return a
Promise that resolves once headers are set or emit an event/call a callback when
setup completes), then change the test to await that Promise or listen for the
event before asserting headers so no arbitrary delay is required; alternatively
add a test-only hook on streamSSE to signal completion and await it in the test.
| it('should call callback with stream writer', async () => { | ||
| const ctx: any = { | ||
| set: () => {}, | ||
| }; | ||
|
|
||
| let streamReceived = false; | ||
|
|
||
| streamSSE(ctx, async (stream) => { | ||
| streamReceived = true; | ||
| assert(stream.writeSSE, 'Stream should have writeSSE method'); | ||
| assert(stream.sleep, 'Stream should have sleep method'); | ||
| assert(stream.close, 'Stream should have close method'); | ||
| }); | ||
|
|
||
| // 等待回调执行 | ||
| await new Promise(resolve => setTimeout(resolve, 10)); | ||
|
|
||
| assert(streamReceived, 'Callback should be called'); | ||
| }); |
There was a problem hiding this comment.
Same timing issue applies here.
Similar to the previous test, this relies on setTimeout with 10ms delay (line 175) to wait for callback execution. This is prone to flakiness.
🤖 Prompt for AI Agents
In plugin/langchain/test/sse-utils.test.ts around lines 160 to 178, the test
uses a 10ms setTimeout to wait for the callback which is flaky; instead change
the test to await the callback deterministically by wrapping streamSSE call in a
Promise and resolving inside the callback (or have the callback call a resolve
function) so the test waits for the callback to finish and then assert
streamReceived and the stream methods; remove the setTimeout and make the test
async-await the created Promise.
| it('should handle errors in callback', async () => { | ||
| const ctx: any = { | ||
| set: () => {}, | ||
| }; | ||
|
|
||
| const error = new Error('Test error'); | ||
| let errorReceived = false; | ||
|
|
||
| await streamSSE(ctx, async () => { | ||
| throw error; | ||
| }, async (e) => { | ||
| errorReceived = true; | ||
| assert.strictEqual(e, error); | ||
| }); | ||
|
|
||
| // 等待错误处理 | ||
| await new Promise(resolve => setTimeout(resolve, 10)); | ||
|
|
||
| assert(errorReceived, 'Error handler should be called'); | ||
| }); |
There was a problem hiding this comment.
Error handling test has timing dependency.
The test waits 10ms (line 196) for error handling to complete. This is brittle and may not be sufficient in all environments. Consider refactoring to use event-based synchronization or making streamSSE return a promise for proper awaiting.
🤖 Prompt for AI Agents
In plugin/langchain/test/sse-utils.test.ts around lines 180 to 199, the test
uses a hardcoded 10ms setTimeout to wait for the error handler which is brittle;
change the test to await a deterministic signal instead — either refactor
streamSSE to return a Promise that resolves/rejects when the stream
completes/errors and await that Promise in the test, or have the test pass in an
async callback that returns a Promise and await the streamSSE call itself;
remove the setTimeout and assert errorReceived only after awaiting the returned
promise so the test reliably waits for the error handler to run.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/langchain/lib/runs/RunsService.ts (1)
8-10: Add explicit return type annotation for better type safety.The
getConfigs()method lacks an explicit return type. Adding a return type improves code clarity and enables compile-time type checking.🔎 Proposed fix
- public getConfigs() { + public getConfigs(): ModuleConfig { return this.moduleConfig; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/langchain/app/controller/RunsController.tsplugin/langchain/lib/runs/RunsService.tsplugin/langchain/test/agent.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/langchain/app/controller/RunsController.ts
- plugin/langchain/test/agent.test.ts
🔇 Additional comments (1)
plugin/langchain/lib/runs/RunsService.ts (1)
3-6: LGTM!The singleton service setup with
@SingletonProtoand dependency injection ofModuleConfigvia@Inject()follows standard tegg patterns correctly.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
plugin/langchain/typings/index.d.ts (1)
126-126: Same type safety issue: Replace{}with a more specific type.This declaration has the same issue as line 116. Both module augmentations should use a consistent, safer type for the agents field.
plugin/langchain/package.json (1)
80-81: Package versions flagged in prior review.The uuid and zod version concerns were already raised in a previous review cycle.
plugin/langchain/app/controller/RunsController.ts (1)
139-139: Usectx.logger.errorinstead ofconsole.error.This was already flagged in a prior review.
🧹 Nitpick comments (7)
plugin/langchain/typings/index.d.ts (1)
113-131: Consider adding documentation for the agents field.The new
agentsfield lacks documentation explaining its purpose and expected structure. Consider adding JSDoc comments to clarify what agents are configured here and how they relate to the LangGraph functionality mentioned in the PR context.Example documentation
export type LangChainModuleConfig = { ChatModel?: ChatModelConfigModuleConfigType; + /** + * Configuration for LangGraph agents. + * Maps agent names to their respective configurations. + */ agents?: Record<string, AgentConfig>; };plugin/langchain/package.json (1)
32-33: Redundant file patterns in thefilesarray.The patterns
app/controller/**/*.jsandapp/controller/**/*.d.tsare already covered byapp/**/*.jsandapp/**/*.d.tson lines 30-31. These entries can be removed.🔎 Proposed fix
"app/**/*.js", "app/**/*.d.ts", - "app/controller/**/*.js", - "app/controller/**/*.d.ts", "typings/*.d.ts"plugin/langchain/app/controller/RunsController.ts (1)
51-54: Replaceconsole.logwithctx.loggerfor proper logging.Using
console.logbypasses the application's logging configuration. Usectx.logger.infofor consistent log formatting, collection, and level control.🔎 Proposed fix
- console.log('streamStatelessRun', { - run, - agentConfigs: this.runsService.getAllAgentConfigs(), - }); + ctx.logger.info('streamStatelessRun', { + run, + agentConfigs: this.runsService.getAllAgentConfigs(), + });plugin/langchain/lib/assistants/AssistantsInitService.ts (2)
30-43: Replaceconsole.logwith a proper logger.Multiple
console.logcalls in the initialization flow bypass application logging configuration. Consider injecting a logger or using a framework-provided logging mechanism for consistent output formatting and level control.
87-89: Redundant null-coalescing after truthy check.Line 87 already continues if
!moduleInfo.config.agents, soagentson line 89 is guaranteed to be truthy. The|| {}is unnecessary.🔎 Proposed fix
if (!moduleInfo.config.agents) continue; - const agents = moduleInfo.config.agents || {}; + const agents = moduleInfo.config.agents;plugin/langchain/lib/assistants/AssistantsRepository.ts (1)
105-113: Consider using nullish coalescing for clarity.Lines 106 and 112 use
|| nullto convertundefinedtonull. While functionally correct here,?? nullmore precisely expresses the intent (only coalesceundefined/null) and avoids edge cases with other falsy values.🔎 Proposed change
async get(assistantId: string, _auth?: AuthContext): Promise<Assistant | null> { - return this.assistants.get(assistantId) || null; + return this.assistants.get(assistantId) ?? null; } async getByGraphId(graphId: string): Promise<Assistant | null> { const assistantId = this.assistantsByGraphId.get(graphId); if (!assistantId) return null; - return this.assistants.get(assistantId) || null; + return this.assistants.get(assistantId) ?? null; }plugin/langchain/lib/runs/RunsService.ts (1)
31-42: Remove redundant fallback in line 36.Line 35 already guards with
if (moduleInfo.config.agents), so the|| {}on line 36 is unreachable.🔎 Simplified version
for (const [moduleName, moduleInfo] of this.moduleConfigs) { if (moduleInfo.config.agents) { - const agents = moduleInfo.config.agents || {}; + const agents = moduleInfo.config.agents; for (const [agentName, agentConfig] of Object.entries(agents)) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/common-util/src/ModuleConfigs.tscore/common-util/test/ModuleConfig.test.tscore/langchain-decorator/src/util/GraphInfoUtil.tsplugin/langchain/app/controller/RunsController.tsplugin/langchain/lib/assistants/AssistantsInitService.tsplugin/langchain/lib/assistants/AssistantsRepository.tsplugin/langchain/lib/runs/Graph.tsplugin/langchain/lib/runs/RunsRepository.tsplugin/langchain/lib/runs/RunsService.tsplugin/langchain/package.jsonplugin/langchain/test/agent.test.tsplugin/langchain/test/fixtures/apps/agent/app/modules/bar/module.ymlplugin/langchain/typings/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/langchain/test/fixtures/apps/agent/app/modules/bar/module.yml
- plugin/langchain/test/agent.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T08:31:56.726Z
Learnt from: Dipper30
Repo: eggjs/tegg PR: 385
File: plugin/dns-cache/lib/DnsResolver.ts:6-6
Timestamp: 2025-12-25T08:31:56.726Z
Learning: The eggjs/tegg-dns-cache plugin in plugin/dns-cache intentionally supports only IPv4 addresses, not IPv6. The IP_REGEX in plugin/dns-cache/lib/DnsResolver.ts is deliberately IPv4-only by design.
Applied to files:
plugin/langchain/package.json
🧬 Code graph analysis (7)
core/langchain-decorator/src/util/GraphInfoUtil.ts (3)
core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/langchain-decorator/src/model/GraphMetadata.ts (1)
IGraphMetadata(4-8)core/metadata/src/model/ProtoDescriptorHelper.ts (1)
clazz(90-132)
core/common-util/src/ModuleConfigs.ts (1)
core/tegg/index.ts (1)
ModuleConfigHolder(10-10)
plugin/langchain/lib/assistants/AssistantsInitService.ts (5)
plugin/langchain/lib/runs/Graph.ts (2)
NAMESPACE_GRAPH(8-10)SingletonProto(12-26)plugin/langchain/lib/assistants/AssistantsRepository.ts (1)
SingletonProto(51-162)plugin/langchain/lib/runs/RunsRepository.ts (1)
SingletonProto(74-279)core/lifecycle/src/decorator/index.ts (1)
LifecyclePostInject(27-27)core/langchain-decorator/src/util/GraphInfoUtil.ts (1)
GraphInfoUtil(6-30)
plugin/langchain/lib/runs/RunsRepository.ts (1)
plugin/langchain/app/controller/types.ts (6)
RunStatus(33-39)Metadata(6-6)MultitaskStrategy(52-52)IfNotExists(56-56)RunKwargs(101-115)AuthContext(243-250)
plugin/langchain/lib/assistants/AssistantsRepository.ts (1)
plugin/langchain/app/controller/types.ts (4)
RunnableConfig(62-79)Metadata(6-6)OnConflictBehavior(54-54)AuthContext(243-250)
plugin/langchain/lib/runs/Graph.ts (1)
core/langchain-decorator/src/util/GraphInfoUtil.ts (1)
GraphInfoUtil(6-30)
core/common-util/test/ModuleConfig.test.ts (1)
core/common-util/src/ModuleConfigs.ts (1)
ModuleConfigs(3-14)
⏰ 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). (3)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (13)
plugin/langchain/lib/runs/RunsRepository.ts (1)
74-80: LGTM - In-memory repository implementation is appropriate for initial development.The dual-map approach (runs by ID and runs by thread) provides efficient lookups. The TODO comment appropriately notes this should be replaced with a database implementation.
core/common-util/src/ModuleConfigs.ts (1)
11-13: LGTM - Clean iterator implementation.The generator method correctly delegates to
Object.entries(), enablingfor...ofiteration over module configs. The implementation is idiomatic and well-tested.core/langchain-decorator/src/util/GraphInfoUtil.ts (1)
18-25: LGTM - Graph lookup by name.The linear search implementation is appropriate given the expected small number of registered graphs. If the graph count grows significantly, consider adding a
Map<string, EggProtoImplClass>index by name for O(1) lookups.core/common-util/test/ModuleConfig.test.ts (1)
145-182: LGTM - Good test coverage for the iterator functionality.The tests cover both populated and empty configurations, verifying the iteration order and content. The placement outside nested describe blocks is slightly inconsistent with the file structure but functionally correct.
plugin/langchain/lib/runs/Graph.ts (1)
12-26: LGTM - Graph utility provides clean abstraction for graph-to-assistant ID mapping.The implementation correctly uses UUID v5 for deterministic ID generation based on graph name, matching the langgraphjs approach.
plugin/langchain/lib/assistants/AssistantsInitService.ts (1)
28-44: Initialization logic is well-structured.The
@LifecyclePostInject()pattern for seeding assistants from multiple sources (GraphInfoUtil and moduleConfigs) is clean. The idempotentif_exists: 'do_nothing'strategy prevents duplicate registration issues.plugin/langchain/app/controller/RunsController.ts (1)
38-49: Method signature and behavior verified.RunsService.createValidRunaccepts the parameters as called:threadId: string | undefined,payload: RunCreateDTO, andkwargswith optionalheaders. The method explicitly handles undefined threadId for stateless runs at line 118, marking them astemporarywhenthreadId == nullandon_completionis'delete'(the default). Headers are properly extracted and filtered before being added to the run configuration.plugin/langchain/lib/assistants/AssistantsRepository.ts (2)
10-45: LGTM! Well-defined repository interfaces.The interface contracts are clear, properly typed, and follow async patterns consistently.
159-161: LGTM! Clean utility method.Simple and correct implementation for retrieving all assistants.
plugin/langchain/lib/runs/RunsService.ts (4)
79-172: LGTM! Comprehensive configuration assembly.The logic properly handles:
- Stream mode normalization with sensible defaults
- Multitask strategy extraction
- Checkpoint configuration merging
- Header filtering (excluding sensitive keys)
- Auth context integration
- RunKwargs construction
197-218: Inflight cancellation logic is well-guarded.The try-catch around
runsRepository.cancelproperly handles scenarios where inflight runs may already be cancelled. The warning message provides useful debugging context.
221-229: Clear error handling for multitask conflicts.The reject strategy error message clearly explains the issue and suggests alternatives. The "Unreachable state" fallback is good defensive programming.
234-267: LGTM! Clean delegation to repository.The CRUD helpers correctly forward calls to
RunsRepositorywith proper signatures.
| // 存储 assistant | ||
| this.assistants.set(assistantId, assistant); | ||
| this.assistantsByGraphId.set(graphId, assistantId); |
There was a problem hiding this comment.
Critical: assistantsByGraphId allows only one assistant per graph_id.
Line 100 unconditionally overwrites the graph_id → assistant_id mapping. If:
- Multiple assistants share the same
graph_id, only the most recently stored assistant will be retrievable viagetByGraphId. - An existing assistant's
graph_idchanges during an update, the old mapping remains orphaned.
Impact: Data loss and stale index entries.
🔎 Recommended fixes
Option 1: If the design requires 1:1 mapping, validate uniqueness:
async put(...): Promise<Assistant> {
...
+ // Enforce 1:1 graph_id <-> assistant_id mapping
+ const existingAssistantId = this.assistantsByGraphId.get(graphId);
+ if (existingAssistantId && existingAssistantId !== assistantId) {
+ throw new Error(`Graph "${graphId}" is already associated with assistant "${existingAssistantId}"`);
+ }
+
const existing = this.assistants.get(assistantId);
...Option 2: If 1:many is valid, change the index structure:
-private assistantsByGraphId: Map<string, string> = new Map();
+private assistantsByGraphId: Map<string, Set<string>> = new Map();and update getByGraphId to return Assistant[] or handle multiple results.
Option 3: Clean up stale mappings on update:
+ // Remove old graph_id mapping if assistant is being updated with a new graph_id
+ if (existing && existing.graph_id !== graphId) {
+ this.assistantsByGraphId.delete(existing.graph_id);
+ }
+
this.assistants.set(assistantId, assistant);
this.assistantsByGraphId.set(graphId, assistantId);| if (metadata) { | ||
| results = results.filter(a => { | ||
| for (const [ key, value ] of Object.entries(metadata)) { | ||
| if (a.metadata[key] !== value) return false; | ||
| } | ||
| return true; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Metadata filtering uses reference equality for complex values.
Line 143 compares a.metadata[key] !== value using strict equality. This breaks for nested objects or arrays, as it checks reference identity rather than structural equality.
Example failure:
search({ metadata: { config: { foo: 'bar' } } })
// Won't match assistants even if they have identical nested config🔎 Proposed solutions
Option 1: Document the limitation (quick fix):
// 过滤 metadata
+// Note: Uses strict equality; nested objects/arrays must be the same reference
if (metadata) {Option 2: Use shallow equality for common cases:
results = results.filter(a => {
for (const [key, value] of Object.entries(metadata)) {
- if (a.metadata[key] !== value) return false;
+ const aValue = a.metadata[key];
+ // Shallow equality check
+ if (aValue !== value && JSON.stringify(aValue) !== JSON.stringify(value)) {
+ return false;
+ }
}
return true;
});Option 3: Use a deep equality library (recommended for production):
import { isEqual } from 'lodash-es';
results = results.filter(a => {
for (const [key, value] of Object.entries(metadata)) {
if (!isEqual(a.metadata[key], value)) return false;
}
return true;
});🤖 Prompt for AI Agents
In plugin/langchain/lib/assistants/AssistantsRepository.ts around lines 140 to
147, the current metadata filter uses strict === which fails for nested
objects/arrays; replace the reference-equality check with a deep equality check
(e.g., import isEqual from 'lodash-es' and use if (!isEqual(a.metadata[key],
value)) return false), ensure you handle missing keys (treat undefined as
non-match) and update imports accordingly.
| // Magic NAMESPACE from | ||
| // https://github.com/langchain-ai/langgraphjs/blob/main/libs/langgraph-api/src/graph/load.mts#L27 | ||
| // Dont ask me why... | ||
| export const NAMESPACE_GRAPH = uuid.parse( | ||
| '6ba7b821-9dad-11d1-80b4-00c04fd430c8', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate NAMESPACE_GRAPH constant - consolidate to avoid drift.
This constant is also defined in plugin/langchain/lib/assistants/AssistantsInitService.ts (line 11). Having the same magic UUID in two places risks them diverging. Consider exporting NAMESPACE_GRAPH from one canonical location (this file is a good candidate) and importing it elsewhere.
🔎 Proposed approach
Keep NAMESPACE_GRAPH exported from Graph.ts (as it already is), then in AssistantsInitService.ts:
-import { v5 as uuidv5, parse as uuidParse } from 'uuid';
+import { v5 as uuidv5 } from 'uuid';
+import { NAMESPACE_GRAPH } from '../runs/Graph';
-const NAMESPACE_GRAPH = uuidParse('6ba7b821-9dad-11d1-80b4-00c04fd430c8');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/langchain/lib/runs/Graph.ts around lines 5-10 there is a magic
NAMESPACE_GRAPH UUID; the same constant appears in
plugin/langchain/lib/assistants/AssistantsInitService.ts (line 11) causing
duplication. Make Graph.ts the canonical export (keep the existing export) and
remove the duplicate constant from AssistantsInitService.ts, replacing it with
an import from '../runs/Graph' (adjust relative path as needed). After changing
the import, run a quick grep/TS compile to ensure no other copies exist and
update any tests or files referencing the old duplicate name/path.
| const run: Run = { | ||
| run_id: runId, | ||
| thread_id: threadId || '', | ||
| assistant_id: assistantId, | ||
| created_at: now, | ||
| updated_at: now, | ||
| status, | ||
| metadata, | ||
| kwargs, | ||
| multitask_strategy: multitaskStrategy, | ||
| }; |
There was a problem hiding this comment.
Consider using null instead of empty string for absent thread_id.
Setting thread_id: threadId || '' means stateless runs get an empty string. This could cause subtle issues: an empty string is truthy in some contexts, and the search() method (line 234) would never find stateless runs since they'd have thread_id: '' but no index entry in runsByThread. Consider using null or undefined for consistency with the absence semantic.
🔎 Proposed fix
const run: Run = {
run_id: runId,
- thread_id: threadId || '',
+ thread_id: threadId ?? null,
assistant_id: assistantId,
created_at: now,
updated_at: now,This requires ensuring the Run type allows thread_id: string | null.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/langchain/lib/runs/RunsRepository.ts around lines 125 to 135, the code
sets thread_id: runId || '' which stores an empty string for absent thread IDs;
change this to use null for absent thread IDs (thread_id: threadId ?? null),
update the Run type to allow thread_id: string | null, and audit any code that
expects a non-null string (e.g., the runsByThread index and the search() method
around line 234) to treat null as “stateless” (skip indexing or use a separate
key) so stateless runs are stored and found consistently.
| console.log('📊 Creating run for assistant:', { | ||
| assistantId, | ||
| graphId: assistant.graph_id, | ||
| name: assistant.name, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace console.log with a proper logger.
Production code should use a structured logger (e.g., this.logger.debug) instead of console.log for better log management, filtering, and observability.
🔎 Recommended pattern
-console.log('📊 Creating run for assistant:', {
+this.logger.debug('Creating run for assistant', {
assistantId,
graphId: assistant.graph_id,
name: assistant.name,
});📝 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.
| console.log('📊 Creating run for assistant:', { | |
| assistantId, | |
| graphId: assistant.graph_id, | |
| name: assistant.name, | |
| }); | |
| this.logger.debug('Creating run for assistant', { | |
| assistantId, | |
| graphId: assistant.graph_id, | |
| name: assistant.name, | |
| }); |
🤖 Prompt for AI Agents
In plugin/langchain/lib/runs/RunsService.ts around lines 69 to 73, replace the
direct console.log call with the service's structured logger: use
this.logger.debug (or this.logger.info if more appropriate) and pass the same
contextual object (assistantId, graphId: assistant.graph_id, name:
assistant.name) so logs remain structured; ensure the RunsService class has a
logger property initialized (or injected) and imported logger type if missing,
and remove the console.log call.
| const graph = this.graph.getGraph(assistant.graph_id); | ||
| console.log('---> graph instance', graph); |
There was a problem hiding this comment.
Validate that graph exists before proceeding.
Line 76 calls this.graph.getGraph(assistant.graph_id) but never checks if the returned graph is null or undefined. If the graph doesn't exist, subsequent operations may fail silently or throw cryptic errors.
🔎 Proposed fix
const graph = this.graph.getGraph(assistant.graph_id);
-console.log('---> graph instance', graph);
+if (!graph) {
+ throw new Error(`Graph "${assistant.graph_id}" not found`);
+}
+this.logger.debug('Loaded graph instance', { graphId: assistant.graph_id });📝 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 graph = this.graph.getGraph(assistant.graph_id); | |
| console.log('---> graph instance', graph); | |
| const graph = this.graph.getGraph(assistant.graph_id); | |
| if (!graph) { | |
| throw new Error(`Graph "${assistant.graph_id}" not found`); | |
| } | |
| this.logger.debug('Loaded graph instance', { graphId: assistant.graph_id }); |
🤖 Prompt for AI Agents
In plugin/langchain/lib/runs/RunsService.ts around lines 76-77, the code calls
this.graph.getGraph(assistant.graph_id) but doesn’t check for a null/undefined
result; add a guard that validates graph exists immediately after retrieval, log
a clear error including assistant.graph_id if missing, and return or throw an
appropriate error (preferably a typed error or reject the promise) to stop
further processing and avoid cryptic downstream failures.
|
|
||
| // 处理创建成功的情况 | ||
| if (first?.run_id === runId) { | ||
| console.log('Created run', { run_id: runId, thread_id: threadId }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace console.log with structured logging.
Same issue as line 69—use a proper logger instead of console.log.
🔎 Proposed fix
-console.log('Created run', { run_id: runId, thread_id: threadId });
+this.logger.info('Created run', { run_id: runId, thread_id: threadId });🤖 Prompt for AI Agents
In plugin/langchain/lib/runs/RunsService.ts around line 194, replace the direct
console.log call with the project's structured logger; use the existing logger
instance (or import/get one if not present) and emit an info-level log with
structured fields for run_id and thread_id instead of console.log, ensuring the
message follows the same logging pattern used elsewhere in the file (e.g.,
include contextual text and pass runId/threadId as properties).
| private generateRunId(): string { | ||
| return `run_${Date.now()}_${Math.random().toString(36).substring(2, 15)}`; | ||
| } |
There was a problem hiding this comment.
ID generation is not collision-resistant.
The current implementation (Date.now() + Math.random()) can produce duplicate IDs under high concurrency or in distributed deployments where multiple instances generate IDs simultaneously.
🔎 Recommended fix using UUID v4
Install a UUID library:
npm install uuidThen replace the implementation:
+import { v4 as uuidv4 } from 'uuid';
+
private generateRunId(): string {
- return `run_${Date.now()}_${Math.random().toString(36).substring(2, 15)}`;
+ return `run_${uuidv4()}`;
}Alternatively, if UUIDs are already available elsewhere in the codebase, reuse that utility.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/langchain/lib/runs/RunsService.ts around lines 273 to 275, the
generateRunId implementation using Date.now() + Math.random() is not
collision-resistant; replace it with a UUID v4 generator (either add/import from
the 'uuid' package or reuse an existing project UUID utility), update the file
imports to pull in uuidv4 (or the utility), change generateRunId to return a v4
UUID string, and run/update any tests or consumers that assume the old run_
prefix or format (or if needed, prepend a stable prefix like "run_" to the
UUID).
| declare module '@eggjs/tegg' { | ||
| export type LangChainModuleConfig = { | ||
| ChatModel?: ChatModelConfigModuleConfigType; | ||
| agents?: Record<string, {}>; |
There was a problem hiding this comment.
Replace {} with a more specific or safer type.
The {} type in TypeScript doesn't mean "empty object" - it's the type for any non-nullish value. This provides virtually no type safety and could accept strings, numbers, functions, or any object. Consider using Record<string, unknown> at minimum, or define a specific agent configuration type (e.g., AgentConfig) if the agent structure is known.
🔎 Proposed fix with a safer type
- agents?: Record<string, {}>;
+ agents?: Record<string, unknown>;Or better yet, define a specific agent type:
+ export interface AgentConfig {
+ // Define agent configuration properties here
+ }
+
export type LangChainModuleConfig = {
ChatModel?: ChatModelConfigModuleConfigType;
- agents?: Record<string, {}>;
+ agents?: Record<string, AgentConfig>;
};📝 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.
| agents?: Record<string, {}>; | |
| agents?: Record<string, unknown>; |
🤖 Prompt for AI Agents
In plugin/langchain/typings/index.d.ts around line 116, the agents property is
typed as Record<string, {}> which is too permissive; replace {} with a safer
type such as unknown (e.g., Record<string, unknown>) or, better, define and use
a specific AgentConfig interface/type that models the agent configuration and
use Record<string, AgentConfig> to provide proper type safety and intent.
1 participant
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Middleware
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.