feat(tegg): add AgentController for building AI agent HTTP APIs#5812
feat(tegg): add AgentController for building AI agent HTTP APIs#5812jerryliang64 wants to merge 12 commits intoeggjs:nextfrom
Conversation
Introduce a new @AgentController decorator and runtime that provides OpenAI Assistants API-compatible HTTP endpoints for building conversational AI applications within the Tegg framework. Key additions: - @AgentController decorator auto-generates 7 HTTP routes (threads, runs, streaming) - AgentHandler interface with smart defaults — only execRun() is required - @eggjs/agent-runtime package with FileAgentStore and pluggable storage abstraction - SSE streaming support, async/sync execution modes, and cancellation via AbortSignal - Unified exports via @eggjs/tegg/agent entry point - Full test coverage: unit tests, integration tests for both manual and default modes Co-Authored-By: Claude <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 a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as "Agent\nController"
participant Runtime as "AgentRuntime"
participant Host as "AgentControllerHost"
participant Store as "AgentStore"
Client->>Controller: POST /api/v1/runs/wait (syncRun)
Controller->>Runtime: syncRun(input)
Runtime->>Store: getThread(threadId) / createThread()
Store-->>Runtime: ThreadRecord
Runtime->>Host: execRun(input)
Host-->>Runtime: AsyncGenerator of AgentStreamMessage
Runtime->>Store: appendMessages(...) / updateRun(status=completed)
Runtime-->>Controller: RunObject
Controller-->>Client: 200 {RunObject}
sequenceDiagram
participant Client
participant Controller as "Agent\nController"
participant Runtime as "AgentRuntime"
participant Host as "AgentControllerHost"
participant Store as "AgentStore"
Client->>Controller: POST /api/v1/runs/stream (streamRun)
Controller->>Runtime: streamRun(input)
Runtime->>Store: getThread() / createThread()
Runtime->>Controller: start SSE stream
Runtime->>Host: execRun(input, signal)
Host-->>Runtime: AsyncGenerator events
loop per message
Runtime->>Controller: SSE: thread.message.created
Runtime->>Controller: SSE: thread.message.delta
Runtime->>Controller: SSE: thread.message.completed
end
Runtime->>Store: appendMessages() / updateRun(status=completed)
Runtime->>Controller: SSE: thread.run.completed / done
Controller-->>Client: stream end
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)
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 introduces a powerful new feature for building AI agent HTTP APIs with minimal effort. By leveraging a new 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
|
![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 an excellent new feature, @AgentController, for building AI agent APIs compatible with the OpenAI Assistants API. The implementation is comprehensive, adding a new @eggjs/agent-runtime package with a default file-based store and smart default implementations for all the necessary agent routes. This significantly simplifies the process for developers, who only need to implement the execRun method to get a fully functional agent. The code is well-structured, robustly handling async operations, streaming, and cancellation, and is accompanied by a thorough set of unit and integration tests.
I have two suggestions for improvement: one critical issue regarding an invalid Node.js engine version in the new package that would break installation, and one medium-severity suggestion to improve the type safety and developer experience for providing a custom agent store.
| "typescript": "catalog:" | ||
| }, | ||
| "engines": { | ||
| "node": ">=22.18.0" |
There was a problem hiding this comment.
The specified Node.js engine version >=22.18.0 appears to be a typo, as this version has not been released. This will prevent users from installing the package. Please use a valid version range, such as >=22.0.0.
| "node": ">=22.18.0" | |
| "node": ">=22.0.0" |
| // 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.
The return type Promise<unknown> for createStore is not type-safe and lacks clarity for developers implementing this interface. The framework expects this method to return an object conforming to the AgentStore interface from @eggjs/agent-runtime. To improve type safety and developer experience, please add a TSDoc comment clarifying the contract and consider using any.
| createStore?(): Promise<unknown>; | |
| /** | |
| * Optional method to create a custom agent store. | |
| * The returned object must conform to the `AgentStore` interface from `@eggjs/agent-runtime`. | |
| */ | |
| createStore?(): Promise<any>; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
tegg/core/agent-runtime/tsconfig.json (1)
1-3: Confirm this tsconfig follows the new core-package baseline.This currently extends the repo root config; please verify it satisfies the core-package rule to extend
@eggjs/tsconfigand keepsemitDecoratorMetadataenabled for DI type inference.As per coding guidelines,
tegg/core/*/tsconfig.jsonshould extend@eggjs/tsconfigandtegg/**/tsconfig.jsonshould useemitDecoratorMetadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/tsconfig.json` around lines 1 - 3, The tsconfig in tegg/core/agent-runtime currently extends the repo root and may not enable decorator metadata; update this file so it extends "@eggjs/tsconfig" (replace the existing "extends" value) and add/ensure a "compilerOptions" object contains "emitDecoratorMetadata": true to satisfy the core-package baseline and DI type inference; target the "extends" property and the "compilerOptions.emitDecoratorMetadata" setting in tegg/core/agent-runtime/tsconfig.json when making the change.tegg/core/agent-runtime/test/FileAgentStore.test.ts (1)
52-54: Consider adding tests for path traversal prevention.The
FileAgentStore.safePath()method (as seen in the relevant snippets) includes validation to prevent path traversal attacks. Consider adding tests to verify this security boundary:🛡️ Suggested additional test cases
it('should reject path traversal attempts in thread id', async () => { await assert.rejects( () => store.getThread('../../../etc/passwd'), /Invalid id/ ); await assert.rejects( () => store.getThread('thread_../foo'), /Invalid id/ ); }); it('should reject empty id', async () => { await assert.rejects( () => store.getThread(''), /Invalid id: id must not be empty/ ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts` around lines 52 - 54, Add tests to cover FileAgentStore.safePath path-traversal and empty-id validation by calling store.getThread with malicious or empty ids; specifically add cases that assert rejects for ids like '../../../etc/passwd' and 'thread_../foo' expecting /Invalid id/ and for '' expecting /Invalid id: id must not be empty/ to ensure FileAgentStore.safePath and getThread properly reject traversal and empty identifiers.tegg/core/agent-runtime/test/agentDefaults.test.ts (1)
290-291: Consider replacingsetTimeoutwith a more deterministic approach.Using
setTimeout(resolve, 50)introduces timing-dependent test behavior. While this works, it can be flaky on slower CI runners.Consider using a polling approach or a synchronization primitive if available:
♻️ Suggested alternative
- // Let background task start running - await new Promise((resolve) => setTimeout(resolve, 50)); + // Wait for the run to transition to in_progress + let run = await mockInstance.__agentStore.getRun(result.id); + while (run.status === 'queued') { + await new Promise((resolve) => setTimeout(resolve, 10)); + run = await mockInstance.__agentStore.getRun(result.id); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/agentDefaults.test.ts` around lines 290 - 291, Replace the ad-hoc sleep (await new Promise(resolve => setTimeout(resolve, 50))) with a deterministic wait for the background task to reach the expected state: either subscribe/await a completion/published event from the background task, or implement a short polling loop that checks the observable condition (e.g., a flag, queue length, or method on the component under test) with a small delay and an overall timeout; update the test in agentDefaults.test.ts to poll/assert the actual condition instead of sleeping so tests are not timing-dependent.tegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/app/controller/BaseAgentController.ts (1)
4-18: Minor: Timer cleanup on normal completion.The
sleepfunction clears the timer on abort but not on normal completion. While this works (the timer fires and resolves), for completeness in production-quality code you'd typically clear the timer in the resolve path too. However, since this is test fixture code and the timer will resolve naturally, this is acceptable.♻️ Optional: Complete timer cleanup
function sleep(ms: number, signal?: AbortSignal): Promise<void> { return new Promise((resolve, reject) => { + let resolved = false; const timer = setTimeout(resolve, ms); signal?.addEventListener( 'abort', () => { + if (resolved) return; clearTimeout(timer); const err = new Error('Aborted'); err.name = 'AbortError'; reject(err); }, { once: true }, ); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/app/controller/BaseAgentController.ts` around lines 4 - 18, The sleep function (sleep) clears the timeout when aborted but doesn't clear it on normal completion; update the Promise resolve path to call clearTimeout(timer) (and optionally remove the abort listener) before resolving so the timer is cleaned up on success as well, keeping the AbortError behavior unchanged.tegg/core/controller-decorator/src/decorator/agent/AgentController.ts (1)
23-36: Add explicit return type forcreateNotImplemented.Per coding guidelines, all exported functions should have explicit return types. While this function is not exported, adding a return type improves clarity.
♻️ Proposed fix
-function createNotImplemented(methodName: string, hasParam: boolean) { +function createNotImplemented(methodName: string, hasParam: boolean): (...args: unknown[]) => Promise<never> { let fn;🤖 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/AgentController.ts` around lines 23 - 36, Add an explicit return type to the createNotImplemented function to match its actual return value (an async function assigned to fn with a Symbol.for('AGENT_NOT_IMPLEMENTED') property), e.g. annotate it to return (... ) => Promise<never> or a suitable Function type; update the signature "function createNotImplemented(methodName: string, hasParam: boolean): /*explicit type*/" while keeping the internal logic and preserving the (fn as any)[Symbol.for('AGENT_NOT_IMPLEMENTED')] assignment.tegg/plugin/controller/test/http/agent.test.ts (1)
112-130: Consider extracting SSE parsing to a helper.The SSE parsing logic is duplicated conceptually (parsing
\n\n-separated events). Consider extracting this to a reusable helper function if more streaming tests are added.♻️ Optional: Extract SSE parsing helper
function parseSSEEvents(text: string): Array<{ event: string; data: unknown }> { const events: Array<{ event: string; data: unknown }> = []; const rawEvents = text.split('\n\n').filter(Boolean); for (const raw of rawEvents) { const lines = raw.split('\n'); let event = ''; let data = ''; for (const line of lines) { if (line.startsWith('event: ')) event = line.slice(7); if (line.startsWith('data: ')) data = line.slice(6); } if (event && data) { try { events.push({ event, data: JSON.parse(data) }); } catch { events.push({ event, data }); } } } return events; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/http/agent.test.ts` around lines 112 - 130, Extract the duplicated SSE parsing logic into a helper function (e.g., parseSSEEvents(text): Array<{ event: string; data: unknown }>) and replace the inline block in agent.test.ts that builds events from res.text with a single call to that helper; ensure the helper iterates raw events split by '\n\n', parses "event: " and "data: " lines, attempts JSON.parse for data and falls back to raw string on failure, then returns the events array so existing code that used the local events variable continues to work unchanged.tegg/core/agent-runtime/src/enhanceAgentController.ts (1)
45-65: Consider error handling for store initialization failure.If
this.__agentStore.init()throws, the wrappedinit()will propagate the error, which is likely the desired behavior. However, consider whether partial state should be cleaned up on failure (e.g.,__agentStoreis set but__runningTasksis not).♻️ Optional: Defensive initialization ordering
clazz.prototype.init = async function () { // Allow user to provide custom store via createStore() if (typeof this.createStore === 'function') { this.__agentStore = await this.createStore(); } else { const dataDir = process.env.TEGG_AGENT_DATA_DIR || path.join(process.cwd(), '.agent-data'); this.__agentStore = new FileAgentStore({ dataDir }); } + this.__runningTasks = new Map(); + if (this.__agentStore.init) { await (this.__agentStore as AgentStore).init!(); } - this.__runningTasks = new Map(); if (originalInit) { await originalInit.call(this); } };This ensures
__runningTasksis always set even if store init fails, providing consistent state for potential cleanup paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/enhanceAgentController.ts` around lines 45 - 65, The wrapped init (clazz.prototype.init) currently sets this.__agentStore then awaits its init, but if init throws __runningTasks may remain unset; move or ensure initialization of this.__runningTasks before awaiting store init and add a try/catch/finally around awaiting (this.__agentStore as AgentStore).init to either clean up/reset this.__agentStore on failure or guarantee __runningTasks is set (e.g., set this.__runningTasks = new Map() before calling init and in catch clear this.__agentStore or rethrow after cleanup), and keep calling originalInit.call(this) only after successful setup; reference the symbols clazz.prototype.init, this.createStore, FileAgentStore, this.__agentStore, this.__runningTasks, and originalInit when making the change.tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts (1)
14-26: Module-level state may cause test pollution.The in-memory stores and counters are defined at module level, meaning state persists across all tests using this fixture. Consider adding a reset mechanism or using instance-level storage to ensure test isolation.
♻️ Proposed approach
// Add a static reset method for test cleanup `@AgentController`() export class AgentTestController implements AgentHandler { private static threads = new Map<...>(); private static runs = new Map<...>(); private static threadCounter = 0; private static runCounter = 0; static reset(): void { this.threads.clear(); this.runs.clear(); this.threadCounter = 0; this.runCounter = 0; } // ... use AgentTestController.threads instead of threads }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts` around lines 14 - 26, Module-level in-memory state (threads, runs, threadCounter, runCounter) can leak between tests; convert these to static members on AgentTestController and add a public static reset() method that clears the maps and resets counters (e.g., AgentTestController.threads.clear(), AgentTestController.runs.clear(), set threadCounter/runCounter to 0) and update methods in the class to reference the static fields instead of the module-level variables so tests can call AgentTestController.reset() for isolation.tegg/core/agent-runtime/src/FileAgentStore.ts (1)
100-104:updateRunhas same read-modify-write race asappendMessages.Concurrent updates to the same run could result in lost updates. While
appendMessagesdocuments this limitation,updateRundoes not. Consider adding a similar note for consistency.📝 Add documentation comment
+ // Note: read-modify-write without locking. Concurrent updates to the same run may lose changes. + // This is acceptable for a default file-based store; production stores should implement proper locking. async updateRun(runId: string, updates: Partial<RunRecord>): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/FileAgentStore.ts` around lines 100 - 104, The updateRun method performs a read-modify-write on a RunRecord (via getRun, Object.assign, and writeFile to safePath(this.runsDir, runId)) and therefore has the same concurrent write race as appendMessages; add a brief documentation comment above updateRun mirroring appendMessages that documents the read-modify-write race condition and instructs callers to serialize updates (or use an external locking strategy / switch to atomic/merge semantics) to avoid lost updates.tegg/plugin/controller/test/http/base-agent.test.ts (3)
179-185: Consider polling instead of fixed delay for async run completion.Using a fixed 500ms delay is fragile—tests may fail on slower CI environments or waste time when runs complete faster. A polling approach with timeout would be more robust.
♻️ Example polling approach
- // Wait for background task - await new Promise((resolve) => setTimeout(resolve, 500)); - - // Verify persisted via getRun - const getRes = await app.httpRequest().get(`/api/v1/runs/${res.body.id}`).expect(200); + // Poll for completion with timeout + let getRes; + const deadline = Date.now() + 5000; + while (Date.now() < deadline) { + getRes = await app.httpRequest().get(`/api/v1/runs/${res.body.id}`).expect(200); + if (getRes.body.status !== 'queued') break; + await new Promise((resolve) => setTimeout(resolve, 50)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/http/base-agent.test.ts` around lines 179 - 185, Replace the fragile fixed 500ms delay with a polling loop that repeatedly calls app.httpRequest().get(`/api/v1/runs/${res.body.id}`) until the expected condition (e.g., getRes.body.metadata matching meta or a completed run status) is observed or a timeout is reached; implement the loop in the test (base-agent.test.ts) using a short interval (e.g., 100–200ms) and a max timeout (e.g., a few seconds), await each GET, break when assert.deepEqual(getRes.body.metadata, meta) would succeed, and throw a clear error if the timeout elapses so the test fails deterministically instead of relying on setTimeout.
222-240: Extract SSE parsing helper to reduce duplication.The SSE parsing logic is duplicated between tests. Extracting it to a helper function would improve maintainability.
♻️ Proposed helper extraction
// Add to test/utils.ts or at the top of this file function parseSSEEvents(text: string): { event: string; data: any }[] { const events: { event: string; data: any }[] = []; const rawEvents = text.split('\n\n').filter(Boolean); for (const raw of rawEvents) { const lines = raw.split('\n'); let event = ''; let data = ''; for (const line of lines) { if (line.startsWith('event: ')) event = line.slice(7); if (line.startsWith('data: ')) data = line.slice(6); } if (event && data) { try { events.push({ event, data: JSON.parse(data) }); } catch { events.push({ event, data }); } } } return events; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/http/base-agent.test.ts` around lines 222 - 240, Extract the SSE parsing block into a reusable helper named parseSSEEvents(text: string): { event: string; data: any }[] (place it in a shared test utils file or at the top of the test file), then replace the duplicated parsing loop in base-agent.test.ts with a call to parseSSEEvents(res.text). Ensure the helper handles JSON.parse errors the same way as the original loop and export/import the helper so other tests that duplicate this logic can use it as well.
58-60: Consider using 404 status code for "not found" errors.Returning HTTP 500 for a non-existent thread conflates server errors with client errors. A 404 status would be more RESTful and help clients distinguish between missing resources and actual failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/http/base-agent.test.ts` around lines 58 - 60, Update the test named "should return 500 for non-existent thread" to expect a 404 status instead of 500 and ensure the route handler for GET /api/v1/threads/:id returns a 404 when the thread is not found; specifically change the assertion in the test to .expect(404) and modify the controller/handler (the GET /api/v1/threads/:id route or the method that looks up a thread) to throw or send a NotFound response (e.g., throw a NotFoundError or set ctx.status = 404 and a helpful message) when the lookup returns null so the test passes and missing resources return RESTful 404s.tegg/core/agent-runtime/src/agentDefaults.ts (1)
275-280: Missing explicit return type annotation.Per coding guidelines, exported functions should have explicit return types. The factory function returns
async function(...)but lacks a return type annotation.♻️ Proposed fix
function defaultStreamRun() { - return async function (this: AgentInstance, input: CreateRunInput): Promise<void> { + return async function (this: AgentInstance, input: CreateRunInput): Promise<void> {The inner function already has
Promise<void>, but the outer factory could be typed:-function defaultStreamRun() { +function defaultStreamRun(): (this: AgentInstance, input: CreateRunInput) => Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/agentDefaults.ts` around lines 275 - 280, Add an explicit return type to the factory function defaultStreamRun: declare that defaultStreamRun returns a function whose this is AgentInstance, which accepts a CreateRunInput and returns a Promise<void>; update the signature of defaultStreamRun to include that return type so the exported function has an explicit type rather than relying on inference.
🤖 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/agentDefaults.ts`:
- Around line 117-121: The mapping of message parts in agentDefaults.ts assumes
each content item with type 'text' has a .text property, which can be undefined
and lead to { value: undefined }; update the transform that maps m.content (the
branch using m.content.map((p) => ({ type: 'text' as const, text: { value:
p.text, annotations: [] } }))) to defensively handle missing p.text by either
skipping parts without p.text or substituting a safe default (e.g., empty
string) and ensure annotations is present; locate the m.content.map call and add
a guard that checks p && p.text before using p.text, returning a valid { type:
'text', text: { value: <safe string>, annotations: [] } } or filtering the part
out.
In `@tegg/core/agent-runtime/src/FileAgentStore.ts`:
- Around line 24-33: The current safePath method can be bypassed by crafted ids;
change it to resolve and validate paths and restrict id characters: resolve
baseDir with path.resolve, ensure the resulting file path is
path.resolve(baseDir, `${id}.json`), and check that the resolved file path
starts with resolvedBaseDir + path.sep; additionally enforce a strict ID format
(e.g., allow only alphanumerics, dash, underscore) and reject any id containing
path separators or null bytes before constructing the path to prevent traversal
(update safePath to perform these checks and throw on invalid ids).
In `@tegg/core/controller-decorator/src/decorator/agent/AgentController.ts`:
- Around line 106-108: Add an inline comment at the call site in AgentController
where PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false,
5)) is invoked explaining why the depth argument is 5: list the expected stack
frames (AgentController → SingletonProto → Prototype → StackUtil) so readers
know this magic number accounts for decorator nesting and that changing
decorator structure requires adjusting the depth; reference the symbols
PrototypeUtil.setFilePath, StackUtil.getCalleeFromStack, AgentController,
SingletonProto, and Prototype in the comment for clarity.
In `@tegg/core/controller-decorator/src/model/AgentControllerTypes.ts`:
- Line 50: RunStatus currently defines a 'cancelling' variant that is never
used; remove 'cancelling' from the union type declaration in the
AgentControllerTypes RunStatus export (symbol: RunStatus) unless you intend to
implement a transitional cancellation state—if you do implement it, add code
paths that set status to 'cancelling' (e.g., in cancellation initiation logic
and in whatever worker/cleanup path completes cancellation) and update any
status checks and tests to handle the transitional state instead of jumping
directly to 'cancelled'.
In `@tegg/core/controller-decorator/test/AgentController.test.ts`:
- Around line 13-14: Update the ESM relative imports to use .js extensions:
replace occurrences of '../src/index.ts' and '../src/model/index.ts' in the
import statements in AgentController.test.ts with '../src/index.js' and
'../src/model/index.js' respectively so the imports that bring in the test
symbols (the module exporting the test helpers and the HTTPControllerMeta
import) follow the tegg ESM .js extension convention.
- Line 25: The test currently uses `as any` to access symbol and dynamic
properties—replace those casts with explicit, narrow types: define an interface
(e.g., IAgentControllerShape) that includes [Symbol.for('AGENT_CONTROLLER')]:
boolean and assert `(AgentFooController as unknown as
IAgentControllerShape)[Symbol.for('AGENT_CONTROLLER')]`; for the prototype
access replace `as any` with a typed Record or interface matching the prototype
shape (e.g., Record<string, unknown> or IAgentPrototype) and assert via `as
unknown as IAgentPrototype`; for the instance with dynamic properties define a
concrete type (e.g., IAgentInstance { /* dynamic props */ }) and cast via `as
unknown as IAgentInstance`; use unknown-to-interface assertions instead of `any`
for the referenced symbols and objects (AgentFooController, the prototype, and
the instance).
In
`@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts`:
- Around line 189-200: The cancelRun function currently returns a "cancelled"
RunObject even when the run is missing (runs.get(runId) is undefined); instead,
mirror the behavior of getRun by detecting a missing run and throwing the same
error (or returning the same error pattern) rather than returning a successful
cancellation. Locate cancelRun (and reference runs.get and nowUnix) and change
it to throw the missing-run error when run is undefined; only set run.status =
'cancelled' and return the cancelled RunObject when the run exists.
In `@tegg/plugin/controller/test/http/base-agent.test.ts`:
- Around line 39-41: The test incorrectly compares res.body.created_at (Unix
seconds) to Date.now() (milliseconds); change the assertion to compare like
units—either assert(res.body.created_at * 1000 < Date.now()) or
assert(res.body.created_at < Math.floor(Date.now() / 1000))—so the created_at
timestamp is validated correctly; update the two related assertions around
res.body.created_at accordingly.
---
Nitpick comments:
In `@tegg/core/agent-runtime/src/agentDefaults.ts`:
- Around line 275-280: Add an explicit return type to the factory function
defaultStreamRun: declare that defaultStreamRun returns a function whose this is
AgentInstance, which accepts a CreateRunInput and returns a Promise<void>;
update the signature of defaultStreamRun to include that return type so the
exported function has an explicit type rather than relying on inference.
In `@tegg/core/agent-runtime/src/enhanceAgentController.ts`:
- Around line 45-65: The wrapped init (clazz.prototype.init) currently sets
this.__agentStore then awaits its init, but if init throws __runningTasks may
remain unset; move or ensure initialization of this.__runningTasks before
awaiting store init and add a try/catch/finally around awaiting
(this.__agentStore as AgentStore).init to either clean up/reset
this.__agentStore on failure or guarantee __runningTasks is set (e.g., set
this.__runningTasks = new Map() before calling init and in catch clear
this.__agentStore or rethrow after cleanup), and keep calling
originalInit.call(this) only after successful setup; reference the symbols
clazz.prototype.init, this.createStore, FileAgentStore, this.__agentStore,
this.__runningTasks, and originalInit when making the change.
In `@tegg/core/agent-runtime/src/FileAgentStore.ts`:
- Around line 100-104: The updateRun method performs a read-modify-write on a
RunRecord (via getRun, Object.assign, and writeFile to safePath(this.runsDir,
runId)) and therefore has the same concurrent write race as appendMessages; add
a brief documentation comment above updateRun mirroring appendMessages that
documents the read-modify-write race condition and instructs callers to
serialize updates (or use an external locking strategy / switch to atomic/merge
semantics) to avoid lost updates.
In `@tegg/core/agent-runtime/test/agentDefaults.test.ts`:
- Around line 290-291: Replace the ad-hoc sleep (await new Promise(resolve =>
setTimeout(resolve, 50))) with a deterministic wait for the background task to
reach the expected state: either subscribe/await a completion/published event
from the background task, or implement a short polling loop that checks the
observable condition (e.g., a flag, queue length, or method on the component
under test) with a small delay and an overall timeout; update the test in
agentDefaults.test.ts to poll/assert the actual condition instead of sleeping so
tests are not timing-dependent.
In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts`:
- Around line 52-54: Add tests to cover FileAgentStore.safePath path-traversal
and empty-id validation by calling store.getThread with malicious or empty ids;
specifically add cases that assert rejects for ids like '../../../etc/passwd'
and 'thread_../foo' expecting /Invalid id/ and for '' expecting /Invalid id: id
must not be empty/ to ensure FileAgentStore.safePath and getThread properly
reject traversal and empty identifiers.
In `@tegg/core/agent-runtime/tsconfig.json`:
- Around line 1-3: The tsconfig in tegg/core/agent-runtime currently extends the
repo root and may not enable decorator metadata; update this file so it extends
"@eggjs/tsconfig" (replace the existing "extends" value) and add/ensure a
"compilerOptions" object contains "emitDecoratorMetadata": true to satisfy the
core-package baseline and DI type inference; target the "extends" property and
the "compilerOptions.emitDecoratorMetadata" setting in
tegg/core/agent-runtime/tsconfig.json when making the change.
In `@tegg/core/controller-decorator/src/decorator/agent/AgentController.ts`:
- Around line 23-36: Add an explicit return type to the createNotImplemented
function to match its actual return value (an async function assigned to fn with
a Symbol.for('AGENT_NOT_IMPLEMENTED') property), e.g. annotate it to return (...
) => Promise<never> or a suitable Function type; update the signature "function
createNotImplemented(methodName: string, hasParam: boolean): /*explicit type*/"
while keeping the internal logic and preserving the (fn as
any)[Symbol.for('AGENT_NOT_IMPLEMENTED')] assignment.
In
`@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts`:
- Around line 14-26: Module-level in-memory state (threads, runs, threadCounter,
runCounter) can leak between tests; convert these to static members on
AgentTestController and add a public static reset() method that clears the maps
and resets counters (e.g., AgentTestController.threads.clear(),
AgentTestController.runs.clear(), set threadCounter/runCounter to 0) and update
methods in the class to reference the static fields instead of the module-level
variables so tests can call AgentTestController.reset() for isolation.
In
`@tegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/app/controller/BaseAgentController.ts`:
- Around line 4-18: The sleep function (sleep) clears the timeout when aborted
but doesn't clear it on normal completion; update the Promise resolve path to
call clearTimeout(timer) (and optionally remove the abort listener) before
resolving so the timer is cleaned up on success as well, keeping the AbortError
behavior unchanged.
In `@tegg/plugin/controller/test/http/agent.test.ts`:
- Around line 112-130: Extract the duplicated SSE parsing logic into a helper
function (e.g., parseSSEEvents(text): Array<{ event: string; data: unknown }>)
and replace the inline block in agent.test.ts that builds events from res.text
with a single call to that helper; ensure the helper iterates raw events split
by '\n\n', parses "event: " and "data: " lines, attempts JSON.parse for data and
falls back to raw string on failure, then returns the events array so existing
code that used the local events variable continues to work unchanged.
In `@tegg/plugin/controller/test/http/base-agent.test.ts`:
- Around line 179-185: Replace the fragile fixed 500ms delay with a polling loop
that repeatedly calls app.httpRequest().get(`/api/v1/runs/${res.body.id}`) until
the expected condition (e.g., getRes.body.metadata matching meta or a completed
run status) is observed or a timeout is reached; implement the loop in the test
(base-agent.test.ts) using a short interval (e.g., 100–200ms) and a max timeout
(e.g., a few seconds), await each GET, break when
assert.deepEqual(getRes.body.metadata, meta) would succeed, and throw a clear
error if the timeout elapses so the test fails deterministically instead of
relying on setTimeout.
- Around line 222-240: Extract the SSE parsing block into a reusable helper
named parseSSEEvents(text: string): { event: string; data: any }[] (place it in
a shared test utils file or at the top of the test file), then replace the
duplicated parsing loop in base-agent.test.ts with a call to
parseSSEEvents(res.text). Ensure the helper handles JSON.parse errors the same
way as the original loop and export/import the helper so other tests that
duplicate this logic can use it as well.
- Around line 58-60: Update the test named "should return 500 for non-existent
thread" to expect a 404 status instead of 500 and ensure the route handler for
GET /api/v1/threads/:id returns a 404 when the thread is not found; specifically
change the assertion in the test to .expect(404) and modify the
controller/handler (the GET /api/v1/threads/:id route or the method that looks
up a thread) to throw or send a NotFound response (e.g., throw a NotFoundError
or set ctx.status = 404 and a helpful message) when the lookup returns null so
the test passes and missing resources return RESTful 404s.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
tegg/core/agent-runtime/package.jsontegg/core/agent-runtime/src/AgentStore.tstegg/core/agent-runtime/src/FileAgentStore.tstegg/core/agent-runtime/src/agentDefaults.tstegg/core/agent-runtime/src/enhanceAgentController.tstegg/core/agent-runtime/src/index.tstegg/core/agent-runtime/test/FileAgentStore.test.tstegg/core/agent-runtime/test/agentDefaults.test.tstegg/core/agent-runtime/test/enhanceAgentController.test.tstegg/core/agent-runtime/tsconfig.jsontegg/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/model/AgentControllerTypes.tstegg/core/controller-decorator/src/model/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/plugin/controller/package.jsontegg/plugin/controller/src/lib/EggControllerPrototypeHook.tstegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.tstegg/plugin/controller/test/fixtures/apps/agent-controller-app/config/config.default.tstegg/plugin/controller/test/fixtures/apps/agent-controller-app/config/plugin.tstegg/plugin/controller/test/fixtures/apps/agent-controller-app/package.jsontegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/app/controller/BaseAgentController.tstegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/config/config.default.tstegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/config/plugin.tstegg/plugin/controller/test/fixtures/apps/base-agent-controller-app/package.jsontegg/plugin/controller/test/http/agent.test.tstegg/plugin/controller/test/http/base-agent.test.ts
🔥 Files not summarized due to errors (1)
- tegg/core/controller-decorator/test/AgentController.test.ts: Error: Server error: no LLM provider could handle the message
| content: | ||
| typeof m.content === 'string' | ||
| ? [{ type: 'text' as const, text: { value: m.content, annotations: [] } }] | ||
| : m.content.map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })), | ||
| })); |
There was a problem hiding this comment.
Potential runtime error if content part lacks text property.
The filter checks for type === 'text', but the subsequent map assumes all filtered items have a .text property. If the input message contains malformed content parts where type === 'text' but text is undefined, this will produce { value: undefined }.
🛡️ Proposed defensive check
? [{ type: 'text' as const, text: { value: m.content, annotations: [] } }]
- : m.content.map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })),
+ : m.content
+ .filter((p) => p.type === 'text' && typeof p.text === 'string')
+ .map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })),📝 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.
| content: | |
| typeof m.content === 'string' | |
| ? [{ type: 'text' as const, text: { value: m.content, annotations: [] } }] | |
| : m.content.map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })), | |
| })); | |
| content: | |
| typeof m.content === 'string' | |
| ? [{ type: 'text' as const, text: { value: m.content, annotations: [] } }] | |
| : m.content | |
| .filter((p) => p.type === 'text' && typeof p.text === 'string') | |
| .map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })), | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/agentDefaults.ts` around lines 117 - 121, The
mapping of message parts in agentDefaults.ts assumes each content item with type
'text' has a .text property, which can be undefined and lead to { value:
undefined }; update the transform that maps m.content (the branch using
m.content.map((p) => ({ type: 'text' as const, text: { value: p.text,
annotations: [] } }))) to defensively handle missing p.text by either skipping
parts without p.text or substituting a safe default (e.g., empty string) and
ensure annotations is present; locate the m.content.map call and add a guard
that checks p && p.text before using p.text, returning a valid { type: 'text',
text: { value: <safe string>, annotations: [] } } or filtering the part out.
| private safePath(baseDir: string, id: string): string { | ||
| if (!id) { | ||
| throw new Error('Invalid id: id must not be empty'); | ||
| } | ||
| const filePath = path.join(baseDir, `${id}.json`); | ||
| if (!filePath.startsWith(baseDir + path.sep)) { | ||
| throw new Error(`Invalid id: ${id}`); | ||
| } | ||
| return filePath; | ||
| } |
There was a problem hiding this comment.
Path traversal check may be insufficient for edge cases.
The safePath check could be bypassed in edge cases. For example, if id contains path separators that get normalized differently, or if baseDir doesn't end with a separator. Consider validating the ID format directly.
🛡️ Proposed enhanced validation
private safePath(baseDir: string, id: string): string {
if (!id) {
throw new Error('Invalid id: id must not be empty');
}
+ // Reject IDs containing path separators or traversal sequences
+ if (/[/\\]|\.\./.test(id)) {
+ throw new Error(`Invalid id: ${id}`);
+ }
const filePath = path.join(baseDir, `${id}.json`);
- if (!filePath.startsWith(baseDir + path.sep)) {
+ // Resolve to absolute paths for reliable comparison
+ const resolvedPath = path.resolve(filePath);
+ const resolvedBase = path.resolve(baseDir);
+ if (!resolvedPath.startsWith(resolvedBase + path.sep)) {
throw new Error(`Invalid id: ${id}`);
}
return filePath;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/FileAgentStore.ts` around lines 24 - 33, The
current safePath method can be bypassed by crafted ids; change it to resolve and
validate paths and restrict id characters: resolve baseDir with path.resolve,
ensure the resulting file path is path.resolve(baseDir, `${id}.json`), and check
that the resolved file path starts with resolvedBaseDir + path.sep; additionally
enforce a strict ID format (e.g., allow only alphanumerics, dash, underscore)
and reject any id containing path separators or null bytes before constructing
the path to prevent traversal (update safePath to perform these checks and throw
on invalid ids).
|
|
||
| // ===== Run types ===== | ||
|
|
||
| export type RunStatus = 'queued' | 'in_progress' | 'completed' | 'failed' | 'cancelled' | 'cancelling' | 'expired'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the files mentioned
fd -t f "AgentControllerTypes.ts" --type ts
fd -t f "agentDefaults.ts" --type tsRepository: eggjs/egg
Length of output: 392
🏁 Script executed:
# Search for TERMINAL_RUN_STATUSES definition and usage
rg -n "TERMINAL_RUN_STATUSES" -A 3 -B 1 --type=tsRepository: eggjs/egg
Length of output: 931
🏁 Script executed:
# Search for all usage of 'cancelling' status
rg -n "'cancelling'" --type=ts
rg -n '"cancelling"' --type=tsRepository: eggjs/egg
Length of output: 236
🏁 Script executed:
# Search for RunStatus type definition and any related status constants
rg -n "RunStatus" --type=ts -A 1Repository: eggjs/egg
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Search for where 'cancelling' status is assigned or used
rg -n "cancelling" --type f -A 2 -B 2 | head -100Repository: eggjs/egg
Length of output: 78
🏁 Script executed:
# Search for status transitions/assignments
rg -n "\.status\s*=" -A 1 --type f | grep -C 3 "cancelling"Repository: eggjs/egg
Length of output: 78
🏁 Script executed:
# Check if there's logic that handles the 'cancelling' state specifically
rg -n "case.*cancelling|'cancelling'.*case" --type fRepository: eggjs/egg
Length of output: 78
🏁 Script executed:
# Look for state transition logic
fd -type f -name "*.ts" | xargs rg -l "status.*transition|state.*machine" | head -10Repository: eggjs/egg
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Search for where 'cancelling' status is assigned or used in code
rg "cancelling" -A 2 -B 2 | head -150Repository: eggjs/egg
Length of output: 549
🏁 Script executed:
# Search for status assignment patterns
rg "status\s*[=:]" | grep -i "cancel"Repository: eggjs/egg
Length of output: 689
🏁 Script executed:
# Look for all TypeScript files that reference 'cancelling'
rg -l "cancelling"Repository: eggjs/egg
Length of output: 117
🏁 Script executed:
# Check the structure around TERMINAL_RUN_STATUSES to understand its purpose better
rg -B 5 -A 10 "TERMINAL_RUN_STATUSES"Repository: eggjs/egg
Length of output: 2481
🏁 Script executed:
#!/bin/bash
# Search for TODO or comments related to cancelling or state transitions
rg -i "todo.*cancel|cancel.*todo|cancelling.*todo" -B 2 -A 2
rg -i "todo|fixme" | grep -i "cancel\|transition\|state"Repository: eggjs/egg
Length of output: 148
🏁 Script executed:
# Check git history to see if 'cancelling' was recently added
git log --oneline -n 50 -- tegg/core/controller-decorator/src/model/AgentControllerTypes.tsRepository: eggjs/egg
Length of output: 125
🏁 Script executed:
# Check if there's any test that expects 'cancelling' status
rg "'cancelling'|\"cancelling\"" -B 3 -A 3Repository: eggjs/egg
Length of output: 707
🏁 Script executed:
# Verify the complete implementation of defaultCancelRun to understand the flow
rg -A 30 "function defaultCancelRun"Repository: eggjs/egg
Length of output: 2474
Remove unused 'cancelling' status from RunStatus type or implement the transitional state logic.
RunStatus includes 'cancelling' but it's never set anywhere in the codebase. The cancellation flow directly transitions runs to 'cancelled' status. Either remove 'cancelling' from the type if it's not needed, or implement the intended transitional state logic if it's planned for future use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/controller-decorator/src/model/AgentControllerTypes.ts` at line 50,
RunStatus currently defines a 'cancelling' variant that is never used; remove
'cancelling' from the union type declaration in the AgentControllerTypes
RunStatus export (symbol: RunStatus) unless you intend to implement a
transitional cancellation state—if you do implement it, add code paths that set
status to 'cancelling' (e.g., in cancellation initiation logic and in whatever
worker/cleanup path completes cancellation) and update any status checks and
tests to handle the transitional state instead of jumping directly to
'cancelled'.
| } from '../src/index.ts'; | ||
| import { HTTPControllerMeta } from '../src/model/index.ts'; |
There was a problem hiding this comment.
Use .js extensions in relative ESM imports under tegg/.
These relative imports currently use .ts, which conflicts with the tegg ESM import convention.
Suggested change
-} from '../src/index.ts';
-import { HTTPControllerMeta } from '../src/model/index.ts';
+} from '../src/index.js';
+import { HTTPControllerMeta } from '../src/model/index.js';As per coding guidelines: “tegg/**/*.{ts,tsx}: 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.
| } from '../src/index.ts'; | |
| import { HTTPControllerMeta } from '../src/model/index.ts'; | |
| } from '../src/index.js'; | |
| import { HTTPControllerMeta } from '../src/model/index.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/controller-decorator/test/AgentController.test.ts` around lines 13
- 14, Update the ESM relative imports to use .js extensions: replace occurrences
of '../src/index.ts' and '../src/model/index.ts' in the import statements in
AgentController.test.ts with '../src/index.js' and '../src/model/index.js'
respectively so the imports that bring in the test symbols (the module exporting
the test helpers and the HTTPControllerMeta import) follow the tegg ESM .js
extension convention.
| async cancelRun(runId: string): Promise<RunObject> { | ||
| const run = runs.get(runId); | ||
| if (run) { | ||
| run.status = 'cancelled'; | ||
| } | ||
| return { | ||
| id: runId, | ||
| object: 'thread.run', | ||
| created_at: run?.created_at ?? nowUnix(), | ||
| status: 'cancelled', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Potential null reference in cancelRun.
If the run doesn't exist, run is undefined, but the code still accesses run?.created_at. While this produces undefined which falls back to nowUnix(), returning a successful cancellation response for a non-existent run is misleading—consider throwing an error consistent with getRun.
🐛 Proposed fix
async cancelRun(runId: string): Promise<RunObject> {
const run = runs.get(runId);
+ if (!run) {
+ throw new Error(`Run ${runId} not found`);
+ }
- if (run) {
- run.status = 'cancelled';
- }
+ run.status = 'cancelled';
return {
id: runId,
object: 'thread.run',
- created_at: run?.created_at ?? nowUnix(),
+ created_at: run.created_at,
status: 'cancelled',
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async cancelRun(runId: string): Promise<RunObject> { | |
| const run = runs.get(runId); | |
| if (run) { | |
| run.status = 'cancelled'; | |
| } | |
| return { | |
| id: runId, | |
| object: 'thread.run', | |
| created_at: run?.created_at ?? nowUnix(), | |
| status: 'cancelled', | |
| }; | |
| } | |
| async cancelRun(runId: string): Promise<RunObject> { | |
| const run = runs.get(runId); | |
| if (!run) { | |
| throw new Error(`Run ${runId} not found`); | |
| } | |
| run.status = 'cancelled'; | |
| return { | |
| id: runId, | |
| object: 'thread.run', | |
| created_at: run.created_at, | |
| status: 'cancelled', | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts`
around lines 189 - 200, The cancelRun function currently returns a "cancelled"
RunObject even when the run is missing (runs.get(runId) is undefined); instead,
mirror the behavior of getRun by detecting a missing run and throwing the same
error (or returning the same error pattern) rather than returning a successful
cancellation. Locate cancelRun (and reference runs.get and nowUnix) and change
it to throw the missing-run error when run is undefined; only set run.status =
'cancelled' and return the cancelled RunObject when the run exists.
| __agentStore: AgentStore; | ||
| __runningTasks: Map<string, { promise: Promise<void>; abortController: AbortController }>; |
There was a problem hiding this comment.
已在 9951d13d 中改为 Symbol 属性（AGENT_STORE / RUNNING_TASKS），在 1625d011 中进一步重构为 AgentRuntime 类的私有字段 store / runningTasks，不再暴露在 controller 实例上。
There was a problem hiding this comment.
已在 9951d13d 中改为 Symbol 属性（AGENT_STORE / RUNNING_TASKS），在 1625d011 中进一步重构为 AgentRuntime 类的私有字段 store / runningTasks，不再暴露在 controller 实例上。
| }; | ||
| } | ||
|
|
||
| export const AGENT_DEFAULT_FACTORIES: Record<string, () => Function> = { |
There was a problem hiding this comment.
已在 1625d011 中重构：删除了 agentDefaults.ts 中的 7 个工厂函数，创建了 AgentRuntime 类，store 和 runningTasks 为私有字段，通过 host 引用委托 execRun 调用（对 AOP/mock 友好）。controller 实例上只挂一个 [AGENT_RUNTIME] symbol 指向 runtime。
| // - Stub methods must be marked with Symbol.for('AGENT_NOT_IMPLEMENTED'). | ||
| export function enhanceAgentController(clazz: EggProtoImplClass): void { | ||
| // Only enhance classes marked by @AgentController decorator | ||
| if (!(clazz as any)[Symbol.for('AGENT_CONTROLLER')]) { |
There was a problem hiding this comment.
已在 9951d13d 中修复：创建了 AgentInfoUtil 工具类，用 AgentInfoUtil.isAgentController(clazz) 替代 (clazz as any)[Symbol.for(...)]。
| input, | ||
| config, | ||
| metadata, | ||
| created_at: Math.floor(Date.now() / 1000), |
There was a problem hiding this comment.
已在 9951d13d 中修复：createRun 中的 Math.floor(Date.now() / 1000) 已改为复用 nowUnix() 工具函数。
| private async writeFile(filePath: string, data: unknown): Promise<void> { | ||
| const tmpPath = `${filePath}.${crypto.randomUUID()}.tmp`; | ||
| await fs.writeFile(tmpPath, JSON.stringify(data), 'utf-8'); | ||
| await fs.rename(tmpPath, filePath); |
There was a problem hiding this comment.
这个是考虑原子操作？会有并发写吗，没有的话 fs open 然后用 fd 去写就可以了。现在这样会反复 open
There was a problem hiding this comment.
如果 thread_id 一样，会有并发，这里先简化么？有没有并发，其实主要看client侧怎么用的，目前初期估计不太会碰到
There was a problem hiding this comment.
已在 9951d13d 中简化为直接 fs.writeFile，去掉了 tmp+rename。
关于并发写：分析后发现同一进程内同一 run 的 updateRun 不会并发（cancelRun 会 await task.promise 等后台任务结束后才写）。真正的并发场景是 cluster mode 多 worker 共享 dataDir 时对同一 thread 的 appendMessages，这需要跨进程的文件锁才能解决。当前先保留简单实现并注释说明限制，后续可通过在 FileAgentStore 内加 withLock（基于 Node.js 22+ 的 FileHandle.lock()）扩展，不影响外部接口。
| const content = await fs.readFile(filePath, 'utf-8'); | ||
| return JSON.parse(content); |
There was a problem hiding this comment.
catch 搞小点？下面 ENOENT 是针对 readfile 的。
There was a problem hiding this comment.
已在 9951d13d 中修复：缩小了 try 范围，只包裹 fs.readFile，JSON.parse 移到 catch 外面。
| throw new Error(`${methodName} not implemented`); | ||
| }; | ||
| } | ||
| (fn as any)[Symbol.for('AGENT_NOT_IMPLEMENTED')] = true; |
There was a problem hiding this comment.
已在 9951d13d 中修复：创建了 AgentInfoUtil.setNotImplemented(fn) 替代 (fn as any)[Symbol.for(...)] = true。
| func(constructor); | ||
|
|
||
| // Set file path for prototype | ||
| PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5)); |
There was a problem hiding this comment.
是的，depth 5 是对的。AgentController 与 HTTPController（HTTPController.ts:33）和 MCPController（MCPController.ts:20）使用相同的模式，它们都是 factory decorator + 内部调用 SingletonProto()，所以调用栈结构一致，depth 都是 5。
There was a problem hiding this comment.
这里先这样，我在另一个 PR 里准备把这个模式去掉了，改成用 Loader 的方式去重新注入 filepath
|
Overview This PR introduces an @AgentController decorator that auto-generates 7 OpenAI Assistants API-compatible HTTP endpoints. The design follows a "smart defaults" New package: @eggjs/agent-runtime (tegg/core/agent-runtime) Critical Issues
tegg/core/agent-runtime is NOT added to the root tsconfig.json references array. Per CLAUDE.md: "When adding new packages, add them to root tsconfig.json
The lockfile diff introduces an empty packages/skills importer entry, but no packages/skills directory exists in the repo. This is an unrelated artifact from a
Integration tests expect HTTP 500 for non-existent threads/runs (e.g., agent.test.ts and base-agent.test.ts). These should be 404. Returning 500 for "not Style / Convention Issues
Existing controller-decorator tests consistently use .js extensions: The PR mixes .ts and .js: The tegg CLAUDE.md states: "All packages target ESM with .js extensions in imports." The PR should be consistent with the existing codebase — either all .js or
In AgentTestController.ts fixture:
In the AgentTestController.ts fixture, as any is used to bypass type checking on RunStatus. This suggests the RunStatus type is too narrow for actual usage, Design Issues
The @AgentController() decorator hardcodes the API path prefix to /api/v1. There's no way to customize it. If a user wants /api/v2/ or needs multiple agent
If a user applies @AgentController() but forgets to implement execRun(), all 7 routes will be registered with stub implementations. The error only surfaces at
streamRun in agentDefaults.ts accesses the raw response via Symbol.for('context#eggContext') and writes SSE directly, bypassing Koa's middleware chain. This
In the AgentHandler interface, createStore?(): Promise should return Promise for type safety. Currently a non-conforming object passes
enhanceAgentController.ts defaults to path.join(process.cwd(), '.agent-data'). This is fragile — it depends on where the process was launched, not where the Testing Issues
streamRun is the most complex default implementation (SSE streaming, abort handling, chunked responses) and has zero unit test coverage. It's only tested
base-agent.test.ts uses setTimeout(resolve, 500) to wait for background tasks:
enhanceAgentController.test.ts directly mutates process.env.TEGG_AGENT_DATA_DIR. If a test fails between beforeEach and afterEach, the env var leaks. Use Minor Issues
The type Record<string, () => Function> is very loose. A more specific union type would improve safety.
The 'cancelling' status is defined but never used — cancelRun goes directly to 'cancelled'.
Three different content representations exist:
This is confusing and error-prone. Consolidating to fewer shapes would help.
The comment acknowledges a read-modify-write without locking. Concurrent appends to the same thread may silently lose messages. For a framework-provided
In agentDefaults.ts, defaultAsyncRun's error path: What's Done Well
Summary ┌──────────────────┬────────────────────────────────────────────────────────────┐ The architecture is sound — the "smart defaults" pattern is elegant. However, the PR needs attention on: (1) missing infrastructure setup (tsconfig reference, |
- Remove stale `packages/skills` entry from pnpm-lock.yaml
- Replace hardcoded `Symbol.for('context#eggContext')` with named
`EGG_CONTEXT` constant in agentDefaults and test fixture, with
comment linking to canonical definition in @eggjs/module-common
- Add `AgentNotFoundError` (404) and `AgentConflictError` (409) error
classes so Koa returns proper HTTP status codes instead of 500
- Update FileAgentStore, agentDefaults, and test fixtures to use
the new error classes
- Update integration tests to expect 404 for not-found cases
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
tegg/core/agent-runtime/src/agentDefaults.ts (3)
15-16: Import extensions should use.jsfor ESM compatibility.As per coding guidelines, ESM imports should use
.jsextensions.♻️ Proposed fix
-import type { AgentStore } from './AgentStore.ts'; -import { AgentConflictError } from './errors.ts'; +import type { AgentStore } from './AgentStore.js'; +import { AgentConflictError } from './errors.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/agentDefaults.ts` around lines 15 - 16, Update the import statement extensions in agentDefaults.ts to use .js for ESM compatibility: change the import sources that reference './AgentStore.ts' and './errors.ts' to './AgentStore.js' and './errors.js' respectively so the imported symbols AgentStore and AgentConflictError resolve correctly at runtime in an ESM environment.
200-207: Useunknowninstead ofanyfor caught errors.Multiple catch blocks use
err: any. Per coding guidelines, preferunknownand narrow types explicitly.♻️ Example fix for one catch block
- } catch (err: any) { + } catch (err) { + const message = err instanceof Error ? err.message : String(err); const failedAt = nowUnix(); await this.__agentStore.updateRun(run.id, { status: 'failed', - last_error: { code: 'EXEC_ERROR', message: err.message }, + last_error: { code: 'EXEC_ERROR', message }, failed_at: failedAt, });Apply similar pattern to catch blocks at lines 250, 425.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/agentDefaults.ts` around lines 200 - 207, Change the catch parameter from err: any to err: unknown and narrow it before using err.message: e.g., in the catch around the call to this.__agentStore.updateRun (where you call nowUnix() and use run.id to set last_error), detect if err is an instance of Error and pull err.message, otherwise convert to a safe string (String(err) or a default message) and use that value for last_error.message; apply the same pattern to the other catch blocks in this file that currently use err: any so all error handling is type-safe.
514-522: Consider stronger typing forAGENT_DEFAULT_FACTORIES.The
() => Functionreturn type is loose. A more specific type or generic interface would improve type safety and IDE support.♻️ Proposed type improvement
-export const AGENT_DEFAULT_FACTORIES: Record<string, () => Function> = { +type AgentDefaultFactory = () => (...args: unknown[]) => unknown; +export const AGENT_DEFAULT_FACTORIES: Record<string, AgentDefaultFactory> = {Or define explicit factory types for each method signature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/agentDefaults.ts` around lines 514 - 522, AGENT_DEFAULT_FACTORIES is typed too loosely as Record<string, () => Function>; replace it with a precise factory-type mapping by defining an interface or type alias that lists each factory key with its exact function signature (or use typeof on the corresponding defaults), e.g. entries for createThread, getThread, syncRun, asyncRun, streamRun, getRun, cancelRun pointing to the concrete types of defaultCreateThread, defaultGetThread, defaultSyncRun, defaultAsyncRun, defaultStreamRun, defaultGetRun, and defaultCancelRun; then change the AGENT_DEFAULT_FACTORIES declaration to use that new type so callers get full type safety and IDE completion.tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts (3)
17-28: Global in-memory stores may cause test pollution.The module-level
threads,runsMaps and counters persist across test runs. Consider clearing these in test setup/teardown or using instance-level state.♻️ Alternative: Add reset function
// Add at module level export function resetTestState(): void { threads.clear(); runs.clear(); threadCounter = 0; runCounter = 0; }Then call in test
beforeEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts` around lines 17 - 28, The module-level in-memory stores threads, runs and counters threadCounter/runCounter persist across tests and cause pollution; add an exported reset function (e.g., resetTestState) that clears threads.clear(), runs.clear() and resets threadCounter = 0 and runCounter = 0, then call this resetTestState from your test setup (beforeEach or afterEach) to ensure a fresh state for each test run and avoid cross-test interference.
187-187: Avoidas anycast for status field.The
as anycast indicates a type mismatch. Consider using a properRunStatustype or union type.♻️ Proposed fix
- status: run.status as any, + status: run.status as RunObject['status'],Or update the in-memory store to use
RunObject['status']type for the status field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts` at line 187, The code uses a blanket cast "as any" for the status field (status: run.status as any) which hides a type mismatch; replace the cast by annotating the field with the correct type (e.g. RunStatus or RunObject['status']) and/or update the in-memory store type so run.status already has that type; import or reference the RunStatus/RunObject type and change the returned object in AgentTestController (the place where status: run.status is set) to use the precise type instead of "as any".
37-38: Consider using oxlint-disable instead of eslint-disable.Per PR review comments, the project uses oxlint rather than ESLint.
♻️ Proposed fix
- // eslint-disable-next-line `@typescript-eslint/no-unused-vars` + // oxlint-disable-next-line `@typescript-eslint/no-unused-vars`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts` around lines 37 - 38, Replace the ESLint directive with the project's linter directive by changing the line that disables linting above the async generator function execRun (async *execRun(_input: CreateRunInput): AsyncGenerator<AgentStreamMessage>) from using "// eslint-disable-next-line `@typescript-eslint/no-unused-vars`" to the equivalent "// oxlint-disable-next-line `@typescript-eslint/no-unused-vars`" so the project's oxlint recognizes and ignores the unused parameter; ensure only the directive text is changed and no other code in the execRun method is modified.tegg/core/agent-runtime/src/FileAgentStore.ts (2)
7-8: Import extensions should use.jsfor ESM compatibility.As per coding guidelines, ESM imports should use
.jsextensions rather than.tsfor proper runtime resolution.♻️ Proposed fix
-import type { AgentStore, ThreadRecord, RunRecord } from './AgentStore.ts'; -import { AgentNotFoundError } from './errors.ts'; +import type { AgentStore, ThreadRecord, RunRecord } from './AgentStore.js'; +import { AgentNotFoundError } from './errors.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/FileAgentStore.ts` around lines 7 - 8, Update the import statements in FileAgentStore.ts to use .js extensions for ESM runtime resolution: change the type import of AgentStore, ThreadRecord, RunRecord from './AgentStore.ts' to './AgentStore.js' and change the AgentNotFoundError import from './errors.ts' to './errors.js' so the module specifiers are ESM-compatible.
118-118: Avoidanytype; useunknowninstead.The coding guidelines recommend avoiding
anyand usingunknownwhen the type is truly unknown.♻️ Proposed fix
- } catch (err: any) { - if (err.code === 'ENOENT') { + } catch (err: unknown) { + if (err instanceof Error && 'code' in err && err.code === 'ENOENT') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/FileAgentStore.ts` at line 118, Replace the catch clause currently declared as "catch (err: any)" in FileAgentStore.ts with "catch (err: unknown)" and then narrow the error before using it (e.g., check "err instanceof Error" or typeof checks) so you only access error.message or stack after safe type guards; update any downstream references to err to use the narrowed variable or a safe extractor to satisfy the no-any guideline.tegg/core/agent-runtime/test/FileAgentStore.test.ts (2)
53-63: Consider adding path traversal test cases.The PR review flagged missing error-path tests for path traversal in FileAgentStore. Consider adding tests for malicious IDs like
../etc/passwdorfoo/../bar.🧪 Suggested test case
it('should reject path traversal attempts', async () => { await assert.rejects( () => store.getThread('../etc/passwd'), /Invalid id/, ); await assert.rejects( () => store.getThread('foo/../bar'), /Invalid id/, ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts` around lines 53 - 63, Add negative test cases to FileAgentStore.test.ts that assert FileAgentStore.getThread rejects path-traversal IDs; specifically add a test (e.g., alongside the existing AgentNotFoundError test) that calls store.getThread('../etc/passwd') and store.getThread('foo/../bar') and asserts each call rejects with an "Invalid id" error (or a matcher /Invalid id/). Reference the FileAgentStore.getThread method and ensure both inputs are covered as separate assert.rejects expectations within the new it('should reject path traversal attempts', ...) test.
7-8: Import extensions should use.jsfor ESM compatibility.As per coding guidelines, ESM imports should use
.jsextensions.♻️ Proposed fix
-import { AgentNotFoundError } from '../src/errors.ts'; -import { FileAgentStore } from '../src/FileAgentStore.ts'; +import { AgentNotFoundError } from '../src/errors.js'; +import { FileAgentStore } from '../src/FileAgentStore.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts` around lines 7 - 8, Update the ESM import statements so they use .js extensions: change the imports that bring in AgentNotFoundError and FileAgentStore to reference their compiled .js module files instead of .ts. Locate the import lines that reference '../src/errors.ts' and '../src/FileAgentStore.ts' (symbols: AgentNotFoundError, FileAgentStore) and modify the module specifiers to '../src/errors.js' and '../src/FileAgentStore.js' respectively so they are ESM-compatible.tegg/core/agent-runtime/test/agentDefaults.test.ts (3)
7-9: Import extensions should use.jsfor ESM compatibility.As per coding guidelines, ESM imports should use
.jsextensions.♻️ Proposed fix
-import { AGENT_DEFAULT_FACTORIES } from '../src/agentDefaults.ts'; -import { AgentNotFoundError } from '../src/errors.ts'; -import { FileAgentStore } from '../src/FileAgentStore.ts'; +import { AGENT_DEFAULT_FACTORIES } from '../src/agentDefaults.js'; +import { AgentNotFoundError } from '../src/errors.js'; +import { FileAgentStore } from '../src/FileAgentStore.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/agentDefaults.test.ts` around lines 7 - 9, Update the ESM imports in agentDefaults.test.ts to use .js extensions instead of .ts: locate the import statements that reference AGENT_DEFAULT_FACTORIES, AgentNotFoundError, and FileAgentStore and change their module specifiers to end with .js so the test runs under ESM (e.g., update the imports that currently import from '../src/agentDefaults.ts', '../src/errors.ts', and '../src/FileAgentStore.ts' to use '.js').
265-317: Missing tests forstreamRundefault implementation.The PR review noted that there are no unit tests for the
streamRundefault (SSE, abort handling). Consider adding test coverage for the streaming path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/agentDefaults.test.ts` around lines 265 - 317, Add a new unit test that mirrors the existing cancelRun test but targets the streaming path: call AGENT_DEFAULT_FACTORIES.streamRun() and bind it to mockInstance, implement/mockInstance.execStream (or the method streamRun uses) as an async generator that yields initial SSE assistant event(s), then waits with abort-signal-aware logic (listen to signal.abort and reject/throw on abort) before yielding final events; start the streamRun, let it begin, then call AGENT_DEFAULT_FACTORIES.cancelRun() to cancel the run and assert the cancel response (id, object, status === 'cancelled'), await the background task from mockInstance.__runningTasks.get(id), then fetch the run via mockInstance.__agentStore.getRun(id) and assert run.status === 'cancelled' and run.cancelled_at is set; ensure the test asserts that initial SSE events were produced before cancellation to validate streaming behavior and abort handling.
298-299: FixedsetTimeoutdelay may cause flaky tests.The PR review flagged that using fixed delays like
setTimeout(resolve, 50)can cause flaky tests. Consider polling with a timeout instead, or usingBackgroundTaskHelperas recommended in project guidelines.♻️ Polling alternative
// Poll for task to start running instead of fixed delay const pollForInProgress = async (maxWait = 1000) => { const start = Date.now(); while (Date.now() - start < maxWait) { const run = await mockInstance.__agentStore.getRun(result.id); if (run.status === 'in_progress') return; await new Promise(r => setTimeout(r, 10)); } }; await pollForInProgress();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/agentDefaults.test.ts` around lines 298 - 299, Replace the fixed sleep used to "Let background task start running" with a polling or helper-based wait to avoid flakiness: instead of awaiting new Promise(resolve => setTimeout(resolve, 50)), poll the run status via mockInstance.__agentStore.getRun(result.id) until run.status === 'in_progress' (with a max timeout), or use the project's BackgroundTaskHelper utility to wait for the background task to start; ensure the wait times out and fails the test if the run never becomes 'in_progress'.
🤖 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/agentDefaults.ts`:
- Around line 283-452: The handler in defaultStreamRun should stop bypassing Koa
by removing ctx.respond = false and direct writes to ctx.res; instead create a
PassThrough stream (imported from stream) and assign it to ctx.body, set SSE
headers via ctx.set, and write events to that PassThrough (keeping the same
event strings currently written to res). Wire abortController to the stream
close (stream.on('close', () => abortController.abort())) and ensure you call
stream.end() in the finally block rather than res.end(); keep all uses of
runObj, msgObj, abortController, and accumulatedContent unchanged but replace
res.write/res.writableEnded checks with writes to the PassThrough stream and
stream.writableEnded checks.
---
Nitpick comments:
In `@tegg/core/agent-runtime/src/agentDefaults.ts`:
- Around line 15-16: Update the import statement extensions in agentDefaults.ts
to use .js for ESM compatibility: change the import sources that reference
'./AgentStore.ts' and './errors.ts' to './AgentStore.js' and './errors.js'
respectively so the imported symbols AgentStore and AgentConflictError resolve
correctly at runtime in an ESM environment.
- Around line 200-207: Change the catch parameter from err: any to err: unknown
and narrow it before using err.message: e.g., in the catch around the call to
this.__agentStore.updateRun (where you call nowUnix() and use run.id to set
last_error), detect if err is an instance of Error and pull err.message,
otherwise convert to a safe string (String(err) or a default message) and use
that value for last_error.message; apply the same pattern to the other catch
blocks in this file that currently use err: any so all error handling is
type-safe.
- Around line 514-522: AGENT_DEFAULT_FACTORIES is typed too loosely as
Record<string, () => Function>; replace it with a precise factory-type mapping
by defining an interface or type alias that lists each factory key with its
exact function signature (or use typeof on the corresponding defaults), e.g.
entries for createThread, getThread, syncRun, asyncRun, streamRun, getRun,
cancelRun pointing to the concrete types of defaultCreateThread,
defaultGetThread, defaultSyncRun, defaultAsyncRun, defaultStreamRun,
defaultGetRun, and defaultCancelRun; then change the AGENT_DEFAULT_FACTORIES
declaration to use that new type so callers get full type safety and IDE
completion.
In `@tegg/core/agent-runtime/src/FileAgentStore.ts`:
- Around line 7-8: Update the import statements in FileAgentStore.ts to use .js
extensions for ESM runtime resolution: change the type import of AgentStore,
ThreadRecord, RunRecord from './AgentStore.ts' to './AgentStore.js' and change
the AgentNotFoundError import from './errors.ts' to './errors.js' so the module
specifiers are ESM-compatible.
- Line 118: Replace the catch clause currently declared as "catch (err: any)" in
FileAgentStore.ts with "catch (err: unknown)" and then narrow the error before
using it (e.g., check "err instanceof Error" or typeof checks) so you only
access error.message or stack after safe type guards; update any downstream
references to err to use the narrowed variable or a safe extractor to satisfy
the no-any guideline.
In `@tegg/core/agent-runtime/test/agentDefaults.test.ts`:
- Around line 7-9: Update the ESM imports in agentDefaults.test.ts to use .js
extensions instead of .ts: locate the import statements that reference
AGENT_DEFAULT_FACTORIES, AgentNotFoundError, and FileAgentStore and change their
module specifiers to end with .js so the test runs under ESM (e.g., update the
imports that currently import from '../src/agentDefaults.ts',
'../src/errors.ts', and '../src/FileAgentStore.ts' to use '.js').
- Around line 265-317: Add a new unit test that mirrors the existing cancelRun
test but targets the streaming path: call AGENT_DEFAULT_FACTORIES.streamRun()
and bind it to mockInstance, implement/mockInstance.execStream (or the method
streamRun uses) as an async generator that yields initial SSE assistant
event(s), then waits with abort-signal-aware logic (listen to signal.abort and
reject/throw on abort) before yielding final events; start the streamRun, let it
begin, then call AGENT_DEFAULT_FACTORIES.cancelRun() to cancel the run and
assert the cancel response (id, object, status === 'cancelled'), await the
background task from mockInstance.__runningTasks.get(id), then fetch the run via
mockInstance.__agentStore.getRun(id) and assert run.status === 'cancelled' and
run.cancelled_at is set; ensure the test asserts that initial SSE events were
produced before cancellation to validate streaming behavior and abort handling.
- Around line 298-299: Replace the fixed sleep used to "Let background task
start running" with a polling or helper-based wait to avoid flakiness: instead
of awaiting new Promise(resolve => setTimeout(resolve, 50)), poll the run status
via mockInstance.__agentStore.getRun(result.id) until run.status ===
'in_progress' (with a max timeout), or use the project's BackgroundTaskHelper
utility to wait for the background task to start; ensure the wait times out and
fails the test if the run never becomes 'in_progress'.
In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts`:
- Around line 53-63: Add negative test cases to FileAgentStore.test.ts that
assert FileAgentStore.getThread rejects path-traversal IDs; specifically add a
test (e.g., alongside the existing AgentNotFoundError test) that calls
store.getThread('../etc/passwd') and store.getThread('foo/../bar') and asserts
each call rejects with an "Invalid id" error (or a matcher /Invalid id/).
Reference the FileAgentStore.getThread method and ensure both inputs are covered
as separate assert.rejects expectations within the new it('should reject path
traversal attempts', ...) test.
- Around line 7-8: Update the ESM import statements so they use .js extensions:
change the imports that bring in AgentNotFoundError and FileAgentStore to
reference their compiled .js module files instead of .ts. Locate the import
lines that reference '../src/errors.ts' and '../src/FileAgentStore.ts' (symbols:
AgentNotFoundError, FileAgentStore) and modify the module specifiers to
'../src/errors.js' and '../src/FileAgentStore.js' respectively so they are
ESM-compatible.
In
`@tegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.ts`:
- Around line 17-28: The module-level in-memory stores threads, runs and
counters threadCounter/runCounter persist across tests and cause pollution; add
an exported reset function (e.g., resetTestState) that clears threads.clear(),
runs.clear() and resets threadCounter = 0 and runCounter = 0, then call this
resetTestState from your test setup (beforeEach or afterEach) to ensure a fresh
state for each test run and avoid cross-test interference.
- Line 187: The code uses a blanket cast "as any" for the status field (status:
run.status as any) which hides a type mismatch; replace the cast by annotating
the field with the correct type (e.g. RunStatus or RunObject['status']) and/or
update the in-memory store type so run.status already has that type; import or
reference the RunStatus/RunObject type and change the returned object in
AgentTestController (the place where status: run.status is set) to use the
precise type instead of "as any".
- Around line 37-38: Replace the ESLint directive with the project's linter
directive by changing the line that disables linting above the async generator
function execRun (async *execRun(_input: CreateRunInput):
AsyncGenerator<AgentStreamMessage>) from using "// eslint-disable-next-line
`@typescript-eslint/no-unused-vars`" to the equivalent "//
oxlint-disable-next-line `@typescript-eslint/no-unused-vars`" so the project's
oxlint recognizes and ignores the unused parameter; ensure only the directive
text is changed and no other code in the execRun method is modified.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
tegg/core/agent-runtime/src/FileAgentStore.tstegg/core/agent-runtime/src/agentDefaults.tstegg/core/agent-runtime/src/errors.tstegg/core/agent-runtime/src/index.tstegg/core/agent-runtime/test/FileAgentStore.test.tstegg/core/agent-runtime/test/agentDefaults.test.tstegg/core/tegg/src/agent.tstegg/plugin/controller/test/fixtures/apps/agent-controller-app/app/controller/AgentTestController.tstegg/plugin/controller/test/http/agent.test.tstegg/plugin/controller/test/http/base-agent.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tegg/core/agent-runtime/src/index.ts
- tegg/plugin/controller/test/http/base-agent.test.ts
- tegg/plugin/controller/test/http/agent.test.ts
| function defaultStreamRun() { | ||
| return async function (this: AgentInstance, input: CreateRunInput): Promise<void> { | ||
| const runtimeCtx = ContextHandler.getContext(); | ||
| if (!runtimeCtx) { | ||
| throw new Error('streamRun must be called within a request context'); | ||
| } | ||
| const ctx = runtimeCtx.get(EGG_CONTEXT); | ||
|
|
||
| // Bypass Koa response handling — write SSE directly to the raw response | ||
| ctx.respond = false; | ||
| const res = ctx.res; | ||
| res.writeHead(200, { | ||
| 'Content-Type': 'text/event-stream', | ||
| 'Cache-Control': 'no-cache', | ||
| Connection: 'keep-alive', | ||
| }); | ||
|
|
||
| // Abort execRun generator when client disconnects | ||
| const abortController = new AbortController(); | ||
| res.on('close', () => abortController.abort()); | ||
|
|
||
| let threadId = input.thread_id; | ||
| if (!threadId) { | ||
| const thread = await this.__agentStore.createThread(); | ||
| threadId = thread.id; | ||
| input = { ...input, thread_id: threadId }; | ||
| } | ||
|
|
||
| const run = await this.__agentStore.createRun(input.input.messages, threadId, input.config, input.metadata); | ||
|
|
||
| const runObj: RunObject = { | ||
| id: run.id, | ||
| object: 'thread.run', | ||
| created_at: run.created_at, | ||
| thread_id: threadId, | ||
| status: 'queued', | ||
| metadata: run.metadata, | ||
| }; | ||
|
|
||
| // event: thread.run.created | ||
| res.write(`event: thread.run.created\ndata: ${JSON.stringify(runObj)}\n\n`); | ||
|
|
||
| // event: thread.run.in_progress | ||
| runObj.status = 'in_progress'; | ||
| runObj.started_at = nowUnix(); | ||
| await this.__agentStore.updateRun(run.id, { status: 'in_progress', started_at: runObj.started_at }); | ||
| res.write(`event: thread.run.in_progress\ndata: ${JSON.stringify(runObj)}\n\n`); | ||
|
|
||
| const msgId = newMsgId(); | ||
| const accumulatedContent: MessageObject['content'] = []; | ||
|
|
||
| // event: thread.message.created | ||
| const msgObj: MessageObject = { | ||
| id: msgId, | ||
| object: 'thread.message', | ||
| created_at: nowUnix(), | ||
| run_id: run.id, | ||
| role: 'assistant', | ||
| status: 'in_progress', | ||
| content: [], | ||
| }; | ||
| res.write(`event: thread.message.created\ndata: ${JSON.stringify(msgObj)}\n\n`); | ||
|
|
||
| let promptTokens = 0; | ||
| let completionTokens = 0; | ||
| let totalTokens = 0; | ||
| let hasUsage = false; | ||
|
|
||
| try { | ||
| for await (const msg of this.execRun(input, abortController.signal)) { | ||
| if (abortController.signal.aborted) break; | ||
| if (msg.message) { | ||
| const contentBlocks = toContentBlocks(msg.message); | ||
| accumulatedContent.push(...contentBlocks); | ||
|
|
||
| // event: thread.message.delta | ||
| const delta: MessageDeltaObject = { | ||
| id: msgId, | ||
| object: 'thread.message.delta', | ||
| delta: { content: contentBlocks }, | ||
| }; | ||
| res.write(`event: thread.message.delta\ndata: ${JSON.stringify(delta)}\n\n`); | ||
| } | ||
| if (msg.usage) { | ||
| hasUsage = true; | ||
| promptTokens += msg.usage.prompt_tokens ?? 0; | ||
| completionTokens += msg.usage.completion_tokens ?? 0; | ||
| totalTokens += msg.usage.total_tokens ?? 0; | ||
| } | ||
| } | ||
|
|
||
| // If client disconnected / abort signaled, emit cancelled and return | ||
| if (abortController.signal.aborted) { | ||
| const cancelledAt = nowUnix(); | ||
| try { | ||
| await this.__agentStore.updateRun(run.id, { status: 'cancelled', cancelled_at: cancelledAt }); | ||
| } catch { | ||
| // Ignore store update failure during abort | ||
| } | ||
| runObj.status = 'cancelled'; | ||
| runObj.cancelled_at = cancelledAt; | ||
| if (!res.writableEnded) { | ||
| res.write(`event: thread.run.cancelled\ndata: ${JSON.stringify(runObj)}\n\n`); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // event: thread.message.completed | ||
| msgObj.status = 'completed'; | ||
| msgObj.content = accumulatedContent; | ||
| res.write(`event: thread.message.completed\ndata: ${JSON.stringify(msgObj)}\n\n`); | ||
|
|
||
| // Build final output | ||
| const output: MessageObject[] = accumulatedContent.length > 0 ? [msgObj] : []; | ||
| let usage: RunObject['usage']; | ||
| if (hasUsage) { | ||
| usage = { | ||
| prompt_tokens: promptTokens, | ||
| completion_tokens: completionTokens, | ||
| total_tokens: Math.max(promptTokens + completionTokens, totalTokens), | ||
| }; | ||
| } | ||
|
|
||
| const completedAt = nowUnix(); | ||
| await this.__agentStore.updateRun(run.id, { | ||
| status: 'completed', | ||
| output, | ||
| usage, | ||
| completed_at: completedAt, | ||
| }); | ||
|
|
||
| await this.__agentStore.appendMessages(threadId!, [ | ||
| ...toInputMessageObjects(input.input.messages, threadId), | ||
| ...output, | ||
| ]); | ||
|
|
||
| // event: thread.run.completed | ||
| runObj.status = 'completed'; | ||
| runObj.completed_at = completedAt; | ||
| runObj.usage = usage; | ||
| runObj.output = output; | ||
| res.write(`event: thread.run.completed\ndata: ${JSON.stringify(runObj)}\n\n`); | ||
| } catch (err: any) { | ||
| const failedAt = nowUnix(); | ||
| try { | ||
| await this.__agentStore.updateRun(run.id, { | ||
| status: 'failed', | ||
| last_error: { code: 'EXEC_ERROR', message: err.message }, | ||
| failed_at: failedAt, | ||
| }); | ||
| } catch (storeErr) { | ||
| // Log store update failure but don't swallow the original error | ||
| console.error('[AgentController] failed to update run status after error:', storeErr); | ||
| } | ||
|
|
||
| // event: thread.run.failed | ||
| runObj.status = 'failed'; | ||
| runObj.failed_at = failedAt; | ||
| runObj.last_error = { code: 'EXEC_ERROR', message: err.message }; | ||
| if (!res.writableEnded) { | ||
| res.write(`event: thread.run.failed\ndata: ${JSON.stringify(runObj)}\n\n`); | ||
| } | ||
| } finally { | ||
| // event: done | ||
| if (!res.writableEnded) { | ||
| res.write('event: done\ndata: [DONE]\n\n'); | ||
| res.end(); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Koa SSE streaming best practices middleware bypass ctx.respond
💡 Result:
Koa SSE streaming: best practices (and when to bypass Koa with ctx.respond = false)
1) Prefer ctx.body = stream over ctx.res.write(...)
Koa natively supports streaming responses by assigning a Node stream to ctx.body, and it will pipe it for you. This usually plays better with Koa middleware than writing to ctx.res directly. Koa also notes special behavior when you set a stream as the body (error handling + destroying the stream on request close). [1]
Pattern (recommended):
import Router from '@koa/router'
import { PassThrough } from 'node:stream'
const router = new Router()
router.get('/events', async (ctx) => {
ctx.set('Content-Type', 'text/event-stream')
ctx.set('Cache-Control', 'no-cache')
ctx.set('Connection', 'keep-alive')
// If you're behind nginx, this header is commonly used to prevent buffering:
ctx.set('X-Accel-Buffering', 'no') // see notes below
const stream = new PassThrough()
ctx.status = 200
ctx.body = stream
// optional: tell browser how long to wait before reconnect
stream.write('retry: 5000\n\n')
const ping = setInterval(() => stream.write(': keep-alive\n\n'), 15000)
ctx.req.on('close', () => {
clearInterval(ping)
stream.end()
})
// emit events
stream.write(`event: hello\ndata: ${JSON.stringify({ ok: true })}\n\n`)
})2) If you must write to ctx.res, set ctx.respond = false (and do it early)
Koa documents ctx.respond = false as the way to bypass Koa’s built-in response handling so you can write to the raw res object—while warning it’s “not supported” and may break middleware expectations. [1]
This is also a common fix for SSE handlers that use ctx.res.write(...) and otherwise end up with weird behavior (like unexpected 404 / reconnect loops). [2]
Raw res pattern (only if necessary):
router.get('/events', async (ctx) => {
ctx.respond = false // bypass Koa response handling [1][2]
ctx.res.writeHead(200, {
'Content-Type': 'text/event-stream',
'Cache-Control': 'no-cache',
'Connection': 'keep-alive',
'X-Accel-Buffering': 'no',
})
ctx.res.write(': keep-alive\n\n')
const ping = setInterval(() => ctx.res.write(': keep-alive\n\n'), 15000)
ctx.req.on('close', () => {
clearInterval(ping)
ctx.res.end()
})
})Middleware implication: once you bypass Koa (ctx.respond = false), you should treat the request like “handled”; don’t await next() into middleware that might set ctx.body, do compression, etc., or you’ll get hard-to-debug interactions.
3) SSE protocol essentials (headers + formatting)
- Response must be
Content-Type: text/event-stream. [3] - Events are UTF‑8 text blocks separated by a blank line (
\n\n). [3] - Send periodic comment lines (
: ping\n\n) as keep-alives to avoid idle timeouts. [3]
4) If you’re behind NGINX / a gateway, disable buffering for SSE
Reverse proxies often buffer responses unless configured not to; for SSE you typically want streaming flush-through. NGINX explicitly calls out SSE as a case where disabling buffering is useful. [4]
(Implementation details vary by product; common approaches include route-level buffering disable and/or X-Accel-Buffering: no depending on your NGINX setup. [3][4])
Rule of thumb
- Use
ctx.body = PassThrough()for SSE whenever possible (most Koa-friendly). [1] - Use
ctx.respond = falseonly when you need rawres.write()control and you accept the middleware tradeoffs. [1][2]
Sources: Koa docs on ctx.respond + streaming behavior [1], Koa SSE discussion recommending ctx.respond=false when using ctx.res [2], MDN SSE formatting/keep-alive guidance [3], NGINX guidance on disabling buffering for SSE/streaming [4].
Refactor to use ctx.body = PassThrough() instead of bypassing Koa middleware.
The current implementation uses ctx.respond = false and writes directly to ctx.res, which Koa documents as "not supported" and warns may break middleware expectations. This bypasses CORS, error handling, and other middleware in the chain.
Koa natively supports SSE streaming via ctx.body = stream (using a PassThrough() stream), which handles response piping while preserving middleware behavior. This approach is more reliable and maintains compatibility with Koa's middleware ecosystem. Use this pattern unless there's a specific reason raw res control is necessary—which should then be documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/agentDefaults.ts` around lines 283 - 452, The
handler in defaultStreamRun should stop bypassing Koa by removing ctx.respond =
false and direct writes to ctx.res; instead create a PassThrough stream
(imported from stream) and assign it to ctx.body, set SSE headers via ctx.set,
and write events to that PassThrough (keeping the same event strings currently
written to res). Wire abortController to the stream close (stream.on('close', ()
=> abortController.abort())) and ensure you call stream.end() in the finally
block rather than res.end(); keep all uses of runObj, msgObj, abortController,
and accumulatedContent unchanged but replace res.write/res.writableEnded checks
with writes to the PassThrough stream and stream.writableEnded checks.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #5812 +/- ##
==========================================
- Coverage 85.22% 81.03% -4.20%
==========================================
Files 650 652 +2
Lines 12515 12750 +235
Branches 1434 1449 +15
==========================================
- Hits 10666 10332 -334
- Misses 1729 2249 +520
- Partials 120 169 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…me class Replace the 7 factory functions in agentDefaults.ts with an AgentRuntime class that holds `store` and `runningTasks` as private fields and delegates execution back to the controller via a `host` reference. This follows the OO style requested in PR review. - Create AgentRuntime class with createThread, getThread, syncRun, asyncRun, streamRun, getRun, cancelRun as instance methods - Define AgentControllerHost interface for host delegation (AOP/mock friendly) - Simplify enhanceAgentController to create AgentRuntime instance on init and delegate stub methods via this[AGENT_RUNTIME] - Delete agentDefaults.ts (all logic moved into AgentRuntime.ts) - Update FileAgentStore concurrency comment to clarify cluster-mode limitation - Rewrite tests to directly instantiate AgentRuntime instead of using factory.call(mockInstance) pattern Closes eggjs#5812 (comment: "需要用 oo 的风格来写") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…data Use dedicated AgentInfoUtil class and MetadataKey constants instead of inline Symbol.for() for agent controller/method markers. Also adds agent-runtime to tsconfig references and moves controller-decorator to dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
tegg/core/controller-decorator/test/AgentController.test.ts (2)
14-15:⚠️ Potential issue | 🟡 MinorSwitch relative ESM imports to
.jsextensions.Line 14–15 still use
.tsin relative imports undertegg/, which is inconsistent with the tegg ESM import convention.Suggested change
-} from '../src/index.ts'; -import { HTTPControllerMeta } from '../src/model/index.ts'; +} from '../src/index.js'; +import { HTTPControllerMeta } from '../src/model/index.js';As per coding guidelines: “tegg/**/*.{ts,tsx} : 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/AgentController.test.ts` around lines 14 - 15, Update the two relative ESM imports to use .js extensions: change the import from '../src/index.ts' (where controller exports like HTTPControllerMeta are referenced) and from '../src/model/index.ts' to use '../src/index.js' and '../src/model/index.js' respectively so the test (AgentController.test.ts) follows the tegg ESM import convention; ensure any other tegg/* imports in this file also use .js.
121-121:⚠️ Potential issue | 🟡 MinorReplace
as anytest casts with narrowunknown-based shapes.Line 121 and Line 145 still use
as any, which weakens static checks.Suggested change
- const proto = AgentFooController.prototype as any; + const proto = AgentFooController.prototype as Record<string, unknown>; @@ - const instance = new AgentFooController() as any; - await assert.rejects(() => instance[name](...args), new RegExp(`${name} not implemented`)); + const instance = new AgentFooController() as unknown as Record<string, (...args: unknown[]) => Promise<unknown>>; + await assert.rejects(() => instance[name](...args), new RegExp(`${name} not implemented`));As per coding guidelines: “**/*.ts : Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown”.
Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/controller-decorator/test/AgentController.test.ts` at line 121, Tests currently cast AgentFooController.prototype to "any", weakening type checks; replace those casts with an unknown-based narrow shape: change the cast of AgentFooController.prototype (used at the spots around the current `const proto = AgentFooController.prototype as any;` and the other occurrence at lines ~145-146) to `as unknown as { <only-the-properties-you-need>: <their-types> }` (or define a small local interface/type for the required properties) so the test keeps stricter typing while still allowing access to the specific fields/methods the assertions use (refer to AgentFooController.prototype and the specific property/method names the test accesses to build the narrow shape).
🧹 Nitpick comments (5)
tegg/core/agent-runtime/test/AgentRuntime.test.ts (5)
211-234: Missing error-path test forgetRunwith non-existent run.The
getThreadsection includes a test forAgentNotFoundErrorwith a non-existent thread, butgetRunlacks an equivalent test. For consistency and coverage, consider adding a 404 error path test.💚 Suggested test addition
it('should throw AgentNotFoundError for non-existent run', async () => { await assert.rejects( () => runtime.getRun('run_nonexistent'), (err: unknown) => { assert(err instanceof AgentNotFoundError); assert.equal(err.status, 404); return true; }, ); });🤖 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 211 - 234, Add a negative test in the "getRun" describe block to mirror the getThread coverage: call runtime.getRun('run_nonexistent') and assert it rejects with an instance of AgentNotFoundError and that err.status === 404; place the test alongside the existing getRun tests (referencing runtime.getRun and AgentNotFoundError) and use assert.rejects to verify the thrown error and status.
80-83: Pervasiveas anycasts in test inputs reduce type safety.Multiple test cases cast the input to
any. This masks potential type mismatches between test data and the actualCreateRunInputinterface. Consider defining a helper or using properly typed test fixtures.♻️ Example: define typed test input helper
// At the top of the describe block function makeRunInput(messages: Array<{ role: string; content: string }>, options?: { thread_id?: string; metadata?: Record<string, unknown> }): CreateRunInput { return { input: { messages }, thread_id: options?.thread_id, metadata: options?.metadata, } as CreateRunInput; } // In tests: const result = await runtime.syncRun(makeRunInput([{ role: 'user', content: 'Hi' }]));Also applies to: 104-107, 115-118, 127-130, 139-141, 155-157, 166-168, 179-181, 196-199, 213-215, 226-228, 264-266
🤖 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 80 - 83, Tests are using pervasive `as any` on inputs to runtime.syncRun which hides type issues; create a typed helper (e.g., makeRunInput) that returns a CreateRunInput and use it in tests instead of casting, ensuring runtime.syncRun calls like runtime.syncRun(makeRunInput([{ role: 'user', content: 'Hi' }])) are properly typed; add appropriate import/type reference to CreateRunInput at top of the test file and update all occurrences listed (around the calls to runtime.syncRun) to use the helper rather than `as any`.
244-257: Consider extracting abort-aware delay helper.The inline promise with abort signal handling is somewhat complex. This pattern could be extracted into a test utility for reuse and readability.
🤖 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 244 - 257, Extract the inline abort-aware Promise into a reusable test helper (e.g., waitWithAbort or delayWithAbort) that accepts a duration (ms) and an AbortSignal and returns a Promise<void> which resolves after the timeout or rejects when the signal aborts; implement it in the test utilities and replace the inline block in AgentRuntime.test.ts with a call to that helper, ensuring you export/import the helper, preserve the { once: true } event listener behavior, and add proper TypeScript typing for the AbortSignal and returned Promise.
21-36: Avoidas anycasts; use proper types for the mock host.The mock
hostusesanyfor the input parameter and is cast toany. This bypasses type safety and could mask type errors. Consider using the actualCreateRunInputtype or a minimal compatible interface.As per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown."
♻️ Suggested improvement
+import type { CreateRunInput } from '../src/AgentRuntime.ts'; + host = { - async *execRun(input: any) { + async *execRun(input: CreateRunInput) { const messages = input.input.messages; yield { type: 'assistant', message: { role: 'assistant', content: [{ type: 'text', text: `Hello ${messages.length} messages` }], }, }; yield { type: 'result', usage: { prompt_tokens: 10, completion_tokens: 5 }, }; }, - } as any; + } as AgentControllerHost;🤖 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 21 - 36, The mock host currently uses `any` and an `as any` cast; change `async *execRun(input: any)` to use the real request type (e.g. `CreateRunInput`) or a minimal interface that includes `input.messages` (or use `unknown` and narrow it) and remove the `as any` host cast; update the `host` variable to be typed (e.g. `Host`/`RunnerHost` or `Pick` with `execRun`) so TypeScript enforces the `execRun` signature and message shape (refer to the `host` object and the `execRun` generator function in the test to locate where to import/create the proper types and remove `any`).
12-281: Test coverage gaps: streamRun and error paths not tested.This test suite provides good coverage for the happy paths but lacks:
- Unit tests for
streamRun(SSE streaming, abort handling)- Error-path tests (e.g.,
execRunthrows an exception mid-stream)- Background failure handling in
asyncRunThese gaps were noted in the PR review comments. Consider adding these tests for robustness.
Would you like me to help draft test cases for
streamRunand error scenarios?🤖 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 12 - 281, Add unit tests that cover streamRun and failure/error paths: implement a test that replaces host.execRun with an async generator that yields SSE-like assistant/result chunks and respects an AbortSignal, then call runtime.streamRun to assert it emits the expected assistant messages, handles client abort (simulate by aborting the signal) and that the corresponding run stored via FileAgentStore has correct status and timestamps; add a test where host.execRun throws mid-stream (or rejects) to ensure runtime.syncRun/streamRun and asyncRun background tasks transition the run to an error/cancelled state, record the error in the run metadata/usage, and that runtime.asyncRun background failure is observed after runtime.destroy (use runtime.asyncRun to queue then await destroy and assert store.getRun shows failure and proper fields). Reference AgentRuntime.streamRun, AgentRuntime.syncRun, AgentRuntime.asyncRun, host.execRun, runtime.destroy, and FileAgentStore.getRun when locating where to insert these tests.
🤖 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 502-507: Before awaiting in-flight tasks in destroy(), first
signal cancellation to each running task so long-running execRun generators can
exit; iterate this.runningTasks.values() and for each task call its cancellation
mechanism (e.g. task.controller.abort() if using an AbortController or
task.abort() if an abort method is provided), then collect task.promise and
await Promise.allSettled(pending) as before. Ensure you reference the existing
destroy() method and runningTasks map and handle both AbortController-style
(.abort()) and task-level (.abort()) APIs safely (guard for existence) before
awaiting.
- Around line 204-209: Change the catch signatures from using `any` to `unknown`
(e.g., in AgentRuntime's catch blocks around the calls that call
`this.store.updateRun`) and safely extract an error message before using it:
detect if the caught value is an Error (err instanceof Error) and use
err.message, otherwise coerce to a string (String(err) or a fallback like
'Unknown error'); then pass that safe message into `last_error.message` (and
keep the same `code: 'EXEC_ERROR'` behavior). Apply this pattern to the three
catch sites referenced (the catch at the block updating `run.id` with status
'failed' and the other two catch blocks around the other `this.store.updateRun`
calls) so no catch uses `any` and all error messages are safely derived from an
`unknown`.
- Around line 290-297: You are bypassing Koa by setting ctx.respond = false to
stream SSE in streamRun; ensure every possible error path inside streamRun
(including async operations, AbortController cancellation, and any event-emitter
callbacks) is caught and turned into a single SSE "error" event before closing
the raw res, and keep the existing try-catch-finally/AbortController flow
intact; also add a short in-code comment near the ctx.respond = false line
documenting the architectural trade-off (bypasses Koa middleware error handlers)
and referencing streamRun and the AbortController so future maintainers know to
update the SSE error-emission logic if streamRun changes.
In `@tegg/core/agent-runtime/src/enhanceAgentController.ts`:
- Around line 51-52: The default dataDir uses process.cwd(), which can be
different from the Egg app root; change the default to use the application base
directory instead: when computing dataDir (the variable passed into new
FileAgentStore), prefer an explicit app base directory (e.g. app.baseDir or a
dedicated TEGG_APP_BASE_DIR env var) over process.cwd(), while still honoring
TEGG_AGENT_DATA_DIR if set; update the code around dataDir and FileAgentStore
instantiation in enhanceAgentController to derive the path from the app/appInfo
base directory rather than process.cwd().
- Around line 34-41: The enhancement must fail fast when an `@AgentController`
class omits execRun: after building stubMethods (from AGENT_METHOD_NAMES by
checking clazz.prototype and AgentInfoUtil.isNotImplemented), explicitly check
if stubMethods contains "execRun" and throw a clear error (or call a helper to
abort enhancement) indicating execRun is required on the controller class;
reference the symbols execRun, AGENT_METHOD_NAMES, stubMethods, clazz.prototype,
and AgentInfoUtil.isNotImplemented so you place the check immediately after the
stubMethods population.
In `@tegg/core/agent-runtime/test/AgentRuntime.test.ts`:
- Around line 50-52: The current test in AgentRuntime.test.ts uses
result.created_at < Date.now(), which wrongly compares Unix seconds to
milliseconds and always passes; update the assertion to validate that created_at
is a seconds-since-epoch value and within a plausible range by comparing against
Date.now()/1000 (or checking magnitude between a lower bound like 1_000_000_000
and an upper bound derived from Math.floor(Date.now()/1000) ± a small
tolerance). Replace the existing check on result.created_at with these
seconds-based assertions to ensure the timestamp is in seconds and recent.
- Around line 268-269: Replace the flaky fixed wait (await new Promise(resolve
=> setTimeout(resolve, 50))) with a polling approach that waits for the
background task to reach the expected state (use BackgroundTaskHelper if
available, or poll a predicate with a short interval and an overall timeout) so
tests no longer rely on timing; update the test in AgentRuntime.test.ts where
the setTimeout is used to instead repeatedly check the background task condition
(e.g., a status flag, queue length, or BackgroundTaskHelper.isIdle/hasStarted)
until success or throw after a reasonable timeout to fail deterministically.
In `@tegg/core/controller-decorator/src/decorator/agent/AgentController.ts`:
- Around line 98-100: The code currently hardcodes the API base path via
HTTPInfoUtil.setHTTPPath('/api/v1', constructor) in AgentController.ts; change
this to use a configurable value (e.g., an options parameter to the
AgentController decorator or a clearly named env var) instead of the literal
string. Read the basePath from the decorator options (falling back to a sensible
default like '/api/v1' if not provided) and pass that variable into
HTTPInfoUtil.setHTTPPath(basePath, constructor); update the AgentController
decorator signature to accept and propagate the options and ensure any callers
supply or inherit the desired basePath.
- Around line 93-94: Add an explicit return type to the exported AgentController
function: change its signature to declare it returns a ClassDecorator (or the
explicit function type (constructor: EggProtoImplClass) => void). For example,
update `export function AgentController()` to `export function
AgentController(): ClassDecorator` (or `: (constructor: EggProtoImplClass) =>
void`) so the returned inner function signature `function (constructor:
EggProtoImplClass) { ... }` matches the annotated return type.
---
Duplicate comments:
In `@tegg/core/controller-decorator/test/AgentController.test.ts`:
- Around line 14-15: Update the two relative ESM imports to use .js extensions:
change the import from '../src/index.ts' (where controller exports like
HTTPControllerMeta are referenced) and from '../src/model/index.ts' to use
'../src/index.js' and '../src/model/index.js' respectively so the test
(AgentController.test.ts) follows the tegg ESM import convention; ensure any
other tegg/* imports in this file also use .js.
- Line 121: Tests currently cast AgentFooController.prototype to "any",
weakening type checks; replace those casts with an unknown-based narrow shape:
change the cast of AgentFooController.prototype (used at the spots around the
current `const proto = AgentFooController.prototype as any;` and the other
occurrence at lines ~145-146) to `as unknown as {
<only-the-properties-you-need>: <their-types> }` (or define a small local
interface/type for the required properties) so the test keeps stricter typing
while still allowing access to the specific fields/methods the assertions use
(refer to AgentFooController.prototype and the specific property/method names
the test accesses to build the narrow shape).
---
Nitpick comments:
In `@tegg/core/agent-runtime/test/AgentRuntime.test.ts`:
- Around line 211-234: Add a negative test in the "getRun" describe block to
mirror the getThread coverage: call runtime.getRun('run_nonexistent') and assert
it rejects with an instance of AgentNotFoundError and that err.status === 404;
place the test alongside the existing getRun tests (referencing runtime.getRun
and AgentNotFoundError) and use assert.rejects to verify the thrown error and
status.
- Around line 80-83: Tests are using pervasive `as any` on inputs to
runtime.syncRun which hides type issues; create a typed helper (e.g.,
makeRunInput) that returns a CreateRunInput and use it in tests instead of
casting, ensuring runtime.syncRun calls like runtime.syncRun(makeRunInput([{
role: 'user', content: 'Hi' }])) are properly typed; add appropriate import/type
reference to CreateRunInput at top of the test file and update all occurrences
listed (around the calls to runtime.syncRun) to use the helper rather than `as
any`.
- Around line 244-257: Extract the inline abort-aware Promise into a reusable
test helper (e.g., waitWithAbort or delayWithAbort) that accepts a duration (ms)
and an AbortSignal and returns a Promise<void> which resolves after the timeout
or rejects when the signal aborts; implement it in the test utilities and
replace the inline block in AgentRuntime.test.ts with a call to that helper,
ensuring you export/import the helper, preserve the { once: true } event
listener behavior, and add proper TypeScript typing for the AbortSignal and
returned Promise.
- Around line 21-36: The mock host currently uses `any` and an `as any` cast;
change `async *execRun(input: any)` to use the real request type (e.g.
`CreateRunInput`) or a minimal interface that includes `input.messages` (or use
`unknown` and narrow it) and remove the `as any` host cast; update the `host`
variable to be typed (e.g. `Host`/`RunnerHost` or `Pick` with `execRun`) so
TypeScript enforces the `execRun` signature and message shape (refer to the
`host` object and the `execRun` generator function in the test to locate where
to import/create the proper types and remove `any`).
- Around line 12-281: Add unit tests that cover streamRun and failure/error
paths: implement a test that replaces host.execRun with an async generator that
yields SSE-like assistant/result chunks and respects an AbortSignal, then call
runtime.streamRun to assert it emits the expected assistant messages, handles
client abort (simulate by aborting the signal) and that the corresponding run
stored via FileAgentStore has correct status and timestamps; add a test where
host.execRun throws mid-stream (or rejects) to ensure runtime.syncRun/streamRun
and asyncRun background tasks transition the run to an error/cancelled state,
record the error in the run metadata/usage, and that runtime.asyncRun background
failure is observed after runtime.destroy (use runtime.asyncRun to queue then
await destroy and assert store.getRun shows failure and proper fields).
Reference AgentRuntime.streamRun, AgentRuntime.syncRun, AgentRuntime.asyncRun,
host.execRun, runtime.destroy, and FileAgentStore.getRun when locating where to
insert these tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
tegg/core/agent-runtime/package.jsontegg/core/agent-runtime/src/AgentRuntime.tstegg/core/agent-runtime/src/FileAgentStore.tstegg/core/agent-runtime/src/enhanceAgentController.tstegg/core/agent-runtime/src/index.tstegg/core/agent-runtime/src/utils.tstegg/core/agent-runtime/test/AgentRuntime.test.tstegg/core/agent-runtime/test/enhanceAgentController.test.tstegg/core/controller-decorator/src/decorator/agent/AgentController.tstegg/core/controller-decorator/src/util/AgentInfoUtil.tstegg/core/controller-decorator/src/util/index.tstegg/core/controller-decorator/test/AgentController.test.tstegg/core/types/src/controller-decorator/MetadataKey.tstegg/plugin/controller/src/lib/EggControllerPrototypeHook.tstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (5)
- tegg/core/agent-runtime/test/enhanceAgentController.test.ts
- tegg/core/agent-runtime/src/index.ts
- tegg/plugin/controller/src/lib/EggControllerPrototypeHook.ts
- tegg/core/agent-runtime/package.json
- tegg/core/agent-runtime/src/FileAgentStore.ts
| } catch (err: any) { | ||
| const failedAt = nowUnix(); | ||
| await this.store.updateRun(run.id, { | ||
| status: 'failed', | ||
| last_error: { code: 'EXEC_ERROR', message: err.message }, | ||
| failed_at: failedAt, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and get basic info
if [ -f "tegg/core/agent-runtime/src/AgentRuntime.ts" ]; then
wc -l tegg/core/agent-runtime/src/AgentRuntime.ts
else
echo "File not found"
find . -name "AgentRuntime.ts" -type f
fiRepository: eggjs/egg
Length of output: 100
🏁 Script executed:
# Extract the relevant sections around the flagged lines
# Line 204 area
echo "=== Around line 204 ==="
sed -n '200,215p' tegg/core/agent-runtime/src/AgentRuntime.ts
# Line 252 area
echo "=== Around line 252 ==="
sed -n '248,265p' tegg/core/agent-runtime/src/AgentRuntime.ts
# Line 421 area
echo "=== Around line 421 ==="
sed -n '417,435p' tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 1887
🏁 Script executed:
# Also check if there's a helper function or pattern for error message extraction already in the file
rg "getErrorMessage|instanceof Error|err\.message" tegg/core/agent-runtime/src/AgentRuntime.ts -A 2 -B 2Repository: eggjs/egg
Length of output: 879
🏁 Script executed:
# Check if there are any existing error handling utilities or patterns in the file
echo "=== Looking for error handling patterns ==="
rg "Error|instanceof|message" tegg/core/agent-runtime/src/AgentRuntime.ts | head -20Repository: eggjs/egg
Length of output: 1420
🏁 Script executed:
# Check related files for error handling patterns
echo "=== Error handling in related files ==="
find tegg/core/agent-runtime/src -name "*.ts" -type f | head -5 | xargs -I {} sh -c 'echo "File: {}"; rg "instanceof Error|getError|getErrorMessage" {} | head -3'Repository: eggjs/egg
Length of output: 346
🏁 Script executed:
# Check if there's an error utility module
find tegg -name "*error*" -o -name "*util*" | grep -E "\.(ts|js)$" | head -10Repository: eggjs/egg
Length of output: 406
🏁 Script executed:
# Check the utils.ts file for existing error utilities
echo "=== tegg/core/agent-runtime/src/utils.ts ==="
cat tegg/core/agent-runtime/src/utils.tsRepository: eggjs/egg
Length of output: 289
🏁 Script executed:
# Check the errors.ts file to understand error types
echo "=== tegg/core/agent-runtime/src/errors.ts ==="
cat tegg/core/agent-runtime/src/errors.tsRepository: eggjs/egg
Length of output: 732
🏁 Script executed:
# Check the top of AgentRuntime.ts to see imports and understand type patterns
echo "=== Top of AgentRuntime.ts (first 50 lines) ==="
head -50 tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 1789
🏁 Script executed:
# Check if there are any other catch blocks with 'any' type
rg "catch\s*\(\s*\w+\s*:\s*any" tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 129
🏁 Script executed:
# Also check for other error handling patterns that might use 'any'
rg ":\s*any\s*\)" tegg/core/agent-runtime/src/AgentRuntime.ts | head -10Repository: eggjs/egg
Length of output: 129
Replace err: any in catch blocks with unknown + safe message extraction.
Lines 204, 252, and 421 use any, which drops type safety on the error path and violates TypeScript guidelines.
Suggested change
+function getErrorMessage(err: unknown): string {
+ return err instanceof Error ? err.message : String(err);
+}
@@
- } catch (err: any) {
+ } catch (err: unknown) {
+ const message = getErrorMessage(err);
const failedAt = nowUnix();
await this.store.updateRun(run.id, {
status: 'failed',
- last_error: { code: 'EXEC_ERROR', message: err.message },
+ last_error: { code: 'EXEC_ERROR', message },
failed_at: failedAt,
});
@@
- } catch (err: any) {
+ } catch (err: unknown) {
+ const message = getErrorMessage(err);
if (!abortController.signal.aborted) {
try {
await this.store.updateRun(run.id, {
status: 'failed',
- last_error: { code: 'EXEC_ERROR', message: err.message },
+ last_error: { code: 'EXEC_ERROR', message },
failed_at: nowUnix(),
});
@@
- } catch (err: any) {
+ } catch (err: unknown) {
+ const message = getErrorMessage(err);
const failedAt = nowUnix();
try {
await this.store.updateRun(run.id, {
status: 'failed',
- last_error: { code: 'EXEC_ERROR', message: err.message },
+ last_error: { code: 'EXEC_ERROR', message },
failed_at: failedAt,
});
} catch (storeErr) {
console.error('[AgentController] failed to update run status after error:', storeErr);
}
@@
- runObj.last_error = { code: 'EXEC_ERROR', message: err.message };
+ runObj.last_error = { code: 'EXEC_ERROR', message };Per coding guidelines: "**/*.ts : Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown". Also applies to lines 252–258 and 421–427.
🤖 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 204 - 209, Change
the catch signatures from using `any` to `unknown` (e.g., in AgentRuntime's
catch blocks around the calls that call `this.store.updateRun`) and safely
extract an error message before using it: detect if the caught value is an Error
(err instanceof Error) and use err.message, otherwise coerce to a string
(String(err) or a fallback like 'Unknown error'); then pass that safe message
into `last_error.message` (and keep the same `code: 'EXEC_ERROR'` behavior).
Apply this pattern to the three catch sites referenced (the catch at the block
updating `run.id` with status 'failed' and the other two catch blocks around the
other `this.store.updateRun` calls) so no catch uses `any` and all error
messages are safely derived from an `unknown`.
| // Bypass Koa response handling — write SSE directly to the raw response | ||
| ctx.respond = false; | ||
| const res = ctx.res; | ||
| res.writeHead(200, { | ||
| 'Content-Type': 'text/event-stream', | ||
| 'Cache-Control': 'no-cache', | ||
| Connection: 'keep-alive', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "AgentRuntime.ts" | head -20Repository: eggjs/egg
Length of output: 98
🏁 Script executed:
wc -l ./tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 102
🏁 Script executed:
sed -n '280,310p' ./tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 1056
🏁 Script executed:
sed -n '283,350p' ./tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 2201
🏁 Script executed:
sed -n '350,400p' ./tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 1971
🏁 Script executed:
sed -n '400,450p' ./tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 1705
🏁 Script executed:
rg -n "ctx\.respond\s*=\s*false" tegg/core/agent-runtime/src/ -A 3 -B 3Repository: eggjs/egg
Length of output: 629
🏁 Script executed:
sed -n '283,450p' ./tegg/core/agent-runtime/src/AgentRuntime.ts | grep -E "(catch|finally|error|Error)" -i -B 2 -A 2Repository: eggjs/egg
Length of output: 1290
🏁 Script executed:
sed -n '283,350p' ./tegg/core/agent-runtime/src/AgentRuntime.ts | cat -nRepository: eggjs/egg
Length of output: 2677
🏁 Script executed:
sed -n '400,450p' ./tegg/core/agent-runtime/src/AgentRuntime.ts | cat -nRepository: eggjs/egg
Length of output: 2062
The ctx.respond = false bypass is necessary for SSE streaming but ensure error handling remains robust.
Setting ctx.respond = false to write directly to the raw response is required for Server-Sent Events and is the standard Koa pattern for streaming. However, this does bypass Koa's middleware-level error handlers. Verify that all error paths within streamRun (lines 351–448) are properly caught and communicated to the client. The current implementation includes try-catch-finally with error event sending, client disconnect handling via AbortController, and response closure guarantees, but document this architectural trade-off and ensure it remains comprehensive as the method evolves.
🤖 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 290 - 297, You are
bypassing Koa by setting ctx.respond = false to stream SSE in streamRun; ensure
every possible error path inside streamRun (including async operations,
AbortController cancellation, and any event-emitter callbacks) is caught and
turned into a single SSE "error" event before closing the raw res, and keep the
existing try-catch-finally/AbortController flow intact; also add a short in-code
comment near the ctx.respond = false line documenting the architectural
trade-off (bypasses Koa middleware error handlers) and referencing streamRun and
the AbortController so future maintainers know to update the SSE error-emission
logic if streamRun changes.
| async destroy(): Promise<void> { | ||
| // Wait for in-flight background tasks | ||
| if (this.runningTasks.size) { | ||
| const pending = Array.from(this.runningTasks.values()).map((t) => t.promise); | ||
| await Promise.allSettled(pending); | ||
| } |
There was a problem hiding this comment.
Abort running tasks before waiting in destroy().
destroy() currently waits on in-flight tasks without signalling cancellation first; long-running execRun generators can stall shutdown.
Suggested change
async destroy(): Promise<void> {
- // Wait for in-flight background tasks
+ // Abort and wait for in-flight background tasks
if (this.runningTasks.size) {
+ for (const task of this.runningTasks.values()) {
+ task.abortController.abort();
+ }
const pending = Array.from(this.runningTasks.values()).map((t) => t.promise);
await Promise.allSettled(pending);
}🤖 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 502 - 507, Before
awaiting in-flight tasks in destroy(), first signal cancellation to each running
task so long-running execRun generators can exit; iterate
this.runningTasks.values() and for each task call its cancellation mechanism
(e.g. task.controller.abort() if using an AbortController or task.abort() if an
abort method is provided), then collect task.promise and await
Promise.allSettled(pending) as before. Ensure you reference the existing
destroy() method and runningTasks map and handle both AbortController-style
(.abort()) and task-level (.abort()) APIs safely (guard for existence) before
awaiting.
| // Identify which methods are stubs vs user-defined | ||
| const stubMethods = new Set<string>(); | ||
| for (const name of AGENT_METHOD_NAMES) { | ||
| const method = clazz.prototype[name]; | ||
| if (!method || AgentInfoUtil.isNotImplemented(method)) { | ||
| stubMethods.add(name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail fast if execRun() is missing on an @AgentController class.
Enhancement currently proceeds even when the controller does not implement execRun, so the first call fails at runtime instead of at enhancement time.
Suggested change
// Identify which methods are stubs vs user-defined
+ if (typeof clazz.prototype.execRun !== 'function') {
+ throw new Error(`@AgentController class "${clazz.name}" must implement execRun(input, signal?)`);
+ }
+
const stubMethods = new Set<string>();📝 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.
| // Identify which methods are stubs vs user-defined | |
| const stubMethods = new Set<string>(); | |
| for (const name of AGENT_METHOD_NAMES) { | |
| const method = clazz.prototype[name]; | |
| if (!method || AgentInfoUtil.isNotImplemented(method)) { | |
| stubMethods.add(name); | |
| } | |
| } | |
| // Identify which methods are stubs vs user-defined | |
| if (typeof clazz.prototype.execRun !== 'function') { | |
| throw new Error(`@AgentController class "${clazz.name}" must implement execRun(input, signal?)`); | |
| } | |
| const stubMethods = new Set<string>(); | |
| for (const name of AGENT_METHOD_NAMES) { | |
| const method = clazz.prototype[name]; | |
| if (!method || AgentInfoUtil.isNotImplemented(method)) { | |
| stubMethods.add(name); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/enhanceAgentController.ts` around lines 34 - 41,
The enhancement must fail fast when an `@AgentController` class omits execRun:
after building stubMethods (from AGENT_METHOD_NAMES by checking clazz.prototype
and AgentInfoUtil.isNotImplemented), explicitly check if stubMethods contains
"execRun" and throw a clear error (or call a helper to abort enhancement)
indicating execRun is required on the controller class; reference the symbols
execRun, AGENT_METHOD_NAMES, stubMethods, clazz.prototype, and
AgentInfoUtil.isNotImplemented so you place the check immediately after the
stubMethods population.
| const dataDir = process.env.TEGG_AGENT_DATA_DIR || path.join(process.cwd(), '.agent-data'); | ||
| store = new FileAgentStore({ dataDir }); |
There was a problem hiding this comment.
Use app base directory for default store path, not process.cwd().
Line 51 ties storage to process working directory, which can differ from Egg app root under service managers and tooling.
Suggested change
- const dataDir = process.env.TEGG_AGENT_DATA_DIR || path.join(process.cwd(), '.agent-data');
+ const appBaseDir = typeof this.app?.baseDir === 'string' ? this.app.baseDir : process.cwd();
+ const dataDir = process.env.TEGG_AGENT_DATA_DIR || path.join(appBaseDir, '.agent-data');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dataDir = process.env.TEGG_AGENT_DATA_DIR || path.join(process.cwd(), '.agent-data'); | |
| store = new FileAgentStore({ dataDir }); | |
| const appBaseDir = typeof this.app?.baseDir === 'string' ? this.app.baseDir : process.cwd(); | |
| const dataDir = process.env.TEGG_AGENT_DATA_DIR || path.join(appBaseDir, '.agent-data'); | |
| store = new FileAgentStore({ dataDir }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/agent-runtime/src/enhanceAgentController.ts` around lines 51 - 52,
The default dataDir uses process.cwd(), which can be different from the Egg app
root; change the default to use the application base directory instead: when
computing dataDir (the variable passed into new FileAgentStore), prefer an
explicit app base directory (e.g. app.baseDir or a dedicated TEGG_APP_BASE_DIR
env var) over process.cwd(), while still honoring TEGG_AGENT_DATA_DIR if set;
update the code around dataDir and FileAgentStore instantiation in
enhanceAgentController to derive the path from the app/appInfo base directory
rather than process.cwd().
| // Let background task start running | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
Fixed setTimeout delay creates flaky test.
Using a fixed 50ms wait to let the background task start is timing-dependent and can fail on slow CI environments. Prefer polling with a timeout for deterministic behavior.
Based on learnings: "Use BackgroundTaskHelper instead of setTimeout/setImmediate for async tasks in tests" and PR review feedback mentioned "async tests use fixed setTimeout waits; prefer polling with timeout."
🔧 Suggested polling approach
- // Let background task start running
- await new Promise((resolve) => setTimeout(resolve, 50));
+ // Poll until run status changes to in_progress
+ const pollUntilInProgress = async (runId: string, timeoutMs = 2000): Promise<void> => {
+ const start = Date.now();
+ while (Date.now() - start < timeoutMs) {
+ const run = await store.getRun(runId);
+ if (run.status === 'in_progress') return;
+ await new Promise(r => setTimeout(r, 10));
+ }
+ throw new Error('Timed out waiting for run to start');
+ };
+ await pollUntilInProgress(result.id);🤖 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 268 - 269,
Replace the flaky fixed wait (await new Promise(resolve => setTimeout(resolve,
50))) with a polling approach that waits for the background task to reach the
expected state (use BackgroundTaskHelper if available, or poll a predicate with
a short interval and an overall timeout) so tests no longer rely on timing;
update the test in AgentRuntime.test.ts where the setTimeout is used to instead
repeatedly check the background task condition (e.g., a status flag, queue
length, or BackgroundTaskHelper.isIdle/hasStarted) until success or throw after
a reasonable timeout to fail deterministically.
| export function AgentController() { | ||
| return function (constructor: EggProtoImplClass) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tegg/core/controller-decorator/src/decorator/agent/AgentController.ts | head -120 | tail -40Repository: eggjs/egg
Length of output: 1666
🏁 Script executed:
head -30 tegg/core/controller-decorator/src/decorator/agent/AgentController.tsRepository: eggjs/egg
Length of output: 1268
🏁 Script executed:
grep -n "^export" tegg/core/controller-decorator/src/decorator/agent/AgentController.tsRepository: eggjs/egg
Length of output: 91
Add explicit return type to exported AgentController function.
Line 93 exports a function without a return type annotation, violating the isolatedDeclarations requirement.
Suggested change
-export function AgentController() {
+export function AgentController(): (constructor: EggProtoImplClass) => void {
return function (constructor: EggProtoImplClass) {🤖 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/AgentController.ts` around
lines 93 - 94, Add an explicit return type to the exported AgentController
function: change its signature to declare it returns a ClassDecorator (or the
explicit function type (constructor: EggProtoImplClass) => void). For example,
update `export function AgentController()` to `export function
AgentController(): ClassDecorator` (or `: (constructor: EggProtoImplClass) =>
void`) so the returned inner function signature `function (constructor:
EggProtoImplClass) { ... }` matches the annotated return type.
| // Set the fixed base HTTP path | ||
| HTTPInfoUtil.setHTTPPath('/api/v1', constructor); | ||
|
|
There was a problem hiding this comment.
Make the base path configurable instead of hardcoding /api/v1.
Line 99 hardcodes API prefix, which makes route versioning and multiple agent-controller mounts difficult.
Suggested direction
-export function AgentController(): (constructor: EggProtoImplClass) => void {
+interface AgentControllerOptions {
+ basePath?: string;
+}
+
+export function AgentController(options: AgentControllerOptions = {}): (constructor: EggProtoImplClass) => void {
return function (constructor: EggProtoImplClass) {
+ const basePath = options.basePath ?? '/api/v1';
ControllerInfoUtil.setControllerType(constructor, ControllerType.HTTP);
- HTTPInfoUtil.setHTTPPath('/api/v1', constructor);
+ HTTPInfoUtil.setHTTPPath(basePath, constructor);🤖 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/AgentController.ts` around
lines 98 - 100, The code currently hardcodes the API base path via
HTTPInfoUtil.setHTTPPath('/api/v1', constructor) in AgentController.ts; change
this to use a configurable value (e.g., an options parameter to the
AgentController decorator or a clearly named env var) instead of the literal
string. Read the basePath from the decorator options (falling back to a sensible
default like '/api/v1' if not provided) and pass that variable into
HTTPInfoUtil.setHTTPPath(basePath, constructor); update the AgentController
decorator signature to accept and propagate the options and ensure any callers
supply or inherit the desired basePath.
…atic methods - Move TERMINAL_RUN_STATUSES, toContentBlocks, toMessageObject, extractFromStreamMessages, toInputMessageObjects from module scope into AgentRuntime class as private static members - Use atomic write (write-tmp-then-rename) in FileAgentStore to prevent data corruption on process crash - Remove misleading comment suggesting FileAgentStore is not for production Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tegg/core/agent-runtime/src/AgentRuntime.ts (1)
110-128:⚠️ Potential issue | 🟡 MinorMissing type filter when mapping content array.
toInputMessageObjectsmaps all items inm.contentwithout filtering fortype === 'text'first. If non-text content parts exist,p.textwill be undefined.🛡️ Proposed defensive fix
content: typeof m.content === 'string' ? [{ type: 'text' as const, text: { value: m.content, annotations: [] } }] - : m.content.map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })), + : m.content + .filter((p): p is typeof p & { text: string } => p.type === 'text' && typeof p.text === 'string') + .map((p) => ({ type: 'text' as const, text: { value: p.text, annotations: [] } })),🤖 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 110 - 128, toInputMessageObjects currently maps m.content array blindly and assumes each part has p.text; update the mapping to filter out non-text parts first (i.e. only keep parts where part.type === 'text') before mapping to the MessageObject text shape so p.text is defined, and ensure the resulting content array is empty-handled (e.g., skip the message or convert to a single text part) to avoid producing MessageObject entries with undefined text; change the logic inside toInputMessageObjects that builds content from m.content to use a filter + map on m.content and reference the m.content and MessageObject construction paths when applying the fix.
🧹 Nitpick comments (1)
tegg/core/agent-runtime/src/FileAgentStore.ts (1)
102-107: Consider explicitly omittingthread_idfrom safe updates.The destructuring strips
idandobject, butthread_idis also immutable per the RunRecord contract and should likely be excluded as well.♻️ Proposed fix
async updateRun(runId: string, updates: Partial<RunRecord>): Promise<void> { const run = await this.getRun(runId); - const { id: _, object: __, ...safeUpdates } = updates; + const { id: _, object: __, thread_id: ___, ...safeUpdates } = updates; Object.assign(run, safeUpdates); await this.writeFile(this.safePath(this.runsDir, runId), run); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/src/FileAgentStore.ts` around lines 102 - 107, The updateRun method currently strips id and object but allows thread_id to be mutated; update the safe-updates logic in updateRun to also omit thread_id from updates (e.g., extend the destructuring that creates safeUpdates to include thread_id, or explicitly delete thread_id from safeUpdates) so that RunRecord's immutable thread_id cannot be overwritten before Object.assign and the subsequent writeFile to this.safePath(this.runsDir, runId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tegg/core/agent-runtime/src/AgentRuntime.ts`:
- Around line 110-128: toInputMessageObjects currently maps m.content array
blindly and assumes each part has p.text; update the mapping to filter out
non-text parts first (i.e. only keep parts where part.type === 'text') before
mapping to the MessageObject text shape so p.text is defined, and ensure the
resulting content array is empty-handled (e.g., skip the message or convert to a
single text part) to avoid producing MessageObject entries with undefined text;
change the logic inside toInputMessageObjects that builds content from m.content
to use a filter + map on m.content and reference the m.content and MessageObject
construction paths when applying the fix.
---
Nitpick comments:
In `@tegg/core/agent-runtime/src/FileAgentStore.ts`:
- Around line 102-107: The updateRun method currently strips id and object but
allows thread_id to be mutated; update the safe-updates logic in updateRun to
also omit thread_id from updates (e.g., extend the destructuring that creates
safeUpdates to include thread_id, or explicitly delete thread_id from
safeUpdates) so that RunRecord's immutable thread_id cannot be overwritten
before Object.assign and the subsequent writeFile to this.safePath(this.runsDir,
runId).
…ate.now() The test assertions were comparing created_at (Unix seconds) against Date.now() (milliseconds), which always passes and validates nothing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tegg/core/agent-runtime/test/FileAgentStore.test.ts (1)
53-63: Add regression tests for invalid IDs/path traversal in store access paths.Current negative tests validate missing records, but they don’t lock behavior for invalid IDs like empty string or
../..., which are explicitly guarded integg/core/agent-runtime/src/FileAgentStore.ts(safePath).Suggested test additions
+ it('should reject invalid thread id inputs', async () => { + await assert.rejects(() => store.getThread(''), /Invalid id/); + await assert.rejects(() => store.getThread('../evil'), /Invalid id/); + }); + + it('should reject invalid run id inputs', async () => { + await assert.rejects(() => store.getRun(''), /Invalid id/); + await assert.rejects(() => store.getRun('../evil'), /Invalid id/); + });Also applies to: 136-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts` around lines 53 - 63, Add regression tests to FileAgentStore.test.ts that assert FileAgentStore's safePath enforcement by verifying methods like getThread and getAgent (and any other store accessors used in the file) reject invalid IDs such as empty string and path-traversal inputs (e.g., '../etc' or '../../secret') with the appropriate error (AgentNotFoundError or a specific validation error) and status code; update the existing negative test blocks (the one using getThread around lines 53-63 and the similar block around lines 136-146) to include additional asserts that calling store.getThread('') and store.getThread('../bad') (and analogously store.getAgent('') / store.getAgent('../bad')) rejects via assert.rejects and verifies the error type, status, and message pattern indicating unsafe/invalid path as enforced by FileAgentStore.safePath.tegg/core/agent-runtime/test/AgentRuntime.test.ts (1)
79-209: Add explicit failure-path tests forsyncRunandasyncRun.Happy-path coverage is good, but missing cases where
execRun()throws (and where store update on failure path is attempted) leaves status/error transition behavior under-tested.🤖 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 79 - 209, Add tests that cover failure paths by mocking execRun to throw and asserting the runtime transitions the run to a failed state and records the error; specifically, for runtime.syncRun and runtime.asyncRun simulate execRun throwing (for async, wait via runtime.destroy), then verify store.getRun(result.id) shows run.status === 'failed', run.error contains the thrown error/message, timestamps (completed_at) are set, and that store.updateRun was invoked with the failure update. Locate and modify tests referencing runtime.syncRun, runtime.asyncRun, execRun, store.getRun, and store.updateRun to add 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/test/AgentRuntime.test.ts`:
- Around line 22-37: The test fixture uses broad any casts; change
execRun(input: any) to execRun(input: CreateRunInput) and remove the "as any"
cast on the host fixture (the object passed to new AgentRuntime(host, store)) so
the host conforms to AgentControllerHost, ensuring runtime is constructed with
correctly typed host and store; also remove the "as any" casts from the various
test call sites that pass input objects (the CreateRunInput mocks used where
execRun is invoked) so they are typed as CreateRunInput instead of any—update
the test fixtures/mocks to satisfy the CreateRunInput and AgentControllerHost
interfaces rather than using any/unknown casts.
---
Nitpick comments:
In `@tegg/core/agent-runtime/test/AgentRuntime.test.ts`:
- Around line 79-209: Add tests that cover failure paths by mocking execRun to
throw and asserting the runtime transitions the run to a failed state and
records the error; specifically, for runtime.syncRun and runtime.asyncRun
simulate execRun throwing (for async, wait via runtime.destroy), then verify
store.getRun(result.id) shows run.status === 'failed', run.error contains the
thrown error/message, timestamps (completed_at) are set, and that
store.updateRun was invoked with the failure update. Locate and modify tests
referencing runtime.syncRun, runtime.asyncRun, execRun, store.getRun, and
store.updateRun to add these assertions.
In `@tegg/core/agent-runtime/test/FileAgentStore.test.ts`:
- Around line 53-63: Add regression tests to FileAgentStore.test.ts that assert
FileAgentStore's safePath enforcement by verifying methods like getThread and
getAgent (and any other store accessors used in the file) reject invalid IDs
such as empty string and path-traversal inputs (e.g., '../etc' or
'../../secret') with the appropriate error (AgentNotFoundError or a specific
validation error) and status code; update the existing negative test blocks (the
one using getThread around lines 53-63 and the similar block around lines
136-146) to include additional asserts that calling store.getThread('') and
store.getThread('../bad') (and analogously store.getAgent('') /
store.getAgent('../bad')) rejects via assert.rejects and verifies the error
type, status, and message pattern indicating unsafe/invalid path as enforced by
FileAgentStore.safePath.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tegg/core/agent-runtime/test/AgentRuntime.test.tstegg/core/agent-runtime/test/FileAgentStore.test.tstegg/plugin/controller/test/http/base-agent.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tegg/plugin/controller/test/http/base-agent.test.ts
| async *execRun(input: any) { | ||
| const messages = input.input.messages; | ||
| yield { | ||
| type: 'assistant', | ||
| message: { | ||
| role: 'assistant', | ||
| content: [{ type: 'text', text: `Hello ${messages.length} messages` }], | ||
| }, | ||
| }; | ||
| yield { | ||
| type: 'result', | ||
| usage: { prompt_tokens: 10, completion_tokens: 5 }, | ||
| }; | ||
| }, | ||
| } as any; | ||
| runtime = new AgentRuntime(host, store); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and get its structure
fd -t f "AgentRuntime.test.ts" tegg/Repository: eggjs/egg
Length of output: 102
🏁 Script executed:
# Search for all 'any' type usages in the test file
rg -nP '\bas\s+any\b|:\s+any\b' tegg/core/agent-runtime/test/AgentRuntime.test.ts --context=2Repository: eggjs/egg
Length of output: 3184
🏁 Script executed:
# Get line count to verify line numbers are within bounds
wc -l tegg/core/agent-runtime/test/AgentRuntime.test.tsRepository: eggjs/egg
Length of output: 106
🏁 Script executed:
# Check the AgentRuntime source to understand what the host type should be
fd -t f "AgentRuntime.ts" tegg/core/agent-runtime/ --exclude="*.test.ts"Repository: eggjs/egg
Length of output: 96
🏁 Script executed:
# Look for the AgentControllerHost interface definition
rg -n "interface AgentControllerHost|type AgentControllerHost|class AgentControllerHost" tegg/core/agent-runtime/ --type tsRepository: eggjs/egg
Length of output: 138
🏁 Script executed:
# Check the exact method signature that these tests are calling
rg -A5 "syncRun\(|asyncRun\(" tegg/core/agent-runtime/src/agent-runtime.ts | head -40Repository: eggjs/egg
Length of output: 137
🏁 Script executed:
# Get the interface definition for AgentControllerHost
rg -A10 "export interface AgentControllerHost" tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 515
🏁 Script executed:
# Check the method signatures for syncRun and asyncRun
rg -B2 -A5 "syncRun\(|asyncRun\(" tegg/core/agent-runtime/src/AgentRuntime.ts | head -50Repository: eggjs/egg
Length of output: 564
🏁 Script executed:
# Look at the actual run input interface
rg -n "interface.*Run.*Input|type.*Run.*Input" tegg/core/agent-runtime/src/Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Find CreateRunInput type definition
rg -B5 -A10 "type CreateRunInput|interface CreateRunInput" tegg/core/agent-runtime/src/Repository: eggjs/egg
Length of output: 35
🏁 Script executed:
# Check imports in AgentRuntime.ts to see where CreateRunInput comes from
head -30 tegg/core/agent-runtime/src/AgentRuntime.tsRepository: eggjs/egg
Length of output: 1021
🏁 Script executed:
# Check if CreateRunInput has optional properties or if the test inputs match the interface
rg -B3 -A15 "export.*type CreateRunInput|export.*interface CreateRunInput" --type ts | grep -A15 "CreateRunInput"Repository: eggjs/egg
Length of output: 1426
🏁 Script executed:
# Check what fields are being passed in test vs what CreateRunInput expects
cat -n tegg/core/agent-runtime/test/AgentRuntime.test.ts | sed -n '80,85p;105,110p'Repository: eggjs/egg
Length of output: 621
🏁 Script executed:
# Verify that CreateRunInput can properly type the test inputs
cat -n tegg/core/agent-runtime/test/AgentRuntime.test.ts | sed -n '21,37p'Repository: eggjs/egg
Length of output: 654
🏁 Script executed:
# Check if there are similar patterns in other test files
fd -t f "*.test.ts" tegg/ -x grep -l "as any" {} \; | head -5Repository: eggjs/egg
Length of output: 341
🏁 Script executed:
# Check what imports are available at the top of the test file
head -20 tegg/core/agent-runtime/test/AgentRuntime.test.tsRepository: eggjs/egg
Length of output: 797
🏁 Script executed:
# Verify if CreateRunInput is imported or available in scope
rg "CreateRunInput|AgentControllerHost" tegg/core/agent-runtime/test/AgentRuntime.test.tsRepository: eggjs/egg
Length of output: 152
Replace any type annotations with proper types in test fixtures.
The test file imports AgentControllerHost but casts the host fixture as any at line 36, and repeatedly casts input objects as any at call sites (lines 83, 107, 118, 130, 141, 157, 181, 199, 215, 229, 239, 262). Additionally, execRun parameter at line 22 uses input: any instead of input: CreateRunInput. These bypass TypeScript's type checking when the required types (CreateRunInput, AgentControllerHost) are already properly defined and imported. Replace any with the correct types:
- Line 22:
input: CreateRunInput - Line 36: Remove
as anycast onceexecRunis properly typed - Lines 83, 107, 118, 130, 141, 157, 181, 199, 215, 229, 239, 262, 266: Remove
as anycasts from input objects
Per coding guidelines: "Avoid 'any' type in TypeScript; use 'unknown' when type is truly unknown."
🤖 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 22 - 37, The
test fixture uses broad any casts; change execRun(input: any) to execRun(input:
CreateRunInput) and remove the "as any" cast on the host fixture (the object
passed to new AgentRuntime(host, store)) so the host conforms to
AgentControllerHost, ensuring runtime is constructed with correctly typed host
and store; also remove the "as any" casts from the various test call sites that
pass input objects (the CreateRunInput mocks used where execRun is invoked) so
they are typed as CreateRunInput instead of any—update the test fixtures/mocks
to satisfy the CreateRunInput and AgentControllerHost interfaces rather than
using any/unknown casts.
| /** | ||
| * Convert an AgentStreamMessage's message field into OpenAI MessageContentBlock[]. | ||
| */ | ||
| private static toContentBlocks(msg: AgentStreamMessage['message']): MessageContentBlock[] { |
There was a problem hiding this comment.
这个方法应该和 AgentRuntime 没关系？需要单独抽一个类做 message 的类型转换。
| /** | ||
| * Convert an AgentStreamMessage's message field into OpenAI MessageContentBlock[]. | ||
| */ | ||
| private static toContentBlocks(msg: AgentStreamMessage['message']): MessageContentBlock[] { |
There was a problem hiding this comment.
AgentStreamMessage['message'] 用 type 定义出来，别用到处用匿名类型。 类似
type AgentStreamMessagePayload = AgentStreamMessage['message']| } | ||
| if (msg.usage) { | ||
| hasUsage = true; | ||
| promptTokens += msg.usage.prompt_tokens ?? 0; |
There was a problem hiding this comment.
是否应该是处理的 tomessageobject 之后的对象，逻辑中还有 snake 命名的很奇怪。
There was a problem hiding this comment.
主要这里的字段都是跟OpenAI对应的，是希望只在透出的那一层再转化回来吗？我们自己存储和消费的时候都转化成驼峰？这样message看起来我都要遍历一遍，洗这个字段。
| usage = { | ||
| prompt_tokens: promptTokens, | ||
| completion_tokens: completionTokens, | ||
| total_tokens: Math.max(promptTokens + completionTokens, totalTokens), |
There was a problem hiding this comment.
需要补充下注释，什么时候会 prompt + completion > total
|
|
||
| try { | ||
| const startedAt = nowUnix(); | ||
| await this.store.updateRun(run.id, { status: 'in_progress', started_at: startedAt }); |
There was a problem hiding this comment.
同上，snake 的风格不要泄漏到代码中。保持在存储内部。
| for await (const msg of this.host.execRun(input)) { | ||
| streamMessages.push(msg); | ||
| } |
There was a problem hiding this comment.
这个当前先简化？后面作为follow-up来补充呢，因为目前是否使用我们提供的会话历史获取功能还不确定，先只考虑成功的情况？
| object: 'thread.message', | ||
| created_at: nowUnix(), | ||
| run_id: run.id, | ||
| role: 'assistant', |
| }; | ||
| } | ||
|
|
||
| async streamRun(input: CreateRunInput): Promise<void> { |
There was a problem hiding this comment.
这个方法写了太多内容，是否要拆分 store 和 stream
| // event: thread.run.failed | ||
| runObj.status = 'failed'; | ||
| runObj.failed_at = failedAt; | ||
| runObj.last_error = { code: 'EXEC_ERROR', message: err.message }; |
There was a problem hiding this comment.
需要考虑建模，包成一个方法，而不是写三个赋值。
例如
class Run {
execFail(error: Error) {
this.status = RUN_STAUTS.FAILED;
this.failedAt = Date.now();
this.lastError = xxx
}
}| // Wait for in-flight background tasks | ||
| if (this.runningTasks.size) { | ||
| const pending = Array.from(this.runningTasks.values()).map((t) => t.promise); | ||
| await Promise.allSettled(pending); |
|
|
||
| // Wrap init() lifecycle to create AgentRuntime | ||
| const originalInit = clazz.prototype.init; | ||
| clazz.prototype.init = async function () { |
There was a problem hiding this comment.
最好不要去修改 prototype。可以参考下 orm 中的 SingletonModelProto/SingletonModelObject 动态生成一个新的对象。在这个对象中去转发实现。
例如
class DefaultOpenAIController {
xxx() {
let impl = Reflect.get(this.controllerImpl, 'xxx');
if (impl) return Reflect.apply;
return this.defaultImpl();
}
}…rrorCode constants - Replace RunStatus union type with const object + type alias pattern - Add AgentSSEEvent and AgentErrorCode const object definitions - Add AgentStreamMessagePayload type alias - Extract 4 message conversion functions from AgentRuntime into standalone MessageConverter module (toContentBlocks, toMessageObject, extractFromStreamMessages, toInputMessageObjects) - Fix total_tokens calculation: use direct addition instead of Math.max - Replace all string literals with constant references in AgentRuntime and FileAgentStore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ransitions Extract RunObject construction and state mutation logic into a dedicated RunBuilder class. This replaces scattered inline object literals and field assignments with explicit state transition methods (start, complete, fail, cancel, snapshot), making the run lifecycle clearer and less error-prone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rom HTTP
- Define SSEWriter interface (writeEvent, closed, end, onClose) as an
abstract SSE transport layer
- Implement NodeSSEWriter backed by node http.ServerResponse
- Refactor AgentRuntime.streamRun to accept SSEWriter parameter instead
of accessing ctx.res directly, removing ContextHandler/EGG_CONTEXT
dependency from AgentRuntime
- Move HTTP context handling (ctx.respond, NodeSSEWriter creation) to
enhanceAgentController's streamRun stub
- Use res.once('close') instead of res.on('close') for client disconnect
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Change AgentRuntime constructor to accept AgentRuntimeOptions object with host, store, and optional logger fields - Define AgentRuntimeLogger interface for logger abstraction - Replace all console.error calls with this.logger.error - Default to console when no logger is provided - Update enhanceAgentController and tests for new constructor signature Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rototype Move stub method replacement from prototype-level to instance-level. Previously, enhanceAgentController replaced stub methods directly on clazz.prototype, which affected all instances. Now, delegate methods are installed on each instance during init(), leaving the original prototype untouched. Also adds createAgentRuntime factory function which resolves the pre-existing TS2556 spread argument type error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace enhanceAgentController with AgentControllerProto/AgentControllerObject pattern following ORM's SingletonModelProto/SingletonModelObject design - Eliminate all `any` usage; use Record<string|symbol, unknown> and Reflect.apply - Inject egg logger via AgentRuntimeLogger interface instead of console - Add camelCase conversion layer: RunBuilder uses camelCase internally, converts to snake_case only at store/API boundaries - Extract consumeStreamMessages from streamRun for better readability - Use abort-first strategy in destroy() for faster graceful shutdown - Add AGENT_CONTROLLER_PROTO_IMPL_TYPE for custom proto/object creation - Delete enhanceAgentController.ts and its test (prototype patching removed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
@AgentControllerdecorator that auto-generates 7 OpenAI Assistants API-compatible HTTP endpoints (threads, runs, streaming)@eggjs/agent-runtimepackage withFileAgentStore, pluggable storage abstraction, and smart default implementationsexecRun()to get a fully functional AI agent API with SSE streaming, async/sync execution, and cancellation supportChanges
New package:
@eggjs/agent-runtimeAgentStoreinterface for pluggable storage (threads & runs persistence)FileAgentStore— default file-based implementation for developmentagentDefaults— smart default implementations for all 7 routesenhanceAgentController— runtime enhancement applied viaEggControllerPrototypeHookcontroller-decorator extensions
@AgentController()decorator — marks class and auto-generates route metadataAgentHandlerinterface — defines the contract for agent controllersAgentControllerTypes— full OpenAI-compatible type definitions (ThreadObject, RunObject, MessageObject, etc.)Integration
EggControllerPrototypeHookenhanced to detect and process@AgentControllerclasses@eggjs/tegg/agententry pointGenerated HTTP endpoints
/api/v1/threads/api/v1/threads/:id/api/v1/runs/api/v1/runs/stream/api/v1/runs/wait/api/v1/runs/:id/api/v1/runs/:id/cancelTest plan
@AgentControllerdecorator metadataFileAgentStore(CRUD, persistence, error handling)agentDefaults(all 7 route defaults, usage aggregation, thread management)enhanceAgentController(placeholder replacement, init/destroy wrapping, custom store)agent-controller-app) — all routes manually implementedbase-agent-controller-app) — onlyexecRunimplemented, SSE streaming, cancellation, metadata persistence🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests