feat: add agent-runtime package with @AgentController decorator#411
Merged
jerryliang64 merged 2 commits intomasterfrom Mar 15, 2026
Merged
feat: add agent-runtime package with @AgentController decorator#411jerryliang64 merged 2 commits intomasterfrom
jerryliang64 merged 2 commits intomasterfrom
Conversation
…nd @AgentController decorator Migrated from eggjs/egg next branch (commits ee3c309a, 1a4fb20c, 3e4afccb). - core/types/agent-runtime: type definitions for AgentStore, AgentRuntime, messages, errors - core/agent-runtime: AgentRuntime with sync/async/stream execution, OSSAgentStore, HttpSSEWriter - core/controller-decorator: @AgentController decorator with smart defaults pattern, AgentInfoUtil - core/runtime: export EggObjectUtil for plugin use - plugin/controller: AgentControllerProto + AgentControllerObject lifecycle integration - core/tegg: agent.ts facade re-exporting decorator, runtime classes and types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Summary of ChangesHello, 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 Egg.js Tegg framework by integrating a new agent runtime system. It provides developers with a standardized and streamlined way to build agent-based applications, offering built-in support for thread and run management, persistent storage, and real-time event streaming. The introduction of the Highlights
Changelog
Activity
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
|
📝 WalkthroughWalkthroughAdds a new agent-runtime package with AgentRuntime orchestration (sync/async/stream/run streaming), OSS-backed storage and object client, SSE streaming utilities, run/message utilities, controller decorator integration, types, tests, and Egg.js prototype/object glue to expose agent controllers. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
participant AgentRuntime
participant Store as AgentStore
participant Executor
participant SSEWriter
end
Client->>AgentRuntime: streamRun(input, writer)
activate AgentRuntime
AgentRuntime->>Store: createThread()
Store-->>AgentRuntime: thread
AgentRuntime->>SSEWriter: writeEvent(ThreadRunCreated, run)
AgentRuntime->>Executor: execRun(input)
Executor-->>AgentRuntime: AsyncGenerator<AgentStreamMessage>
AgentRuntime->>SSEWriter: writeEvent(ThreadRunInProgress, run)
loop consume stream messages
Executor-->>AgentRuntime: AgentStreamMessage
AgentRuntime->>SSEWriter: writeEvent(ThreadMessageDelta, delta)
AgentRuntime->>Store: updateRun(output/usage)
end
AgentRuntime->>SSEWriter: writeEvent(ThreadRunCompleted, run)
AgentRuntime->>SSEWriter: writeEvent(Done)
AgentRuntime->>SSEWriter: end()
deactivate AgentRuntime
sequenceDiagram
rect rgba(200,255,200,0.5)
participant App as EggJs App
participant Proto as AgentControllerProto
participant Object as AgentControllerObject
participant Store as AgentStore
participant Runtime as AgentRuntime
end
App->>Proto: createProto()
Proto-->>App: AgentControllerProto
App->>Object: createObject(proto, lifecycleCtx)
Object->>Object: postConstruct / inject / init
Object->>Store: createStore()
Store-->>Object: store instance
Object->>Runtime: AgentRuntime.create(executor, store, logger)
Runtime-->>Object: runtime instance
Object->>Object: delegate sync/async/stream run methods
Object-->>App: ready object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: a comprehensive agent runtime and an associated @AgentController decorator. The changes are well-structured across several new packages and files, with a clear separation of concerns between the core runtime logic, the storage abstraction, the user-facing decorator, and the framework integration glue. The implementation appears robust, with thorough handling of asynchronous operations, state transitions, and error conditions. The addition of extensive tests is also a great plus.
My review includes a couple of suggestions for improving maintainability by reducing code duplication in AgentRuntime.ts and AgentControllerObject.ts. These are not critical issues but potential refinements for the long-term health of the codebase.
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
core/agent-runtime/package.json-22-28 (1)
22-28:⚠️ Potential issue | 🟡 MinorSimplify
filesarray to avoid referencing non-existent files.The
filesarray includes"index.js"and"index.d.ts"at the root level, but these files don't exist in the repository. The compiled output goes to thedist/directory (as specified bymainandtypes). This pattern is inconsistent with other core packages, which use"dist/**/*.js"and"dist/**/*.d.ts"or similar patterns.Remove the non-existent root-level file references:
Suggested diff
"files": [ - "dist", - "index.js", - "index.d.ts" + "dist" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/package.json` around lines 22 - 28, The package.json "files" array incorrectly lists root-level "index.js" and "index.d.ts" which don't exist; update the "files" entry to only include the compiled output under dist (e.g., replace or remove the root-level "index.js" and "index.d.ts" and use patterns like "dist/**/*.js" and "dist/**/*.d.ts") so it matches the "main": "dist/index.js" and "types": "dist/index.d.ts" settings and other core packages' conventions.core/types/agent-runtime/errors.ts-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorRemove redundant type annotations to fix ESLint errors.
The pipeline is failing because TypeScript can trivially infer the
numbertype from the numeric literals. Remove the explicit type annotations.🔧 Proposed fix
export class AgentNotFoundError extends Error { - status: number = 404; + status = 404;export class AgentConflictError extends Error { - status: number = 409; + status = 409;export class InvalidRunStateTransitionError extends Error { - status: number = 409; + status = 409;Also applies to: 20-20, 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/types/agent-runtime/errors.ts` at line 7, Remove the redundant explicit type annotations on the numeric literal `status` properties in this errors file: locate each `status: number = 404;` (and the two other similar `status: number = ...;` declarations) and change them to just `status = 404;` (or the appropriate numeric literal) so TypeScript infers the type and ESLint errors are resolved.core/agent-runtime/test/OSSAgentStore.test.ts-70-70 (1)
70-70:⚠️ Potential issue | 🟡 MinorNormalize the empty-array spacing in these fixtures.
CI is already failing with
array-bracket-spacingon theseannotations: []literals, so this file will not lint cleanly until they match the repo's configured bracket style.Also applies to: 78-78, 96-96, 106-106, 125-125, 147-147, 157-157, 232-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/OSSAgentStore.test.ts` at line 70, Update the empty-array bracket spacing for the test fixtures so they match the repo's array-bracket-spacing rule: replace each occurrence of "annotations: []" with "annotations: [ ]" in OSSAgentStore.test.ts (affecting the fixtures around the "content" objects noted at the comment and the other occurrences at the listed lines) so all empty-array literals use the normalized "[ ]" spacing.core/agent-runtime/test/HttpSSEWriter.test.ts-54-57 (1)
54-57:⚠️ Potential issue | 🟡 MinorUse dot notation for
connection.CI is already failing on this assertion.
headers.connectionsatisfies the lint rule without changing the test behavior.♻️ Proposed fix
- assert.equal(res.writtenHead.headers['connection'], 'keep-alive'); + assert.equal(res.writtenHead.headers.connection, 'keep-alive');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/HttpSSEWriter.test.ts` around lines 54 - 57, The assertion uses bracket notation for the 'connection' header which violates the lint rule; update the test to use dot notation by accessing res.writtenHead.headers.connection instead of res.writtenHead.headers['connection'] (leave the other header assertions as-is) so the assertion still checks 'keep-alive' but conforms to linting.plugin/controller/test/lib/AgentControllerProto.test.ts-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorFix ESLint arrow-parens error.
The pipeline is failing due to parentheses around single function argument.
🔧 Proposed fix
- return qs.every((q) => q.attribute === 'valid'); + return qs.every(q => q.attribute === 'valid');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/test/lib/AgentControllerProto.test.ts` at line 33, The ESLint error is caused by unnecessary parentheses around the single arrow function parameter in the expression using Array.prototype.every; update the expression qs.every((q) => q.attribute === 'valid') to remove the parentheses so it reads qs.every(q => q.attribute === 'valid') (modify the occurrence in AgentControllerProto.test where the every callback is defined) and re-run lint/tests.plugin/controller/test/lib/AgentControllerProto.test.ts-142-142 (1)
142-142:⚠️ Potential issue | 🟡 MinorFix ESLint array-bracket-spacing error.
🔧 Proposed fix
- assert.deepStrictEqual(result, { constructed: true, args: ['a', 'b'] }); + assert.deepStrictEqual(result, { constructed: true, args: [ 'a', 'b' ] });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/test/lib/AgentControllerProto.test.ts` at line 142, The test assertion in AgentControllerProto.test.ts is tripping the ESLint array-bracket-spacing rule; update the expected array in the assert.deepStrictEqual call so its bracket spacing matches the project's ESLint config (for example change args: ['a', 'b'] to args: [ 'a', 'b' ]), i.e. edit the assert.deepStrictEqual(result, { constructed: true, args: ... }) line to use the correct spacing inside the array brackets.core/controller-decorator/test/AgentController.test.ts-108-152 (1)
108-152:⚠️ Potential issue | 🟡 MinorFix ESLint formatting errors flagged by CI.
Multiple lines have ESLint errors for
array-bracket-spacing(spaces required after[and before]). The pipeline is failing due to these formatting issues.🔧 Proposed fix for array-bracket-spacing
- const methods = ['createThread', 'getThread', 'asyncRun', 'streamRun', 'syncRun', 'getRun', 'cancelRun']; + const methods = [ 'createThread', 'getThread', 'asyncRun', 'streamRun', 'syncRun', 'getRun', 'cancelRun' ];Apply similar spacing fixes to lines 134, 147, 151, 152, 182, 187, 192, 197, 202, 207, and 210.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/controller-decorator/test/AgentController.test.ts` around lines 108 - 152, The test has ESLint array-bracket-spacing failures; update the array literals (e.g., methods, routeMethods, and stubMethods args arrays such as ['createThread'], ['thread_1'], [{ input: { messages: [] } }], etc.) to include a space after '[' and before ']' where required so they follow "array-bracket-spacing": "always" (make them like [ 'createThread' ], [ 'thread_1' ], [ { input: { messages: [] } } ]). Locate occurrences in the AgentFooController test blocks (variables named methods, routeMethods, stubMethods and the args arrays inside the stubMethods entries) and fix spacing consistently across all listed arrays flagged by CI.core/agent-runtime/test/OSSObjectStorageClient.test.ts-72-72 (1)
72-72:⚠️ Potential issue | 🟡 MinorFix ESLint array-bracket-spacing error.
🔧 Proposed fix
- const [key, body] = mockOSS.put.mock.calls[0]; + const [ key, body ] = mockOSS.put.mock.calls[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/OSSObjectStorageClient.test.ts` at line 72, Add an ESLint rule override for the failing line: prepend the destructuring of mockOSS.put.mock.calls[0] with a single-line ESLint disable for array-bracket-spacing (e.g. add "// eslint-disable-next-line array-bracket-spacing") immediately above the line containing "const [key, body] = mockOSS.put.mock.calls[0];" so the test compiles without changing semantics or surrounding code; keep the existing variable names (key, body) and the mock call reference.core/controller-decorator/src/decorator/agent/AgentController.ts-30-43 (1)
30-43:⚠️ Potential issue | 🟡 MinorFix unused parameter
_argflagged by ESLint.The pipeline is failing because
_argis defined but never used. Since the underscore prefix convention isn't sufficient for this ESLint config, either usevoidor add a directive.🔧 Proposed fix using void expression
if (hasParam) { - fn = async function (_arg: unknown) { + fn = async function (arg: unknown) { + void arg; // Unused but required for function.length === 1 throw new Error(`${methodName} not implemented`); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/controller-decorator/src/decorator/agent/AgentController.ts` around lines 30 - 43, The async stub created by createNotImplemented has an unused parameter _arg which ESLint flags; update the branch that creates the param-taking stub (inside createNotImplemented) to explicitly mark the parameter as used (e.g., add a void usage like "void _arg;" as the first statement) so the linter stops complaining while preserving the async signature, then keep calling AgentInfoUtil.setNotImplemented(fn) and return fn unchanged.core/controller-decorator/test/AgentController.test.ts-174-208 (1)
174-208:⚠️ Potential issue | 🟡 MinorFix ESLint arrow-parens errors.
The arrow functions with single parameters should not have parentheses per the project's ESLint configuration.
🔧 Proposed fix for arrow-parens
- const createThread = meta.methods.find((m) => m.name === 'createThread')!; + const createThread = meta.methods.find(m => m.name === 'createThread')!;Apply similar fixes to lines 179, 189, 199, and 208.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/controller-decorator/test/AgentController.test.ts` around lines 174 - 208, ESLint is flagging unnecessary parentheses around single-parameter arrow functions in the test; update the arrow callbacks used with meta.methods.find so they use the concise form (e.g., change (m) => m.name === 'createThread' to m => m.name === 'createThread') for each occurrence referenced by the test (createThread, getThread, asyncRun, streamRun, syncRun, getRun, cancelRun) and any other single-parameter find callbacks mentioned in the comment.core/agent-runtime/src/MessageConverter.ts-23-27 (1)
23-27:⚠️ Potential issue | 🟡 MinorFix arrow-parens lint errors.
The pipeline is failing due to parentheses around single function arguments. The linter expects no parentheses for single arrow function parameters.
🔧 Proposed fix
if (Array.isArray(content)) { return content - .filter((part) => part.type === ContentBlockType.Text) - .map((part) => ({ type: ContentBlockType.Text, text: { value: part.text, annotations: [] } })); + .filter(part => part.type === ContentBlockType.Text) + .map(part => ({ type: ContentBlockType.Text, text: { value: part.text, annotations: [] } })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/src/MessageConverter.ts` around lines 23 - 27, The lint failure is caused by unnecessary parentheses around single arrow function parameters in the array handling inside MessageConverter; update the arrow functions used in the filter and map (where ContentBlockType.Text and the parameter name part are referenced) to use unparenthesized single-parameter syntax (e.g., change (part) => ... to part => ...) so the linter rule for arrow-parens is satisfied.plugin/controller/lib/AgentControllerObject.ts-202-203 (1)
202-203:⚠️ Potential issue | 🟡 MinorFix dot-notation lint error.
The pipeline suggests using dot notation instead of bracket notation.
🔧 Proposed fix
- const streamRunFn = instance['streamRun']; + const streamRunFn = instance.streamRun;Also apply to line 237:
- instance['streamRun'] = async (input: CreateRunInput): Promise<void> => { + instance.streamRun = async (input: CreateRunInput): Promise<void> => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/lib/AgentControllerObject.ts` around lines 202 - 203, Replace bracket-property access with dot notation: change uses of instance['streamRun'] to instance.streamRun when assigning streamRunFn and anywhere else (e.g., the second occurrence around the block at line ~237). Update the checks that create streamRunIsStub to reference streamRunFn derived from instance.streamRun and keep the existing type/function check using typeof and AgentInfoUtil.isNotImplemented(streamRunFn).plugin/controller/lib/AgentControllerObject.ts-101-122 (1)
101-122:⚠️ Potential issue | 🟡 MinorFix arrow-parens lint error.
The pipeline is failing due to parentheses around the single function argument.
🔧 Proposed fix
await Promise.all( - this.proto.injectObjects.map(async (injectObject) => { + this.proto.injectObjects.map(async injectObject => { const proto = injectObject.proto;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/lib/AgentControllerObject.ts` around lines 101 - 122, The lint failure is caused by parentheses around the single arrow parameter in the map callback; update the Promise.all injection step in AgentControllerObject (the this.proto.injectObjects.map callback) to use a parameter without parentheses (change async (injectObject) => to async injectObject =>) so the arrow-parens rule is satisfied while keeping the function body unchanged; ensure no other logic in the callback (loadUnit lookup, initType check, EggContainerFactory.getOrCreateEggObject call, and this.injectProperty calls) is modified.core/agent-runtime/test/AgentRuntime.test.ts-59-79 (1)
59-79:⚠️ Potential issue | 🟡 MinorFix generator-star-spacing lint errors.
The pipeline is failing due to spacing around the
*in generator functions. This affects multiple generator function definitions in this file.🔧 Proposed fix for generator spacing
-function createSlowExecRun(chunks: AgentStreamMessage[], onYielded?: () => void): AgentExecutor['execRun'] { - return async function* (_input: CreateRunInput, signal?: AbortSignal): AsyncGenerator<AgentStreamMessage> { +function createSlowExecRun(chunks: AgentStreamMessage[], onYielded?: () => void): AgentExecutor['execRun'] { + return async function*(_input: CreateRunInput, signal?: AbortSignal): AsyncGenerator<AgentStreamMessage> {Apply similar spacing fixes to all generator functions in the file (lines 85, 106, 241, 365, 401).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/AgentRuntime.test.ts` around lines 59 - 79, Fix the generator-star-spacing lint errors by normalizing the spacing around the '*' for all generator functions in this test file: update the inner generator in createSlowExecRun (the "async function*" declaration returned) and the other generator function definitions referenced in the review so they use the project's ESLint style (no space before '*' and a single space after it, e.g. "function* name()" or "async function* ()" as appropriate); apply the same change to every generator declaration/expression in the file so the linter stops failing.core/agent-runtime/src/MessageConverter.ts-49-55 (1)
49-55:⚠️ Potential issue | 🟡 MinorFix indentation lint errors.
The return type declaration has incorrect indentation per the pipeline failure.
🔧 Proposed fix
static extractFromStreamMessages( messages: AgentStreamMessage[], runId?: string, - ): { - output: MessageObject[]; - usage?: RunUsage; - } { + ): { output: MessageObject[]; usage?: RunUsage } {Or keep multiline but with correct indentation:
static extractFromStreamMessages( messages: AgentStreamMessage[], runId?: string, - ): { - output: MessageObject[]; - usage?: RunUsage; - } { + ): { + output: MessageObject[]; + usage?: RunUsage; + } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/src/MessageConverter.ts` around lines 49 - 55, The return type annotation for MessageConverter.extractFromStreamMessages is mis-indented causing lint failures; reformat the method signature so the multiline return type aligns with the opening parenthesis or collapse it to a single line—ensure the returned type block "{ output: MessageObject[]; usage?: RunUsage; }" is indented to match the function parameters and surrounding style used in this file (adjust the signature in the static extractFromStreamMessages declaration).core/agent-runtime/src/MessageConverter.ts-110-128 (1)
110-128:⚠️ Potential issue | 🟡 MinorFix arrow-parens lint errors in toInputMessageObjects.
🔧 Proposed fix
static toInputMessageObjects(messages: CreateRunInput['input']['messages'], threadId?: string): MessageObject[] { return messages .filter( - (m): m is typeof m & { role: Exclude<typeof m.role, typeof MessageRole.System> } => + m => m is typeof m & { role: Exclude<typeof m.role, typeof MessageRole.System> } => m.role !== MessageRole.System, ) - .map((m) => ({ + .map(m => ({ id: newMsgId(), object: AgentObjectType.ThreadMessage, createdAt: nowUnix(), threadId, role: m.role, status: MessageStatus.Completed, content: typeof m.content === 'string' ? [{ type: ContentBlockType.Text, text: { value: m.content, annotations: [] } }] - : m.content.map((p) => ({ type: ContentBlockType.Text, text: { value: p.text, annotations: [] } })), + : m.content.map(p => ({ type: ContentBlockType.Text, text: { value: p.text, annotations: [] } })), })); }Note: The type guard syntax
(m): m is ...may need to keep parentheses - verify with your ESLint config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/src/MessageConverter.ts` around lines 110 - 128, The lint errors come from inconsistent arrow-parens in toInputMessageObjects; keep the type-guard/filter signature as-is ((m): m is ... => ...) but remove unnecessary parentheses for single-arg arrow callbacks elsewhere: change .map((m) => ... ) to .map(m => ... ) and change m.content.map((p) => ...) to m.content.map(p => ... ), keeping all types and fields (newMsgId, AgentObjectType.ThreadMessage, MessageStatus.Completed, ContentBlockType.Text, MessageRole.System) unchanged.core/agent-runtime/src/OSSAgentStore.ts-110-114 (1)
110-114:⚠️ Potential issue | 🟡 MinorFix array-bracket-spacing lint error.
The pipeline is failing due to spacing in the array destructuring.
🔧 Proposed fix
- const [metaData, messagesData] = await Promise.all([ + const [ metaData, messagesData ] = await Promise.all([Note: Verify the exact spacing rule in your ESLint config - some configs require spaces inside brackets, others don't.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/src/OSSAgentStore.ts` around lines 110 - 114, The array destructuring in getThread is failing lint due to bracket spacing; update the destructuring of [metaData, messagesData] in the getThread method to match your ESLint bracket-spacing rule (either add spaces inside the brackets like [ metaData, messagesData ] or remove them to be [metaData, messagesData ]/ [metaData, messagesData] as per config), and ensure the same spacing style is used consistently for the Promise.all call that references this.threadMetaKey(threadId) and this.threadMessagesKey(threadId).
🧹 Nitpick comments (4)
plugin/controller/test/lib/AgentControllerProto.test.ts (1)
169-187: Test for unregistered creator relies on internal implementation details.Accessing
(EggPrototypeCreatorFactory as any).creatorMap.delete()is fragile and may break if the internal implementation changes. This is acceptable for ensuring complete test coverage, but consider adding a comment noting this is testing an edge case through internal access.📝 Suggested comment
it('should throw when default creator is not registered', () => { - // Temporarily remove the creator + // Temporarily remove the creator by accessing internal map (fragile but needed for edge case) const saved = EggPrototypeCreatorFactory.getPrototypeCreator(DEFAULT_PROTO_IMPL_TYPE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/test/lib/AgentControllerProto.test.ts` around lines 169 - 187, The test manipulates EggPrototypeCreatorFactory internals by calling (EggPrototypeCreatorFactory as any).creatorMap.delete(DEFAULT_PROTO_IMPL_TYPE), which is fragile; add an explanatory comment above that line stating this internal access is intentional to simulate the edge case of an unregistered creator for coverage, referencing EggPrototypeCreatorFactory, creatorMap, DEFAULT_PROTO_IMPL_TYPE and AgentControllerProto.createProto so future maintainers understand why the implementation detail is touched and should not be refactored away without updating the test.core/agent-runtime/test/OSSObjectStorageClient.test.ts (1)
8-45: ThemockFnhelper contains dead code that creates confusion.The original
fnfunction (lines 11-18) and its methods (lines 19-23) are defined but never used - onlywrappedFnis returned. This duplicated logic is confusing and increases maintenance burden.♻️ Proposed fix: Remove dead code
function mockFn() { const calls: any[][] = []; let nextResults: Array<{ type: 'resolve' | 'reject'; value: any }> = []; - const fn = (...args: any[]) => { - calls.push(args); - const result = nextResults.shift(); - if (result) { - return result.type === 'resolve' ? Promise.resolve(result.value) : Promise.reject(result.value); - } - return Promise.resolve({}); - }; - fn.mock = { calls }; - fn.mockResolvedValue = (val: any) => { nextResults = []; fn.mockResolvedValueOnce(val); nextResults = nextResults.map(() => ({ type: 'resolve' as const, value: val })); (fn as any)._defaultResult = { type: 'resolve', value: val }; return fn; }; - fn.mockResolvedValueOnce = (val: any) => { nextResults.push({ type: 'resolve', value: val }); return fn; }; - fn.mockRejectedValue = (val: any) => { (fn as any)._defaultResult = { type: 'reject', value: val }; return fn; }; - fn.mockRejectedValueOnce = (val: any) => { nextResults.push({ type: 'reject', value: val }); return fn; }; - // Override fn to use default result when nextResults is empty const wrappedFn: any = (...args: any[]) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/OSSObjectStorageClient.test.ts` around lines 8 - 45, The test helper mockFn contains dead/duplicated code: the inner fn (and its methods mockResolvedValue/mockRejectedValue/etc.) is created but never used; only wrappedFn is returned. Remove the unused fn block and its associated mock.* assignments, and keep the wrappedFn implementation and its mock methods (using nextResults and _defaultResult) intact; ensure references to nextResults and the mock.* API remain on wrappedFn so existing tests using mockFn.mock, mockResolvedValue, mockResolvedValueOnce, mockRejectedValue, and mockRejectedValueOnce continue to work.core/agent-runtime/src/OSSObjectStorageClient.ts (1)
72-91: Append retry handles single position mismatch but may fail with highly concurrent writers.The append logic retries once on
PositionNotEqualToLength, which handles the common case of a stale cache. However, if multiple concurrent writers are appending to the same object, the retry could also encounter a position mismatch and throw. This is likely acceptable for the intended single-writer use case, but worth documenting.Consider adding a brief comment noting this limitation, or implementing a bounded retry loop if concurrent writers are expected:
if (isOSSError(err, 'PositionNotEqualToLength')) { + // Single retry handles cache staleness; concurrent multi-writer scenarios + // may still fail if the object grows between HEAD and retry. const head = await this.client.head(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/src/OSSObjectStorageClient.ts` around lines 72 - 91, The append method (OSSObjectStorageClient.append) currently retries once on PositionNotEqualToLength using appendPositions and a head() lookup, but can still fail under high concurrency if multiple writers race; update the implementation to either (a) add a short comment above append noting this single-retry limitation and that concurrent writers may still cause failures, referencing append, appendPositions, isOSSError, client.append and client.head, or (b) implement a bounded retry loop: on PositionNotEqualToLength re-read the current position via client.head and retry append up to N times with exponential/backoff before failing, updating appendPositions on each success and propagating the last error if retries exhausted—choose one approach and apply it consistently in append.core/controller-decorator/src/decorator/agent/AgentController.ts (1)
114-116: Stack depth 5 is fragile and may break with transpiler/bundler changes.The hardcoded stack depth assumes a specific call stack structure through decorator application. This could silently produce incorrect file paths if the transpiler (tsc, esbuild, swc) or decorator transform changes.
Consider documenting why depth 5 is needed and under which build conditions it was verified:
// Set file path for prototype - // Stack depth 5: [0] getCalleeFromStack → [1] decorator fn → [2-4] reflect/oxc runtime → [5] user source + // Stack depth 5: [0] getCalleeFromStack → [1] decorator fn → [2-4] reflect/oxc runtime → [5] user source + // Verified with tsc + experimentalDecorators. May need adjustment for other transpilers. PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/controller-decorator/src/decorator/agent/AgentController.ts` around lines 114 - 116, The use of a hardcoded stack depth (5) in PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5)) is fragile; change the call to derive depth dynamically or make it configurable and add a robust fallback so transpiler/bundler stack shape changes don't produce wrong file paths. Update AgentController.ts to (a) add a configurable constant or optional parameter for stack depth used by StackUtil.getCalleeFromStack, (b) implement a detection loop or heuristic in StackUtil to scan stack frames for the first user-source-like path and return that index if found, and (c) keep a documented fallback value (previously 5) and a comment explaining why that fallback was chosen and in which build environments it was validated; reference PrototypeUtil.setFilePath, StackUtil.getCalleeFromStack and the constructor to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/agent-runtime/src/AgentRuntime.ts`:
- Around line 131-140: The current sequence calls
this.store.appendMessages(threadId, ...) then this.store.updateRun(run.id,
rb.complete(...)), which can leave messages persisted while run update fails;
create and use an atomic store method (e.g., Store.completeRunWithMessages or
completeRunTransaction) that takes run id, completed run payload
(rb.complete(...)), and the message objects and performs both writes inside a
single transaction/atomic operation; replace the separate calls in AgentRuntime
(the locations using appendMessages + updateRun) with this new aggregate method
to ensure consistency and avoid duplicated/advanced thread history on retry.
- Around line 409-420: The current cancelRun() does a freshRun = await
this.store.getRun(runId) then unconditionally calls this.store.updateRun(runId,
rb.cancel()), which still allows a lost-update if another worker terminalizes
the run between the read and the write; change the final write to a
conditional/compare-and-set update that only applies the cancel transition if
the run is still in the expected pre-cancel state or version: use the store's
conditional update API (e.g., an expectedStatus/expectedVersion/ETag parameter
or a compareAndSet-like method) when calling updateRun so the update fails if
freshRun has changed, and on failure re-read freshRun and return its terminal
snapshot (using RunBuilder.fromRecord(freshRun).snapshot()) rather than
overwriting it.
- Around line 97-105: The local AbortController branches (the immediate
signal.aborted check and the signal 'abort' listener, plus any direct abort
paths in destroy()) currently short-circuit without persisting cancellation
state; instead, funnel these through the existing store-backed cancellation
logic by invoking the same cancellation path used by cancelRun() (e.g., call the
cancelRun/runCancellation helper with the current run id or otherwise persist
"cancelling/cancelled" to the run store) whenever abortController.abort() would
be triggered; update the abort listener and the immediate-aborted branch in
syncRun(), and the destroy() abort path to call that shared cancellation
function so stored runs are moved out of in_progress consistently.
- Around line 243-261: The runningTasks registration (creation of
resolveTask/taskPromise and this.runningTasks.set(...)) must be moved inside the
existing try block that has the finally which resolves resolveTask so the map
entry cannot leak if writer.writeEvent or this.store.updateRun throws; update
the code around the sequence that creates
msgId/MessageConverter.createStreamMessage and calls writer.writeEvent and
this.store.updateRun so that resolveTask and taskPromise are initialized and
this.runningTasks.set(run.id, { promise: taskPromise, abortController }) occurs
after entering the try and before any calls that can throw (e.g.,
writer.writeEvent, this.store.updateRun), and ensure the finally still calls
resolveTask(); apply the same change to the second identical block around the
stream-run handling (the other place that creates resolveTask/taskPromise and
sets runningTasks).
In `@core/agent-runtime/src/HttpSSEWriter.ts`:
- Around line 7-10: The close signal can be missed if a consumer calls onClose()
after _closed has already been set because the callback is only stored in
closeCallbacks and never invoked; update HttpSSEWriter so that onClose(callback)
checks _closed and if true immediately invokes the callback (instead of just
pushing it), and ensure the code path that sets _closed (e.g., the internal
close/res-close handler that iterates closeCallbacks) always drains and clears
closeCallbacks after invoking them; add a regression test that closes the
response first and then registers an onClose callback to verify it is called
synchronously/once.
In `@core/types/agent-runtime/index.ts`:
- Around line 1-5: The barrel index.ts is re-exporting message-model symbols
multiple times; pick a single source (preferably AgentMessage) and remove
duplicate wildcard re-exports: either (A) remove the intermediate re-exports of
message types from AgentRuntime.ts and AgentStore.ts so index.ts can continue
exporting everything via export * from './AgentMessage', or (B) keep
AgentRuntime.ts and AgentStore.ts but change core/types/agent-runtime/index.ts
to explicit named exports for the message types (ContentBlockType,
InputContentPart, TextContentBlock, MessageContentBlock, InputMessage,
MessageObject) by importing them from './AgentMessage' and exporting those names
directly; update imports accordingly so message-model symbols are only sourced
from AgentMessage.
---
Minor comments:
In `@core/agent-runtime/package.json`:
- Around line 22-28: The package.json "files" array incorrectly lists root-level
"index.js" and "index.d.ts" which don't exist; update the "files" entry to only
include the compiled output under dist (e.g., replace or remove the root-level
"index.js" and "index.d.ts" and use patterns like "dist/**/*.js" and
"dist/**/*.d.ts") so it matches the "main": "dist/index.js" and "types":
"dist/index.d.ts" settings and other core packages' conventions.
In `@core/agent-runtime/src/MessageConverter.ts`:
- Around line 23-27: The lint failure is caused by unnecessary parentheses
around single arrow function parameters in the array handling inside
MessageConverter; update the arrow functions used in the filter and map (where
ContentBlockType.Text and the parameter name part are referenced) to use
unparenthesized single-parameter syntax (e.g., change (part) => ... to part =>
...) so the linter rule for arrow-parens is satisfied.
- Around line 49-55: The return type annotation for
MessageConverter.extractFromStreamMessages is mis-indented causing lint
failures; reformat the method signature so the multiline return type aligns with
the opening parenthesis or collapse it to a single line—ensure the returned type
block "{ output: MessageObject[]; usage?: RunUsage; }" is indented to match the
function parameters and surrounding style used in this file (adjust the
signature in the static extractFromStreamMessages declaration).
- Around line 110-128: The lint errors come from inconsistent arrow-parens in
toInputMessageObjects; keep the type-guard/filter signature as-is ((m): m is ...
=> ...) but remove unnecessary parentheses for single-arg arrow callbacks
elsewhere: change .map((m) => ... ) to .map(m => ... ) and change
m.content.map((p) => ...) to m.content.map(p => ... ), keeping all types and
fields (newMsgId, AgentObjectType.ThreadMessage, MessageStatus.Completed,
ContentBlockType.Text, MessageRole.System) unchanged.
In `@core/agent-runtime/src/OSSAgentStore.ts`:
- Around line 110-114: The array destructuring in getThread is failing lint due
to bracket spacing; update the destructuring of [metaData, messagesData] in the
getThread method to match your ESLint bracket-spacing rule (either add spaces
inside the brackets like [ metaData, messagesData ] or remove them to be
[metaData, messagesData ]/ [metaData, messagesData] as per config), and ensure
the same spacing style is used consistently for the Promise.all call that
references this.threadMetaKey(threadId) and this.threadMessagesKey(threadId).
In `@core/agent-runtime/test/AgentRuntime.test.ts`:
- Around line 59-79: Fix the generator-star-spacing lint errors by normalizing
the spacing around the '*' for all generator functions in this test file: update
the inner generator in createSlowExecRun (the "async function*" declaration
returned) and the other generator function definitions referenced in the review
so they use the project's ESLint style (no space before '*' and a single space
after it, e.g. "function* name()" or "async function* ()" as appropriate); apply
the same change to every generator declaration/expression in the file so the
linter stops failing.
In `@core/agent-runtime/test/HttpSSEWriter.test.ts`:
- Around line 54-57: The assertion uses bracket notation for the 'connection'
header which violates the lint rule; update the test to use dot notation by
accessing res.writtenHead.headers.connection instead of
res.writtenHead.headers['connection'] (leave the other header assertions as-is)
so the assertion still checks 'keep-alive' but conforms to linting.
In `@core/agent-runtime/test/OSSAgentStore.test.ts`:
- Line 70: Update the empty-array bracket spacing for the test fixtures so they
match the repo's array-bracket-spacing rule: replace each occurrence of
"annotations: []" with "annotations: [ ]" in OSSAgentStore.test.ts (affecting
the fixtures around the "content" objects noted at the comment and the other
occurrences at the listed lines) so all empty-array literals use the normalized
"[ ]" spacing.
In `@core/agent-runtime/test/OSSObjectStorageClient.test.ts`:
- Line 72: Add an ESLint rule override for the failing line: prepend the
destructuring of mockOSS.put.mock.calls[0] with a single-line ESLint disable for
array-bracket-spacing (e.g. add "// eslint-disable-next-line
array-bracket-spacing") immediately above the line containing "const [key, body]
= mockOSS.put.mock.calls[0];" so the test compiles without changing semantics or
surrounding code; keep the existing variable names (key, body) and the mock call
reference.
In `@core/controller-decorator/src/decorator/agent/AgentController.ts`:
- Around line 30-43: The async stub created by createNotImplemented has an
unused parameter _arg which ESLint flags; update the branch that creates the
param-taking stub (inside createNotImplemented) to explicitly mark the parameter
as used (e.g., add a void usage like "void _arg;" as the first statement) so the
linter stops complaining while preserving the async signature, then keep calling
AgentInfoUtil.setNotImplemented(fn) and return fn unchanged.
In `@core/controller-decorator/test/AgentController.test.ts`:
- Around line 108-152: The test has ESLint array-bracket-spacing failures;
update the array literals (e.g., methods, routeMethods, and stubMethods args
arrays such as ['createThread'], ['thread_1'], [{ input: { messages: [] } }],
etc.) to include a space after '[' and before ']' where required so they follow
"array-bracket-spacing": "always" (make them like [ 'createThread' ], [
'thread_1' ], [ { input: { messages: [] } } ]). Locate occurrences in the
AgentFooController test blocks (variables named methods, routeMethods,
stubMethods and the args arrays inside the stubMethods entries) and fix spacing
consistently across all listed arrays flagged by CI.
- Around line 174-208: ESLint is flagging unnecessary parentheses around
single-parameter arrow functions in the test; update the arrow callbacks used
with meta.methods.find so they use the concise form (e.g., change (m) => m.name
=== 'createThread' to m => m.name === 'createThread') for each occurrence
referenced by the test (createThread, getThread, asyncRun, streamRun, syncRun,
getRun, cancelRun) and any other single-parameter find callbacks mentioned in
the comment.
In `@core/types/agent-runtime/errors.ts`:
- Line 7: Remove the redundant explicit type annotations on the numeric literal
`status` properties in this errors file: locate each `status: number = 404;`
(and the two other similar `status: number = ...;` declarations) and change them
to just `status = 404;` (or the appropriate numeric literal) so TypeScript
infers the type and ESLint errors are resolved.
In `@plugin/controller/lib/AgentControllerObject.ts`:
- Around line 202-203: Replace bracket-property access with dot notation: change
uses of instance['streamRun'] to instance.streamRun when assigning streamRunFn
and anywhere else (e.g., the second occurrence around the block at line ~237).
Update the checks that create streamRunIsStub to reference streamRunFn derived
from instance.streamRun and keep the existing type/function check using typeof
and AgentInfoUtil.isNotImplemented(streamRunFn).
- Around line 101-122: The lint failure is caused by parentheses around the
single arrow parameter in the map callback; update the Promise.all injection
step in AgentControllerObject (the this.proto.injectObjects.map callback) to use
a parameter without parentheses (change async (injectObject) => to async
injectObject =>) so the arrow-parens rule is satisfied while keeping the
function body unchanged; ensure no other logic in the callback (loadUnit lookup,
initType check, EggContainerFactory.getOrCreateEggObject call, and
this.injectProperty calls) is modified.
In `@plugin/controller/test/lib/AgentControllerProto.test.ts`:
- Line 33: The ESLint error is caused by unnecessary parentheses around the
single arrow function parameter in the expression using Array.prototype.every;
update the expression qs.every((q) => q.attribute === 'valid') to remove the
parentheses so it reads qs.every(q => q.attribute === 'valid') (modify the
occurrence in AgentControllerProto.test where the every callback is defined) and
re-run lint/tests.
- Line 142: The test assertion in AgentControllerProto.test.ts is tripping the
ESLint array-bracket-spacing rule; update the expected array in the
assert.deepStrictEqual call so its bracket spacing matches the project's ESLint
config (for example change args: ['a', 'b'] to args: [ 'a', 'b' ]), i.e. edit
the assert.deepStrictEqual(result, { constructed: true, args: ... }) line to use
the correct spacing inside the array brackets.
---
Nitpick comments:
In `@core/agent-runtime/src/OSSObjectStorageClient.ts`:
- Around line 72-91: The append method (OSSObjectStorageClient.append) currently
retries once on PositionNotEqualToLength using appendPositions and a head()
lookup, but can still fail under high concurrency if multiple writers race;
update the implementation to either (a) add a short comment above append noting
this single-retry limitation and that concurrent writers may still cause
failures, referencing append, appendPositions, isOSSError, client.append and
client.head, or (b) implement a bounded retry loop: on PositionNotEqualToLength
re-read the current position via client.head and retry append up to N times with
exponential/backoff before failing, updating appendPositions on each success and
propagating the last error if retries exhausted—choose one approach and apply it
consistently in append.
In `@core/agent-runtime/test/OSSObjectStorageClient.test.ts`:
- Around line 8-45: The test helper mockFn contains dead/duplicated code: the
inner fn (and its methods mockResolvedValue/mockRejectedValue/etc.) is created
but never used; only wrappedFn is returned. Remove the unused fn block and its
associated mock.* assignments, and keep the wrappedFn implementation and its
mock methods (using nextResults and _defaultResult) intact; ensure references to
nextResults and the mock.* API remain on wrappedFn so existing tests using
mockFn.mock, mockResolvedValue, mockResolvedValueOnce, mockRejectedValue, and
mockRejectedValueOnce continue to work.
In `@core/controller-decorator/src/decorator/agent/AgentController.ts`:
- Around line 114-116: The use of a hardcoded stack depth (5) in
PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5))
is fragile; change the call to derive depth dynamically or make it configurable
and add a robust fallback so transpiler/bundler stack shape changes don't
produce wrong file paths. Update AgentController.ts to (a) add a configurable
constant or optional parameter for stack depth used by
StackUtil.getCalleeFromStack, (b) implement a detection loop or heuristic in
StackUtil to scan stack frames for the first user-source-like path and return
that index if found, and (c) keep a documented fallback value (previously 5) and
a comment explaining why that fallback was chosen and in which build
environments it was validated; reference PrototypeUtil.setFilePath,
StackUtil.getCalleeFromStack and the constructor to locate the change.
In `@plugin/controller/test/lib/AgentControllerProto.test.ts`:
- Around line 169-187: The test manipulates EggPrototypeCreatorFactory internals
by calling (EggPrototypeCreatorFactory as
any).creatorMap.delete(DEFAULT_PROTO_IMPL_TYPE), which is fragile; add an
explanatory comment above that line stating this internal access is intentional
to simulate the edge case of an unregistered creator for coverage, referencing
EggPrototypeCreatorFactory, creatorMap, DEFAULT_PROTO_IMPL_TYPE and
AgentControllerProto.createProto so future maintainers understand why the
implementation detail is touched and should not be refactored away without
updating the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94b50553-9e2a-4775-b2d4-6eae46cc0187
📒 Files selected for processing (42)
core/agent-runtime/index.tscore/agent-runtime/package.jsoncore/agent-runtime/src/AgentRuntime.tscore/agent-runtime/src/AgentStoreUtils.tscore/agent-runtime/src/HttpSSEWriter.tscore/agent-runtime/src/MessageConverter.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/src/OSSObjectStorageClient.tscore/agent-runtime/src/RunBuilder.tscore/agent-runtime/src/SSEWriter.tscore/agent-runtime/test/AgentRuntime.test.tscore/agent-runtime/test/HttpSSEWriter.test.tscore/agent-runtime/test/MessageConverter.test.tscore/agent-runtime/test/OSSAgentStore.test.tscore/agent-runtime/test/OSSObjectStorageClient.test.tscore/agent-runtime/test/RunBuilder.test.tscore/agent-runtime/test/helpers.tscore/agent-runtime/tsconfig.jsoncore/agent-runtime/tsconfig.pub.jsoncore/controller-decorator/index.tscore/controller-decorator/src/decorator/agent/AgentController.tscore/controller-decorator/src/decorator/agent/AgentHandler.tscore/controller-decorator/src/decorator/agent/index.tscore/controller-decorator/src/util/AgentInfoUtil.tscore/controller-decorator/test/AgentController.test.tscore/controller-decorator/test/fixtures/AgentFooController.tscore/runtime/index.tscore/tegg/agent.tscore/tegg/package.jsoncore/types/agent-runtime/AgentMessage.tscore/types/agent-runtime/AgentRuntime.tscore/types/agent-runtime/AgentStore.tscore/types/agent-runtime/ObjectStorageClient.tscore/types/agent-runtime/errors.tscore/types/agent-runtime/index.tscore/types/controller-decorator/MetadataKey.tscore/types/index.tsplugin/controller/app.tsplugin/controller/lib/AgentControllerObject.tsplugin/controller/lib/AgentControllerProto.tsplugin/controller/package.jsonplugin/controller/test/lib/AgentControllerProto.test.ts
Fix formatting and code style issues flagged by eslint-config-egg/typescript: - arrow-parens, array-bracket-spacing, indent, dot-notation, generator-star-spacing - Remove unused destructured variables in OSSAgentStore - Remove duplicate type re-exports in agent-runtime types - Add eslint-disable for intentional unused parameter in AgentController stub Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
plugin/controller/test/lib/AgentControllerProto.test.ts (2)
169-186: The test relies on internal implementation details.Directly accessing
creatorMapis fragile and could break ifEggPrototypeCreatorFactory's internals change. This is acceptable for now since the factory lacks anunregisterAPI, but consider documenting this coupling.📝 Add explanatory comment
it('should throw when default creator is not registered', () => { - // Temporarily remove the creator + // Temporarily remove the creator by accessing internal state. + // The factory lacks an unregister API, so direct map access is necessary. const saved = EggPrototypeCreatorFactory.getPrototypeCreator(DEFAULT_PROTO_IMPL_TYPE); EggPrototypeCreatorFactory.registerPrototypeCreator(DEFAULT_PROTO_IMPL_TYPE, undefined as any); - // Force the map entry to be deleted so getPrototypeCreator returns undefined (EggPrototypeCreatorFactory as any).creatorMap.delete(DEFAULT_PROTO_IMPL_TYPE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/test/lib/AgentControllerProto.test.ts` around lines 169 - 186, The test directly manipulates the internal creatorMap of EggPrototypeCreatorFactory (deleting the DEFAULT_PROTO_IMPL_TYPE entry) which is fragile; update the test by adding an explanatory comment above that manipulation explaining why we access creatorMap (no public unregister API exists) and that this is an intentional coupling for the test to simulate an unregistered default creator; reference EggPrototypeCreatorFactory, creatorMap, DEFAULT_PROTO_IMPL_TYPE and AgentControllerProto.createProto in the comment so future maintainers know the rationale and can replace the approach if an unregister API is added.
71-74: Consider moving shared fixtures intobeforeEachfor test isolation.Creating
delegateandprotoat the describe scope is fine here since all tests are read-only. However, usingbeforeEachwould provide stronger isolation guarantees and prevent subtle bugs if future tests modify state.♻️ Suggested refactor using beforeEach
describe('getter delegation', () => { - const delegate = createMockDelegate(); - const proto = new AgentControllerProto(delegate); + let delegate: EggPrototype; + let proto: AgentControllerProto; + + beforeEach(() => { + delegate = createMockDelegate(); + proto = new AgentControllerProto(delegate); + }); it('should delegate id', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/controller/test/lib/AgentControllerProto.test.ts` around lines 71 - 74, The test-suite creates shared fixtures at describe scope (delegate = createMockDelegate(), proto = new AgentControllerProto(delegate)), which can lead to shared-state leaks; change those to be initialized in a beforeEach hook instead: declare delegate and proto as mutable (let) at the describe level, then assign delegate = createMockDelegate() and proto = new AgentControllerProto(delegate) inside beforeEach so each test gets a fresh instance; update any test references accordingly to use the new let-scoped variables.core/controller-decorator/src/decorator/agent/AgentController.ts (1)
115-117: Stack depth may be fragile across runtime environments.The hardcoded depth of 5 depends on the specific call stack structure. Consider adding a comment noting this may need adjustment if the decorator invocation path changes, or consider a more robust approach like searching for the first non-internal frame.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/controller-decorator/src/decorator/agent/AgentController.ts` around lines 115 - 117, The hardcoded stack depth used when calling StackUtil.getCalleeFromStack(false, 5) to set the prototype file path is fragile; update the code around PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5)) to use a more robust approach (e.g., change StackUtil.getCalleeFromStack to accept a sentinel/search mode or add a new helper that scans the call stack for the first non-internal/user frame) or at minimum add a clear comment documenting that the depth 5 is dependent on runtime and may need adjustment; ensure you reference and update the StackUtil.getCalleeFromStack call and PrototypeUtil.setFilePath usage so the constructor gets a reliable file path across environments.core/agent-runtime/test/AgentRuntime.test.ts (2)
399-411: Same generator-without-yield pattern for error testing.Similar to line 240, the generator at line 400-402 throws immediately without yielding. This is intentional but could benefit from a clarifying comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/AgentRuntime.test.ts` around lines 399 - 411, The test assigns executor.execRun to an async generator that immediately throws (no yields) to simulate an execution failure; add a brief clarifying comment above the executor.execRun assignment in the 'should emit failed event when execRun throws' test explaining that the immediate throw-without-yield is intentional for error-path testing (reference executor.execRun, runtime.streamRun, MockSSEWriter and the expectation of AgentSSEEvent.ThreadRunFailed and AgentSSEEvent.Done) so future readers won't interpret it as a mistake.
239-262: Generator without yield is intentional but could be cleaner.The static analyzer flags that the generator at lines 240-242 never yields. This is intentional for testing error paths, but using
async function*syntax for a function that only throws is slightly misleading.Consider using a non-generator async function if the executor interface allows, or add a comment explaining the intentional behavior:
📝 Suggested comment
+ // Intentionally throws without yielding to test error handling path executor.execRun = async function* (): AsyncGenerator<AgentStreamMessage> { throw new Error('exec failed'); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/test/AgentRuntime.test.ts` around lines 239 - 262, The test assigns executor.execRun to an async generator that immediately throws but never yields (execRun) which is flagged by static analysis; either replace the generator with a plain async function that throws if the executor interface accepts a non-generator, or keep the current async function* and add a concise comment above the executor.execRun assignment explaining "intentional generator that throws immediately to exercise error path; no yields required" so readers and linters understand this is deliberate (reference symbols: executor.execRun and runtime.syncRun, and the surrounding test that manipulates store.updateRun).core/agent-runtime/src/RunBuilder.ts (1)
49-52: Consider the empty string fallback forthreadId.When
run.threadIdis undefined,fromRecorduses an empty string. This could cause issues if downstream code (e.g., SSE events, API responses) expects a validthread_prefixed ID.If runs without a
threadIdare intentionally supported, consider documenting this behavior. Otherwise, this may produce confusing API responses withthreadId: "".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/agent-runtime/src/RunBuilder.ts` around lines 49 - 52, The fallback to an empty string in RunBuilder.fromRecord causes downstream code to receive an invalid/empty threadId; update RunBuilder.fromRecord to produce a valid thread ID when run.threadId is missing (or explicitly document/support empty IDs). For example, inside RunBuilder.fromRecord use a deterministic fallback such as a generated/thread-prefixed value (e.g., `thread_${run.id}`) or call a helper that ensures the `threadId` format before passing it to RunBuilder.create; reference RunBuilder.fromRecord, RunBuilder.create and RunRecord.threadId and ensure the chosen fallback preserves any expected `thread_` prefix so downstream SSE/API consumers get a valid threadId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/controller/lib/AgentControllerObject.ts`:
- Around line 219-224: Before calling AgentRuntime.create, ensure
AgentControllerObject.logger is set by validating AgentControllerObject.logger
(and/or that setLogger() has been called); if it's undefined, throw an explicit
error with a clear message indicating the plugin misconfiguration rather than
letting AgentRuntime.create/constructor fail later. Locate the block that
constructs runtime (AgentRuntime.create with executor: this._obj, store, logger:
AgentControllerObject.logger) and add a precondition check that raises a
descriptive exception explaining that a logger must be configured via
setLogger() before creating the AgentRuntime.
In `@plugin/controller/test/lib/AgentControllerProto.test.ts`:
- Around line 155-159: The teardown in the after() hook only restores a previous
creator when originalCreator is truthy, leaving the mock registered if there was
no prior creator; update the after() hook to fully clean up the mock for
DEFAULT_PROTO_IMPL_TYPE by either calling the factory's unregister/remove method
(e.g.,
EggPrototypeCreatorFactory.unregisterPrototypeCreator(DEFAULT_PROTO_IMPL_TYPE))
or, if no unregister exists, explicitly reset the entry by calling
EggPrototypeCreatorFactory.registerPrototypeCreator(DEFAULT_PROTO_IMPL_TYPE,
undefined) when originalCreator is falsy; keep restoring the originalCreator
when it exists.
---
Nitpick comments:
In `@core/agent-runtime/src/RunBuilder.ts`:
- Around line 49-52: The fallback to an empty string in RunBuilder.fromRecord
causes downstream code to receive an invalid/empty threadId; update
RunBuilder.fromRecord to produce a valid thread ID when run.threadId is missing
(or explicitly document/support empty IDs). For example, inside
RunBuilder.fromRecord use a deterministic fallback such as a
generated/thread-prefixed value (e.g., `thread_${run.id}`) or call a helper that
ensures the `threadId` format before passing it to RunBuilder.create; reference
RunBuilder.fromRecord, RunBuilder.create and RunRecord.threadId and ensure the
chosen fallback preserves any expected `thread_` prefix so downstream SSE/API
consumers get a valid threadId.
In `@core/agent-runtime/test/AgentRuntime.test.ts`:
- Around line 399-411: The test assigns executor.execRun to an async generator
that immediately throws (no yields) to simulate an execution failure; add a
brief clarifying comment above the executor.execRun assignment in the 'should
emit failed event when execRun throws' test explaining that the immediate
throw-without-yield is intentional for error-path testing (reference
executor.execRun, runtime.streamRun, MockSSEWriter and the expectation of
AgentSSEEvent.ThreadRunFailed and AgentSSEEvent.Done) so future readers won't
interpret it as a mistake.
- Around line 239-262: The test assigns executor.execRun to an async generator
that immediately throws but never yields (execRun) which is flagged by static
analysis; either replace the generator with a plain async function that throws
if the executor interface accepts a non-generator, or keep the current async
function* and add a concise comment above the executor.execRun assignment
explaining "intentional generator that throws immediately to exercise error
path; no yields required" so readers and linters understand this is deliberate
(reference symbols: executor.execRun and runtime.syncRun, and the surrounding
test that manipulates store.updateRun).
In `@core/controller-decorator/src/decorator/agent/AgentController.ts`:
- Around line 115-117: The hardcoded stack depth used when calling
StackUtil.getCalleeFromStack(false, 5) to set the prototype file path is
fragile; update the code around PrototypeUtil.setFilePath(constructor,
StackUtil.getCalleeFromStack(false, 5)) to use a more robust approach (e.g.,
change StackUtil.getCalleeFromStack to accept a sentinel/search mode or add a
new helper that scans the call stack for the first non-internal/user frame) or
at minimum add a clear comment documenting that the depth 5 is dependent on
runtime and may need adjustment; ensure you reference and update the
StackUtil.getCalleeFromStack call and PrototypeUtil.setFilePath usage so the
constructor gets a reliable file path across environments.
In `@plugin/controller/test/lib/AgentControllerProto.test.ts`:
- Around line 169-186: The test directly manipulates the internal creatorMap of
EggPrototypeCreatorFactory (deleting the DEFAULT_PROTO_IMPL_TYPE entry) which is
fragile; update the test by adding an explanatory comment above that
manipulation explaining why we access creatorMap (no public unregister API
exists) and that this is an intentional coupling for the test to simulate an
unregistered default creator; reference EggPrototypeCreatorFactory, creatorMap,
DEFAULT_PROTO_IMPL_TYPE and AgentControllerProto.createProto in the comment so
future maintainers know the rationale and can replace the approach if an
unregister API is added.
- Around line 71-74: The test-suite creates shared fixtures at describe scope
(delegate = createMockDelegate(), proto = new AgentControllerProto(delegate)),
which can lead to shared-state leaks; change those to be initialized in a
beforeEach hook instead: declare delegate and proto as mutable (let) at the
describe level, then assign delegate = createMockDelegate() and proto = new
AgentControllerProto(delegate) inside beforeEach so each test gets a fresh
instance; update any test references accordingly to use the new let-scoped
variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1aa51de9-36fc-424e-8f82-b2167239ee79
📒 Files selected for processing (16)
core/agent-runtime/src/AgentRuntime.tscore/agent-runtime/src/MessageConverter.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/src/RunBuilder.tscore/agent-runtime/test/AgentRuntime.test.tscore/agent-runtime/test/HttpSSEWriter.test.tscore/agent-runtime/test/OSSAgentStore.test.tscore/agent-runtime/test/OSSObjectStorageClient.test.tscore/agent-runtime/test/RunBuilder.test.tscore/controller-decorator/src/decorator/agent/AgentController.tscore/controller-decorator/test/AgentController.test.tscore/types/agent-runtime/AgentRuntime.tscore/types/agent-runtime/AgentStore.tscore/types/agent-runtime/errors.tsplugin/controller/lib/AgentControllerObject.tsplugin/controller/test/lib/AgentControllerProto.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- core/agent-runtime/test/HttpSSEWriter.test.ts
- core/agent-runtime/test/OSSObjectStorageClient.test.ts
- core/types/agent-runtime/errors.ts
- core/agent-runtime/src/MessageConverter.ts
- core/agent-runtime/test/RunBuilder.test.ts
- core/types/agent-runtime/AgentRuntime.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
core/agent-runtimepackage: AgentRuntime engine with sync/async/stream execution modes, OSSAgentStore for OSS-backed persistent storage, HttpSSEWriter for Server-Sent Events@AgentControllerdecorator incore/controller-decorator: auto-generates 7 HTTP routes (threads CRUD, runs CRUD with stream/sync/async), smart defaults pattern where users only implementexecRun+createStoreAgentControllerProto+AgentControllerObjectinplugin/controller: custom EggObject lifecycle with AgentRuntime installation between postInject and init hookscore/types/agent-runtime: AgentStore, AgentRuntime, messages, errorscore/tegg/agent.tsfacade for convenient re-exportsMigrated from eggjs/egg next branch (commits ee3c309a, 1a4fb20c, 3e4afccb), adapted for tegg independent repo conventions (mocha tests, tsc build, package name mappings, default exports).
Test plan
tsc --noEmitpasses for core/agent-runtime, core/controller-decorator, plugin/controller🤖 Generated with Claude Code
Summary by CodeRabbit