Repository navigation
Add package retriever hooks - #280
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds package-owned retrievers end-to-end: manifest/schema, KV-backed per-scope manifest cache, retriever execution service, integration into registration/refresh, search/memory/agent tool flows, result validation/limits (≤20), tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as "Search / Memory Tool"
participant Service as "Package Retriever Service"
participant Cache as "KV Manifest Cache"
participant Registry as "Package Registry"
participant Loader as "Bundle Loader"
participant Executor as "Module Executor"
Tool->>Service: runPackageRetrievers({ baseUrl, userId, scope, query, ... })
activate Service
Service->>Cache: listPackageRetrieversForScope({ userId, scope, limit })
Cache-->>Service: [retriever entries]
loop per retriever entry
Service->>Registry: fetch savedPackage & entity source
Registry-->>Service: savedPackage, source
Service->>Loader: load published bundle for entryPoint
Loader-->>Service: bundled module
Service->>Executor: execute retriever({ params: { query, scope, memoryContext, limit, conversationId } })
Executor-->>Service: { results: [...] } or error
Service->>Service: validate/normalize/truncate results (≤20)
end
Service->>Service: aggregate results, sort by score
Service-->>Tool: { results, warnings }
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
April 28, 2026 23:11
Contributor
|
🔎 Preview deployed: https://kody-pr-280.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/worker/src/mcp/tools/search.ts (1)
1673-1679:⚠️ Potential issue | 🟠 Major
maxResponseSizeno longer bounds responses once retriever memories are attached.Line 1678 adds
memories.retrieverResults, but Line 1765 only trimsmatches. If the memory-side retriever payload is large, the formatted response can still exceedmaxResponseSizeeven after all matches are dropped. The trim step needs to shed memory-side retriever data too.Also applies to: 1765-1768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 1673 - 1679, The response trimming only drops matches but leaves large retriever-side payloads attached via searchMemories (constructed from memoryToolContext.memories and specifically memories.retrieverResults), so maxResponseSize can still be exceeded; update the trimming logic in the code paths around where matches are truncated (the block that reduces matches when enforcing maxResponseSize) to also remove or truncate memory-side retriever data by clearing or replacing memoryToolContext.memories.retrieverResults (and any retriever payloads nested on searchMemories) when you drop matches — e.g., set retrieverResults = [] or strip heavy fields from each retrieverResult whenever you perform the matches trim so the formatted response is actually bounded by maxResponseSize.
🧹 Nitpick comments (2)
packages/worker/src/mcp/tools/memory-tool-context.node.test.ts (1)
44-83: Consider usingbeforeEachfor mock setup.The test manually calls
setupMemoryContextMocks()at the start. If more tests are added to this file, usingbeforeEachwould ensure consistent mock state and avoid accidental state leakage between tests.♻️ Suggested refactor
+import { beforeEach, expect, test, vi } from 'vitest' -import { expect, test, vi } from 'vitest' // ... mock setup ... +beforeEach(() => { + setupMemoryContextMocks() +}) + test('loadRelevantMemoriesForTool returns context retriever results alongside memories', async () => { - setupMemoryContextMocks() const result = await loadRelevantMemoriesForTool({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/memory-tool-context.node.test.ts` around lines 44 - 83, The test currently calls setupMemoryContextMocks() inline; move that call into a beforeEach block so mocks are reset before every test to prevent state leakage—add a beforeEach(() => setupMemoryContextMocks()) at the top of the test suite containing the test that calls loadRelevantMemoriesForTool, and remove the inline setupMemoryContextMocks() call from the test; ensure any other tests in the same file rely on the shared beforeEach and that mockModule.runPackageRetrievers expectations remain unchanged.packages/worker/src/package-retrievers/manifest-cache.ts (1)
199-207: Old revisioned manifest blobs are never removed from KV.Refresh writes a new manifest object under a revision-specific key, and delete only rewrites the scope indexes. That leaves orphaned manifest-cache entries behind indefinitely, so storage usage grows on every package refresh/delete cycle. Consider deleting superseded manifest keys during refresh and cleanup.
Also applies to: 254-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 199 - 207, The code writes revisioned manifest blobs via buildPackageRetrieverManifestCacheKey and getRetrieverKv.put but never removes superseded manifest keys (manifestKey), leaving orphaned KV entries; update the refresh and delete flows (the code around the manifest write and the sections referenced ~254-281) to enumerate previous revision keys from your scope/index, call getRetrieverKv().delete on each superseded buildPackageRetrieverManifestCacheKey(...) (or directly delete the old manifestKey) when replacing or deleting a package, and ensure deletion happens after successful write/removal so orphaned manifest-cache entries are cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/packages-and-manifests.md`:
- Around line 103-106: Update the Retriever return contract text to explicitly
state the runtime-enforced maximum of 20 results: mention that the returned
object must be { results: [...] } with at most 20 items (each item containing
id, title, summary, optional details/score/source/url/metadata), and note that
payloads with >20 items will be rejected at validation time; also add a short
recommendation to paginate or truncate results in the Retriever implementation
(referencing the "Retriever exports" and "results" contract) so package authors
avoid deployment-time validation failures.
In `@packages/worker/src/agent-turn/tools.ts`:
- Around line 85-98: The call to runPackageRetrievers (and its dynamic import)
can throw and currently can abort the entire search tool; wrap the dynamic
import and the runPackageRetrievers(...) call in a try/catch, logging a warning
(not an error) on failures and setting retrieverSearch to an empty/neutral
result (e.g., empty hits/[] and no-op metadata) so downstream code can continue;
ensure the shape of the fallback matches what the rest of the search tool
expects (reference runPackageRetrievers, retrieverSearch, and
resolveSearchMemoryContext) so types/consumers aren’t broken.
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 419-429: parseEntityRef currently allows the 'retriever_result'
type but SearchEntityDetail has no retriever_result variant and
resolveEntityDetail/formatEntityDetailMarkdown lack handlers, causing unresolved
references; either remove 'retriever_result' from the allowed types in
parseEntityRef to prevent parsing it as resolvable, or add a retriever_result
case to SearchEntityDetail and implement its handling in resolveEntityDetail and
formatEntityDetailMarkdown (implement resolver logic to fetch the retriever
result detail and a formatter to render it) so parsed retriever_result refs can
be resolved and formatted consistently with other entity types.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 1572-1590: The call to runPackageRetrievers (inside the async
branch that sets retrieverRun) can throw and currently bubbles up, so wrap the
await runPackageRetrievers(...) invocation in a try/catch; on error return a
fallback object like { results: [], warnings: [String(error) || 'retriever
error'] } so retrieverRun is always an object and the subsequent
warnings.push(...retrieverRun.warnings) and search flow continue; locate the
async IIFE that references runPackageRetrievers, resolveSearchMemoryContext,
userId and replace the direct await with a guarded try/catch returning the
empty-results object on failure.
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 212-249: The read-modify-write in readScopeIndex -> build
nextRetrievers -> writeScopeIndex is racy and can drop concurrent updates;
change to an atomic update or lower contention: either implement optimistic
compare-and-swap by reading a version/updatedAt from readScopeIndex
(existing.updatedAt or add a version field) and pass it to writeScopeIndex so
the write fails if the index changed, retrying the merge, or refactor the
storage shape to per-retriever records (instead of one list) so add/remove
operations are independent; apply the same fix for the other occurrence around
lines 264-280 that uses existing.retrievers, nextRetrievers and
retrieverScopeIndexVersion.
- Around line 296-324: The current code slices index.retrievers early (slice(0,
input.limit ?? 10)), which can drop valid retrievers if earlier entries are
stale; remove that early slice and iterate all index.retrievers, performing the
KV lookup via getRetrieverKv(...) and buildPackageRetrieverManifestCacheKey(...)
and validating cached objects against
retrieverManifestCacheVersion/PackageRetrieverManifestCache/userId/packageId/revision
as you already do, collect matching retrievers that include input.scope, then
after flattening manifests.flat() apply the limit once (e.g., result =
manifests.flat().slice(0, input.limit ?? 10)) before returning so the requested
limit is applied to valid, filtered retrievers rather than to raw index entries.
In `@packages/worker/src/package-retrievers/service.ts`:
- Around line 104-108: The cache validation should only accept entries whose
published_commit matches the requested revision: replace the current condition
that compares input.entry.revision against (source.published_commit ??
source.indexed_commit ?? source.updated_at) with a strict comparison to
source.published_commit alone (i.e., treat missing published_commit as a
mismatch). Locate the check in service.ts (the if block that currently uses the
nullish-fallback chain) and change it to only compare source.published_commit to
input.entry.revision; this aligns with normalizeRevision() in manifest-cache.ts
which only stores published_commit.
- Around line 253-281: The code currently exposes raw retriever exception text
(outcome.reason) in console logs and in the user-facing warning pushed to
warnings inside the package retriever flow; change the console.error payload and
the warning string pushed in the block that handles failure (referencing
outcome.reason, entry.packageId, entry.kodyId, entry.retrieverKey, input.scope,
and warnings) to use a fixed, non-sensitive message (e.g., "package retriever
failed" plus retriever identifiers) while keeping
Sentry.captureException(outcome.reason, {...}) unchanged so the full error is
recorded only in Sentry; ensure no raw error message or stack is concatenated
into logs or warnings.
In `@packages/worker/src/package-runtime/published-bundle-artifacts.ts`:
- Around line 356-383: The retriever loop rebuilds and re-persists module
artifacts already handled by the generic manifest.exports logic, causing
redundant builds and writes; modify the logic in the retriever iteration
(listPackageRetrievers and its use of retriever.exportName) to first deduplicate
export names (or consult the already-handled set from the manifest.exports pass)
and only call buildModuleBundle and persistPublishedBundleArtifact for
exportNames that were not already processed; alternatively extract the shared
"resolve export target + persist module artifact" steps into a helper used by
both flows and ensure you skip or no-op for exportNames present in the
manifest.exports pass to avoid repeated builds and DB/KV churn.
---
Outside diff comments:
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 1673-1679: The response trimming only drops matches but leaves
large retriever-side payloads attached via searchMemories (constructed from
memoryToolContext.memories and specifically memories.retrieverResults), so
maxResponseSize can still be exceeded; update the trimming logic in the code
paths around where matches are truncated (the block that reduces matches when
enforcing maxResponseSize) to also remove or truncate memory-side retriever data
by clearing or replacing memoryToolContext.memories.retrieverResults (and any
retriever payloads nested on searchMemories) when you drop matches — e.g., set
retrieverResults = [] or strip heavy fields from each retrieverResult whenever
you perform the matches trim so the formatted response is actually bounded by
maxResponseSize.
---
Nitpick comments:
In `@packages/worker/src/mcp/tools/memory-tool-context.node.test.ts`:
- Around line 44-83: The test currently calls setupMemoryContextMocks() inline;
move that call into a beforeEach block so mocks are reset before every test to
prevent state leakage—add a beforeEach(() => setupMemoryContextMocks()) at the
top of the test suite containing the test that calls
loadRelevantMemoriesForTool, and remove the inline setupMemoryContextMocks()
call from the test; ensure any other tests in the same file rely on the shared
beforeEach and that mockModule.runPackageRetrievers expectations remain
unchanged.
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 199-207: The code writes revisioned manifest blobs via
buildPackageRetrieverManifestCacheKey and getRetrieverKv.put but never removes
superseded manifest keys (manifestKey), leaving orphaned KV entries; update the
refresh and delete flows (the code around the manifest write and the sections
referenced ~254-281) to enumerate previous revision keys from your scope/index,
call getRetrieverKv().delete on each superseded
buildPackageRetrieverManifestCacheKey(...) (or directly delete the old
manifestKey) when replacing or deleting a package, and ensure deletion happens
after successful write/removal so orphaned manifest-cache entries are cleaned
up.
🪄 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: 2368bc48-165a-4769-9b33-3d01d653f743
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
docs/contributing/packages-and-manifests.mdpackage.jsonpackages/worker/src/agent-turn/tools.tspackages/worker/src/mcp/tools/memory-tool-context.node.test.tspackages/worker/src/mcp/tools/memory-tool-context.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-registry/manifest.node.test.tspackages/worker/src/package-registry/manifest.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-registry/types.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.tspackages/worker/src/package-retrievers/service.tspackages/worker/src/package-retrievers/types.tspackages/worker/src/package-runtime/published-bundle-artifacts.ts
Comment on lines
+103
to
+106
| Retriever exports receive `params` with `query`, `scope`, `memoryContext`, | ||
| `limit`, and `conversationId`, and return `{ "results": [...] }` where each | ||
| result has `id`, `title`, `summary`, optional `details`, optional `score`, | ||
| optional `source`, optional `url`, and optional `metadata`. |
There was a problem hiding this comment.
Document the enforced 20-result cap.
The return contract here says { results: [...] }, but the runtime schema rejects payloads with more than 20 items. Without that limit in the docs, package authors can implement retrievers that fail validation only after deployment.
Suggested docs tweak
-Retriever exports receive `params` with `query`, `scope`, `memoryContext`,
-`limit`, and `conversationId`, and return `{ "results": [...] }` where each
+Retriever exports receive `params` with `query`, `scope`, `memoryContext`,
+`limit`, and `conversationId`, and return `{ "results": [...] }` with at most
+20 results, where each
result has `id`, `title`, `summary`, optional `details`, optional `score`,
optional `source`, optional `url`, and optional `metadata`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing/packages-and-manifests.md` around lines 103 - 106, Update
the Retriever return contract text to explicitly state the runtime-enforced
maximum of 20 results: mention that the returned object must be { results: [...]
} with at most 20 items (each item containing id, title, summary, optional
details/score/source/url/metadata), and note that payloads with >20 items will
be rejected at validation time; also add a short recommendation to paginate or
truncate results in the Retriever implementation (referencing the "Retriever
exports" and "results" contract) so package authors avoid deployment-time
validation failures.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/tools/search-format.ts (1)
435-449:⚠️ Potential issue | 🟠 MajorAlign structured memory contract with retriever sidecar support.
formatSearchMarkdownnow acceptsmemories.retrieverResults(Line 448), butSearchResultStructuredContent.memoriesstill omits that field. This creates type drift and makes the new field easy to drop in typed call paths.💡 Suggested contract update
export type SearchResultStructuredContent = { ... memories?: { surfaced: Array<{ id: string category: string | null status: string subject: string summary: string details: string tags: Array<string> updatedAt: string }> suppressedCount: number retrievalQuery: string + retrieverResults?: Array<PackageRetrieverSurfaceResult> } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search-format.ts` around lines 435 - 449, The structured memory contract needs to include the same optional retrieverResults field that formatSearchMarkdown accepts: update the SearchResultStructuredContent type (the memories property) to add retrieverResults?: Array<PackageRetrieverSurfaceResult> (matching the shape used by formatSearchMarkdown) so typed call paths cannot drop the field; ensure imports/reference to PackageRetrieverSurfaceResult are added where SearchResultStructuredContent is declared and run TS checks to verify compatibility with formatSearchMarkdown and any callers that construct memories objects.
♻️ Duplicate comments (1)
packages/worker/src/package-retrievers/manifest-cache.ts (1)
329-342:⚠️ Potential issue | 🔴 CriticalAvoid concurrent delete/put against the same scope-entry keys.
At Line 329, deletes and writes are dispatched together. When a retriever key already exists, both operations target the same key; if
deletecompletes afterput, the refreshed entry is lost.🔧 Suggested fix (serialize delete then write)
- await Promise.all([ - ...existingScopeEntryKeys.map( - async (key) => await getRetrieverKv(input.env).delete(key), - ), - ...registeredRetrievers - .filter((retriever) => retriever.scopes.includes(scope)) - .map((retriever) => - writeScopeEntry({ - env: input.env, - scope, - entry: toScopeIndexEntry(retriever), - }), - ), - ]) + await Promise.all( + existingScopeEntryKeys.map( + async (key) => await getRetrieverKv(input.env).delete(key), + ), + ) + await Promise.all( + registeredRetrievers + .filter((retriever) => retriever.scopes.includes(scope)) + .map((retriever) => + writeScopeEntry({ + env: input.env, + scope, + entry: toScopeIndexEntry(retriever), + }), + ), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 329 - 342, Concurrent deletes and writes against the same scope-entry keys can race and lose updates; change the logic so you first delete all keys (use getRetrieverKv(input.env).delete for each key from existingScopeEntryKeys and await Promise.all on those deletes) and only after those deletions complete perform the writes for registeredRetrievers (calling writeScopeEntry for each toScopeIndexEntry(retriever) where retriever.scopes includes scope and await Promise.all on the writes); keep references to existingScopeEntryKeys, registeredRetrievers, getRetrieverKv, writeScopeEntry, toScopeIndexEntry, scope and input.env when implementing the two-step serialized flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 435-449: The structured memory contract needs to include the same
optional retrieverResults field that formatSearchMarkdown accepts: update the
SearchResultStructuredContent type (the memories property) to add
retrieverResults?: Array<PackageRetrieverSurfaceResult> (matching the shape used
by formatSearchMarkdown) so typed call paths cannot drop the field; ensure
imports/reference to PackageRetrieverSurfaceResult are added where
SearchResultStructuredContent is declared and run TS checks to verify
compatibility with formatSearchMarkdown and any callers that construct memories
objects.
---
Duplicate comments:
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 329-342: Concurrent deletes and writes against the same
scope-entry keys can race and lose updates; change the logic so you first delete
all keys (use getRetrieverKv(input.env).delete for each key from
existingScopeEntryKeys and await Promise.all on those deletes) and only after
those deletions complete perform the writes for registeredRetrievers (calling
writeScopeEntry for each toScopeIndexEntry(retriever) where retriever.scopes
includes scope and await Promise.all on the writes); keep references to
existingScopeEntryKeys, registeredRetrievers, getRetrieverKv, writeScopeEntry,
toScopeIndexEntry, scope and input.env when implementing the two-step serialized
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1eaf89d5-e81b-46f4-9129-d6b6436f6e9b
📒 Files selected for processing (7)
docs/contributing/packages-and-manifests.mdpackages/worker/src/agent-turn/tools.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.tspackages/worker/src/package-retrievers/service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/contributing/packages-and-manifests.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/mcp/tools/memory-tool-context.ts (1)
50-57: Preserve minimal error context in fail-open logging.The fail-open path is good, but dropping the exception details (Line 56) makes retriever incidents hard to diagnose.
Suggested tweak
} catch (error) { - console.error( + console.warn( JSON.stringify({ message: 'package context retrievers unavailable', + error: + error instanceof Error ? error.message : String(error), }), ) - void error return { results: [], warnings: [], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/memory-tool-context.ts` around lines 50 - 57, In the catch(error) block inside memory-tool-context.ts (the fail-open path for retrievers), change the console.error call so it preserves minimal error context: include error message and stack (e.g., add fields like error: String(error) or message: (error instanceof Error ? error.message : String(error)) and stack: (error instanceof Error ? error.stack : undefined)) into the JSON being logged, then return the same fail-open object as before; keep the existing void error removal of linter noise but ensure the logged JSON contains the error details for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 170-188: The current fallback in listScopeEntryKeys constructs
keys from readLegacyScopeIndex when kv.list is missing, which can miss new
writes because writeLegacyScopeIndex rebuilds the legacy index via
readScopeEntries (circular dependency); remove the typeof kv.list fallback
branch from listScopeEntryKeys so it always relies on the primary kv.list
behavior and instead centralize the legacy fallback inside readScopeEntries:
when kv.list is unavailable, have readScopeEntries call readLegacyScopeIndex and
return entries (and let buildPackageRetrieverScopeEntryKey still be used when
deriving keys there), ensuring writeLegacyScopeIndex continues to rebuild via
readScopeEntries without causing stale snapshots.
- Around line 243-250: The second filter currently assumes entry.scopes is an
array and calls entry.scopes.includes(input.scope), which can throw for
malformed KV rows; update the predicate to guard the property first (e.g. ensure
typeof entry === 'object' && entry !== null && Array.isArray(entry.scopes'))
and/or add a type guard that narrows PackageRetrieverIndexEntry to entries where
scopes is an array of strings, then only call
entry.scopes.includes(input.scope); reference the existing filter chain and the
symbols PackageRetrieverIndexEntry, entry.scopes, input.userId and input.scope
when making the change.
- Around line 365-392: removePackageRetrieverManifestCacheEntries currently only
clears scope index entries but leaves package-retriever-manifest:* rows in KV;
after deleting existingScopeEntryKeys (and before/after calling
writeLegacyScopeIndex), also enumerate and delete manifest records for the
package by using getRetrieverKv(input.env).list(...) with a prefix for the
package retriever manifests (e.g.
"package-retriever-manifest:{userId}:{packageId}:" or otherwise filter by
packageId) and then await Promise.all over the returned keys to call
.delete(key). Keep existing calls to listScopeEntryKeys, getRetrieverKv, and
writeLegacyScopeIndex but add this additional manifest-key listing+deletion step
inside removePackageRetrieverManifestCacheEntries so no orphaned
package-retriever-manifest entries remain.
---
Nitpick comments:
In `@packages/worker/src/mcp/tools/memory-tool-context.ts`:
- Around line 50-57: In the catch(error) block inside memory-tool-context.ts
(the fail-open path for retrievers), change the console.error call so it
preserves minimal error context: include error message and stack (e.g., add
fields like error: String(error) or message: (error instanceof Error ?
error.message : String(error)) and stack: (error instanceof Error ? error.stack
: undefined)) into the JSON being logged, then return the same fail-open object
as before; keep the existing void error removal of linter noise but ensure the
logged JSON contains the error details for diagnostics.
🪄 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: 9acd241f-96bb-4c1d-9645-8260b61dcc70
📒 Files selected for processing (5)
packages/worker/src/agent-turn/tools.tspackages/worker/src/mcp/tools/memory-tool-context.node.test.tspackages/worker/src/mcp/tools/memory-tool-context.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/worker/src/package-retrievers/manifest-cache.ts (2)
321-417:⚠️ Potential issue | 🟠 MajorRefresh and delete can still resurrect the same package.
These two flows snapshot current keys first and then mutate KV independently. If
removePackageRetrieverManifestCacheEntriesoverlaps a refresh for the same package, a late refresh can rewrite the manifest/scope entries after delete has already listed the old keys, making a deleted package discoverable again. This still needs package-level coordination or a tombstone/version check before writes.Also applies to: 420-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 321 - 417, Refresh flow can race with removePackageRetrieverManifestCacheEntries and re-create a deleted package; add a tombstone/version check to prevent resurrection by making delete write a tombstone and refresh abort if tombstone exists or version mismatches. Concretely: in removePackageRetrieverManifestCacheEntries create/update a package-level tombstone key (e.g., using buildPackageRetrieverManifestCacheKey or a new buildPackageRetrieverTombstoneKey) storing revision or timestamp; in refreshPackageRetrieverManifestCache, before putting manifestKey and before writing scope entries (writeScopeEntry / writeLegacyScopeIndex), read that tombstone and abort/return null if the tombstone indicates deletion for the same or newer revision/timestamp; ensure the same tombstone convention is used in both refreshPackageRetrieverManifestCache and removePackageRetrieverManifestCacheEntries so writes are conditional on tombstone absence or older version.
420-460:⚠️ Potential issue | 🟠 MajorNon-listing KV still leaks manifest-cache rows on delete.
manifestCacheKeysis sourced fromlistManifestCacheKeys, but that helper returns[]wheneverkv.listis unavailable (Line 211). In the fallback mode this file otherwise supports, package removal never deletes anypackage-retriever-manifest:*rows, so deleted-package metadata is left behind indefinitely. This needs a non-listcleanup path too, which likely means storing package/revision references somewhere deletions can read withoutkv.list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 420 - 460, removePackageRetrieverManifestCacheEntries currently relies solely on listManifestCacheKeys (which returns [] when kv.list is unavailable), so in fallback environments no package-retriever-manifest:* rows get deleted; add a non-list cleanup path by deriving and deleting manifest keys deterministically from stored package/revision references: ensure the manifest creation path stores per-package revision references (e.g., via a new writeManifestIndex or by augmenting writeLegacyScopeIndex/readScopeEntries) and update removePackageRetrieverManifestCacheEntries to, if listManifestCacheKeys returns empty, enumerate revisions for the given packageId from that index and delete keys using the known key pattern with getRetrieverKv(env).delete(key); keep existing scope cleanup (readScopeEntries, listScopeEntryKeys, writeLegacyScopeIndex) but add this fallback deletion so manifests are always removed even when kv.list is unsupported.
🧹 Nitpick comments (1)
packages/worker/src/mcp/tools/memory-tool-context.ts (1)
50-56: Keep the caught error in the log payload.This path only logs a static message and then drops
error, so retriever import/runtime regressions will be very hard to triage from worker logs. Please include at least the error name/message/stack while still returning empty results.Possible change
} catch (error) { console.error( JSON.stringify({ message: 'package context retrievers unavailable', + error: + error instanceof Error + ? { + name: error.name, + message: error.message, + stack: error.stack, + } + : String(error), }), ) - void error return { results: [], warnings: [], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/memory-tool-context.ts` around lines 50 - 56, The catch block in memory-tool-context.ts currently logs only a static message and discards the caught error; update that catch (in the package context retriever import/runtime path) to include the error details (error.name, error.message and error.stack) in the JSON payload passed to console.error (e.g., add fields like errorName, errorMessage, errorStack), remove the no-op "void error", and preserve the existing behavior of returning empty results from the surrounding function so failures are logged with useful diagnostics while still returning the empty context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 486-493: The retriever-provided fields
(input.memories.retrieverResults[].title, .summary, .details, .source, .url) are
injected verbatim into markdown; update search-format.ts to escape or safely
quote these fields before pushing them into lines (e.g., sanitize or
markdown-escape title and summary in the loop that builds "## Relevant retriever
results"), reusing the existing escape/quote helper implemented in
memory-tool-context.ts (import and call that helper for all affected fields),
and apply the same change to the other block referenced (the code around the
590-602 region) so all retriever outputs are consistently escaped.
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 499-523: The manifest handling assumes cached.retrievers is a
well-formed array and will throw if it's malformed; update the block in the
Promise.all map (around getRetrieverKv / buildPackageRetrieverManifestCacheKey
and the cached variable) to validate cached.retrievers before iterating: check
Array.isArray(cached.retrievers) and that each entry has the expected properties
(retrieverKey truthy and scopes is an array) and if the check fails return [] so
the code fails open; apply the same defensive checks before calling .filter and
.includes (using a guard like typeof retriever.retrieverKey === 'string' and
Array.isArray(retriever.scopes')) to avoid runtime exceptions.
---
Duplicate comments:
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 321-417: Refresh flow can race with
removePackageRetrieverManifestCacheEntries and re-create a deleted package; add
a tombstone/version check to prevent resurrection by making delete write a
tombstone and refresh abort if tombstone exists or version mismatches.
Concretely: in removePackageRetrieverManifestCacheEntries create/update a
package-level tombstone key (e.g., using buildPackageRetrieverManifestCacheKey
or a new buildPackageRetrieverTombstoneKey) storing revision or timestamp; in
refreshPackageRetrieverManifestCache, before putting manifestKey and before
writing scope entries (writeScopeEntry / writeLegacyScopeIndex), read that
tombstone and abort/return null if the tombstone indicates deletion for the same
or newer revision/timestamp; ensure the same tombstone convention is used in
both refreshPackageRetrieverManifestCache and
removePackageRetrieverManifestCacheEntries so writes are conditional on
tombstone absence or older version.
- Around line 420-460: removePackageRetrieverManifestCacheEntries currently
relies solely on listManifestCacheKeys (which returns [] when kv.list is
unavailable), so in fallback environments no package-retriever-manifest:* rows
get deleted; add a non-list cleanup path by deriving and deleting manifest keys
deterministically from stored package/revision references: ensure the manifest
creation path stores per-package revision references (e.g., via a new
writeManifestIndex or by augmenting writeLegacyScopeIndex/readScopeEntries) and
update removePackageRetrieverManifestCacheEntries to, if listManifestCacheKeys
returns empty, enumerate revisions for the given packageId from that index and
delete keys using the known key pattern with getRetrieverKv(env).delete(key);
keep existing scope cleanup (readScopeEntries, listScopeEntryKeys,
writeLegacyScopeIndex) but add this fallback deletion so manifests are always
removed even when kv.list is unsupported.
---
Nitpick comments:
In `@packages/worker/src/mcp/tools/memory-tool-context.ts`:
- Around line 50-56: The catch block in memory-tool-context.ts currently logs
only a static message and discards the caught error; update that catch (in the
package context retriever import/runtime path) to include the error details
(error.name, error.message and error.stack) in the JSON payload passed to
console.error (e.g., add fields like errorName, errorMessage, errorStack),
remove the no-op "void error", and preserve the existing behavior of returning
empty results from the surrounding function so failures are logged with useful
diagnostics while still returning the empty context.
🪄 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: d44bdcd3-353f-4d0c-bccd-a6c3dc9a774b
📒 Files selected for processing (5)
packages/worker/src/mcp/tools/memory-tool-context.node.test.tspackages/worker/src/mcp/tools/memory-tool-context.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/package-retrievers/manifest-cache.node.test.ts
- packages/worker/src/mcp/tools/memory-tool-context.node.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/tools/memory-tool-context.ts (1)
68-80:⚠️ Potential issue | 🟠 MajorStop hiding the retriever env requirement behind
as Env.These APIs still accept an env shape that omits
BUNDLE_ARTIFACTS_KV, then cast it toEnvbefore invoking the retriever service. A caller can satisfy the current type and silently get no retriever context becauserunPackageRetrieversfalls back to empty results when that binding is missing. Make the retriever KV dependency explicit in the input/helper types instead of masking it with a cast.Also applies to: 97-104, 122-129, 146-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/memory-tool-context.ts` around lines 68 - 80, The code hides a required retriever binding by casting the input env to Env before calling runPackageRetrievers; make the dependency explicit by updating the input type for loadRelevantMemoriesForTool (and the helper usages at the other noted sites) to require BUNDLE_ARTIFACTS_KV (e.g., change env: Pick<Env, 'APP_DB' | 'AI'> & Partial<Pick<Env, 'CAPABILITY_VECTOR_INDEX'>> to include BUNDLE_ARTIFACTS_KV), remove the unsafe cast to Env, and pass the strongly-typed env directly into runPackageRetrievers so callers cannot omit the retriever KV. Ensure the same change is applied to the other referenced functions/usages (lines ~97-104, 122-129, 146-152) that currently perform the cast.
🧹 Nitpick comments (2)
packages/worker/src/package-retrievers/manifest-cache.ts (2)
303-305: Consider batching KV reads for large entry sets.
Promise.allon all entry keys creates unbounded concurrent KV requests. While KV handles concurrent reads well, consider batching (e.g., chunks of 50) if you expect users to have many retrievers, to avoid potential rate limiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 303 - 305, The current Promise.all over entryKeys (which calls kv.get for each key) can create unbounded concurrent KV reads; modify the code that populates storedEntries to read keys in bounded batches (e.g., chunks of 50) instead of one large Promise.all. Implement a simple batching loop or helper (chunking entryKeys into arrays of size 50), for each chunk call Promise.all([...chunk].map(key => kv.get(key, 'json'))), await each batch, and concatenate results into storedEntries so the rest of the logic that uses storedEntries remains unchanged.
506-544: Optimization opportunity: fetch manifests incrementally to match limit.The current implementation fetches all manifests before applying the limit at line 543. For users with many retrievers, this could be inefficient. Consider fetching in batches until the limit is satisfied, though the current approach correctly prioritizes correctness over performance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 506 - 544, The function listPackageRetrieversForScope currently reads all scope entries then fetches every manifest before applying the limit; change it to fetch manifests incrementally (sequentially or in small batches) until the requested limit is reached to avoid unnecessary work. Iterate over the references returned by readScopeEntries and for each one call getRetrieverKv(... buildPackageRetrieverManifestCacheKey(...), 'json'), validate the manifest against retrieverManifestCacheVersion and the cached fields, filter entries to PackageRetrieverManifestCacheEntry that match reference.retrieverKey and include input.scope, push those into a result array, and stop fetching more manifests once result.length >= (input.limit ?? 10); finally return result.slice(0, input.limit ?? 10). Ensure behavior and validations (version, userId, packageId, revision, Array.isArray(retrievers)) remain identical to the existing checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/tools/memory-tool-context.ts`:
- Around line 85-117: The current call to runContextPackageRetrievers (and the
similar call at lines 134-159) discards retrieverResult.warnings; update the
return shapes and mapping in memory-tool-context.ts so retriever warnings are
propagated to consumers (e.g., include retrieverWarnings or merge into existing
warnings on MemoryToolSummary). Concretely, modify the Promise.all result
handling for surfaceRelevantMemories and runContextPackageRetrievers to capture
retrieverResult.warnings and include them in the returned object (alongside
retrieverResults and suppressedCount / retrievalQuery), and apply the same
change to the other block that mirrors this behavior so partial retriever
failures are not dropped.
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 893-898: The emitted retriever metadata in the package-detail
markdown should escape free text and format manifest-derived code spans: update
the block that builds the "## Retrievers" lines (the loop over retrievers where
retriever.key, retriever.exportName, retriever.description and retriever.scopes
are used) to call escapeMarkdownText for retriever.description and
formatMarkdownInlineCode for each code span (retriever.key, retriever.exportName
and each scope string) so backticks and other markdown characters are safely
encoded; locate the code in search-format.ts (the lines pushing into lines
within the search({ entity: ... }) related function) and replace raw
interpolations with those helper calls.
---
Outside diff comments:
In `@packages/worker/src/mcp/tools/memory-tool-context.ts`:
- Around line 68-80: The code hides a required retriever binding by casting the
input env to Env before calling runPackageRetrievers; make the dependency
explicit by updating the input type for loadRelevantMemoriesForTool (and the
helper usages at the other noted sites) to require BUNDLE_ARTIFACTS_KV (e.g.,
change env: Pick<Env, 'APP_DB' | 'AI'> & Partial<Pick<Env,
'CAPABILITY_VECTOR_INDEX'>> to include BUNDLE_ARTIFACTS_KV), remove the unsafe
cast to Env, and pass the strongly-typed env directly into runPackageRetrievers
so callers cannot omit the retriever KV. Ensure the same change is applied to
the other referenced functions/usages (lines ~97-104, 122-129, 146-152) that
currently perform the cast.
---
Nitpick comments:
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 303-305: The current Promise.all over entryKeys (which calls
kv.get for each key) can create unbounded concurrent KV reads; modify the code
that populates storedEntries to read keys in bounded batches (e.g., chunks of
50) instead of one large Promise.all. Implement a simple batching loop or helper
(chunking entryKeys into arrays of size 50), for each chunk call
Promise.all([...chunk].map(key => kv.get(key, 'json'))), await each batch, and
concatenate results into storedEntries so the rest of the logic that uses
storedEntries remains unchanged.
- Around line 506-544: The function listPackageRetrieversForScope currently
reads all scope entries then fetches every manifest before applying the limit;
change it to fetch manifests incrementally (sequentially or in small batches)
until the requested limit is reached to avoid unnecessary work. Iterate over the
references returned by readScopeEntries and for each one call getRetrieverKv(...
buildPackageRetrieverManifestCacheKey(...), 'json'), validate the manifest
against retrieverManifestCacheVersion and the cached fields, filter entries to
PackageRetrieverManifestCacheEntry that match reference.retrieverKey and include
input.scope, push those into a result array, and stop fetching more manifests
once result.length >= (input.limit ?? 10); finally return result.slice(0,
input.limit ?? 10). Ensure behavior and validations (version, userId, packageId,
revision, Array.isArray(retrievers)) remain identical to the existing checks.
🪄 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: 442dd286-e108-413b-8e95-740c3c0d1a75
📒 Files selected for processing (9)
packages/worker/src/mcp/tools/markdown-safety.tspackages/worker/src/mcp/tools/memory-tool-context.node.test.tspackages/worker/src/mcp/tools/memory-tool-context.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.ts
✅ Files skipped from review due to trivial changes (2)
- packages/worker/src/mcp/tools/search-format.node.test.ts
- packages/worker/src/mcp/tools/memory-tool-context.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/worker/src/package-registry/service.ts
- packages/worker/src/package-retrievers/manifest-cache.node.test.ts
- packages/worker/src/package-registry/service.node.test.ts
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cab8623. Configure here.
| match: { | ||
| type: 'retriever_result' as const, | ||
| ...result, | ||
| }, |
Contributor
There was a problem hiding this comment.
Retriever results type mismatch with SearchMatch union
Low Severity
The retriever_result variant of SearchMatch declares source: string (required) and details: string | null, but buildRetrieverResultCandidates constructs the match by spreading PackageRetrieverSurfaceResult which has source?: string (possibly undefined) and details?: string (possibly undefined, not null). At runtime source can be undefined, contradicting the type. Downstream code in formatMatchBlock and toSlimStructuredMatches uses ?? fallbacks that mask the mismatch, but any new consumer trusting the SearchMatch type would not guard against undefined.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cab8623. Configure here.
| userId, | ||
| }), | ||
| retrieverRunPromise, | ||
| ]) |
Contributor
There was a problem hiding this comment.
Retriever promise runs unnecessarily for entity-only searches
Low Severity
The retrieverRunPromise is started before checking args.entity, so entity-only lookups pay the cost of invoking package retrievers (DB queries, KV reads, module execution) even though the retriever results are discarded in the entity branch's early return. The condition could additionally check !args.entity to avoid launching retriever work that will never be used.
Reviewed by Cursor Bugbot for commit cab8623. Configure here.
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
package.json#kody.retrieversmanifest support for package-declared search/context retrievers.retriever_resultoutput that is not treated as a resolvable entity.memories.retrieverResultsin the structured search response type and avoid empty memory headings for retriever-only context.Testing
npm run lintnpm run typechecknpx vitest run packages/worker/src/package-registry/manifest.node.test.ts packages/worker/src/package-retrievers/manifest-cache.node.test.ts packages/worker/src/mcp/tools/search.node.test.ts packages/worker/src/mcp/tools/search-format.node.test.ts packages/worker/src/mcp/tools/memory-tool-context.node.test.ts packages/worker/src/package-registry/service.node.test.tsnpx vitest run packages/worker/src/package-retrievers/manifest-cache.node.test.ts packages/worker/src/mcp/observability.workers.test.tsnpx vitest run packages/worker/src/mcp/tools/memory-tool-context.node.test.ts packages/worker/src/package-retrievers/manifest-cache.node.test.ts packages/worker/src/mcp/observability.workers.test.tsnpx vitest run packages/worker/src/package-retrievers/manifest-cache.node.test.tsnpx vitest run packages/worker/src/mcp/tools/search-format.node.test.ts packages/worker/src/mcp/tools/search.node.test.tsnpx vitest run packages/worker/src/mcp/tools/memory-tool-context.node.test.tsnpx vitest run packages/worker/src/package-registry/service.node.test.ts packages/worker/src/package-retrievers/manifest-cache.node.test.ts packages/worker/src/mcp/tools/search-format.node.test.ts packages/worker/src/mcp/tools/memory-tool-context.node.test.tsnpx vitest run packages/worker/src/mcp/tools/search-handler.node.test.ts packages/worker/src/mcp/tools/search.node.test.tsgit push -u origin cursor/retriever-hooks-5d59(pre-push rannpm run test:push, including worker tests and Playwright E2E)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests