feat: add experiment-gated self-improving learning, bounded memory, and background curation#244
Closed
iskandarsulaili wants to merge 6 commits into
Closed
feat: add experiment-gated self-improving learning, bounded memory, and background curation#244iskandarsulaili wants to merge 6 commits into
iskandarsulaili wants to merge 6 commits into
Conversation
- Introduced SelfImprovingManager to facilitate background learning from task outcomes. - Added LearningStore, FeedbackCollector, PatternAnalyzer, ImprovementApplier, and CodeIndexAdapter for modular functionality. - Implemented experiment gating for enabling/disabling self-improvement features. - Created comprehensive tests for SelfImprovingManager to ensure functionality and stability. - Updated localization files to include settings for self-improvement in multiple languages.
…re integration - Introduced MemoryStore for managing memory entries and snapshots. - Added SkillUsageStore for tracking skill usage telemetry. - Implemented ActionExecutor to handle action execution and logging. - Updated SelfImprovingManager to initialize and utilize MemoryStore and SkillUsageStore. - Enhanced telemetry reporting in SelfImprovingManager's status. - Added tests for ActionExecutor, MemoryStore, and SkillUsageStore to ensure functionality. - Updated types to include new SkillUsageStore and MemoryStore interfaces.
…rovement - Added ReviewPromptFactory to generate structured review prompts for memory and skill reviews. - Introduced TranscriptRecall to store and manage transcript entries for task outcomes. - Enhanced SelfImprovingManager to utilize ReviewPromptFactory and TranscriptRecall. - Updated CuratorService to support new functionality and maintain skill usage. - Created unit tests for ReviewPromptFactory, TranscriptRecall, and CuratorService to ensure reliability. - Modified types and index files to include new services and types.
…settings management
iskandarsulaili
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a complete self-improving learning system to the extension. It records task outcomes and user corrections as learning signals, analyzes patterns from events, generates typed improvement actions for prompts/tools/error-avoidance/skills, and executes those actions to enrich system prompts and skill suggestions. The implementation includes persistent file-backed stores for learning state, memory context, skill telemetry, and transcripts, all gated behind a ChangesSelf-Improving Learning System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
src/__tests__/extension.spec.ts (1)
180-188: ⚡ Quick winAssert the new activation hook instead of only stubbing it.
This mock keeps the suite green even if
activate()stops callinginitializeSelfImproving(). Please add an expectation in one activation test so the new provider wiring is actually covered.🤖 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 `@src/__tests__/extension.spec.ts` around lines 180 - 188, The test currently stubs ClineProvider but doesn't assert that the new activation hook is invoked; update an activation test to call the extension's activate() and then assert that the mocked provider's initializeSelfImproving method was called (e.g., expect(mockInstance.initializeSelfImproving).toHaveBeenCalled()), using the mock instance created in the vi.mock for "../core/webview/ClineProvider"; this ensures activate() actually wires and invokes initializeSelfImproving rather than silently stubbing it out.src/core/prompts/system.ts (1)
84-85: ⚡ Quick winMake the learned-guidance separator explicit.
learningContextis appended raw here, so whether it renders as a separate block now depends ongetPromptContextString()including its own leading newline. Add the delimiter in this template so prompt section boundaries stay deterministic.Proposed change
- const learningContext = selfImprovingManager?.getPromptContextString() || "" + const learningContext = selfImprovingManager?.getPromptContextString() || "" + const learningSection = learningContext ? `\n${learningContext}` : "" @@ -${skillsSection ? `\n${skillsSection}` : ""}${learningContext} +${skillsSection ? `\n${skillsSection}` : ""}${learningSection}Also applies to: 101-101
🤖 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 `@src/core/prompts/system.ts` around lines 84 - 85, The template currently appends learningContext raw (learningContext = selfImprovingManager?.getPromptContextString() || "") which makes section boundaries brittle; change the code that injects the self-improving guidance to prepend a deterministic delimiter (e.g., "\n\n---\n" or "\n\n[Self-improving guidance]\n") before learningContext when non-empty, so when using selfImprovingManager.getPromptContextString() the guidance is always separated as its own block; update both places where learningContext is appended (the occurrence using learningContext and the other at lines referenced) to use this delimiter only when learningContext is not empty.src/services/self-improving/types.ts (1)
100-112: ⚡ Quick winUse shared defaults instead of duplicating config/state literals.
Keeping local copies of defaults here can silently diverge from
@roo-code/typesover time.♻️ Proposed fix
-import type { +import { + DEFAULT_LEARNING_CONFIG, + EMPTY_LEARNING_STATE, + type ActionType, + type FeedbackSignal, + type ImprovementAction, + type LearnedPattern, + type LearningConfig, + type LearningEvent, + type LearningState, + type LearningTelemetry, + type PatternState, + type PatternType, +} from "`@roo-code/types`" - ActionType, - FeedbackSignal, - ImprovementAction, - LearnedPattern, - LearningConfig, - LearningEvent, - LearningState, - LearningTelemetry, - PatternState, - PatternType, -} from "`@roo-code/types`" @@ -export const DEFAULT_CONFIG: LearningConfig = { - enabled: false, - reviewOnTurnCount: 10, - reviewOnToolIterationCount: 50, - maxStoredPatterns: 100, - maxStoredEvents: 500, - maxPromptPatterns: 5, - curatorEnabled: true, - curatorIntervalMs: 3600000, - staleAfterDays: 14, - archiveAfterDays: 60, - codeIndexCorrelationEnabled: true, -} +export const DEFAULT_CONFIG: LearningConfig = DEFAULT_LEARNING_CONFIG @@ -export const EMPTY_STATE: LearningState = { - version: 1, - config: DEFAULT_CONFIG, - counters: { - userTurnsSinceReview: 0, - toolIterationsSinceReview: 0, - }, - patterns: [], - archivedPatterns: [], - recentEvents: [], - pendingActions: [], - telemetry: { - promptEnrichmentUses: 0, - toolPreferenceUses: 0, - errorAvoidanceUses: 0, - skillSuggestionCount: 0, - }, -} +export const EMPTY_STATE: LearningState = EMPTY_LEARNING_STATEAlso applies to: 117-134
🤖 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 `@src/services/self-improving/types.ts` around lines 100 - 112, The local DEFAULT_CONFIG object duplicates shared literals and can drift from the canonical defaults in `@roo-code/types`; replace the local literal with the shared default export from that package (e.g. import the exported defaults such as DEFAULT_LEARNING_CONFIG or equivalent from '`@roo-code/types`'), then export or re-export that imported value (or create a small override via spread like const DEFAULT_CONFIG = { ...ImportedDefault, <overrides> }) instead of hardcoding the fields; do the same for the other duplicated config/state literals referenced around the DEFAULT_CONFIG block and any duplicated blocks at 117-134 so the module consistently uses the shared symbols (refer to DEFAULT_CONFIG and LearningConfig in this file to locate where to replace literals).src/services/self-improving/LearningStore.ts (1)
319-323: ⚡ Quick winPrevent
updatePatternfrom mutating pattern identity.
Partial<LearnedPattern>allowsidupdates, which can desynchronize identity-based operations.♻️ Proposed fix
-updatePattern(id: string, updates: Partial<LearnedPattern>): void { +updatePattern(id: string, updates: Partial<Omit<LearnedPattern, "id">>): void { const index = this.state.patterns.findIndex((pattern) => pattern.id === id) if (index >= 0) { - this.state.patterns[index] = { ...this.state.patterns[index], ...updates } + this.state.patterns[index] = { ...this.state.patterns[index], ...updates, id } } }🤖 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 `@src/services/self-improving/LearningStore.ts` around lines 319 - 323, The updatePattern method currently merges updates that may include id, which can change an object's identity; modify updatePattern (in LearningStore) to ignore any id in the updates before merging: detect and remove the id property from the Partial<LearnedPattern> updates (or create a new object that omits id) and then apply the spread merge into this.state.patterns[index] so the stored pattern.id remains unchanged; ensure you still perform the existence check using the passed id parameter.src/services/self-improving/ActionExecutor.ts (1)
180-188: ⚡ Quick winNormalize string-array payload values before dedupe.
readStringArrayPayloadkeeps original string values, so" ENOENT "and"ENOENT"are treated as different and can emit noisy tags.Proposed fix
private readStringArrayPayload(value: unknown): string[] { if (!Array.isArray(value)) { return [] } return Array.from( - new Set(value.filter((item): item is string => typeof item === "string" && item.trim().length > 0)), + new Set( + value + .filter((item): item is string => typeof item === "string") + .map((item) => item.trim()) + .filter((item) => item.length > 0), + ), ) }🤖 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 `@src/services/self-improving/ActionExecutor.ts` around lines 180 - 188, readStringArrayPayload currently deduplicates raw strings so values like " ENOENT " and "ENOENT" remain distinct; update readStringArrayPayload to normalize each item by trimming whitespace (and then filtering out empty strings), then dedupe the trimmed values (e.g., map items to item.trim(), filter by length>0, then use Set/Array.from) before returning the string[] so noisy duplicate tags are removed; refer to the readStringArrayPayload function and the value parameter when making the change.
🤖 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 `@packages/types/src/index.ts`:
- Line 18: There is a duplicated re-export of "./marketplace.js" — remove the
duplicate export statement "export * from \"./marketplace.js\"" so that only one
re-export of marketplace remains in the file; keep the remaining export as-is
and run the type/build checks to ensure nothing else relies on the duplicate.
In `@src/core/webview/ClineProvider.ts`:
- Around line 258-279: The current recordTaskCompletionForLearning handler
records every TaskAborted as a failure; change it to only record failures for
terminal aborts by checking the abort reason/flag emitted with
RooCodeEventName.TaskAborted (or from removeClineFromStack) before calling
this.selfImprovingManager.recordTaskCompletion with success=false; update the
logic inside recordTaskCompletionForLearning (and the similar block around lines
297-327) to inspect the event payload (e.g., abortReason or isTerminal) and only
set success:false when the abort is terminal, otherwise skip or treat as
non-failure.
In `@src/services/self-improving/__tests__/SkillUsageStore.spec.ts`:
- Around line 25-29: Replace the fixed sleep in the test with a deterministic
wait that polls for the persisted file/contents instead of setTimeout;
specifically, instead of awaiting new Promise(resolve => setTimeout(resolve,
20)) before reading path.join(tempDir, "self-improving", "skill-usage.json"),
implement a retry loop or helper (e.g., waitForFile or waitUntil) that
repeatedly attempts fs.readFile and JSON.parse with a short delay and timeout
until the file exists and contains the expected JSON, then assign that parsed
value to persisted; this ensures SkillUsageStore persistence is observed
deterministically under CI.
In `@src/services/self-improving/CuratorService.ts`:
- Around line 118-121: The current shouldRun(now:number,
lastUserActivityAt?:number) implementation short-circuits and returns false when
config.firstRunDeferred is true, firstRunDone is false and lastRunAt === 0,
which prevents callers that only call shouldRun() from ever invoking run() to
clear the deferred state; change shouldRun so it does not permanently block the
initial deferred state—allow it to return true (or otherwise indicate runnable)
when config.firstRunDeferred && !this.firstRunDone && this.lastRunAt === 0 so
callers can proceed to call run(), and apply the same change to the other
identical check around lines 155-158; refer to the shouldRun and run methods and
the config.firstRunDeferred, firstRunDone and lastRunAt fields when making the
change.
- Around line 104-115: The initialize() implementation in CuratorService
currently sets this.initialized = true in the finally block which marks the
service initialized even when fs.mkdir or loadState() fails; move the
this.initialized = true assignment out of finally and into the successful path
(after both fs.mkdir(this.backupsDir, { recursive: true }) and
fs.mkdir(this.reportsDir, { recursive: true }) and await this.loadState()
complete and the logger appendLine call) so initialization is only marked
complete on success; leave the catch to log the error (and optionally rethrow or
return) so failed setup does not block future retries.
In `@src/services/self-improving/LearningStore.ts`:
- Around line 75-80: The code currently assigns parsed disk JSON into
LearningStore.state without schema validation; update the load path in
LearningStore (the block that calls this.mergeWithDefaults(parsed) and
this.loadPatternFiles()) to first validate the parsed object against the
learning schema (e.g., call an existing validateLearningState(parsed) or use
learningStateSchema.validate(parsed)) and only proceed if validation passes,
otherwise log an error and fall back to defaults; apply the same validation
logic to the other load path around lines 128-133 so both places validate parsed
JSON before calling mergeWithDefaults or loadPatternFiles.
In `@src/services/self-improving/PatternAnalyzer.ts`:
- Around line 124-126: The current lookup uses summary.includes(...) which can
collide on overlapping names; update the matching in PatternAnalyzer where
existingPatterns.find(...) is used (the call that checks pattern.patternType ===
"tool" && pattern.summary.includes(toolKey) and the similar check around lines
188-190) to compare a structured field instead of a substring: match
pattern.patternType === "tool" and pattern.context?.toolKey === toolKey (or
pattern.metadata?.toolKey === toolKey if your Pattern shape uses metadata),
falling back to an exact summary equality only if the structured key is absent;
modify the code paths that create patterns to populate that structured toolKey
field so future lookups use the stable identifier.
In `@src/services/self-improving/SelfImprovingManager.ts`:
- Around line 269-272: When runReviewCycle() is invoked by checkReviewTriggers()
and getRecentEvents() returns an empty array, ensure the user-turn/tool review
counters are reset before returning so the threshold doesn't remain met; in
SelfImprovingManager locate the block using this.runtime.store.getRecentEvents()
(and the analogous block around lines 515-523) and add logic to clear or reset
the relevant counters on this.runtime (e.g., the userTurnCounter/toolTurnCounter
or whichever fields track review signals) immediately prior to the early return,
then return as before.
- Around line 166-187: The code dereferences this.runtime after await points
which can be cleared by dispose(); fix by capturing a local snapshot at the top
of the method/block (e.g. const runtime = this.runtime) and use runtime
everywhere in this block (runtime.feedbackCollector.createTaskEvent,
runtime.store.addEvent, runtime.store.incrementToolIterations, runtime.store
passed into checkReviewTriggers) and similarly snapshot any other late-bound
dependencies you use across awaits (e.g. const transcriptRecall =
this.transcriptRecall) so all subsequent awaits reference the locals instead of
this.runtime; apply the same pattern to the other affected blocks referenced
(around the createTaskEvent / transcriptRecall.record / checkReviewTriggers
usages).
In `@src/services/self-improving/SkillUsageStore.ts`:
- Around line 133-135: The unawaited writes from queuePersist() (calling
persist()) can overlap and cause stale overwrites; fix by serializing persists
with a single in-flight promise chain: add a private field (e.g.,
persistPromise: Promise<void> | null) and change queuePersist() to chain the new
persist() onto persistPromise (persistPromise = (persistPromise ??
Promise.resolve()).then(() => this.persist(), () => this.persist())), ensuring
each persist runs after the previous completes; also update all other call sites
that currently call this.persist() unawaited (the other persisted call sites
referenced around the block at line ~229) to use the same chaining approach or
await the resulting persistPromise so no two persists run concurrently.
In `@src/services/self-improving/TranscriptRecall.ts`:
- Around line 119-121: The persisted JSON may contain malformed entries that
break search() when it calls .toLowerCase(); in the Array.isArray(parsed) branch
where this.entries is set, validate and sanitize each item before storing:
iterate over parsed.slice(-TranscriptRecall.MAX_ENTRIES), ensure each entry is
an object with the expected fields (e.g., a string 'text' property), coerce or
normalize types (convert text to String and trim, or skip entries missing text),
and assign the cleaned array to this.entries so downstream code (search(),
.toLowerCase()) cannot throw on unexpected shapes.
---
Nitpick comments:
In `@src/__tests__/extension.spec.ts`:
- Around line 180-188: The test currently stubs ClineProvider but doesn't assert
that the new activation hook is invoked; update an activation test to call the
extension's activate() and then assert that the mocked provider's
initializeSelfImproving method was called (e.g.,
expect(mockInstance.initializeSelfImproving).toHaveBeenCalled()), using the mock
instance created in the vi.mock for "../core/webview/ClineProvider"; this
ensures activate() actually wires and invokes initializeSelfImproving rather
than silently stubbing it out.
In `@src/core/prompts/system.ts`:
- Around line 84-85: The template currently appends learningContext raw
(learningContext = selfImprovingManager?.getPromptContextString() || "") which
makes section boundaries brittle; change the code that injects the
self-improving guidance to prepend a deterministic delimiter (e.g., "\n\n---\n"
or "\n\n[Self-improving guidance]\n") before learningContext when non-empty, so
when using selfImprovingManager.getPromptContextString() the guidance is always
separated as its own block; update both places where learningContext is appended
(the occurrence using learningContext and the other at lines referenced) to use
this delimiter only when learningContext is not empty.
In `@src/services/self-improving/ActionExecutor.ts`:
- Around line 180-188: readStringArrayPayload currently deduplicates raw strings
so values like " ENOENT " and "ENOENT" remain distinct; update
readStringArrayPayload to normalize each item by trimming whitespace (and then
filtering out empty strings), then dedupe the trimmed values (e.g., map items to
item.trim(), filter by length>0, then use Set/Array.from) before returning the
string[] so noisy duplicate tags are removed; refer to the
readStringArrayPayload function and the value parameter when making the change.
In `@src/services/self-improving/LearningStore.ts`:
- Around line 319-323: The updatePattern method currently merges updates that
may include id, which can change an object's identity; modify updatePattern (in
LearningStore) to ignore any id in the updates before merging: detect and remove
the id property from the Partial<LearnedPattern> updates (or create a new object
that omits id) and then apply the spread merge into this.state.patterns[index]
so the stored pattern.id remains unchanged; ensure you still perform the
existence check using the passed id parameter.
In `@src/services/self-improving/types.ts`:
- Around line 100-112: The local DEFAULT_CONFIG object duplicates shared
literals and can drift from the canonical defaults in `@roo-code/types`; replace
the local literal with the shared default export from that package (e.g. import
the exported defaults such as DEFAULT_LEARNING_CONFIG or equivalent from
'`@roo-code/types`'), then export or re-export that imported value (or create a
small override via spread like const DEFAULT_CONFIG = { ...ImportedDefault,
<overrides> }) instead of hardcoding the fields; do the same for the other
duplicated config/state literals referenced around the DEFAULT_CONFIG block and
any duplicated blocks at 117-134 so the module consistently uses the shared
symbols (refer to DEFAULT_CONFIG and LearningConfig in this file to locate where
to replace literals).
🪄 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 Plus
Run ID: 780fd481-5bc0-4674-833e-d44088b96c36
📒 Files selected for processing (56)
packages/types/src/__tests__/learning-memory.test.tspackages/types/src/experiment.tspackages/types/src/index.tspackages/types/src/learning.tspackages/types/src/memory.tspackages/types/src/vscode-extension-host.tssrc/__tests__/extension.spec.tssrc/core/prompts/__tests__/system-prompt.spec.tssrc/core/prompts/system.tssrc/core/task/Task.tssrc/core/webview/ClineProvider.tssrc/core/webview/generateSystemPrompt.tssrc/core/webview/webviewMessageHandler.tssrc/extension.tssrc/services/self-improving/ActionExecutor.tssrc/services/self-improving/CodeIndexAdapter.tssrc/services/self-improving/CuratorService.tssrc/services/self-improving/FeedbackCollector.tssrc/services/self-improving/ImprovementApplier.tssrc/services/self-improving/LearningStore.tssrc/services/self-improving/MemoryStore.tssrc/services/self-improving/PatternAnalyzer.tssrc/services/self-improving/ReviewPromptFactory.tssrc/services/self-improving/SelfImprovingManager.tssrc/services/self-improving/SkillUsageStore.tssrc/services/self-improving/TranscriptRecall.tssrc/services/self-improving/__tests__/ActionExecutor.spec.tssrc/services/self-improving/__tests__/CuratorService.spec.tssrc/services/self-improving/__tests__/LearningStore.spec.tssrc/services/self-improving/__tests__/MemoryStore.spec.tssrc/services/self-improving/__tests__/ReviewPromptFactory.spec.tssrc/services/self-improving/__tests__/SelfImprovingManager.spec.tssrc/services/self-improving/__tests__/SkillUsageStore.spec.tssrc/services/self-improving/__tests__/TranscriptRecall.spec.tssrc/services/self-improving/index.tssrc/services/self-improving/types.tssrc/shared/__tests__/experiments.spec.tssrc/shared/experiments.tswebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
| export * from "./image-generation.js" | ||
| export * from "./ipc.js" | ||
| export * from "./learning.js" | ||
| export * from "./marketplace.js" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate star re-exports in packages/types/src/index.ts
rg -n '^export \* from "\./[^"]+\.js"$' packages/types/src/index.ts \
| sed -E 's/.*"\.\/([^"]+)\.js".*/\1/' \
| sort \
| uniq -cd
# Expected: a count >1 for "marketplace"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 85
🏁 Script executed:
#!/bin/bash
nl -ba packages/types/src/index.ts | sed -n '1,120p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Show the relevant portion of packages/types/src/index.ts with line numbers
cat -n packages/types/src/index.ts | sed -n '1,80p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1466
Remove the duplicate marketplace re-export
packages/types/src/index.ts re-exports ./marketplace.js twice (lines 18 and 28), which is redundant.
♻️ Proposed fix
export * from "./learning.js"
export * from "./marketplace.js"
export * from "./mcp.js"
export * from "./message.js"
export * from "./memory.js"
@@
export * from "./skills.js"
-export * from "./marketplace.js"
export * from "./telemetry.js"🤖 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 `@packages/types/src/index.ts` at line 18, There is a duplicated re-export of
"./marketplace.js" — remove the duplicate export statement "export * from
\"./marketplace.js\"" so that only one re-export of marketplace remains in the
file; keep the remaining export as-is and run the type/build checks to ensure
nothing else relies on the duplicate.
Comment on lines
+258
to
+279
| const recordTaskCompletionForLearning = (success: boolean) => { | ||
| void instance | ||
| .getTaskMode() | ||
| .catch(() => defaultModeSlug) | ||
| .then((mode) => | ||
| this.selfImprovingManager.recordTaskCompletion({ | ||
| taskId: instance.taskId, | ||
| mode, | ||
| workspacePath: this.currentWorkspacePath, | ||
| success, | ||
| ...(success | ||
| ? { | ||
| toolNames: instance.toolUsage ? Object.keys(instance.toolUsage) : undefined, | ||
| } | ||
| : {}), | ||
| }), | ||
| ) | ||
| .catch((error) => { | ||
| this.log( | ||
| `[SelfImproving] recordTaskCompletion error: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't treat every TaskAborted as a failed completion.
RooCodeEventName.TaskAborted is also emitted for non-terminal control-flow paths in this class. ClineProvider.removeClineFromStack() aborts tasks during delegation and rehydration, so Line 327 will persist false failures for tasks that are only being swapped out. That will poison the learning store and skew the patterns/actions generated from it. Please gate failure recording on terminal abort reasons instead of every abort event.
Also applies to: 297-327
🤖 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 `@src/core/webview/ClineProvider.ts` around lines 258 - 279, The current
recordTaskCompletionForLearning handler records every TaskAborted as a failure;
change it to only record failures for terminal aborts by checking the abort
reason/flag emitted with RooCodeEventName.TaskAborted (or from
removeClineFromStack) before calling
this.selfImprovingManager.recordTaskCompletion with success=false; update the
logic inside recordTaskCompletionForLearning (and the similar block around lines
297-327) to inspect the event payload (e.g., abortReason or isTerminal) and only
set success:false when the abort is terminal, otherwise skip or treat as
non-failure.
Comment on lines
+25
to
+29
| await new Promise((resolve) => setTimeout(resolve, 20)) | ||
|
|
||
| const persisted = JSON.parse( | ||
| await fs.readFile(path.join(tempDir, "self-improving", "skill-usage.json"), "utf8"), | ||
| ) as Array<{ skillId: string; skillName: string }> |
Contributor
There was a problem hiding this comment.
Replace fixed sleep with deterministic persistence waiting.
Using a hardcoded setTimeout(20) makes this assertion timing-dependent and can intermittently fail under CI load.
Proposed fix
- await new Promise((resolve) => setTimeout(resolve, 20))
-
- const persisted = JSON.parse(
- await fs.readFile(path.join(tempDir, "self-improving", "skill-usage.json"), "utf8"),
- ) as Array<{ skillId: string; skillName: string }>
+ const skillUsagePath = path.join(tempDir, "self-improving", "skill-usage.json")
+ let raw = ""
+ for (let i = 0; i < 20; i++) {
+ try {
+ raw = await fs.readFile(skillUsagePath, "utf8")
+ break
+ } catch {
+ await new Promise((resolve) => setTimeout(resolve, 10))
+ }
+ }
+ const persisted = JSON.parse(raw) as Array<{ skillId: string; skillName: 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.
Suggested change
| await new Promise((resolve) => setTimeout(resolve, 20)) | |
| const persisted = JSON.parse( | |
| await fs.readFile(path.join(tempDir, "self-improving", "skill-usage.json"), "utf8"), | |
| ) as Array<{ skillId: string; skillName: string }> | |
| const skillUsagePath = path.join(tempDir, "self-improving", "skill-usage.json") | |
| let raw = "" | |
| for (let i = 0; i < 20; i++) { | |
| try { | |
| raw = await fs.readFile(skillUsagePath, "utf8") | |
| break | |
| } catch { | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| } | |
| } | |
| const persisted = JSON.parse(raw) as Array<{ skillId: string; skillName: string }> |
🤖 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 `@src/services/self-improving/__tests__/SkillUsageStore.spec.ts` around lines
25 - 29, Replace the fixed sleep in the test with a deterministic wait that
polls for the persisted file/contents instead of setTimeout; specifically,
instead of awaiting new Promise(resolve => setTimeout(resolve, 20)) before
reading path.join(tempDir, "self-improving", "skill-usage.json"), implement a
retry loop or helper (e.g., waitForFile or waitUntil) that repeatedly attempts
fs.readFile and JSON.parse with a short delay and timeout until the file exists
and contains the expected JSON, then assign that parsed value to persisted; this
ensures SkillUsageStore persistence is observed deterministically under CI.
Comment on lines
+104
to
+115
| try { | ||
| await fs.mkdir(this.backupsDir, { recursive: true }) | ||
| await fs.mkdir(this.reportsDir, { recursive: true }) | ||
| await this.loadState() | ||
| this.logger.appendLine("[CuratorService] Initialized") | ||
| } catch (error) { | ||
| this.logger.appendLine( | ||
| `[CuratorService] Initialization error: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } finally { | ||
| this.initialized = true | ||
| } |
Contributor
There was a problem hiding this comment.
Do not mark initialization complete when setup fails.
initialize() sets initialized = true in finally, so a transient fs/load failure makes the service skip all future initialization retries.
Proposed fix
async initialize(): Promise<void> {
if (this.initialized) {
return
}
try {
await fs.mkdir(this.backupsDir, { recursive: true })
await fs.mkdir(this.reportsDir, { recursive: true })
await this.loadState()
+ this.initialized = true
this.logger.appendLine("[CuratorService] Initialized")
} catch (error) {
this.logger.appendLine(
`[CuratorService] Initialization error: ${error instanceof Error ? error.message : String(error)}`,
)
- } finally {
- this.initialized = true
+ throw error
}
}🤖 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 `@src/services/self-improving/CuratorService.ts` around lines 104 - 115, The
initialize() implementation in CuratorService currently sets this.initialized =
true in the finally block which marks the service initialized even when fs.mkdir
or loadState() fails; move the this.initialized = true assignment out of finally
and into the successful path (after both fs.mkdir(this.backupsDir, { recursive:
true }) and fs.mkdir(this.reportsDir, { recursive: true }) and await
this.loadState() complete and the logger appendLine call) so initialization is
only marked complete on success; leave the catch to log the error (and
optionally rethrow or return) so failed setup does not block future retries.
Comment on lines
+118
to
+121
| shouldRun(now: number, lastUserActivityAt?: number): boolean { | ||
| if (this.config.firstRunDeferred && !this.firstRunDone && this.lastRunAt === 0) { | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
shouldRun() first-run deferral can deadlock preflight callers.
shouldRun() returns false for initial deferred state, but only run() clears that state. Any caller that checks shouldRun() before calling run() can end up never running the deferral step.
Proposed fix
shouldRun(now: number, lastUserActivityAt?: number): boolean {
- if (this.config.firstRunDeferred && !this.firstRunDone && this.lastRunAt === 0) {
- return false
- }
-
if (now - this.lastRunAt < this.config.intervalMs) {
return false
}Also applies to: 155-158
🤖 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 `@src/services/self-improving/CuratorService.ts` around lines 118 - 121, The
current shouldRun(now:number, lastUserActivityAt?:number) implementation
short-circuits and returns false when config.firstRunDeferred is true,
firstRunDone is false and lastRunAt === 0, which prevents callers that only call
shouldRun() from ever invoking run() to clear the deferred state; change
shouldRun so it does not permanently block the initial deferred state—allow it
to return true (or otherwise indicate runnable) when config.firstRunDeferred &&
!this.firstRunDone && this.lastRunAt === 0 so callers can proceed to call run(),
and apply the same change to the other identical check around lines 155-158;
refer to the shouldRun and run methods and the config.firstRunDeferred,
firstRunDone and lastRunAt fields when making the change.
Comment on lines
+124
to
+126
| const existing = existingPatterns.find( | ||
| (pattern) => pattern.patternType === "tool" && pattern.summary.includes(toolKey), | ||
| ) |
Contributor
There was a problem hiding this comment.
Match existing patterns by structured context, not summary substring.
Line 124 and Line 188 rely on summary.includes(...), which can update the wrong pattern when names overlap.
Suggested fix
- const existing = existingPatterns.find(
- (pattern) => pattern.patternType === "tool" && pattern.summary.includes(toolKey),
- )
+ const existing = existingPatterns.find((pattern) => {
+ if (pattern.patternType !== "tool") return false
+ const existingKey = [...(pattern.context.toolNames ?? [])].sort().join(",")
+ return existingKey === toolKey
+ })- const existing = existingPatterns.find(
- (pattern) => pattern.patternType === "prompt" && pattern.summary.includes(toolName),
- )
+ const existing = existingPatterns.find((pattern) => {
+ if (pattern.patternType !== "prompt") return false
+ return pattern.context.toolNames?.length === 1 && pattern.context.toolNames[0] === toolName
+ })Also applies to: 188-190
🤖 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 `@src/services/self-improving/PatternAnalyzer.ts` around lines 124 - 126, The
current lookup uses summary.includes(...) which can collide on overlapping
names; update the matching in PatternAnalyzer where existingPatterns.find(...)
is used (the call that checks pattern.patternType === "tool" &&
pattern.summary.includes(toolKey) and the similar check around lines 188-190) to
compare a structured field instead of a substring: match pattern.patternType ===
"tool" and pattern.context?.toolKey === toolKey (or pattern.metadata?.toolKey
=== toolKey if your Pattern shape uses metadata), falling back to an exact
summary equality only if the structured key is absent; modify the code paths
that create patterns to populate that structured toolKey field so future lookups
use the stable identifier.
Comment on lines
+166
to
+187
| try { | ||
| const event = this.runtime.feedbackCollector.createTaskEvent(info) | ||
| this.runtime.store.addEvent(event) | ||
| this.lastUserActivityAt = event.timestamp | ||
| await this.transcriptRecall.record({ | ||
| id: event.id, | ||
| timestamp: event.timestamp, | ||
| taskId: info.taskId, | ||
| mode: info.mode, | ||
| summary: info.success | ||
| ? `Task completed: ${info.mode || "unknown"}` | ||
| : `Task failed: ${info.errorKey || "unknown"}`, | ||
| signal: info.success ? "TASK_SUCCESS" : "TASK_FAILURE", | ||
| workspacePath: info.workspacePath, | ||
| toolNames: info.toolNames, | ||
| errorKey: info.errorKey, | ||
| success: info.success, | ||
| }) | ||
| this.runtime.store.incrementToolIterations( | ||
| Math.max(1, info.toolIterationCount ?? info.toolNames?.length ?? 1), | ||
| ) | ||
| await this.checkReviewTriggers(this.runtime.store) |
Contributor
There was a problem hiding this comment.
Avoid dereferencing this.runtime after await in in-flight operations.
dispose() can clear this.runtime while these methods are mid-flight, which can throw at the next this.runtime.store... access after an await. Capture a local runtime snapshot and use it throughout each method.
Proposed fix pattern
async runReviewCycle(): Promise<void> {
if (!SelfImprovingManager.isExperimentEnabled(this.getExperiments())) {
return
}
-
- if (!this.started || !this.runtime) {
+ const runtime = this.runtime
+ if (!this.started || !runtime) {
return
}
@@
- const events = [...this.runtime.store.getRecentEvents()] as LearningEvent[]
+ const events = [...runtime.store.getRecentEvents()] as LearningEvent[]
@@
- const existingPatterns = [...this.runtime.store.getPatterns()] as LearnedPattern[]
- const newPatterns = this.runtime.patternAnalyzer.analyze(events, existingPatterns)
+ const existingPatterns = [...runtime.store.getPatterns()] as LearnedPattern[]
+ const newPatterns = runtime.patternAnalyzer.analyze(events, existingPatterns)
@@
- this.runtime.store.addPattern(pattern)
+ runtime.store.addPattern(pattern)
@@
- const actions = this.runtime.improvementApplier.generateActions([
- ...this.runtime.store.getPatterns(),
+ const actions = runtime.improvementApplier.generateActions([
+ ...runtime.store.getPatterns(),
] as LearnedPattern[])
@@
- this.runtime.store.addAction(action)
+ runtime.store.addAction(action)
@@
- const pendingActions = [...this.runtime.store.getPendingActions()] as ImprovementAction[]
+ const pendingActions = [...runtime.store.getPendingActions()] as ImprovementAction[]
@@
- this.runtime.store.removeAction(actionId)
+ runtime.store.removeAction(actionId)
@@
- this.updateReviewTelemetry(this.runtime.store, actions)
+ this.updateReviewTelemetry(runtime.store, actions)
this.promptRevision += 1
- this.runtime.store.resetCounters()
- await this.runtime.store.persist()
+ runtime.store.resetCounters()
+ await runtime.store.persist()Also applies to: 268-303, 327-334, 462-464
🤖 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 `@src/services/self-improving/SelfImprovingManager.ts` around lines 166 - 187,
The code dereferences this.runtime after await points which can be cleared by
dispose(); fix by capturing a local snapshot at the top of the method/block
(e.g. const runtime = this.runtime) and use runtime everywhere in this block
(runtime.feedbackCollector.createTaskEvent, runtime.store.addEvent,
runtime.store.incrementToolIterations, runtime.store passed into
checkReviewTriggers) and similarly snapshot any other late-bound dependencies
you use across awaits (e.g. const transcriptRecall = this.transcriptRecall) so
all subsequent awaits reference the locals instead of this.runtime; apply the
same pattern to the other affected blocks referenced (around the createTaskEvent
/ transcriptRecall.record / checkReviewTriggers usages).
Comment on lines
+269
to
+272
| const events = [...this.runtime.store.getRecentEvents()] as LearningEvent[] | ||
| if (events.length === 0) { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Reset counters when a review trigger fires but there are no events.
If checkReviewTriggers() calls runReviewCycle() from user-turn/tool counters and getRecentEvents() is empty, the early return leaves counters unchanged. That keeps the threshold perpetually met and re-triggers review checks on every subsequent signal.
Proposed fix
const events = [...this.runtime.store.getRecentEvents()] as LearningEvent[]
if (events.length === 0) {
+ this.runtime.store.resetCounters()
+ await this.runtime.store.persist()
return
}Also applies to: 515-523
🤖 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 `@src/services/self-improving/SelfImprovingManager.ts` around lines 269 - 272,
When runReviewCycle() is invoked by checkReviewTriggers() and getRecentEvents()
returns an empty array, ensure the user-turn/tool review counters are reset
before returning so the threshold doesn't remain met; in SelfImprovingManager
locate the block using this.runtime.store.getRecentEvents() (and the analogous
block around lines 515-523) and add logic to clear or reset the relevant
counters on this.runtime (e.g., the userTurnCounter/toolTurnCounter or whichever
fields track review signals) immediately prior to the early return, then return
as before.
Comment on lines
+133
to
+135
| private queuePersist(): void { | ||
| void this.persist() | ||
| } |
Contributor
There was a problem hiding this comment.
Serialize persistence writes to prevent stale overwrite.
Line 133 launches unawaited writes; if two persists overlap, an older write can finish after a newer one and clobber newer disk state.
Suggested fix
export class SkillUsageStore {
+ private persistChain: Promise<void> = Promise.resolve()
+
private async persist(): Promise<void> {
try {
await safeWriteJson(this.filePath, Array.from(this.records.values()), { prettyPrint: true })
} catch (error) {
this.logger.appendLine(
`[SkillUsageStore] Persist error: ${error instanceof Error ? error.message : String(error)}`,
)
}
}
+ private persistSequentially(): Promise<void> {
+ this.persistChain = this.persistChain.then(() => this.persist())
+ return this.persistChain
+ }
+
private queuePersist(): void {
- void this.persist()
+ void this.persistSequentially()
}- await this.persist()
+ await this.persistSequentially()(Apply the second replacement to all awaited persist call sites.)
Also applies to: 229-229
🤖 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 `@src/services/self-improving/SkillUsageStore.ts` around lines 133 - 135, The
unawaited writes from queuePersist() (calling persist()) can overlap and cause
stale overwrites; fix by serializing persists with a single in-flight promise
chain: add a private field (e.g., persistPromise: Promise<void> | null) and
change queuePersist() to chain the new persist() onto persistPromise
(persistPromise = (persistPromise ?? Promise.resolve()).then(() =>
this.persist(), () => this.persist())), ensuring each persist runs after the
previous completes; also update all other call sites that currently call
this.persist() unawaited (the other persisted call sites referenced around the
block at line ~229) to use the same chaining approach or await the resulting
persistPromise so no two persists run concurrently.
Comment on lines
+119
to
+121
| if (Array.isArray(parsed)) { | ||
| this.entries = parsed.slice(-TranscriptRecall.MAX_ENTRIES) | ||
| } |
Contributor
There was a problem hiding this comment.
Sanitize loaded transcript entries before storing in memory.
Line 119 currently trusts persisted JSON shape; malformed entries can crash search() when .toLowerCase() is called.
Suggested fix
+ private sanitizeEntry(value: unknown): TranscriptEntry | null {
+ if (!value || typeof value !== "object") return null
+ const candidate = value as Partial<TranscriptEntry>
+ if (typeof candidate.id !== "string") return null
+ if (typeof candidate.timestamp !== "number") return null
+ if (typeof candidate.summary !== "string") return null
+ if (typeof candidate.signal !== "string") return null
+ return {
+ ...candidate,
+ id: candidate.id,
+ timestamp: candidate.timestamp,
+ summary: candidate.summary,
+ signal: candidate.signal,
+ toolNames: Array.isArray(candidate.toolNames) ? [...candidate.toolNames] : undefined,
+ }
+ }
+
private async loadFromDisk(): Promise<void> {
try {
const raw = await fs.readFile(this.filePath, "utf-8")
const parsed = JSON.parse(raw)
if (Array.isArray(parsed)) {
- this.entries = parsed.slice(-TranscriptRecall.MAX_ENTRIES)
+ this.entries = parsed
+ .map((entry) => this.sanitizeEntry(entry))
+ .filter((entry): entry is TranscriptEntry => entry !== null)
+ .slice(-TranscriptRecall.MAX_ENTRIES)
}📝 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 (Array.isArray(parsed)) { | |
| this.entries = parsed.slice(-TranscriptRecall.MAX_ENTRIES) | |
| } | |
| private sanitizeEntry(value: unknown): TranscriptEntry | null { | |
| if (!value || typeof value !== "object") return null | |
| const candidate = value as Partial<TranscriptEntry> | |
| if (typeof candidate.id !== "string") return null | |
| if (typeof candidate.timestamp !== "number") return null | |
| if (typeof candidate.summary !== "string") return null | |
| if (typeof candidate.signal !== "string") return null | |
| return { | |
| ...candidate, | |
| id: candidate.id, | |
| timestamp: candidate.timestamp, | |
| summary: candidate.summary, | |
| signal: candidate.signal, | |
| toolNames: Array.isArray(candidate.toolNames) ? [...candidate.toolNames] : undefined, | |
| } | |
| } | |
| private async loadFromDisk(): Promise<void> { | |
| try { | |
| const raw = await fs.readFile(this.filePath, "utf-8") | |
| const parsed = JSON.parse(raw) | |
| if (Array.isArray(parsed)) { | |
| this.entries = parsed | |
| .map((entry) => this.sanitizeEntry(entry)) | |
| .filter((entry): entry is TranscriptEntry => entry !== null) | |
| .slice(-TranscriptRecall.MAX_ENTRIES) | |
| } | |
| } | |
| } |
🤖 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 `@src/services/self-improving/TranscriptRecall.ts` around lines 119 - 121, The
persisted JSON may contain malformed entries that break search() when it calls
.toLowerCase(); in the Array.isArray(parsed) branch where this.entries is set,
validate and sanitize each item before storing: iterate over
parsed.slice(-TranscriptRecall.MAX_ENTRIES), ensure each entry is an object with
the expected fields (e.g., a string 'text' property), coerce or normalize types
(convert text to String and trim, or skip entries missing text), and assign the
cleaned array to this.entries so downstream code (search(), .toLowerCase())
cannot throw on unexpected shapes.
iskandarsulaili
commented
May 22, 2026
Author
iskandarsulaili
left a comment
iskandarsulaili
left a comment
There was a problem hiding this comment.
Commented in CodeRabbit Change Stack
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
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
1 participant
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.
Description
This PR adds an experiment-gated self-improving subsystem so Zoo can learn from task outcomes, user corrections, and recurring workflow patterns instead of repeatedly making the same corrected mistakes.
Key implementation details in this PR:
SelfImprovingManageras the orchestration layer for learning, review, and prompt-context generation.CuratorService,ReviewPromptFactory,TranscriptRecall,MemoryStore,SkillUsageStore) to keep retained guidance bounded and maintainable over time.selfImprovingexperiment flag and surfaces status through extension state/settings so it remains opt-in and low-risk.Design choices / trade-offs reviewers should note:
Test Procedure
selfImprovingexperiment in Zoo Code's experimental settings.SelfImprovingManagerLearningStoreMemoryStoreSkillUsageStoreCuratorServiceTranscriptRecallActionExecutorReviewPromptFactoryEnvironment:
selfImprovingexperiment enabled for feature-path validationPre-Submission Checklist
Documentation Updates
Recommended documentation updates:
selfImprovingexperiment does.Additional Notes
I did not find a directly matching open issue/PR in
Zoo-Code-Org/Zoo-Codefor this self-improving learning system. The issue is the critical product gap itself: Zoo currently has no bounded mechanism to retain lessons from corrected failures, repeated task outcomes, or recurring workflow signals, which makes it more likely to repeat avoidable mistakes over time. This PR is the implementation path toward solving that gap with an experiment-gated, bounded, and reversible design.Get in Touch
Discord: @iskandarsulaili
Summary by CodeRabbit
New Features
Documentation