feat(tegg): [3/3] add @AgentController decorator with plugin integration#5827
feat(tegg): [3/3] add @AgentController decorator with plugin integration#5827jerryliang64 wants to merge 13 commits intonextfrom
Conversation
- Add @AgentController() decorator that auto-registers 7 OpenAI-compatible HTTP routes under /api/v1 - Add AgentHandler interface, AgentInfoUtil metadata utilities, and AgentControllerProto/AgentControllerObject for full IoC lifecycle integration - Add NodeSSEWriter implementation with lazy header writing and lowercase HTTP headers - Enforce createStore() method in @AgentController (no default store) - Use AgentRuntime.create() with AgentExecutor interface - Add @eggjs/tegg/agent public API entry point for re-exports - Add AGENT_CONTROLLER_PROTO_IMPL_TYPE and metadata symbols in @eggjs/tegg-types - Register agent controller factory in controller plugin boot hook (app.ts) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds agent controller and agent-runtime features: Node SSE writer, AgentController decorator and AgentHandler interface, metadata utilities, AgentControllerProto/Object with AgentRuntime wiring and streamRun → NodeSSEWriter integration, package exports, tests, and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Lifecycle as EggObject Lifecycle
participant ACO as AgentControllerObject
participant User as Controller (user object)
participant Store as AgentStore
participant Runtime as AgentRuntime
participant Client as HTTP Client/Response
Lifecycle->>ACO: init(ctx)
ACO->>User: call createStore()? (if implemented)
User-->>Store: returns AgentStore
ACO->>Store: store.init()? (if present)
ACO->>Runtime: AgentRuntime.create(executor=User, store, logger)
ACO->>ACO: bind runtime methods (asyncRun/syncRun/streamRun...)
ACO->>User: call user.init()
User-->>ACO: init complete
Client->>ACO: streamRun HTTP request
ACO->>ACO: disable auto-response, create NodeSSEWriter(res)
ACO->>Runtime: runtime.streamRun(input)
Runtime-->>ACO: yield AgentStreamMessage
ACO->>Client: NodeSSEWriter.writeEvent(event, data)
Client-->>ACO: may close connection -> NodeSSEWriter.onClose triggers
ACO->>Runtime: stop/cleanup on client disconnect
Lifecycle->>ACO: destroy(ctx)
ACO->>Runtime: runtime.destroy()
Runtime-->>ACO: cleanup complete
ACO->>User: run preDestroy/destroy hooks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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, 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, the third in a series, introduces 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
|
Deploying egg with
|
| Latest commit: |
2af6c6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://918411d4.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-pr3-agent-controller.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
2af6c6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dce1b716.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-pr3-agent-controller.egg-v3.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the @AgentController decorator, a significant feature for creating OpenAI-compatible API endpoints, integrating new decorators, interfaces (AgentHandler), and a NodeSSEWriter with the tegg IoC container. A medium-severity SSE injection vulnerability has been identified in NodeSSEWriter.ts due to unsanitized event parameters. Additionally, there's a potential maintainability issue in AgentControllerObject.ts due to code duplication. Addressing the SSE injection is critical for security, and resolving the code duplication will improve maintainability.
| writeEvent(event: string, data: unknown): void { | ||
| if (this._closed) return; | ||
| this.ensureHeaders(); | ||
| this.res.write(`event: ${event}\ndata: ${JSON.stringify(data)}\n\n`); |
There was a problem hiding this comment.
The writeEvent method directly concatenates the event parameter into the SSE response stream without sanitization. If an attacker can control the event string (e.g., through a malicious agent implementation or manipulated LLM output that influences the event type), they can inject arbitrary SSE fields (like id, retry) or even split the event by injecting newline characters (\n or \r). This can lead to protocol manipulation and potentially mislead the client-side SSE handler.
To remediate this, sanitize the event string by removing or escaping newline characters before using it in the response.
const sanitizedEvent = event.replace(/[\r\n]/g, '');
this.res.write(`event: ${sanitizedEvent}\ndata: ${JSON.stringify(data)}\n\n`);| async init(ctx: EggObjectLifeCycleContext): Promise<void> { | ||
| try { | ||
| // 1. Construct object | ||
| this._obj = this.proto.constructEggObject(); | ||
| const objLifecycleHook = this._obj as EggObjectLifecycle; | ||
|
|
||
| // 2. Global preCreate hook | ||
| await EggObjectLifecycleUtil.objectPreCreate(ctx, this); | ||
|
|
||
| // 3. Self postConstruct hook | ||
| const postConstructMethod = | ||
| EggObjectLifecycleUtil.getLifecycleHook('postConstruct', this.proto) ?? 'postConstruct'; | ||
| if (objLifecycleHook[postConstructMethod]) { | ||
| await objLifecycleHook[postConstructMethod](ctx, this); | ||
| } | ||
|
|
||
| // 4. Self preInject hook | ||
| const preInjectMethod = EggObjectLifecycleUtil.getLifecycleHook('preInject', this.proto) ?? 'preInject'; | ||
| if (objLifecycleHook[preInjectMethod]) { | ||
| await objLifecycleHook[preInjectMethod](ctx, this); | ||
| } | ||
|
|
||
| // 5. Inject dependencies | ||
| await Promise.all( | ||
| this.proto.injectObjects.map(async (injectObject) => { | ||
| const proto = injectObject.proto; | ||
| const loadUnit = LoadUnitFactory.getLoadUnitById(proto.loadUnitId); | ||
| if (!loadUnit) { | ||
| throw new Error(`can not find load unit: ${proto.loadUnitId}`); | ||
| } | ||
| if ( | ||
| this.proto.initType !== ObjectInitType.CONTEXT && | ||
| injectObject.proto.initType === ObjectInitType.CONTEXT | ||
| ) { | ||
| this.injectProperty( | ||
| injectObject.refName, | ||
| EggObjectUtil.contextEggObjectGetProperty(proto, injectObject.objName), | ||
| ); | ||
| } else { | ||
| const injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| this.injectProperty(injectObject.refName, EggObjectUtil.eggObjectGetProperty(injectObj)); | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| // 6. Global postCreate hook | ||
| await EggObjectLifecycleUtil.objectPostCreate(ctx, this); | ||
|
|
||
| // 7. Self postInject hook | ||
| const postInjectMethod = EggObjectLifecycleUtil.getLifecycleHook('postInject', this.proto) ?? 'postInject'; | ||
| if (objLifecycleHook[postInjectMethod]) { | ||
| await objLifecycleHook[postInjectMethod](ctx, this); | ||
| } | ||
|
|
||
| // === AgentRuntime installation (before user init) === | ||
| await this.installAgentRuntime(); | ||
|
|
||
| // 8. Self init hook (user's init()) | ||
| const initMethod = EggObjectLifecycleUtil.getLifecycleHook('init', this.proto) ?? 'init'; | ||
| if (objLifecycleHook[initMethod]) { | ||
| await objLifecycleHook[initMethod](ctx, this); | ||
| } | ||
|
|
||
| // 9. Ready | ||
| this.status = EggObjectStatus.READY; | ||
| } catch (e) { | ||
| // Clean up runtime if it was created but init failed | ||
| if (this.runtime) { | ||
| try { | ||
| await this.runtime.destroy(); | ||
| } catch { | ||
| // Swallow cleanup errors to preserve the original exception | ||
| } | ||
| this.runtime = undefined; | ||
| } | ||
| this.status = EggObjectStatus.ERROR; | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
The init method in AgentControllerObject appears to be a complete reimplementation of the object initialization and dependency injection lifecycle, likely copied from EggObjectImpl.initWithInjectProperty. While this allows for the crucial insertion of installAgentRuntime() at the correct point in the lifecycle (after postInject and before the user's init), it introduces a significant maintainability risk.
Any future changes to the core object lifecycle in EggObjectImpl will not be automatically reflected here, potentially leading to subtle bugs. This code duplication could become a source of technical debt.
Consider exploring alternatives that could reduce this duplication. For example:
- Could
EggObjectImplbe made extensible, allowingAgentControllerObjectto inherit from it and override a specific lifecycle hook? - Could a new, more granular lifecycle hook be introduced into the core framework specifically for this type of enhancement?
If this is the only viable approach given the current framework design, please add a prominent comment linking to the source of the copied logic (e.g., EggObjectImpl.initWithInjectProperty) to aid future maintenance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tegg/core/agent-runtime/test/NodeSSEWriter.test.ts (1)
38-40: Drop the repeatedas anycasts in these tests.The fixture only needs a small
ServerResponsesurface, but the current casts and lint suppressions bypass the repo’s TS rules and make API drift easy to miss. A typed helper or anunknownbridge would keep the mock checked without eight separate suppressions.As per coding guidelines, "Avoid
anytype; useunknownwhen type is truly unknown in TypeScript".Also applies to: 54-56, 65-67, 74-76, 89-90, 102-103, 118-119, 135-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/NodeSSEWriter.test.ts` around lines 38 - 40, The tests use repeated "as any" casts for the fixture (res) which bypass type checks; replace those casts by either (a) creating a small typed mock helper like createResMock(): Partial<ServerResponse> that implements only the methods used by NodeSSEWriter and use it as res as ServerResponse, or (b) use an unknown bridge (res as unknown as ServerResponse) instead of "as any", and remove the eslint-disable comments; update each instantiation of NodeSSEWriter (and other places flagged) to use the typed mock or unknown cast so TypeScript validates the ServerResponse surface while keeping the test behavior the same.tegg/core/controller-decorator/test/fixtures/AgentFooController.ts (1)
3-4: Consider using.jsextension for ESM imports.The imports use
.tsextensions, but the coding guidelines specify using.jsextensions for ESM files. There's also inconsistency within this PR -AgentController.test.tsline 16 imports this fixture with.jsextension ('./fixtures/AgentFooController.js').🔧 Suggested change for consistency
-import { AgentController } from '../../src/decorator/agent/AgentController.ts'; -import type { AgentHandler } from '../../src/decorator/agent/AgentHandler.ts'; +import { AgentController } from '../../src/decorator/agent/AgentController.js'; +import type { AgentHandler } from '../../src/decorator/agent/AgentHandler.js';As per coding guidelines: "All imports should use
.jsextensions for ESM files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/controller-decorator/test/fixtures/AgentFooController.ts` around lines 3 - 4, Update the ESM imports in the AgentFooController fixture to use .js extensions to match project conventions and the test import; specifically change the import specifiers that reference '../../src/decorator/agent/AgentController.ts' and '../../src/decorator/agent/AgentHandler.ts' to use '../../src/decorator/agent/AgentController.js' and '../../src/decorator/agent/AgentHandler.js' so the fixture (AgentFooController.ts) and AgentController.test.ts use consistent .js imports.tegg/plugin/controller/src/lib/AgentControllerObject.ts (1)
220-227: Consider validating logger availability before AgentRuntime creation.If
AgentControllerObject.setLogger()is not called beforecreateObject(), the logger passed toAgentRuntime.create()will beundefined. While the wiring inapp.tsensuressetLoggeris called in the constructor, this implicit dependency could cause issues if the order changes or in test scenarios.🛡️ Suggested defensive check
// Create runtime with AgentRuntime.create factory + if (!AgentControllerObject.logger) { + throw new Error('AgentControllerObject.setLogger() must be called before creating agent controllers'); + } const runtime = AgentRuntime.create({ executor: this._obj as AgentExecutor, store, logger: AgentControllerObject.logger, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/src/lib/AgentControllerObject.ts` around lines 220 - 227, The code passes AgentControllerObject.logger into AgentRuntime.create without ensuring it is set; update createObject (the block that calls AgentRuntime.create) to validate that AgentControllerObject.logger is available (e.g., check this.logger or AgentControllerObject.logger) before creating the runtime and either throw a clear error or provide a sensible default logger; ensure the check occurs right before calling AgentRuntime.create so instance[AGENT_RUNTIME], this.runtime, and the executor wiring only proceed when a valid logger is present.tegg/plugin/controller/src/lib/AgentControllerProto.ts (1)
92-94: Avoidanytype; useunknownfor type-safe delegation.The
...args: anyparameter violates the coding guideline to avoidanytype. Additionally, public methods should have explicit return type annotations perisolatedDeclarationsrequirements.🔧 Proposed fix
- constructEggObject(...args: any): object { + constructEggObject(...args: unknown[]): object { return this.delegate.constructEggObject(...args); }As per coding guidelines: "Avoid
anytype; useunknownwhen type is truly unknown in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/src/lib/AgentControllerProto.ts` around lines 92 - 94, Change the constructEggObject signature to avoid `any` by declaring the rest parameter as `...args: unknown[]` and add an explicit return type annotation (e.g., `: object`) to satisfy isolatedDeclarations; forward the arguments to the delegate call via `this.delegate.constructEggObject(...args)` (if the delegate API requires a narrower type, narrow or assert the args at the call site inside the method) so the public method stays type-safe and declaration-compliant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts`:
- Around line 30-33: writeEvent currently ignores res.write()'s return value and
can overflow memory; update NodeSSEWriter.writeEvent (and ensureHeaders if
needed) to handle backpressure by either: (a) implementing the drain
pattern—check the boolean result of this.res.write(...), and when it returns
false stop emitting/writing further events until this.res.once('drain', ...)
resumes writing; or (b) refactor the producer to a Readable stream and pipe it
to this.res so Node handles backpressure automatically; ensure _closed is still
respected and that pending events are queued/paused during the drain period to
avoid dropping data.
In `@tegg/core/controller-decorator/src/decorator/agent/AgentHandler.ts`:
- Line 14: The AgentHandler.createStore signature uses Promise<unknown> and is
optional; update it to return the concrete store contract and make it required
so incompatible implementations can't compile. Replace the declaration
createStore?(): Promise<unknown>; with createStore(): Promise<AgentStore>; (or
the actual store interface name used in your codebase), import that store
interface into the file, and update all concrete classes implementing
AgentHandler to return the correct AgentStore type from createStore.
---
Nitpick comments:
In `@tegg/core/agent-runtime/test/NodeSSEWriter.test.ts`:
- Around line 38-40: The tests use repeated "as any" casts for the fixture (res)
which bypass type checks; replace those casts by either (a) creating a small
typed mock helper like createResMock(): Partial<ServerResponse> that implements
only the methods used by NodeSSEWriter and use it as res as ServerResponse, or
(b) use an unknown bridge (res as unknown as ServerResponse) instead of "as
any", and remove the eslint-disable comments; update each instantiation of
NodeSSEWriter (and other places flagged) to use the typed mock or unknown cast
so TypeScript validates the ServerResponse surface while keeping the test
behavior the same.
In `@tegg/core/controller-decorator/test/fixtures/AgentFooController.ts`:
- Around line 3-4: Update the ESM imports in the AgentFooController fixture to
use .js extensions to match project conventions and the test import;
specifically change the import specifiers that reference
'../../src/decorator/agent/AgentController.ts' and
'../../src/decorator/agent/AgentHandler.ts' to use
'../../src/decorator/agent/AgentController.js' and
'../../src/decorator/agent/AgentHandler.js' so the fixture
(AgentFooController.ts) and AgentController.test.ts use consistent .js imports.
In `@tegg/plugin/controller/src/lib/AgentControllerObject.ts`:
- Around line 220-227: The code passes AgentControllerObject.logger into
AgentRuntime.create without ensuring it is set; update createObject (the block
that calls AgentRuntime.create) to validate that AgentControllerObject.logger is
available (e.g., check this.logger or AgentControllerObject.logger) before
creating the runtime and either throw a clear error or provide a sensible
default logger; ensure the check occurs right before calling AgentRuntime.create
so instance[AGENT_RUNTIME], this.runtime, and the executor wiring only proceed
when a valid logger is present.
In `@tegg/plugin/controller/src/lib/AgentControllerProto.ts`:
- Around line 92-94: Change the constructEggObject signature to avoid `any` by
declaring the rest parameter as `...args: unknown[]` and add an explicit return
type annotation (e.g., `: object`) to satisfy isolatedDeclarations; forward the
arguments to the delegate call via `this.delegate.constructEggObject(...args)`
(if the delegate API requires a narrower type, narrow or assert the args at the
call site inside the method) so the public method stays type-safe and
declaration-compliant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 050a68fd-27d8-4089-baae-a041e2b72d19
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltegg/core/controller-decorator/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
tegg/core/agent-runtime/src/NodeSSEWriter.tstegg/core/agent-runtime/src/index.tstegg/core/agent-runtime/test/NodeSSEWriter.test.tstegg/core/controller-decorator/src/decorator/agent/AgentController.tstegg/core/controller-decorator/src/decorator/agent/AgentHandler.tstegg/core/controller-decorator/src/decorator/index.tstegg/core/controller-decorator/src/util/AgentInfoUtil.tstegg/core/controller-decorator/src/util/index.tstegg/core/controller-decorator/test/AgentController.test.tstegg/core/controller-decorator/test/fixtures/AgentFooController.tstegg/core/tegg/package.jsontegg/core/tegg/src/agent.tstegg/core/types/src/controller-decorator/MetadataKey.tstegg/plugin/controller/package.jsontegg/plugin/controller/src/app.tstegg/plugin/controller/src/lib/AgentControllerObject.tstegg/plugin/controller/src/lib/AgentControllerProto.ts
| writeEvent(event: string, data: unknown): void { | ||
| if (this._closed) return; | ||
| this.ensureHeaders(); | ||
| this.res.write(`event: ${event}\ndata: ${JSON.stringify(data)}\n\n`); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current Node.js, what does http.ServerResponse.write()returningfalse mean, and what is the recommended way to handle backpressure for streaming/SSE responses?
💡 Result:
What http.ServerResponse.write() returning false means
In current Node.js, response.write(chunk) returns:
true: the chunk was flushed to the OS (kernel) buffer.false: some/all of the chunk had to be queued in user-space memory because the writable buffer is “full” (i.e., at/overhighWaterMark). When capacity is available again, Node will emit'drain'. (nodejs.org)
So false is the backpressure signal: stop producing/writing more data until 'drain' fires, otherwise Node will keep buffering and you can run up memory (and in extreme cases create a security issue if the peer never reads). (nodejs.org)
Recommended backpressure handling for streaming/SSE responses
1) If you can model output as a stream: use pipe() / pipeline()
Node’s guidance is: if data can be generated on demand, encapsulate it as a Readable and connect it to the response (a Writable) via pipe() (or stream.pipeline() for better cleanup/error propagation). This automatically applies backpressure. (nodejs.org)
2) If you’re doing manual res.write() (common in SSE): wait for 'drain'
Pattern:
import { once } from 'node:events';
function writeSSE(res, msg) {
const ok = res.write(`data: ${msg}\n\n`);
return ok ? Promise.resolve() : once(res, 'drain');
}
// producer loop
for await (const msg of source) {
await writeSSE(res, msg); // this is the backpressure gate
}This is exactly the contract: when write() returns false, pause your producer and resume only after 'drain'. (nodejs.org)
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://nodejs.org/api/stream.html
- 3: https://nodejs.org/api/stream.html
- 4: https://nodejs.org/api/http.html
Handle ServerResponse.write() backpressure for reliable SSE streaming.
On slow clients, res.write() returns false when the internal buffer reaches capacity (highWaterMark). The current implementation ignores this signal and continues writing, causing Node to buffer unsent chunks in user-space memory. For long-lived agent streaming responses that can produce high volumes of events, this creates a memory leak and reliability risk.
Implement one of the two Node.js-recommended patterns:
- Drain pattern (if using manual
res.write()): Check the return value and pause the event source until the'drain'event fires before resuming writes. - Stream API (preferred): Model the event source as a
Readableand usepipe()orstream.pipeline()to connect it to the response; this applies backpressure automatically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts` around lines 30 - 33,
writeEvent currently ignores res.write()'s return value and can overflow memory;
update NodeSSEWriter.writeEvent (and ensureHeaders if needed) to handle
backpressure by either: (a) implementing the drain pattern—check the boolean
result of this.res.write(...), and when it returns false stop emitting/writing
further events until this.res.once('drain', ...) resumes writing; or (b)
refactor the producer to a Readable stream and pipe it to this.res so Node
handles backpressure automatically; ensure _closed is still respected and that
pending events are queued/paused during the drain period to avoid dropping data.
| // SSE streaming, async execution, and cancellation via smart defaults. | ||
| export interface AgentHandler { | ||
| execRun(input: CreateRunInput, signal?: AbortSignal): AsyncGenerator<AgentStreamMessage>; | ||
| createStore?(): Promise<unknown>; |
There was a problem hiding this comment.
Type createStore() to the actual store contract.
Promise<unknown> erases the new store requirement and still allows incompatible implementations to compile. Since agent controllers are now expected to provide a store, this should return the concrete store interface and ideally not stay optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/controller-decorator/src/decorator/agent/AgentHandler.ts` at line
14, The AgentHandler.createStore signature uses Promise<unknown> and is
optional; update it to return the concrete store contract and make it required
so incompatible implementations can't compile. Replace the declaration
createStore?(): Promise<unknown>; with createStore(): Promise<AgentStore>; (or
the actual store interface name used in your codebase), import that store
interface into the file, and update all concrete classes implementing
AgentHandler to return the correct AgentStore type from createStore.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5827 +/- ##
==========================================
+ Coverage 85.39% 85.74% +0.34%
==========================================
Files 659 665 +6
Lines 12813 12963 +150
Branches 1474 1486 +12
==========================================
+ Hits 10942 11115 +173
+ Misses 1751 1728 -23
Partials 120 120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Save close handler reference and remove it in end() to prevent memory leaks. Clear closeCallbacks after execution to release refs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tegg/core/agent-runtime/src/NodeSSEWriter.ts (1)
33-36:⚠️ Potential issue | 🟠 Major
writeEvent()still ignores write backpressure.Line 36 drops the boolean from
res.write(). On long-lived SSE streams, a slow reader will make that returnfalse; continuing to emit after that lets Node buffer unsent chunks in memory. Please stop producing or queue events until'drain'fires.In current Node.js, what does http.ServerResponse.write() returning false mean, and what is the recommended backpressure handling pattern for manual SSE responses?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts` around lines 33 - 36, The writeEvent method in NodeSSEWriter currently ignores write backpressure by discarding the boolean return of this.res.write; update NodeSSEWriter.writeEvent to observe the boolean result from this.res.write(...) and, when it returns false, stop writing further events and queue the event payloads (or set a _paused flag) and attach a one-time 'drain' listener on this.res to flush the queue and clear the paused state; reuse ensureHeaders() as before, respect this._closed, and make sure the drain handler is cleaned up to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts`:
- Around line 43-48: The end() implementation clears the close listener and
closeCallbacks before calling this.res.end(), which prevents onClose() callbacks
from running on normal completion; change end() to set this._closed = true
early, then call this.res.end(), and only after the stream has finished (or
immediately after res.end() while guarding against duplicate invocation) remove
the 'close' listener (onResClose) and clear/execute closeCallbacks so registered
onClose() cleanup always runs; reference functions/fields: end(), onResClose,
onClose(), closeCallbacks, res.end(), and _closed to locate and adjust the
logic.
- Line 3: Update the ES module import specifier in NodeSSEWriter by replacing
the TypeScript file extension with .js: change the import of SSEWriter from
'./SSEWriter.ts' to './SSEWriter.js' (the import statement that declares "import
type { SSEWriter } from './SSEWriter.ts';"); ensure the rest of the file still
compiles with the type-only import style.
---
Duplicate comments:
In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts`:
- Around line 33-36: The writeEvent method in NodeSSEWriter currently ignores
write backpressure by discarding the boolean return of this.res.write; update
NodeSSEWriter.writeEvent to observe the boolean result from this.res.write(...)
and, when it returns false, stop writing further events and queue the event
payloads (or set a _paused flag) and attach a one-time 'drain' listener on
this.res to flush the queue and clear the paused state; reuse ensureHeaders() as
before, respect this._closed, and make sure the drain handler is cleaned up to
avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec09778b-34b9-424d-b3b1-707c3af0bb83
📒 Files selected for processing (1)
tegg/core/agent-runtime/src/NodeSSEWriter.ts
| @@ -0,0 +1,55 @@ | |||
| import type { ServerResponse } from 'node:http'; | |||
|
|
|||
| import type { SSEWriter } from './SSEWriter.ts'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this package is explicitly configured to allow `.ts` import specifiers.
fd '^tsconfig.*\.json$' -x sh -c 'echo "== {} =="; sed -n "1,220p" "{}"'
rg -n '"allowImportingTsExtensions"|"noEmit"|"moduleResolution"|"module"' -g 'tsconfig*.json'Repository: eggjs/egg
Length of output: 24635
🏁 Script executed:
# Check import patterns in tegg directory to confirm the .js extension guideline is consistently applied
rg "import\s+(type\s+)?\{.*\}\s+from\s+['\"]\..*\.(ts|js)" tegg/ --no-heading -A 0 | head -30Repository: eggjs/egg
Length of output: 4047
Use a .js specifier here.
Line 3 imports ./SSEWriter.ts, which violates the repository's ESM convention for the tegg/ directory. Please change this to ./SSEWriter.js.
Suggested change
-import type { SSEWriter } from './SSEWriter.ts';
+import type { SSEWriter } from './SSEWriter.js';📝 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 type { SSEWriter } from './SSEWriter.ts'; | |
| import type { SSEWriter } from './SSEWriter.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts` at line 3, Update the ES module
import specifier in NodeSSEWriter by replacing the TypeScript file extension
with .js: change the import of SSEWriter from './SSEWriter.ts' to
'./SSEWriter.js' (the import statement that declares "import type { SSEWriter }
from './SSEWriter.ts';"); ensure the rest of the file still compiles with the
type-only import style.
| end(): void { | ||
| if (!this._closed) { | ||
| this._closed = true; | ||
| this.res.off('close', this.onResClose); | ||
| this.closeCallbacks.length = 0; | ||
| this.res.end(); |
There was a problem hiding this comment.
end() drops registered onClose() cleanup.
Lines 46-47 remove the close listener and clear closeCallbacks before res.end() closes the response, so callbacks registered via onClose() never run on the normal completion path. That makes cleanup behavior depend on how the stream ends.
Suggested fix
+ private runCloseCallbacks(): void {
+ if (this._closed) return;
+ this._closed = true;
+ this.res.off('close', this.onResClose);
+ const callbacks = this.closeCallbacks.splice(0);
+ for (const cb of callbacks) cb();
+ }
+
constructor(res: ServerResponse) {
this.res = res;
this.onResClose = () => {
- this._closed = true;
- for (const cb of this.closeCallbacks) cb();
- this.closeCallbacks.length = 0;
+ this.runCloseCallbacks();
};
res.on('close', this.onResClose);
}
@@
end(): void {
- if (!this._closed) {
- this._closed = true;
- this.res.off('close', this.onResClose);
- this.closeCallbacks.length = 0;
- this.res.end();
- }
+ if (this._closed) return;
+ this.runCloseCallbacks();
+ this.res.end();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/NodeSSEWriter.ts` around lines 43 - 48, The end()
implementation clears the close listener and closeCallbacks before calling
this.res.end(), which prevents onClose() callbacks from running on normal
completion; change end() to set this._closed = true early, then call
this.res.end(), and only after the stream has finished (or immediately after
res.end() while guarding against duplicate invocation) remove the 'close'
listener (onResClose) and clear/execute closeCallbacks so registered onClose()
cleanup always runs; reference functions/fields: end(), onResClose, onClose(),
closeCallbacks, res.end(), and _closed to locate and adjust the logic.
…tions - Add agent/index.ts barrel export to match http/index.ts and mcp/index.ts - Rename setIsAgentController to setAgentController for set/get consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tegg/core/controller-decorator/src/util/AgentInfoUtil.ts (1)
18-24: Consider unifying metadata access through MetadataUtil for consistency.These methods use raw
Reflect.defineMetadata/Reflect.getMetadatawhile other methods in this class useMetadataUtil. The divergence is understandable sinceMetadataUtilis typed forEggProtoImplClass, notFunction.For long-term maintainability, consider either:
- Extending
MetadataUtilwith function-level metadata helpers (e.g.,defineFunctionMetaData,getFunctionBooleanMetaData)- Or add a brief comment explaining why raw
Reflectis used hereThis is a minor consistency nit and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/controller-decorator/src/util/AgentInfoUtil.ts` around lines 18 - 24, The setNotImplemented and isNotImplemented functions use raw Reflect metadata while the rest of the class uses MetadataUtil; to unify, add function-level helpers to MetadataUtil (e.g., defineFunctionMetaData(key: string, target: Function, value: any) and getFunctionBooleanMetaData(key: string, target: Function): boolean) and then replace calls in AgentInfoUtil.setNotImplemented and AgentInfoUtil.isNotImplemented to use MetadataUtil.defineFunctionMetaData(CONTROLLER_AGENT_NOT_IMPLEMENTED, fn, true) and MetadataUtil.getFunctionBooleanMetaData(CONTROLLER_AGENT_NOT_IMPLEMENTED, fn) respectively; alternatively, if you prefer not to change MetadataUtil now, add a concise comment above these two methods explaining why raw Reflect is used and referencing the typing limitation of MetadataUtil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tegg/core/controller-decorator/src/util/AgentInfoUtil.ts`:
- Around line 18-24: The setNotImplemented and isNotImplemented functions use
raw Reflect metadata while the rest of the class uses MetadataUtil; to unify,
add function-level helpers to MetadataUtil (e.g., defineFunctionMetaData(key:
string, target: Function, value: any) and getFunctionBooleanMetaData(key:
string, target: Function): boolean) and then replace calls in
AgentInfoUtil.setNotImplemented and AgentInfoUtil.isNotImplemented to use
MetadataUtil.defineFunctionMetaData(CONTROLLER_AGENT_NOT_IMPLEMENTED, fn, true)
and MetadataUtil.getFunctionBooleanMetaData(CONTROLLER_AGENT_NOT_IMPLEMENTED,
fn) respectively; alternatively, if you prefer not to change MetadataUtil now,
add a concise comment above these two methods explaining why raw Reflect is used
and referencing the typing limitation of MetadataUtil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b569f124-ab75-4c17-85e5-a825df264bca
📒 Files selected for processing (4)
tegg/core/controller-decorator/src/decorator/agent/AgentController.tstegg/core/controller-decorator/src/decorator/agent/index.tstegg/core/controller-decorator/src/decorator/index.tstegg/core/controller-decorator/src/util/AgentInfoUtil.ts
✅ Files skipped from review due to trivial changes (1)
- tegg/core/controller-decorator/src/decorator/agent/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tegg/core/controller-decorator/src/decorator/agent/AgentController.ts
…Obj mutation - Unify CreateRunInput.input.messages to use InputMessage[] from AgentStore, eliminating duplicate type definitions - Add abortController.signal.aborted check in streamRun catch block to mark aborted runs as cancelled instead of failed, consistent with syncRun/asyncRun - Replace msgObj mutation with new object spread to avoid aliasing issues - Replace inline import() types with top-level imports in AgentRuntime types - Add streamRun thread messages persistence test for parity with syncRun/asyncRun Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Change EggLogger import from 'egg-logger' to 'egg' in AgentControllerObject to fix oxlint TS2307 module resolution error - Update @eggjs/tegg exports snapshot to include new AgentController exports from controller-decorator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/types/src/agent-runtime/AgentRuntime.ts`:
- Line 1: Update the ESM import specifier in AgentRuntime.ts to use a .js
extension: change the type import line that imports AgentRunConfig,
InputMessage, MessageObject, RunStatus from './AgentStore.ts' to import from
'./AgentStore.js' (keep it as an `import type` and only modify the module
specifier).
In `@tegg/plugin/controller/src/lib/AgentControllerObject.ts`:
- Line 19: Update the local ESM import specifier to use a .js extension: replace
the import of AgentControllerProto in AgentControllerObject (the line importing
AgentControllerProto) so it references './AgentControllerProto.js' instead of
'./AgentControllerProto.ts' to follow the repo's ESM import convention.
- Around line 205-225: The store created by createStoreFn and initialized via
store.init may leak if AgentRuntime.create throws before this.runtime is
assigned; wrap the store initialization and AgentRuntime.create call in a
try/catch/finally such that on any error prior to assigning this.runtime you
call store.destroy() (or await store.destroy() if async) to clean up resources,
rethrow the error, and only set this.runtime = runtime after successful
creation; reference createStoreFn, store.init, AgentRuntime.create, this.runtime
and store.destroy in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ccc6c33-3efd-43ea-915a-6587144c0eb4
⛔ Files ignored due to path filters (1)
tegg/core/tegg/test/__snapshots__/exports.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
tegg/core/agent-runtime/src/AgentRuntime.tstegg/core/agent-runtime/test/AgentRuntime.test.tstegg/core/types/src/agent-runtime/AgentRuntime.tstegg/plugin/controller/src/lib/AgentControllerObject.ts
| @@ -1,3 +1,5 @@ | |||
| import type { AgentRunConfig, InputMessage, MessageObject, RunStatus } from './AgentStore.ts'; | |||
There was a problem hiding this comment.
Switch the local ESM import to .js.
This new import still uses a .ts specifier, which can leak TypeScript-only paths into emitted ESM.
♻️ Suggested fix
-import type { AgentRunConfig, InputMessage, MessageObject, RunStatus } from './AgentStore.ts';
+import type { AgentRunConfig, InputMessage, MessageObject, RunStatus } from './AgentStore.js';As per coding guidelines, "All imports should use .js extensions for ESM files".
📝 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 type { AgentRunConfig, InputMessage, MessageObject, RunStatus } from './AgentStore.ts'; | |
| import type { AgentRunConfig, InputMessage, MessageObject, RunStatus } from './AgentStore.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/types/src/agent-runtime/AgentRuntime.ts` at line 1, Update the ESM
import specifier in AgentRuntime.ts to use a .js extension: change the type
import line that imports AgentRunConfig, InputMessage, MessageObject, RunStatus
from './AgentStore.ts' to import from './AgentStore.js' (keep it as an `import
type` and only modify the module specifier).
| import type { AgentStore, CreateRunInput } from '@eggjs/tegg-types/agent-runtime'; | ||
| import type { EggLogger } from 'egg'; | ||
|
|
||
| import { AgentControllerProto } from './AgentControllerProto.ts'; |
There was a problem hiding this comment.
Use a .js specifier for the local ESM import.
./AgentControllerProto.ts breaks the repo's ESM import convention for tegg/* sources.
♻️ Suggested fix
-import { AgentControllerProto } from './AgentControllerProto.ts';
+import { AgentControllerProto } from './AgentControllerProto.js';As per coding guidelines, "All imports should use .js extensions for ESM files".
📝 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 { AgentControllerProto } from './AgentControllerProto.ts'; | |
| import { AgentControllerProto } from './AgentControllerProto.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/plugin/controller/src/lib/AgentControllerObject.ts` at line 19, Update
the local ESM import specifier to use a .js extension: replace the import of
AgentControllerProto in AgentControllerObject (the line importing
AgentControllerProto) so it references './AgentControllerProto.js' instead of
'./AgentControllerProto.ts' to follow the repo's ESM import convention.
| // Create store — user must implement createStore() | ||
| let store: AgentStore; | ||
| const createStoreFn = instance['createStore']; | ||
| if (typeof createStoreFn === 'function') { | ||
| store = (await Reflect.apply(createStoreFn, this._obj, [])) as AgentStore; | ||
| } else { | ||
| throw new Error( | ||
| '@AgentController requires a createStore() method. ' + | ||
| 'Implement createStore() in your controller to provide an AgentStore instance.', | ||
| ); | ||
| } | ||
| if (store.init) { | ||
| await store.init(); | ||
| } | ||
|
|
||
| // Create runtime with AgentRuntime.create factory | ||
| const runtime = AgentRuntime.create({ | ||
| executor: this._obj as AgentExecutor, | ||
| store, | ||
| logger: AgentControllerObject.logger, | ||
| }); |
There was a problem hiding this comment.
Destroy the store if runtime installation fails before ownership transfers.
createStore() / store.init() can allocate handles, but the only init-failure cleanup path goes through this.runtime.destroy(). If store.init() or AgentRuntime.create() throws before this.runtime is assigned, the store never gets destroy() and its resources leak.
🛠️ Suggested fix
- let store: AgentStore;
+ let store: AgentStore;
const createStoreFn = instance['createStore'];
if (typeof createStoreFn === 'function') {
store = (await Reflect.apply(createStoreFn, this._obj, [])) as AgentStore;
} else {
throw new Error(
@@
- if (store.init) {
- await store.init();
- }
-
- // Create runtime with AgentRuntime.create factory
- const runtime = AgentRuntime.create({
- executor: this._obj as AgentExecutor,
- store,
- logger: AgentControllerObject.logger,
- });
+ try {
+ await store.init?.();
+ } catch (err) {
+ await store.destroy?.();
+ throw err;
+ }
+
+ let runtime: AgentRuntime;
+ try {
+ runtime = AgentRuntime.create({
+ executor: this._obj as AgentExecutor,
+ store,
+ logger: AgentControllerObject.logger,
+ });
+ } catch (err) {
+ await store.destroy?.();
+ throw err;
+ }
this.runtime = runtime;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/plugin/controller/src/lib/AgentControllerObject.ts` around lines 205 -
225, The store created by createStoreFn and initialized via store.init may leak
if AgentRuntime.create throws before this.runtime is assigned; wrap the store
initialization and AgentRuntime.create call in a try/catch/finally such that on
any error prior to assigning this.runtime you call store.destroy() (or await
store.destroy() if async) to clean up resources, rethrow the error, and only set
this.runtime = runtime after successful creation; reference createStoreFn,
store.init, AgentRuntime.create, this.runtime and store.destroy in your changes.
…ymbols Add missing AGENT_CONTROLLER_PROTO_IMPL_TYPE and CONTROLLER_AGENT_* symbols to the exports stability snapshot, fixing CI shard 3/3 failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mutation Replace loose index signature with explicit fields (role, status, content, runId, threadId) for type safety. Add MessageConverter.completeMessage() factory method to avoid mutating message objects in AgentRuntime streaming, aligning with the immutable pattern already used by RunBuilder. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add unit tests to cover previously untested code paths: - AgentControllerProto: constructor delegation, symbol property copying, all getter/method delegations, and createProto static factory - AgentInfoUtil: setEnhanced/isEnhanced methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tegg/core/agent-runtime/test/AgentRuntime.test.ts (1)
322-358: Assert thatthread.message.createdstaysin_progress.This test only checks event names. The new
completeMessage()path exists to prevent theThreadMessageCreatedpayload from being rewritten tocompletedby reference, andMockSSEWriteris exactly the harness that can catch that regression. Please assert that the created event still hasstatus === in_progressandcontent === []after the completed event is emitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/AgentRuntime.test.ts` around lines 322 - 358, The test is missing assertions that the ThreadMessageCreated event's payload remains status === ThreadMessageStatus.InProgress and content === [] after the ThreadMessageCompleted event; using MockSSEWriter, find the ThreadMessageCreated event in writer.events (event === AgentSSEEvent.ThreadMessageCreated), capture its data before/after the completion sequence and assert its status is InProgress and its content is an empty array (use the same event object from writer.events to catch reference mutations introduced by completeMessage()). Ensure you reference AgentSSEEvent.ThreadMessageCreated, AgentSSEEvent.ThreadMessageCompleted, MockSSEWriter, and the completeMessage() path when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/agent-runtime/src/AgentRuntime.ts`:
- Around line 303-324: The code currently mutates the terminal builder rb to
completed before an awaited update and then reuses rb in the error path
(rb.cancelling() / rb.fail()), which can cause InvalidRunStateTransitionError
and emit a completed snapshot; instead, after any failed await to
this.store.updateRun(run.id, rb.complete()/...), rehydrate the authoritative run
state from the store (e.g., fetch the persisted run record for run.id) and
construct a new RunBuilder from that persisted record before calling
cancelling() or fail(); then use that fresh builder to call
this.store.updateRun(...) and
writer.writeEvent(AgentSSEEvent.ThreadRunCancelled/ThreadRunFailed, ...) so you
never call cancelling()/fail() on a builder already transitioned to completed.
---
Nitpick comments:
In `@tegg/core/agent-runtime/test/AgentRuntime.test.ts`:
- Around line 322-358: The test is missing assertions that the
ThreadMessageCreated event's payload remains status ===
ThreadMessageStatus.InProgress and content === [] after the
ThreadMessageCompleted event; using MockSSEWriter, find the ThreadMessageCreated
event in writer.events (event === AgentSSEEvent.ThreadMessageCreated), capture
its data before/after the completion sequence and assert its status is
InProgress and its content is an empty array (use the same event object from
writer.events to catch reference mutations introduced by completeMessage()).
Ensure you reference AgentSSEEvent.ThreadMessageCreated,
AgentSSEEvent.ThreadMessageCompleted, MockSSEWriter, and the completeMessage()
path when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e136a122-362c-4338-bd21-b2b265a981cb
⛔ Files ignored due to path filters (1)
tegg/core/types/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
tegg/core/agent-runtime/src/AgentRuntime.tstegg/core/agent-runtime/src/MessageConverter.tstegg/core/agent-runtime/test/AgentRuntime.test.tstegg/core/agent-runtime/test/MessageConverter.test.tstegg/core/agent-runtime/test/RunBuilder.test.tstegg/core/types/src/agent-runtime/AgentStore.ts
| if (abortController.signal.aborted) { | ||
| // Client disconnected or cancelRun fired — mark as cancelled, not failed | ||
| rb.cancelling(); | ||
| try { | ||
| await this.store.updateRun(run.id, rb.cancel()); | ||
| } catch (storeErr) { | ||
| this.logger.error('[AgentRuntime] failed to write cancelled status during stream error:', storeErr); | ||
| } | ||
| if (!writer.closed) { | ||
| writer.writeEvent(AgentSSEEvent.ThreadRunCancelled, rb.snapshot()); | ||
| } | ||
| } else { | ||
| try { | ||
| await this.store.updateRun(run.id, rb.fail(err as Error)); | ||
| } catch (storeErr) { | ||
| this.logger.error('[AgentRuntime] failed to update run status after error:', storeErr); | ||
| } | ||
|
|
||
| // event: thread.run.failed | ||
| if (!writer.closed) { | ||
| writer.writeEvent(AgentSSEEvent.ThreadRunFailed, rb.snapshot()); | ||
| // event: thread.run.failed | ||
| if (!writer.closed) { | ||
| writer.writeEvent(AgentSSEEvent.ThreadRunFailed, rb.snapshot()); | ||
| } |
There was a problem hiding this comment.
Don't reuse a post-complete() builder in the error path.
Line 298 mutates rb to completed before the awaited store write. If that write or Line 301 throws, this catch block reuses a terminal builder, so rb.cancelling()/rb.fail() can throw InvalidRunStateTransitionError, and ThreadRunFailed may be emitted with a completed snapshot. Rehydrate from the persisted run record before deciding whether this should become cancelled or failed.
Suggested direction
} catch (err: unknown) {
+ const latest = await this.store.getRun(run.id).catch(() => undefined);
+ const errorRb = latest ? RunBuilder.fromRecord(latest) : rb;
+
if (abortController.signal.aborted) {
// Client disconnected or cancelRun fired — mark as cancelled, not failed
- rb.cancelling();
+ if (AgentRuntime.TERMINAL_RUN_STATUSES.has(errorRb.snapshot().status)) {
+ return;
+ }
+ errorRb.cancelling();
try {
- await this.store.updateRun(run.id, rb.cancel());
+ await this.store.updateRun(run.id, errorRb.cancel());
} catch (storeErr) {
this.logger.error('[AgentRuntime] failed to write cancelled status during stream error:', storeErr);
}
if (!writer.closed) {
- writer.writeEvent(AgentSSEEvent.ThreadRunCancelled, rb.snapshot());
+ writer.writeEvent(AgentSSEEvent.ThreadRunCancelled, errorRb.snapshot());
}
} else {
try {
- await this.store.updateRun(run.id, rb.fail(err as Error));
+ if (!AgentRuntime.TERMINAL_RUN_STATUSES.has(errorRb.snapshot().status)) {
+ await this.store.updateRun(run.id, errorRb.fail(err as Error));
+ }
} catch (storeErr) {
this.logger.error('[AgentRuntime] failed to update run status after error:', storeErr);
}
// event: thread.run.failed
if (!writer.closed) {
- writer.writeEvent(AgentSSEEvent.ThreadRunFailed, rb.snapshot());
+ writer.writeEvent(AgentSSEEvent.ThreadRunFailed, errorRb.snapshot());
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/AgentRuntime.ts` around lines 303 - 324, The code
currently mutates the terminal builder rb to completed before an awaited update
and then reuses rb in the error path (rb.cancelling() / rb.fail()), which can
cause InvalidRunStateTransitionError and emit a completed snapshot; instead,
after any failed await to this.store.updateRun(run.id, rb.complete()/...),
rehydrate the authoritative run state from the store (e.g., fetch the persisted
run record for run.id) and construct a new RunBuilder from that persisted record
before calling cancelling() or fail(); then use that fresh builder to call
this.store.updateRun(...) and
writer.writeEvent(AgentSSEEvent.ThreadRunCancelled/ThreadRunFailed, ...) so you
never call cancelling()/fail() on a builder already transitioned to completed.
"Node" prefix is redundant in a Node.js framework — the class actually adapts the HTTP ServerResponse for SSE, so "Http" is more precise. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move ContentBlockType, InputContentPart, TextContentBlock, MessageContentBlock, InputMessage, and MessageObject into a dedicated AgentMessage.ts file to eliminate the circular dependency between AgentStore.ts and AgentRuntime.ts (previously worked around with an inline import() type). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The runtime already throws if createStore is missing. Align the interface with the actual behavior so implementors get a compile-time error instead of a runtime surprise. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous commit made createStore required in AgentHandler interface but missed updating this test fixture, causing typecheck CI failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
@AgentController()decorator that auto-registers 7 OpenAI-compatible HTTP routes under/api/v1AgentHandlerinterface,AgentInfoUtilmetadata utilities, andAgentControllerProto/AgentControllerObjectfor full IoC lifecycle integrationNodeSSEWriterimplementation with lazy header writing and lowercase HTTP headerscreateStore()method in@AgentController(no default store — users must provide their ownAgentStore)AgentRuntime.create()withAgentExecutorinterface (replaces removedcreateAgentRuntime+AgentControllerHost)AGENT_CONTROLLER_PROTO_IMPL_TYPEand metadata symbols in@eggjs/tegg-typesapp.ts)@eggjs/tegg/agentpublic API entry point for re-exportsStacked PRs
This is part of a 3-PR series (review in order):
Changes from previous PR (#5820)
AgentHandler.tsandAgentFooController.tsnow import types from@eggjs/tegg-types/agent-runtimeinstead of non-existentmodel/AgentControllerTypes.tsNodeSSEWriter: New implementation in@eggjs/agent-runtimewith lazy header writing, lowercase HTTP headers (content-type), and singleres.write()per eventcreateStore(): RemovedFileAgentStorefallback —@AgentControllernow requires users to implementcreateStore()AgentRuntime.create(): ReplacedcreateAgentRuntime({ host })withAgentRuntime.create({ executor })to match PR2's APIagent.tsexports: RemovedFileAgentStore, addedNodeSSEWriter, types now re-exported from@eggjs/agent-runtime@eggjs/controller-decoratorexports (they live in@eggjs/tegg-types/agent-runtime)Test plan
tsc --noEmittype checking passes for all affected packages🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores