feat: add experiment-gated self-improving learning, bounded memory, background curation, and auto-create/update skill#252
Open
iskandarsulaili wants to merge 25 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
…oryBackendFactory
iskandarsulaili
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
📝 WalkthroughWalkthroughAdds an experiment-gated "self-improving" subsystem: Zod types, memory backends, persistent LearningStore, event→pattern analysis, action generation/execution, a SelfImprovingManager orchestrator with timers/curator, prompt injection, provider integration, tests, and i18n strings. ChangesSelf-Improving Learning Subsystem
🎯 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: 14
🧹 Nitpick comments (2)
src/services/self-improving/types.ts (1)
78-78: ⚡ Quick winTighten
getExperimentsto the sharedExperimentscontract.Line 78 uses
Record<string, boolean>, which allows misspelled keys and bypasses compile-time checks for experiment flags.Suggested patch
import type { ActionType, + Experiments, FeedbackSignal, @@ - getExperiments: () => Record<string, boolean> | undefined + getExperiments: () => Experiments | undefined🤖 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` at line 78, Tighten the getExperiments signature to return the shared Experiments contract instead of a loose Record<string, boolean>; update the type of getExperiments from "Record<string, boolean> | undefined" to "Experiments | undefined", import or reference the shared Experiments type and adjust any callers of getExperiments to use the canonical keys defined by Experiments (e.g., update types in the exported interface where getExperiments is declared and ensure usage sites validate against the Experiments shape).src/services/self-improving/SelfImprovingManager.ts (1)
142-166: 💤 Low valueConsider disposing all initialized stores for completeness.
skillUsageStore,curatorService, andtranscriptRecallare initialized unconditionally in the constructor but are not disposed indispose(). If these stores hold file handles or other resources, they may leak when the manager is disposed.🤖 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 142 - 166, The dispose() method currently persists and disposes memoryStore but misses disposing other stores initialized in the constructor; update dispose() to also gracefully persist/close and dispose skillUsageStore, curatorService, and transcriptRecall (if they exist)—for example, call any available persist/flush methods before calling their dispose/close methods, using optional chaining or instanceof checks (e.g., this.skillUsageStore?.dispose?.(), this.curatorService?.dispose?.(), this.transcriptRecall?.dispose?.()) and ensure these calls occur inside the try block alongside memoryStore disposal so resources are released even on errors.
🤖 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/__tests__/learning-memory.test.ts`:
- Line 1: The test file header contains a stale Vitest command and should point
to the test's real path; update the comment on line 1 from "npx vitest run
src/__tests__/learning-memory.test.ts" to reference the correct location
"packages/types/src/__tests__/learning-memory.test.ts" (or use a
workspace-root-friendly command) so local runs target the actual file.
In `@packages/types/src/vscode-extension-host.ts`:
- Around line 364-372: Update the selfImprovingStatus type in
packages/types/src/vscode-extension-host.ts so it matches the object returned by
SelfImprovingManager.getStatus(): add the missing fields memoryEntries,
memoryBackend, skillRecords, and curatorStatus (with the same shapes/types used
by getStatus(), e.g., memoryEntries: number, memoryBackend?: string,
skillRecords: number, and curatorStatus as the status object returned by the
manager). Preferably import or reference the getStatus() return/interface if
available to keep types in sync and avoid duplication; otherwise add fields with
the concrete types used by SelfImprovingManager.getStatus().
In `@src/services/self-improving/__tests__/MemoryBackendFactory.spec.ts`:
- Around line 21-23: The test is not exercising the unknown-backend fallback
because it passes "builtin"; update the spec so MemoryBackendFactory.create is
invoked with an unsupported value (e.g., "unknown" or "unsupported") instead of
"builtin", then assert the returned backend is a MemoryStore; specifically
change the call to MemoryBackendFactory.create("unsupported" as any, baseDir,
logger) and keep the expect(backend).toBeInstanceOf(MemoryStore) assertion to
validate the fallback behavior.
In `@src/services/self-improving/__tests__/SkillUsageStore.spec.ts`:
- Around line 20-33: The test currently uses a fixed sleep after calling
SkillUsageStore.getOrCreate which is flaky; replace the setTimeout delay in
SkillUsageStore.spec.ts with a deterministic polling loop that waits for the
persisted file (self-improving/skill-usage.json) to exist and contain the
expected entry within a bounded timeout (e.g., 1–2s), checking repeatedly (using
fs.stat or readFile) and throwing if the timeout elapses; keep assertions the
same and reference SkillUsageStore.getOrCreate and the persisted file path when
implementing the poll.
In `@src/services/self-improving/AgentMemoryAdapter.ts`:
- Around line 191-199: The forgetByContent function currently will match
everything if substring is empty because
entry.content.toLowerCase().includes(substring.toLowerCase()) is true for "", so
add an early guard in forgetByContent to return 0 (or reject) when substring is
empty or only whitespace; check substring.trim().length === 0 before calling
this.search and bail out. Keep the existing case-insensitive check and the loop
that calls this.forget(entry.id) unchanged; reference the forgetByContent
method, the substring parameter, and the includes(...) check when making the
change.
In `@src/services/self-improving/CuratorService.ts`:
- Around line 118-121: The current shouldRun() condition uses this.firstRunDone,
causing shouldRun() to always return false before run() ever executes and
creating a deadlock; remove the dependency on this.firstRunDone from the gating
logic so shouldRun() no longer permanently denies the first execution —
specifically update the condition in shouldRun() (the method named shouldRun in
CuratorService) to not include "!this.firstRunDone" (keep deferral behavior
based on config and lastRunAt if needed) and leave setting this.firstRunDone
inside run() unchanged.
In `@src/services/self-improving/LearningStore.ts`:
- Around line 186-191: The persist() implementation currently writes STATE_FILE,
pattern files, archive files and the index in parallel (via safeWriteJson,
persistPatternFiles, persistPatternFiles, writePatternIndex), which can leave
files inconsistent on partial failure; change persist() to first persist all
pattern artifacts and the index (call persistPatternFiles for this.patternsDir
and this.archiveDir and writePatternIndex sequentially or in a grouped
Promise.all) and only after those complete successfully write the authoritative
state.json last using safeWriteJson (preferably write to a temp file and
atomically rename to STATE_FILE) so recovery reads one clear source of truth;
update function names referenced: persist(), safeWriteJson, persistPatternFiles,
writePatternIndex, and STATE_FILE.
In `@src/services/self-improving/MemoryStore.ts`:
- Around line 99-104: The recall method in MemoryStore concatenates
this.environment and this.userProfile then slices the tail, which can drop newer
entries if array order differs; modify the recall function (after await
this.ensureInitialized()) to merge both arrays, sort the combined array globally
by each entry's timestamp (newest first) and then take the top maxResults before
mapping to this.cloneEntry so results are globally time-accurate; use the
existing variables this.environment, this.userProfile, the recall function and
cloneEntry method to locate and change the logic.
- Around line 126-138: In forgetByContent, calling .includes on an empty string
will match every entry and wipe both stores; add a guard at the start of the
async forgetByContent(substring: string) method (after await
this.ensureInitialized()) to treat empty or whitespace-only substring safely
(e.g., if (!substring || substring.trim() === "") return 0 or throw a specific
error), so that environment and userProfile arrays are not filtered when
substring is empty; reference the forgetByContent function and the environment
and userProfile properties when making this change.
In `@src/services/self-improving/PatternAnalyzer.ts`:
- Around line 124-126: The merge logic in PatternAnalyzer that finds existing
patterns using pattern.summary.includes(toolKey) is fragile; change it to use a
structured context key instead (e.g., check pattern.context?.toolKey === toolKey
or pattern.context?.tools?.includes(toolKey) depending on how tools are
represented) so merges are exact and deterministic; update both the find at the
existingPatterns.find call (patternType "tool") and the similar check around
lines 188-190 to use the structured context property (pattern.context) rather
than substring matching on summary, and ensure any pattern
creation/serialization code sets that context key accordingly.
- Around line 193-199: When updating an existing tool-preference pattern in
PatternAnalyzer (the patterns.push block that spreads existing), preserve
cumulative frequency instead of overwriting it with the current batch count;
change the frequency assignment to use existing.frequency + total (e.g.,
frequency: (existing.frequency ?? 0) + total) so historical evidence is
retained, keep lastSeenAt, successRate and confidenceScore updates as-is, and
ensure you reference the same existing object/variable when computing the new
cumulative value.
In `@src/services/self-improving/TranscriptRecall.ts`:
- Around line 56-67: The record() method mutates and persists this.entries
without ensuring the instance is initialized; call await this.initialize() at
the start of TranscriptRecall.record() (or otherwise ensure initialize()
completes) before pushing/slicing entries and calling this.persist() so prior
on-disk state and any required directories are loaded/created; update references
to this.entries, TranscriptRecall.MAX_ENTRIES, and this.persist() to assume
initialization has completed.
- Around line 119-121: When loading parsed JSON into this.entries in
TranscriptRecall (the block that currently does if (Array.isArray(parsed)) {
this.entries = parsed.slice(-TranscriptRecall.MAX_ENTRIES) }), validate and
sanitize each item before assigning: ensure parsed is an array of objects, each
entry has the expected fields (at minimum a string summary usable by
entry.summary.toLowerCase()), coerce or skip malformed entries, and then take
the last TranscriptRecall.MAX_ENTRIES of the validated list; this prevents
runtime errors during search-time operations like entry.summary.toLowerCase().
In `@src/services/self-improving/types.ts`:
- Around line 104-138: DEFAULT_CONFIG and EMPTY_STATE duplicate canonical
defaults and should be removed here and imported from the single source of
truth; replace the local declarations by importing the canonical DEFAULT_CONFIG
and EMPTY_STATE (or the canonical factory functions) used by the types layer,
ensure the file references the LearningConfig and LearningState types and any
identifiers (DEFAULT_CONFIG, EMPTY_STATE) are updated to the imported symbols,
and remove or replace any direct mutations so the single canonical definitions
drive manager behavior.
---
Nitpick comments:
In `@src/services/self-improving/SelfImprovingManager.ts`:
- Around line 142-166: The dispose() method currently persists and disposes
memoryStore but misses disposing other stores initialized in the constructor;
update dispose() to also gracefully persist/close and dispose skillUsageStore,
curatorService, and transcriptRecall (if they exist)—for example, call any
available persist/flush methods before calling their dispose/close methods,
using optional chaining or instanceof checks (e.g.,
this.skillUsageStore?.dispose?.(), this.curatorService?.dispose?.(),
this.transcriptRecall?.dispose?.()) and ensure these calls occur inside the try
block alongside memoryStore disposal so resources are released even on errors.
In `@src/services/self-improving/types.ts`:
- Line 78: Tighten the getExperiments signature to return the shared Experiments
contract instead of a loose Record<string, boolean>; update the type of
getExperiments from "Record<string, boolean> | undefined" to "Experiments |
undefined", import or reference the shared Experiments type and adjust any
callers of getExperiments to use the canonical keys defined by Experiments
(e.g., update types in the exported interface where getExperiments is declared
and ensure usage sites validate against the Experiments shape).
🪄 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: 12dec26c-96df-46e2-aae0-7bc4f2cbfa8a
📒 Files selected for processing (61)
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/AgentMemoryAdapter.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/MemoryBackend.tssrc/services/self-improving/MemoryBackendFactory.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__/AgentMemoryAdapter.spec.tssrc/services/self-improving/__tests__/CuratorService.spec.tssrc/services/self-improving/__tests__/LearningStore.spec.tssrc/services/self-improving/__tests__/MemoryBackendFactory.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
Contributor
|
Hello. Thanks for your contribution! this looks like a really cool feature! Can you please break down your PR into smaller chunks so we can review it? |
Author
That might be challenging as it thight with each other. Any tips? |
- guard empty forgetByContent queries in both memory backends - sort recall results globally by timestamp across stores - add focused regression tests for deletion guards and recall ordering
- match tool-based patterns by structured context instead of summaries - preserve cumulative frequency for existing tool preferences - add regression tests for tool-combination and preference updates
- exercise unknown memory-backend fallback explicitly - replace fixed sleep with bounded polling in skill usage persistence test - correct the package types vitest file header path
- lazily initialize transcript recall before recording new entries - validate persisted transcript entries before loading them - add regression tests for lazy init and malformed data handling
- reuse shared learning defaults and empty state from @roo-code/types - tighten experiment access to the shared Experiments contract - expand webview selfImprovingStatus typing to match manager output
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/services/self-improving/__tests__/TranscriptRecall.spec.ts`:
- Around line 80-81: Update the test assertion in TranscriptRecall.spec.ts to
reflect that getRecent returns entries in newest-first (recency) order: change
the expectation on recall.getRecent(2).map(entry => entry.id) to assert
["entry-1", "entry-0"] so the test aligns with the intended semantics of the
getRecent function.
In `@src/services/self-improving/AgentMemoryAdapter.ts`:
- Around line 192-201: The deletion loop trims and lowercases the input into
normalized but still calls this.search(substring, 50) with the raw substring,
causing mismatches; change the search call in AgentMemoryAdapter (where
substring and normalized are defined) to use normalized instead (i.e.,
this.search(normalized, 50)) so the backend query and the local includes check
(entry.content.toLowerCase().includes(normalized)) use the same
trimmed/lowercased value.
In `@src/services/self-improving/PatternAnalyzer.ts`:
- Around line 193-198: The update currently replaces successRate when merging a
new batch into a pattern; change this to a weighted average so successRate is
cumulative: in PatternAnalyzer where you push into patterns (merging into
existing), compute combinedSuccess = (existing.successRate * existing.frequency
+ successRate * total) / (existing.frequency + total) and set successRate to
that value (using existing.frequency + total as the new frequency), while
keeping frequency, lastSeenAt, and confidenceScore updates as-is; reference the
variables existing, total, successRate and the patterns push/merge logic to
locate and apply the change.
In `@src/services/self-improving/TranscriptRecall.ts`:
- Around line 57-59: Concurrent record() calls can trigger multiple initialize()
runs and later loadFromDisk() can overwrite this.entries; fix by memoizing
initialization behind a shared promise (e.g., add this.initializingPromise).
Change initialize() to set and return this.initializingPromise (and clear
it/mark this.initialized on resolution), and in record() await
this.initializingPromise if present or assign it when calling initialize() so
all callers await the same promise; reference initialize(), record(),
loadFromDisk(), this.entries, and this.initialized when applying the change.
🪄 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: a5505315-4816-403d-b2e7-4c5f4cd50663
📒 Files selected for processing (14)
packages/types/src/__tests__/learning-memory.test.tspackages/types/src/vscode-extension-host.tssrc/services/self-improving/AgentMemoryAdapter.tssrc/services/self-improving/MemoryStore.tssrc/services/self-improving/PatternAnalyzer.tssrc/services/self-improving/SelfImprovingManager.tssrc/services/self-improving/TranscriptRecall.tssrc/services/self-improving/__tests__/AgentMemoryAdapter.spec.tssrc/services/self-improving/__tests__/MemoryBackendFactory.spec.tssrc/services/self-improving/__tests__/MemoryStore.spec.tssrc/services/self-improving/__tests__/PatternAnalyzer.spec.tssrc/services/self-improving/__tests__/SkillUsageStore.spec.tssrc/services/self-improving/__tests__/TranscriptRecall.spec.tssrc/services/self-improving/types.ts
Comment on lines
+80
to
+81
| expect(recall.getRecent(2).map((entry) => entry.id)).toEqual(["entry-0", "entry-1"]) | ||
| }) |
Contributor
There was a problem hiding this comment.
Fix getRecent ordering assertion to reflect recency semantics.
Line 80 currently expects oldest-first order after appending a newer entry. For a getRecent API, this should typically assert newest-first (["entry-1", "entry-0"]) to avoid locking in inverted behavior.
Suggested patch
- expect(recall.getRecent(2).map((entry) => entry.id)).toEqual(["entry-0", "entry-1"])
+ expect(recall.getRecent(2).map((entry) => entry.id)).toEqual(["entry-1", "entry-0"])📝 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
| expect(recall.getRecent(2).map((entry) => entry.id)).toEqual(["entry-0", "entry-1"]) | |
| }) | |
| expect(recall.getRecent(2).map((entry) => entry.id)).toEqual(["entry-1", "entry-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/__tests__/TranscriptRecall.spec.ts` around lines
80 - 81, Update the test assertion in TranscriptRecall.spec.ts to reflect that
getRecent returns entries in newest-first (recency) order: change the
expectation on recall.getRecent(2).map(entry => entry.id) to assert ["entry-1",
"entry-0"] so the test aligns with the intended semantics of the getRecent
function.
Comment on lines
+192
to
+201
| const normalized = substring.trim().toLowerCase() | ||
| if (!normalized) { | ||
| return 0 | ||
| } | ||
|
|
||
| const entries = await this.search(substring, 50) | ||
| let removed = 0 | ||
|
|
||
| for (const entry of entries) { | ||
| if (entry.content.toLowerCase().includes(normalized)) { |
Contributor
There was a problem hiding this comment.
Pass the normalized value into search().
The local match uses normalized, but the backend query still uses the raw substring. Inputs like " foo " can miss search results and leave matching memories undeleted. Use the trimmed value for both steps.
Suggested fix
- const entries = await this.search(substring, 50)
+ const entries = await this.search(normalized, 50)🤖 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/AgentMemoryAdapter.ts` around lines 192 - 201,
The deletion loop trims and lowercases the input into normalized but still calls
this.search(substring, 50) with the raw substring, causing mismatches; change
the search call in AgentMemoryAdapter (where substring and normalized are
defined) to use normalized instead (i.e., this.search(normalized, 50)) so the
backend query and the local includes check
(entry.content.toLowerCase().includes(normalized)) use the same
trimmed/lowercased value.
- search agentmemory with normalized forget queries - keep prompt-pattern success rates cumulative with weighted updates - add regression coverage for trimmed queries and cumulative scoring
- memoize first-time initialization behind a shared promise - prevent concurrent record calls from dropping loaded transcript entries - add regression coverage for concurrent lazy record paths
- pass trimmed substring to agentmemory search instead of lowercasing it - keep case-insensitive local verification for actual deletion - tighten regression coverage to assert trim-without-lowercase semantics
- record correction feedback when users reply to outstanding task asks - keep generic user-turn tracking intact for normal follow-up messages - add a regression test around interactive correction replies
- capture real codebase_search hit counts and top score for learning events - let self-improving accept explicit code index hit details from callers - add regression tests for the tool seam and manager event recording
- assert activation calls initializeSelfImproving through the provider - keep the coverage at the extension startup seam rather than only mocked provider construction
iskandarsulaili
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/skills/SkillsManager.ts (1)
408-456:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
SKILL.mdstructure before persisting create/update content.Line 427 only enforces non-empty text. If frontmatter is malformed or
namemismatches, the write succeeds butdiscoverSkills()can drop the skill, leaving a “created/updated” result that is not actually loadable.Proposed guard pattern
async createSkillFromContent( name: string, source: "global" | "project", description: string, content: string, modeSlugs?: string[], ): Promise<string> { + const parsed = matter(content) + const contentName = parsed.data?.name + const contentDescription = parsed.data?.description + + if (typeof contentName !== "string" || contentName !== name) { + throw new Error(t("skills:errors.name_format")) + } + if (typeof contentDescription !== "string") { + throw new Error(t("skills:errors.description_length", { length: 0 })) + } + if (!content.trim()) { throw new Error(t("skills:errors.description_length", { length: 0 })) } ... } async updateSkillContent( name: string, source: "global" | "project", content: string, mode?: string, ): Promise<void> { + const parsed = matter(content) + if (parsed.data?.name !== name) { + throw new Error(t("skills:errors.name_format")) + } + if (typeof parsed.data?.description !== "string" || !parsed.data.description.trim()) { + throw new Error(t("skills:errors.description_length", { length: 0 })) + } ... }Also applies to: 461-475
🤖 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/skills/SkillsManager.ts` around lines 408 - 456, The createSkillFromContent implementation writes SKILL.md after only checking non-empty content, which allows malformed frontmatter or mismatched names to be persisted and later dropped by discoverSkills(); before writing (in createSkillFromContent) parse and validate the SKILL.md structure (frontmatter fields, required title/name slug match, and any required metadata) using the same parser discoverSkills uses, and reject with a descriptive error if invalid; perform the same pre-write validation in the corresponding update function (the code around lines 461-475 likely updateSkillFromContent) and only write the file (or atomically write a temp file then rename) after validation succeeds so discoverSkills will reliably load the new/updated skill.
🧹 Nitpick comments (3)
src/core/tools/CodebaseSearchTool.ts (1)
259-277: ⚡ Quick winRemove unnecessary
as anyinContextProxy.spec.ts
ContextProxy.setValueis already typed assetValue<K extends keyof RooCodeSettings>(key: K, value: RooCodeSettings[K]), and@roo-code/typesglobalSettingsSchemaincludesmemoryBackend,agentMemoryUrl,selfImprovingScope, andselfImprovingAutoSkillsScope, so theas anycasts in the “should persist self-improving …” tests can be removed.🤖 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/tools/CodebaseSearchTool.ts` around lines 259 - 277, The tests in ContextProxy.spec.ts use unnecessary `as any` casts when calling ContextProxy.setValue; since setValue is typed as setValue<K extends keyof RooCodeSettings>(key: K, value: RooCodeSettings[K]) and the RooCodeSettings keys (memoryBackend, agentMemoryUrl, selfImprovingScope, selfImprovingAutoSkillsScope) are defined in the `@roo-code/types` globalSettingsSchema, remove the `as any` casts from the “should persist self-improving …” tests and pass the correctly typed values directly to ContextProxy.setValue so TypeScript correctly infers the types without forced casts.src/services/self-improving/AgentMemoryAdapter.ts (1)
191-207: 💤 Low valueMinor inconsistency: search uses
substring.trim()but filter usesnormalized(trimmed + lowercased).Line 197 searches with
substring.trim()while line 201 filters withnormalized(trimmed and lowercased). If the backend search is case-sensitive, this could return fewer results than expected, causing some matches to be missed.Consider using the same normalized value for consistency:
- const entries = await this.search(substring.trim(), 50) + const entries = await this.search(normalized, 50)🤖 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/AgentMemoryAdapter.ts` around lines 191 - 207, In AgentMemoryAdapter.forgetByContent, the call to this.search uses substring.trim() while the subsequent filter uses normalized (trimmed + lowercased), causing potential mismatches; update the search call to use the same normalized value (pass normalized into this.search) so that search and the content filter both use the identical trimmed/lowercased string, keeping the rest of the logic (including this.forget(entry.id) and removed counting) unchanged.src/core/tools/__tests__/CodebaseSearchTool.spec.ts (1)
39-50: ⚡ Quick winConsider extracting task mock setup into a test helper.
The task mock object is complex with nested structure (
providerRef.deref(),getSelfImprovingManager). If additional tests are added (as recommended), this setup will be duplicated. Consider extracting it into a helper function to improve maintainability and reduce duplication.♻️ Example helper pattern
function createMockTask(overrides?: { taskId?: string getSelfImprovingManager?: () => any cwd?: string }) { return { cwd: overrides?.cwd ?? "/workspace", taskId: overrides?.taskId ?? "task-1", consecutiveMistakeCount: 0, providerRef: { deref: vi.fn().mockReturnValue({ context: {}, getSelfImprovingManager: overrides?.getSelfImprovingManager ?? vi.fn().mockReturnValue(null), }), }, say: vi.fn().mockResolvedValue(undefined), } as any }🤖 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/tools/__tests__/CodebaseSearchTool.spec.ts` around lines 39 - 50, Extract the complex task mock in src/core/tools/__tests__/CodebaseSearchTool.spec.ts into a reusable helper (e.g., createMockTask) to avoid duplication: move the nested providerRef.deref() mock and getSelfImprovingManager wiring plus cwd, taskId, consecutiveMistakeCount, and say into the helper, accept optional overrides for taskId, cwd, and getSelfImprovingManager, and update tests to call createMockTask() instead of inlining the task object; ensure references to providerRef.deref, getSelfImprovingManager, and say remain identical so existing test assertions continue to work.
🤖 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 `@src/core/config/__tests__/ContextProxy.spec.ts`:
- Around line 259-267: The test uses unnecessary `as any` casts when calling
proxy.setValue for "memoryBackend" and "agentMemoryUrl"; remove those casts and
pass properly typed string literals so TypeScript infers the correct types from
RooCodeSettings/globalSettingsSchema, e.g. call proxy.setValue("memoryBackend",
"agentmemory") and proxy.setValue("agentMemoryUrl",
"http://agentmemory.internal:4001") and keep the subsequent assertions via
proxy.getGlobalSettings() unchanged to validate persistence.
- Around line 269-277: The test should assert that ContextProxy.setValue routes
the enum-typed keys to the global state updater instead of merely roundtripping
via getGlobalSettings; modify the test to call
proxy.setValue("selfImprovingScope", "workspace") and
proxy.setValue("selfImprovingAutoSkillsScope", "global") (without sprawling `as
any` casts), and verify that the mock/stub for updateGlobalState was called with
the expected key/value pairs (e.g., updateGlobalState("selfImprovingScope",
"workspace") and updateGlobalState("selfImprovingAutoSkillsScope", "global")),
while keeping the existing checks of getGlobalSettings minimal or removing
them—this ensures setValue uses the global routing logic (not
SECRET_STATE_KEYS/GLOBAL_SECRET_KEYS) and preserves type-safety against the
z.enum-backed globalSettingsSchema.
In `@src/core/task/Task.ts`:
- Around line 1472-1483: The code currently awaits
provider.getSelfImprovingManager?.().recordUserCorrection(...) (inside the
shouldRecordCorrection branch) which blocks submitUserMessage flow and delays
handleWebviewAskResponse; change this to fire-and-forget: call
provider.getSelfImprovingManager?.().recordUserCorrection({...}) without
awaiting (e.g., prefix with void or chain .catch) so it runs asynchronously,
still capturing/logging errors (use .catch((e)=>console.error(...))). Keep the
same payload (taskId: this.taskId, success: false, corrected: true) and leave
the shouldRecordCorrection check in place.
In `@src/core/tools/__tests__/CodebaseSearchTool.spec.ts`:
- Around line 18-68: The test only covers the happy path for
CodebaseSearchTool.execute; add unit tests to cover error and edge cases:
simulate CodeIndexManager.getInstance().searchIndex throwing an error and assert
callbacks.handleError is called; mock getSelfImprovingManager() returning
null/undefined and assert no recordCodeIndexEvent calls and tool still returns
gracefully; make recordCodeIndexEvent reject and assert handleError/logging
behavior; add tests for empty searchIndex results, isFeatureEnabled=false,
isFeatureConfigured=false, results missing score, and multiple search results to
verify hits/topScore aggregation and that vscode.workspace.asRelativePath is
invoked; use the existing symbols CodebaseSearchTool,
CodeIndexManager.getInstance, searchIndex, getSelfImprovingManager,
recordCodeIndexEvent and execute to locate where to add/modify specs.
In `@src/core/webview/ClineProvider.ts`:
- Around line 1944-1946: getGlobalStateSafe currently casts this.contextProxy to
any and uses optional chaining, losing type safety; change its signature to
accept key: keyof GlobalState (or a generic K extends keyof GlobalState) and
call this.contextProxy.getGlobalState(key) directly (remove the any cast and ?.
usage) so the returned type is tied to GlobalState (e.g., GlobalState[K] or
generic T constrained to GlobalState[keyof GlobalState]); update the function
name getGlobalStateSafe and any callers to satisfy the stronger typed key and
return type.
In `@src/services/self-improving/ImprovementApplier.ts`:
- Line 40: The default hasSkill fallback in ImprovementApplier currently ignores
the second parameter; update the default assignment for this.hasSkill (which now
uses options.hasSkill) to accept both (name, source) and return false by default
to avoid accidental matches (i.e., use (name: string, source?: string) =>
false), ensuring callers that pass (skillName, source) receive the safer
behavior; keep the real implementation supplied via options.hasSkill when
auto-skills are enabled and leave getSkillNames untouched.
In `@src/services/skills/SkillsManager.ts`:
- Around line 467-471: The current lookup uses getSkill(name, source, mode)
which fails for skills whose keys are stored under the primary mode slug while
the update passes a secondary mode (e.g., modeSlugs: ["code","architect"]);
change the lookup to fall back to finding by name/source and verifying the
provided mode against the skill's modeSlugs: call this.getSkill(...) first, and
if it returns undefined and mode is provided, call
this.findSkillByNameAndSource(name, source) and ensure the returned skill exists
and (if mode) skill.modeSlugs includes the requested mode before proceeding
(otherwise throw the same not_found error); update the logic around getSkill,
findSkillByNameAndSource, and modeSlugs accordingly.
In `@webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx`:
- Around line 515-587: The tests for SettingsView (in the two "shows ..." it
blocks) currently only assert vscode.postMessage after clicking the save button,
which misses regressions where inputs leak immediately to live state; update
both tests (the one exercising auto-skills controls and the one exercising
memory backend/agentmemory) to assert vscode.postMessage was not called before
any user edits (after getSettingsContent/hydration) and again after performing
user interactions but before clicking the save button (i.e. before invoking
saveButton click) to ensure the cached-state pattern in SettingsView
(renderSettingsView, activateTab, getSettingsContent,
experimental-self-improving-checkbox, self-improving-* inputs) is preserved.
In `@webview-ui/src/components/settings/ExperimentalSettings.tsx`:
- Around line 160-162: Hardcoded user-facing labels "Workspace", "Global", and
"Built-in" in the SelectItem components of ExperimentalSettings.tsx should be
replaced with calls to the project's localization helper (the same translation
function used elsewhere in this file, e.g. the `t`/`formatMessage` from the
existing i18n hook) so the UI strings are localizable; update the SelectItem
children for values "workspace", "global", and "builtin" (and the other
occurrences around the file) to use translation keys like
`settings.scope.workspace`, `settings.scope.global`, `settings.scope.builtin`,
and add those keys to the translation resource files.
---
Outside diff comments:
In `@src/services/skills/SkillsManager.ts`:
- Around line 408-456: The createSkillFromContent implementation writes SKILL.md
after only checking non-empty content, which allows malformed frontmatter or
mismatched names to be persisted and later dropped by discoverSkills(); before
writing (in createSkillFromContent) parse and validate the SKILL.md structure
(frontmatter fields, required title/name slug match, and any required metadata)
using the same parser discoverSkills uses, and reject with a descriptive error
if invalid; perform the same pre-write validation in the corresponding update
function (the code around lines 461-475 likely updateSkillFromContent) and only
write the file (or atomically write a temp file then rename) after validation
succeeds so discoverSkills will reliably load the new/updated skill.
---
Nitpick comments:
In `@src/core/tools/__tests__/CodebaseSearchTool.spec.ts`:
- Around line 39-50: Extract the complex task mock in
src/core/tools/__tests__/CodebaseSearchTool.spec.ts into a reusable helper
(e.g., createMockTask) to avoid duplication: move the nested providerRef.deref()
mock and getSelfImprovingManager wiring plus cwd, taskId,
consecutiveMistakeCount, and say into the helper, accept optional overrides for
taskId, cwd, and getSelfImprovingManager, and update tests to call
createMockTask() instead of inlining the task object; ensure references to
providerRef.deref, getSelfImprovingManager, and say remain identical so existing
test assertions continue to work.
In `@src/core/tools/CodebaseSearchTool.ts`:
- Around line 259-277: The tests in ContextProxy.spec.ts use unnecessary `as
any` casts when calling ContextProxy.setValue; since setValue is typed as
setValue<K extends keyof RooCodeSettings>(key: K, value: RooCodeSettings[K]) and
the RooCodeSettings keys (memoryBackend, agentMemoryUrl, selfImprovingScope,
selfImprovingAutoSkillsScope) are defined in the `@roo-code/types`
globalSettingsSchema, remove the `as any` casts from the “should persist
self-improving …” tests and pass the correctly typed values directly to
ContextProxy.setValue so TypeScript correctly infers the types without forced
casts.
In `@src/services/self-improving/AgentMemoryAdapter.ts`:
- Around line 191-207: In AgentMemoryAdapter.forgetByContent, the call to
this.search uses substring.trim() while the subsequent filter uses normalized
(trimmed + lowercased), causing potential mismatches; update the search call to
use the same normalized value (pass normalized into this.search) so that search
and the content filter both use the identical trimmed/lowercased string, keeping
the rest of the logic (including this.forget(entry.id) and removed counting)
unchanged.
🪄 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: ff3a3c42-ec1c-4daf-8459-f45b0c317ce7
📒 Files selected for processing (36)
packages/types/src/experiment.tspackages/types/src/global-settings.tspackages/types/src/learning.tspackages/types/src/vscode-extension-host.tssrc/__tests__/extension.spec.tssrc/core/config/__tests__/ContextProxy.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/tools/CodebaseSearchTool.tssrc/core/tools/__tests__/CodebaseSearchTool.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tssrc/services/self-improving/ActionExecutor.tssrc/services/self-improving/AgentMemoryAdapter.tssrc/services/self-improving/ImprovementApplier.tssrc/services/self-improving/PatternAnalyzer.tssrc/services/self-improving/SelfImprovingManager.tssrc/services/self-improving/TranscriptRecall.tssrc/services/self-improving/__tests__/ActionExecutor.spec.tssrc/services/self-improving/__tests__/AgentMemoryAdapter.spec.tssrc/services/self-improving/__tests__/ImprovementApplier.spec.tssrc/services/self-improving/__tests__/PatternAnalyzer.spec.tssrc/services/self-improving/__tests__/SelfImprovingManager.spec.tssrc/services/self-improving/__tests__/TranscriptRecall.spec.tssrc/services/self-improving/types.tssrc/services/skills/SkillsManager.tssrc/services/skills/__tests__/SkillsManager.spec.tssrc/shared/__tests__/experiments.spec.tssrc/shared/experiments.tswebview-ui/src/components/settings/ExperimentalFeature.tsxwebview-ui/src/components/settings/ExperimentalSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/components/settings/__tests__/SettingsView.spec.tsxwebview-ui/src/i18n/locales/en/settings.json
✅ Files skipped from review due to trivial changes (1)
- src/shared/tests/experiments.spec.ts
Comment on lines
+259
to
+267
| it("should persist self-improving memory backend settings in global settings", async () => { | ||
| await proxy.setValue("memoryBackend" as any, "agentmemory" as any) | ||
| await proxy.setValue("agentMemoryUrl" as any, "http://agentmemory.internal:4001" as any) | ||
|
|
||
| const globalSettings = proxy.getGlobalSettings() as any | ||
|
|
||
| expect(globalSettings.memoryBackend).toBe("agentmemory") | ||
| expect(globalSettings.agentMemoryUrl).toBe("http://agentmemory.internal:4001") | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if memoryBackend and agentMemoryUrl are defined in type files
rg -n --type=ts -C3 'memoryBackend|agentMemoryUrl' 'packages/types/src/'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 2602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show surrounding tests (around lines 244-267 and 259-267)
spec="src/core/config/__tests__/ContextProxy.spec.ts"
if [ -f "$spec" ]; then
echo "=== $spec (excerpt) ==="
nl -ba "$spec" | sed -n '220,290p'
else
echo "Missing $spec" >&2
fi
# 2) Locate ContextProxy implementation
echo "=== Locate ContextProxy implementation ==="
fd -a "ContextProxy" src/core/config 2>/dev/null || true
# fallback: search for "class ContextProxy" / "setValue("
rg -n --hidden --no-ignore -S "class ContextProxy|setValue\\(" src/core/config | head -n 50
# 3) If we can find an implementation file, print the setValue method body vicinity
impl_files=$(rg -l --hidden --no-ignore -S "class ContextProxy" src/core/config || true)
if [ -z "$impl_files" ]; then
impl_files=$(rg -l --hidden --no-ignore -S "setValue\\(" src/core/config || true)
fi
for f in $impl_files; do
echo "=== Candidate impl: $f (setValue-related excerpts) ==="
# Print lines around setValue + routing-related calls if present
nl -ba "$f" | sed -n '1,200p' >/dev/null 2>&1 || true
rg -n "setValue\\(|updateGlobalState|storeSecret|getGlobalSettings" "$f" | head -n 80
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec="src/core/config/__tests__/ContextProxy.spec.ts"
echo "=== $spec (excerpt) ==="
if [ -f "$spec" ]; then
# show lines 220-290 with line numbers using cat -n
cat -n "$spec" | sed -n '220,290p'
else
echo "Missing $spec" >&2
fi
echo
echo "=== Locate ContextProxy implementation ==="
# List candidate files under src/core/config
rg -n --hidden --no-ignore -S "class ContextProxy" src/core/config | head -n 50 || true
rg -n --hidden --no-ignore -S "setValue\\(" src/core/config | head -n 50 || true
# Pick the most likely implementation file(s) and print setValue-related lines and nearby routing calls
impl_files=$(rg -l --hidden --no-ignore -S "class ContextProxy" src/core/config || true)
if [ -z "$impl_files" ]; then
impl_files=$(rg -l --hidden --no-ignore -S "setValue\\(" src/core/config || true)
fi
for f in $impl_files; do
echo
echo "=== Candidate impl: $f (routing-related snippets) ==="
# Show some setValue and global-state/secret calls around them
rg -n "setValue\\(|updateGlobalState|storeSecret|getGlobalSettings" "$f" | head -n 120 || true
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/core/config/ContextProxy.ts"
echo "=== $file (key sections) ==="
# Show imports/types for GlobalStateKey/SecretStateKey/GlobalSettings if present near top
echo
echo "--- Top (first 120 lines) ---"
sed -n '1,160p' "$file"
# Show GlobalSettings getter + setValue routing area
echo
echo "--- Around setValue implementation (lines 480-570) ---"
sed -n '480,570p' "$file"
# Show updateGlobalState and storeSecret implementations
echo
echo "--- updateGlobalState / storeSecret (lines 320-460) ---"
sed -n '320,460p' "$file"
# Show getGlobalSettings implementation and any mapping
echo
echo "--- getGlobalSettings (lines 410-520 approx) ---"
sed -n '410,520p' "$file"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 17172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate RooCodeSettings type definition ==="
rg -n --type=ts "type RooCodeSettings|interface RooCodeSettings|export type RooCodeSettings|export interface RooCodeSettings" packages/types/src
echo
echo "=== Inspect memoryBackend/agentMemoryUrl within RooCodeSettings ==="
rg -n --type=ts -C3 "RooCodeSettings.*memoryBackend|memoryBackend.*RooCodeSettings|agentMemoryUrl.*RooCodeSettings|RooCodeSettings.*agentMemoryUrl" packages/types/src
echo
echo "=== Locate GLOBAL_STATE_KEYS / GLOBAL_SETTINGS_KEYS inclusion ==="
rg -n --type=ts "GLOBAL_STATE_KEYS|GLOBAL_SETTINGS_KEYS" packages/types/srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All occurrences of memoryBackend / agentMemoryUrl in packages/types/src ==="
rg -n --type=ts "memoryBackend|agentMemoryUrl" packages/types/src | head -n 200
echo
echo "=== Show global-settings.ts around RooCodeSettings + relevant schemas ==="
file="packages/types/src/global-settings.ts"
if [ -f "$file" ]; then
# show around where RooCodeSettings is declared
rg -n "export type RooCodeSettings" "$file"
line=$(rg -n "export type RooCodeSettings" "$file" | head -n1 | cut -d: -f1)
start=$((line-80))
end=$((line+80))
echo "--- $file ($start-$end) ---"
sed -n "${start},${end}p" "$file"
else
echo "Missing $file" >&2
fi
echo
echo "=== Locate key lists used for routing in ContextProxy ==="
rg -n --type=ts "GLOBAL_STATE_KEYS|GLOBAL_SETTINGS_KEYS|GLOBAL_SECRET_KEYS|SECRET_STATE_KEYS|isSecretStateKey|GLOBAL_SECRET_KEYS" packages/types/src | head -n 200Repository: Zoo-Code-Org/Zoo-Code
Length of output: 7566
Remove unnecessary as any casts for memoryBackend / agentMemoryUrl
memoryBackend and agentMemoryUrl are already present in @roo-code/types (globalSettingsSchema → RooCodeSettings), and ContextProxy.setValue will route them through updateGlobalState (they’re not secret keys). Since the test already asserts via getGlobalSettings(), the as any casts can be removed to improve type safety and consistency.
🤖 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/config/__tests__/ContextProxy.spec.ts` around lines 259 - 267, The
test uses unnecessary `as any` casts when calling proxy.setValue for
"memoryBackend" and "agentMemoryUrl"; remove those casts and pass properly typed
string literals so TypeScript infers the correct types from
RooCodeSettings/globalSettingsSchema, e.g. call proxy.setValue("memoryBackend",
"agentmemory") and proxy.setValue("agentMemoryUrl",
"http://agentmemory.internal:4001") and keep the subsequent assertions via
proxy.getGlobalSettings() unchanged to validate persistence.
Comment on lines
+269
to
+277
| it("should persist self-improving scope settings in global settings", async () => { | ||
| await proxy.setValue("selfImprovingScope" as any, "workspace" as any) | ||
| await proxy.setValue("selfImprovingAutoSkillsScope" as any, "global" as any) | ||
|
|
||
| const globalSettings = proxy.getGlobalSettings() as any | ||
|
|
||
| expect(globalSettings.selfImprovingScope).toBe("workspace") | ||
| expect(globalSettings.selfImprovingAutoSkillsScope).toBe("global") | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if scope settings are defined in type files and GLOBAL_STATE_KEYS
rg -n --type=ts -C3 'selfImprovingScope|selfImprovingAutoSkillsScope' 'packages/types/src/'
# Check if they're in GLOBAL_STATE_KEYS or SECRET_STATE_KEYS
rg -n --type=ts 'selfImprovingScope|selfImprovingAutoSkillsScope' --glob '**/types/**/index.ts' --glob '**/types/**/*.ts'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how these keys are stored/routed (GLOBAL_STATE_KEYS/SECRET_STATE_KEYS) and where getGlobalSettings/setValue are implemented.
rg -n --hidden --no-ignore -S 'GLOBAL_STATE_KEYS|SECRET_STATE_KEYS' .
rg -n -S 'getGlobalSettings\(|setValue\(' src/core/config .
# Find all usages of the specific keys outside type definitions.
rg -n -S 'selfImprovingScope|selfImprovingAutoSkillsScope' src/core packages . \
--glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 19653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where these keys are declared in packages/types/src/global-settings.ts (arrays and schemas)
rg -n "selfImprovingScope|selfImprovingAutoSkillsScope" packages/types/src/global-settings.ts -n -C2
# Show how GLOBAL_STATE_KEYS is constructed (so we can see whether these keys land there)
rg -n "export const GLOBAL_STATE_KEYS" packages/types/src/global-settings.ts -n -C6
# Show ContextProxy.setValue routing logic for global vs secret keys
rg -n "async setValue\\(|setValue\\(" src/core/config/ContextProxy.ts -n -C3
rg -n "getGlobalSettings\\(" src/core/config/ContextProxy.ts -n -C10
# Pull the relevant setValue implementation block
perl -0777 -ne 'print $1 if /public async setValue\([^)]*\)\s*:\s*Promise<[^>]*>\s*{[^}]*\}/s' src/core/config/ContextProxy.ts | head -n 120
# Also show getGlobalState/getSecret helpers if present
rg -n "getGlobalStateSafe|getGlobalState\\(|getSecret\\(" src/core/config/ContextProxy.ts -n -C3Repository: Zoo-Code-Org/Zoo-Code
Length of output: 2988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate setValue method in ContextProxy.ts
rg -n "public (async )?setValue\\b" src/core/config/ContextProxy.ts
# Print surrounding lines for the setValue implementation
# (use sed with line numbers from rg output)
sed -n '520,620p' src/core/config/ContextProxy.ts
# Print getValues/getValue and isSecretStateKey helpers usage near bottom
sed -n '620,760p' src/core/config/ContextProxy.ts
# Print getValues() body specifically (search for "public getValues()" first)
rg -n "public getValues\\(" src/core/config/ContextProxy.ts
# Then print around it
sed -n '540,620p' src/core/config/ContextProxy.ts
# Print getGlobalState implementation for routing correctness
rg -n "getGlobalState\\(" src/core/config/ContextProxy.ts
sed -n '300,520p' src/core/config/ContextProxy.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 13372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "selfImprovingScope|selfImprovingAutoSkillsScope" packages/types/src/global-settings.ts -n
# Show definitions of SECRET_STATE_KEYS/GLOBAL_SECRET_KEYS and isSecretStateKey
rg -n "export const SECRET_STATE_KEYS|export const GLOBAL_SECRET_KEYS|function isSecretStateKey|isSecretStateKey\\s*=" packages/types/src/global-settings.ts -n -C3Repository: Zoo-Code-Org/Zoo-Code
Length of output: 956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show GLOBAL_SETTINGS_KEYS array contents around the selfImproving* keys (if present)
rg -n "export const GLOBAL_SETTINGS_KEYS" packages/types/src/global-settings.ts -n
# Print a bounded window around the array to locate selfImprovingScope entries
python3 - <<'PY'
import re, pathlib
p = pathlib.Path("packages/types/src/global-settings.ts")
text = p.read_text(encoding="utf-8")
m = re.search(r"export const GLOBAL_SETTINGS_KEYS\s*=\s*\[(.*?)\]\s*as const", text, flags=re.S)
if not m:
print("GLOBAL_SETTINGS_KEYS array not found")
raise SystemExit(0)
arr = m.group(1)
for key in ["selfImprovingScope", "selfImprovingAutoSkillsScope"]:
idx = arr.find(key)
print(f"{key} present: {idx!=-1}")
# Print small slices around occurrences
for key in ["selfImprovingScope", "selfImprovingAutoSkillsScope"]:
idx = arr.find(key)
if idx!=-1:
start = max(0, idx-80)
end = min(len(arr), idx+120)
print("\n--- context for", key, "---\n")
print(arr[start:end])
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact test setup and assertions around the snippet
sed -n '230,310p' src/core/config/__tests__/ContextProxy.spec.ts | nl -baRepository: Zoo-Code-Org/Zoo-Code
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact test setup and assertions around the snippet with line numbers
sed -n '230,310p' src/core/config/__tests__/ContextProxy.spec.ts | cat -nRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3665
Improve type-safety and assert routing for self-improving scope keys (ContextProxy.spec.ts lines 269-277)
selfImprovingScope/selfImprovingAutoSkillsScopeare already modeled in types (z.enum(["workspace","global"]), and included inglobalSettingsSchema), andContextProxy.setValueroutes them to global state (they are not part ofSECRET_STATE_KEYS/GLOBAL_SECRET_KEYS).- The test currently only roundtrips via
getGlobalSettings()and keepsas any; strengthen it by assertingupdateGlobalStateis called with the right arguments (like the existing secret vs global routing tests) and drop/limit theas anycasts.
🤖 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/config/__tests__/ContextProxy.spec.ts` around lines 269 - 277, The
test should assert that ContextProxy.setValue routes the enum-typed keys to the
global state updater instead of merely roundtripping via getGlobalSettings;
modify the test to call proxy.setValue("selfImprovingScope", "workspace") and
proxy.setValue("selfImprovingAutoSkillsScope", "global") (without sprawling `as
any` casts), and verify that the mock/stub for updateGlobalState was called with
the expected key/value pairs (e.g., updateGlobalState("selfImprovingScope",
"workspace") and updateGlobalState("selfImprovingAutoSkillsScope", "global")),
while keeping the existing checks of getGlobalSettings minimal or removing
them—this ensures setValue uses the global routing logic (not
SECRET_STATE_KEYS/GLOBAL_SECRET_KEYS) and preserves type-safety against the
z.enum-backed globalSettingsSchema.
Comment on lines
+1472
to
+1483
| if (shouldRecordCorrection) { | ||
| await provider | ||
| .getSelfImprovingManager?.() | ||
| ?.recordUserCorrection({ | ||
| taskId: this.taskId, | ||
| success: false, | ||
| corrected: true, | ||
| }) | ||
| .catch((error: unknown) => { | ||
| console.error("[Task#submitUserMessage] Failed to record user correction:", error) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid blocking user message submission on correction telemetry.
Line 1472 waits on recordUserCorrection(...) before handleWebviewAskResponse(...). If memory/self-improving backends are slow, user responses can lag or appear stuck.
Suggested change
if (shouldRecordCorrection) {
- await provider
- .getSelfImprovingManager?.()
- ?.recordUserCorrection({
- taskId: this.taskId,
- success: false,
- corrected: true,
- })
- .catch((error: unknown) => {
- console.error("[Task#submitUserMessage] Failed to record user correction:", error)
- })
+ try {
+ const selfImprovingManager = provider.getSelfImprovingManager?.()
+ if (selfImprovingManager) {
+ void selfImprovingManager
+ .recordUserCorrection({
+ taskId: this.taskId,
+ success: false,
+ corrected: true,
+ })
+ .catch((error: unknown) => {
+ console.error("[Task#submitUserMessage] Failed to record user correction:", error)
+ })
+ }
+ } catch (error) {
+ console.error("[Task#submitUserMessage] Failed to start user correction recording:", 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/core/task/Task.ts` around lines 1472 - 1483, The code currently awaits
provider.getSelfImprovingManager?.().recordUserCorrection(...) (inside the
shouldRecordCorrection branch) which blocks submitUserMessage flow and delays
handleWebviewAskResponse; change this to fire-and-forget: call
provider.getSelfImprovingManager?.().recordUserCorrection({...}) without
awaiting (e.g., prefix with void or chain .catch) so it runs asynchronously,
still capturing/logging errors (use .catch((e)=>console.error(...))). Keep the
same payload (taskId: this.taskId, success: false, corrected: true) and leave
the shouldRecordCorrection check in place.
Comment on lines
+18
to
+68
| describe("CodebaseSearchTool", () => { | ||
| it("records self-improving code index hit details from search results", async () => { | ||
| const recordCodeIndexEvent = vi.fn().mockResolvedValue(undefined) | ||
| const getSelfImprovingManager = vi.fn().mockReturnValue({ recordCodeIndexEvent }) | ||
| const searchIndex = vi.fn().mockResolvedValue([ | ||
| { | ||
| payload: { | ||
| filePath: "/workspace/src/example.ts", | ||
| startLine: 10, | ||
| endLine: 20, | ||
| codeChunk: "const answer = 42", | ||
| }, | ||
| score: 0.87, | ||
| }, | ||
| ]) | ||
| vi.mocked(CodeIndexManager.getInstance).mockReturnValue({ | ||
| isFeatureEnabled: true, | ||
| isFeatureConfigured: true, | ||
| searchIndex, | ||
| } as any) | ||
|
|
||
| const task = { | ||
| cwd: "/workspace", | ||
| taskId: "task-1", | ||
| consecutiveMistakeCount: 0, | ||
| providerRef: { | ||
| deref: vi.fn().mockReturnValue({ | ||
| context: {}, | ||
| getSelfImprovingManager, | ||
| }), | ||
| }, | ||
| say: vi.fn().mockResolvedValue(undefined), | ||
| } as any | ||
| const callbacks = { | ||
| askApproval: vi.fn().mockResolvedValue(true), | ||
| handleError: vi.fn().mockResolvedValue(undefined), | ||
| pushToolResult: vi.fn(), | ||
| } | ||
|
|
||
| const tool = new CodebaseSearchTool() | ||
| await tool.execute({ query: "find the answer" }, task, callbacks) | ||
|
|
||
| expect(recordCodeIndexEvent).toHaveBeenCalledWith("task-1", { | ||
| available: true, | ||
| hits: 1, | ||
| topScore: 0.87, | ||
| }) | ||
| expect(callbacks.handleError).not.toHaveBeenCalled() | ||
| expect(vscode.workspace.asRelativePath).toHaveBeenCalledWith("/workspace/src/example.ts", false) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Expand test coverage beyond the happy path.
This test file only covers the happy path where the code index feature is enabled, configured, and returns results. Critical gaps include:
Missing error scenarios:
- CodeIndexManager throws an error during search
getSelfImprovingManager()returns null/undefinedrecordCodeIndexEventrejects
Missing edge cases:
- Empty search results (
searchIndexreturns[]) - Feature disabled (
isFeatureEnabled: false) - Feature not configured (
isFeatureConfigured: false) - Search results without score field
- Multiple search results (verify all are processed)
Missing core functionality tests:
- Search behavior without self-improving manager present
- Query formatting and passing to searchIndex
- Result formatting for the tool output
As per coding guidelines, package-local unit tests should cover error handling and edge cases comprehensively.
🤖 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/tools/__tests__/CodebaseSearchTool.spec.ts` around lines 18 - 68,
The test only covers the happy path for CodebaseSearchTool.execute; add unit
tests to cover error and edge cases: simulate
CodeIndexManager.getInstance().searchIndex throwing an error and assert
callbacks.handleError is called; mock getSelfImprovingManager() returning
null/undefined and assert no recordCodeIndexEvent calls and tool still returns
gracefully; make recordCodeIndexEvent reject and assert handleError/logging
behavior; add tests for empty searchIndex results, isFeatureEnabled=false,
isFeatureConfigured=false, results missing score, and multiple search results to
verify hits/topScore aggregation and that vscode.workspace.asRelativePath is
invoked; use the existing symbols CodebaseSearchTool,
CodeIndexManager.getInstance, searchIndex, getSelfImprovingManager,
recordCodeIndexEvent and execute to locate where to add/modify specs.
Comment on lines
+1944
to
+1946
| private getGlobalStateSafe<T = unknown>(key: string): T | undefined { | ||
| return (this.contextProxy as any).getGlobalState?.(key) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ContextProxy API surface
ast-grep --pattern 'class ContextProxy {
$$$
}'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 41210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/core/webview/ClineProvider.ts"
# Show the helper implementation area
sed -n '1910,1985p' "$FILE"
# Find where contextProxy is declared/typed in ClineProvider
rg -n "contextProxy" "$FILE"
# Show the definition/declaration line(s) for contextProxy (near first match)
# (Use line numbers from rg output to grab a small window around it)
first_line=$(rg -n "contextProxy" "$FILE" | head -n1 | cut -d: -f1)
start=$((first_line-20))
end=$((first_line+40))
sed -n "${start},${end}p" "$FILE"
# Find all usages of getGlobalStateSafe
rg -n "getGlobalStateSafe" "$FILE"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 8597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate GlobalStateKey / key lists in ContextProxy.ts
FILE="src/core/config/ContextProxy.ts"
rg -n "type GlobalStateKey|enum GlobalStateKey|interface GlobalStateKey|GLOBAL_STATE_KEYS" "$FILE"
rg -n "\"experiments\"|experiments" "$FILE"
# Show the relevant region where GlobalStateKey and GLOBAL_STATE_KEYS are defined
# (grab around first match of GLOBAL_STATE_KEYS)
first=$(rg -n "GLOBAL_STATE_KEYS" "$FILE" | head -n1 | cut -d: -f1)
start=$((first-40))
end=$((first+120))
sed -n "${start},${end}p" "$FILE"
# Find signature/implementation of getGlobalState for context
rg -n "getGlobalState<" "$FILE"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm whether "experiments" is part of GlobalState / keys
rg -n "\"experiments\"" src/core/config || true
rg -n "experiments" src/core/config || true
# Find where GlobalState is defined (type GlobalStateKey = keyof GlobalState suggests GlobalState comes from a type definition import)
# We'll locate the import(s) and the GlobalState type.
FILE="src/core/config/ContextProxy.ts"
rg -n "import .*GlobalState|type GlobalState|interface GlobalState|export type GlobalState" "$FILE" || true
# Show the top of ContextProxy.ts where GlobalState is imported/defined
sed -n '1,120p' "$FILE"
# Find definition of GlobalState in the codebase
rg -n "type GlobalState\\s*=|interface GlobalState|export type GlobalState" src/core/config src | head -n 50Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where "experiments" is defined/used across the repo
rg -n "\"experiments\"|experiments" .
# 2) Inspect SelfImprovingManager.onSettingsChanged signature and expected type
rg -n "class SelfImprovingManager|onSettingsChanged\\(" src/core || true
# 3) Show the getGlobalStateSafe call sites + their string keys
FILE="src/core/webview/ClineProvider.ts"
rg -n "getGlobalStateSafe\\(" "$FILE"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 22333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find GlobalState type in `@roo-code/types`
rg -n "export type GlobalState|type GlobalState" packages/types src packages/types | head -n 50
# 2) Check whether "experiments" exists as a key on GlobalState
rg -n "\"experiments\"|experiments:" packages/types src/packages/types | head -n 50
# 3) Show ContextProxy imports/types usage in ClineProvider.ts
FILE="src/core/webview/ClineProvider.ts"
rg -n "from \"\\.\\./core/config/ContextProxy\"|from \".*/ContextProxy\"|import .*ContextProxy" "$FILE" || true
sed -n '1,120p' "$FILE"
# 4) Show any other helper wrappers around contextProxy getters in ClineProvider
rg -n "getGlobalStateSafe|getValue\\(|getGlobalState\\(" "$FILE" -nRepository: Zoo-Code-Org/Zoo-Code
Length of output: 817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect GlobalState definition
FILE="packages/types/src/global-settings.ts"
rg -n "export type GlobalState|experiments:" "$FILE"
sed -n '300,360p' "$FILE"
sed -n '160,230p' "$FILE"
# Inspect ContextProxy.getGlobalState signature (already seen, but grab exact lines)
CTX="src/core/config/ContextProxy.ts"
rg -n "getGlobalState<|getGlobalState\\(" "$CTX" | head -n 20
sed -n '330,380p' "$CTX"
# Inspect ClineProvider helper and its surrounding context (imports/types nearby already partly known)
CLINE="src/core/webview/ClineProvider.ts"
sed -n '1,60p' "$CLINE"
sed -n '1925,1970p' "$CLINE"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 8738
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/types/src/global-settings.ts"
for k in experiments memoryBackend agentMemoryUrl selfImprovingScope selfImprovingAutoSkillsScope; do
echo "---- $k"
rg -n "\"$k\"|\\b$k\\b" "$FILE" || true
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 471
getGlobalStateSafe uses unnecessary any/optional chaining (type safety loss).
ContextProxy already provides getGlobalState, so the (this.contextProxy as any) cast and ?. bypass the typed GlobalState key/value APIs. Type the key as keyof GlobalState and call this.contextProxy.getGlobalState(key) directly.
Suggested change
- private getGlobalStateSafe<T = unknown>(key: string): T | undefined {
- return (this.contextProxy as any).getGlobalState?.(key)
- }
+ private getGlobalStateSafe<K extends keyof GlobalState>(key: K): GlobalState[K] | undefined {
+ return this.contextProxy.getGlobalState(key)
+ }🤖 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 1944 - 1946,
getGlobalStateSafe currently casts this.contextProxy to any and uses optional
chaining, losing type safety; change its signature to accept key: keyof
GlobalState (or a generic K extends keyof GlobalState) and call
this.contextProxy.getGlobalState(key) directly (remove the any cast and ?.
usage) so the returned type is tied to GlobalState (e.g., GlobalState[K] or
generic T constrained to GlobalState[keyof GlobalState]); update the function
name getGlobalStateSafe and any callers to satisfy the stronger typed key and
return type.
| this.getSkillProvenance = options.getSkillProvenance ?? (() => "unknown") | ||
| this.getSkillProvenanceForSource = | ||
| options.getSkillProvenanceForSource ?? ((name: string) => this.getSkillProvenance(name)) | ||
| this.hasSkill = options.hasSkill ?? ((name: string) => this.getSkillNames().includes(name)) |
Contributor
There was a problem hiding this comment.
Default hasSkill implementation ignores the source parameter.
The default fallback (name: string) => this.getSkillNames().includes(name) ignores the source parameter, but callers at line 186 pass both skillName and source. This could cause incorrect skill existence checks when distinguishing global vs project skills.
- this.hasSkill = options.hasSkill ?? ((name: string) => this.getSkillNames().includes(name))
+ this.hasSkill = options.hasSkill ?? ((_name: string, _source: "global" | "project") => false)A safer default is to return false (skill doesn't exist), which prevents accidental updates to non-existent skills. The actual implementation should be provided when auto-skills are enabled.
📝 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
| this.hasSkill = options.hasSkill ?? ((name: string) => this.getSkillNames().includes(name)) | |
| this.hasSkill = options.hasSkill ?? ((_name: string, _source: "global" | "project") => false) |
🤖 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/ImprovementApplier.ts` at line 40, The default
hasSkill fallback in ImprovementApplier currently ignores the second parameter;
update the default assignment for this.hasSkill (which now uses
options.hasSkill) to accept both (name, source) and return false by default to
avoid accidental matches (i.e., use (name: string, source?: string) => false),
ensuring callers that pass (skillName, source) receive the safer behavior; keep
the real implementation supplied via options.hasSkill when auto-skills are
enabled and leave getSkillNames untouched.
Comment on lines
+467
to
471
| const skill = mode ? this.getSkill(name, source, mode) : this.findSkillByNameAndSource(name, source) | ||
| if (!skill) { | ||
| const modeInfo = mode ? ` (mode: ${mode})` : "" | ||
| throw new Error(t("skills:errors.not_found", { name, source, modeInfo })) | ||
| } |
Contributor
There was a problem hiding this comment.
Mode-scoped updates can fail for valid multi-mode skills.
Line 467 uses getSkill(name, source, mode), but keys are indexed by only the primary mode slug. A skill with modeSlugs: ["code", "architect"] won’t be found when updating with mode = "architect".
Suggested lookup fix
-const skill = mode ? this.getSkill(name, source, mode) : this.findSkillByNameAndSource(name, source)
+const skill = mode
+ ? Array.from(this.skills.values()).find(
+ (s) =>
+ s.name === name &&
+ s.source === source &&
+ (!s.modeSlugs || s.modeSlugs.length === 0 || s.modeSlugs.includes(mode)),
+ )
+ : this.findSkillByNameAndSource(name, source)📝 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
| const skill = mode ? this.getSkill(name, source, mode) : this.findSkillByNameAndSource(name, source) | |
| if (!skill) { | |
| const modeInfo = mode ? ` (mode: ${mode})` : "" | |
| throw new Error(t("skills:errors.not_found", { name, source, modeInfo })) | |
| } | |
| const skill = mode | |
| ? Array.from(this.skills.values()).find( | |
| (s) => | |
| s.name === name && | |
| s.source === source && | |
| (!s.modeSlugs || s.modeSlugs.length === 0 || s.modeSlugs.includes(mode)), | |
| ) | |
| : this.findSkillByNameAndSource(name, source) | |
| if (!skill) { | |
| const modeInfo = mode ? ` (mode: ${mode})` : "" | |
| throw new Error(t("skills:errors.not_found", { name, source, modeInfo })) | |
| } |
🤖 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/skills/SkillsManager.ts` around lines 467 - 471, The current
lookup uses getSkill(name, source, mode) which fails for skills whose keys are
stored under the primary mode slug while the update passes a secondary mode
(e.g., modeSlugs: ["code","architect"]); change the lookup to fall back to
finding by name/source and verifying the provided mode against the skill's
modeSlugs: call this.getSkill(...) first, and if it returns undefined and mode
is provided, call this.findSkillByNameAndSource(name, source) and ensure the
returned skill exists and (if mode) skill.modeSlugs includes the requested mode
before proceeding (otherwise throw the same not_found error); update the logic
around getSkill, findSkillByNameAndSource, and modeSlugs accordingly.
Comment on lines
+515
to
+587
| it("shows the auto-skills sub-option under self-improving and saves it", () => { | ||
| const { activateTab, getSettingsContent } = renderSettingsView() | ||
|
|
||
| activateTab("experimental") | ||
|
|
||
| const content = getSettingsContent() | ||
| const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox") | ||
| fireEvent.click(selfImprovingCheckbox) | ||
| expect(selfImprovingCheckbox).toBeChecked() | ||
|
|
||
| const autoSkillsCheckbox = within(content).getByTestId("experimental-self-improving-auto-skills-checkbox") | ||
| fireEvent.click(autoSkillsCheckbox) | ||
| expect(autoSkillsCheckbox).toBeChecked() | ||
|
|
||
| const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select") | ||
| fireEvent.change(selfImprovingScopeSelect, { target: { value: "workspace" } }) | ||
|
|
||
| const autoSkillsScopeSelect = within(content).getByTestId("self-improving-auto-skills-scope-select") | ||
| fireEvent.change(autoSkillsScopeSelect, { target: { value: "global" } }) | ||
|
|
||
| const saveButton = screen.getByTestId("save-button") | ||
| fireEvent.click(saveButton) | ||
|
|
||
| expect(vscode.postMessage).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: "updateSettings", | ||
| updatedSettings: expect.objectContaining({ | ||
| selfImprovingScope: "workspace", | ||
| selfImprovingAutoSkillsScope: "global", | ||
| experiments: expect.objectContaining({ | ||
| selfImproving: true, | ||
| selfImprovingAutoSkills: true, | ||
| }), | ||
| }), | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it("shows memory backend controls for self-improving and saves agentmemory settings", () => { | ||
| const { activateTab, getSettingsContent } = renderSettingsView() | ||
|
|
||
| activateTab("experimental") | ||
|
|
||
| const content = getSettingsContent() | ||
| const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox") | ||
| fireEvent.click(selfImprovingCheckbox) | ||
| expect(selfImprovingCheckbox).toBeChecked() | ||
|
|
||
| const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select") | ||
| fireEvent.change(selfImprovingScopeSelect, { target: { value: "global" } }) | ||
|
|
||
| const backendSelect = within(content).getByTestId("self-improving-memory-backend-select") | ||
| fireEvent.change(backendSelect, { target: { value: "agentmemory" } }) | ||
|
|
||
| const urlInput = within(content).getByTestId("self-improving-agent-memory-url-input") | ||
| fireEvent.change(urlInput, { target: { value: "http://agentmemory.internal:4001" } }) | ||
| expect(urlInput).toHaveValue("http://agentmemory.internal:4001") | ||
|
|
||
| const saveButton = screen.getByTestId("save-button") | ||
| fireEvent.click(saveButton) | ||
|
|
||
| expect(vscode.postMessage).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: "updateSettings", | ||
| updatedSettings: expect.objectContaining({ | ||
| selfImprovingScope: "global", | ||
| memoryBackend: "agentmemory", | ||
| agentMemoryUrl: "http://agentmemory.internal:4001", | ||
| }), | ||
| }), | ||
| ) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert no postMessage before Save for the new experimental controls.
These assertions only verify the final payload after Save; they would still pass if changes leaked to live state immediately (the exact race the cached-state pattern is meant to prevent). Add an explicit pre-save expect(vscode.postMessage).not.toHaveBeenCalled() check after user edits (and ideally after hydration before edits).
Suggested test hardening
it("shows the auto-skills sub-option under self-improving and saves it", () => {
const { activateTab, getSettingsContent } = renderSettingsView()
activateTab("experimental")
const content = getSettingsContent()
const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox")
fireEvent.click(selfImprovingCheckbox)
const autoSkillsCheckbox = within(content).getByTestId("experimental-self-improving-auto-skills-checkbox")
fireEvent.click(autoSkillsCheckbox)
const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select")
fireEvent.change(selfImprovingScopeSelect, { target: { value: "workspace" } })
const autoSkillsScopeSelect = within(content).getByTestId("self-improving-auto-skills-scope-select")
fireEvent.change(autoSkillsScopeSelect, { target: { value: "global" } })
+ expect(vscode.postMessage).not.toHaveBeenCalled()
+
const saveButton = screen.getByTestId("save-button")
fireEvent.click(saveButton)
expect(vscode.postMessage).toHaveBeenCalledWith(As per coding guidelines, webview-ui/src/**/SettingsView.{ts,tsx}: “For SettingsView, preserve the cached-state pattern: inputs should operate on local cachedState until the user saves, and tests should distinguish automatic initialization from real user edits.”
📝 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
| it("shows the auto-skills sub-option under self-improving and saves it", () => { | |
| const { activateTab, getSettingsContent } = renderSettingsView() | |
| activateTab("experimental") | |
| const content = getSettingsContent() | |
| const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox") | |
| fireEvent.click(selfImprovingCheckbox) | |
| expect(selfImprovingCheckbox).toBeChecked() | |
| const autoSkillsCheckbox = within(content).getByTestId("experimental-self-improving-auto-skills-checkbox") | |
| fireEvent.click(autoSkillsCheckbox) | |
| expect(autoSkillsCheckbox).toBeChecked() | |
| const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select") | |
| fireEvent.change(selfImprovingScopeSelect, { target: { value: "workspace" } }) | |
| const autoSkillsScopeSelect = within(content).getByTestId("self-improving-auto-skills-scope-select") | |
| fireEvent.change(autoSkillsScopeSelect, { target: { value: "global" } }) | |
| const saveButton = screen.getByTestId("save-button") | |
| fireEvent.click(saveButton) | |
| expect(vscode.postMessage).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| type: "updateSettings", | |
| updatedSettings: expect.objectContaining({ | |
| selfImprovingScope: "workspace", | |
| selfImprovingAutoSkillsScope: "global", | |
| experiments: expect.objectContaining({ | |
| selfImproving: true, | |
| selfImprovingAutoSkills: true, | |
| }), | |
| }), | |
| }), | |
| ) | |
| }) | |
| it("shows memory backend controls for self-improving and saves agentmemory settings", () => { | |
| const { activateTab, getSettingsContent } = renderSettingsView() | |
| activateTab("experimental") | |
| const content = getSettingsContent() | |
| const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox") | |
| fireEvent.click(selfImprovingCheckbox) | |
| expect(selfImprovingCheckbox).toBeChecked() | |
| const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select") | |
| fireEvent.change(selfImprovingScopeSelect, { target: { value: "global" } }) | |
| const backendSelect = within(content).getByTestId("self-improving-memory-backend-select") | |
| fireEvent.change(backendSelect, { target: { value: "agentmemory" } }) | |
| const urlInput = within(content).getByTestId("self-improving-agent-memory-url-input") | |
| fireEvent.change(urlInput, { target: { value: "http://agentmemory.internal:4001" } }) | |
| expect(urlInput).toHaveValue("http://agentmemory.internal:4001") | |
| const saveButton = screen.getByTestId("save-button") | |
| fireEvent.click(saveButton) | |
| expect(vscode.postMessage).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| type: "updateSettings", | |
| updatedSettings: expect.objectContaining({ | |
| selfImprovingScope: "global", | |
| memoryBackend: "agentmemory", | |
| agentMemoryUrl: "http://agentmemory.internal:4001", | |
| }), | |
| }), | |
| ) | |
| }) | |
| }) | |
| it("shows the auto-skills sub-option under self-improving and saves it", () => { | |
| const { activateTab, getSettingsContent } = renderSettingsView() | |
| activateTab("experimental") | |
| const content = getSettingsContent() | |
| const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox") | |
| fireEvent.click(selfImprovingCheckbox) | |
| const autoSkillsCheckbox = within(content).getByTestId("experimental-self-improving-auto-skills-checkbox") | |
| fireEvent.click(autoSkillsCheckbox) | |
| const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select") | |
| fireEvent.change(selfImprovingScopeSelect, { target: { value: "workspace" } }) | |
| const autoSkillsScopeSelect = within(content).getByTestId("self-improving-auto-skills-scope-select") | |
| fireEvent.change(autoSkillsScopeSelect, { target: { value: "global" } }) | |
| expect(vscode.postMessage).not.toHaveBeenCalled() | |
| const saveButton = screen.getByTestId("save-button") | |
| fireEvent.click(saveButton) | |
| expect(vscode.postMessage).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| type: "updateSettings", | |
| updatedSettings: expect.objectContaining({ | |
| selfImprovingScope: "workspace", | |
| selfImprovingAutoSkillsScope: "global", | |
| experiments: expect.objectContaining({ | |
| selfImproving: true, | |
| selfImprovingAutoSkills: true, | |
| }), | |
| }), | |
| }), | |
| ) | |
| }) | |
| it("shows memory backend controls for self-improving and saves agentmemory settings", () => { | |
| const { activateTab, getSettingsContent } = renderSettingsView() | |
| activateTab("experimental") | |
| const content = getSettingsContent() | |
| const selfImprovingCheckbox = within(content).getByTestId("experimental-self-improving-checkbox") | |
| fireEvent.click(selfImprovingCheckbox) | |
| const selfImprovingScopeSelect = within(content).getByTestId("self-improving-scope-select") | |
| fireEvent.change(selfImprovingScopeSelect, { target: { value: "global" } }) | |
| const backendSelect = within(content).getByTestId("self-improving-memory-backend-select") | |
| fireEvent.change(backendSelect, { target: { value: "agentmemory" } }) | |
| const urlInput = within(content).getByTestId("self-improving-agent-memory-url-input") | |
| fireEvent.change(urlInput, { target: { value: "http://agentmemory.internal:4001" } }) | |
| expect(vscode.postMessage).not.toHaveBeenCalled() | |
| const saveButton = screen.getByTestId("save-button") | |
| fireEvent.click(saveButton) | |
| expect(vscode.postMessage).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| type: "updateSettings", | |
| updatedSettings: expect.objectContaining({ | |
| selfImprovingScope: "global", | |
| memoryBackend: "agentmemory", | |
| agentMemoryUrl: "http://agentmemory.internal:4001", | |
| }), | |
| }), | |
| ) | |
| }) |
🤖 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 `@webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx` around
lines 515 - 587, The tests for SettingsView (in the two "shows ..." it blocks)
currently only assert vscode.postMessage after clicking the save button, which
misses regressions where inputs leak immediately to live state; update both
tests (the one exercising auto-skills controls and the one exercising memory
backend/agentmemory) to assert vscode.postMessage was not called before any user
edits (after getSettingsContent/hydration) and again after performing user
interactions but before clicking the save button (i.e. before invoking
saveButton click) to ensure the cached-state pattern in SettingsView
(renderSettingsView, activateTab, getSettingsContent,
experimental-self-improving-checkbox, self-improving-* inputs) is preserved.
Comment on lines
+160
to
+162
| <SelectItem value="workspace">Workspace</SelectItem> | ||
| <SelectItem value="global">Global</SelectItem> | ||
| </SelectContent> |
Contributor
There was a problem hiding this comment.
Localize newly added select option labels instead of hardcoded English.
Workspace, Global, and Built-in are user-facing strings in a translated settings UI; hardcoding them creates l10n drift.
Also applies to: 201-203, 229-230
🤖 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 `@webview-ui/src/components/settings/ExperimentalSettings.tsx` around lines 160
- 162, Hardcoded user-facing labels "Workspace", "Global", and "Built-in" in the
SelectItem components of ExperimentalSettings.tsx should be replaced with calls
to the project's localization helper (the same translation function used
elsewhere in this file, e.g. the `t`/`formatMessage` from the existing i18n
hook) so the UI strings are localizable; update the SelectItem children for
values "workspace", "global", and "builtin" (and the other occurrences around
the file) to use translation keys like `settings.scope.workspace`,
`settings.scope.global`, `settings.scope.builtin`, and add those keys to the
translation resource files.
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.
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.PatternAnalyzerto detect recurring patterns in task outcomes and workflow signals.FeedbackCollectorto capture user corrections and feedback during task interactions.ImprovementApplierto apply learned improvements derived from analyzed patterns and feedback.CodeIndexAdapterto integrate code index search hits into the self-improving pipeline, enriching learning signals with codebase context.ActionExecutorto execute improvement actions, including skill mutations (auto-create/update skills based on learned patterns).selfImprovingexperiment flag and surfaces status through extension state/settings so it remains opt-in and low-risk.ARCHITECTURE.mdandGAP_ANALYSIS.md) in the service directory to guide future development and review.AgentMemoryAdapteras an optional memory backend that connects to the agentmemory REST API (https://github.com/rohitg00/agentmemory), providing 124+ memory endpoints and 53 MCP tools as an alternative to the built-inMemoryStore.MemoryBackendinterface andMemoryBackendFactoryso the system can transparently switch between built-in file-based memory and agentmemory's REST API backend via thememoryBackendconfiguration option.Design choices / trade-offs reviewers should note:
MemoryBackendinterface — users can choose between the built-in file-basedMemoryStore(default, zero external dependencies) or theagentmemoryREST API backend for advanced semantic search, consolidation, and cross-session memory features.Video/Screenshot
self-improving-zoo-code.mp4
Commit History
The branch contains 22 commits spanning feature implementation, hardening, and test coverage:
a40bdc15— feat: implement Self-Improving Manager for adaptive learning98057936— feat: Enhance SelfImprovingManager with MemoryStore and SkillUsageStore integrationc6d0b55c— feat: Implement ReviewPromptFactory and TranscriptRecall for self-improvement445e9ede— feat: Enhance self-improvement system with user message tracking and settings managementf15a9a6b— feat: Implement memory backend system with AgentMemoryAdapter and MemoryBackendFactoryfb39c36c— fix: harden self-improving memory deletion and recall9f2688ff— fix: tighten self-improving pattern merges53490f1b— test: tighten self-improving regression coveragef1210f7e— fix: harden transcript recall loading327a7d34— refactor: align self-improving shared typesf90b8845— fix: refine self-improving memory search and scoringe38aad81— fix: serialize transcript recall lazy initialization7860dc10— fix: preserve substring text in agentmemory forget search4f729743— fix: capture interactive user corrections for self-improving29af44c3— fix: feed code index search hits into self-improvingf6581653— test: verify extension startup initializes self-improvingf672d566— feat: add skill mutation APIs and auto-skill toggle732be8ff— feat: wire self-improving auto skill actions02ed3aed— feat: expose auto-skill toggle in experimental settingsf6d34080— test: cover auto-skill experiment gating7cf030e5— Add configurable self-improving memory backend settings99ce4f29— [verified] Add workspace/global self-improving scope controlsIntegration Points
Files modified outside the service folder to wire the self-improving subsystem into the extension:
src/extension.ts— initializes theSelfImprovingManageron extension startupsrc/core/prompts/system.ts— injects learned guidance context into system prompts when the experiment is enabledsrc/core/webview/ClineProvider.ts— exposes self-improving status, settings, and state through the providersrc/core/webview/webviewMessageHandler.ts— refreshes runtime behavior on settings updates (toggle, scope, auto-skill, memory backend)packages/types/src/vscode-extension-host.ts— adds self-improving state/status typing to the extension host typeswebview-ui/src/components/settings/ExperimentalSettings.tsx— adds experiment checkbox, scope selector, auto-skill toggle, memory-backend selector, and agentmemory URL controlswebview-ui/src/components/settings/SettingsView.tsx— wires self-improving settings UI into the cached state patternComplete List of New Service Files
All under
src/services/self-improving/:SelfImprovingManager.tsLearningStore.tsMemoryStore.tsMemoryBackend.tsMemoryBackendFactory.tsAgentMemoryAdapter.tsSkillUsageStore.tsTranscriptRecall.tsCuratorService.tsReviewPromptFactory.tsActionExecutor.tsImprovementApplier.tsPatternAnalyzer.tsFeedbackCollector.tsCodeIndexAdapter.tstypes.tsARCHITECTURE.mdGAP_ANALYSIS.md__tests__/Test Procedure
selfImprovingexperiment in Zoo Code's experimental settings.SelfImprovingManagerLearningStoreMemoryStoreSkillUsageStoreCuratorServiceTranscriptRecallActionExecutorReviewPromptFactoryAgentMemoryAdapterMemoryBackendFactoryPatternAnalyzerFeedbackCollectorImprovementApplierCodeIndexAdapternpx agentmemory) and configure thememoryBackendoption to"agentmemory"to verify the REST API adapter connects and stores/recalls memory correctly.Environment:
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.memoryBackendoption. When set to"agentmemory", the system connects to a running agentmemory server athttp://localhost:4001(configurable). The adapter uses REST API calls (not npm imports) so no additional dependencies are required.Get in Touch
Discord: @iskandarsulaili
Summary by CodeRabbit
Release Notes
New Features
Documentation