feat: Implement memory management tools and split plugins folder #185
Conversation
📝 WalkthroughWalkthroughAdds two new plugins (Remember, Approval), a comprehensive utils library (fs/crypto/storage), provider/context plumbing for context-scoped providers and context-extensions, many storage/crypto/storage adapters and tests, new demo e2e app for Remember, packaging/project configs for multiple plugins, and documentation updates. No removals of declared public APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Transport as Transport/Server
participant ToolRunner as Tool Execution
participant PluginReg as PluginRegistry
participant Approval as ApprovalCheckPlugin
participant Remember as RememberAccessor
participant Crypto as RememberCrypto
participant Storage as StorageAdapter
Client->>Transport: HTTP/Socket request (invoke tool)
Transport->>PluginReg: resolve tool + providers
PluginReg-->>ToolRunner: tool instance (with contextProviders)
ToolRunner->>Approval: pre-exec approval check
Approval->>PluginReg: fetch tool approval config
alt approval required
Approval->>Client: reject with ApprovalRequiredError / PKCE challenge
else approval OK
Approval-->>ToolRunner: continue
end
ToolRunner->>Remember: call accessor (set/get/forget/list)
Remember->>Crypto: encrypt/decrypt if enabled
Crypto->>Storage: store/retrieve serialized blob
Storage-->>Crypto: raw data
Crypto-->>Remember: decrypted/plain value
Remember-->>ToolRunner: result
ToolRunner-->>Transport: response
Transport-->>Client: response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
|
| Status | Scanner |  Critical |
 High |
 Medium |
 Low |
Total (2) |
|---|---|---|---|---|---|---|
| Open Source Security | 0 | 0 | 0 | 0 | See details | |
| ⛔ | Code Security | 0 | 2 | 0 | 0 | 2 issues |
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Fix all issues with AI Agents 🤖
In @apps/e2e/demo-e2e-remember/jest.e2e.config.ts:
- Around line 34-40: The moduleNameMapper entry for
'^@frontmcp/plugins/remember$' is incorrect and unnecessary; remove that mapping
so Jest uses the existing '^@frontmcp/plugins$' barrel mapping
(moduleNameMapper) which re-exports the remember plugin
(@frontmcp/plugin-remember); update the moduleNameMapper block in
jest.e2e.config.ts by deleting the '^@frontmcp/plugins/remember$' line to match
demo-e2e-ui/demo-e2e-transparent behavior and allow proper resolution via the
barrel export.
In @docs/draft/docs/plugins/remember-plugin.mdx:
- Around line 546-548: The example incorrectly uses this.remember.forget('*', {
scope: 'user' }) which is invalid because remember.forget requires a specific
key; replace it by first calling this.remember.list({ scope: 'user' })
(optionally with a pattern like pattern: 'session_*') to retrieve matching keys
and then iterate over the returned keys, calling this.remember.forget(key, {
scope: 'user' }) for each key to perform bulk deletion.
In @libs/plugins/src/index.ts:
- Around line 31-41: The current re-export lists RememberPlugin as a named
export but the source package exports it as the default; update the export to
use "default as RememberPlugin" while keeping the existing named exports
(RememberPluginOptions, RememberPluginOptionsInput, RememberAccessorToken,
ApprovalServiceToken, RememberScope, ApprovalScope, ApprovalState) so the module
signature matches other plugins like CachePlugin and DashboardPlugin.
In @libs/sdk/src/common/tokens/plugin.tokens.ts:
- Line 18: Add a documentation page for the new public token "contextExtensions"
(referenced as tokenFactory.meta('contextExtensions') in plugin.tokens.ts) under
the docs tree (either docs/draft/docs/** or docs/docs/**) that explains the
token purpose, schema/shape, runtime behavior, and a short usage example showing
how to register and consume the token in a plugin; ensure the doc follows
existing SDK docs conventions (frontmatter, headings, examples) and links from
any token/API index so the new API is discoverable.
In @libs/sdk/src/context/context-extension.ts:
- Around line 73-86: The getter added to PromptContext.prototype swallows the
original exception in a bare catch and rethrows a generic Error; update the
catch in the Object.defineProperty getter (the function using this.get(token as
Token<unknown>)) to capture the caught error (e), and rethrow a new Error that
includes defaultErrorMessage plus the original error details (or attach the
original error as the cause) so debugging shows the underlying failure from
this.get.
- Around line 61-71: The property getter on ExecutionContextBase currently
swallows all exceptions from this.get(token as Token<unknown>) by using a bare
catch; change it to catch the error as a variable, inspect or log it, and only
replace it with the defaultErrorMessage for the specific "token not found"
case—otherwise rethrow or wrap the original error (preserving the original error
as the cause) so provider initialization/type errors aren't lost; update the
getter on ExecutionContextBase.prototype to catch (err), differentiate
token-missing from other errors (or at minimum log err) and then either throw
new Error(defaultErrorMessage) for token-not-found or rethrow/wrap err for other
failures.
In @libs/sdk/src/plugin/plugin.registry.ts:
- Around line 253-261: The exported mapping incorrectly casts instances to
ProviderEntry even for CONTEXT-scoped providers; update the construction of
`exported` (the map over `normalized` created from `dynamicProviders` and
`providers.getAllSingletons()`) to avoid the unsafe cast on
`singletons.get(def.provide)`: either remove the `as ProviderEntry` cast and let
the type be optional/undefined, or explicitly use a fallback so `instance` is
typed as `ProviderEntry | undefined` (e.g., return `singletons.get(def.provide)
|| undefined`), ensuring `mergeFromRegistry` still works for GLOBAL-scoped
providers without masking type errors.
In @libs/sdk/src/provider/provider.utils.ts:
- Around line 17-34: The parameter of extractProviderMetadata is currently typed
as any; replace it with a concrete type (e.g., a new ProviderLike interface that
includes provide?, metadata?, name?, scope?, description?, id?) or use unknown
plus a type-guard to validate the shape before accessing properties; update
references inside extractProviderMetadata to use the new type so nested can be
typed as ProviderMetadata | undefined and remove any uses of any while
preserving the existing logic that merges nested and top-level fields and falls
back to tokenName(item.provide) || 'UnknownProvider' when name is missing.
In @libs/uipack/package.json:
- Line 62: The package.json currently lists a non-existent dependency
"enclave-vm" which will break installs; open the deps block in
libs/uipack/package.json, remove the "enclave-vm": "^2.1.0" entry if it's
unused, or replace it with the correct npm package name/version if it was a typo
(confirm the intended package), or configure it as a private/internal package
(and update package.json to use the proper registry/alias) before committing;
ensure package-lock/yarn.lock is updated and run install to verify the fix.
In @plugins/plugin-cache/README.md:
- Around line 30-43: Update the README CachePlugin example to match the actual
API: replace the incorrect properties (store, redisUrl, ttl) with the real ones
(type and defaultTTL) and provide separate valid configuration examples for each
supported type: 'memory' (type: 'memory', defaultTTL), 'redis' (type: 'redis',
config: { host, port, password?, db? }, defaultTTL), 'redis-client' (type:
'redis-client', client: RedisClient instance, defaultTTL) and 'global-store'
(type: 'global-store' used when FrontMcp global redis.provider is 'vercel-kv');
keep references to CachePlugin.configure and defaultTTL and the config/client
shapes so readers can match the cache.types.ts definitions.
In @plugins/plugin-remember/src/__tests__/remember-memory.provider.test.ts:
- Around line 1-350: Add tests covering constructor validation, operations after
close, invalid inputs and error propagation: create new RememberMemoryProvider
instances with negative, zero, NaN and non-numeric sweepInterval values and
assert constructor does not crash or throws expected error types; after calling
RememberMemoryProvider.close() assert that setValue rejects with an error (use
instanceof checks on the thrown error), getValue and keys behave as expected
(getValue returns undefined, keys rejects or returns empty consistent with
implementation); add tests for invalid patterns to keys() (e.g., malformed regex
like '[invalid') asserting it throws and that the thrown error is instanceof the
expected Error class, and add TTL edge cases such as Infinity and extremely
large numbers verifying behavior (value persists for Infinity, and extremely
large TTL does not crash), referencing the class RememberMemoryProvider and its
methods setValue, getValue, keys, close, exists when adding assertions.
In @plugins/plugin-remember/src/approval/approval-check.hook.ts:
- Around line 96-108: The current resolveApprovalConfig function spreads
...config after setting defaults which allows explicit undefined properties in
config to overwrite defaults; change the construction so undefineds don't
override defaults by either spreading config first and then setting required:
result.required ??= true and defaultScope: result.defaultScope ??=
ApprovalScope.SESSION, or by returning { ...config, required: config.required ??
true, defaultScope: config.defaultScope ?? ApprovalScope.SESSION } (ensure you
still handle boolean shortcuts at top); refer to resolveApprovalConfig,
ToolApprovalRequirement, and ApprovalScope when updating the return logic.
In @plugins/plugin-remember/src/providers/remember-accessor.provider.ts:
- Around line 260-273: In buildScopePrefix, the const toolName declared in the
case 'tool' falls through the switch's lexical scope; wrap the 'tool' case body
in its own block so toolName is block-scoped (e.g., change case 'tool': to case
'tool': { ... } ), then return from inside that block and close it before the
next case to satisfy static analysis.
In @plugins/plugin-remember/src/providers/remember-memory.provider.ts:
- Around line 178-184: The patternToRegex function builds a RegExp from user
input and can be exploited for ReDoS via many wildcards; to fix it, validate and
constrain the pattern before converting: enforce a maximum pattern length (e.g.
200 chars) and a maximum count or maximum run length of wildcard characters ('*'
and '?') and reject or truncate patterns that exceed those limits, then proceed
with the existing escaping + replacement logic in patternToRegex; also consider
rejecting patterns containing pathological repetitions (e.g. /(.*){n}/-like
sequences) or pre-checking for excessive backtracking tokens before constructing
the RegExp.
In @plugins/plugin-remember/src/providers/remember-redis.provider.ts:
- Around line 118-123: The close() method currently always calls
this.client.quit(), which will close an externally-provided Redis client when
options.type === 'redis-client'; change the provider to track ownership (e.g.,
add a private ownsClient flag set true when the provider creates the client
internally and false when initialized with type: 'redis-client' or given a
client) and update close() to only call this.client.quit() if ownsClient is true
(otherwise no-op), leaving externally-owned clients open.
In @plugins/plugin-remember/src/tools/forget.tool.ts:
- Around line 57-59: The scope validation calls this.fail(...) but does not stop
execution; add an early return (or throw) immediately after the this.fail(...)
call in the same validation block so the function does not continue when scope
is invalid—locate the scope check that uses allowedScopes.includes(scope) and
this.fail(...) in forget.tool.ts and ensure it returns (or throws) right after
calling this.fail to prevent further processing.
In @plugins/plugin-remember/src/tools/list-memories.tool.ts:
- Around line 59-61: In the validation block inside list-memories.tool.ts where
you call this.fail(new Error(`Scope '${scope}' is not allowed...`)), add an
explicit return immediately after the this.fail(...) call (or change this.fail
to throw) so execution cannot continue past the invalid-scope check; locate the
this.fail(...) call in the scope validation inside the ListMemoriesTool (or the
method that performs scope validation) and ensure you exit the function after
signaling failure.
In @plugins/plugin-remember/src/tools/remember-this.tool.ts:
- Around line 69-71: The validation block in remember-this.tool.ts calls
this.fail(...) when scope is invalid but does not stop execution, so add an
immediate early exit after the this.fail(...) call (e.g., return or throw) to
prevent continued execution; update the same validation in the scope-checking
logic that uses allowedScopes and scope to mirror the fix made in
list-memories.tool.ts so execution halts after this.fail().
🧹 Nitpick comments (27)
plugins/plugin-remember/src/remember.secret-persistence.ts (3)
63-67: Consider stricter base64url length validation.The comment correctly notes that 32 bytes base64url-encoded yields ~43 characters, but
z.string().min(32)would accept shorter strings (32-42 chars) that can't be valid 32-byte base64url encodings. This is unlikely to cause issues in practice since you generate the secrets yourself, but stricter validation could catch corruption earlier.🔎 Proposed stricter validation
const rememberSecretDataSchema = z.object({ - secret: z.string().min(32), // base64url of 32 bytes is ~43 chars + secret: z.string().min(43).max(44), // base64url of 32 bytes = 43 chars (no padding) createdAt: z.number().positive().int(), version: z.literal(1), });
140-170: LGTM with minor suggestion.Good error handling with graceful ENOENT handling for first-run scenarios. The use of
unknowntype for caught errors follows coding guidelines.One minor refinement: on line 160, after validation you cast the raw
dataobject. Since Zod'ssafeParsealready returns typed data viaresult.data, you could leverage that directly for stronger type safety without the cast.🔎 Optional: use Zod's parsed output
// Validate structure using Zod schema - const validation = validateSecretData(data); - if (!validation.valid) { + const parseResult = rememberSecretDataSchema.safeParse(data); + if (!parseResult.success) { console.warn( - `[RememberSecretPersistence] Invalid secret file format at ${secretPath}: ${validation.error}, will regenerate`, + `[RememberSecretPersistence] Invalid secret file format at ${secretPath}: ${parseResult.error.issues[0]?.message ?? 'Invalid structure'}, will regenerate`, ); return null; } + + // Additional time-based validation + const parsed = parseResult.data; + const now = Date.now(); + if (parsed.createdAt > now + 60000 || parsed.createdAt < now - 100 * 365 * 24 * 60 * 60 * 1000) { + console.warn(`[RememberSecretPersistence] Invalid createdAt timestamp, will regenerate`); + return null; + } - return data as RememberSecretData; + return parsed;This would require inlining the time validation or refactoring
validateSecretDatato return the parsed data on success.
307-312: Consider clearing the promise guard inclearCachedSecretfor complete test isolation.Currently
clearCachedSecretonly clearscachedSecretbut notsecretGenerationPromise. If tests call this while a generation is in-flight, subsequent calls might still return the in-flight promise. For testing purposes this is likely fine since tests typically await operations, but for complete isolation you could clear both.🔎 Proposed fix for complete cache clearing
/** * Clear the cached secret (for testing). */ export function clearCachedSecret(): void { cachedSecret = null; + secretGenerationPromise = null; }apps/e2e/demo-e2e-remember/src/apps/memory/prompts/memory-summary.prompt.ts (2)
17-17: Validate scope argument to prevent runtime errors.The scope is type-asserted without validation. If a user provides an invalid scope value (e.g.,
"invalid"), it will be cast to the union type but may cause unexpected behavior when passed toremember.list()orremember.get().🔎 Suggested validation
async execute(args: Record<string, string>): Promise<GetPromptResult> { - const scope = (args['scope'] ?? 'session') as 'session' | 'user' | 'tool' | 'global'; + const validScopes = ['session', 'user', 'tool', 'global'] as const; + const scopeArg = args['scope'] ?? 'session'; + if (!validScopes.includes(scopeArg as any)) { + throw new Error(`Invalid scope: ${scopeArg}. Must be one of: ${validScopes.join(', ')}`); + } + const scope = scopeArg as 'session' | 'user' | 'tool' | 'global'; const remember = this.get(RememberAccessorToken);
21-25: Consider pagination for large memory stores.The current implementation fetches all memories in a loop without pagination or limits. While appropriate for demo/e2e purposes, this could cause performance issues or memory exhaustion with large datasets in production scenarios.
apps/e2e/demo-e2e-remember/src/apps/memory/tools/check-memory.tool.ts (1)
5-10: Consider splitting the scope field definition for readability.The scope field definition on line 8 chains multiple methods on a single line. While functionally correct, splitting it across multiple lines (as done in
recall-value.tool.tslines 8-12) would improve readability.🔎 Proposed refactor for improved readability
const inputSchema = z .object({ key: z.string().describe('Key to check'), - scope: z.enum(['session', 'user', 'tool', 'global']).optional().default('session').describe('Scope to check in'), + scope: z + .enum(['session', 'user', 'tool', 'global']) + .optional() + .default('session') + .describe('Scope to check in'), }) .strict();plugins/plugin-remember/src/approval/approval.types.ts (1)
71-75: Consider using.strict()on Zod schemas for stricter validation.Based on learnings, validation schemas should use
.strict()to reject unknown properties. This applies toapprovalContextSchemaand other object schemas in this file.🔎 Suggested fix
-export const approvalContextSchema = z.object({ +export const approvalContextSchema = z.object({ type: z.string().min(1), identifier: z.string().min(1), metadata: z.record(z.string(), z.unknown()).optional(), -}); +}).strict();Apply similar changes to
delegationContextSchema,approvalGrantorSchema,approvalRevokerSchema,approvalRecordSchema, andtoolApprovalRequirementSchema.plugins/plugin-remember/src/approval/approval.utils.ts (1)
375-383: Default to 'user' source may mask missing data.When
inputisundefined,normalizeGrantordefaults to{ source: 'user' }. This could silently hide cases where the grantor was genuinely missing, making audit trails incomplete.Consider throwing an error or requiring explicit input, especially for audit-critical paths.
apps/e2e/demo-e2e-remember/src/apps/memory/tools/remember-value.tool.ts (2)
5-12: Consider adding validation constraints on the TTL field.The
ttlfield accepts any number without bounds. Consider adding constraints to prevent negative values or unreasonably large values that could cause issues with the memory storage backend.🔎 Suggested validation constraints
const inputSchema = z .object({ key: z.string().describe('Key to store the value under'), value: z.string().describe('Value to remember'), scope: z.enum(['session', 'user', 'tool', 'global']).optional().default('session').describe('Scope of the memory'), - ttl: z.number().optional().describe('Time to live in seconds'), + ttl: z.number().int().positive().max(31536000).optional().describe('Time to live in seconds (max 1 year)'), }) .strict();
14-19: Consider using the same enum type for scope in the output schema.The output schema defines
scopeas a genericz.string(), while the input uses a specific enum. Using the same enum in the output would provide better type safety and consistency.🔎 Suggested change
const outputSchema = z.object({ success: z.boolean(), key: z.string(), - scope: z.string(), + scope: z.enum(['session', 'user', 'tool', 'global']), message: z.string(), });apps/e2e/demo-e2e-remember/e2e/remember.e2e.test.ts (1)
42-50: Consider parsing JSON responses for more robust assertions.The test uses
toHaveTextContent('"found":false')which relies on string matching of JSON output. This is brittle and could pass even if the JSON structure changes.🔎 More robust test assertion approach
- expect(result).toBeSuccessful(); - expect(result).toHaveTextContent('"found":false'); - expect(result).toHaveTextContent('"value":null'); + expect(result).toBeSuccessful(); + const response = JSON.parse(result.content[0].text); + expect(response.found).toBe(false); + expect(response.value).toBeNull();This approach validates the actual structure rather than string patterns.
apps/e2e/demo-e2e-remember/src/apps/memory/resources/memory-stats.resource.ts (1)
26-42: Consider adding error handling for memory operations.The
remember.list()calls could potentially throw errors (e.g., storage backend failures, permission issues). Currently, any exceptions would propagate uncaught to the resource caller.🔎 Add defensive error handling
async execute(): Promise<Output> { const remember = this.get(RememberAccessorToken); - const sessionKeys = await remember.list({ scope: 'session' }); - const globalKeys = await remember.list({ scope: 'global' }); + + let sessionKeys: string[]; + let globalKeys: string[]; + + try { + sessionKeys = await remember.list({ scope: 'session' }); + globalKeys = await remember.list({ scope: 'global' }); + } catch (error) { + throw new Error(`Failed to retrieve memory stats: ${error}`); + } return {libs/sdk/src/plugin/plugin.registry.ts (1)
240-278: Complex dual registration with defensive runtime checks.The dual registration to both
this.providers(app-level) andthis.scope.providers(scope-level) is necessary because app providers are a child of scope providers, not a parent. However, the runtime type checking at lines 271-277 indicates architectural coupling.The runtime checks (
'mergeFromRegistry' in scopeProvidersandtypeofcheck) suggest that theProviderRegistryInterfaceshould formally declaremergeFromRegistryif it's expected to be callable on scope providers. Consider either:
- Add
mergeFromRegistryto the interface if it's part of the contract- Use a more specific type for
scope.providersthat declares this method- Document why this runtime check is necessary if there's a legitimate reason for the interface mismatch
The current approach works but couples the code to implementation details rather than interface contracts.
Based on coding guidelines: Use specific error classes with proper error handling instead of silent failures.
libs/sdk/src/provider/provider.registry.ts (1)
872-883: Consider clarifying the type assertion on line 876.The cast
token as anyinthis.order.has(token as any)suppresses type checking. Sincethis.orderisSet<Token>andtokencomes from iteratingthis.defs(which isMap<Token, ProviderRecord>), the types should align without casting.Suggested fix
for (const [token, rec] of this.defs) { // Skip if already in order (already processed above) - if (this.order.has(token as any)) continue; + if (this.order.has(token)) continue;plugins/plugin-remember/src/__tests__/approval.service.test.ts (1)
41-46: Potential test flakiness with short timeout.The 10ms delay after a 1ms TTL approval may be insufficient on slow CI runners. Consider using a slightly longer margin or a deterministic approach (e.g., mocking
Date.now).Suggested improvement
it('returns false when approval is expired', async () => { - await service.grantTimeLimitedApproval('tool-1', 1); // 1ms TTL - await new Promise((r) => setTimeout(r, 10)); + await service.grantTimeLimitedApproval('tool-1', 50); // 50ms TTL + await new Promise((r) => setTimeout(r, 100)); // Wait for expiration const result = await service.isApproved('tool-1'); expect(result).toBe(false); });plugins/plugin-remember/src/__tests__/remember-accessor.provider.test.ts (1)
385-404: Consider making the encryption validation test more explicit.The test at lines 396-399 indirectly validates encryption by checking that the raw value doesn't match the plaintext. Consider adding an explicit assertion or comment to make the test intention clearer.
🔎 Suggested improvement
// Should not be readable as plain JSON expect(() => { const parsed = JSON.parse(raw!); + // When encrypted, the raw value should be an encrypted blob, not plain data expect(parsed.value).not.toBe('my-password'); }).not.toThrow();plugins/plugin-remember/src/remember.plugin.ts (2)
217-223: Add explicit types to factory parameters for clarity.The
useFactorycallback parameters (store,ctx,cfg) lack type annotations. While TypeScript may infer types from theinjectarray, explicit types improve readability and catch errors earlier.🔎 Proposed fix
providers.push({ name: 'remember:accessor', provide: RememberAccessorToken, scope: ProviderScope.CONTEXT, inject: () => [RememberStoreToken, FRONTMCP_CONTEXT, RememberConfigToken] as const, - useFactory: (store, ctx, cfg) => createRememberAccessor(store, ctx, cfg), + useFactory: (store: RememberStoreToken, ctx: FrontMcpContext, cfg: RememberPluginOptions) => + createRememberAccessor(store, ctx, cfg), });
243-249: Consider extracting userId resolution to a shared utility.The userId extraction logic duplicates the pattern from
RememberAccessor(seeplugins/plugin-remember/src/providers/remember-accessor.provider.tslines 234-243). Centralizing this logic would reduce duplication and ensure consistent behavior.🔎 Example extraction
// In a shared utility file, e.g., remember.utils.ts export function resolveUserId(ctx: FrontMcpContext): string | undefined { const authInfo = ctx.authInfo; return ( (authInfo?.extra?.['userId'] as string | undefined) ?? (authInfo?.extra?.['sub'] as string | undefined) ?? authInfo?.clientId ); }plugins/plugin-remember/src/approval/approval-check.hook.ts (1)
43-48: Consider handling missing sessionId more explicitly.Defaulting
sessionIdto'unknown'when unavailable could cause approval collisions if multiple unauthenticated/anonymous sessions share the same approval records. Consider logging a warning or using a more unique fallback (e.g., request ID) to prevent unintended approval sharing.🔎 Suggested improvement
const ctx = toolContext.tryGetContext?.(); - const sessionId = ctx?.sessionId ?? 'unknown'; + const sessionId = ctx?.sessionId; + if (!sessionId) { + // No session context - cannot manage approvals + // Consider whether to allow or deny by default + return; + }Alternatively, if anonymous approvals are intentional, document this behavior.
plugins/plugin-remember/src/providers/remember-vercel-kv.provider.ts (1)
45-68: Consider using dynamic import for ESM compatibility.Using
require()works but may cause issues in ESM-only environments. Consider using dynamicimport()with proper async initialization, or document that this provider requires a CommonJS environment.🔎 Alternative with dynamic import
+ private kvPromise?: Promise<VercelKvClient>; + constructor(options: RememberVercelKvProviderOptions = {}) { - // Lazy import @vercel/kv to avoid bundling when not used - const vercelKv = require('@vercel/kv'); - // Validate partial configuration const hasUrl = options.url !== undefined; const hasToken = options.token !== undefined; if (hasUrl !== hasToken) { throw new Error( `RememberVercelKvProvider: Both 'url' and 'token' must be provided together. ` + `Received: url=${hasUrl ? 'provided' : 'missing'}, token=${hasToken ? 'provided' : 'missing'}`, ); } + + this.kvPromise = this.initKv(options); + this.keyPrefix = options.keyPrefix ?? 'remember:'; + this.defaultTTL = options.defaultTTL; + } + + private async initKv(options: RememberVercelKvProviderOptions): Promise<VercelKvClient> { + const vercelKv = await import('@vercel/kv'); if (options.url && options.token) { - this.kv = vercelKv.createClient({ + return vercelKv.createClient({ url: options.url, token: options.token, }); } else { - this.kv = vercelKv.kv; + return vercelKv.kv; } - - this.keyPrefix = options.keyPrefix ?? 'remember:'; - this.defaultTTL = options.defaultTTL; }Note: This would require updating all methods to await
this.kvPromisebefore use.plugins/plugin-remember/src/approval/approval-memory.store.ts (1)
180-198: Mutation before delete has no effect.Lines 186-190 update the existing record with revocation details, but Line 193 immediately deletes the record. The mutations are lost. If audit logging is needed, consider emitting an event or returning the final record state before deletion.
🔎 Suggested improvement
async revokeApproval(options: RevokeApprovalOptions): Promise<boolean> { const key = this.buildKey(options.toolId, options.sessionId, options.userId, options.context); const existing = this.approvals.get(key); if (existing) { - // Update with revocation info (for potential audit logging before delete) + // Build revoked record for potential audit/logging const revokedBy = normalizeRevoker(options.revokedBy); - existing.state = ApprovalState.EXPIRED; - existing.revokedAt = Date.now(); - existing.revokedBy = revokedBy; - existing.revocationReason = options.reason; + const revokedRecord: ApprovalRecord = { + ...existing, + state: ApprovalState.EXPIRED, + revokedAt: Date.now(), + revokedBy, + revocationReason: options.reason, + }; + + // TODO: Emit audit event or log revokedRecord if needed // Delete the record (in future, could move to separate revocation log) this.approvals.delete(key); return true; } return false; }plugins/plugin-remember/src/remember.crypto.ts (2)
101-105: Avoid including the custom key in the HKDF info parameter.Including the raw custom key in the
contextstring (which becomes the HKDFinfoparameter) is unusual. While not a direct security vulnerability (HKDF info is public), it leaks key material into what is typically treated as public context data and could complicate debugging/logging.🔎 Proposed fix
case 'custom': // Custom: use provided key as IKM ikm = source.key; - context = `remember:custom:${source.key}`; + context = 'remember:custom'; break;
179-196: Silent error suppression may hide decryption issues.Returning
nullon any decryption failure (including JSON parse errors on Line 192) makes debugging difficult. Consider logging errors at debug level or differentiating between decryption failures and parse failures for better observability.🔎 Proposed enhancement for observability
export async function decryptValue<T = unknown>( blob: EncryptedBlob, keySource: EncryptionKeySource, ): Promise<T | null> { try { const key = await deriveEncryptionKey(keySource); const iv = base64urlDecode(blob.iv); const tag = base64urlDecode(blob.tag); const ciphertext = base64urlDecode(blob.data); const decrypted = decryptAesGcm(key, ciphertext, iv, tag); return JSON.parse(textDecoder.decode(decrypted)) as T; - } catch { + } catch (err) { + if (process.env['DEBUG']) { + console.debug('[RememberPlugin:Crypto] Decryption failed:', err); + } return null; } }plugins/plugin-remember/src/approval/approval.service.ts (2)
1-12: Remove unused type imports.
ApprovalScope(type),ApprovalSourceType, andRevocationSourceTypeare imported but not used in this file.ApprovalScopeis only used asApprovalScopeEnum(the runtime enum import on line 12).🔎 Proposed fix
import type { ApprovalStore, ApprovalQuery } from './approval-store.interface'; import type { ApprovalRecord, - ApprovalScope, ApprovalContext, ApprovalGrantor, ApprovalRevoker, - ApprovalSourceType, - RevocationSourceType, } from './approval.types'; import { ApprovalScope as ApprovalScopeEnum, ApprovalState } from './approval.types';
127-133: Consider documenting query override behavior.The
queryApprovalsmethod merges user-providedsessionId/userIdwith defaults, but the spread order means user values are overwritten. This may be intentional (to scope queries to the current context), but should be documented./** * Query approvals with custom filters. + * Note: sessionId and userId default to the current context if not provided. * * @param query - Query filters * @returns Array of matching approval records */plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
67-78: Type safety concern with fallback to raw string.When JSON parsing fails, returning
raw as unknown as Tbypasses type safety. If the stored value is a plain string butTis expected to be an object, this could cause runtime errors downstream.🔎 Proposed fix with explicit string handling
async getValue<T = unknown>(key: string, defaultValue?: T): Promise<T | undefined> { const fullKey = this.keyPrefix + key; const raw = await this.client.get(fullKey); if (raw === null) return defaultValue; try { return JSON.parse(raw) as T; } catch { - return raw as unknown as T; + // Only return raw string if T is compatible with string + // This maintains backward compatibility for string-typed values + return raw as T; } }Alternatively, consider always storing values as JSON (even strings) and removing the fallback entirely, which would make the behavior more predictable.
plugins/plugin-remember/src/remember.types.ts (1)
204-206: Consider strengthening RememberPluginOptionsInput type.The current type allows any partial combination, which could lead to invalid configurations (e.g.,
type: 'redis'withoutconfig). Consider using discriminated union patterns or runtime validation.This is acceptable for initial implementation since runtime validation likely occurs in the plugin initialization code. However, you could strengthen this with conditional types:
export type RememberPluginOptionsInput = | Partial<MemoryRememberPluginOptions> | (Partial<RedisRememberPluginOptions> & { type: 'redis'; config: RedisRememberPluginOptions['config'] }) | // ... similar for other types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/plugin-remember/src/remember.secret-persistence.ts (1)
141-171: Consider using type guards for error handling.The error handling uses type assertions like
(error as NodeJS.ErrnoException)and(error as Error). While this pattern is common in Node.js, consider using type guards for stricter TypeScript compliance:function isNodeError(error: unknown): error is NodeJS.ErrnoException { return typeof error === 'object' && error !== null && 'code' in error; }However, this is a minor refinement—the current implementation is acceptable and widely used.
apps/e2e/demo-e2e-remember/src/apps/memory/tools/list-memories.tool.ts (2)
7-11: Consider removing redundant.optional()call.In Zod v4,
.default()already makes a field optional and provides a default value when the input isundefined. The.optional()call before.default()is redundant.🔎 Proposed refactor
- scope: z - .enum(['session', 'user', 'tool', 'global']) - .optional() - .default('session') - .describe('Scope to list memories from'), + scope: z + .enum(['session', 'user', 'tool', 'global']) + .default('session') + .describe('Scope to list memories from'),
32-43: Consider adding a limit parameter to prevent unbounded results.The current implementation returns all matching memory keys without any limit. If a user has thousands of stored memories, this could cause performance issues, timeouts, or overwhelming responses.
🔎 Proposed enhancement
Add a
limitparameter to the input schema and apply it in the execute method:const inputSchema = z .object({ scope: z .enum(['session', 'user', 'tool', 'global']) .optional() .default('session') .describe('Scope to list memories from'), pattern: z.string().optional().describe('Optional pattern to filter keys'), + limit: z.number().int().positive().optional().default(50).describe('Maximum number of keys to return'), }) .strict(); const outputSchema = z.object({ scope: z.string(), keys: z.array(z.string()), count: z.number(), + truncated: z.boolean().describe('Whether results were truncated due to limit'), });async execute(input: Input): Promise<Output> { const keys = await this.remember.list({ scope: input.scope, pattern: input.pattern, }); + const limitedKeys = keys.slice(0, input.limit); + return { scope: input.scope, - keys, - count: keys.length, + keys: limitedKeys, + count: limitedKeys.length, + truncated: keys.length > input.limit, }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/e2e/demo-e2e-remember/src/apps/memory/index.tsapps/e2e/demo-e2e-remember/src/apps/memory/prompts/memory-summary.prompt.tsapps/e2e/demo-e2e-remember/src/apps/memory/resources/memory-stats.resource.tsapps/e2e/demo-e2e-remember/src/apps/memory/tools/check-memory.tool.tsapps/e2e/demo-e2e-remember/src/apps/memory/tools/forget-value.tool.tsapps/e2e/demo-e2e-remember/src/apps/memory/tools/list-memories.tool.tsapps/e2e/demo-e2e-remember/src/apps/memory/tools/recall-value.tool.tsapps/e2e/demo-e2e-remember/src/apps/memory/tools/remember-value.tool.tsplugins/plugin-remember/src/remember.secret-persistence.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/e2e/demo-e2e-remember/src/apps/memory/tools/remember-value.tool.ts
- apps/e2e/demo-e2e-remember/src/apps/memory/resources/memory-stats.resource.ts
- apps/e2e/demo-e2e-remember/src/apps/memory/index.ts
- apps/e2e/demo-e2e-remember/src/apps/memory/tools/check-memory.tool.ts
- apps/e2e/demo-e2e-remember/src/apps/memory/prompts/memory-summary.prompt.ts
- apps/e2e/demo-e2e-remember/src/apps/memory/tools/recall-value.tool.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
apps/e2e/demo-e2e-remember/src/apps/memory/tools/list-memories.tool.tsplugins/plugin-remember/src/remember.secret-persistence.tsapps/e2e/demo-e2e-remember/src/apps/memory/tools/forget-value.tool.ts
🧬 Code graph analysis (2)
apps/e2e/demo-e2e-remember/src/apps/memory/tools/list-memories.tool.ts (5)
apps/e2e/demo-e2e-remember/src/apps/memory/tools/check-memory.tool.ts (1)
Tool(21-39)apps/e2e/demo-e2e-remember/src/apps/memory/tools/forget-value.tool.ts (1)
Tool(26-45)apps/e2e/demo-e2e-remember/src/apps/memory/tools/recall-value.tool.ts (1)
Tool(26-45)apps/e2e/demo-e2e-remember/src/apps/memory/tools/remember-value.tool.ts (1)
Tool(24-44)plugins/plugin-remember/src/index.ts (1)
ListMemoriesTool(144-144)
apps/e2e/demo-e2e-remember/src/apps/memory/tools/forget-value.tool.ts (4)
apps/e2e/demo-e2e-remember/src/apps/memory/tools/check-memory.tool.ts (1)
Tool(21-39)apps/e2e/demo-e2e-remember/src/apps/memory/tools/list-memories.tool.ts (1)
Tool(25-44)apps/e2e/demo-e2e-remember/src/apps/memory/tools/recall-value.tool.ts (1)
Tool(26-45)apps/e2e/demo-e2e-remember/src/apps/memory/tools/remember-value.tool.ts (1)
Tool(24-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
🔇 Additional comments (16)
apps/e2e/demo-e2e-remember/src/apps/memory/tools/forget-value.tool.ts (5)
1-3: LGTM! Clean imports.The imports are well-structured and necessary for the tool implementation. The type augmentation import on line 3 properly enables
this.remembertyping.
5-14: LGTM! Schema definition is consistent and well-structured.The input schema correctly follows the same pattern as other memory tools in the app (remember-value, recall-value, check-memory, list-memories). The use of
.strict()prevents unintended properties, and the scope enum with a sensible default of'session'aligns with Zod 4 best practices.
16-21: LGTM! Output schema matches the established pattern.The output schema is consistent with
remember-value.tool.tsand other memory tools. Thescopefield is typed asz.string()rather than the enum, which appropriately handles the resolved enum value in the output.
23-24: LGTM! Proper type inference.The type aliases correctly use
z.inferto derive TypeScript types from the schemas, ensuring type safety throughout the implementation.
26-45: LGTM! Well-implemented and consistent with other memory tools.The
ForgetValueToolcorrectly:
- Uses the
@Tooldecorator with appropriate metadata- Extends
ToolContextwith proper generic type parameters (follows coding guidelines)- Implements the
executemethod with proper async/await- Calls
this.remember.forget()with the correct parameters- Returns a structured success response
The implementation matches the pattern established by
remember-value.tool.tsand other memory tools in this app. Error handling is appropriately delegated to the framework level—any errors fromthis.remember.forget()will propagate automatically.plugins/plugin-remember/src/remember.secret-persistence.ts (6)
53-54: Snyk false positive: This is a file path, not a hardcoded secret.The Snyk warning about a hardcoded secret on line 53 appears to be a false positive.
DEFAULT_SECRET_FILE_PATHis a file path constant indicating where the secret will be stored, not the secret value itself. The actual secret is generated dynamically using cryptographically secure random bytes in thegenerateSecret()function (line 250).The comment explicitly clarifies: "not the secret itself".
64-95: LGTM! Solid validation with appropriate time bounds.The validation logic correctly uses Zod v4's
safeParseand applies sensible time boundaries (1-minute future drift for clock skew, 100-year maximum age). The minimum secret length check (32 characters) aligns with the base64url encoding of 32 bytes.
104-114: LGTM! Good security posture for production environments.Disabling persistence in production by default (requiring explicit
forceEnable) is the correct approach, especially for multi-server deployments. The documentation correctly warns against using file-based persistence in production and recommends theREMEMBER_SECRETenvironment variable instead.
183-217: LGTM! Excellent security practices for secret persistence.The implementation follows security best practices:
- Atomic write pattern (temp file + rename) prevents corruption
- Restrictive file permissions (
0o600) ensure owner-only access- Directory permissions (
0o700) prevent unauthorized directory access- Unique temp filename with random bytes prevents collisions
- Proper error handling with temp file cleanup
This is a well-implemented secure persistence layer.
224-234: LGTM! Correct idempotent deletion behavior.Properly handles the
ENOENTcase (file already deleted) as a success condition, making the operation idempotent. Other errors are logged but don't throw, which is appropriate for a cleanup operation.
265-306: LGTM! Excellent concurrency handling with promise guard.The implementation correctly prevents race conditions during secret generation:
- Promise guard (
secretGenerationPromise) ensures only one generation occurs at a time- Finally block guarantees cleanup even on errors
- Caching reduces file I/O overhead
- The pattern handles concurrent callers elegantly by making them await the same promise
The module-level state is appropriate for this use case and provides good performance characteristics.
apps/e2e/demo-e2e-remember/src/apps/memory/tools/list-memories.tool.ts (5)
1-3: LGTM!The imports are appropriate. The side-effect import of
@frontmcp/plugin-rememberenables type augmentation forthis.remember.
16-20: AI summary describes non-existent output field.The AI summary claims the output schema includes a
truncatedboolean field and mentions "truncation indicator," but the actual implementation only returnsscope,keys, andcount. Thetruncatedfield is not present in the code.
22-23: LGTM!Standard and correct type inference from Zod schemas.
25-31: LGTM!The tool decorator configuration and class definition follow the standard pattern and correctly use type parameters.
32-43: AI summary describes non-existent features.The AI summary claims this method "validates scope against allowed scopes" and "applies limit truncation (default 50)," but the actual implementation:
- Only relies on the input schema's enum constraint (no additional runtime validation)
- Has no limit or truncation logic whatsoever
- Returns all matching keys without any upper bound
The description is inconsistent with the actual code.
…ndling, and Redis client management
There was a problem hiding this comment.
Actionable comments posted: 8
Fix all issues with AI Agents 🤖
In @apps/e2e/demo-e2e-remember/jest.e2e.config.ts:
- Around line 34-40: The moduleNameMapper includes a redundant mapping for
'@frontmcp/plugin-remember' which is already re-exported by the
'@frontmcp/plugins' barrel; remove the '^@frontmcp/plugin-remember$' entry from
moduleNameMapper in jest.e2e.config.ts so only the '^@frontmcp/plugins$' mapping
remains, ensuring tests resolve RememberPlugin via the plugins barrel export and
match the other E2E configs.
In @plugins/plugin-remember/src/approval/approval-check.hook.ts:
- Around line 153-177: In handleApprovalRequired, the approval-state string is
determined incorrectly by comparing existingApproval?.state to
ApprovalState.EXPIRED; instead call the expiration helper on the existing record
(use existingApproval?.isExpired() or the module's isExpired(existingApproval)
helper) so an expired approval (e.g., state APPROVED with past expiresAt) yields
'expired' rather than 'pending'; update the condition in handleApprovalRequired
(the throw new ApprovalRequiredError block) to use the isExpired check and keep
the rest of the payload unchanged.
- Around line 122-134: The code is using unsafe assertions when extracting
ApprovalContext in getCurrentContext; add a runtime type guard method named
isApprovalContext(value: unknown): value is ApprovalContext that verifies
object, non-null, and that 'type' and 'identifier' exist and are strings, then
replace the direct casts for toolContext?.input?.['context'] and
ctx?.authInfo?.extra?.['approvalContext'] with checks that call
isApprovalContext(...) before assigning to contextFromInput and
contextFromSession, returning the validated ApprovalContext or undefined so
downstream callers like isPreApprovedContext can safely access type and
identifier.
- Around line 45-48: The code assigns userId using unsafe type assertions on
ctx.authInfo.extra['userId'] and ['sub']; replace the casts with runtime type
guards: check typeof ctx?.authInfo?.extra?.['userId'] === 'string' (and
similarly for 'sub') before using them, or extract via a small helper like
getStringExtra(key) that returns string | undefined after validation, then fall
back to ctx?.authInfo?.clientId; remove the "as string | undefined" assertions
to satisfy strict TypeScript rules and prevent non-string values from leaking
downstream.
In @plugins/plugin-remember/src/providers/remember-memory.provider.ts:
- Around line 119-139: In keys(), when skipping expired entries call the public
async delete method instead of directly mutating the Map: replace direct
this.memory.delete(key) with await this.delete(key) (or return value-handled
this.delete(key)) so the timeout cleanup and other side effects in the
delete(key) method run; keep the surrounding isExpired(entry) check and continue
logic otherwise unchanged.
In @plugins/plugin-remember/src/providers/remember-redis.provider.ts:
- Line 45: The DEBUG env check in remember-redis.provider.ts uses a loose truthy
test (process.env['DEBUG']) which treats strings like "false" or "0" as true;
change the condition to a strict boolean check such as comparing to 'true'
(process.env.DEBUG === 'true') or use a small parse helper (e.g.,
parseBoolean(process.env.DEBUG)) where the check occurs (the if around the DEBUG
conditional in the provider) so that only explicit true enables debug behavior.
In @plugins/plugin-remember/src/tools/list-memories.tool.ts:
- Around line 9-16: The input schema object listMemoriesInputSchema should be
wrapped in z.object() to match the other tool schemas; replace the plain object
with a z.object({ ... }) that preserves the same keys and validations (scope,
pattern, limit) and keep descriptions and optional modifiers unchanged so
validation behavior remains identical to the existing output schema and the
other tools (forget.tool.ts, remember-this.tool.ts).
In @plugins/plugin-remember/src/tools/remember-this.tool.ts:
- Around line 35-41: The manually defined RememberThisInput type can drift from
the schema; replace it with a derived type using z.infer so it stays consistent
with rememberThisInputSchema (i.e., change RememberThisInput to be
z.infer<typeof rememberThisInputSchema>); ensure rememberThisInputSchema is
wrapped in z.object() first if it isn't already, and update any imports/usages
that reference RememberThisInput to use the inferred type.
♻️ Duplicate comments (6)
plugins/plugin-remember/src/tools/list-memories.tool.ts (1)
59-61: Past review issue resolved.The code now correctly uses
throw this.fail(...)to ensure execution stops after the validation failure. This addresses the concern from the previous review and is consistent with the pattern used in forget.tool.ts and remember-this.tool.ts.plugins/plugin-remember/src/providers/remember-memory.provider.ts (1)
184-201: ReDoS protections correctly implemented.The method includes comprehensive ReDoS protections with
MAX_PATTERN_LENGTHandMAX_WILDCARDSlimits, addressing the concern from the previous review. The static analysis warning is a false positive in this context.libs/sdk/src/plugin/plugin.registry.ts (1)
253-262: The type castas ProviderEntry | undefinedstill masks the actual return type.The comment on line 258-260 states the cast is safe because
mergeFromRegistryonly uses instances for GLOBAL-scoped providers. However,singletons.get(def.provide)returnsunknown(fromReadonlyMap<Token, unknown>), notProviderEntry. The cast obscures this type mismatch. A cleaner approach would be to let the type be inferred or use explicit type narrowing.🔎 Proposed improvement
const exported = normalized.map((def) => ({ token: def.provide, def, - // For CONTEXT-scoped providers, instance may not exist yet (built per-request). - // mergeFromRegistry only uses instance for GLOBAL-scoped providers. - // The singletons map stores ProviderEntry values, so this cast is safe. - instance: singletons.get(def.provide) as ProviderEntry | undefined, + // For CONTEXT-scoped providers, instance may not exist yet (built per-request). + // mergeFromRegistry only uses instance for GLOBAL-scoped providers. + instance: singletons.get(def.provide) as ProviderEntry | undefined, }));Since
mergeFromRegistrysignature expectsProviderEntry | undefined, andsingletons.get()returnsunknown, consider updatinggetAllSingletons()return type toReadonlyMap<Token, ProviderEntry>if that's the actual invariant, or acceptunknowninmergeFromRegistry.docs/draft/docs/plugins/remember-plugin.mdx (1)
547-551: Past issue resolved: bulk deletion now correctly useslist()thenforget().The code correctly implements the bulk deletion pattern that was flagged in the previous review:
- ✅ Calls
this.remember.list({ scope: 'user' })to retrieve all keys- ✅ Iterates over each key
- ✅ Calls
this.remember.forget(key, { scope: 'user' })for each individual keyThis is the proper way to delete multiple keys since
forget()doesn't support wildcard patterns.plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
23-24: LGTM! Ownership tracking properly implemented.The
ownsClientfield correctly addresses the previous review concern about closing externally-provided Redis clients. This ensures that only internally-created clients are closed via theclose()method.plugins/plugin-remember/src/tools/remember-this.tool.ts (1)
69-71: Past review concern not fully resolved - verifythis.fail()usage pattern.The past review suggested adding
returnbeforethis.fail(), but the current code usesthrow this.fail(). Whilethrowdoes stop execution, it's unclear ifthis.fail()is designed to be thrown or returned. Ifthis.fail()returns an error result that should be passed back to the SDK, throwing it could cause incorrect error handling or double-wrapping.Verify the correct usage pattern for
this.fail()in the SDK:#!/bin/bash # Search for this.fail() usage patterns in the SDK and other tools rg -A 2 -B 2 'this\.fail\(' --type ts plugins/ libs/sdk/ # Check ToolContext.fail() implementation ast-grep --pattern 'class ToolContext { $$$ fail($$$) { $$$ } $$$ }'If
this.fail()is meant to be returned (not thrown), use the pattern from the past review:return this.fail(new Error(`Scope '${scope}' is not allowed. Allowed scopes: ${allowedScopes.join(', ')}`));
🧹 Nitpick comments (8)
plugins/plugin-remember/src/tools/list-memories.tool.ts (1)
10-11: Consider using shared constants for scope values.The scope enum values
['session', 'user', 'tool', 'global']are hardcoded here and duplicated in forget.tool.ts and remember-this.tool.ts. If the RememberScope type changes, all tools must be updated manually, risking inconsistency.Consider defining a shared constant or deriving the enum values from the RememberScope type to maintain a single source of truth.
Also applies to: 56-57
plugins/plugin-remember/src/providers/remember-memory.provider.ts (1)
31-35: Add validation for sweepIntervalSeconds parameter.The constructor accepts
sweepIntervalSecondswithout validation. If a caller passes0, negative values, or extremely large values, it could cause unexpected behavior (e.g., continuous sweeping or indefinite delay of cleanup).🔎 Suggested validation
constructor(sweepIntervalSeconds = 60) { + if (sweepIntervalSeconds <= 0) { + throw new Error('sweepIntervalSeconds must be positive'); + } this.sweeper = setInterval(() => this.sweep(), sweepIntervalSeconds * 1000); // Don't keep the process alive just for the sweeper (this.sweeper as { unref?: () => void }).unref?.(); }libs/sdk/src/provider/provider.utils.ts (1)
109-110: Consider usingunknowninstead of type assertion toProviderLike.The final error path casts
itemtoProviderLiketo access.name, but at this point we knowitemis neither a class nor a valid object. The cast could be misleading. Using optional chaining on the original parameter would be safer.🔎 Proposed improvement
- const name = (item as ProviderLike)?.name ?? String(item); + const name = (typeof item === 'object' && item !== null && 'name' in item) + ? (item as { name?: string }).name ?? String(item) + : String(item); throw new Error(`Invalid provider '${name}'. Expected a class or a provider object.`);Alternatively, since we're in an error path anyway:
- const name = (item as ProviderLike)?.name ?? String(item); + const name = String((item as Record<string, unknown>)?.name ?? item); throw new Error(`Invalid provider '${name}'. Expected a class or a provider object.`);libs/sdk/src/provider/provider.registry.ts (1)
873-884: Consider avoiding theas anycast on line 877.The cast
token as anyforthis.order.has()check is needed becauseorderisSet<Token>but iteration overdefsyields the key type directly. This works but could be cleaner.🔎 Suggested improvement
The
orderset storesTokenvalues, anddefskeys are alsoToken. The cast suggests a type mismatch that could be addressed by ensuring consistent typing:for (const [token, rec] of this.defs) { // Skip if already in order (already processed above) - if (this.order.has(token as any)) continue; + if (this.order.has(token)) continue;If the types are actually compatible, the cast may not be needed. If there's a genuine mismatch between
this.defskey type andthis.orderelement type, consider aligning them in the class definition.docs/draft/docs/plugins/remember-plugin.mdx (1)
63-83: Clarify the relationship between simple and configured plugin initialization.The documentation shows two initialization patterns that may confuse users:
- Line 70:
plugins: [RememberPlugin](simple, uses defaults)- Line 498-508:
RememberPlugin.init({ ... })(configured)Consider adding a note in this basic setup section explaining that
RememberPluginwithout.init()uses default settings (memory store, encryption enabled), and that users should see the "Storage Options" and "Complete Example" sections for customization.Suggested clarification
@App({ id: 'my-app', name: 'My App', - plugins: [RememberPlugin], // Default: memory store, encryption enabled + plugins: [RememberPlugin], // Uses defaults: memory store, encryption enabled tools: [ /* your tools */ ], }) class MyApp {}And add a note after the code block:
<Info> This uses default settings (memory store, encryption enabled). To customize storage, encryption, or approval settings, use `RememberPlugin.init({ ... })` instead. See [Storage Options](#storage-options) and [Complete Example](#complete-example). </Info>plugins/plugin-remember/src/approval/approval-check.hook.ts (1)
59-59: Consider failing fast if ApprovalStore is not available.Based on learnings, hooks should validate dependencies and fail fast with specific error messages. If
ApprovalStoreTokenis not registered,this.get()might throw a generic error.Consider adding explicit validation:
const approvalStore = this.get(ApprovalStoreToken); if (!approvalStore) { throw new Error('ApprovalStore is not configured. Ensure RememberPlugin is properly initialized.'); }plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
50-52: Consider structured error handling.The error handler only logs to
console.error. For better observability and error tracking, consider:
- Using a structured logger
- Re-throwing or emitting errors through a proper error handling mechanism
- Allowing callers to handle connection errors
plugins/plugin-remember/src/tools/remember-this.tool.ts (1)
73-77: Consider adding error handling for storage operations.The
remember.set()call has no try-catch wrapper. If the storage operation fails (e.g., Redis connection error, serialization failure), the error will propagate unhandled, potentially causing a poor user experience.🔎 Suggested error handling approach
+ try { await remember.set(input.key, input.value, { scope, ttl: input.ttl, brand: input.brand, }); + } catch (error) { + return this.fail(new Error(`Failed to store memory: ${error instanceof Error ? error.message : String(error)}`)); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/e2e/demo-e2e-remember/jest.e2e.config.tsdocs/draft/docs/plugins/remember-plugin.mdxlibs/plugins/src/index.tslibs/sdk/src/context/context-extension.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/provider/provider.registry.tslibs/sdk/src/provider/provider.utils.tsplugins/plugin-cache/README.mdplugins/plugin-remember/src/approval/approval-check.hook.tsplugins/plugin-remember/src/index.tsplugins/plugin-remember/src/providers/remember-accessor.provider.tsplugins/plugin-remember/src/providers/remember-memory.provider.tsplugins/plugin-remember/src/providers/remember-redis.provider.tsplugins/plugin-remember/src/tools/forget.tool.tsplugins/plugin-remember/src/tools/list-memories.tool.tsplugins/plugin-remember/src/tools/remember-this.tool.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/plugin-remember/src/providers/remember-accessor.provider.ts
- libs/plugins/src/index.ts
- plugins/plugin-cache/README.md
- plugins/plugin-remember/src/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
plugins/plugin-remember/src/providers/remember-redis.provider.tslibs/sdk/src/plugin/plugin.registry.tsapps/e2e/demo-e2e-remember/jest.e2e.config.tslibs/sdk/src/context/context-extension.tslibs/sdk/src/provider/provider.utils.tsplugins/plugin-remember/src/approval/approval-check.hook.tsplugins/plugin-remember/src/tools/list-memories.tool.tsplugins/plugin-remember/src/tools/remember-this.tool.tsplugins/plugin-remember/src/tools/forget.tool.tslibs/sdk/src/provider/provider.registry.tsplugins/plugin-remember/src/providers/remember-memory.provider.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/context/context-extension.tslibs/sdk/src/provider/provider.utils.tslibs/sdk/src/provider/provider.registry.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/context/context-extension.tslibs/sdk/src/provider/provider.utils.tslibs/sdk/src/provider/provider.registry.ts
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/plugins/remember-plugin.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/plugins/remember-plugin.mdx
🧠 Learnings (16)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/common/records/**/*.ts : Centralize record types in common/records directory and import from there instead of module-specific files
Applied to files:
libs/sdk/src/plugin/plugin.registry.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/package.json : Entry points must match the documented paths: frontmcp/ui/react, frontmcp/ui/renderers, frontmcp/ui/render, frontmcp/ui/universal, frontmcp/ui/bundler, frontmcp/ui/bridge, frontmcp/ui/components, frontmcp/ui/layouts, frontmcp/ui/web-components
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/bundler/**/*.{ts,tsx} : The bundler module must re-export utilities from frontmcp/uipack/bundler and provide SSR component bundling functionality
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.{ts,tsx} : Never import React-free utilities from frontmcp/ui; use frontmcp/uipack for bundling, build tools, platform adapters, and theme utilities
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `changeScope` property instead of `scope` in change events to avoid confusion with Scope class
Applied to files:
libs/sdk/src/context/context-extension.tslibs/sdk/src/provider/provider.utils.tsplugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `getCapabilities()` method for dynamic capability exposure instead of hardcoding capabilities
Applied to files:
libs/sdk/src/context/context-extension.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/context/context-extension.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{src}/**/*.{ts,tsx,js,jsx} : Do not expose internal error details in public APIs - use sanitized error messages
Applied to files:
libs/sdk/src/context/context-extension.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Include error class `instanceof` checks in tests to validate error handling
Applied to files:
libs/sdk/src/context/context-extension.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows and fail fast on invalid hook configurations with specific error messages
Applied to files:
libs/sdk/src/context/context-extension.tsplugins/plugin-remember/src/approval/approval-check.hook.tsplugins/plugin-remember/src/tools/list-memories.tool.tsplugins/plugin-remember/src/tools/remember-this.tool.tsplugins/plugin-remember/src/tools/forget.tool.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/sdk/src/context/context-extension.tslibs/sdk/src/provider/provider.utils.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific error classes with MCP error codes instead of generic errors
Applied to files:
libs/sdk/src/context/context-extension.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Use strict TypeScript mode with no `any` types without strong justification - use `unknown` instead for generic type defaults
Applied to files:
libs/sdk/src/provider/provider.utils.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{src}/**/*.{ts,tsx} : Do not use `any` type without justification in TypeScript code
Applied to files:
libs/sdk/src/provider/provider.utils.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/provider/provider.utils.ts
🧬 Code graph analysis (6)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
plugins/plugin-remember/src/remember.types.ts (2)
RedisRememberPluginOptions(154-162)RedisClientRememberPluginOptions(167-170)
libs/sdk/src/plugin/plugin.registry.ts (3)
libs/sdk/src/context/context-extension.ts (1)
installContextExtensions(42-92)libs/sdk/src/provider/provider.utils.ts (1)
normalizeProvider(55-111)libs/sdk/src/provider/provider.registry.ts (1)
ProviderRegistry(32-1021)
libs/sdk/src/context/context-extension.ts (1)
libs/sdk/src/common/metadata/plugin.metadata.ts (1)
ContextExtension(30-48)
plugins/plugin-remember/src/tools/list-memories.tool.ts (5)
libs/plugins/src/index.ts (2)
RememberScope(38-38)RememberAccessorToken(36-36)plugins/plugin-remember/src/tools/forget.tool.ts (1)
Tool(37-74)plugins/plugin-remember/src/tools/remember-this.tool.ts (1)
Tool(49-88)plugins/plugin-remember/src/providers/remember-memory.provider.ts (1)
keys(119-139)plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
keys(105-121)
plugins/plugin-remember/src/tools/forget.tool.ts (2)
plugins/plugin-remember/src/tools/list-memories.tool.ts (1)
Tool(39-81)plugins/plugin-remember/src/tools/remember-this.tool.ts (1)
Tool(49-88)
libs/sdk/src/provider/provider.registry.ts (2)
libs/sdk/src/common/interfaces/execution-context.interface.ts (1)
scope(134-136)libs/sdk/src/context/frontmcp-context.ts (1)
scope(306-308)
🪛 ast-grep (0.40.3)
plugins/plugin-remember/src/providers/remember-memory.provider.ts
[warning] 199-199: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escaped}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (36)
plugins/plugin-remember/src/tools/forget.tool.ts (4)
1-4: LGTM!Imports are clean and properly organized with appropriate type-only imports.
6-32: LGTM!Schema definitions are well-structured and follow established patterns. The use of
z.inferforForgetOutputis a good practice for maintaining type safety.
34-48: LGTM!Tool decorator configuration is appropriate, with clear user-facing description and correct
readOnlyHint: falsefor this state-modifying operation.
49-74: Previous issue resolved - implementation looks good!The past review concern about missing early return has been properly addressed. The code now uses
throw this.fail(...)(line 58), which is actually better than the suggestedreturn this.fail(...)because it guarantees execution stops. This pattern is consistent with other tools in the plugin (remember-this.tool.tsandlist-memories.tool.ts).The implementation correctly:
- Validates scope against allowed scopes
- Checks existence before deletion (for accurate
existedflag)- Performs the deletion
- Returns comprehensive result information
plugins/plugin-remember/src/providers/remember-memory.provider.ts (3)
41-69: LGTM! Well-designed TTL handling.The implementation correctly handles TTL edge cases with the
ttlSeconds > 0check, clears previous timeouts, respects Node.js setTimeout limits, and usesunref()to prevent keeping the process alive.
75-91: LGTM! Proper expiration handling and graceful fallback.The method correctly checks expiration, uses the public
delete()method to ensure timeout cleanup, and includes a sensible fallback for malformed data with appropriate type assertions.
144-150: LGTM! Comprehensive cleanup.The close method properly releases all resources: sweeper interval, per-key timeouts, and memory storage.
libs/sdk/src/plugin/plugin.registry.ts (2)
232-238: LGTM! Context extension installation is correctly placed.The context extension installation is properly guarded with a null/length check and called after hook registration, ensuring plugin-provided extensions are available for downstream tool/resource/prompt contexts.
271-278: Good defensive runtime check for scope provider method availability.The runtime guard checking for
mergeFromRegistryexistence before calling it is appropriate defensive coding, sincescope.providersis typed asProviderRegistryInterfacewhich may not expose this method. The type assertion toProviderRegistryis justified by the preceding runtime check.libs/sdk/src/provider/provider.utils.ts (3)
4-21: Well-structured ProviderLike interface.The new
ProviderLikeinterface properly types the provider configuration object with appropriate optional fields and supports both top-level and nested metadata patterns. This addresses the previous concern aboutanytypes in this utility.
31-53: LGTM! Clean metadata extraction with proper precedence.The
extractProviderMetadatafunction correctly implements the documented behavior: nested metadata is used as the base, top-level fields override when present, and a default name is provided as fallback. The conditional on line 50 properly handles the case whereitem.providemight be undefined.
55-72: Good improvement to type safety in normalizeProvider.The signature change from
anytoType | ProviderLikeand the use of the typedproviderItemvariable improves type safety. The metadata extraction is now centralized throughextractProviderMetadata.libs/sdk/src/provider/provider.registry.ts (2)
620-634: LGTM! Properly documented and implemented instance handling.The updated comment accurately describes the behavior: instances are only set for GLOBAL-scoped providers when defined. The scope defaulting to
GLOBALmatches the behavior ingetProviderScope().
669-690: Good refactor separating provider registration from instantiation.The logic correctly:
- Registers all providers in the graph (tokens, defs, edges)
- Only instantiates GLOBAL-scoped providers
- Defers CONTEXT-scoped provider instantiation to
buildViews()This enables dynamic providers from plugins to be properly resolved during both tool context creation and flow execution.
libs/sdk/src/context/context-extension.ts (5)
42-55: LGTM! Proper idempotency and conflict detection.The function correctly:
- Skips already-installed extensions (idempotent)
- Warns and skips when property already exists from another source
- Uses
Object.prototype.hasOwnProperty.callfor safe property existence check
60-72: Error handling now properly preserves the original error.The error handling has been improved to preserve the original error as the
cause, addressing the previous review comment. This enables debugging while still providing a user-friendly error message.
74-88: Consistent error handling for PromptContext.The same improved error handling pattern is applied to PromptContext, maintaining consistency across both context types.
102-115: LGTM! Useful inspection utilities.The
isContextExtensionInstalledandgetInstalledContextExtensionshelpers provide useful introspection capabilities for testing and debugging.
1-31: Good module structure with clear documentation.The module documentation clearly explains the purpose and usage pattern. The module-level
installedExtensionsSet provides appropriate singleton tracking for idempotent installation.Note: Module-level state means extensions persist across tests. Consider exposing a
clearInstalledExtensions()function (marked@internal) for test cleanup, as no cleanup mechanism currently exists and prototype modifications (lines 61-72, 76-87) could cause test pollution.docs/draft/docs/plugins/remember-plugin.mdx (1)
357-417: All API signatures in the documentation are correct and match the implementation.The
brandparameter,patternsupport, andset()options documented in lines 357-417 have been verified against the implementation:
brand: 'preference'is valid—it's a PayloadBrandType for type discriminationpattern: 'user:*'is supported by thelist()method with glob-style matching- All
set()parameters (scope,ttl,brand,metadata) match the RememberSetOptions interfaceNo changes needed.
plugins/plugin-remember/src/approval/approval-check.hook.ts (3)
1-20: LGTM! Clean plugin setup.The imports, decorator, and class declaration follow SDK patterns correctly. The plugin name and description are clear.
96-109: LGTM! Spread operator issue from previous review has been resolved.The configuration resolution now correctly spreads
configfirst, then applies defaults with??, ensuring that explicitundefinedvalues don't override the fallback values. This matches the recommended pattern from the previous review.
139-147: LGTM! Pre-approval matching logic is correct.The comparison logic properly checks both
typeandidentifierfor context matching. Note: this assumescurrentContexthas the correct shape (see comment ongetCurrentContextfor type safety concerns).apps/e2e/demo-e2e-remember/jest.e2e.config.ts (4)
1-5: LGTM: Clean ESM-to-CommonJS interop setup.The import structure correctly sets up Jest configuration with proper TypeScript typing and CommonJS interop for loading the coverage preset.
7-16: LGTM: Well-configured E2E test settings.The configuration appropriately sets up the E2E test environment with sequential execution (
maxWorkers: 1), adequate timeout (60s), andforceExitto prevent hanging tests.
16-33: LGTM: Proper TypeScript and decorator transformation setup.The transform configuration correctly enables decorators and decorator metadata with @swc/jest, which is necessary for dependency injection and modern TypeScript features. The ES2022 target and jose library inclusion are appropriate.
41-45: LGTM: Proper coverage configuration and export.The coverage directory follows the conventional path structure, and the config is correctly typed and exported for Jest consumption.
plugins/plugin-remember/src/providers/remember-redis.provider.ts (4)
58-67: LGTM! setValue implementation is correct.The method correctly:
- Constructs the prefixed key
- Serializes non-string values to JSON
- Uses Redis
SETwith optionalEXparameter for TTL support
72-83: Verify type safety in getValue fallback.When
JSON.parsefails (line 78-82), the method casts the raw string toTwithout validation:return raw as unknown as T;This can cause type safety issues if:
- Caller expects a structured type (e.g.,
{ name: string }) but gets a plain string- The stored value is not JSON but caller expects a parsed object
Consider one of these approaches:
- Document that callers must handle potential type mismatches
- Add runtime validation using Zod (already a dependency per library context)
- Return
unknownand require callers to validateAs per coding guidelines, avoid type assertions without validation. Consider if the current fallback behavior meets type safety requirements for this provider's use cases.
105-121: LGTM! Efficient key enumeration using SCAN.The method correctly uses Redis
SCANfor non-blocking cursor-based iteration instead of the blockingKEYScommand. The prefix stripping logic ensures callers receive keys without the internal prefix.
37-41: Document that Redis client may not be immediately ready after construction.The provider instantiates the Redis client with
lazyConnect: false, which attempts an immediate connection in the constructor but returns synchronously before the connection is established. While ioredis handles this via built-in command queuing and auto-reconnection (withmaxRetriesPerRequest: 3), this behavior should be documented.Consider adding a comment or README note clarifying that:
- The provider may not be ready to serve requests immediately after construction
- Initial operations may fail if Redis is unavailable, but will retry up to 3 times
- Connection errors are logged asynchronously (see lines 50-52)
If explicit readiness checking becomes necessary, implement a
ready()orisConnected()method; however, the current implicit approach is consistent with how ioredis is used elsewhere in the codebase.plugins/plugin-remember/src/tools/remember-this.tool.ts (5)
1-4: LGTM!Imports are clean and follow best practices.
28-33: LGTM!Output schema is properly defined using
z.object()wrapper, following correct Zod v4 patterns.
49-59: LGTM!Tool decorator is properly configured with clear description and appropriate metadata. The
readOnlyHint: falsecorrectly indicates this tool modifies state.
79-79: LGTM!The expiration calculation correctly converts TTL from seconds to milliseconds and adds it to the current timestamp.
9-23: No changes needed. The plain object format forrememberThisInputSchemais the correct and designed pattern.The SDK's
ToolInputTypeexplicitly accepts bothz.ZodRawShape(plain object) andz.ZodObject<any>. The framework is designed to work with plain objects—buildAgentToolDefinitions()explicitly expectsinputSchemato be aZodRawShapeand internally converts it usingz.object(tool.inputSchema)when needed. This format is consistently used across all tools in the plugin and is the standard pattern.Likely an incorrect or invalid review comment.
plugins/plugin-remember/src/providers/remember-redis.provider.ts
Outdated
Show resolved
Hide resolved
… type safety and cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @plugins/plugin-remember/src/providers/remember-redis.provider.ts:
- Around line 78-82: The fallback path unsafely casts a raw string to T (the
JSON.parse(raw) catch returns raw as unknown as T); replace this with a safe
behavior: in the catch return undefined and update the function/method signature
to return T | undefined (or change to a discriminated union that includes string
if that is desired), update all callers to handle undefined (or the union), and
add a JSDoc above the method documenting the new return contract; alternatively,
if you must keep returning the raw string, explicitly change the return type to
T | string and add runtime validation to coerce/validate the parsed value before
casting.
🧹 Nitpick comments (4)
apps/e2e/demo-e2e-remember/jest.e2e.config.ts (1)
4-5: Consider adding explicit type annotation for the coverage preset.The
e2eCoveragePresetloaded viarequireis implicitly typed asany. While this is acceptable for configuration files and CommonJS interop, you could optionally add an explicit type annotation for improved type safety:const require = createRequire(import.meta.url); -const e2eCoveragePreset = require('../../../jest.e2e.coverage.preset.js'); +const e2eCoveragePreset: Partial<Config.InitialOptions> = require('../../../jest.e2e.coverage.preset.js');However, since this is standard Jest configuration boilerplate and the spread operator on line 41 will surface any incompatibilities, this remains an optional improvement.
plugins/plugin-remember/src/approval/approval-check.hook.ts (1)
122-150: Great: Runtime validation implemented correctly.The introduction of
isApprovalContexttype guard and its usage ingetCurrentContextproperly addresses the unsafe type assertion concerns. The type guard validates object structure at runtime before returning typed values.✅ Addressed past review comment: unsafe type assertions on context extraction.
💡 Optional: Minor type guard refinement
The assertions on lines 147-148 could be replaced with a slightly safer pattern:
private isApprovalContext(value: unknown): value is ApprovalContext { - return ( - typeof value === 'object' && - value !== null && - 'type' in value && - 'identifier' in value && - typeof (value as ApprovalContext).type === 'string' && - typeof (value as ApprovalContext).identifier === 'string' - ); + if (typeof value !== 'object' || value === null) return false; + const obj = value as Record<string, unknown>; + return ( + 'type' in obj && + 'identifier' in obj && + typeof obj.type === 'string' && + typeof obj.identifier === 'string' + ); }This avoids casting to
ApprovalContextbefore validation is complete, though both approaches are acceptable for type guard implementation.plugins/plugin-remember/src/providers/remember-memory.provider.ts (1)
174-182: Consider using public delete() method for consistency.The
sweep()method directly manipulatesthis.memory.delete(key)and clears timeouts manually, while other methods (likekeys()) now properly delegate to the publicdelete()method. For consistency and maintainability, consider refactoring.🔎 Proposed refactor for consistency
/** * Periodically remove expired keys to keep memory tidy. */ - private sweep(): void { + private async sweep(): Promise<void> { const now = Date.now(); + const expiredKeys: string[] = []; for (const [key, entry] of this.memory) { if (entry.expiresAt !== undefined && entry.expiresAt <= now) { - if (entry.timeout) clearTimeout(entry.timeout); - this.memory.delete(key); + expiredKeys.push(key); } } + for (const key of expiredKeys) { + await this.delete(key); + } }Also update the constructor to handle the async sweep:
constructor(sweepIntervalSeconds = 60) { - this.sweeper = setInterval(() => this.sweep(), sweepIntervalSeconds * 1000); + this.sweeper = setInterval(() => void this.sweep(), sweepIntervalSeconds * 1000); // Don't keep the process alive just for the sweeper (this.sweeper as { unref?: () => void }).unref?.(); }plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
26-27: Consider documenting keyPrefix format expectations.The
keyPrefixis concatenated directly with keys without adding separators (e.g.,this.keyPrefix + key). This means users must include any desired separator (like":") as part of the prefix. While this provides flexibility, it might not be immediately obvious and could lead to unintended key collisions.Consider adding a JSDoc note to document this behavior:
/** * @param options.keyPrefix - Prefix for all Redis keys. Include any separator (e.g., "user:") as part of the prefix. */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/e2e/demo-e2e-remember/jest.e2e.config.tsplugins/plugin-remember/src/approval/approval-check.hook.tsplugins/plugin-remember/src/providers/remember-memory.provider.tsplugins/plugin-remember/src/providers/remember-redis.provider.tsplugins/plugin-remember/src/tools/forget.tool.tsplugins/plugin-remember/src/tools/list-memories.tool.tsplugins/plugin-remember/src/tools/remember-this.tool.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/plugin-remember/src/tools/remember-this.tool.ts
- plugins/plugin-remember/src/tools/list-memories.tool.ts
- plugins/plugin-remember/src/tools/forget.tool.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
apps/e2e/demo-e2e-remember/jest.e2e.config.tsplugins/plugin-remember/src/providers/remember-memory.provider.tsplugins/plugin-remember/src/providers/remember-redis.provider.tsplugins/plugin-remember/src/approval/approval-check.hook.ts
🧠 Learnings (13)
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{package.json,*.ts,*.tsx,*.js,*.jsx} : Do not add React dependencies to frontmcp/uipack - it must remain React-free. Use frontmcp/ui for React components.
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.{ts,tsx} : Never import React-free utilities from frontmcp/ui; use frontmcp/uipack for bundling, build tools, platform adapters, and theme utilities
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/bundler/**/*.{ts,tsx} : The bundler module must re-export utilities from frontmcp/uipack/bundler and provide SSR component bundling functionality
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/package.json : Entry points must match the documented paths: frontmcp/ui/react, frontmcp/ui/renderers, frontmcp/ui/render, frontmcp/ui/universal, frontmcp/ui/bundler, frontmcp/ui/bridge, frontmcp/ui/components, frontmcp/ui/layouts, frontmcp/ui/web-components
Applied to files:
apps/e2e/demo-e2e-remember/jest.e2e.config.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows and fail fast on invalid hook configurations with specific error messages
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{src}/**/*.{ts,tsx} : Do not use `any` type without justification in TypeScript code
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.{ts,tsx} : Avoid using any type without justification; all props, return types, and generics must be properly typed
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Use strict TypeScript mode with no `any` types without strong justification - use `unknown` instead for generic type defaults
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using proper validation
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : MCP bridge hooks (useMcpBridge, useCallTool, useToolInput, useToolOutput, useTheme) must be properly typed and handle loading/error states
Applied to files:
plugins/plugin-remember/src/approval/approval-check.hook.ts
🧬 Code graph analysis (3)
apps/e2e/demo-e2e-remember/jest.e2e.config.ts (1)
libs/sdk/src/common/utils/decide-request-intent.utils.ts (1)
Config(58-60)
plugins/plugin-remember/src/providers/remember-memory.provider.ts (1)
plugins/plugin-remember/src/index.ts (1)
RememberStoreInterface(80-80)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
plugins/plugin-remember/src/remember.types.ts (2)
RedisRememberPluginOptions(154-162)RedisClientRememberPluginOptions(167-170)
🪛 ast-grep (0.40.3)
plugins/plugin-remember/src/providers/remember-memory.provider.ts
[warning] 210-210: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escaped}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (21)
apps/e2e/demo-e2e-remember/jest.e2e.config.ts (3)
1-2: LGTM! Proper type imports for Jest ESM configuration.The imports correctly establish type safety for the Jest config and provide the ESM/CommonJS interop utility.
7-42: LGTM! Properly configured E2E test suite with past issue resolved.The configuration is correctly typed and follows standard E2E testing patterns:
- Serial test execution (
maxWorkers: 1) prevents race conditions- Reasonable timeout (60s) for E2E scenarios
- Proper TypeScript transformation with decorator support
- Module mappings correctly reference source files for internal packages
The previous review concern about redundant
@frontmcp/plugin-remembermapping has been successfully addressed—the moduleNameMapper now includes only the necessary@frontmcp/pluginsbarrel export (line 38), consistent with the other 17 E2E configs in the codebase.
44-44: LGTM! Standard default export.The configuration is correctly exported as the default module export.
plugins/plugin-remember/src/approval/approval-check.hook.ts (5)
1-20: LGTM: Clean plugin setup.The imports, decorator, and class declaration follow SDK patterns correctly with proper typing.
24-91: Excellent: All approval flow logic is correct and type-safe.The main hook properly validates approval requirements, checks state, and handles all edge cases. The introduction of
getStringExtra(lines 155-159) has resolved the previous type safety concerns for userId extraction.✅ Addressed past review comment: unsafe type assertions on userId/sub extraction.
96-109: Perfect: Default value handling fixed.The spread operator now correctly appears before applying defaults with the nullish coalescing operator. This ensures that explicit
undefinedvalues in the config won't override the computed defaults.✅ Addressed past review comment: spread operator override issue.
155-159: LGTM: Clean type-safe helper.This helper method provides proper runtime validation for extracting string values from extra metadata, eliminating the need for unsafe type assertions.
164-205: Excellent: Expiry logic corrected.The
handleApprovalRequiredmethod now correctly uses theisExpiredhelper (line 189) to determine if an existing approval has expired, rather than checking for anEXPIREDstate. This properly handles approvals with stateAPPROVEDbutexpiresAtin the past.✅ Addressed past review comment: incorrect expiry check in error state determination.
plugins/plugin-remember/src/providers/remember-memory.provider.ts (11)
1-14: LGTM! Clean imports and well-typed internal interface.The Entry interface is properly structured with appropriate types for the in-memory storage implementation.
16-16: LGTM! Appropriate constant for Node.js setTimeout limit.The MAX_TIMEOUT_MS value correctly reflects Node.js's setTimeout limitation.
22-35: LGTM! Proper provider setup with background sweeper.The constructor correctly initializes the periodic sweeper and uses
unref()to prevent blocking process exit. The type assertion forunrefis acceptable given Node.js runtime behavior.
41-69: LGTM! Robust setValue implementation with proper TTL handling.The method correctly:
- Serializes values consistently via JSON.stringify
- Cleans up previous timeouts before overwriting
- Bounds-checks TTL against Node.js setTimeout limits
- Uses unref() to prevent blocking process exit
75-91: LGTM! Type-safe getValue with proper defaults.The method correctly uses
unknownas the default generic type parameter (per coding guidelines) and includes robust error handling for expired entries and malformed data.
96-100: LGTM! Clean delete implementation.Properly cleans up both the timeout and the memory entry.
105-113: LGTM! Proper expiration handling.Correctly delegates to the public
delete()method for expired entries, maintaining consistency.
119-150: LGTM! Expired entry cleanup now uses public delete method.The two-pass approach properly addresses the previous review feedback:
- First pass collects expired keys
- Deletes them using the public
await this.delete(key)method (lines 131-132)- Second pass filters and returns matching keys
This ensures timeout cleanup happens consistently through the public API.
155-161: LGTM! Comprehensive resource cleanup.The close method properly releases all resources (sweeper, timeouts, and memory).
167-169: LGTM! Simple and correct expiration check.
184-212: LGTM! Robust ReDoS protections in place.The
patternToRegexmethod includes comprehensive protections against Regular Expression Denial of Service:
- Pattern length limit (200 chars)
- Wildcard count limit (20 wildcards)
- Proper escaping of special regex characters
The static analysis warning is a false positive given these safeguards. The past review comment has been properly addressed.
plugins/plugin-remember/src/providers/remember-redis.provider.ts (2)
21-24: Past review issue properly addressed.The
ownsClientflag correctly tracks whether this provider created the Redis client internally, addressing the previous concern about closing externally-provided clients. The implementation distinguishes ownership based on the initialization mode.
43-48: Past DEBUG check issue resolved.The DEBUG environment variable check now uses strict equality (
=== 'true'), correctly addressing the previous concern about loose truthy checks treating values like"false"or"0"as true.
…detailed documentation
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @plugins/plugin-remember/src/providers/remember-redis.provider.ts:
- Around line 62-71: In setValue, validate inputs: in the setValue method check
for value === undefined and reject/throw or handle explicitly (do not pass
JSON.stringify(undefined) to Redis); convert non-string values with
JSON.stringify and ensure the result is a string before calling this.client.set
(use fullKey). Also validate ttlSeconds when present: require it to be a
positive integer (>0) (or coerce/round and reject non-numeric/<=0 values) before
choosing the 'EX' branch, and throw or return an error for invalid TTLs so
this.client.set is never called with a bad ttlSeconds.
🧹 Nitpick comments (1)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
114-134: Consider adding safety limits for large key sets.The SCAN-based implementation correctly uses non-blocking iteration and properly strips the prefix from results. However, with millions of matching keys, this method could take considerable time and accumulate large arrays in memory.
Consider adding an optional
maxKeysparameter or documenting the potential performance implications for large key sets in the JSDoc.🔎 Optional enhancement with maxKeys limit
/** * List keys matching a pattern. * Uses Redis SCAN for efficient iteration. + * @param pattern - Optional pattern to match keys (default: '*') + * @param maxKeys - Optional limit on number of keys to return */ - async keys(pattern?: string): Promise<string[]> { + async keys(pattern?: string, maxKeys?: number): Promise<string[]> { const searchPattern = this.keyPrefix + (pattern ?? '*'); const result: string[] = []; let cursor = '0'; do { const [nextCursor, keys] = await this.client.scan(cursor, 'MATCH', searchPattern, 'COUNT', 100); cursor = nextCursor; // Strip prefix from keys for (const key of keys) { result.push(key.slice(this.keyPrefix.length)); + if (maxKeys && result.length >= maxKeys) { + return result; + } } } while (cursor !== '0'); return result; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/sdk/src/prompt/flows/__tests__/get-prompt.flow.test.tsplugins/plugin-remember/src/providers/remember-memory.provider.tsplugins/plugin-remember/src/providers/remember-redis.provider.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/prompt/flows/__tests__/get-prompt.flow.test.tsplugins/plugin-remember/src/providers/remember-redis.provider.tsplugins/plugin-remember/src/providers/remember-memory.provider.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including error cases and constructor validation
Include error classinstanceofchecks in tests to validate error handling
Files:
libs/sdk/src/prompt/flows/__tests__/get-prompt.flow.test.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/prompt/flows/__tests__/get-prompt.flow.test.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/prompt/flows/__tests__/get-prompt.flow.test.ts
🧠 Learnings (2)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
plugins/plugin-remember/src/providers/remember-redis.provider.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
plugins/plugin-remember/src/providers/remember-redis.provider.ts
🧬 Code graph analysis (1)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
plugins/plugin-remember/src/remember.types.ts (2)
RedisRememberPluginOptions(154-162)RedisClientRememberPluginOptions(167-170)
🪛 ast-grep (0.40.3)
plugins/plugin-remember/src/providers/remember-memory.provider.ts
[warning] 211-211: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escaped}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (18)
libs/sdk/src/prompt/flows/__tests__/get-prompt.flow.test.ts (1)
280-281: LGTM! The assertion update appropriately accommodates context augmentation.The change from
toEqualtotoMatchObjectcorrectly allows for additional internal properties that may be injected by FlowContextProviders while still validating thatauthInfois passed through correctly. The explanatory comment on line 280 makes the intent clear.plugins/plugin-remember/src/providers/remember-memory.provider.ts (10)
1-14: LGTM: Clean type definitions.The Entry interface is well-structured with appropriate optional fields for TTL management and timeout tracking.
16-16: LGTM: Correct Node.js timeout limit.The constant correctly defines the maximum safe value for setTimeout in Node.js.
22-35: LGTM: Proper initialization with defensive unref() pattern.The constructor correctly initializes the periodic sweeper and uses unref() to prevent keeping the event loop alive, with appropriate defensive typing for cross-environment compatibility.
41-69: LGTM: Robust setValue implementation with proper TTL handling.The method correctly handles:
- Consistent JSON serialization for round-trip behavior
- Timeout cleanup for existing entries to prevent leaks
- Node.js setTimeout limit by only scheduling timers within the 24.8-day window
- Unreferencing timeouts to avoid keeping the process alive
75-91: LGTM: Correct getValue implementation with proper expiration handling.The method appropriately:
- Uses the public delete() method for expired entries (cleanup consistency)
- Deserializes JSON values to match setValue serialization
- Provides a defensive fallback for legacy or malformed data
96-100: LGTM: Proper delete implementation.The method correctly clears any associated timeout before removing the entry, preventing timer leaks.
105-113: LGTM: Correct exists implementation.The method properly checks expiration and uses the public delete() method for cleanup consistency.
119-150: LGTM: Properly addresses previous review concern.The two-pass approach correctly:
- Collects expired keys without modifying the map during iteration
- Deletes expired keys using the public method for timeout cleanup
- Filters remaining keys by pattern
The slight inefficiency of iterating twice is an acceptable tradeoff for correctness and safety.
155-161: LGTM: Comprehensive cleanup in close().The method properly releases all resources: sweeper interval, per-entry timeouts, and the memory map itself.
167-213: LGTM: Private methods are correct and ReDoS-protected.The private methods are well-implemented:
- isExpired: Simple and correct expiration check
- sweep: Batch cleanup with proper timeout clearing (direct
memory.delete()is acceptable here since timeouts are already cleared)- patternToRegex: Properly addresses the previous ReDoS concern with appropriate safeguards:
- Pattern length limit (500 chars) accommodates long scoped keys
- Wildcard count limit (20) prevents catastrophic backtracking
- Correct escaping and conversion of glob patterns
The static analysis warning at line 211 is a false positive given the protection validations at lines 198-206.
plugins/plugin-remember/src/providers/remember-redis.provider.ts (7)
1-9: LGTM! Clean imports and type definitions.The imports are well-organized, and the
RedisRememberOptionstype alias clearly combines the two configuration modes. The type aliasing ofRedis as RedisClientprovides good clarity.
11-29: LGTM! Well-structured class with proper ownership tracking.The provider decorator is correctly configured with GLOBAL scope, and the class properties are appropriately typed with readonly modifiers. The
ownsClientflag correctly addresses the previous concern about closing externally-provided clients.
30-57: Constructor handles initialization well, but consider async connection verification.The constructor properly manages client ownership and event handlers. The strict DEBUG check correctly addresses the previous review feedback. However, with
lazyConnect: false, connection errors may not surface until the first operation rather than during construction.Consider whether the current behavior is acceptable for your use case, or if you need to await the connection and handle errors during initialization.
73-96: LGTM! Type-safe implementation with proper error handling.The method correctly addresses the previous review feedback by returning
defaultValueinstead of performing an unsafe cast when JSON parsing fails. The generic type parameter withunknowndefault follows the coding guidelines, and the JSDoc clearly documents the fallback behavior.
98-104: LGTM! Clean delete implementation.The method correctly handles key prefixing and performs the delete operation. The implementation is straightforward and correct.
106-112: LGTM! Correct existence check.The method properly prefixes the key and correctly converts Redis's numeric EXISTS result to a boolean.
136-145: LGTM! Correct ownership-aware cleanup.The method correctly addresses the previous review feedback by only closing the Redis connection when the provider owns the client. This prevents unexpected closure of externally-provided shared connections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (1)
121-157: Correct implementations with production-ready SCAN usage.All three methods are properly implemented:
- The
deleteandexistsmethods are straightforward and correct- The
keysmethod usesSCANinstead ofKEYS, which is the recommended approach for production environments to avoid blocking the Redis server- The cursor-based iteration correctly continues until completion
- Key prefix stripping logic handles all cases correctly, including empty prefixes
The SCAN
COUNTparameter is hardcoded to 100 (line 147). For most use cases this is fine, but if you anticipate very large key sets, consider making it configurable in future iterations:async keys(pattern?: string, scanCount = 100): Promise<string[]> { // ... const [nextCursor, keys] = await this.client.scan( cursor, 'MATCH', searchPattern, 'COUNT', scanCount ); // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/plugin-remember/src/providers/remember-redis.provider.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
plugins/plugin-remember/src/providers/remember-redis.provider.ts
🧠 Learnings (2)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
plugins/plugin-remember/src/providers/remember-redis.provider.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
plugins/plugin-remember/src/providers/remember-redis.provider.ts
🧬 Code graph analysis (1)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (5)
plugins/plugin-remember/src/remember.types.ts (2)
RedisRememberPluginOptions(154-162)RedisClientRememberPluginOptions(167-170)plugins/plugin-remember/src/index.ts (1)
RememberStoreInterface(80-80)libs/uipack/src/handlebars/index.ts (1)
defaultValue(424-424)scripts/apply-doc-patches.mjs (1)
raw(71-71)scripts/bump-synchronized-versions.mjs (1)
result(130-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (5)
plugins/plugin-remember/src/providers/remember-redis.provider.ts (5)
1-28: Excellent setup and type safety!The imports, type definitions, and field declarations are well-structured:
- The
ownsClientfield properly tracks client ownership for lifecycle management- All fields are
readonlyfor immutability- The union type cleanly supports both internal and external Redis clients
- The
@Providerdecorator is correctly configured withGLOBALscope for distributed storage
30-57: Constructor properly addresses all previous concerns.The dual-mode initialization is correctly implemented:
- Client ownership tracking prevents closing externally-provided clients (✅ past issue resolved)
- The DEBUG check now uses strict equality to avoid truthy string issues (✅ past issue resolved)
- Config spread pattern allows users to override defaults while maintaining sensible fallbacks
- Event handlers are intentionally only attached to internally-created clients, leaving external client lifecycle management to the caller
59-94: Validation is thorough and properly addresses previous issues.The setValue method now includes comprehensive input validation:
- Properly rejects
undefinedvalues with a helpful error message (✅ past issue resolved)- Extensive TTL validation covers all edge cases including non-numbers, non-finite values (Infinity), negative values, and non-integers (✅ past issue resolved)
- Error messages are descriptive and include actual values for easier debugging
- The implementation correctly handles both TTL and non-TTL storage paths
96-119: Safe error handling and proper type safety.The getValue method correctly addresses the previous type safety concern:
- Returns
defaultValuefor both missing keys and unparseable values (✅ past issue resolved)- No unsafe casts that could violate type contracts
- The JSDoc clearly documents the fallback behavior
- The
T = unknowntype parameter default follows coding guidelinesThe type assertion
as Ton line 114 is standard practice for JSON deserialization and is acceptable without runtime schema validation.
159-168: Lifecycle management correctly respects client ownership.The close method properly addresses the previous concern:
- Only closes connections that were created internally by checking
ownsClient(✅ past issue resolved)- Externally-provided clients are left open for the caller to manage
- The JSDoc clearly documents this ownership-based behavior
This is the correct pattern for resource management when supporting both internal and external client instances.
There was a problem hiding this comment.
Actionable comments posted: 12
Fix all issues with AI Agents 🤖
In
@apps/e2e/demo-e2e-transport-recreation/src/apps/transport-test/tools/increment-counter.tool.ts:
- Around line 28-29: The comment misstates guarantees about sessionId; update
the inline comment near the call to tryGetContext() so it accurately reflects
that tryGetContext() may return undefined and that FrontMcpContext.sessionId
itself can be undefined in some modes (hence the use of ctx?.sessionId), or
explicitly state that the context may be undefined while sessionId is always set
when a context exists—referencing tryGetContext(), FrontMcpContext.sessionId,
and authInfo.sessionId to make the distinction clear.
In @libs/sdk/src/agent/__tests__/agent.registry.test.ts:
- Around line 365-366: Add focused unit tests for the agentToolName sanitization
and collision behavior: create a new test (or new file) targeting the
agentToolName function that asserts inputs with special characters, consecutive
underscores, and leading/trailing underscores produce the expected sanitized
output (prefixed with "invoke_") and that names normalize consistently; also add
a test that simulates two tools resolving to the same sanitized name and asserts
the collision warning is emitted (spy on the logger/warn used by the registry).
Update or extend libs/sdk/src/agent/__tests__/agent.registry.test.ts (or add
agentToolName.test.ts) to cover these cases and verify the toolDef?.name
expectation still equals 'invoke_tool-agent' for the normal case.
In @libs/sdk/src/common/types/options/transport.options.ts:
- Around line 254-282: The serverless auto-detection in isDistributedMode
currently misses Netlify and Cloudflare Pages; update the enabled === 'auto'
branch to also check process.env for NETLIFY and CF_PAGES alongside the existing
keys. Specifically, inside isDistributedMode inspect const env = typeof process
!== 'undefined' ? process.env : {}; and add env['NETLIFY'] and env['CF_PAGES']
to the OR list so Netlify (NETLIFY="true") and Cloudflare Pages are detected;
leave other checks (VERCEL, AWS_LAMBDA_FUNCTION_NAME, K_SERVICE, etc.)
unchanged.
In @libs/utils/src/storage/adapters/redis.ts:
- Around line 345-363: buildRedisOptions currently returns minimal options when
this.options.url is set, so the Redis URL never reaches the client; update
buildRedisOptions to include the URL in the returned RedisOptions (e.g., include
a url: this.options.url property alongside lazyConnect/maxRetriesPerRequest) or
otherwise parse the URL into host/port/password fields, and ensure the Redis
client instantiation in connect() (where new RedisClass(redisOptions) is called)
receives that URL-containing options object so the client connects using the
provided URL.
- Around line 148-170: The doSet method is missing ensureConnected(), uses a
non-null assertion on this.client and an any cast for set; fix by calling
this.ensureConnected() at the start of doSet, validate that this.client is
defined and throw or return a clear error if not, and replace the (this.client
as any).set invocation with the properly typed Redis client call (use the Redis
client interface already used elsewhere—e.g., this.client.set(prefixedKey,
value, ...flags) or this.client.set(prefixedKey, value, { EX: ttlSeconds, NX/XX
flags } depending on client API) so no non-null assertions or any casts are
needed; keep building prefixedKey and option handling as-is but pass options
using the typed client method rather than casting to any.
In @libs/utils/src/storage/adapters/upstash.ts:
- Around line 244-254: The loop's cursor parsing when calling this.client!.scan
can yield NaN (parseInt(nextCursor, 10)), causing an infinite loop; modify the
cursor assignment in the do/while to defensively convert nextCursor to a numeric
cursor (e.g., use Number(nextCursor) or parseInt and then check isNaN) and if
the conversion yields NaN fallback to 0 so the while condition can terminate;
keep the rest of the logic (pushing this.unprefixKey(key) into result) unchanged
and ensure the variable used in the while is the sanitized numeric cursor.
- Around line 286-361: The publish/subscribe mismatch: publish() uses native
PUBLISH while subscribe() polls a list queue, so update publish(channel,
message) to push messages to the same queue subscribe() polls (use the same
prefixedChannel and listKey `${prefixedChannel}:queue`) instead of calling
this.client.publish; either call the existing publishToQueue(...) helper if it
exists (or implement an rpush/lpush to that list via this.client) and return the
pushed message count, keeping the same pubSubEnabled short-circuit and error
semantics.
In @libs/utils/src/storage/factory.ts:
- Around line 75-76: Replace the generic Error thrown in the createAdapter
switch branch for 'auto' with the project's StorageConfigError to keep error
types consistent; update the throw in the case 'auto' inside createAdapter (or
the surrounding factory function) to instantiate StorageConfigError with the
same message and ensure StorageConfigError is imported or referenced the same
way it's used at the later throw site.
In @libs/utils/src/storage/utils/pattern.ts:
- Around line 154-156: The issue: escapeGlob currently outputs backslash-escaped
wildcards (e.g., "user\\*") but globToRegex treats the backslash as a regex char
and then converts '*' into a wildcard, producing incorrect regexes; fix by
updating globToRegex to recognize backslash-escaped wildcards and treat them as
literal characters. Modify globToRegex to first parse sequences like "\\*" and
"\\?" (and "\\\\" for a literal backslash) and convert those into escaped regex
literals (e.g., match '*' or '?', not wildcard behavior), then continue
converting unescaped '*' and '?' into ".*" and "." respectively and escape other
regex metacharacters; ensure both escapeGlob and globToRegex consistently handle
backslashes so escaped patterns from escapeGlob produce literal matches.
In @plugins/plugin-remember/src/approval/approval-storage.store.ts:
- Around line 491-509: The docstring for createApprovalMemoryStore is misleading
because the created ApprovalStorageStore has initialized = false and callers
must call initialize() (or ensureInitialized() will throw); update the example
and description to state that initialize() must be invoked (e.g., replace the
comment with "await store.initialize(); // Now ready to use") and/or add a
sentence that createApprovalMemoryStore returns an uninitialized
ApprovalStorageStore requiring a call to ApprovalStorageStore.initialize()
before use, referencing createApprovalMemoryStore,
ApprovalStorageStore.initialize, and ensureInitialized to make the requirement
clear.
In @plugins/plugin-remember/src/providers/remember-storage.provider.ts:
- Around line 249-257: The JSDoc for createRememberMemoryProvider is incorrect:
although the example claims "No need to call initialize() - storage is already
connected", the provider's public methods (setValue, getValue, delete, exists,
keys) call ensureInitialized() and will throw unless initialize() is awaited;
update the documentation/example for createRememberMemoryProvider to require
calling await provider.initialize() before any use (mirror the initialize usage
shown in the other class examples) and clarify that createRememberMemoryProvider
returns an uninitialized RememberStorageProvider instance that must be
initialized prior to calling its methods.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/provider/provider.registry.ts (1)
635-654: UsegetProviderScope()for consistent scope normalization.Line 647 accesses
def.metadata.scopedirectly and defaults toProviderScope.GLOBAL, but this bypasses the scope normalization logic ingetProviderScope()(lines 265-278) that converts deprecatedSESSION/REQUESTscopes toCONTEXT.🔎 Proposed fix
mergeFromRegistry( providedBy: ProviderRegistry, exported: { token: Token<ProviderInterface>; def: ProviderRecord; /** Instance may be undefined for CONTEXT-scoped providers (built per-request) */ instance: ProviderEntry | undefined; }[], ) { for (const { token, def, instance } of exported) { - // Use default GLOBAL scope when scope is not explicitly set (undefined) - // This matches the behavior in getProviderScope() and resolveFromViews() - const scope = def.metadata.scope ?? ProviderScope.GLOBAL; + // Use getProviderScope() to ensure consistent scope normalization + // (SESSION/REQUEST → CONTEXT, and undefined → GLOBAL) + const scope = this.getProviderScope(def); if (scope === ProviderScope.GLOBAL && instance !== undefined) { this.instances.set(token, instance); } this.defs.set(token, def); this.providedBy.set(token, providedBy); } }
♻️ Duplicate comments (1)
libs/uipack/package.json (1)
62-62: Verify that enclave-vm@2.2.0 exists and check for breaking changes.A previous review flagged
enclave-vmas non-existent on npm. Before merging, confirm that:
- The package
enclave-vmexists on npm (or is properly configured as a private package)- Version 2.2.0 is available
- There are no breaking changes or security issues between 2.0.0 and 2.2.0
If the package doesn't exist, this will cause installation failures.
Does the npm package enclave-vm exist and what is the latest version available?enclave-vm npm 2.2.0 breaking changes security vulnerabilities
🧹 Nitpick comments (21)
plugins/plugin-remember/src/approval/approval-storage.store.ts (4)
47-55:normalizeRevokeris defined but never used.The
normalizeRevokerhelper is not called anywhere in this file. TherevokeApprovalmethod doesn't utilize therevokedByfield fromRevokeApprovalOptions, suggesting incomplete implementation or dead code.If revocation tracking is intended, consider recording the revoker before deletion. Otherwise, remove this unused function.
128-134: Avoid non-null assertion onstorageproperty.Per coding guidelines, non-null assertions (
!) should be avoided in favor of proper error handling. WhileensureInitialized()guards against uninitialized usage, the typing could be more explicit.🔎 Proposed fix
- private storage!: NamespacedStorage; + private storage: NamespacedStorage | undefined;Then update
ensureInitialized()to return the storage with proper narrowing:- private ensureInitialized(): void { + private ensureInitialized(): NamespacedStorage { if (!this.initialized) { throw new Error('ApprovalStorageStore not initialized. Call initialize() first.'); } + // TypeScript will narrow this after the check above + return this.storage!; }Usage would become
const storage = this.ensureInitialized();which makes the guard explicit at each call site.
198-205: Consider runtime validation for parsed records.The
as ApprovalRecordcast trusts that stored JSON matches the expected structure. Corrupted or externally-modified storage data could cause subtle runtime errors.For production storage backends (Redis, etc.), consider adding lightweight validation using Zod or a manual check of required fields.
401-425: Potential performance issue with large datasets.
clearExpiredApprovalsfetches all keys and values into memory before filtering. For production backends with many approval records, this could cause memory pressure and latency spikes during the cleanup interval.Consider:
- Using cursor-based iteration (SCAN for Redis) to process keys in batches
- Relying more heavily on the TTL set during
grantApproval(line 321) for automatic expiration, reducing the need for manual cleanuplibs/utils/src/storage/__tests__/factory.test.ts (2)
88-99: Fallback test doesn't exercise actual fallback behavior.This test sets
fallback: 'memory'but since no distributed backend is configured (no env vars), it will use memory storage directly rather than falling back after a connection failure. The test name suggests it should verify fallback on connection failure, but it doesn't actually trigger that scenario.Consider adding a test that configures a distributed backend (e.g., invalid Redis URL) and verifies fallback behavior when the connection actually fails.
🔎 Example test for actual fallback behavior
it('should fallback to memory on connection failure in non-production', async () => { // Configure invalid Redis to trigger connection failure process.env['REDIS_URL'] = 'redis://invalid-host:9999'; const storage = await createStorage({ type: 'auto', fallback: 'memory', }); // Should succeed using memory fallback expect(await storage.ping()).toBe(true); await storage.disconnect(); });
6-100: Missing error case tests per coding guidelines.The test suite covers happy paths well but lacks error case coverage. Per coding guidelines: "Test all code paths including error cases and constructor validation" and "Include error class
instanceofchecks in tests."Consider adding tests for:
- Invalid storage type
- Configuration errors (missing required options for specific backends)
- Connection timeout/failure scenarios
libs/utils/src/storage/__tests__/ttl.test.ts (1)
94-98: Minor timing race in test assertion.The test calls
Date.now()before and afterttlToExpiresAt, but then callsDate.now()again in the assertion at line 97. In slow CI environments, this could cause flakiness.🔎 Suggested fix
it('should handle small TTL values', () => { + const before = Date.now(); const expiresAt = ttlToExpiresAt(1); - expect(expiresAt).toBeGreaterThan(Date.now()); - expect(expiresAt).toBeLessThanOrEqual(Date.now() + 1000 + 10); // +10ms buffer + const after = Date.now(); + expect(expiresAt).toBeGreaterThanOrEqual(before + 1000); + expect(expiresAt).toBeLessThanOrEqual(after + 1000 + 10); // +10ms buffer });libs/utils/src/storage/__tests__/namespace.test.ts (1)
4-12: Unused import:NamespacedStorageImpl.
NamespacedStorageImplis imported but never used in the tests. The tests use the factory functions (createRootStorage,createNamespacedStorage) instead.🔎 Remove unused import
import { MemoryStorageAdapter } from '../adapters/memory'; import { - NamespacedStorageImpl, buildPrefix, createRootStorage, createNamespacedStorage, NAMESPACE_SEPARATOR, } from '../namespace';libs/utils/src/storage/adapters/vercel-kv.ts (3)
12-12: Unused import:globToRegex.
globToRegexis imported but never used in this file. Thekeys()method usesscanwith the pattern directly rather than converting to regex.🔎 Remove unused import
import { validateTTL } from '../utils/ttl'; -import { globToRegex } from '../utils/pattern';
109-112: Non-null assertions on options after validation.Per coding guidelines: "Avoid non-null assertions (
!) - use proper error handling." While the constructor validates thaturlandtokenexist, the non-null assertions at lines 110-111 could be avoided by narrowing the type.🔎 Type-safe alternative
+ private readonly url: string; + private readonly token: string; + constructor(options: VercelKvAdapterOptions = {}) { super(); // Get URL and token from options or environment const url = options.url ?? process.env['KV_REST_API_URL']; const token = options.token ?? process.env['KV_REST_API_TOKEN']; if (!url || !token) { throw new StorageConfigError( 'vercel-kv', 'KV_REST_API_URL and KV_REST_API_TOKEN must be provided via options or environment variables.', ); } - this.options = { ...options, url, token }; + this.url = url; + this.token = token; + this.options = { ...options, url, token }; this.keyPrefix = options.keyPrefix ?? ''; }Then use
this.urlandthis.tokendirectly:} else { this.client = createClient({ - url: this.options.url!, - token: this.options.token!, + url: this.url, + token: this.token, }); }
222-250:keys()silently falls back on scan failure.The empty catch block at line 245 silently swallows all errors from
scanand falls back tokeys. This could mask real issues (e.g., authentication errors, rate limiting) that would also affect the fallback.Consider logging a warning or only catching specific "scan not supported" errors.
🔎 Proposed improvement
} catch (e) { // Fallback to keys command (may be slow) + // Only fallback for specific errors, re-throw unexpected ones + if (e instanceof Error && !e.message.includes('SCAN')) { + throw new StorageOperationError('keys', e.message, e); + } const allKeys = await this.client!.keys(prefixedPattern); return allKeys.map((k) => this.unprefixKey(k)); }plugins/plugin-remember/src/providers/remember-storage.provider.ts (1)
89-89: Avoid definite assignment assertion - use proper error handling instead.The
!operator bypasses TypeScript's safety checks. Consider makingstoragenullable (private storage?: NamespacedStorage) and relying onensureInitialized()to throw a clear error if accessed before initialization.🔎 Proposed refactor
- private storage!: NamespacedStorage; + private storage?: NamespacedStorage;Then update methods to handle the nullable type:
private ensureInitialized(): void { if (!this.initialized) { throw new Error('RememberStorageProvider not initialized. Call initialize() first.'); } + if (!this.storage) { + throw new Error('RememberStorageProvider storage not configured. Call initialize() first.'); + } }Based on coding guidelines: Avoid non-null assertions (
!) - use proper error handling instead.libs/utils/src/storage/adapters/base.ts (1)
183-185: Consider using a specific error class for validation errors.Line 184 throws a generic
Errorfor mutually exclusive options, while the rest of the storage system uses specific error classes. For consistency and better error handling, consider creating aStorageValidationErroror using an existing appropriate error class.💡 Suggested approach
If a
StorageValidationErrordoesn't exist yet, you could add it tolibs/utils/src/storage/errors.ts:export class StorageValidationError extends StorageError { constructor(message: string) { super(message); this.name = 'StorageValidationError'; } }Then use it here:
// Check mutually exclusive flags if (options.ifNotExists && options.ifExists) { - throw new Error('ifNotExists and ifExists are mutually exclusive'); + throw new StorageValidationError('ifNotExists and ifExists are mutually exclusive'); }libs/utils/src/storage/adapters/redis.ts (2)
195-224:anycast in mset pipeline violates coding guidelines.Line 220 uses
(pipeline as any).set(...)which violates the strict TypeScript guidelines. Consider using a properly typed approach or documenting why the cast is necessary.🔎 Proposed fix
- (pipeline as any).set(...args); + // ioredis pipeline.set accepts variadic args for SET command options + (pipeline.set as (...args: (string | number)[]) => unknown)(...args);
380-394: Swallowed errors in message handler reduce observability.The catch block on line 388-390 silently ignores handler errors. Consider at least logging the error for debugging purposes, as completely swallowing errors can make issues difficult to diagnose.
libs/utils/src/storage/adapters/memory.ts (3)
251-270: Modifying Map during iteration may cause unexpected behavior.In the
keys()method, callingthis.deleteEntry(key)on line 260 while iterating overthis.storemodifies the Map during iteration. While JavaScript's Map iteration is generally safe when deleting the current key, this can still lead to subtle bugs or skipped entries in some edge cases.🔎 Proposed fix - collect expired keys first, then delete
async keys(pattern: string = '*'): Promise<string[]> { this.ensureConnected(); const regex = globToRegex(pattern); const result: string[] = []; + const expiredKeys: string[] = []; for (const [key, entry] of this.store) { // Skip expired entries if (isExpired(entry.expiresAt)) { - this.deleteEntry(key); + expiredKeys.push(key); continue; } if (regex.test(key)) { result.push(key); } } + // Clean up expired entries after iteration + for (const key of expiredKeys) { + this.deleteEntry(key); + } return result; }
393-401: LRU tracking with array has O(n) complexity per access.The
touchLRUmethod usesindexOfandspliceon an array, both of which are O(n) operations. For large caches with frequent access, this could become a performance bottleneck.For a development/testing adapter this is acceptable, but if used with significant
maxEntries, consider using a more efficient data structure like a doubly-linked list with a Map for O(1) operations.
331-336: Pub/sub publish implementation has subtle evaluation order.The comma operator on line 334 works but is non-obvious. The expression
(this.emitter.emit(...), this.emitter.listenerCount(channel))emits first, then returns the listener count. Consider splitting for clarity.🔎 Proposed fix for readability
override async publish(channel: string, message: string): Promise<number> { this.ensureConnected(); - return this.emitter.listenerCount(channel) > 0 - ? (this.emitter.emit(channel, message, channel), this.emitter.listenerCount(channel)) - : 0; + const listenerCount = this.emitter.listenerCount(channel); + if (listenerCount > 0) { + this.emitter.emit(channel, message, channel); + } + return listenerCount; }libs/utils/src/storage/adapters/upstash.ts (2)
163-167: MissingensureConnected()in doSet method.Similar to the Redis adapter, the
doSetmethod doesn't callensureConnected(). While this may be called by the base class, adding it here ensures defensive programming.🔎 Proposed fix
protected async doSet(key: string, value: string, options?: SetOptions): Promise<void> { + this.ensureConnected(); const prefixedKey = this.prefixKey(key);
315-334: Polling interval processes only one message per cycle.The polling loop (lines 315-334) only pops one message per interval with
rpop. If messages arrive faster than the polling rate (100ms), the queue will grow unbounded. Consider processing all available messages in each poll cycle.🔎 Proposed fix - drain queue per poll
const interval = setInterval(async () => { try { - // Pop messages from the list - const message = await this.client!.rpop(listKey); - if (message) { + // Drain all available messages from the list + let message: string | null; + while ((message = await this.client!.rpop(listKey)) !== null) { const handlers = this.subscriptionHandlers.get(prefixedChannel); if (handlers) { for (const h of handlers) { try { h(message, channel); } catch { // Ignore handler errors } } } } } catch { // Ignore polling errors } }, PUBSUB_POLL_INTERVAL_MS);libs/sdk/src/__test-utils__/mocks/prompt-registry.mock.ts (1)
171-189: Refine mock buildViews return structure.The mock
buildViewsimplementation reusescontextDepsfor thesessionandrequestmaps (lines 176-177). This may not accurately represent real behavior where these scopes have distinct lifecycles. Consider returning distinct empty maps forsessionandrequest, reservingcontextDepsonly for thecontextmap.🔎 Proposed refinement
providers: { buildViews: jest.fn(async (_sessionKey: string, contextDeps?: Map<unknown, unknown>) => ({ global: new Map(), context: contextDeps ?? new Map(), - session: contextDeps ?? new Map(), - request: contextDeps ?? new Map(), + session: new Map(), + request: new Map(), })),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (43)
apps/e2e/demo-e2e-transport-recreation/src/apps/transport-test/tools/increment-counter.tool.tslibs/sdk/src/__test-utils__/mocks/prompt-registry.mock.tslibs/sdk/src/agent/__tests__/agent.registry.test.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/entries/prompt.entry.tslibs/sdk/src/common/entries/tool.entry.tslibs/sdk/src/common/types/options/transport.options.tslibs/sdk/src/prompt/flows/get-prompt.flow.tslibs/sdk/src/prompt/prompt.instance.tslibs/sdk/src/provider/__tests__/provider-session-scope.test.tslibs/sdk/src/provider/provider.registry.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/tool/tool.instance.tslibs/uipack/package.jsonlibs/utils/src/index.tslibs/utils/src/storage/__tests__/errors.test.tslibs/utils/src/storage/__tests__/factory.test.tslibs/utils/src/storage/__tests__/memory-adapter.test.tslibs/utils/src/storage/__tests__/namespace.test.tslibs/utils/src/storage/__tests__/pattern.test.tslibs/utils/src/storage/__tests__/ttl.test.tslibs/utils/src/storage/adapters/base.tslibs/utils/src/storage/adapters/index.tslibs/utils/src/storage/adapters/memory.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/storage/adapters/upstash.tslibs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/storage/errors.tslibs/utils/src/storage/factory.tslibs/utils/src/storage/index.tslibs/utils/src/storage/namespace.tslibs/utils/src/storage/types.tslibs/utils/src/storage/utils/index.tslibs/utils/src/storage/utils/pattern.tslibs/utils/src/storage/utils/ttl.tspackage.jsonplugins/plugin-codecall/package.jsonplugins/plugin-remember/src/approval/approval-storage.store.tsplugins/plugin-remember/src/approval/index.tsplugins/plugin-remember/src/providers/index.tsplugins/plugin-remember/src/providers/remember-storage.provider.ts
✅ Files skipped from review due to trivial changes (2)
- libs/utils/src/storage/tests/pattern.test.ts
- libs/utils/src/storage/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/sdk/src/common/entries/tool.entry.ts
- plugins/plugin-codecall/package.json
- package.json
- libs/sdk/src/tool/tool.instance.ts
- plugins/plugin-remember/src/approval/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/utils/src/storage/adapters/index.tslibs/utils/src/storage/__tests__/factory.test.tslibs/utils/src/storage/__tests__/ttl.test.tslibs/utils/src/storage/__tests__/namespace.test.tsplugins/plugin-remember/src/providers/index.tsapps/e2e/demo-e2e-transport-recreation/src/apps/transport-test/tools/increment-counter.tool.tsplugins/plugin-remember/src/approval/approval-storage.store.tsplugins/plugin-remember/src/providers/remember-storage.provider.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/utils/src/storage/utils/pattern.tslibs/sdk/src/prompt/prompt.instance.tslibs/sdk/src/provider/__tests__/provider-session-scope.test.tslibs/utils/src/storage/utils/ttl.tslibs/sdk/src/common/entries/prompt.entry.tslibs/utils/src/storage/errors.tslibs/utils/src/storage/factory.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/__test-utils__/mocks/prompt-registry.mock.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/types/options/transport.options.tslibs/sdk/src/agent/__tests__/agent.registry.test.tslibs/utils/src/storage/__tests__/memory-adapter.test.tslibs/sdk/src/prompt/flows/get-prompt.flow.tslibs/utils/src/storage/index.tslibs/sdk/src/agent/agent.registry.tslibs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/storage/__tests__/errors.test.tslibs/utils/src/storage/namespace.tslibs/utils/src/index.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/storage/adapters/memory.tslibs/utils/src/storage/types.tslibs/utils/src/storage/adapters/base.tslibs/sdk/src/provider/provider.registry.tslibs/utils/src/storage/adapters/upstash.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/utils/src/storage/adapters/index.tslibs/utils/src/storage/__tests__/factory.test.tslibs/uipack/package.jsonlibs/utils/src/storage/__tests__/ttl.test.tslibs/utils/src/storage/__tests__/namespace.test.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/utils/src/storage/utils/pattern.tslibs/sdk/src/prompt/prompt.instance.tslibs/sdk/src/provider/__tests__/provider-session-scope.test.tslibs/utils/src/storage/utils/ttl.tslibs/sdk/src/common/entries/prompt.entry.tslibs/utils/src/storage/errors.tslibs/utils/src/storage/factory.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/__test-utils__/mocks/prompt-registry.mock.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/types/options/transport.options.tslibs/sdk/src/agent/__tests__/agent.registry.test.tslibs/utils/src/storage/__tests__/memory-adapter.test.tslibs/sdk/src/prompt/flows/get-prompt.flow.tslibs/utils/src/storage/index.tslibs/sdk/src/agent/agent.registry.tslibs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/storage/__tests__/errors.test.tslibs/utils/src/storage/namespace.tslibs/utils/src/index.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/storage/adapters/memory.tslibs/utils/src/storage/types.tslibs/utils/src/storage/adapters/base.tslibs/sdk/src/provider/provider.registry.tslibs/utils/src/storage/adapters/upstash.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including error cases and constructor validation
Include error classinstanceofchecks in tests to validate error handling
Files:
libs/utils/src/storage/__tests__/factory.test.tslibs/utils/src/storage/__tests__/ttl.test.tslibs/utils/src/storage/__tests__/namespace.test.tslibs/sdk/src/provider/__tests__/provider-session-scope.test.tslibs/sdk/src/agent/__tests__/agent.registry.test.tslibs/utils/src/storage/__tests__/memory-adapter.test.tslibs/utils/src/storage/__tests__/errors.test.ts
libs/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Each library in /libs/* is independent and publishable under @frontmcp/* scope
Files:
libs/uipack/package.json
libs/uipack/**/{package.json,*.ts,*.tsx,*.js,*.jsx}
📄 CodeRabbit inference engine (libs/uipack/CLAUDE.md)
Do not add React dependencies to @frontmcp/uipack - it must remain React-free. Use @frontmcp/ui for React components.
Files:
libs/uipack/package.json
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/prompt/prompt.instance.tslibs/sdk/src/provider/__tests__/provider-session-scope.test.tslibs/sdk/src/common/entries/prompt.entry.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/__test-utils__/mocks/prompt-registry.mock.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/types/options/transport.options.tslibs/sdk/src/agent/__tests__/agent.registry.test.tslibs/sdk/src/prompt/flows/get-prompt.flow.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/provider/provider.registry.ts
🧠 Learnings (21)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
libs/utils/src/storage/adapters/index.tsplugins/plugin-remember/src/providers/index.tslibs/utils/src/storage/index.tslibs/utils/src/index.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/index.{ts,js} : Export all public APIs through appropriate entry points (frontmcp/uipack, frontmcp/uipack/adapters, frontmcp/uipack/theme, etc.)
Applied to files:
libs/utils/src/storage/adapters/index.tslibs/uipack/package.jsonplugins/plugin-remember/src/providers/index.tslibs/utils/src/storage/index.tslibs/utils/src/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/common/records/**/*.ts : Centralize record types in common/records directory and import from there instead of module-specific files
Applied to files:
libs/utils/src/storage/adapters/index.tslibs/utils/src/storage/index.tslibs/utils/src/index.tslibs/utils/src/storage/types.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/utils/src/storage/__tests__/factory.test.tslibs/utils/src/storage/__tests__/namespace.test.tslibs/utils/src/storage/__tests__/memory-adapter.test.tslibs/utils/src/storage/__tests__/errors.test.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and constructor validation
Applied to files:
libs/utils/src/storage/__tests__/factory.test.tslibs/utils/src/storage/__tests__/ttl.test.tslibs/utils/src/storage/__tests__/namespace.test.tslibs/utils/src/storage/__tests__/errors.test.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{package.json,*.ts,*.tsx,*.js,*.jsx} : Do not add React dependencies to frontmcp/uipack - it must remain React-free. Use frontmcp/ui for React components.
Applied to files:
libs/uipack/package.json
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/package.json : The frontmcp/ui package requires React as a peer dependency (^18.0.0 || ^19.0.0)
Applied to files:
libs/uipack/package.json
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/*/package.json : Each library in /libs/* is independent and publishable under frontmcp/* scope
Applied to files:
libs/uipack/package.json
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/package.json : Entry points must match the documented paths: frontmcp/ui/react, frontmcp/ui/renderers, frontmcp/ui/render, frontmcp/ui/universal, frontmcp/ui/bundler, frontmcp/ui/bridge, frontmcp/ui/components, frontmcp/ui/layouts, frontmcp/ui/web-components
Applied to files:
libs/uipack/package.json
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/bundler/**/*.{ts,tsx} : The bundler module must re-export utilities from frontmcp/uipack/bundler and provide SSR component bundling functionality
Applied to files:
libs/uipack/package.jsonlibs/utils/src/index.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.{ts,tsx} : Never import React-free utilities from frontmcp/ui; use frontmcp/uipack for bundling, build tools, platform adapters, and theme utilities
Applied to files:
libs/uipack/package.json
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Use React Testing Library for component tests and include SSR/hydration tests for all interactive components
Applied to files:
libs/utils/src/storage/__tests__/ttl.test.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/*.{test,spec}.{ts,tsx,js,jsx} : Every component and utility must test invalid inputs and edge cases
Applied to files:
libs/utils/src/storage/__tests__/ttl.test.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : MCP bridge hooks (useMcpBridge, useCallTool, useToolInput, useToolOutput, useTheme) must be properly typed and handle loading/error states
Applied to files:
libs/sdk/src/tool/flows/call-tool.flow.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using proper validation
Applied to files:
libs/utils/src/storage/utils/pattern.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `changeScope` property instead of `scope` in change events to avoid confusion with Scope class
Applied to files:
libs/sdk/src/provider/__tests__/provider-session-scope.test.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/agent/agent.registry.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific error classes with MCP error codes instead of generic errors
Applied to files:
libs/utils/src/storage/errors.tslibs/utils/src/storage/__tests__/errors.test.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Organize code following the frontmcp/uipack directory structure (adapters/, bundler/, theme/, renderers/, validation/, etc.)
Applied to files:
libs/utils/src/storage/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
libs/utils/src/storage/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Include error class `instanceof` checks in tests to validate error handling
Applied to files:
libs/utils/src/storage/__tests__/errors.test.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{src}/**/*.{ts,tsx,js,jsx} : Do not expose internal error details in public APIs - use sanitized error messages
Applied to files:
libs/utils/src/storage/__tests__/errors.test.ts
🧬 Code graph analysis (11)
libs/utils/src/storage/__tests__/namespace.test.ts (2)
libs/utils/src/storage/types.ts (1)
NamespacedStorage(322-344)libs/utils/src/storage/namespace.ts (4)
createRootStorage(224-226)buildPrefix(28-33)createNamespacedStorage(235-239)keys(164-169)
libs/sdk/src/tool/flows/call-tool.flow.ts (4)
libs/sdk/src/provider/flow-context-providers.ts (1)
FlowContextProviders(23-53)libs/sdk/src/common/interfaces/flow.interface.ts (1)
context(93-96)libs/sdk/src/common/interfaces/tool.interface.ts (2)
input(68-70)input(72-79)libs/sdk/src/common/interfaces/agent.interface.ts (2)
input(333-335)input(337-344)
libs/utils/src/storage/utils/pattern.ts (1)
libs/utils/src/storage/errors.ts (1)
StoragePatternError(133-138)
libs/sdk/src/prompt/prompt.instance.ts (5)
libs/sdk/src/provider/provider.registry.ts (1)
ProviderRegistry(44-1078)libs/sdk/src/scope/scope.instance.ts (2)
Scope(37-384)providers(325-327)libs/sdk/src/tool/tool.instance.ts (1)
providers(111-113)libs/sdk/src/common/entries/prompt.entry.ts (1)
PromptGetExtra(13-20)libs/sdk/src/common/interfaces/prompt.interface.ts (1)
scope(84-86)
libs/utils/src/storage/errors.ts (7)
libs/utils/src/index.ts (8)
StorageError(93-93)StorageConnectionError(94-94)StorageOperationError(95-95)StorageNotSupportedError(96-96)StorageConfigError(97-97)StorageTTLError(98-98)StoragePatternError(99-99)StorageNotConnectedError(100-100)libs/utils/src/storage/index.ts (8)
StorageError(39-39)StorageConnectionError(40-40)StorageOperationError(41-41)StorageNotSupportedError(42-42)StorageConfigError(43-43)StorageTTLError(44-44)StoragePatternError(45-45)StorageNotConnectedError(46-46)libs/utils/src/storage/adapters/memory.ts (1)
ttl(229-245)libs/utils/src/storage/adapters/redis.ts (1)
ttl(244-250)libs/utils/src/storage/adapters/upstash.ts (1)
ttl(226-231)libs/utils/src/storage/adapters/vercel-kv.ts (1)
ttl(210-216)libs/utils/src/storage/namespace.ts (1)
ttl(156-158)
libs/utils/src/storage/__tests__/memory-adapter.test.ts (3)
libs/utils/src/storage/adapters/memory.ts (3)
MemoryStorageAdapter(56-477)ttl(229-245)keys(251-270)libs/utils/src/storage/errors.ts (2)
StorageOperationError(60-65)StorageNotConnectedError(148-153)libs/utils/src/storage/adapters/base.ts (1)
count(120-123)
libs/sdk/src/prompt/flows/get-prompt.flow.ts (2)
libs/sdk/src/provider/flow-context-providers.ts (1)
FlowContextProviders(23-53)libs/sdk/src/common/interfaces/flow.interface.ts (1)
context(93-96)
libs/utils/src/storage/namespace.ts (2)
libs/utils/src/index.ts (11)
NAMESPACE_SEPARATOR(91-91)buildPrefix(90-90)NamespacedStorageImpl(87-87)NamespacedStorage(74-74)StorageAdapter(73-73)SetOptions(76-76)SetEntry(77-77)MessageHandler(78-78)Unsubscribe(79-79)createRootStorage(88-88)createNamespacedStorage(89-89)libs/utils/src/storage/types.ts (6)
NamespacedStorage(322-344)StorageAdapter(79-296)SetOptions(15-33)SetEntry(38-42)MessageHandler(47-47)Unsubscribe(52-52)
libs/utils/src/storage/adapters/memory.ts (4)
libs/utils/src/storage/types.ts (2)
MemoryAdapterOptions(359-377)SetOptions(15-33)libs/utils/src/storage/utils/ttl.ts (4)
isExpired(82-85)ttlToExpiresAt(61-63)validateTTL(21-41)expiresAtToTTL(71-74)libs/utils/src/storage/utils/pattern.ts (1)
globToRegex(49-101)libs/utils/src/storage/errors.ts (1)
StorageOperationError(60-65)
libs/utils/src/storage/types.ts (5)
libs/utils/src/storage/adapters/memory.ts (1)
keys(251-270)libs/utils/src/storage/adapters/redis.ts (1)
keys(256-274)libs/utils/src/storage/adapters/upstash.ts (1)
keys(237-257)libs/utils/src/storage/adapters/vercel-kv.ts (1)
keys(222-250)libs/utils/src/storage/namespace.ts (1)
keys(164-169)
libs/sdk/src/provider/provider.registry.ts (3)
libs/sdk/src/common/types/options/transport.options.ts (2)
DistributedConfigInput(249-252)shouldCacheProviders(287-297)libs/sdk/src/common/interfaces/execution-context.interface.ts (1)
scope(134-136)libs/sdk/src/context/frontmcp-context.ts (1)
scope(306-308)
🪛 ast-grep (0.40.3)
libs/utils/src/storage/utils/pattern.ts
[warning] 96-96: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(regexStr)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Libraries
apps/e2e/demo-e2e-transport-recreation/src/apps/transport-test/tools/increment-counter.tool.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 15
Fix all issues with AI Agents 🤖
In @CLAUDE.md:
- Around line 288-321: Update the documentation path reference: replace the old
example file path string
"libs/plugins/src/remember/remember.context-extension.ts" with the new location
"plugins/plugin-remember/src/remember.context-extension.ts" so the README points
to the correct plugin file for the context-extension example.
In @docs/draft/docs/plugins/overview.mdx:
- Around line 33-45: The docs incorrectly claim "per-entry key derivation";
update the Card text to reflect the real implementation by stating that
encryption uses AES-256-GCM with HKDF-SHA256 scope-based key derivation and a
unique random 12-byte IV per entry — mention deriveEncryptionKey() as the
function that derives a single key per scope (session/user/tool/global) and
clarify that entry uniqueness is provided by the IV rather than per-entry keys.
In @libs/plugins/src/index.ts:
- Around line 1-17: The comment "// Remember Plugin" is misleading because it
sits above the cache export; fix by either removing that lone comment or
replacing it with accurate per-export comments: add "// Cache Plugin" above
export * from '@frontmcp/plugin-cache'; "// Codecall Plugin" above export * from
'@frontmcp/plugin-codecall'; "// Dashboard Plugin" above export * from
'@frontmcp/plugin-dashboard'; and "// Remember Plugin" above export * from
'@frontmcp/plugin-remember'; ensure the comments align with the corresponding
export statements (export * from '@frontmcp/plugin-cache', export * from
'@frontmcp/plugin-codecall', export * from '@frontmcp/plugin-dashboard', export
* from '@frontmcp/plugin-remember').
In @libs/sdk/src/common/types/options/transport.options.ts:
- Around line 254-284: The isDistributedMode function is incorrectly treating
GOOGLE_CLOUD_PROJECT as a serverless indicator; remove the GOOGLE_CLOUD_PROJECT
environment check from the 'auto' detection block so only specific serverless
indicators (e.g., K_SERVICE for Cloud Run) remain; update the boolean expression
in isDistributedMode to exclude env['GOOGLE_CLOUD_PROJECT'] and keep the rest of
the checks unchanged.
In
@libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts:
- Around line 286-296: Update the "should not save when persistence disabled"
test to assert the specific error type when access(secretPath) rejects: replace
the generic rejects.toThrow() with an instanceof-style assertion (e.g., await
expect(access(secretPath)).rejects.toBeInstanceOf(Error)) and optionally also
assert the errno/code (e.g., error.code === 'ENOENT') to ensure the missing-file
error is validated; locate this change in the test block that calls
saveSecret(...) and then access(secretPath).
In @libs/utils/src/crypto/secret-persistence/schema.ts:
- Around line 13-17: The Zod schema secretDataSchema allows extra properties
because it lacks .strict(); update the declaration of secretDataSchema (the
z.object({...}) expression) to call .strict() on the object schema so only the
defined keys (secret, createdAt, version) are accepted during validation.
In @libs/utils/src/storage/adapters/redis.ts:
- Around line 152-175: The problem is the use of an unsafe any cast and non-null
assertion when calling Redis in doSet; instead ensure the class's client is
typed as the proper ioredis Redis type (e.g., import and use Redis from
'ioredis' so client: Redis | null), keep the ensureConnected() check to
guarantee it's non-null, then call this.client.set(...args) directly (no any
cast) — adjust the declaration of this.client and its usages (including get,
ensureConnected, and doSet) to use the Redis type and proper null handling so
TypeScript accepts the variadic set call without any.
- Around line 200-229: The mset method uses an any cast on pipeline when calling
(pipeline as any).set(...args); remove the any by using the Redis pipeline's
typed set command or by invoking pipeline.set as pipeline.set(prefixedKey,
entry.value, ...(entry.options?.ttlSeconds ? ['EX', entry.options.ttlSeconds] :
[]), entry.options?.ifNotExists ? 'NX' : entry.options?.ifExists ? 'XX' :
undefined) while preserving types—locate the mset function, the pipeline
variable, and how args is built, then replace the dynamic args array + any cast
with a properly typed call to pipeline.set (or pipeline.call if using ioredis
CommandType) so TypeScript knows the parameter types and no any cast is used.
In @libs/utils/src/storage/adapters/upstash.ts:
- Around line 244-257: The cursor parsing in the do/while loop that handles
this.client!.scan can produce NaN when doing parseInt(nextCursor, 10), which
would make the loop never exit; change the assignment so you parse the string
into a number and guard against NaN (e.g., let parsed = typeof nextCursor ===
'string' ? parseInt(nextCursor, 10) : nextCursor; if (Number.isNaN(parsed))
parsed = 0; cursor = parsed;), ensuring the loop will terminate; update the
block around the scan call/variables (nextCursor, keys, cursor) accordingly.
- Around line 169-183: The doSet method uses this.client! without ensuring the
adapter is connected; add an await this.ensureConnected() as the first line
inside protected async doSet(key: string, value: string, options?: SetOptions)
so the client is initialized before use, then keep the existing prefixKey,
setOptions construction, and the final await this.client!.set(...) call
unchanged.
In @plugins/plugin-approval/jest.config.ts:
- Around line 41-48: The jest coverageThreshold currently sets
branches/functions/lines/statements to 80% but the project standard and the AI
summary expect 95%; update the coverageThreshold.global values for branches,
functions, lines, and statements to 95 and also adjust any AI-generated summary
text that claims 95% so it matches the actual values (search for the
coverageThreshold object and the AI summary string and make them consistent).
- Line 5: The jest.config.ts transformIgnorePatterns excludes jose,
@noble/hashes, and @noble/ciphers but those packages are not declared in
plugins/plugin-approval's package.json; either add the missing packages to
plugins/plugin-approval/package.json dependencies (so Jest will legitimately
need to transform them) or remove those packages from the
transformIgnorePatterns array in jest.config.ts if the plugin does not use them;
update package.json by adding the exact package names and appropriate versions
used in the workspace and run yarn/npm install, or edit the
transformIgnorePatterns line in jest.config.ts to drop the unnecessary
exclusions.
In @plugins/plugin-approval/project.json:
- Around line 28-31: The esbuildOptions.external list includes
"reflect-metadata" but package.json dependencies lack it; add "reflect-metadata"
to the package.json "dependencies" section (not devDependencies or
peerDependencies) so it will be bundled/resolved at runtime, then install/update
lockfile (npm/yarn/pnpm) to ensure the dependency is present.
In @plugins/plugin-approval/src/approval/guards.d.ts:
- Around line 1-5: Update the module documentation in the JSDoc at the top of
this file: change the @module tag value from "@frontmcp/utils/approval" to the
correct package name "@frontmcp/plugin-approval" so the documented module path
matches the actual plugin module.
In @plugins/plugin-approval/src/approval/index.d.ts:
- Around line 7-18: Update the JSDoc module tag and the example import to
reference the correct package: change the @module tag from
"@frontmcp/utils/approval" to "@frontmcp/plugin-approval" and update the example
import source from "@frontmcp/utils" to "@frontmcp/plugin-approval" so the
listed symbols (ApprovalScope, ApprovalState, userGrantor, testGrantor,
isHumanGrantor, approvalRecordSchema) correctly reference this package.
🧹 Nitpick comments (20)
libs/utils/src/storage/factory.ts (1)
7-11: Unused import:StorageConnectionError.
StorageConnectionErroris imported but never used in this file. The connection errors are re-thrown from the adapters themselves.🔎 Proposed fix
-import { StorageConfigError, StorageConnectionError } from './errors'; +import { StorageConfigError } from './errors';libs/utils/src/storage/adapters/redis.ts (1)
385-398: Consider logging handler errors instead of silently ignoring.Line 393-395 catches and ignores handler errors. While preventing one handler from affecting others is correct, silently swallowing errors makes debugging difficult.
🔎 Proposed fix
try { handler(message, unprefixedChannel); } catch { - // Ignore handler errors + // Log handler errors but don't propagate + console.warn('[redis] Pub/sub handler error:', e instanceof Error ? e.message : String(e)); }libs/utils/src/storage/adapters/upstash.ts (1)
318-337: Polling processes only one message per cycle - potential message backlog.The polling loop uses
rpopto get a single message per 100ms interval. If messages arrive faster than 10/second, they'll accumulate in the queue. Consider processing all available messages in each poll cycle.🔎 Proposed fix
const interval = setInterval(async () => { try { - // Pop messages from the list - const message = await this.client!.rpop(listKey); - if (message) { + // Pop all available messages from the list + let message: string | null; + while ((message = await this.client!.rpop(listKey)) !== null) { const handlers = this.subscriptionHandlers.get(prefixedChannel); if (handlers) { for (const h of handlers) { try { h(message, channel); } catch { // Ignore handler errors } } } } } catch { // Ignore polling errors } }, PUBSUB_POLL_INTERVAL_MS);libs/sdk/src/common/types/options/transport.options.ts (1)
249-252: Consider derivingDistributedConfigInputfrom the schema.The type is manually defined but could be derived from the Zod schema for better maintainability. If the schema changes, the type would automatically stay in sync.
🔎 Suggested approach
Extract the distributed schema and derive the input type:
// After transportPersistenceConfigSchema definition const distributedConfigSchema = z.object({ enabled: z.union([z.boolean(), z.literal('auto')]).default(false), providerCaching: z.boolean().optional(), }); // Then in transportOptionsSchema: distributed: distributedConfigSchema.optional(), // Then for type export: export type DistributedConfigInput = z.input<typeof distributedConfigSchema>;This ensures the type automatically reflects any schema changes.
plugins/plugin-approval/src/services/approval.service.ts (1)
131-133: Consider using a specific error class instead of generic Error.Throwing a generic
Errormakes it harder for callers to handle this specific case programmatically. Consider creating or using a specific error class (e.g.,ApprovalErrororMissingUserIdError) that can be caught and handled appropriately.🔎 Proposed refactor using a specific error
If an
ApprovalErrorclass exists or is created in the approval/errors module:if (!this.userId) { - throw new Error('Cannot grant user approval without userId'); + throw new ApprovalError('Cannot grant user approval without userId', { + code: 'MISSING_USER_ID', + scope: ApprovalScope.USER, + }); }plugins/plugin-approval/src/approval/__tests__/approval.test.ts (1)
1-513: Excellent test coverage for the approval module!This test suite comprehensively covers:
- All enum values
- All 9 grantor factories with various configurations
- All 5 revoker factories
- Normalization functions with different input types
- All 7 guard functions
- Schema validations with positive and negative cases
- All 6 error classes including properties, messages, and JSON-RPC conversions
- Proper
instanceofchecks (line 420)The test structure is well-organized and follows testing best practices.
💡 Optional: Consider additional schema edge case tests
While the current test coverage is strong, you could enhance schema tests with more edge cases:
- Boundary values for numeric fields (zero, MAX_SAFE_INTEGER)
- Empty arrays vs undefined for array fields
- Null vs undefined for optional fields
- Unicode/special characters in string fields
- Very long strings for identifier/displayName fields
For example:
describe('approvalRecordSchema edge cases', () => { it('should handle very long toolId', () => { const record = { toolId: 'a'.repeat(1000), state: ApprovalState.APPROVED, scope: ApprovalScope.SESSION, grantedAt: Date.now(), grantedBy: { source: 'user' }, }; const result = approvalRecordSchema.safeParse(record); // Define expected behavior }); it('should handle empty approvalChain array', () => { const record = { toolId: 'file_write', state: ApprovalState.APPROVED, scope: ApprovalScope.SESSION, grantedAt: Date.now(), grantedBy: { source: 'user' }, approvalChain: [], }; const result = approvalRecordSchema.safeParse(record); // Define expected behavior }); });This would bring the coverage even closer to the 95%+ target across all edge cases.
libs/utils/src/crypto/pkce/__tests__/pkce.test.ts (1)
177-185: Test conditional could skip assertion in edge case.The conditional
if (challenge !== uppercaseChallenge)could theoretically skip the assertion if the generated challenge happens to be all uppercase (extremely unlikely but not impossible). Consider using a deterministic verifier for this test to guarantee the assertion runs.🔎 Suggested improvement
it('should be case-sensitive', () => { - const verifier = generateCodeVerifier(); - const challenge = generateCodeChallenge(verifier); - const uppercaseChallenge = challenge.toUpperCase(); - // If challenge was already uppercase or lowercase, skip - if (challenge !== uppercaseChallenge) { - expect(verifyCodeChallenge(verifier, uppercaseChallenge)).toBe(false); - } + // Use RFC test vector to ensure deterministic challenge with mixed case + const verifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + const challenge = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'; + const uppercaseChallenge = challenge.toUpperCase(); + expect(verifyCodeChallenge(verifier, uppercaseChallenge)).toBe(false); });libs/utils/src/crypto/pkce/pkce.ts (1)
110-113: Consider timing-safe comparison for challenge verification.The current implementation uses simple string equality (
===), which could theoretically leak timing information about how many characters match. While PKCE challenges are typically single-use and this is a low-risk scenario, using constant-time comparison would be a defense-in-depth measure.🔎 Suggested improvement
+import { timingSafeEqual } from '../'; + +const textEncoder = new TextEncoder(); + export function verifyCodeChallenge(codeVerifier: string, codeChallenge: string): boolean { const computedChallenge = generateCodeChallenge(codeVerifier); - return computedChallenge === codeChallenge; + // Use timing-safe comparison to prevent timing attacks + if (computedChallenge.length !== codeChallenge.length) { + return false; + } + const a = textEncoder.encode(computedChallenge); + const b = textEncoder.encode(codeChallenge); + return timingSafeEqual(a, b); }plugins/plugin-approval/src/approval.plugin.ts (1)
228-235: Consider type narrowing for userId extraction.The userId extraction accesses
authInfo?.extra?.['userId']with a type assertion. While the optional chaining prevents runtime errors, theas string | undefinedassertion relies on the stored value being a string.🔎 Suggested improvement for safer type handling
useFactory: (store, ctx: FrontMcpContext) => { - const userId = - (ctx.authInfo?.extra?.['userId'] as string | undefined) ?? - (ctx.authInfo?.extra?.['sub'] as string | undefined) ?? - ctx.authInfo?.clientId; + const rawUserId = ctx.authInfo?.extra?.['userId'] ?? ctx.authInfo?.extra?.['sub']; + const userId = typeof rawUserId === 'string' ? rawUserId : ctx.authInfo?.clientId; return createApprovalService(store, ctx.sessionId, userId); },libs/utils/src/fs/fs.ts (1)
17-42: Non-null assertions in lazy-load helpers.Lines 23, 32, and 42 use non-null assertions (
!) after the assignment. While these are safe because the variable is assigned within theifblock, the coding guidelines advise avoiding non-null assertions. Consider restructuring to avoid them.🔎 Suggested refactor to avoid non-null assertions
function getFs(): typeof import('fs') { if (!_fs) { assertNode('File system operations'); - _fs = require('fs'); } - return _fs!; + // TypeScript can't infer _fs is non-null after assignment in if block + // but assertNode throws if not Node, so this is safe + return _fs as typeof import('fs'); } function getFsp(): typeof import('fs').promises { if (!_fsp) { assertNode('File system operations'); - _fsp = require('fs').promises; } - return _fsp!; + return _fsp as typeof import('fs').promises; } function getSpawn(): typeof import('child_process').spawn { if (!_spawn) { assertNode('Child process operations'); - _spawn = require('child_process').spawn; } - return _spawn!; + return _spawn as typeof import('child_process').spawn; }Note: This is a minor stylistic concern. The
ascast is semantically equivalent but aligns better with the guideline to avoid!. Based on coding guidelines, non-null assertions should be avoided.plugins/plugin-approval/src/hooks/approval-check.hook.ts (1)
59-60: Consider adding a null check after resolving ApprovalStoreToken.If the
ApprovalStoreTokenis not registered in the container,this.get()may returnundefined. Theas ApprovalStorecast bypasses this check.- const approvalStore = this.get(ApprovalStoreToken) as ApprovalStore; + const approvalStore = this.get(ApprovalStoreToken); + if (!approvalStore) { + throw new Error('ApprovalStore not registered. Ensure ApprovalPlugin is loaded.'); + } const approval = await approvalStore.getApproval(tool.fullName, sessionId, userId);plugins/plugin-approval/src/services/challenge.service.ts (2)
94-100: Non-null assertion onstorageviolates coding guidelines.Per coding guidelines, avoid non-null assertions (
!). WhileensureInitialized()guards access, the assertion could be replaced with a safer pattern.🔎 Suggested approach
- private storage!: NamespacedStorage; + private storage: NamespacedStorage | undefined; private readonly options: Required<Omit<ChallengeServiceOptions, 'storageInstance'>> & { storageInstance?: RootStorage | NamespacedStorage; };Then in
ensureInitialized():private ensureInitialized(): void { - if (!this.initialized) { + if (!this.initialized || !this.storage) { throw new Error('ChallengeService not initialized. Call initialize() first.'); } }Or keep the assertion but add a runtime check in
initialize()to guarantee assignment.
180-198: Add validation after JSON.parse to prevent corrupted data issues.The
JSON.parse(recordJson) as ChallengeRecordcast trusts that storage contains valid data. If storage is corrupted or tampered with, this could cause downstream errors.🔎 Consider adding schema validation
const parsed = JSON.parse(recordJson); // Validate required fields exist if ( typeof parsed !== 'object' || parsed === null || typeof parsed.toolId !== 'string' || typeof parsed.sessionId !== 'string' || typeof parsed.expiresAt !== 'number' ) { await this.storage.delete(codeChallenge); throw new ChallengeValidationError('invalid', 'Corrupted challenge record'); } const record = parsed as ChallengeRecord;Alternatively, use the Zod schema if
ChallengeRecordhas one defined.plugins/plugin-approval/src/approval/errors.ts (2)
15-20: Consider capturing the stack trace for proper inheritance.The base
ApprovalErrorclass setsthis.namebut doesn't callError.captureStackTrace(Node.js) or setObject.setPrototypeOf. In some environments,instanceofchecks or stack traces may not work as expected.🔎 Proposed enhancement
export class ApprovalError extends Error { constructor(message: string) { super(message); this.name = 'ApprovalError'; + Object.setPrototypeOf(this, new.target.prototype); } }
74-86: Consider includingreasonin JSON-RPC error data for debugging.The
reasonis captured in the constructor but excluded fromtoJsonRpcError()data. While this may be intentional for security (not exposing internal details), it could hinder debugging. If the reason is safe to expose, consider adding it:🔎 Proposed fix
toJsonRpcError() { return { code: -32603, // Internal Error message: 'Approval operation failed', data: { type: 'approval_operation_error', operation: this.operation, + reason: this.reason, }, }; }Based on learnings, if
reasonmay contain sensitive internal details, the current approach of excluding it is correct.plugins/plugin-approval/src/stores/approval-storage.store.ts (3)
71-77: Non-null assertion onstoragecould be avoided.Per coding guidelines, avoid non-null assertions (
!). WhileensureInitialized()guards access, consider using a getter that throws if uninitialized for stronger guarantees:🔎 Proposed refactor
- private storage!: NamespacedStorage; + private _storage: NamespacedStorage | undefined; + + private get storage(): NamespacedStorage { + if (!this._storage) { + throw new Error('ApprovalStorageStore not initialized. Call initialize() first.'); + } + return this._storage; + }Then update
initialize()to setthis._storageinstead ofthis.storage.As per coding guidelines, this would eliminate the non-null assertion while maintaining the same runtime behavior.
164-202: Potential performance concern with large datasets.
queryApprovalsfetches all keys matching the pattern and loads all values into memory for filtering. With many approvals, this could cause memory pressure.Consider documenting this limitation or implementing pagination for production use cases with high approval volumes.
290-311: Redundant expiry cleanup when storage TTL is configured.
clearExpiredApprovalsscans all keys periodically, butgrantApprovalalready setsttlSecondson storage entries (line 228). Redis/Vercel KV/Upstash will auto-expire these entries.This method is only useful for memory storage or when TTL wasn't set. Consider skipping this scan when using backends with native TTL support, or documenting that it's primarily for memory storage.
plugins/plugin-approval/src/approval/schemas.ts (1)
32-36: Consider using.strict()for object schemas.The object schemas allow extra properties to pass through silently. While the
.strict()learning specifically applies tolibs/uipack/**, using strict mode here would catch unexpected properties during validation:🔎 Example for approvalContextSchema
export const approvalContextSchema = z.object({ type: z.string().min(1), identifier: z.string().min(1), metadata: z.record(z.string(), z.unknown()).optional(), -}); +}).strict();Apply similarly to other object schemas if stricter validation is desired.
plugins/plugin-approval/src/index.ts (1)
38-42: Dual export pattern may cause confusion.Exporting both
default as ApprovalPluginandApprovalPlugin as ApprovalPluginClassallows two import styles but could confuse consumers. Consider documenting the preferred import pattern in the module docstring.Per coding guidelines about barrel files, ensure the preferred public API is clear.
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts
Show resolved
Hide resolved
… better module clarity
… better module clarity
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @docs/draft/docs/plugins/overview.mdx:
- Line 37: The docs line incorrectly says "per-value key derivation" but the
implementation uses scope-based key derivation; update the phrasing to reflect
that deriveEncryptionKey() derives one key per scope (session/user/tool/global)
and that uniqueness is provided by a random 12-byte IV per entry—e.g., change to
"Encrypted AES-256-GCM storage with HKDF scope-based key derivation" or
"Encrypted AES-256-GCM storage with HKDF-SHA256 scope-based key derivation and
unique IV per entry."
In @libs/utils/src/crypto/secret-persistence/schema.ts:
- Line 15: Replace the loose validation on the schema's secret field (currently
"secret: z.string().min(32)") with base64url-aware validation: use
z.base64url().length(43) (or z.base64url().min(43).max(43) if you prefer
explicit bounds) for the "secret" property in the schema so it enforces a
base64url-encoded 32-byte value (~43 chars) instead of any 32+ char string.
In @libs/utils/src/storage/adapters/redis.ts:
- Around line 152-432: The code uses many non-null assertions (this.client! /
this.subscriber!) which violates guidelines; add two narrower helpers like
private getConnectedClient(): Redis { this.ensureConnected(); if (!this.client)
throw new Error('Redis client not connected'); return this.client; } and private
getConnectedSubscriber(): Redis { if (!this.subscriber) throw new Error('Redis
subscriber not created'); return this.subscriber; } then replace all uses of
this.client! with getConnectedClient() (in get, doSet, delete, exists, mget,
mset pipeline, mdelete, expire, ttl, keys, incr, decr, incrBy, publish, etc.)
and replace subscriber usages in createSubscriber and subscribe/unsubscribe
flows with getConnectedSubscriber(); remove remaining non-null assertions and
ensure createSubscriber still sets this.subscriber before attaching listeners.
In @plugins/plugin-approval/src/stores/approval-storage.store.ts:
- Around line 277-288: clearSessionApprovals currently interpolates sessionId
into the glob pattern `*:session:${sessionId}*` which can allow pattern
metacharacters to match unintended keys; escape the sessionId before building
the pattern (use the same escapePattern helper used in queryApprovals), change
the pattern construction in clearSessionApprovals to something like
`*:session:${escapedSessionId}*`, keep the rest of the logic (keys lookup, early
return, mdelete) the same so only properly escaped session IDs are used when
calling this.storage.keys.
- Around line 129-136: The parseRecord function unsafely casts parsed JSON to
ApprovalRecord; create a Zod schema (e.g., ApprovalRecordSchema) that models
ApprovalRecord (using ApprovalState and ApprovalScope for enums and optional
fields as needed), import z, then in parseRecord replace the plain JSON.parse
cast with JSON.parse followed by ApprovalRecordSchema.parse/ safeParse and
return the validated value only on success (otherwise return undefined); ensure
references to parseRecord, ApprovalRecordSchema, ApprovalRecord, ApprovalState,
and ApprovalScope are used so callers get a properly validated ApprovalRecord.
🧹 Nitpick comments (7)
libs/sdk/src/common/types/options/transport.options.ts (1)
200-207: Clarify that the default is computed at runtime, not by the schema.The
@defaultcomment on Line 205 describes the effective runtime behavior, but the schema itself has no default value (it's just.optional()). The actual defaulting logic is in theshouldCacheProvidersfunction.Consider rephrasing to make this distinction clearer, for example:
/** * Enable provider session caching. * When false, CONTEXT-scoped providers are rebuilt on each request. * This is the recommended setting for serverless/distributed deployments. * * When omitted, defaults to true for traditional deployments and false * when distributed mode is enabled (computed by shouldCacheProviders). */libs/utils/src/storage/adapters/upstash.ts (2)
169-184: Add TTL validation for consistency.While
expire()(line 222) callsvalidateTTL(), this method doesn't validateoptions.ttlSeconds. For consistency and defensive coding, validate the TTL before passing it to the client.🔎 Proposed fix
protected async doSet(key: string, value: string, options?: SetOptions): Promise<void> { this.ensureConnected(); const prefixedKey = this.prefixKey(key); const setOptions: { ex?: number; nx?: boolean; xx?: boolean } = {}; if (options?.ttlSeconds) { + validateTTL(options.ttlSeconds); setOptions.ex = options.ttlSeconds; }
372-378: Remove redundantpublishToQueue()method.This method duplicates the exact logic now implemented in
publish()(lines 297-300). Sincepublish()already uses the list-based approach, this separate method is no longer needed and could confuse users about which method to call.🔎 Proposed fix
- /** - * Publish using list-based approach (for polling subscribers). - * This is an alternative to native PUBLISH when subscribers are polling. - */ - async publishToQueue(channel: string, message: string): Promise<void> { - this.ensureConnected(); - - const prefixedChannel = this.prefixKey(`__pubsub__:${channel}`); - const listKey = `${prefixedChannel}:queue`; - await this.client!.lpush(listKey, message); - } -plugins/plugin-approval/src/stores/approval-storage.store.ts (3)
246-275: Consider refactoring to reduce duplication.The method checks context, session, and user scopes with nearly identical logic blocks. This repetition increases maintenance burden and the risk of inconsistent behavior if one block is updated but not the others.
Consider extracting a helper method to check a single scope, then call it for each scope level.
🔎 Proposed refactor
+private async checkScope(key: string): Promise<boolean> { + const value = await this.storage.get(key); + const approval = this.parseRecord(value); + return approval !== undefined && + approval.state === ApprovalState.APPROVED && + !this.isExpired(approval); +} + async isApproved(toolId: string, sessionId: string, userId?: string, context?: ApprovalContext): Promise<boolean> { this.ensureInitialized(); if (context) { const contextKey = this.buildKey(toolId, sessionId, userId, context); - const contextValue = await this.storage.get(contextKey); - const contextApproval = this.parseRecord(contextValue); - if (contextApproval && contextApproval.state === ApprovalState.APPROVED && !this.isExpired(contextApproval)) { + if (await this.checkScope(contextKey)) { return true; } } const sessionKey = this.buildKey(toolId, sessionId); - const sessionValue = await this.storage.get(sessionKey); - const sessionApproval = this.parseRecord(sessionValue); - if (sessionApproval && sessionApproval.state === ApprovalState.APPROVED && !this.isExpired(sessionApproval)) { + if (await this.checkScope(sessionKey)) { return true; } if (userId) { const userKey = this.buildKey(toolId, undefined, userId); - const userValue = await this.storage.get(userKey); - const userApproval = this.parseRecord(userValue); - if (userApproval && userApproval.state === ApprovalState.APPROVED && !this.isExpired(userApproval)) { + if (await this.checkScope(userKey)) { return true; } } return false; }
290-353: Scalability concern: fetching all keys for cleanup and stats.Both
clearExpiredApprovals()(line 294) andgetStats()(line 335) fetch all keys and values from storage. For production deployments with thousands of approvals, this could cause:
- High memory consumption
- Slow response times
- Potential timeouts or crashes
Consider implementing incremental cleanup strategies or adding pagination/streaming support for large datasets.
Possible strategies:
- Use cursor-based iteration if the storage backend supports it
- Limit cleanup to a batch size per interval (e.g., process max 1000 keys per cleanup cycle)
- Maintain separate indexes or counters for stats instead of scanning all records
- Add a warning in documentation about scalability limits with the current implementation
369-380: Consider documenting initialization requirement.While a previous review comment about misleading documentation was marked as addressed, the current docstring for
createApprovalMemoryStoredoesn't mention that callers must callinitialize()before using the returned store.Consider adding a brief note in the docstring to clarify this requirement, especially since
ensureInitialized()will throw if initialization is skipped.🔎 Suggested documentation improvement
/** * Create an ApprovalStorageStore with synchronous memory storage. * Convenience function for simple use cases. + * + * @example + * ```typescript + * const store = createApprovalMemoryStore(); + * await store.initialize(); + * // Now ready to use + * ``` */libs/utils/src/crypto/secret-persistence/schema.ts (1)
42-79: Refactor to return parsed data from validation.The current design forces
parseSecretDatato use a type assertion (data as SecretData) instead of returning the validated data from Zod's parse result. This bypasses Zod's parsed output and doesn't follow TypeScript best practices.🔎 Proposed refactor
Update the validation result type to include the parsed data:
// In types.ts (assumed) export type SecretValidationResult = | { valid: true; data: SecretData } | { valid: false; error: string };Then refactor
validateSecretData:export function validateSecretData(data: unknown): SecretValidationResult { const result = secretDataSchema.safeParse(data); if (!result.success) { return { valid: false, error: result.error.issues[0]?.message ?? 'Invalid secret structure', }; } const parsed = result.data; const now = Date.now(); // Verify createdAt is not in the future (with drift allowance) if (parsed.createdAt > now + MAX_CLOCK_DRIFT_MS) { return { valid: false, error: 'createdAt is in the future' }; } // Verify not too old if (parsed.createdAt < now - MAX_SECRET_AGE_MS) { return { valid: false, error: 'createdAt is too old' }; } - return { valid: true }; + return { valid: true, data: parsed }; }And simplify
parseSecretData:export function parseSecretData(data: unknown): SecretData | null { const validation = validateSecretData(data); if (!validation.valid) { return null; } - return data as SecretData; + return validation.data; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
CLAUDE.mddocs/draft/docs/plugins/overview.mdxlibs/plugins/src/index.tslibs/sdk/src/common/types/options/transport.options.tslibs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.tslibs/utils/src/crypto/secret-persistence/schema.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/storage/adapters/upstash.tsplugins/plugin-approval/jest.config.tsplugins/plugin-approval/package.jsonplugins/plugin-approval/src/approval.context-extension.tsplugins/plugin-approval/src/approval.plugin.tsplugins/plugin-approval/src/approval/errors.d.tsplugins/plugin-approval/src/approval/factories.d.tsplugins/plugin-approval/src/approval/guards.d.tsplugins/plugin-approval/src/approval/index.d.tsplugins/plugin-approval/src/approval/schemas.d.tsplugins/plugin-approval/src/approval/types.d.tsplugins/plugin-approval/src/stores/approval-storage.store.tsplugins/plugin-cache/src/index.tsplugins/plugin-codecall/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- plugins/plugin-cache/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/plugin-approval/package.json
- plugins/plugin-approval/src/approval.context-extension.ts
- plugins/plugin-approval/src/approval.plugin.ts
- libs/utils/src/crypto/secret-persistence/tests/secret-persistence.test.ts
- libs/plugins/src/index.ts
- plugins/plugin-approval/jest.config.ts
- plugins/plugin-approval/src/approval/guards.d.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
plugins/plugin-codecall/src/index.tslibs/sdk/src/common/types/options/transport.options.tsplugins/plugin-approval/src/approval/types.d.tsplugins/plugin-approval/src/stores/approval-storage.store.tslibs/utils/src/crypto/secret-persistence/schema.tsplugins/plugin-approval/src/approval/errors.d.tslibs/utils/src/storage/adapters/redis.tsplugins/plugin-approval/src/approval/schemas.d.tslibs/utils/src/storage/adapters/upstash.tsplugins/plugin-approval/src/approval/factories.d.tsplugins/plugin-approval/src/approval/index.d.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/common/types/options/transport.options.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/common/types/options/transport.options.tslibs/utils/src/crypto/secret-persistence/schema.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/storage/adapters/upstash.ts
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/plugins/overview.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/plugins/overview.mdx
🧠 Learnings (13)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
plugins/plugin-codecall/src/index.tsplugins/plugin-approval/src/approval/index.d.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/index.{ts,js} : Export all public APIs through appropriate entry points (frontmcp/uipack, frontmcp/uipack/adapters, frontmcp/uipack/theme, etc.)
Applied to files:
plugins/plugin-codecall/src/index.tsplugins/plugin-approval/src/approval/index.d.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/bundler/**/*.{ts,tsx} : The bundler module must re-export utilities from frontmcp/uipack/bundler and provide SSR component bundling functionality
Applied to files:
plugins/plugin-codecall/src/index.ts
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/common/types/options/transport.options.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
plugins/plugin-approval/src/approval/types.d.tslibs/utils/src/crypto/secret-persistence/schema.tsCLAUDE.mdplugins/plugin-approval/src/approval/errors.d.tsplugins/plugin-approval/src/approval/index.d.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
plugins/plugin-approval/src/approval/types.d.tslibs/utils/src/crypto/secret-persistence/schema.tsplugins/plugin-approval/src/approval/errors.d.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{src}/**/*.{ts,tsx} : Do not use `any` type without justification in TypeScript code
Applied to files:
plugins/plugin-approval/src/approval/types.d.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : MCP bridge hooks (useMcpBridge, useCallTool, useToolInput, useToolOutput, useTheme) must be properly typed and handle loading/error states
Applied to files:
plugins/plugin-approval/src/approval/types.d.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{validation,**}/**/*.{ts,tsx} : Use `.strict()` on all Zod schemas for validation
Applied to files:
libs/utils/src/crypto/secret-persistence/schema.tsplugins/plugin-approval/src/approval/schemas.d.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/package.json : Entry points must match the documented paths: frontmcp/ui/react, frontmcp/ui/renderers, frontmcp/ui/render, frontmcp/ui/universal, frontmcp/ui/bundler, frontmcp/ui/bridge, frontmcp/ui/components, frontmcp/ui/layouts, frontmcp/ui/web-components
Applied to files:
CLAUDE.mdplugins/plugin-approval/src/approval/index.d.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `changeScope` property instead of `scope` in change events to avoid confusion with Scope class
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific error classes with MCP error codes instead of generic errors
Applied to files:
plugins/plugin-approval/src/approval/errors.d.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/utils/src/storage/adapters/redis.ts
🧬 Code graph analysis (6)
plugins/plugin-approval/src/stores/approval-storage.store.ts (6)
plugins/plugin-approval/src/stores/index.ts (6)
ApprovalStorageStoreOptions(15-15)ApprovalStorageStore(14-14)ApprovalStore(8-8)ApprovalQuery(9-9)GrantApprovalOptions(10-10)RevokeApprovalOptions(11-11)libs/utils/src/index.ts (5)
StorageConfig(149-149)RootStorage(139-139)NamespacedStorage(138-138)createStorage(133-133)createMemoryStorage(134-134)plugins/plugin-approval/src/stores/approval-store.interface.ts (4)
ApprovalStore(123-177)ApprovalQuery(25-55)GrantApprovalOptions(64-91)RevokeApprovalOptions(96-114)libs/utils/src/storage/factory.ts (2)
createStorage(119-176)createMemoryStorage(195-205)plugins/plugin-approval/src/approval/types.d.ts (2)
ApprovalContext(41-48)ApprovalRecord(163-196)plugins/plugin-approval/src/approval/index.ts (5)
ApprovalContext(45-45)ApprovalRecord(53-53)normalizeGrantor(102-102)ApprovalState(43-43)ApprovalScope(42-42)
plugins/plugin-approval/src/approval/errors.d.ts (4)
plugins/plugin-approval/src/approval/index.d.ts (7)
ApprovalError(103-103)ApprovalRequiredError(104-104)ApprovalScope(39-39)ApprovalOperationError(105-105)ApprovalScopeNotAllowedError(106-106)ApprovalExpiredError(107-107)ChallengeValidationError(108-108)plugins/plugin-approval/src/approval/errors.ts (6)
ApprovalError(15-20)ApprovalRequiredError(25-63)ApprovalOperationError(68-87)ApprovalScopeNotAllowedError(92-115)ApprovalExpiredError(120-140)ChallengeValidationError(145-167)plugins/plugin-approval/src/index.ts (6)
ApprovalError(82-82)ApprovalRequiredError(83-83)ApprovalOperationError(84-84)ApprovalScopeNotAllowedError(85-85)ApprovalExpiredError(86-86)ChallengeValidationError(87-87)plugins/plugin-approval/src/types/approval.types.ts (1)
ApprovalScope(10-10)
libs/utils/src/storage/adapters/redis.ts (4)
libs/utils/src/storage/index.ts (8)
RedisStorageAdapter(53-53)RedisAdapterOptions(18-18)MessageHandler(15-15)StorageConfigError(43-43)StorageConnectionError(40-40)SetOptions(13-13)validateTTL(62-62)Unsubscribe(16-16)libs/utils/src/storage/types.ts (4)
RedisAdapterOptions(382-413)MessageHandler(47-47)SetOptions(15-33)Unsubscribe(52-52)libs/utils/src/storage/errors.ts (2)
StorageConfigError(100-105)StorageConnectionError(40-45)libs/utils/src/storage/utils/ttl.ts (1)
validateTTL(21-41)
plugins/plugin-approval/src/approval/schemas.d.ts (1)
plugins/plugin-approval/src/approval/schemas.ts (19)
approvalScopeSchema(14-14)approvalStateSchema(16-16)approvalMethodSchema(18-18)approvalSourceTypeSchema(20-20)revocationMethodSchema(22-22)approvalCategorySchema(24-24)riskLevelSchema(26-26)approvalContextSchema(32-36)delegationContextSchema(42-47)approvalGrantorSchema(53-60)approvalRevokerSchema(62-67)approvalRecordSchema(73-90)toolApprovalRequirementSchema(96-107)ApprovalContextInput(113-113)DelegationContextInput(114-114)ApprovalGrantorInput(115-115)ApprovalRevokerInput(116-116)ApprovalRecordInput(117-117)ToolApprovalRequirementInput(118-118)
libs/utils/src/storage/adapters/upstash.ts (3)
libs/utils/src/storage/types.ts (4)
UpstashAdapterOptions(443-468)MessageHandler(47-47)SetOptions(15-33)Unsubscribe(52-52)libs/utils/src/storage/errors.ts (2)
StorageConfigError(100-105)StorageConnectionError(40-45)libs/utils/src/storage/utils/ttl.ts (1)
validateTTL(21-41)
plugins/plugin-approval/src/approval/factories.d.ts (2)
plugins/plugin-approval/src/approval/factories.ts (16)
userGrantor(56-68)policyGrantor(83-90)adminGrantor(106-118)systemGrantor(132-138)agentGrantor(158-170)apiGrantor(185-193)oauthGrantor(208-216)testGrantor(234-240)customGrantor(260-278)userRevoker(291-298)adminRevoker(307-314)expiryRevoker(321-326)sessionEndRevoker(334-340)policyRevoker(349-356)normalizeGrantor(378-386)normalizeRevoker(404-412)plugins/plugin-approval/src/approval/types.ts (6)
ApprovalMethod(107-107)ApprovalGrantor(167-185)DelegationContext(117-129)ApprovalRevoker(204-216)ApprovalSourceType(93-102)RevocationSourceType(194-194)
🔇 Additional comments (38)
CLAUDE.md (5)
288-320: ✅ Plugin Development guidance and path correction look solid.The path reference at line 320 has been properly corrected to
plugins/plugin-remember/src/remember.context-extension.ts, and the module augmentation and runtime extension examples are clear and technically sound. This provides good developer guidance for extending execution contexts.
322-339: ✅ Crypto utilities guidance is well-motivated and comprehensive.Emphasizing
@frontmcp/utilsfor all cryptographic operations ensures cross-platform compatibility and consistent behavior. The function list is practical and covers key security primitives (HKDF, AES-GCM, SHA-256, base64url encoding/decoding).
341-375: ✅ File system utilities guidance is comprehensive and well-justified.The rationale for centralizing FS operations through
@frontmcp/utilsis clear (cross-platform consistency, lazy-loading, centralized error handling), and the function list is well-organized with practical utilities. The explicit note that these utilities are "Node.js only, throws in browser" sets clear expectations for developers using multi-platform code.
376-392: ✅ RememberPlugin usage example is clear and practical.The example demonstrates the key patterns developers will use when the plugin is installed (
this.remember,this.approval), showing both memory storage and approval checking workflows. This provides helpful guidance for integrating the plugin into tools.
394-419: ✅ Anti-patterns and Do sections properly reinforce new guidance.The new anti-patterns at lines 396-397 (discouraging direct use of
node:cryptoandfs/promises) and the complementary Do pattern at line 410 create clear, actionable guidelines that reinforce the cryptographic and file system utilities guidance. Formatting and placement are consistent with existing patterns.plugins/plugin-codecall/src/index.ts (1)
1-2: LGTM! Clean barrel file implementation.The dual export pattern on Line 1 enables both default and named imports, which is a best practice for library APIs. The wildcard type re-export on Line 2 properly exposes the plugin's public type surface.
Based on learnings, this correctly exports the public API through a barrel file while maintaining consistency with the plugin architecture refactor.
libs/sdk/src/common/types/options/transport.options.ts (1)
254-283: LGTM! Serverless detection is now correct.The serverless environment auto-detection logic properly checks platform-specific indicators. The previous issue regarding
GOOGLE_CLOUD_PROJECThas been resolved—K_SERVICEon Line 275 is the correct and specific check for Google Cloud Run.libs/utils/src/storage/adapters/upstash.ts (4)
1-44: LGTM: Clean lazy-loading pattern.The manual type definition and lazy-loading approach using
require()is appropriate for an optional dependency. The error message clearly guides users to install the required package.
90-107: LGTM: Proper configuration validation.The constructor correctly validates required credentials and provides clear error messaging when configuration is missing.
238-259: LGTM: Cursor parsing is now safe.The NaN guard on line 251 correctly prevents the infinite loop issue flagged in previous reviews.
288-301: LGTM: Pub/sub mechanism is now consistent.The fix correctly aligns
publish()withsubscribe()'s list-based polling approach, resolving the critical mismatch from previous reviews.docs/draft/docs/plugins/overview.mdx (3)
78-78: Clarify the reason for removing "Authentication" from the in-development list.The Note previously mentioned that authentication, rate limiting, observability, and validation plugins were in development. This update removes "Authentication" without explanation. Please confirm whether:
- The Authentication plugin has been completed (but no card is present)
- The Authentication plugin is no longer planned
- This is an unintended omission
239-242: LGTM - CardGroup layout adjusted appropriately.The CardGroup columns changed from 3 to 2 to accommodate the new Remember Plugin card, creating a balanced 2×2 grid with 4 cards. The layout change is appropriate and visually consistent.
47-47: No action needed. The CodeCall overview documentation exists atdocs/draft/docs/plugins/codecall/overview.mdxand the href updates to/docs/plugins/codecall/overvieware correct.plugins/plugin-approval/src/approval/types.d.ts (1)
1-260: Excellent type definitions with strong TypeScript practices.The type definitions are comprehensive and follow TypeScript best practices:
- Consistent use of
Record<string, unknown>instead ofanyfor flexible metadata fields- Well-documented with clear JSDoc comments and examples
- Extensible pattern for
ApprovalSourceTypeusing(string & {})union allows vendor-specific sources while maintaining type safety- All types align well with MCP protocol requirements
Based on learnings, FrontMCP is a TypeScript-first schema validation framework, and these type definitions properly support that architecture.
plugins/plugin-approval/src/approval/errors.d.ts (1)
1-150: Well-structured error classes aligned with MCP protocol.The error class declarations follow MCP best practices:
- Each error class extends the base
ApprovalErrorfor proper error hierarchy- All errors provide
toJsonRpcError()methods for JSON-RPC compatibility with explicit error codes- The
ApprovalRequiredErrorcorrectly uses a subset ofApprovalState(excluding 'approved') since this error is only thrown when approval is neededBased on learnings, these error classes properly use specific error classes with MCP error codes instead of generic errors.
plugins/plugin-approval/src/approval/factories.d.ts (1)
1-273: LGTM! Well-designed factory functions with excellent documentation.The factory function declarations demonstrate good API design:
- Clear separation between grantor and revoker factories
- Comprehensive JSDoc with practical examples for each function
- Flexible normalization helpers that accept multiple input types (
ApprovalGrantor | ApprovalSourceType | undefined)testGrantor()provides a dedicated helper for test environments, improving test code clarityplugins/plugin-approval/src/approval/schemas.d.ts (1)
37-246: The.strict()requirement in the learnings applies only tolibs/uipack/**/{validation,**}/**/*.{ts,tsx}. This file (plugins/plugin-approval/src/approval/schemas.ts) is not in the libs/uipack directory and therefore is not subject to that constraint. The general coding guidelines do not mandate.strict()for all Zod schemas across the codebase. No changes are needed.Likely an incorrect or invalid review comment.
plugins/plugin-approval/src/approval/index.d.ts (6)
1-37: Documentation is now correct and well-structured.The module path and import examples have been properly corrected to reference
@frontmcp/plugin-approval. The usage example effectively demonstrates the approval API with Zod validation and type guards.
38-53: LGTM! Proper type/value export distinction.The exports correctly distinguish between value exports (enums like
ApprovalScope,ApprovalState) and type-only exports (interfaces/types) using thetypekeyword appropriately.
54-74: LGTM! Schema exports are well-organized.The exports properly separate runtime Zod schema objects from their corresponding TypeScript input types, providing a clean API for both validation and type checking. Compatible with Zod 4.
75-92: LGTM! Comprehensive factory function exports.The exported factory functions cover a wide range of authorization sources (user, policy, admin, system, agent, API, OAuth, custom) with consistent naming. The normalization helpers add flexibility for handling various input formats.
93-101: LGTM! Well-structured guard functions.The guard functions follow TypeScript conventions (
is*for type guards,has*for property checks) and provide comprehensive runtime type checking capabilities for the approval system.
102-109: LGTM! Complete error class hierarchy.The exported error classes provide a well-structured hierarchy with a base
ApprovalErrorand specific error types for different failure scenarios, enabling both catch-all and granular error handling.plugins/plugin-approval/src/stores/approval-storage.store.ts (3)
1-55: LGTM! Well-documented configuration interface.The imports and configuration options are clear and well-structured, with appropriate defaults documented.
61-113: LGTM! Solid initialization pattern with proper lifecycle management.The initialization logic correctly distinguishes between owned and external storage instances, and the cleanup interval is properly configured with
unref()to avoid blocking process exit.
355-366: LGTM! Proper cleanup with defensive checks.The close method correctly clears the cleanup interval and only disconnects owned storage instances, with appropriate defensive checks for uninitialized state.
libs/utils/src/storage/adapters/redis.ts (11)
1-27: LGTM! Lazy loading pattern is appropriate.The dynamic import strategy and type-only imports prevent bundling ioredis in browser builds while maintaining type safety.
60-87: LGTM! Configuration validation is robust.The mutual exclusivity checks and environment variable fallbacks provide a good developer experience with clear error messages.
93-117: LGTM! URL handling is now correct.The URL is properly passed to the Redis constructor on line 105, addressing the previous review comment. The type assertion on line 99 is acceptable since
ping()will validate the client's correctness.
119-146: LGTM! Resource management is correct.The disconnect logic properly distinguishes between owned and external clients, ensuring resources are cleaned up only when appropriate.
152-190: Fix removed but non-null assertions remain.The
anycast has been correctly removed fromdoSet()using explicit branches (addressing the previous comment). However, non-null assertions (this.client!) violate the coding guidelines.Separately, non-null assertions violate coding guidelines — see the consolidated comment at the end of this review covering lines 152-432.
196-244: LGTM! Pipeline operations now properly typed.The
anycast has been correctly removed frommset()using explicit branches (addressing the previous comment). Validation before processing is a good practice.Note: Non-null assertions are covered in the consolidated comment at the end of this review.
250-306: LGTM! TTL validation and SCAN implementation are correct.The use of
validateTTL()and SCAN-based key enumeration follow best practices. The SCAN loop properly handles pagination without blocking.Note: Non-null assertions are covered in the consolidated comment at the end of this review.
312-349: LGTM! Pub/Sub implementation is correct.The per-channel handler tracking and cleanup logic are well-designed. Line 345's use of optional chaining (
this.subscriber?.unsubscribe) is a good pattern.Note: Non-null assertions on lines 334, 336 are covered in the consolidated comment at the end of this review.
358-376: LGTM! URL handling is correct.The method correctly returns additional options when a URL is provided, with the URL itself passed separately to the Redis constructor on line 105. This properly addresses the previous review comment.
381-407: LGTM! Subscriber creation and message handling are correct.The type assertion on line 390 is necessary given the options type. Error handling in the message handler (lines 399-403) properly prevents user callback errors from affecting the adapter.
Note: Non-null assertion on line 394 is covered in the consolidated comment at the end of this review.
409-432: LGTM! Helper methods are straightforward.The prefix/unprefix logic and
getClient()accessor are correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts:
- Around line 72-75: The test description is misleading: the spec for the test
using isSecretPersistenceEnabled({ forceEnable: false }) asserts false but reads
"should be enabled..."; update the test name to reflect the expected disabled
behavior (e.g., "should be disabled in production when forceEnable is false") so
it matches the assertion and intent; locate the it(...) block in
secret-persistence.test.ts that references isSecretPersistenceEnabled and change
only the string description, not the assertion or environment setup.
In @libs/utils/src/storage/adapters/vercel-kv.ts:
- Around line 231-258: The keys() method's SCAN loop can infinite-loop because
parseInt(nextCursor, 10) may yield NaN; update the loop in keys() (involving
getConnectedClient(), prefixKey(), unprefixKey()) to defensively
convert/validate nextCursor before assigning to cursor (e.g., use
Number(nextCursor) or parseInt and then check Number.isFinite/Number.isNaN) and
break or throw if conversion fails; ensure the termination condition uses a
validated numeric cursor (or explicit string check for '0') so the do...while
can always exit and fall back to the keys() command on error.
🧹 Nitpick comments (10)
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts (1)
166-212: Consider adding test for invalid base64url characters.The schema uses
z.string().base64url().length(43)to validate the secret format. While you test for short secrets, there's no test verifying that invalid base64url characters (e.g.,+,/, or=padding) are rejected. This would ensure the schema's base64url validation is exercised.🔎 Suggested additional test
it('should reject secret with invalid base64url characters', () => { // Contains '+' and '/' which are standard base64, not base64url const invalidBase64urlSecret = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ+bcdefghijklmno/'; const result = validateSecretData({ secret: invalidBase64urlSecret, createdAt: Date.now(), version: 1 }); expect(result.valid).toBe(false); });libs/utils/src/crypto/secret-persistence/schema.ts (1)
73-79: Consider using Zod's parsed result instead of casting.The function validates data but then casts the original
dataparameter. While safe due to validation, using Zod's parsed result would be more idiomatic and type-safe.🔎 Proposed improvement
export function parseSecretData(data: unknown): SecretData | null { - const validation = validateSecretData(data); - if (!validation.valid) { + const result = secretDataSchema.safeParse(data); + if (!result.success) { + return null; + } + const parsed = result.data; + const now = Date.now(); + // Verify createdAt is not in the future (with drift allowance) + if (parsed.createdAt > now + 60000) { + return null; + } + // Verify not too old + if (parsed.createdAt < now - 100 * 365 * 24 * 60 * 60 * 1000) { return null; } - return data as SecretData; + return parsed; }Alternatively, if you want to keep the current structure, export the parsed result from
validateSecretData:export function validateSecretData(data: unknown): SecretValidationResult & { data?: SecretData } { // ... existing validation ... return { valid: true, data: result.data }; }plugins/plugin-approval/src/stores/approval-storage.store.ts (1)
83-98: Consider makingstorageoptional instead of using definite assignment assertion.Line 83 uses a definite assignment assertion (
storage!: NamespacedStorage) which is discouraged per coding guidelines. While the lifecycle pattern withensureInitialized()guards against undefined access, consider making it optional for better type safety:-private storage!: NamespacedStorage; +private storage?: NamespacedStorage;Then update
ensureInitialized()to include a type guard or throw with a type assertion.libs/utils/src/storage/namespace.ts (3)
28-33: Consider validating thenameparameter.If
nameis an empty string, this returns just":"(or":id:"with an id), which could cause unexpected behavior with key lookups or namespace collisions.🔎 Optional validation
export function buildPrefix(name: string, id?: string): string { + if (!name) { + throw new Error('Namespace name cannot be empty'); + } if (id) { return `${name}${NAMESPACE_SEPARATOR}${id}${NAMESPACE_SEPARATOR}`; } return `${name}${NAMESPACE_SEPARATOR}`; }
90-104: Lifecycle delegation inconsistency.The comment says "delegated to root" but the implementation delegates to
this.adapter. For nested namespaces created vianamespace(),this.adapterandthis.rootare the same (line 87 passesthis.root), so this works correctly. However, the comment is misleading—consider updating it to reflect that delegation goes to the adapter (which happens to be the root for nested storages).
200-210: ReuseunprefixKeyhelper in subscribe handler.The unprefixing logic on line 205 duplicates what
unprefixKeydoes. Consider reusing the helper for consistency.🔎 Proposed refactor
const wrappedHandler: MessageHandler = (message, ch) => { - const unprefixedChannel = ch.startsWith(this.prefix) ? ch.slice(this.prefix.length) : ch; + const unprefixedChannel = this.unprefixKey(ch); handler(message, unprefixedChannel); };Note: This requires either making
unprefixKeynon-private or extracting the logic to a shared helper.libs/utils/src/storage/adapters/memory.ts (4)
64-65: LRU tracking has O(n) performance.The
accessOrderarray withindexOfandspliceoperations (lines 397-401, 410-413) is O(n) per access. For a development/testing adapter with small datasets, this is acceptable. However, if this adapter is used withmaxEntriesset to large values, performance will degrade.For production use cases requiring LRU eviction, consider a doubly-linked list + Map approach for O(1) operations.
285-322: Consider integer overflow protection.The
incrByoperation doesn't validate that the result stays within JavaScript's safe integer range (Number.MAX_SAFE_INTEGER). For extreme cases, this could lead to precision loss.🔎 Optional bounds check
const newValue = currentValue + amount; + + if (!Number.isSafeInteger(newValue)) { + throw new StorageOperationError('incrBy', key, 'Result exceeds safe integer range'); + } // Preserve TTL if exists
332-337: Consider refactoring for readability.The comma operator on line 335 is clever but reduces readability. A more explicit approach would be clearer.
🔎 Suggested refactor
override async publish(channel: string, message: string): Promise<number> { this.ensureConnected(); - return this.emitter.listenerCount(channel) > 0 - ? (this.emitter.emit(channel, message, channel), this.emitter.listenerCount(channel)) - : 0; + const listenerCount = this.emitter.listenerCount(channel); + if (listenerCount > 0) { + this.emitter.emit(channel, message, channel); + } + return listenerCount; }
419-426: Minor redundancy inevictOldest.The
evictOldestmethodshift()s the key fromaccessOrder(line 422), then callsdeleteEntrywhich internally callsremoveLRU. TheremoveLRUcall will search for the already-removed key and find nothing. This is harmless but slightly redundant.🔎 Optional: Skip redundant LRU removal
One option is to inline the deletion logic here without the
removeLRUcall, but the current approach is simple and the cost is negligible for typical usage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/draft/docs/plugins/overview.mdxlibs/utils/src/crypto/index.tslibs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.tslibs/utils/src/crypto/secret-persistence/persistence.tslibs/utils/src/crypto/secret-persistence/schema.tslibs/utils/src/fs/fs.tslibs/utils/src/storage/__tests__/factory.test.tslibs/utils/src/storage/adapters/base.tslibs/utils/src/storage/adapters/memory.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/storage/adapters/upstash.tslibs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/storage/factory.tslibs/utils/src/storage/namespace.tsplugins/plugin-approval/src/stores/approval-storage.store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/utils/src/storage/tests/factory.test.ts
- libs/utils/src/fs/fs.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/utils/src/storage/adapters/upstash.tsplugins/plugin-approval/src/stores/approval-storage.store.tslibs/utils/src/storage/namespace.tslibs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.tslibs/utils/src/storage/factory.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/crypto/index.tslibs/utils/src/storage/adapters/memory.tslibs/utils/src/storage/adapters/base.tslibs/utils/src/crypto/secret-persistence/persistence.tslibs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/crypto/secret-persistence/schema.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/utils/src/storage/adapters/upstash.tslibs/utils/src/storage/namespace.tslibs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.tslibs/utils/src/storage/factory.tslibs/utils/src/storage/adapters/redis.tslibs/utils/src/crypto/index.tslibs/utils/src/storage/adapters/memory.tslibs/utils/src/storage/adapters/base.tslibs/utils/src/crypto/secret-persistence/persistence.tslibs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/crypto/secret-persistence/schema.ts
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/plugins/overview.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/plugins/overview.mdx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including error cases and constructor validation
Include error classinstanceofchecks in tests to validate error handling
Files:
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts
🧠 Learnings (10)
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{validation,**}/**/*.{ts,tsx} : Use `.strict()` on all Zod schemas for validation
Applied to files:
plugins/plugin-approval/src/stores/approval-storage.store.tslibs/utils/src/crypto/secret-persistence/schema.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and constructor validation
Applied to files:
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/{theme,adapters,bundler}/**/*.{test,spec}.{ts,tsx,js,jsx} : Test behavior across all supported platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Include error class `instanceof` checks in tests to validate error handling
Applied to files:
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.tslibs/utils/src/storage/adapters/redis.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
libs/utils/src/crypto/index.ts
📚 Learning: 2026-01-04T14:35:18.353Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2026-01-04T14:35:18.353Z
Learning: Applies to libs/uipack/**/index.{ts,js} : Export all public APIs through appropriate entry points (frontmcp/uipack, frontmcp/uipack/adapters, frontmcp/uipack/theme, etc.)
Applied to files:
libs/utils/src/crypto/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/utils/src/crypto/secret-persistence/schema.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using proper validation
Applied to files:
libs/utils/src/crypto/secret-persistence/schema.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
libs/utils/src/crypto/secret-persistence/schema.ts
🧬 Code graph analysis (4)
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts (2)
libs/utils/src/crypto/secret-persistence/persistence.ts (10)
clearCachedSecret(310-317)isSecretPersistenceEnabled(43-53)resolveSecretPath(61-75)generateSecret(219-221)createSecretData(229-236)saveSecret(150-183)loadSecret(108-138)deleteSecret(191-207)getOrCreateSecret(262-303)isSecretCached(325-328)libs/utils/src/crypto/secret-persistence/schema.ts (3)
validateSecretData(42-65)parseSecretData(73-79)secretDataSchema(13-19)
libs/utils/src/storage/factory.ts (4)
libs/utils/src/index.ts (12)
StorageType(148-148)StorageConfig(149-149)StorageAdapter(137-137)MemoryStorageAdapter(167-167)RedisStorageAdapter(168-168)VercelKvStorageAdapter(169-169)UpstashStorageAdapter(170-170)StorageConfigError(161-161)createStorage(133-133)RootStorage(139-139)createNamespacedStorage(153-153)createRootStorage(152-152)libs/utils/src/storage/types.ts (4)
StorageType(477-477)StorageConfig(482-528)StorageAdapter(79-296)RootStorage(350-350)libs/utils/src/storage/errors.ts (1)
StorageConfigError(100-105)libs/utils/src/storage/namespace.ts (2)
createNamespacedStorage(235-239)createRootStorage(224-226)
libs/utils/src/storage/adapters/memory.ts (4)
libs/utils/src/storage/types.ts (2)
MemoryAdapterOptions(359-377)SetOptions(15-33)libs/utils/src/storage/utils/ttl.ts (4)
isExpired(82-85)ttlToExpiresAt(61-63)validateTTL(21-41)expiresAtToTTL(71-74)libs/utils/src/storage/utils/pattern.ts (1)
globToRegex(49-101)libs/utils/src/storage/errors.ts (1)
StorageOperationError(60-65)
libs/utils/src/crypto/secret-persistence/schema.ts (1)
libs/utils/src/crypto/secret-persistence/types.ts (2)
SecretValidationResult(68-73)SecretData(10-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (35)
docs/draft/docs/plugins/overview.mdx (5)
78-78: LGTM!The note appropriately communicates upcoming plugin development to users, setting clear expectations about the roadmap.
232-232: LGTM!The Remember Plugin comparison table entry accurately summarizes the plugin's characteristics and aligns with the detailed description provided earlier in the document.
239-242: LGTM!The Remember Plugin card in the Next Steps section is well-positioned and provides a clear call-to-action for users to learn about session memory and approval features.
47-47: No changes needed. The CodeCall overview documentation exists atdocs/draft/docs/plugins/codecall/overview.mdxand the href path is correctly configured.
33-45: Backend support and branded payloads claims are accurate.All stated features in the Remember Plugin card have been verified in the codebase:
- Backends (Memory, Redis, Vercel KV, Upstash) are fully implemented with dedicated provider modules
- Branded payloads exist and support semantic categorization with multiple payload types (approval, preference, cache, state, conversation, custom)
- Documentation file exists at the correct path
No changes needed.
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts (2)
242-312: LGTM!Good coverage of save/load operations including edge cases for invalid JSON, invalid structure, and persistence disabled scenarios. The ENOENT error check at line 298 appropriately validates the specific error type.
365-379: LGTM!The concurrency test is well-designed, creating 10 concurrent calls and correctly verifying that all return the same secret. This properly exercises the promise guard mechanism in
getOrCreateSecret.libs/utils/src/crypto/index.ts (1)
223-288: LGTM!The barrel file properly re-exports the new crypto-related modules with clear organization:
- Encrypted blob utilities (types, errors, core functions, serialization, validation)
- PKCE utilities (constants, errors, functions, types)
- Secret persistence utilities (types, schema/validation, persistence operations, generation, high-level API)
The exports are well-organized with comments categorizing each group, following the barrel file pattern per coding guidelines.
libs/utils/src/crypto/secret-persistence/schema.ts (2)
13-19: LGTM!The schema correctly uses
.strict()andz.base64url().length(43)as addressed from previous review comments. The validation properly enforces:
- Exact 43-character base64url-encoded secret (32 bytes)
- Positive integer timestamps and version
- No extra properties allowed
42-65: LGTM!The validation logic correctly:
- Performs schema validation first
- Checks for future timestamps with clock drift allowance
- Checks for excessively old timestamps
- Returns typed validation results with descriptive error messages
libs/utils/src/crypto/secret-persistence/persistence.ts (5)
27-28: Snyk false positive - this is a directory name, not a secret.The Snyk scan flagged line 27 as a "Hardcoded Non-Cryptographic Secret", but
'.frontmcp'is simply a directory name for storing secret files, not a secret value itself. This is a false positive and can be safely ignored. Consider adding a comment to clarify this for future scans.
108-138: LGTM!The
loadSecretfunction correctly:
- Checks persistence enablement first
- Handles missing files gracefully (ENOENT returns null without logging)
- Validates loaded data structure before returning
- Logs warnings for other errors while returning null safely
150-183: LGTM!Excellent implementation of atomic write with proper security:
- Unique temp file name prevents collisions
- Directory created with
0o700(owner-only access)- File written with
0o600(owner read/write only)- Atomic rename ensures no partial writes
- Proper cleanup of temp file on failure
262-303: LGTM with minor observation.The concurrency guard pattern is well-implemented:
- Cache-first lookup avoids unnecessary I/O
- Promise guard prevents duplicate concurrent generation
finallyblock ensures cleanup of the generation promiseOne edge case: if the inner promise rejects, the secret won't be cached, and subsequent calls will retry generation. This is actually desirable behavior for transient failures.
191-207: LGTM!The
deleteSecretfunction correctly treats non-existent files as a success (idempotent deletion), which is the expected behavior for cleanup operations.plugins/plugin-approval/src/stores/approval-storage.store.ts (7)
28-34: LGTM: Well-implemented pattern escaping utility.The
escapePatternfunction correctly escapes glob metacharacters (*, ?, [, ], ) for safe pattern matching. However, ensure it's used consistently throughout the class (see separate comment about line 185).
40-67: LGTM: Well-documented configuration interface.The options interface is clear, type-safe, and provides sensible defaults.
100-131: LGTM: Robust initialization and lifecycle guards.The initialization flow properly handles both external and owned storage instances, sets up optional cleanup intervals with
unref()to avoid blocking Node.js exit, and provides clear error messages viaensureInitialized().
141-153: LGTM: Proper schema validation added.The
parseRecordmethod now usesapprovalRecordSchema.safeParse()to validate parsed JSON before casting, addressing the unsafe type assertion issue from previous reviews. This ensures corrupted or mismatched data won't cause runtime errors.
221-292: LGTM: Core operations are well-implemented.The
grantApproval,revokeApproval, andisApprovedmethods correctly handle:
- TTL management with proper conversion to seconds
- Grantor normalization
- Hierarchical precedence (context → session → user)
- Expiry filtering
- Initialization guards
294-307: LGTM: Pattern injection fixed with proper escaping.The
clearSessionApprovalsmethod now correctly usesescapePattern(sessionId)at line 298 before constructing the glob pattern, addressing the security concern from previous reviews. This prevents metacharacters in session IDs from matching unintended keys.
332-386: LGTM: Statistics and cleanup properly implemented.The
getStatsmethod correctly initializes counters for all enum values, preventing undefined entries. Theclosemethod ensures proper resource cleanup by clearing intervals and disconnecting owned storage connections.libs/utils/src/storage/adapters/upstash.ts (1)
1-408: LGTM! Previous review concerns have been addressed.The cursor parsing now includes NaN guards (line 259), pub/sub uses a consistent queue-based mechanism (lines 298-304, 322-326), and connection handling properly uses
getConnectedClient()throughout. The implementation is type-safe and follows the coding guidelines.libs/utils/src/storage/adapters/base.ts (1)
1-187: LGTM! Well-structured base class.The abstract base class provides a solid contract with appropriate default implementations for batch operations and clear error handling for unsupported operations. The validation and connection guard patterns are consistent and type-safe.
libs/utils/src/storage/adapters/redis.ts (1)
1-456: LGTM! Previous review concerns have been thoroughly addressed.The implementation now:
- Correctly passes URL to Redis constructor (line 104)
- Eliminates
anycasts by using explicit conditional branches for set operations (lines 186-200, 238-252)- Replaces non-null assertions with type-safe
getConnectedClient()andgetConnectedSubscriber()helpers throughoutThe code follows strict TypeScript guidelines and implements proper error handling.
libs/utils/src/storage/factory.ts (1)
1-215: LGTM! Factory implementation is robust and well-structured.The auto-detection logic correctly prioritizes backend types, fallback behavior is properly implemented with appropriate warnings, and error handling is consistent using
StorageConfigError. The dynamic imports with.jsextensions are correct for ESM compatibility.libs/utils/src/storage/namespace.ts (3)
1-13: LGTM!Clean imports and appropriate choice of namespace separator constant.
106-190: LGTM!Core, batch, TTL, key enumeration, and atomic operations correctly apply key prefixing/unprefixing. The
keys()method properly unprefixes returned keys to maintain the namespace abstraction.
217-239: LGTM!Factory functions are well-documented and handle prefix normalization correctly. The check on line 237 properly avoids adding a separator to an empty prefix.
libs/utils/src/storage/adapters/memory.ts (6)
1-33: LGTM!Clean imports, well-documented constants, and appropriate internal interface definition. The
MAX_TIMEOUT_MScorrectly accounts for Node.js timer limitations.
79-107: LGTM!Connection lifecycle is well-implemented with proper cleanup in
disconnect()to prevent memory leaks. The idempotent guards for connect/disconnect are good defensive programming.
131-175: LGTM!The
doSetimplementation correctly handles:
- NX (ifNotExists) and XX (ifExists) semantics
- TTL with proper timeout scheduling and
unref()for clean process exit- LRU eviction before adding new entries when maxEntries is reached
196-246: LGTM!TTL operations correctly validate input, manage timeouts, and follow Redis-like conventions (returning -1 for keys without TTL).
252-271: LGTM!Key enumeration correctly handles expiration cleanup during iteration (safe in JavaScript Maps) and leverages
globToRegexwith built-in ReDoS protection.
455-462: LGTM!The
sweep()method correctly handles deletion during Map iteration, which is safe in JavaScript. The background sweeper withunref()ensures clean process exit.
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.ts
Outdated
Show resolved
Hide resolved
…in persistence logic
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @plugins/plugin-cache/src/__tests__/cache.plugin.test.ts:
- Line 128: The cast uses permissive any types: replace the use of any in the
type assertion for ProviderFactoryType to unknown to satisfy strict TypeScript
rules; find the line where providers[0] is asserted (the const provider =
providers[0] as ProviderFactoryType<any, any>; statement) and change the generic
parameters from any, any to unknown, unknown so the declaration becomes
ProviderFactoryType<unknown, unknown>.
- Line 115: Replace the unsafe any usage in the type cast by changing
ProviderFactoryType<any, any> to ProviderFactoryType<unknown, unknown> so
TypeScript preserves type safety; update the declaration for provider (currently
const provider = providers[0] as ProviderFactoryType<any, any>;) to use
ProviderFactoryType<unknown, unknown> instead.
🧹 Nitpick comments (5)
libs/utils/src/storage/adapters/vercel-kv.ts (2)
36-40: Consider preserving the original error for better debugging.The empty catch block discards the original error, which could contain useful debugging information if the module fails to load for reasons other than being absent (e.g., syntax errors, import issues).
🔎 Suggested improvement
try { return require('@vercel/kv'); - } catch { - throw new Error('@vercel/kv is required for Vercel KV storage adapter. Install it with: npm install @vercel/kv'); + } catch (err) { + throw new Error( + `@vercel/kv is required for Vercel KV storage adapter. Install it with: npm install @vercel/kv. Original error: ${err instanceof Error ? err.message : String(err)}` + ); }
109-111: Remove redundant validation.The URL and token are already validated in the constructor (lines 82-87), so this additional check is unnecessary and unreachable code.
🔎 Proposed refactor
} else { - const url = this.options.url; - const token = this.options.token; - if (!url || !token) { - throw new StorageConfigError('vercel-kv', 'URL and token are required'); - } - this.client = createClient({ url, token }); + this.client = createClient({ url: this.options.url, token: this.options.token }); }libs/utils/src/crypto/secret-persistence/persistence.ts (3)
130-137: Consider using proper type guards instead of type assertions for error handling.The type assertions
(error as NodeJS.ErrnoException)and(error as Error)bypass TypeScript's type safety. Per coding guidelines for strict TypeScript mode, prefer type guards.🔎 Suggested refactor using type guards
- } catch (error: unknown) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - // File doesn't exist - normal for first run - return null; - } - - logger.warn(`${prefix} Failed to load secret from ${secretPath}: ${(error as Error).message}`); - return null; + } catch (error: unknown) { + // Type guard for Node.js errno exceptions + if (typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT') { + // File doesn't exist - normal for first run + return null; + } + + const message = error instanceof Error ? error.message : String(error); + logger.warn(`${prefix} Failed to load secret from ${secretPath}: ${message}`); + return null;
174-183: Same type assertion pattern as inloadSecret.Line 175 uses
(error as Error).messagewhich bypasses type safety. Consider using the same type guard pattern suggested forloadSecretthroughout the file for consistency.🔎 Suggested refactor
} catch (error: unknown) { - logger.error(`${prefix} Failed to save secret to ${secretPath}: ${(error as Error).message}`); + const message = error instanceof Error ? error.message : String(error); + logger.error(`${prefix} Failed to save secret to ${secretPath}: ${message}`); // Clean up temp file if it exists
200-206: Same type assertion pattern for error handling.Lines 201 and 205 use type assertions for error handling. For consistency with the suggested improvements in
loadSecretandsaveSecret, consider using type guards here as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/utils/src/crypto/secret-persistence/__tests__/secret-persistence.test.tslibs/utils/src/crypto/secret-persistence/persistence.tslibs/utils/src/storage/adapters/vercel-kv.tsplugins/plugin-cache/src/__tests__/cache.plugin.test.tsplugins/plugin-remember/src/remember.secret-persistence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/utils/src/crypto/secret-persistence/tests/secret-persistence.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/utils/src/storage/adapters/vercel-kv.tsplugins/plugin-cache/src/__tests__/cache.plugin.test.tslibs/utils/src/crypto/secret-persistence/persistence.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/utils/src/storage/adapters/vercel-kv.tslibs/utils/src/crypto/secret-persistence/persistence.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including error cases and constructor validation
Include error classinstanceofchecks in tests to validate error handling
Files:
plugins/plugin-cache/src/__tests__/cache.plugin.test.ts
🧬 Code graph analysis (1)
plugins/plugin-cache/src/__tests__/cache.plugin.test.ts (1)
plugins/plugin-cache/src/cache.symbol.ts (1)
CacheStoreToken(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
🔇 Additional comments (12)
plugins/plugin-cache/src/__tests__/cache.plugin.test.ts (1)
118-123: Good refactor: using local variable improves readability.Replacing repeated
providers[0]array indexing with a localprovidervariable makes the code cleaner and more maintainable.libs/utils/src/storage/adapters/vercel-kv.ts (8)
75-91: LGTM!The constructor properly validates required configuration (URL and token) from options or environment variables, with appropriate error handling via StorageConfigError.
127-141: LGTM!The lifecycle methods are appropriate for a REST-based client: disconnect is a no-op that cleans up state, and ping performs a lightweight health check.
162-192: LGTM!Core CRUD operations correctly implement key prefixing, conditional sets, and TTL handling. The TTL validation is appropriately delegated to the base class's
set()method.
198-208: LGTM!Batch operations correctly handle edge cases (empty arrays) and properly prefix keys before delegating to the client.
214-225: LGTM!TTL operations properly validate input and correctly handle Redis semantics for non-existent keys and keys without TTL.
231-259: Previous critical issue has been resolved!The cursor parsing logic (lines 245-246) now correctly handles both string and numeric cursors and guards against NaN, which resolves the infinite loop issue flagged in the previous review. The implementation safely falls back to the
keys()command if SCAN fails.
265-275: LGTM!Atomic operations correctly prefix keys and delegate to the underlying client.
281-308: LGTM!Pub/sub is correctly marked as unsupported with a helpful suggestion to use Upstash. The internal key prefixing helpers properly handle prefix addition and removal with appropriate boundary checks.
libs/utils/src/crypto/secret-persistence/persistence.ts (3)
27-28: LGTM: Snyk ignore comment properly justifies the false positive.The ignore comment correctly clarifies that
DEFAULT_SECRET_DIRis a directory path constant, not a cryptographic secret. This addresses the Snyk warning appropriately.
220-237: LGTM: Secret generation correctly uses cryptographic primitives.The implementation properly uses
randomBytesfor cryptographically secure random generation and base64url encoding for safe string representation. The default 32 bytes (256 bits) aligns with AES-256-GCM mentioned in the documentation.
311-318: The multi-plugin cache collision concern is currently not applicable.This utility is currently consumed only by the Remember plugin in production. While the module-level singleton cache is a design choice that could theoretically impact multiple plugins if they are added in the future, the immediate concern is not a practical issue. The
clearCachedSecret()behavior is already documented in tests (line 415 ofsecret-persistence.test.tsexplicitly demonstrates "clear all" semantics), and the Remember plugin's own wrapper enforces clearing the entire cache, so there is no immediate risk of unintended cross-plugin state pollution.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.