feat(agent-runtime): support run thread metadata#454
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughValidates run metadata in AgentRuntime; adds AgentStore.updateThreadMetadata and OSSAgentStore per-thread serialized metadata updates; updates recordLatestRunId to avoid intra-process clobbering; expands tests and JSDoc; and adds many plugin test fixture modules. ChangesThread metadata validation and persistence
Plugin and test fixtures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for persisting and updating thread-specific metadata independently of run metadata. It adds validation for thread metadata, implements the updateThreadMetadata method in OSSAgentStore with concurrent write serialization, and includes comprehensive unit tests. The review feedback highlights two key areas for improvement: a critical bug in the promise chaining of updateThreadMetadata where a single failed update can permanently block all subsequent updates for that thread, and a recommendation to restrict validateThreadMetadata to plain objects to prevent serialization issues with types like Date or Map.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+186
to
+212
| const previous = this.threadMetadataWriteTails.get(threadId) ?? Promise.resolve(); | ||
| let release!: () => void; | ||
| const current = new Promise<void>(resolve => { | ||
| release = resolve; | ||
| }); | ||
| const tail = previous.then(() => current); | ||
| this.threadMetadataWriteTails.set(threadId, tail); | ||
|
|
||
| await previous; | ||
| try { | ||
| const metaData = await this.client.get(this.threadMetaKey(threadId)); | ||
| if (!metaData) { | ||
| throw new AgentNotFoundError(`Thread ${threadId} not found`); | ||
| } | ||
| const meta = JSON.parse(metaData) as ThreadMetadata; | ||
| const mergedMetadata = { ...meta.metadata, ...metadata }; | ||
| await this.client.put(this.threadMetaKey(threadId), JSON.stringify({ | ||
| ...meta, | ||
| metadata: mergedMetadata, | ||
| })); | ||
| this.writeThreadActivityIndex(threadId, meta.createdAt, Date.now(), mergedMetadata); | ||
| } finally { | ||
| release(); | ||
| if (this.threadMetadataWriteTails.get(threadId) === tail) { | ||
| this.threadMetadataWriteTails.delete(threadId); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
There is a critical bug in the promise chaining for updateThreadMetadata.
If a previous metadata update fails (e.g., due to a transient network error or because the thread was not found), the previous promise will be rejected. Because tail is constructed as previous.then(() => current), a rejection of previous means tail will also immediately reject, and the then callback (which resolves current) is never executed.
Furthermore, because await previous is located outside the try...finally block, any subsequent call to updateThreadMetadata will retrieve the rejected tail as its previous, throw the previous error immediately at await previous, and bypass the try block entirely. This means release() is never called, and the tail is never deleted from threadMetadataWriteTails.
This permanently bricks all future metadata updates for that threadId once a single update fails.
To fix this, ensure that the promise chain continues even if a previous promise rejects, by catching the rejection of previous before chaining or awaiting it.
const previous = this.threadMetadataWriteTails.get(threadId) ?? Promise.resolve();
let release!: () => void;
const current = new Promise<void>(resolve => {
release = resolve;
});
const tail = previous.catch(() => {}).then(() => current);
this.threadMetadataWriteTails.set(threadId, tail);
await previous.catch(() => {});
try {
const metaData = await this.client.get(this.threadMetaKey(threadId));
if (!metaData) {
throw new AgentNotFoundError(`Thread ${threadId} not found`);
}
const meta = JSON.parse(metaData) as ThreadMetadata;
const mergedMetadata = { ...meta.metadata, ...metadata };
await this.client.put(this.threadMetaKey(threadId), JSON.stringify({
...meta,
metadata: mergedMetadata,
}));
this.writeThreadActivityIndex(threadId, meta.createdAt, Date.now(), mergedMetadata);
} finally {
release();
if (this.threadMetadataWriteTails.get(threadId) === tail) {
this.threadMetadataWriteTails.delete(threadId);
}
}
Comment on lines
+36
to
+42
| function validateThreadMetadata(value: unknown): Record<string, unknown> | undefined { | ||
| if (value === undefined) return undefined; | ||
| if (value === null || typeof value !== 'object' || Array.isArray(value)) { | ||
| throw new AgentInvalidRequestError("'threadMetadata' must be an object"); | ||
| } | ||
| return value as Record<string, unknown>; | ||
| } |
Contributor
There was a problem hiding this comment.
The validateThreadMetadata function checks if the value is an object, but it does not verify if it is a plain object. Non-plain objects (such as Date, Map, Set, or class instances) will pass this check but may cause issues during shallow merging or JSON serialization (e.g., Date has no enumerable properties and will merge as an empty object, while Map/Set serialize to empty objects {}).
Consider enforcing that the input is a plain object.
function validateThreadMetadata(value: unknown): Record<string, unknown> | undefined {
if (value === undefined) return undefined;
if (
value === null ||
typeof value !== 'object' ||
Array.isArray(value) ||
Object.prototype.toString.call(value) !== '[object Object]'
) {
throw new AgentInvalidRequestError("'threadMetadata' must be a plain object");
}
return value as Record<string, unknown>;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/agent-runtime/src/AgentRuntime.ts`:
- Around line 161-163: Replace the generic Error thrown in AgentRuntime when
updateThreadMetadata is unsupported with a typed request/client error to
preserve API semantics: in the check that references
this.store.updateThreadMetadata inside AgentRuntime (the current "throw new
Error('AgentStore does not support updating thread metadata')"), throw a
BadRequest/RequestError (e.g., RequestError or BadRequestError) instead, import
the chosen error type from your shared errors module, and use it with the same
message so the caller receives a 4xx-style error rather than an HTTP 500.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a07df22-720b-4264-9fc8-2539af309de4
📒 Files selected for processing (7)
core/agent-runtime/src/AgentRuntime.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/test/AgentRuntime.metadata.test.tscore/agent-runtime/test/OSSAgentStore.test.tscore/types/agent-runtime/AgentRuntime.tscore/types/agent-runtime/AgentStore.tscore/types/agent-runtime/errors.ts
Comment on lines
+161
to
+163
| if (!this.store.updateThreadMetadata) { | ||
| throw new Error('AgentStore does not support updating thread metadata'); | ||
| } |
There was a problem hiding this comment.
Use a typed request error instead of a generic Error for unsupported metadata updates.
Line 162 currently throws a generic Error, which likely maps to HTTP 500 even though this is a client-input pathway (threadMetadata on an existing thread). Return a typed error to keep API semantics consistent.
Suggested patch
- if (!this.store.updateThreadMetadata) {
- throw new Error('AgentStore does not support updating thread metadata');
- }
+ if (!this.store.updateThreadMetadata) {
+ throw new AgentInvalidRequestError(
+ "'threadMetadata' is not supported for existing threads by the configured AgentStore",
+ );
+ }📝 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.
Suggested change
| if (!this.store.updateThreadMetadata) { | |
| throw new Error('AgentStore does not support updating thread metadata'); | |
| } | |
| if (!this.store.updateThreadMetadata) { | |
| throw new AgentInvalidRequestError( | |
| "'threadMetadata' is not supported for existing threads by the configured AgentStore", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/agent-runtime/src/AgentRuntime.ts` around lines 161 - 163, Replace the
generic Error thrown in AgentRuntime when updateThreadMetadata is unsupported
with a typed request/client error to preserve API semantics: in the check that
references this.store.updateThreadMetadata inside AgentRuntime (the current
"throw new Error('AgentStore does not support updating thread metadata')"),
throw a BadRequest/RequestError (e.g., RequestError or BadRequestError) instead,
import the chosen error type from your shared errors module, and use it with the
same message so the caller receives a 4xx-style error rather than an HTTP 500.
jerryliang64
commented
Jun 10, 2026
| }; | ||
| const result = await runtime.syncRun({ | ||
| input: { messages: [{ role: 'user', content: 'Hi' }] }, | ||
| threadMetadata, |
Contributor
Author
There was a problem hiding this comment.
字段命名应该是 threadMetadata => metadata
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/agent-runtime/src/AgentRuntime.ts (1)
163-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
AgentInvalidRequestErrorinstead of genericErrorfor this request-path failure.Line 164 throws a generic
Error, which can surface as 5xx for a client-provided metadata path. This should stay consistent with the 4xx metadata validation contract.Suggested patch
if (!this.store.updateThreadMetadata) { - throw new Error('AgentStore does not support updating thread metadata'); + throw new AgentInvalidRequestError( + 'AgentStore does not support updating thread metadata', + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/agent-runtime/src/AgentRuntime.ts` around lines 163 - 165, Replace the generic Error thrown when the store lacks updateThreadMetadata with an AgentInvalidRequestError so the missing-client-provided-metadata path surfaces as a 4xx; locate the check in AgentRuntime (the conditional that tests this.store.updateThreadMetadata) and throw new AgentInvalidRequestError(...) with a clear message (e.g., 'AgentStore does not support updating thread metadata') instead of new Error(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@core/agent-runtime/src/AgentRuntime.ts`:
- Around line 163-165: Replace the generic Error thrown when the store lacks
updateThreadMetadata with an AgentInvalidRequestError so the
missing-client-provided-metadata path surfaces as a 4xx; locate the check in
AgentRuntime (the conditional that tests this.store.updateThreadMetadata) and
throw new AgentInvalidRequestError(...) with a clear message (e.g., 'AgentStore
does not support updating thread metadata') instead of new Error(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 987fd8a3-c425-473c-aefb-5b442d165219
📒 Files selected for processing (3)
core/agent-runtime/src/AgentRuntime.tscore/agent-runtime/test/AgentRuntime.metadata.test.tscore/types/agent-runtime/AgentRuntime.ts
✅ Files skipped from review due to trivial changes (1)
- core/types/agent-runtime/AgentRuntime.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core/agent-runtime/test/AgentRuntime.metadata.test.ts
jerryliang64
force-pushed
the
agent/dev/4a73189b
branch
from
1f03a1d to
543644c
Compare
Co-authored-by: multica-agent <github@multica.ai>
jerryliang64
force-pushed
the
agent/dev/4a73189b
branch
from
543644c to
ff71f08
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/tegg/test/fixtures/apps/background-app/app/controller/app.js`:
- Around line 15-20: The controller method is misspelled as backgroudTimeout;
rename it to backgroundTimeout in the source TypeScript controller so the
exported/compiled JS matches the intended route handler. Update the method
declaration (currently backgroudTimeout) to backgroundTimeout and ensure any
references/callers (e.g., routes or tests that invoke backgroudTimeout) are
updated accordingly; the function that uses
BackgroundService_1.default.backgroundAdd should remain unchanged.
In `@plugin/tegg/test/fixtures/apps/background-app/app/router.js`:
- Line 5: The route path contains a typo: change the route registered as
app.router.get('/backgroudTimeout', app.controller.app.backgroudTimeout) to use
the correct path '/backgroundTimeout' and rename the controller method from
backgroudTimeout to backgroundTimeout in the source TypeScript controller;
update all references (e.g., any uses of app.controller.app.backgroudTimeout) to
app.controller.app.backgroundTimeout so the route and handler names match.
In `@plugin/tegg/test/fixtures/apps/constructor-module-config/app.js`:
- Around line 9-10: The boot hook mutates a deep path without ensuring the full
object chain exists: before writing to
this.app.moduleConfigs.overwrite.config.features.dynamic.bar (in the boot hook
that checks this.app.moduleConfigs.overwrite.config), add a defensive guard or
initialize missing intermediate objects (overwrite, config, features, dynamic)
so the assignment cannot throw; either check the existence of
overwrite.config.features.dynamic or create those nested objects on
this.app.moduleConfigs.overwrite.config prior to setting .bar.
In
`@plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/FooSingletonHello.js`:
- Around line 14-22: The class FooSingletonHello incorrectly extends
AbstractContextHello; change its superclass to AbstractSingletonHello so the
`@SingletonProto`()/@SingletonHello(FOO) decorated implementation matches the
expected abstraction used by SingletonHelloService. Locate the FooSingletonHello
class definition and replace the extends AbstractContextHello reference with
extends AbstractSingletonHello, ensuring imports/aliases for
AbstractSingletonHello are adjusted if necessary so the symbol resolves
correctly.
In `@plugin/tegg/test/fixtures/apps/optional-module/app/modules/root/Root.js`:
- Line 14: Replace the incorrect tegg decorator import in Root.js (currently
requiring "`@eggjs/core-decorator`") with the correct "`@eggjs/tegg`" package and
update uses of the decorators accordingly: change the import that provides
Inject and SingletonProto to require("`@eggjs/tegg`") and ensure the decorator
identifiers used on the Root class and its properties (references to Inject and
SingletonProto) are the ones exported from "`@eggjs/tegg`" so the Root class (and
property decorators around lines where Inject and SingletonProto are applied)
use the correct tegg decorator implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d65be364-9359-457f-a065-adf5a58a6d2c
📒 Files selected for processing (98)
core/agent-runtime/src/AgentRuntime.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/test/AgentRuntime.metadata.test.tscore/agent-runtime/test/OSSAgentStore.test.tscore/types/agent-runtime/AgentRuntime.tscore/types/agent-runtime/AgentStore.tscore/types/agent-runtime/errors.tsplugin/ajv/test/fixtures/apps/ajv-app/modules/demo/FooController.jsplugin/dal/test/fixtures/apps/dal-app/modules/dal/Foo.jsplugin/dal/test/fixtures/apps/dal-app/modules/dal/FooService.jsplugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/FooDAO.jsplugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/base/BaseFooDAO.jsplugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/extension/FooExtension.jsplugin/tegg/test/fixtures/apps/access-level-check/app/controller/app.jsplugin/tegg/test/fixtures/apps/access-level-check/app/router.jsplugin/tegg/test/fixtures/apps/access-level-check/modules/module-a/BarService.jsplugin/tegg/test/fixtures/apps/access-level-check/modules/module-a/FooService.jsplugin/tegg/test/fixtures/apps/access-level-check/modules/module-main/BarService.jsplugin/tegg/test/fixtures/apps/access-level-check/modules/module-main/FooService.jsplugin/tegg/test/fixtures/apps/access-level-check/modules/module-main/MainService.jsplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/App.jsplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/App.jsplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.jsplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/Secret.jsplugin/tegg/test/fixtures/apps/app-with-no-module-json/app/controller/app.jsplugin/tegg/test/fixtures/apps/app-with-no-module-json/app/extend/application.unittest.jsplugin/tegg/test/fixtures/apps/app-with-no-module-json/app/extend/context.jsplugin/tegg/test/fixtures/apps/app-with-no-module-json/app/router.jsplugin/tegg/test/fixtures/apps/app-with-no-module-json/modules/config-module/ConfigService.jsplugin/tegg/test/fixtures/apps/background-app/app/controller/app.jsplugin/tegg/test/fixtures/apps/background-app/app/router.jsplugin/tegg/test/fixtures/apps/background-app/modules/multi-module-background/BackgroundService.jsplugin/tegg/test/fixtures/apps/background-app/modules/multi-module-background/CountService.jsplugin/tegg/test/fixtures/apps/constructor-module-config/app.jsplugin/tegg/test/fixtures/apps/constructor-module-config/app/controller/app.jsplugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/application.unittest.jsplugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/context.jsplugin/tegg/test/fixtures/apps/constructor-module-config/app/router.jsplugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/foo.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/app/controller/app.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/AbstractContextHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/AbstractSingletonHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/FooType.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/HelloService.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/SingletonHelloService.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/decorator/ContextHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/decorator/SingletonHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/BarContextHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/BarSingletonHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/FooContextHello.jsplugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/FooSingletonHello.jsplugin/tegg/test/fixtures/apps/egg-app/app/controller/app.jsplugin/tegg/test/fixtures/apps/egg-app/app/extend/application.jsplugin/tegg/test/fixtures/apps/egg-app/app/extend/application.unittest.jsplugin/tegg/test/fixtures/apps/egg-app/app/extend/context.jsplugin/tegg/test/fixtures/apps/egg-app/app/router.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-common/model/App.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-repo/AppRepo.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-repo/GlobalAppRepo.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-repo/PersistenceService.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/AppService.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/ConfigService.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/CustomLoggerService.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/EggTypeService.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/SingletonFooService.jsplugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/TraceService.jsplugin/tegg/test/fixtures/apps/inject-module-config/app.jsplugin/tegg/test/fixtures/apps/inject-module-config/app/controller/app.jsplugin/tegg/test/fixtures/apps/inject-module-config/app/extend/application.unittest.jsplugin/tegg/test/fixtures/apps/inject-module-config/app/extend/context.jsplugin/tegg/test/fixtures/apps/inject-module-config/app/router.jsplugin/tegg/test/fixtures/apps/inject-module-config/modules/module-with-config/foo.jsplugin/tegg/test/fixtures/apps/inject-module-config/modules/module-with-overwrite-config/bar.jsplugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.jsplugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.jsplugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.jsplugin/tegg/test/fixtures/apps/optional-module/app/modules/root/Root.jsplugin/tegg/test/fixtures/apps/recursive-module-app/app/controller/app.jsplugin/tegg/test/fixtures/apps/recursive-module-app/app/router.jsplugin/tegg/test/fixtures/apps/recursive-module-app/modules/multi-module-repo/AppRepo.jsplugin/tegg/test/fixtures/apps/recursive-module-app/modules/multi-module-service/AppService.jsplugin/tegg/test/fixtures/apps/same-name-protos/app/modules/module-a/BarService.jsplugin/tegg/test/fixtures/apps/same-name-protos/app/modules/module-bar/FooService.jsplugin/tegg/test/fixtures/apps/same-name-protos/app/modules/module-foo/FooService.jsplugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService1.jsplugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2.jsplugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService1.jsplugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2.jsplugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/FooService.jsplugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-foo/FooService.jsplugin/tegg/test/fixtures/apps/schedule-app/app/schedule/foo.jsplugin/tegg/test/fixtures/apps/schedule-app/modules/multi-module-repo/AppRepo.jsplugin/tegg/test/fixtures/apps/schedule-app/modules/multi-module-service/AppService.jsplugin/tegg/test/fixtures/apps/wrong-order-app/app/controller/app.jsplugin/tegg/test/fixtures/apps/wrong-order-app/app/router.jsplugin/tegg/test/fixtures/apps/wrong-order-app/modules/multi-module-repo/AppRepo.jsplugin/tegg/test/fixtures/apps/wrong-order-app/modules/multi-module-service/AppService.js
✅ Files skipped from review due to trivial changes (46)
- plugin/tegg/test/fixtures/apps/access-level-check/app/controller/app.js
- plugin/tegg/test/fixtures/apps/wrong-order-app/modules/multi-module-service/AppService.js
- plugin/tegg/test/fixtures/apps/schedule-app/app/schedule/foo.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/ConfigService.js
- plugin/tegg/test/fixtures/apps/app-with-no-module-json/app/extend/application.unittest.js
- plugin/tegg/test/fixtures/apps/app-with-no-module-json/app/router.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/TraceService.js
- plugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/application.unittest.js
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/App.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/AbstractContextHello.js
- plugin/tegg/test/fixtures/apps/same-name-protos/app/modules/module-foo/FooService.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/BarSingletonHello.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-repo/GlobalAppRepo.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/FooContextHello.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/FooType.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-common/model/App.js
- plugin/tegg/test/fixtures/apps/same-name-protos/app/modules/module-bar/FooService.js
- plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/FooService.js
- plugin/tegg/test/fixtures/apps/background-app/modules/multi-module-background/CountService.js
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.js
- plugin/tegg/test/fixtures/apps/access-level-check/modules/module-main/MainService.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-repo/PersistenceService.js
- plugin/tegg/test/fixtures/apps/egg-app/app/router.js
- plugin/tegg/test/fixtures/apps/wrong-order-app/app/router.js
- plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/AppService.js
- plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/BarContextHello.js
- core/types/agent-runtime/AgentRuntime.ts
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/decorator/SingletonHello.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/AbstractSingletonHello.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/HelloService.js
- plugin/tegg/test/fixtures/apps/egg-app/app/extend/application.unittest.js
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/App.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/controller/app.js
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.js
- plugin/tegg/test/fixtures/apps/recursive-module-app/modules/multi-module-repo/AppRepo.js
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/Secret.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/EggTypeService.js
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/Foo.js
- plugin/tegg/test/fixtures/apps/access-level-check/modules/module-main/FooService.js
- plugin/tegg/test/fixtures/apps/access-level-check/modules/module-a/BarService.js
- plugin/tegg/test/fixtures/apps/egg-app/modules/multi-module-service/SingletonFooService.js
- plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/foo.js
- plugin/dal/test/fixtures/apps/dal-app/modules/dal/dal/dao/base/BaseFooDAO.js
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.js
🚧 Files skipped from review as they are similar to previous changes (5)
- core/types/agent-runtime/errors.ts
- core/types/agent-runtime/AgentStore.ts
- core/agent-runtime/src/OSSAgentStore.ts
- core/agent-runtime/src/AgentRuntime.ts
- core/agent-runtime/test/OSSAgentStore.test.ts
Comment on lines
+15
to
+20
| async backgroudTimeout() { | ||
| const backgroundService = await this.ctx.app.getEggObject(BackgroundService_1.default); | ||
| await backgroundService.backgroundAdd(6000); | ||
| this.ctx.status = 200; | ||
| this.ctx.body = 'done'; | ||
| } |
There was a problem hiding this comment.
Fix typo in method name.
The method name backgroudTimeout is misspelled; it should be backgroundTimeout (missing 'n' in "background").
📝 Proposed fix
This is compiled JavaScript. The fix must be applied to the source TypeScript file:
- async backgroudTimeout() {
+ async backgroundTimeout() {
const backgroundService = await this.ctx.app.getEggObject(BackgroundService_1.default);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/tegg/test/fixtures/apps/background-app/app/controller/app.js` around
lines 15 - 20, The controller method is misspelled as backgroudTimeout; rename
it to backgroundTimeout in the source TypeScript controller so the
exported/compiled JS matches the intended route handler. Update the method
declaration (currently backgroudTimeout) to backgroundTimeout and ensure any
references/callers (e.g., routes or tests that invoke backgroudTimeout) are
updated accordingly; the function that uses
BackgroundService_1.default.backgroundAdd should remain unchanged.
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
| module.exports = (app) => { | ||
| app.router.get('/background', app.controller.app.background); | ||
| app.router.get('/backgroudTimeout', app.controller.app.backgroudTimeout); |
There was a problem hiding this comment.
Fix typo in route path.
The route path /backgroudTimeout is misspelled; it should be /backgroundTimeout (missing 'n' in "background"). This typo matches the controller method name typo and both should be corrected together.
📝 Proposed fix
This is compiled JavaScript. The fix must be applied to the source TypeScript file:
app.router.get('/background', app.controller.app.background);
- app.router.get('/backgroudTimeout', app.controller.app.backgroudTimeout);
+ app.router.get('/backgroundTimeout', app.controller.app.backgroundTimeout);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/tegg/test/fixtures/apps/background-app/app/router.js` at line 5, The
route path contains a typo: change the route registered as
app.router.get('/backgroudTimeout', app.controller.app.backgroudTimeout) to use
the correct path '/backgroundTimeout' and rename the controller method from
backgroudTimeout to backgroundTimeout in the source TypeScript controller;
update all references (e.g., any uses of app.controller.app.backgroudTimeout) to
app.controller.app.backgroundTimeout so the route and handler names match.
Comment on lines
+9
to
+10
| if ((_b = (_a = this.app.moduleConfigs) === null || _a === void 0 ? void 0 : _a.overwrite) === null || _b === void 0 ? void 0 : _b.config) { | ||
| this.app.moduleConfigs.overwrite.config.features.dynamic.bar = 'overwrite foo'; |
There was a problem hiding this comment.
Shared root cause: partial guard before deep config mutation in boot hooks.
Both boot files check only overwrite.config but then write overwrite.config.features.dynamic.bar; guard the full path to avoid boot-time TypeError on partial configs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/tegg/test/fixtures/apps/constructor-module-config/app.js` around lines
9 - 10, The boot hook mutates a deep path without ensuring the full object chain
exists: before writing to
this.app.moduleConfigs.overwrite.config.features.dynamic.bar (in the boot hook
that checks this.app.moduleConfigs.overwrite.config), add a defensive guard or
initialize missing intermediate objects (overwrite, config, features, dynamic)
so the assignment cannot throw; either check the existence of
overwrite.config.features.dynamic or create those nested objects on
this.app.moduleConfigs.overwrite.config prior to setting .bar.
Comment on lines
+14
to
+22
| let FooSingletonHello = class FooSingletonHello extends AbstractContextHello_1.AbstractContextHello { | ||
| constructor() { | ||
| super(...arguments); | ||
| this.id = 0; | ||
| } | ||
| hello() { | ||
| return `hello, foo(singleton:${this.id++})`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Inheritance mismatch: singleton class extends context abstract.
FooSingletonHello is decorated with @SingletonProto() and @SingletonHello(FOO), indicating it's a singleton implementation, but it extends AbstractContextHello instead of AbstractSingletonHello. Given the module includes separate AbstractContextHello and AbstractSingletonHello abstractions, and SingletonHelloService (in this same module) expects to resolve AbstractSingletonHello implementations, this class should extend AbstractSingletonHello.
🔧 Proposed fix
-const AbstractContextHello_1 = require("../AbstractContextHello");
-let FooSingletonHello = class FooSingletonHello extends AbstractContextHello_1.AbstractContextHello {
+const AbstractSingletonHello_1 = require("../AbstractSingletonHello");
+let FooSingletonHello = class FooSingletonHello extends AbstractSingletonHello_1.AbstractSingletonHello {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/impl/FooSingletonHello.js`
around lines 14 - 22, The class FooSingletonHello incorrectly extends
AbstractContextHello; change its superclass to AbstractSingletonHello so the
`@SingletonProto`()/@SingletonHello(FOO) decorated implementation matches the
expected abstraction used by SingletonHelloService. Locate the FooSingletonHello
class definition and replace the extends AbstractContextHello reference with
extends AbstractSingletonHello, ensuring imports/aliases for
AbstractSingletonHello are adjusted if necessary so the symbol resolves
correctly.
| var _a; | ||
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
| exports.RootProto = void 0; | ||
| const core_decorator_1 = require("@eggjs/core-decorator"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if other test fixtures use `@eggjs/tegg` for decorator imports
rg -n "require.*`@eggjs/`(tegg|core-decorator)" plugin/tegg/test/fixtures/ -g "*.js" | head -20Repository: eggjs/tegg
Length of output: 2913
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="plugin/tegg/test/fixtures/apps/optional-module/app/modules/root/Root.js"
echo "== Target file (lines 1-60) =="
if [ -f "$FILE" ]; then
nl -ba "$FILE" | sed -n '1,80p'
else
echo "Missing file: $FILE"
fi
echo
echo "== Search for core-decorator in fixtures =="
rg -n "`@eggjs/core-decorator`" plugin/tegg/test/fixtures/ -g "*.js" || true
echo
echo "== Search for core-decorator in target file =="
if [ -f "$FILE" ]; then
rg -n "`@eggjs/core-decorator`" "$FILE" -g "*.js" || true
fiRepository: eggjs/tegg
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="plugin/tegg/test/fixtures/apps/optional-module/app/modules/root/Root.js"
echo "== Target file (lines 1-60) =="
if [ -f "$FILE" ]; then
awk 'NR<=80 {printf "%5d\t%s\n", NR, $0}' "$FILE"
else
echo "Missing file: $FILE"
fi
echo
echo "== Search for `@eggjs/core-decorator` in fixtures =="
rg -n "`@eggjs/core-decorator`" plugin/tegg/test/fixtures/ -g "*.js" || true
echo
echo "== Search for `@eggjs/core-decorator` in target file =="
if [ -f "$FILE" ]; then
rg -n "`@eggjs/core-decorator`" "$FILE" -g "*.js" || true
fiRepository: eggjs/tegg
Length of output: 2766
Fix tegg decorator imports in fixture (use @eggjs/tegg instead of @eggjs/core-decorator).
plugin/tegg/test/fixtures/apps/optional-module/app/modules/root/Root.js imports @eggjs/core-decorator and uses Inject/SingletonProto from it (lines 14, 20, 24), which violates the coding guideline for tegg decorators.
📦 Proposed fix to align with coding guidelines
-const core_decorator_1 = require("`@eggjs/core-decorator`");
+const tegg_1 = require("`@eggjs/tegg`");And update the decorator usage on lines 20 and 24:
- (0, core_decorator_1.Inject)(),
+ (0, tegg_1.Inject)(),- (0, core_decorator_1.SingletonProto)()
+ (0, tegg_1.SingletonProto)()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/tegg/test/fixtures/apps/optional-module/app/modules/root/Root.js` at
line 14, Replace the incorrect tegg decorator import in Root.js (currently
requiring "`@eggjs/core-decorator`") with the correct "`@eggjs/tegg`" package and
update uses of the decorators accordingly: change the import that provides
Inject and SingletonProto to require("`@eggjs/tegg`") and ensure the decorator
identifiers used on the Root class and its properties (references to Inject and
SingletonProto) are the ones exported from "`@eggjs/tegg`" so the Root class (and
property decorators around lines where Inject and SingletonProto are applied)
use the correct tegg decorator implementations.
Source: Coding guidelines
Reuse the existing `CreateRunInput.metadata` instead of a dedicated input field. On run creation, `metadata` is double-written: kept verbatim on the run record AND shallow-merged into the thread's meta.json — initializing an auto-created thread, or merging into an existing one (matching keys overwritten, others preserved; an empty or omitted object is a no-op). Non-object metadata is rejected with HTTP 400 before any thread/run write. Robustness: - Mirroring metadata onto an existing thread is best-effort: a failed thread write is logged and does not fail run creation (the run record already holds the metadata). A store lacking updateThreadMetadata still errors. - updateThreadMetadata and recordLatestRunId both read-modify-write the same meta.json; they now share a per-thread write lock so concurrent runs on one thread no longer clobber each other (cross-process stays last-writer-wins). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jerryliang64
force-pushed
the
agent/dev/4a73189b
branch
from
ff71f08 to
2c8c8ef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements RUN-10 in
@eggjs/agent-runtime: run creation requests can persist explicit thread-level metadata without changing existing runmetadatasemantics.Changes
threadMetadatatoCreateRunInputfor/runs,/runs/stream, and/runs/wait.threadMetadata.threadMetadatainto existing threads through an optionalAgentStore.updateThreadMetadatacapability.threadMetadatawith an HTTP 400-compatible error before creating thread or run data.null; empty or omitted metadata is a no-op.metadataisolated from thread metadata.Files
core/types/agent-runtime/AgentRuntime.tscore/types/agent-runtime/AgentStore.tscore/types/agent-runtime/errors.tscore/agent-runtime/src/AgentRuntime.tscore/agent-runtime/src/OSSAgentStore.tsTests
npm test -w @eggjs/agent-runtime -- test/AgentRuntime.metadata.test.ts test/OSSAgentStore.test.ts(170 passing)npm run tsc -w @eggjs/agent-runtimenpm run tsc --workspaces --if-presentNot covered / limitations
/raw/streamand/task/streamdo not use the AgentStore thread model and are unchanged.Summary by CodeRabbit
New Features
Tests
Documentation