feat(plugins): implement CodeCall plugin structure with security and error handling #81
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Layer‑0 PreScanner and broad pre‑parse security checks; integrates an AI Scoring Gate and scoring APIs into Enclave; implements a Worker Pool adapter for OS‑level isolation; substantially expands the CodeCall plugin with security, sanitization, auditing, search/indexing, tools, and tests; updates SDK registries, transport handlers, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent / LLM Script
participant CodeCall as CodeCall Plugin
participant PreScan as PreScanner (Layer 0)
participant Access as ToolAccessControlService
participant Enclave as Enclave Runtime
participant Score as ScoringGate
participant Worker as Worker Pool (optional)
participant Audit as AuditLoggerService
Agent->>CodeCall: call meta-tool (search/describe/execute/invoke)
CodeCall->>PreScan: preScan(script/input)
alt pre-scan fails
PreScan-->>CodeCall: PreScannerError
CodeCall-->>Agent: sanitized error result
else pre-scan passes
CodeCall->>Access: assertNotSelfReference + checkAccess(tool)
alt access denied
Access-->>CodeCall: AccessDenied
CodeCall-->>Agent: sanitized error result
else allowed
CodeCall->>Enclave: request execution (script)
Enclave->>Score: extract features & evaluate(code)
Score-->>Enclave: ScoringResult
alt scoring blocks
Enclave-->>CodeCall: ScoringGateError
CodeCall-->>Agent: blocked response
else scoring allows
Enclave->>Worker: (optional) acquire slot & run
Worker-->>Enclave: execution result
Enclave->>Enclave: sanitize output / enrich errors
Enclave-->>CodeCall: result + stats
CodeCall->>Audit: log execution and tool calls
CodeCall-->>Agent: response
end
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
libs/sdk/src/common/interfaces/internal/registry.interface.ts-122-127 (1)
122-127: Type mismatch confirmed: interface declaresToolEntrybut implementation usesToolInstance.
ToolInstanceextendsToolEntry, but the callback signature should align. SincelistAllInstances()returnsreadonly ToolInstance[](line 219), the filter callback receivesToolInstanceobjects only, not genericToolEntry. Update the interface to match:subscribe( - opts: { immediate?: boolean; filter?: (i: ToolEntry) => boolean }, + opts: { immediate?: boolean; filter?: (i: ToolInstance) => boolean }, cb: (evt: ToolChangeEvent) => void, ): () => void;docs/draft/docs/plugins/codecall/overview.mdx-188-196 (1)
188-196: Code example inconsistency with tool response structure.The example shows
users.filter(...)directly, but based on theusers:listtool response schema shown in the API reference ({ users: [...] }), this should beusers.users.filter(...)or the variable should be renamed for clarity.- const users = await callTool('users:list', { limit: 1000 }); - const active = users.filter(u => + const response = await callTool('users:list', { limit: 1000 }); + const active = response.users.filter(u =>libs/plugins/src/codecall/tools/search.tool.ts-37-37 (1)
37-37: Remove debug console.log statement.This
console.logappears to be debug code that should not remain in production. Either remove it or replace with structured logging via the logger service if runtime visibility is needed.- console.log({ query, topK, filter, excludeToolNames });docs/draft/docs/plugins/codecall/api-reference.mdx-547-549 (1)
547-549: The AgentScript guide link is broken and requires fixing.The link to
/docs/plugins/codecall/agentscriptreferences a non-existent page. Theagentscript.mdxfile does not exist indocs/draft/docs/plugins/codecall/.While AgentScript is well-documented throughout the codebase (in
security.mdx,guides/ast-guard.mdx,guides/enclave.mdx), there is no dedicated AgentScript guide file in the CodeCall plugin directory. Either:
- Create the missing
agentscript.mdxfile indocs/draft/docs/plugins/codecall/, or- Update the link to point to an existing guide, such as
/docs/guides/ast-guardwhich contains comprehensive AgentScript documentation.docs/draft/docs/plugins/codecall/configuration.mdx-157-165 (1)
157-165: Duplicate accordion content: "Direct access only" and "CodeCall meta-tools only" show identical configuration.Both accordions at lines 147-155 and 157-165 display the same code block (
enabledInCodeCall: false, visibleInListTools: true), but describe different use cases. The "CodeCall meta-tools only" accordion title and description seem inconsistent with the configuration shown.Consider clarifying the distinction or correcting the configuration for the meta-tools pattern if it should differ.
docs/draft/blog/11-2025/codecall-plugin.mdx-56-60 (1)
56-60: Minor style improvement for sentence variety.Static analysis correctly flagged three successive sentences starting with "It" (lines 58-59). Consider varying the sentence structure for better readability.
-But worse than cost: the model starts **losing context**. It drops earlier instructions. It forgets what it was doing. It hallucinates. +But worse than cost: the model starts **losing context**. Earlier instructions get dropped. The conversation thread breaks. Hallucinations follow.libs/plugins/src/codecall/tools/invoke.tool.ts-139-164 (1)
139-164: Now let me create the final rewritten review comment based on the verification:Remove the
as anytype assertions for optional ZodIssue properties or properly type them using ZodIssue discriminated union.The json-schema-to-zod-v3 library is a well-designed internal package with comprehensive JSON Schema 7 support (oneOf, allOf, anyOf, additionalProperties, etc.) and built-in ReDoS protection. Error handling in invoke.tool.ts is adequate—schema conversion errors and validation errors are both caught and handled gracefully at line 144.
However, lines 159-160 use unsafe
as anycasts forexpectedandreceivedproperties. These properties exist on specific ZodIssue types (e.g.,ZodIssueOptionalUnexpectedType) but not all. Either use the ZodIssue discriminated union type to safely access type-specific properties or remove these fields if they're not essential for the error response. This improves type safety without sacrificing the user-friendly error formatting.Note: The library does not support JSON Schema
$ref(references), which is acceptable for this use case as it eliminates circular reference risks while still covering the common validation scenarios.libs/plugins/src/codecall/services/output-sanitizer.ts-251-258 (1)
251-258: Path regex may cause false positives on URL paths.The regex
(?:\/[\w.-]+)+will match URL paths like/api/users/123and replace them with[path], which may unintentionally sanitize legitimate API response data.Consider either:
- Document this behavior clearly
- Make the path sanitization opt-in or configurable
- Use a more specific regex that targets absolute file paths:
if (config.removeFilePaths) { - const pathRegex = /(?:\/[\w.-]+)+|(?:[A-Za-z]:\\[\w\\.-]+)+/g; + // Match absolute paths: Unix paths starting with / followed by typical directory names, + // or Windows paths like C:\Users\... Avoid matching URL paths like /api/users + const pathRegex = /(?:\/(?:home|usr|var|etc|opt|tmp|Users|root|app|src|lib|node_modules)[\w./-]*)|(?:[A-Za-z]:\\[\w\\.-]+)+/g;libs/plugins/src/codecall/services/error-enrichment.service.ts-169-178 (1)
169-178: Same invalid syntax issue in validation error example.example: `// Check tool schema first -const schema = await codecall:describe({ toolNames: ['users:create'] }); +// Use codecall:describe from your LLM context before writing the script // Then provide correct input const user = await callTool('users:create', { name: 'John', // required: string email: 'j@ex.com', // required: string age: 30 // optional: number });`,docs/live/docs/plugins/codecall-plugin.mdx-302-304 (1)
302-304: Example contradicts self-reference blocking security model.This example shows calling
callTool('codecall:describe', ...)from within a script, but the Security Model section explicitly states that CodeCall meta-tools cannot be called from scripts (lines 99-112 documentSELF_REFERENCE_BLOCKEDbehavior).Either remove this example or replace it with a pattern that doesn't call
codecall:*tools:-const info = await callTool('codecall:describe', { toolNames: ['${toolName}'] }); -console.log(info.tools[0].inputSchema); +// Note: Use codecall:describe from your LLM context before writing the script +// to understand the expected schema for '${toolName}'Committable suggestion skipped: line range outside the PR's diff.
libs/plugins/src/codecall/services/error-enrichment.service.ts-140-146 (1)
140-146: Invalid JavaScript syntax in error pattern example.The example uses
await codecall:search({ query: "..." })which is not valid JavaScript. Additionally, this contradicts the self-reference blocking behavior documented elsewhere:example: `// First search for the tool -const results = await codecall:search({ query: "user management" }); +// Use codecall:search from your LLM context to find tools +// Example tool names: 'users:getById', 'users:list' // Then use the correct tool name const user = await callTool('users:getById', { id: '123' });`,docs/live/docs/plugins/codecall-plugin.mdx-140-146 (1)
140-146: Invalid JavaScript syntax in example.The example uses
await codecall:search({ query: "..." })which is not valid JavaScript syntax. The colon incodecall:searchwould be parsed as a label, not a function call.This should use
callTool()consistently with the rest of the documentation:-const results = await codecall:search({ query: "user management" }); +const results = await callTool('codecall:search', { query: "user management" });However, note that this contradicts the Self-Reference Blocking section (lines 99-112) which states that
codecall:*tools cannot be called from within scripts. Consider clarifying whether this example is meant to run outside ofcodecall:executescripts.Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (34)
.gitignore (1)
94-94: ✓ Approve broad.cachedirectory exclusion.The addition of
**/.cache/is a clean, recursive pattern that will match.cachedirectories at any depth. This is more concise than the existing specific transformer cache paths on lines 54–57 and aligns with standard.gitignoreconventions.You may consider consolidating by removing the specific
.cache/transformers/...entries (lines 54–57) since the new pattern now covers them. However, this is entirely optional and does not impact functionality.libs/sdk/src/transport/adapters/transport.local.adapter.ts (2)
53-56: Capabilities spreading aligns with dynamic registry design; watch for top-level key collisionsMerging capabilities via object spread from
tools,resources, andpromptsregistries fits the dynamic, registry-driven capabilities model and removes hardcoded structures (good). One thing to keep in mind: if multiple registries ever return overlapping top-level capability keys, later spreads will overwrite earlier ones. If that’s not intended, consider documenting the “disjoint keys” invariant forgetCapabilities()implementations or switching to a more explicit merge strategy when a collision is detected.
115-125: Replace non-null assertions onsessionwith explicit validation
ensureAuthInforelies onsession!forsessionIdandsessionIdPayload. This violates the “avoid non-null assertions” guideline and can throw at runtime if a supposedly authenticated request is missingsession. Prefer an explicit guard with a clear error:const { token, user, session } = req[ServerRequestTokens.auth]; if (!session) { // Replace with your preferred error type / code throw new Error('Missing session for authenticated request in ensureAuthInfo'); } req.auth = { token, user, sessionId: session.id, sessionIdPayload: session.payload, // ... } satisfies AuthInfo;This both satisfies the typing guidelines and fails fast with a clearer error path.
libs/sdk/src/tool/flows/tools-list.flow.ts (1)
171-201: TighteninputSchematyping and avoidany/eslint-disable in schema conversion blockThe control flow and fallbacks (raw JSON schema → Zod shape conversion → empty object schema) look solid, but this block currently relies on
anyand an eslint-disable, which goes against the stricter typing guidelines forlibs/sdk/src/**/*.ts. It also shadows the top-levelinputSchemaconstant, which is a bit confusing.You can keep the same runtime behavior while:
- Binding the local variable to the MCP type (
ResponseToolItem['inputSchema']) instead ofany.- Dropping the
eslint-disableby avoiding broadas anycasts.- Renaming the local variable to avoid confusion with the top-level
inputSchema.For example:
- const tools: ResponseToolItem[] = resolved.map(({ finalName, tool }) => { - // Get the input schema - prefer rawInputSchema (JSON Schema), then convert from tool.inputSchema - let inputSchema: any; + const tools: ResponseToolItem[] = resolved.map(({ finalName, tool }) => { + // Get the input schema - prefer rawInputSchema (JSON Schema), then convert from tool.inputSchema + let toolInputSchema: ResponseToolItem['inputSchema']; if (tool.rawInputSchema) { - // Already converted to JSON Schema - inputSchema = tool.rawInputSchema; + // Already converted to JSON Schema + toolInputSchema = tool.rawInputSchema as ResponseToolItem['inputSchema']; } else if (tool.inputSchema && Object.keys(tool.inputSchema).length > 0) { // tool.inputSchema is a ZodRawShape (extracted .shape from ZodObject in ToolInstance constructor) // Convert to JSON Schema try { - // as any used here to prevent hard ts-check on tool input that is redundant - // and just slow down the build process. types here are unnecessary. - // eslint-disable-next-line - inputSchema = zodToJsonSchema(z.object(tool.inputSchema) as any) as any; + const zodObject = z.object(tool.inputSchema); + toolInputSchema = zodToJsonSchema(zodObject) as ResponseToolItem['inputSchema']; } catch (e) { this.logger.warn(`Failed to convert inputSchema for tool ${finalName}:`, e); - inputSchema = { type: 'object', properties: {} }; + toolInputSchema = { type: 'object', properties: {} } as ResponseToolItem['inputSchema']; } } else { // No schema defined - use empty object schema - inputSchema = { type: 'object', properties: {} }; + toolInputSchema = { type: 'object', properties: {} } as ResponseToolItem['inputSchema']; } return { name: finalName, title: tool.metadata.name, description: tool.metadata.description, annotations: tool.metadata.annotations, - inputSchema, + inputSchema: toolInputSchema, }; });This keeps the conversion semantics as-is, but aligns better with the MCP-typed
ListToolsResultSchemaand the repo’s “noany” preference in SDK code.libs/sdk/src/prompt/__tests__/prompt.instance.test.ts (1)
456-457: Consider typing mock contexts instead ofany.The mock context objects are explicitly typed as
any, which bypasses TypeScript's type safety. Consider defining a minimal mock type that matches the expected context interface.As per coding guidelines, avoid
anytypes without strong justification and preferunknownfor generic type defaults when the actual type is unclear.// Example mock type definition type MockContext = { authInfo: { token: string }; }; const mockCtx: MockContext = { authInfo: { token: 'test-token' }, };Also applies to: 479-480
libs/sdk/src/prompt/prompt.registry.ts (1)
362-367: Consider renamingscopetochangeScopeper coding guidelines.The event object uses
scope: 'global'(line 364, 374), but the coding guidelines specify: "Use changeScope property name instead of scope for event properties to avoid confusion with Scope class."This appears to be pre-existing code, but if
PromptChangeEventis being touched, consider aligning with the naming convention. As per coding guidelines, this prevents confusion with theScopeclass used elsewhere in the codebase.if (opts.immediate) { cb({ kind: 'reset', - scope: 'global', + changeScope: 'global', version: this.version, snapshot: this.listAllInstances().filter(filter), }); }Note: This would require updating the
PromptChangeEventtype definition as well.libs/sdk/src/__test-utils__/mocks/provider-registry.mock.ts (2)
142-152: Consider usingunknowninstead ofanyfor the value parameter.Per coding guidelines, prefer
unknownoveranyfor generic type parameters. This applies to thevalueparameter.-export function createMockProviderEntry(token: Token, value: any, scope: ProviderScope = ProviderScope.GLOBAL) { +export function createMockProviderEntry(token: Token, value: unknown, scope: ProviderScope = ProviderScope.GLOBAL) {
166-174: Consider usingunknowninstead ofanyfor the value parameter.Same as above - per coding guidelines, prefer
unknownoverany.-export function addProviderToMock(registry: ProviderRegistry, token: Token, value: any) { +export function addProviderToMock(registry: ProviderRegistry, token: Token, value: unknown) {libs/plugins/package.json (1)
35-43: Pinningjson-schema-to-zod-v3– confirm versioning strategyYou’ve pinned
json-schema-to-zod-v3to"1.0.0"while other deps mostly use caret ranges. If you don’t rely on an exact version, consider^1.0.0to pick up non-breaking fixes automatically; otherwise, a brief note in the PR/commit message about why it’s pinned would help future maintainers.libs/plugins/src/codecall/tools/execute.schema.ts (1)
9-35: EnsurecallTooldocs match actual types and error codesThe description string now documents:
- Signature:
Promise<T | Result<T>>withoptions.throwOnError.- Result shape:
{ success, data, error }.- Error codes:
NOT_FOUND,VALIDATION,EXECUTION,TIMEOUT,ACCESS_DENIED.Please double‑check that:
- The named type (
Result<T>) matches the real exported type name (e.g.,ToolCallResult<T>or similar), so users aren’t confused by a type alias that doesn’t exist.- The error code strings in the table exactly match the values in
TOOL_CALL_ERROR_CODES(or equivalent), to avoid subtle drift between docs and runtime behavior.If they differ, it’s worth updating this description string to reference the canonical type name/error constants.
libs/enclave/src/lib/enclave.ts (1)
192-212: Parallel helper whitelisting – ensure it’s fully wired and boundedAllowing both
paralleland__safe_parallelinallowedGlobalsmakes sense for the new safe runtime helper, but it’s important that all pieces line up:
createSafeRuntimeshould actually define__safe_paralleland ensure it respectstimeout,maxIterations, andmaxToolCalls.- The AgentScript transform (in
transformAgentScript/ the preset) should treatparallelconsistently (e.g., routing user calls to__safe_parallel), so users can’t bypass safety by calling a raw global.- Tests (e.g., the existing security‑hardening / invoke tests) should cover that
parallelcannot be abused to circumvent loop/iteration or tool‑call limits.If those are already in place, this whitelist change looks good; otherwise, it’s worth tightening before exposing
parallelbroadly.docs/draft/docs/plugins/codecall/scaling.mdx (1)
78-96: Add import statement for ToolSearchService.The example references
ToolSearchServicebut doesn't show the import. Consider adding it for completeness:```ts +import { ToolSearchService } from '@frontmcp/plugins'; import { CodeCallPlugin } from '@frontmcp/plugins';libs/plugins/src/codecall/__tests__/invoke.tool.test.ts (1)
27-418: Consider adding constructor validation and error class instanceof checks.Per coding guidelines, test suites should include constructor validation tests and error class
instanceofchecks. Consider adding:
- Constructor validation test for
InvokeTool(if applicable)instanceofchecks for error types returned (e.g., verify error objects are instances of expected error classes if custom error classes are used)describe('Constructor Validation', () => { it('should create InvokeTool instance', () => { const tool = new InvokeTool(); expect(tool).toBeInstanceOf(InvokeTool); }); });As per coding guidelines, this ensures error class behavior is validated.
libs/plugins/src/codecall/codecall.symbol.ts (1)
62-66: Return type union creates ambiguity for callers.The return type
Promise<TResult | ToolCallResult<TResult>>doesn't encode the relationship betweenoptions.throwOnErrorand the return type. Callers must know the convention or check at runtime.Consider using function overloads to provide better type inference:
callTool: { <TInput, TResult>(name: string, input: TInput): Promise<TResult>; <TInput, TResult>(name: string, input: TInput, options: { throwOnError: true }): Promise<TResult>; <TInput, TResult>(name: string, input: TInput, options: { throwOnError: false }): Promise<ToolCallResult<TResult>>; <TInput, TResult>(name: string, input: TInput, options?: CallToolOptions): Promise<TResult | ToolCallResult<TResult>>; };This would allow TypeScript to infer the correct return type based on the
throwOnErrorvalue.libs/plugins/src/codecall/security/self-reference-guard.ts (2)
33-48: Inconsistent normalization: blocklist check uses originaltoolNameinstead ofnormalizedName.Line 38 checks the normalized (lowercase) name against the prefix, but line 43 checks the original
toolNameagainstBLOCKED_CODECALL_TOOLS. Since all blocklist entries use lowercase (codecall:*), a call likeCODECALL:SEARCHwould:
- Pass the blocklist check (original case doesn't match)
- Still be caught by the prefix check (normalized matches)
While the prefix check provides coverage, the blocklist check is effectively redundant and inconsistent. For clarity and defense-in-depth consistency:
export function isBlockedSelfReference(toolName: string): boolean { // Normalize tool name (case-insensitive check for prefix) const normalizedName = toolName.toLowerCase().trim(); // Check for codecall: prefix if (normalizedName.startsWith(CODECALL_TOOL_PREFIX)) { return true; } // Check explicit blocklist - if (BLOCKED_CODECALL_TOOLS.has(toolName)) { + if (BLOCKED_CODECALL_TOOLS.has(normalizedName)) { return true; } return false; }
13-19: Blocklist comment is misleading - all entries already have thecodecall:prefix.The comment states "Even if a tool doesn't have the prefix, these are always blocked," but all four entries in
BLOCKED_CODECALL_TOOLShave thecodecall:prefix. This makes the explicit blocklist currently redundant with the prefix check.Either:
- Update the comment to clarify this is defense-in-depth documentation
- Or add examples of non-prefixed tools that should be blocked (if any exist)
/** - * Explicitly blocked tool names for additional safety. - * Even if a tool doesn't have the prefix, these are always blocked. + * Explicitly blocked tool names for defense-in-depth. + * Currently all have the prefix, but this list serves as documentation + * and would catch any renamed/aliased CodeCall tools. */libs/plugins/src/codecall/tools/invoke.tool.ts (1)
93-107: Multipleas anycasts reduce type safety.The tool execution block uses three
as anyassertions (lines 96, 99, 102) which bypass TypeScript's type checking. Per coding guidelines,anytypes should be avoided without strong justification.Consider defining proper types for the tool context creation and execution:
- const toolContext = tool.create( - toolInput as any, - { - authInfo: this.authInfo, - } as any, - ); - - const result = await toolContext.execute(toolInput as any); + const toolContext = tool.create( + toolInput as Record<string, unknown>, + { + authInfo: this.authInfo, + }, + ); + + const result = await toolContext.execute(toolInput as Record<string, unknown>);If the SDK types don't align, consider adding a type declaration or using
unknownwith runtime validation instead ofany.libs/plugins/src/codecall/codecall.plugin.ts (2)
93-112: Consider removing or converting debug console.log statements.Multiple
console.logstatements (lines 95-96, 99, 108) are present in the hook. For a publishable SDK library, these should either be removed or replaced with a proper logging abstraction that respects log levels.async adjustListTools(flowCtx: FlowCtxOf<'tools:list-tools'>) { const { resolvedTools } = flowCtx.state; - console.log('[CodeCall] adjustListTools hook called, mode:', this.options.mode); - console.log('[CodeCall] Tools before filter:', resolvedTools?.length ?? 0); if (!resolvedTools || resolvedTools.length === 0) { - console.log('[CodeCall] No tools to filter, returning early'); return; } // Filter tools based on mode const filteredTools = resolvedTools.filter(({ tool }) => { return this.shouldShowInListTools(tool, this.options.mode); }); - console.log('[CodeCall] Tools after filter:', filteredTools.length); // Update the state with filtered tools flowCtx.state.set('resolvedTools', filteredTools); }
167-169: Type assertion chain could be improved.The double cast
(tool.metadata as any)?.codecall as CodeCallToolMetadatabypasses type checking entirely. IfToolEntry.metadatahas a known structure, consider extending the type or using a type guard.private getCodeCallMetadata(tool: ToolEntry<any, any, any, any>): CodeCallToolMetadata | undefined { const metadata = tool.metadata as { codecall?: CodeCallToolMetadata } | undefined; return metadata?.codecall; }libs/plugins/src/codecall/__tests__/tool-search.service.test.ts (1)
69-121: Good reactive subscription test coverage.The subscription tests verify initialization, reindexing on changes, and cleanup. The pattern of using
setTimeout(resolve, 0)to wait for microtasks is correct but consider extracting aflushPromiseshelper for clarity.// At the top of the file const flushPromises = () => new Promise(resolve => setTimeout(resolve, 0)); // Then use throughout: await flushPromises();libs/plugins/src/codecall/__tests__/security-hardening.test.ts (1)
66-74: Consider usingexpect.assertions()orexpect().toThrow()pattern instead offail().The
fail()function may not be available in all Jest configurations. A more idiomatic approach:- it('should include tool name in error', () => { - try { - assertNotSelfReference('codecall:execute'); - fail('Should have thrown'); - } catch (error) { - expect(error).toBeInstanceOf(SelfReferenceError); - expect((error as SelfReferenceError).toolName).toBe('codecall:execute'); - } - }); + it('should include tool name in error', () => { + expect.assertions(2); + try { + assertNotSelfReference('codecall:execute'); + } catch (error) { + expect(error).toBeInstanceOf(SelfReferenceError); + expect((error as SelfReferenceError).toolName).toBe('codecall:execute'); + } + });libs/plugins/src/codecall/services/output-sanitizer.ts (2)
97-133: Unsafe type casting with genericTparameter.The function accepts
unknowninput but casts results toTwithout validation, which defeats TypeScript's type safety. When the output is truncated or cannot be serialized, the returned value has a different shape thanT.Consider either:
- Removing the generic and returning
unknown:-export function sanitizeOutput<T = unknown>( +export function sanitizeOutput( output: unknown, config: Partial<OutputSanitizerConfig> = {}, -): SanitizationResult<T> { +): SanitizationResult<unknown> {
- Or documenting that
Tis only a hint and the caller must validate:/** * @remarks The generic T is for caller convenience only. When sanitization * modifies the structure (truncation, serialization failure), the actual * return type may differ. Callers should validate the result if type safety is critical. */
342-345: Avoidanytype - use type guard instead.Per coding guidelines, avoid
anytypes. Use a type guard to safely access thecodeproperty.// Include error code if present - if ('code' in error) { - result['code'] = (error as any).code; + if ('code' in error && error.code !== undefined) { + result['code'] = (error as { code: unknown }).code; }libs/plugins/src/codecall/security/tool-access-control.service.ts (2)
210-227: Repeated regex compilation inisInSetimpacts performance.Every call to
checkAccesstriggersisInSet, which recompiles glob patterns to regex on each iteration. For high-throughput scenarios, this could become a bottleneck.Pre-compile and cache regexes for patterns in the blacklist/whitelist sets:
private readonly blacklistSet: Set<string>; + private readonly blacklistPatternRegexes: RegExp[]; + private readonly whitelistPatternRegexes: RegExp[]; constructor(policy?: ToolAccessPolicy) { // ... this.blacklistSet = new Set([...(this.policy.blacklist || []), ...Array.from(DEFAULT_BLACKLIST)]); + + // Pre-compile glob patterns from sets + this.blacklistPatternRegexes = [...this.blacklistSet] + .filter(p => p.includes('*')) + .map(p => this.globToRegex(p)); + this.whitelistPatternRegexes = [...this.whitelistSet] + .filter(p => p.includes('*')) + .map(p => this.globToRegex(p)); } private isInSet(toolName: string, set: Set<string>): boolean { if (set.has(toolName)) { return true; } - for (const pattern of set) { - if (pattern.includes('*')) { - const regex = this.globToRegex(pattern); - if (regex.test(toolName)) { - return true; - } - } - } + const regexes = set === this.blacklistSet + ? this.blacklistPatternRegexes + : this.whitelistPatternRegexes; + for (const regex of regexes) { + if (regex.test(toolName)) { + return true; + } + } return false; }
245-247: Shallow freeze allows mutation of nested policy properties.
Object.freeze({ ...this.policy })creates a shallow freeze. Nested objects likepatterns.allowandpatterns.denyarrays remain mutable.getPolicy(): Readonly<ToolAccessPolicy> { - return Object.freeze({ ...this.policy }); + return Object.freeze({ + ...this.policy, + patterns: this.policy.patterns + ? Object.freeze({ + allow: this.policy.patterns.allow ? Object.freeze([...this.policy.patterns.allow]) : undefined, + deny: this.policy.patterns.deny ? Object.freeze([...this.policy.patterns.deny]) : undefined, + }) + : undefined, + whitelist: this.policy.whitelist ? Object.freeze([...this.policy.whitelist]) : undefined, + blacklist: this.policy.blacklist ? Object.freeze([...this.policy.blacklist]) : undefined, + }); + }libs/plugins/src/codecall/errors/tool-call.errors.ts (1)
136-163: Inconsistent immutability:ToolAccessDeniedErrorandToolNotFoundErrorare not frozen.
SelfReferenceErroris frozen viaObject.freeze(this)at line 129, butToolAccessDeniedErrorandToolNotFoundErrorare not. This inconsistency could lead to unexpected mutation of error objects.export class ToolAccessDeniedError extends Error { readonly code = TOOL_CALL_ERROR_CODES.ACCESS_DENIED; readonly toolName: string; readonly reason: string; constructor(toolName: string, reason: string) { super(`Access denied for tool "${toolName}": ${reason}`); this.name = 'ToolAccessDeniedError'; this.toolName = toolName; this.reason = reason; + Object.freeze(this); } } export class ToolNotFoundError extends Error { readonly code = TOOL_CALL_ERROR_CODES.NOT_FOUND; readonly toolName: string; constructor(toolName: string) { super(`Tool "${toolName}" not found`); this.name = 'ToolNotFoundError'; this.toolName = toolName; + Object.freeze(this); } }libs/plugins/src/codecall/services/tool-search.service.ts (2)
170-173: Unsafe double cast throughunknown.The cast
event.snapshot as unknown as ToolEntry<any, any>[]bypasses TypeScript's type checking entirely. This suggests a type mismatch that should be addressed at the source.Consider:
- Properly typing the subscription callback's event parameter
- Adding runtime validation if the type is genuinely unknown at compile time
- this.unsubscribe = this.scope.tools.subscribe({ immediate: true }, (event) => { - this.handleToolChange(event.snapshot as unknown as ToolEntry<any, any>[]); + this.unsubscribe = this.scope.tools.subscribe({ immediate: true }, (event) => { + const tools = Array.isArray(event.snapshot) ? event.snapshot : []; + this.handleToolChange(tools as ToolEntry<unknown, unknown>[]); });
294-299: Avoidanytype - use proper typing for metadata access.Per coding guidelines, avoid
anytypes. Use a type guard or explicit interface for accessing CodeCall metadata.private getCodeCallMetadata(tool: ToolEntry<any, any>): CodeCallToolMetadata | undefined { - return (tool.metadata as any)?.codecall as CodeCallToolMetadata | undefined; + const metadata = tool.metadata as { codecall?: CodeCallToolMetadata }; + return metadata.codecall; }Similarly for line 366:
if (tool.rawInputSchema && typeof tool.rawInputSchema === 'object') { - const schema = tool.rawInputSchema as any; + const schema = tool.rawInputSchema as { properties?: Record<string, unknown> }; if (schema.properties) { parts.push(...Object.keys(schema.properties)); } }libs/plugins/src/codecall/utils/describe.utils.ts (1)
95-102: Avoid non-null assertion; refactor for explicit handling.Per coding guidelines, non-null assertions (
!) should be avoided. While the logic here is sound (the condition on line 95 ensures at least one is defined), the assertion can be eliminated:if (schema.oneOf || schema.anyOf) { - // We've confirmed at least one is truthy, so use non-null assertion - const options = (schema.oneOf ?? schema.anyOf)!; - return options + const options = schema.oneOf ?? schema.anyOf ?? []; + return options .filter((s): s is JSONSchema7 => typeof s !== 'boolean') .map((s) => getTypeString(s)) .join(' | '); }The fallback to
[]is safe and maintains the same behavior while satisfying strict type checking.libs/plugins/src/codecall/tools/execute.tool.ts (3)
108-115: Avoidanytype casts; use proper typing orunknown.Per coding guidelines,
anyshould be avoided. These casts suggest the SDK'stool.create()andexecute()methods may need better type inference or the input types should be explicitly typed:const toolContext = tool.create( - toolInput as any, + toolInput as Record<string, unknown>, { authInfo: this.authInfo, - } as any, + } as { authInfo: typeof this.authInfo }, ); - const result = await toolContext.execute(toolInput as any); + const result = await toolContext.execute(toolInput as Record<string, unknown>);If the SDK types don't support this, consider filing an issue to improve the type definitions.
238-259: Useunknowninstead ofanyfor caught errors.Per coding guidelines, use
unknownfor generic types without constraints:- } catch (error: any) { + } catch (error: unknown) { // Check for syntax errors - if (error.name === 'SyntaxError' || error.message?.includes('syntax')) { + const err = error instanceof Error ? error : new Error(String(error)); + if (err.name === 'SyntaxError' || err.message?.includes('syntax')) { return { status: 'syntax_error', error: { - message: error.message || 'Syntax error in script', - location: error.loc ? { line: error.loc.line, column: error.loc.column } : undefined, + message: err.message || 'Syntax error in script', + location: 'loc' in err && err.loc ? { line: (err.loc as { line: number; column: number }).line, column: (err.loc as { line: number; column: number }).column } : undefined, }, }; } // Unexpected error during execution return { status: 'runtime_error', error: { source: 'script', - message: error.message || 'An unexpected error occurred during script execution', - name: error.name || 'Error', - stack: error.stack, + message: err.message || 'An unexpected error occurred during script execution', + name: err.name || 'Error', + stack: err.stack, }, }; }
191-192: Avoid non-null assertion on error object.The
!assertion onexecutionResult.errorshould be avoided per coding guidelines:if (!executionResult.success) { - const error = executionResult.error!; + const error = executionResult.error; + if (!error) { + return { + status: 'runtime_error', + error: { + source: 'script', + message: 'An unknown error occurred', + name: 'Error', + }, + }; + }This provides a safe fallback if the error is unexpectedly undefined.
libs/plugins/src/codecall/services/audit-logger.service.ts (2)
154-159: Consider documenting that execution IDs are for correlation only.
Math.random()is not cryptographically secure, which is fine for correlation IDs but worth documenting. Consider adding a note:/** * Generate a unique execution ID. + * + * Note: These IDs are for correlation purposes only, not for security. + * They should not be used as tokens or secrets. */ generateExecutionId(): string {
406-424: Duplicated sanitization logic with ErrorEnrichmentService.This
sanitizeErrormethod is nearly identical toformatMessageinerror-enrichment.service.ts(lines 384-411). Consider extracting this to a shared utility to maintain DRY:// In a shared utility file export function sanitizeErrorMessage(error: string, maxLength = 200): string { if (!error) return 'Unknown error'; let sanitized = error.replace(/(?:\/[\w.-]+)+|(?:[A-Za-z]:\\[\w\\.-]+)+/g, '[path]'); sanitized = sanitized.replace(/:\d+:\d+/g, ''); sanitized = sanitized.replace(/\n\s*at .*/g, ''); if (sanitized.length > maxLength) { sanitized = sanitized.substring(0, maxLength) + '...'; } return sanitized.trim(); }Both services could then import and use this shared function.
… for security vulnerabilities
There was a problem hiding this comment.
Actionable comments posted: 7
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/tool/flows/call-tool.flow.ts (1)
97-100: Avoid mutatingrawInputinside flows; store_toolOwnerIdin state insteadHere you’re enriching
this.rawInputwith_toolOwnerId:if (tool?.owner) { (this.rawInput as any)._toolOwnerId = tool.owner.id; }Per the SDK flow guidelines,
rawInputshould be treated as immutable, and additional per-flow data should go throughstate.set(...). Consider instead persisting the owner ID in state (and updating any guards/hooks that currently read it fromrawInput):- if (tool?.owner) { - (this.rawInput as any)._toolOwnerId = tool.owner.id; - } + if (tool?.owner) { + this.state.set('toolOwnerId', tool.owner.id); + }This keeps the original request payload untouched and centralizes flow-specific data in the state object, which is consistent with the rest of the flows. Based on learnings,
rawInputmutation in flows should be avoided in favor ofstate.set.
♻️ Duplicate comments (2)
libs/plugins/src/codecall/services/tool-search.service.ts (2)
162-176: Potential infinite loop ifscope.toolsis never initialized.The
setupSubscriptionmethod retries viaqueueMicrotaskifscope.toolsis not available, but there's no limit on retries. Ifscope.toolsnever becomes available, this creates an infinite microtask loop.This was previously flagged. Consider adding a retry limit:
+ private subscriptionRetries = 0; + private static readonly MAX_SUBSCRIPTION_RETRIES = 100; + private setupSubscription(): void { if (!this.scope.tools) { + if (this.subscriptionRetries++ >= ToolSearchService.MAX_SUBSCRIPTION_RETRIES) { + console.warn('ToolSearchService: scope.tools not available after max retries'); + return; + } queueMicrotask(() => this.setupSubscription()); return; } + this.subscriptionRetries = 0;
381-505: Stop words Set recreated on every call - move to module level.The
stopWordsSet is recreated on every call toisStopWord, which is called multiple times per tool during indexing. For large tool registries, this creates significant allocation overhead.+const STOP_WORDS: ReadonlySet<string> = new Set([ + 'the', 'a', 'an', 'and', 'or', 'but', 'in', 'on', 'at', 'to', 'for', 'of', + // ... rest of the words +]); + export class ToolSearchService implements ToolSearch { // ... private isStopWord(word: string): boolean { - const stopWords = new Set([...]); - return stopWords.has(word); + return STOP_WORDS.has(word); }
🧹 Nitpick comments (19)
libs/sdk/src/tool/flows/call-tool.flow.ts (1)
71-80: Clarifying comment onanyis good; consider optional type refinement forparams/ctxThe new comment makes the intentional
anyusage much clearer, which is helpful for future readers. Given the Zod-based validation, this is a reasonable trade-off.If you want to tighten types later without adding redundant guards, one option is to derive the types directly from
inputSchemainstead of using rawany, e.g. something along the lines of:type ParsedInput = z.infer<typeof inputSchema>; let method!: ParsedInput['request']['method']; let params: ParsedInput['request']['params']; let ctx: ParsedInput['ctx'];You might also consider making
ctxa structured object ininputSchema(with at leastauthInfo?: AuthInfo) so thatctx.authInfois guaranteed safe to access. This is purely a polish / safety improvement and not required for this PR. As per coding guidelines, this would further reduce reliance onanywhile keeping the runtime behavior unchanged.libs/ast-guard/src/pre-scanner/mandatory-limits.ts (1)
101-115: JSDoc mentionsConfigurationErrorbut implementation throws genericError.The JSDoc on line 101 states
@throws ConfigurationErrorbut the implementation on line 111 throws a genericError. For consistency with the documentation and to enable better error handling by consumers, consider either updating the JSDoc or implementing a dedicatedConfigurationErrorclass.- throw new Error( + throw new ConfigurationError( `Configuration error: ${limitKey} (${value}) exceeds mandatory maximum (${MANDATORY_LIMITS[limitKey]}). ` + `This limit cannot be exceeded for security reasons.`, );libs/ast-guard/src/pre-scanner/checks/unicode-check.ts (1)
159-160: Consider making the invisible character threshold configurable.The magic number
3for the invisible character threshold is hardcoded. Consider extracting this to a constant or making it configurable viaPreScannerConfigfor consistency with other configurable limits in the pre-scanner.+// Threshold for suspicious invisible character count +const INVISIBLE_CHAR_THRESHOLD = 3; + // Report if suspicious number of invisible chars (more than 3 is suspicious) -if (matches.length > 3) { +if (matches.length > INVISIBLE_CHAR_THRESHOLD) {libs/ast-guard/src/rules/no-regex-literal.rule.ts (2)
119-172: Extract duplicated TemplateLiteral handling logic.The
NewExpression(lines 119-145) andCallExpression(lines 148-172) handlers contain nearly identical logic for handlingTemplateLiteralarguments. Consider extracting this to a shared helper method to reduce duplication and improve maintainability.+ private handleRegExpConstruction(node: any, context: ValidationContext): void { + const patternArg = node.arguments[0]; + if (patternArg?.type === 'Literal' && typeof patternArg.value === 'string') { + const flagsArg = node.arguments[1]; + const flags = flagsArg?.type === 'Literal' && typeof flagsArg.value === 'string' ? flagsArg.value : ''; + this.checkRegex(node, patternArg.value, flags, context); + } else if (patternArg?.type === 'TemplateLiteral') { + if (this.options.blockAll) { + context.report({ + code: 'NO_REGEX_LITERAL', + message: 'Dynamic RegExp with template literal is not allowed in this security mode', + location: this.getLocation(node), + }); + } else { + context.report({ + code: 'REGEX_DYNAMIC_PATTERN', + severity: ValidationSeverity.WARNING, + message: 'RegExp with dynamic pattern cannot be analyzed for ReDoS vulnerabilities', + location: this.getLocation(node), + }); + } + } + }
240-247: Potential ReDoS in allowedPatterns matching.When
allowedPatternscontains aRegExp, callingallowed.test(pattern)executes that regex against the input pattern. If users configure a vulnerable regex inallowedPatterns, this could be exploited. Consider documenting this risk or adding a timeout/safe-regex check for RegExp entries inallowedPatterns.private isAllowedPattern(pattern: string): boolean { return this.options.allowedPatterns.some((allowed) => { if (typeof allowed === 'string') { return pattern === allowed; } + // Note: User-provided RegExp in allowedPatterns should be simple patterns + // to avoid ReDoS in the security rule itself return allowed.test(pattern); }); }libs/ast-guard/docs/SECURITY-AUDIT.md (1)
3-679: LGTM! Comprehensive documentation of Layer 0 Pre-Scanner.The audit report is well-structured and accurately documents:
- Updated test coverage (579 tests including 64 pre-scanner tests)
- New Layer 0 Pre-Scanner with mandatory limits and preset modes
- Attack vectors blocked by the pre-scanner (ReDoS, BiDi, parser DoS)
- Updated security visualization showing defense-in-depth
- Change log tracking additions
The documentation clearly explains the rationale and security benefits of the new pre-scanner layer.
Optional style improvements:
Line 56: Consider rephrasing "Extremely large inputs" to "Very large inputs" or "Excessive input sizes"
Line 57: Consider "different from" instead of "different than" for formal stylelibs/ast-guard/src/pre-scanner/checks/nesting-check.ts (2)
57-60: Escape handling may miss template string escapes.The escape detection on line 57 only triggers when
inStringis true, but template strings (inTemplateString) can also contain escape sequences (e.g.,\`or\$). This could cause the scanner to incorrectly interpret an escaped backtick as the end of a template string.Since this is documented as a heuristic check (line 28-30), this may be acceptable, but consider extending the condition:
- if (char === '\\' && inString) { + if (char === '\\' && (inString || inTemplateString)) { escaped = true; continue; }
176-237: Template string interpolation not tracked in operator check.Unlike
checkNestingDepth, this function tracks string boundaries but doesn't handle template string interpolations. Operators inside${...}expressions would be counted while the function thinks it's still "in a string" after seeing the opening backtick.Given this is a heuristic pre-scanner and the template interpolation edge case is unlikely in attack patterns, this is acceptable, but worth documenting.
libs/ast-guard/src/pre-scanner/checks/string-check.ts (2)
84-92: Minor: Redundant character check on line 89.Line 89 checks
c === '\'but sinceisTemplateis only true whenquote === '`', you could simplify toc === quote`:- if (c === '`' && isTemplate && templateDepth === 0) { + if (c === quote && isTemplate && templateDepth === 0) {This makes the template and regular string end-checks symmetric.
177-184: Minor: Parameter namebytesis misleading for character counts.The function formats string literal lengths which are counted in characters (not bytes). The parameter name
bytescould be confusing:-function formatSize(bytes: number): string { - if (bytes < 1024) return `${bytes} chars`; - if (bytes < 1024 * 1024) return `${(bytes / 1024).toFixed(1)}KB`; - return `${(bytes / (1024 * 1024)).toFixed(1)}MB`; +function formatSize(chars: number): string { + if (chars < 1024) return `${chars} chars`; + if (chars < 1024 * 1024) return `${(chars / 1024).toFixed(1)}KB`; + return `${(chars / (1024 * 1024)).toFixed(1)}MB`; }libs/ast-guard/src/__tests__/pre-scanner.spec.ts (3)
435-451: Consider addinginstanceofchecks for error class tests.Per coding guidelines, error class tests should include
instanceofchecks to verify proper inheritance:it('should create with code and message', () => { const error = new PreScannerError('Test error', 'PRESCANNER_INPUT_TOO_LARGE'); expect(error.message).toBe('Test error'); expect(error.errorCode).toBe('PRESCANNER_INPUT_TOO_LARGE'); expect(error.name).toBe('PreScannerError'); + expect(error).toBeInstanceOf(Error); + expect(error).toBeInstanceOf(PreScannerError); });Based on learnings: "Include constructor validation tests and error class instanceof checks in all test suites."
255-265: Consider addingquickValidatetest for invalid code.The tests cover valid and empty inputs, but could also verify behavior with code that fails scanning (e.g., null byte):
it('should return false for code with null bytes', () => { const scanner = new PreScanner({ preset: 'standard' }); expect(scanner.quickValidate('const x = "\0";')).toBe(false); });This would increase coverage of error paths.
461-487: Consider adding tests forreportFatalErrorandshouldStop.The
ScanStatetests coverreportIssueandfinalize, but could also verify:
reportFatalErrorsets the fatal issueshouldStop()returns true after fatal errorisPassing()returns false after fatal errorit('should track fatal errors via reportFatalError', () => { const state = new ScanState(); state.reportFatalError({ code: 'PRESCANNER_INPUT_TOO_LARGE', message: 'Test fatal', }); expect(state.shouldStop()).toBe(true); expect(state.isPassing()).toBe(false); const result = state.finalize(); expect(result.success).toBe(false); expect(result.fatalIssue).toBeDefined(); });libs/ast-guard/src/validator.ts (1)
128-132: Avoidas anycast for error code.The
as anycast on line 130 violates the coding guidelines requiring strict type checking. Consider using a proper type assertion or adjusting thePreScannerErrorconstructor signature.- preScanError = new PreScannerError( - preScanResult.fatalIssue?.message ?? 'Pre-scan failed', - (preScanResult.fatalIssue?.code as any) ?? 'PRESCANNER_UNKNOWN', - { position: preScanResult.fatalIssue?.position, line: preScanResult.fatalIssue?.line }, - ); + preScanError = new PreScannerError( + preScanResult.fatalIssue?.message ?? 'Pre-scan failed', + (preScanResult.fatalIssue?.code ?? 'PRESCANNER_UNKNOWN') as PreScannerErrorCode, + { position: preScanResult.fatalIssue?.position, line: preScanResult.fatalIssue?.line }, + );You'll need to import
PreScannerErrorCodefrom./pre-scannerand ensure'PRESCANNER_UNKNOWN'is a valid value of that type, or add it to the error codes. Based on learnings, avoidanytypes without strong justification.libs/ast-guard/src/pre-scanner/checks/regex-check.ts (1)
372-474: ReDoS analysis logic is reasonable but has inverted level semantics.The
levelparameter semantics seem inverted: whenlevel === 'polynomial', additional polynomial checks run. This means'catastrophic'skips polynomial checks, which is counterintuitive since catastrophic mode should be more thorough. Consider renaming or inverting the logic.If
'catastrophic'is meant to be the stricter mode, the polynomial checks should run whenlevel === 'catastrophic':- if (level === 'polynomial') { + if (level === 'catastrophic') {Or rename the levels to clarify intent (e.g.,
'basic'vs'thorough').libs/ast-guard/src/pre-scanner/errors.ts (2)
14-51: Missing factory functions for several error codes.The
PRESCANNER_ERROR_CODESobject defines error codes that lack corresponding factory functions inPreScannerErrors:
EXCESSIVE_OPERATORS(line 26)STRING_TOO_LONG(line 36)STRING_TOTAL_EXCEEDED(line 38)UNICODE_SUSPICIOUS(line 42)HOMOGRAPH_ATTACK(line 44)This could lead to inconsistent error creation when these codes are needed elsewhere in the codebase.
Add the missing factory functions to
PreScannerErrors:+ excessiveOperators(count: number, limit: number, position: number): PreScannerError { + return new PreScannerError( + `Consecutive operators (${count}) exceeds maximum allowed (${limit})`, + PRESCANNER_ERROR_CODES.EXCESSIVE_OPERATORS, + { actual: count, limit, position }, + ); + }, + + stringTooLong(length: number, limit: number, position: number): PreScannerError { + return new PreScannerError( + `String literal length (${length}) exceeds maximum allowed (${limit})`, + PRESCANNER_ERROR_CODES.STRING_TOO_LONG, + { actual: length, limit, position }, + ); + }, + + stringTotalExceeded(total: number, limit: number): PreScannerError { + return new PreScannerError( + `Total string content (${formatBytes(total)}) exceeds maximum allowed (${formatBytes(limit)})`, + PRESCANNER_ERROR_CODES.STRING_TOTAL_EXCEEDED, + { actual: total, limit }, + ); + }, + + unicodeSuspicious(position: number, line?: number): PreScannerError { + return new PreScannerError( + 'Suspicious Unicode characters detected', + PRESCANNER_ERROR_CODES.UNICODE_SUSPICIOUS, + { position, line }, + ); + }, + + homographAttack(position: number, line?: number): PreScannerError { + return new PreScannerError( + 'Homograph attack detected: confusable characters found', + PRESCANNER_ERROR_CODES.HOMOGRAPH_ATTACK, + { position, line }, + ); + },
281-285: Consider handling edge cases informatBytes.The function handles the common cases well, but could be improved for edge cases like negative values or very large numbers beyond MB.
function formatBytes(bytes: number): string { + if (bytes < 0) return '0B'; if (bytes < 1024) return `${bytes}B`; if (bytes < 1024 * 1024) return `${(bytes / 1024).toFixed(1)}KB`; + if (bytes < 1024 * 1024 * 1024) return `${(bytes / (1024 * 1024)).toFixed(1)}MB`; - return `${(bytes / (1024 * 1024)).toFixed(1)}MB`; + return `${(bytes / (1024 * 1024 * 1024)).toFixed(1)}GB`; }libs/ast-guard/src/pre-scanner/config.ts (1)
307-312: Consider using explicitundefinedcheck for optional field clamping.The truthy check (
merged.maxRegexLength ?) works correctly since 0 is a valid value that doesn't need clamping. However, using an explicit check improves readability and intent.- maxRegexLength: merged.maxRegexLength - ? clampToMandatoryLimit('ABSOLUTE_MAX_REGEX_LENGTH', merged.maxRegexLength) - : merged.maxRegexLength, - maxRegexCount: merged.maxRegexCount - ? clampToMandatoryLimit('ABSOLUTE_MAX_REGEX_COUNT', merged.maxRegexCount) - : merged.maxRegexCount, + maxRegexLength: merged.maxRegexLength !== undefined + ? clampToMandatoryLimit('ABSOLUTE_MAX_REGEX_LENGTH', merged.maxRegexLength) + : undefined, + maxRegexCount: merged.maxRegexCount !== undefined + ? clampToMandatoryLimit('ABSOLUTE_MAX_REGEX_COUNT', merged.maxRegexCount) + : undefined,libs/plugins/src/codecall/tools/execute.tool.ts (1)
27-42: Consider using error type/instance checks over string matching.The
getErrorCodefunction uses string matching (error.message?.includes('validation')) which is fragile. If error messages change, classification may break.Consider checking error types directly where possible:
function getErrorCode(error: unknown): ToolCallErrorCode { if (!(error instanceof Error)) { return TOOL_CALL_ERROR_CODES.EXECUTION; } - if (error.name === 'ZodError' || error.message?.includes('validation')) { + // Check for Zod errors by constructor name (works with minified code) + if (error.name === 'ZodError' || error.constructor.name === 'ZodError') { return TOOL_CALL_ERROR_CODES.VALIDATION; } - if (error.message?.includes('timeout') || error.message?.includes('timed out')) { + // Check for timeout errors (consider adding a TimeoutError class) + if (error.name === 'TimeoutError' || error.message?.includes('timeout')) { return TOOL_CALL_ERROR_CODES.TIMEOUT; } return TOOL_CALL_ERROR_CODES.EXECUTION; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
docs/draft/blog/11-2025/codecall-plugin.mdx(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/ast-guard/docs/SECURITY-AUDIT.md(6 hunks)libs/ast-guard/package.json(1 hunks)libs/ast-guard/src/__tests__/advanced-security.spec.ts(36 hunks)libs/ast-guard/src/__tests__/pre-scanner.spec.ts(1 hunks)libs/ast-guard/src/__tests__/unicode-security.spec.ts(43 hunks)libs/ast-guard/src/index.ts(4 hunks)libs/ast-guard/src/interfaces.ts(3 hunks)libs/ast-guard/src/pre-scanner/checks/index.ts(1 hunks)libs/ast-guard/src/pre-scanner/checks/nesting-check.ts(1 hunks)libs/ast-guard/src/pre-scanner/checks/regex-check.ts(1 hunks)libs/ast-guard/src/pre-scanner/checks/size-check.ts(1 hunks)libs/ast-guard/src/pre-scanner/checks/string-check.ts(1 hunks)libs/ast-guard/src/pre-scanner/checks/unicode-check.ts(1 hunks)libs/ast-guard/src/pre-scanner/config.ts(1 hunks)libs/ast-guard/src/pre-scanner/errors.ts(1 hunks)libs/ast-guard/src/pre-scanner/index.ts(1 hunks)libs/ast-guard/src/pre-scanner/mandatory-limits.ts(1 hunks)libs/ast-guard/src/pre-scanner/pre-scanner.ts(1 hunks)libs/ast-guard/src/pre-scanner/scan-state.ts(1 hunks)libs/ast-guard/src/presets/agentscript.preset.ts(2 hunks)libs/ast-guard/src/presets/secure.preset.ts(2 hunks)libs/ast-guard/src/presets/strict.preset.ts(2 hunks)libs/ast-guard/src/rules/index.ts(1 hunks)libs/ast-guard/src/rules/no-regex-literal.rule.ts(1 hunks)libs/ast-guard/src/rules/no-regex-methods.rule.ts(1 hunks)libs/ast-guard/src/validator.ts(4 hunks)libs/plugins/src/codecall/services/tool-search.service.ts(10 hunks)libs/plugins/src/codecall/tools/execute.tool.ts(8 hunks)libs/plugins/src/codecall/tools/search.schema.ts(1 hunks)libs/plugins/src/codecall/tools/search.tool.ts(3 hunks)libs/sdk/src/common/interfaces/execution-context.interface.ts(1 hunks)libs/sdk/src/common/records/prompt.record.ts(1 hunks)libs/sdk/src/common/records/resource.record.ts(2 hunks)libs/sdk/src/common/records/tool.record.ts(2 hunks)libs/sdk/src/tool/flows/call-tool.flow.ts(1 hunks)libs/sdk/src/tool/tool.registry.ts(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- libs/sdk/src/common/records/tool.record.ts
- libs/sdk/src/common/records/resource.record.ts
- libs/sdk/src/common/records/prompt.record.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/tool/tool.registry.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/ast-guard/src/rules/index.tslibs/ast-guard/src/pre-scanner/scan-state.tslibs/ast-guard/src/__tests__/unicode-security.spec.tslibs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/interfaces.tslibs/plugins/src/codecall/tools/search.schema.tslibs/sdk/src/common/interfaces/execution-context.interface.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/pre-scanner/mandatory-limits.tslibs/ast-guard/src/pre-scanner/checks/string-check.tslibs/ast-guard/src/pre-scanner/checks/nesting-check.tslibs/ast-guard/src/rules/no-regex-methods.rule.tslibs/ast-guard/src/validator.tslibs/ast-guard/src/rules/no-regex-literal.rule.tslibs/ast-guard/src/presets/strict.preset.tslibs/plugins/src/codecall/tools/execute.tool.tslibs/plugins/src/codecall/services/tool-search.service.tslibs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/index.tslibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/pre-scanner/checks/unicode-check.tslibs/ast-guard/src/pre-scanner/checks/regex-check.tslibs/ast-guard/src/pre-scanner/config.tslibs/ast-guard/src/pre-scanner/errors.tslibs/ast-guard/src/pre-scanner/checks/size-check.tslibs/ast-guard/src/__tests__/advanced-security.spec.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/ast-guard/src/pre-scanner/checks/index.tslibs/ast-guard/src/index.tslibs/plugins/src/codecall/tools/search.tool.ts
libs/ast-guard/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Files:
libs/ast-guard/src/rules/index.tslibs/ast-guard/src/pre-scanner/scan-state.tslibs/ast-guard/src/__tests__/unicode-security.spec.tslibs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/interfaces.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/pre-scanner/mandatory-limits.tslibs/ast-guard/src/pre-scanner/checks/string-check.tslibs/ast-guard/src/pre-scanner/checks/nesting-check.tslibs/ast-guard/src/rules/no-regex-methods.rule.tslibs/ast-guard/src/validator.tslibs/ast-guard/src/rules/no-regex-literal.rule.tslibs/ast-guard/src/presets/strict.preset.tslibs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/index.tslibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/pre-scanner/checks/unicode-check.tslibs/ast-guard/src/pre-scanner/checks/regex-check.tslibs/ast-guard/src/pre-scanner/config.tslibs/ast-guard/src/pre-scanner/errors.tslibs/ast-guard/src/pre-scanner/checks/size-check.tslibs/ast-guard/src/__tests__/advanced-security.spec.tslibs/ast-guard/src/pre-scanner/checks/index.tslibs/ast-guard/src/index.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/ast-guard/src/rules/index.tslibs/ast-guard/src/pre-scanner/scan-state.tslibs/ast-guard/src/__tests__/unicode-security.spec.tslibs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/interfaces.tslibs/plugins/src/codecall/tools/search.schema.tslibs/sdk/src/common/interfaces/execution-context.interface.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/pre-scanner/mandatory-limits.tslibs/ast-guard/src/pre-scanner/checks/string-check.tslibs/ast-guard/src/pre-scanner/checks/nesting-check.tslibs/ast-guard/src/rules/no-regex-methods.rule.tslibs/ast-guard/package.jsonlibs/ast-guard/src/validator.tslibs/ast-guard/src/rules/no-regex-literal.rule.tslibs/ast-guard/src/presets/strict.preset.tslibs/plugins/src/codecall/tools/execute.tool.tslibs/plugins/src/codecall/services/tool-search.service.tslibs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/index.tslibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/pre-scanner/checks/unicode-check.tslibs/ast-guard/src/pre-scanner/checks/regex-check.tslibs/ast-guard/docs/SECURITY-AUDIT.mdlibs/ast-guard/src/pre-scanner/config.tslibs/ast-guard/src/pre-scanner/errors.tslibs/ast-guard/src/pre-scanner/checks/size-check.tslibs/ast-guard/src/__tests__/advanced-security.spec.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/ast-guard/src/pre-scanner/checks/index.tslibs/ast-guard/src/index.tslibs/plugins/src/codecall/tools/search.tool.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead ofunknownfor SDK methods
Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Use changeScope property name instead of scope for event properties to avoid confusion with Scope class
Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Do not mutate rawInput in flows - use state.set() for flow state management instead
Files:
libs/sdk/src/common/interfaces/execution-context.interface.tslibs/sdk/src/tool/flows/call-tool.flow.ts
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/blog/11-2025/codecall-plugin.mdxdocs/draft/docs/plugins/codecall/security.mdx
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/codecall/security.mdx
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document known limitations clearly and write comprehensive security documentation for validation libraries
Files:
libs/ast-guard/docs/SECURITY-AUDIT.md
**/{libs,apps}/**/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/{libs,apps}/**/src/index.ts: Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Files:
libs/ast-guard/src/index.ts
🧠 Learnings (22)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/ast-guard/src/rules/index.tslibs/ast-guard/src/pre-scanner/scan-state.tslibs/ast-guard/src/__tests__/unicode-security.spec.tslibs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/interfaces.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/pre-scanner/mandatory-limits.tslibs/ast-guard/src/pre-scanner/checks/string-check.tslibs/ast-guard/src/pre-scanner/checks/nesting-check.tslibs/ast-guard/src/rules/no-regex-methods.rule.tslibs/ast-guard/package.jsonlibs/ast-guard/src/validator.tslibs/ast-guard/src/rules/no-regex-literal.rule.tslibs/ast-guard/src/presets/strict.preset.tslibs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/index.tslibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/pre-scanner/checks/unicode-check.tslibs/ast-guard/src/pre-scanner/checks/regex-check.tslibs/ast-guard/docs/SECURITY-AUDIT.mdlibs/ast-guard/src/pre-scanner/config.tslibs/ast-guard/src/pre-scanner/errors.tslibs/ast-guard/src/pre-scanner/checks/size-check.tslibs/ast-guard/src/__tests__/advanced-security.spec.tslibs/ast-guard/src/pre-scanner/checks/index.tslibs/ast-guard/src/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Applied to files:
libs/ast-guard/src/rules/index.tslibs/ast-guard/src/pre-scanner/index.tslibs/ast-guard/src/pre-scanner/checks/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/ast-guard/src/rules/index.tslibs/ast-guard/src/pre-scanner/index.tslibs/ast-guard/src/pre-scanner/checks/index.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
libs/ast-guard/src/__tests__/unicode-security.spec.tslibs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/__tests__/advanced-security.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/ast-guard/src/__tests__/unicode-security.spec.tslibs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/__tests__/advanced-security.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Include constructor validation tests and error class instanceof checks in all test suites
Applied to files:
libs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/__tests__/advanced-security.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and edge conditions
Applied to files:
libs/ast-guard/src/__tests__/pre-scanner.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Use Jest with 95%+ coverage requirement enforced through test configuration
Applied to files:
libs/ast-guard/src/__tests__/pre-scanner.spec.tslibs/ast-guard/src/__tests__/advanced-security.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/ast-guard/src/__tests__/pre-scanner.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/ast-guard/src/__tests__/pre-scanner.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/plugins/src/codecall/tools/search.schema.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 using isValidMcpUri refinement in Zod schemas at metadata level
Applied to files:
libs/plugins/src/codecall/tools/search.schema.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Applied to files:
libs/sdk/src/common/interfaces/execution-context.interface.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Use `unknown` instead of `any` for generic type parameters with no constraint or default fallback
Applied to files:
libs/sdk/src/common/interfaces/execution-context.interface.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/ast-guard/src/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Use strict type checking in TypeScript - avoid `any` types without strong justification, prefer `unknown` for generic type defaults
Applied to files:
libs/sdk/src/common/interfaces/execution-context.interface.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/ast-guard/src/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Use type parameters with constraints instead of unconstrained generic types
Applied to files:
libs/sdk/src/common/interfaces/execution-context.interface.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Build must complete without TypeScript warnings using strict mode
Applied to files:
libs/ast-guard/package.jsonlibs/ast-guard/src/__tests__/advanced-security.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdxlibs/ast-guard/docs/SECURITY-AUDIT.md
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/ast-guard/src/pre-scanner/errors.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Do not use prefixes like PT-001 in test names - use descriptive test names instead
Applied to files:
libs/ast-guard/src/__tests__/advanced-security.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows match entry types and fail fast with InvalidHookFlowError for unsupported flows
Applied to files:
libs/sdk/src/tool/flows/call-tool.flow.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Do not mutate rawInput in flows - use state.set() for flow state management instead
Applied to files:
libs/sdk/src/tool/flows/call-tool.flow.ts
🧬 Code graph analysis (13)
libs/ast-guard/src/__tests__/unicode-security.spec.ts (1)
libs/ast-guard/src/validator.ts (1)
JSAstValidator(18-319)
libs/ast-guard/src/__tests__/pre-scanner.spec.ts (8)
libs/ast-guard/src/index.ts (17)
PreScanner(124-124)PRESCANNER_ERROR_CODES(128-128)preScan(125-125)isPreScanValid(126-126)MANDATORY_LIMITS(127-127)exceedsMandatoryLimit(142-142)clampToMandatoryLimit(143-143)getPreScannerPreset(140-140)createPreScannerConfig(141-141)AGENTSCRIPT_PRESCANNER_CONFIG(135-135)STANDARD_PRESCANNER_CONFIG(138-138)STRICT_PRESCANNER_CONFIG(136-136)SECURE_PRESCANNER_CONFIG(137-137)PERMISSIVE_PRESCANNER_CONFIG(139-139)analyzeForReDoS(147-147)REDOS_THRESHOLDS(146-146)ScanState(133-133)libs/ast-guard/src/pre-scanner/index.ts (17)
PreScanner(13-13)PRESCANNER_ERROR_CODES(43-43)preScan(13-13)isPreScanValid(13-13)MANDATORY_LIMITS(17-17)exceedsMandatoryLimit(19-19)clampToMandatoryLimit(20-20)getPreScannerPreset(37-37)createPreScannerConfig(38-38)AGENTSCRIPT_PRESCANNER_CONFIG(32-32)STANDARD_PRESCANNER_CONFIG(35-35)STRICT_PRESCANNER_CONFIG(33-33)SECURE_PRESCANNER_CONFIG(34-34)PERMISSIVE_PRESCANNER_CONFIG(36-36)analyzeForReDoS(67-67)REDOS_THRESHOLDS(65-65)ScanState(59-59)libs/ast-guard/src/pre-scanner/pre-scanner.ts (3)
PreScanner(76-233)preScan(250-253)isPreScanValid(262-265)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)libs/ast-guard/src/pre-scanner/mandatory-limits.ts (3)
MANDATORY_LIMITS(14-68)exceedsMandatoryLimit(83-85)clampToMandatoryLimit(95-97)libs/ast-guard/src/pre-scanner/config.ts (7)
getPreScannerPreset(283-285)createPreScannerConfig(291-314)AGENTSCRIPT_PRESCANNER_CONFIG(148-171)STANDARD_PRESCANNER_CONFIG(224-243)STRICT_PRESCANNER_CONFIG(176-195)SECURE_PRESCANNER_CONFIG(200-219)PERMISSIVE_PRESCANNER_CONFIG(249-267)libs/ast-guard/src/pre-scanner/checks/regex-check.ts (2)
analyzeForReDoS(372-474)REDOS_THRESHOLDS(35-42)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)
libs/ast-guard/src/interfaces.ts (1)
libs/ast-guard/src/index.ts (1)
PreScanConfig(30-30)
libs/ast-guard/src/pre-scanner/mandatory-limits.ts (2)
libs/ast-guard/src/index.ts (4)
MANDATORY_LIMITS(127-127)MandatoryLimitKey(170-170)exceedsMandatoryLimit(142-142)clampToMandatoryLimit(143-143)libs/ast-guard/src/pre-scanner/index.ts (5)
MANDATORY_LIMITS(17-17)MandatoryLimitKey(18-18)exceedsMandatoryLimit(19-19)clampToMandatoryLimit(20-20)validateLimitsAgainstMandatory(21-21)
libs/ast-guard/src/pre-scanner/checks/string-check.ts (3)
libs/ast-guard/src/pre-scanner/config.ts (1)
PreScannerConfig(25-127)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)
libs/ast-guard/src/pre-scanner/checks/nesting-check.ts (3)
libs/ast-guard/src/pre-scanner/config.ts (1)
PreScannerConfig(25-127)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)
libs/ast-guard/src/presets/strict.preset.ts (3)
libs/ast-guard/src/index.ts (2)
NoRegexLiteralRule(61-61)NoRegexMethodsRule(62-62)libs/ast-guard/src/rules/no-regex-literal.rule.ts (1)
NoRegexLiteralRule(87-257)libs/ast-guard/src/rules/no-regex-methods.rule.ts (1)
NoRegexMethodsRule(92-263)
libs/ast-guard/src/pre-scanner/pre-scanner.ts (9)
libs/ast-guard/src/pre-scanner/config.ts (4)
PreScannerPresetLevel(137-137)PreScannerConfig(25-127)createPreScannerConfig(291-314)getPreScannerPreset(283-285)libs/ast-guard/src/pre-scanner/scan-state.ts (2)
PreScanResult(78-89)ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)libs/ast-guard/src/pre-scanner/checks/index.ts (5)
performSizeChecks(9-9)performUnicodeChecks(33-33)performNestingChecks(12-12)performRegexChecks(19-19)performStringChecks(26-26)libs/ast-guard/src/pre-scanner/checks/size-check.ts (1)
performSizeChecks(121-132)libs/ast-guard/src/pre-scanner/checks/unicode-check.ts (1)
performUnicodeChecks(265-281)libs/ast-guard/src/pre-scanner/checks/nesting-check.ts (1)
performNestingChecks(246-251)libs/ast-guard/src/pre-scanner/checks/regex-check.ts (1)
performRegexChecks(524-526)libs/ast-guard/src/pre-scanner/checks/string-check.ts (1)
performStringChecks(173-175)
libs/ast-guard/src/presets/secure.preset.ts (2)
libs/ast-guard/src/index.ts (2)
NoRegexLiteralRule(61-61)NoRegexMethodsRule(62-62)libs/ast-guard/src/rules/no-regex-literal.rule.ts (1)
NoRegexLiteralRule(87-257)
libs/ast-guard/src/pre-scanner/checks/regex-check.ts (5)
libs/ast-guard/src/pre-scanner/checks/index.ts (7)
REDOS_PATTERNS(20-20)REDOS_THRESHOLDS(21-21)ReDoSAnalysisResult(22-22)detectRegexLiterals(16-16)analyzeForReDoS(17-17)calculateStarHeight(18-18)performRegexChecks(19-19)libs/ast-guard/src/pre-scanner/index.ts (8)
REDOS_PATTERNS(64-64)REDOS_THRESHOLDS(65-65)ReDoSAnalysisResult(66-66)PreScannerConfig(26-26)ScanState(59-59)PRESCANNER_ERROR_CODES(43-43)analyzeForReDoS(67-67)calculateStarHeight(68-68)libs/ast-guard/src/pre-scanner/config.ts (1)
PreScannerConfig(25-127)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)
libs/ast-guard/src/pre-scanner/config.ts (1)
libs/ast-guard/src/pre-scanner/mandatory-limits.ts (1)
clampToMandatoryLimit(95-97)
libs/ast-guard/src/pre-scanner/checks/size-check.ts (3)
libs/ast-guard/src/pre-scanner/config.ts (1)
PreScannerConfig(25-127)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)
libs/plugins/src/codecall/tools/search.tool.ts (1)
libs/plugins/src/codecall/tools/search.schema.ts (4)
searchToolDescription(4-24)searchToolInputSchema(26-58)searchToolOutputSchema(62-96)SearchToolInput(60-60)
🪛 ast-grep (0.40.0)
libs/ast-guard/src/pre-scanner/checks/unicode-check.ts
[warning] 42-42: 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([${ALL_BIDI_CHARS.join('')}], 'g')
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)
[warning] 66-75: 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(
[${[ INVISIBLE_CHARS.ZWSP, INVISIBLE_CHARS.WORD_JOINER, INVISIBLE_CHARS.FEFF, INVISIBLE_CHARS.SHY, INVISIBLE_CHARS.CGJ, ].join('')}],
'g',
)
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)
🪛 ESLint
libs/ast-guard/src/pre-scanner/checks/unicode-check.ts
[error] 68-74: Unexpected combined character in character class.
(no-misleading-character-class)
🪛 LanguageTool
docs/draft/blog/11-2025/codecall-plugin.mdx
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ructions. It forgets what it was doing. It hallucinates. Sound familiar? --- ##...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~386-~386: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ur agent uses code to orchestrate them. Your token costs drop by 90%. --- ## The P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/draft/docs/plugins/codecall/security.mdx
[style] ~82-~82: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Extremely large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
[style] ~83-~83: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...aracters can make code appear different than it executes ### Mandatory Limits (Cann...
(DIFFERENT_THAN)
libs/ast-guard/docs/SECURITY-AUDIT.md
[style] ~56-~56: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Extremely large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
[style] ~57-~57: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...aracters can make code appear different than it executes ### Mandatory Limits (Cann...
(DIFFERENT_THAN)
⏰ 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
…o prevent I/O flood attacks
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/draft/docs/plugins/codecall/security.mdx (1)
89-96: Unresolved: Table conflicts with AgentScript preset—clarify 100 MB vs 50 KB limit.This issue was flagged in the prior review and remains unresolved. The mandatory-limits table (line 91) states "Max Input Size: 100 MB," but the AgentScript preset configuration (line 151) hardcodes
maxInputSize: 50_000(50 KB), and the accordion text (line 129) reinforces the 50 KB threshold. These must be consistent.Decide whether 100 MB is the absolute platform cap (with 50 KB as the strict preset default) or if 100 MB is outdated. Then update the table to reflect reality:
-| Max Input Size | 100 MB | Prevents memory exhaustion | +| Max Input Size | 100 MB (absolute) / 50 KB (AgentScript preset) | Prevents memory exhaustion |Or, if 50 KB is the only enforced limit in practice:
-| Max Input Size | 100 MB | Prevents memory exhaustion | +| Max Input Size | 50 KB | Prevents memory exhaustion |Also applies to: 129-133, 151-151
🧹 Nitpick comments (3)
docs/draft/docs/plugins/codecall/security.mdx (1)
82-84: Minor: Grammar and word-choice improvement.Line 82: Replace "Extremely large" with a less overused intensifier (e.g., "Large," "Substantial").
Line 83: Use "different from" instead of "different than."
-3. **Memory Exhaustion**: Extremely large inputs can exhaust memory before validation +3. **Memory Exhaustion**: Large inputs can exhaust memory before validation-4. **Trojan Source Attacks**: Unicode BiDi characters can make code appear different than it executes +4. **Trojan Source Attacks**: Unicode BiDi characters can make code appear different from it executesdocs/draft/blog/11-2025/codecall-plugin.mdx (2)
55-59: Vary sentence structure to improve readability.Lines 55–59 contain three successive sentences beginning with "It" ("It drops earlier instructions. It forgets what it was doing. It hallucinates."), which creates a repetitive rhythm. Consider restructuring to improve flow—for example, using subordinate clauses or passive constructions.
Example revision:
But worse than cost: the model starts **losing context**—dropping earlier instructions, forgetting what it was doing, and hallucinating.
385-387: Vary sentence structure to improve readability.Lines 385–387 contain three successive sentences beginning with "Your" ("Your MCP client now sees…. Your agent uses…. Your token costs drop…"), creating a repetitive rhythm. Consider combining or restructuring for better flow.
Example revision:
Your MCP client now sees **4 tools** instead of 50—your agent orchestrates them via code, and your token costs drop by 90%.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/draft/blog/11-2025/codecall-plugin.mdx(1 hunks)docs/draft/docs/guides/enclave.mdx(3 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/enclave/SECURITY-AUDIT.md(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/draft/docs/guides/enclave.mdx
🧰 Additional context used
📓 Path-based instructions (4)
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/codecall/security.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/codecall/security.mdxdocs/draft/blog/11-2025/codecall-plugin.mdx
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document known limitations clearly and write comprehensive security documentation for validation libraries
Files:
libs/enclave/SECURITY-AUDIT.md
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/enclave/SECURITY-AUDIT.md
🧠 Learnings (3)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
libs/enclave/SECURITY-AUDIT.md
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/enclave/SECURITY-AUDIT.md
🪛 LanguageTool
docs/draft/docs/plugins/codecall/security.mdx
[style] ~82-~82: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Extremely large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
[style] ~83-~83: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...aracters can make code appear different than it executes ### Mandatory Limits (Cann...
(DIFFERENT_THAN)
docs/draft/blog/11-2025/codecall-plugin.mdx
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ructions. It forgets what it was doing. It hallucinates. Sound familiar? --- ##...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~387-~387: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ur agent uses code to orchestrate them. Your token costs drop by 90%. --- ## The P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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
🔇 Additional comments (9)
docs/draft/docs/plugins/codecall/security.mdx (1)
1-592: Comprehensive and well-structured security documentation.The file delivers excellent coverage of the five-layer defense-in-depth model with clear examples, threat categories, and actionable checklists. The Threat Model section (lines 533–574) effectively documents what CodeCall protects against and—critically—what it does not, aligning with the learning to document known limitations clearly. The examples, accordions, and preset configurations are concrete and helpful for operators.
Structure and navigation within the file are intuitive, and cross-references to related docs (AST Guard, Enclave, Configuration) support discoverability.
docs/draft/blog/11-2025/codecall-plugin.mdx (2)
235-235: Verify that all referenced documentation links exist.The blog references several internal documentation paths (e.g.,
/docs/guides/ast-guard,/docs/guides/enclave,/docs/guides/vectoriadb,/docs/plugins/codecall,/docs/plugins/codecall/security). Ensure these paths match the actual documentation structure added or modified in this PR, and update any paths that have changed during docs reorganization.Also applies to: 257-257, 285-285, 322-322, 489-489, 505-505
1-523: Content substantially supports PR objectives and positioning.The blog post effectively communicates the CodeCall value proposition, security architecture, and practical deployment path. The reframed narrative (from generic code-execution pattern to a 100-tool problem solution) strengthens positioning. Security sections appropriately detail the defense-in-depth model. The plugin ecosystem framing and real-world metrics strengthen credibility.
Once documentation links are verified (prior comment), this draft is substantively ready for publication.
libs/enclave/SECURITY-AUDIT.md (6)
3-6: Updated audit metadata and statistics are accurate. Version bumped to v0.0.2 with expanded test coverage (423 tests, 100% pass rate) and vector count increased from 72 to 75. Clear progression from v0.0.1.
116-123: I/O Flood Attack Prevention section is well-documented. Concrete controls (maxConsoleOutputBytes, maxConsoleCalls, cross-method rate limiting) with clear verdict. Integrates cleanly with broader CodeCall security architecture mentioned in PR.
131-140: Clarify the distinction between Spectre-class hardware vulnerability and blocked JavaScript timing primitives. The section correctly identifies that SharedArrayBuffer, Atomics.wait, and performance.now() are blocked, but the language "not applicable" (line 140) may be misinterpreted. Spectre is fundamentally a hardware vulnerability, not a JavaScript API concern; the VM sandbox blocks the JS-based exploitation vectors, but that's different from saying Spectre attacks are "not applicable." Readers may conflate these concepts. Consider revising to: "Spectre-class attacks are not exploitable via JavaScript primitives in the Enclave, as the timing channels required are blocked. Spectre remains a hardware-level threat outside this sandbox's scope."
157-179: Known Limitations section is clear and properly prioritized. Three documented gaps with mitigation strategies and priority levels (Medium, Low). The new "VM Context State Sharing" limitation (lines 173–178) is appropriately flagged for multi-execution scenarios. This aligns with security best practices of being explicit about residual risks.
275-277: New I/O attack vectors (ATK-IO-1 to ATK-IO-3) are properly integrated into the attack matrix. Console output size, call count, and cross-method rate limiting vectors are specific and testable. Consistent with the I/O Flood Attack Prevention section (lines 116–123).
385-391: Updated conclusion and test statistics accurately reflect v0.0.2 scope. All 423 tests passing (0 failures, 0 skipped) with 75 attack vectors tested. The security rating (A+) is well-supported. Version history entry clearly documents the changes (I/O flood protection, Spectre documentation, ATK-44 fix for top-level returns). Comprehensive for production use claim.Also applies to: 400-438
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/draft/docs/plugins/codecall/security.mdx (1)
97-104: Unresolved: 100 MB vs 50 KB limit inconsistency (duplicate of past review).The table at lines 97–104 states "Max Input Size: 100 MB" as a mandatory limit, but the AgentScript preset section (line 159) shows
maxInputSize: 50_000(50 KB). This remains inconsistent and could mislead users about the actual default limit enforced in CodeCall.Consider updating the table to clarify both the absolute maximum and the AgentScript default, as suggested in the prior review:
-| Max Input Size | 100 MB | Prevents memory exhaustion | +| Max Input Size | 100 MB (absolute) / 50 KB (AgentScript default) | Prevents memory exhaustion |Also applies to: 159-162
🧹 Nitpick comments (13)
docs/draft/docs/plugins/codecall/security.mdx (1)
88-92: Optional: Minor style refinements in Layer 0 rationale.Per static analysis suggestions, consider these small wording improvements:
- Line 90: Replace "Extremely large inputs" with a less intensified phrase (e.g., "Large inputs" or "Oversized inputs")
- Line 91: Change "different than" to "different from" (more formal style)
- 3. **Memory Exhaustion**: Extremely large inputs can exhaust memory before validation + 3. **Memory Exhaustion**: Large inputs can exhaust memory before validation - 4. **Trojan Source Attacks**: Unicode BiDi characters can make code appear different than it executes + 4. **Trojan Source Attacks**: Unicode BiDi characters can make code appear different from how it executesdocs/draft/blog/11-2025/codecall-plugin.mdx (2)
55-61: Optional: Reduce repetition in lines 58–60.Three successive sentences begin with "It"—consider rewording for variety:
- Sound familiar? - But worse than cost: the model starts **losing context**. It drops earlier instructions. It forgets what it was doing. It hallucinates. + Sound familiar? + But worse than cost: the model starts **losing context**. Earlier instructions are dropped. Memory of the task fades. Hallucinations increase.
424-429: Optional: Reduce repetition in lines 427–428.Three successive sentences begin with "Your"—consider varying the structure:
- Your MCP client now sees **4 tools** instead of 50. Your agent uses code to orchestrate them. Your token costs drop by 90%. + Your MCP client now sees **4 tools** instead of 50. Code orchestration replaces tool-picking. Token costs drop by 90%.libs/enclave/src/lib/scoring/__tests__/rule-based-scorer.spec.ts (1)
135-138: Avoid non-null assertion; use proper error handling.The non-null assertion
signal!.scoreon line 137 violates the coding guideline to avoid!assertions. Since the previous line already assertssignalis defined, consider restructuring to avoid the assertion.- const signal = result.signals.find(s => s.id === 'LOOP_TOOL_CALL'); - expect(signal).toBeDefined(); - expect(signal!.score).toBeGreaterThan(25); // More than single tool + const signal = result.signals.find(s => s.id === 'LOOP_TOOL_CALL'); + expect(signal).toBeDefined(); + if (signal) { + expect(signal.score).toBeGreaterThan(25); // More than single tool + }As per coding guidelines, prefer proper error handling over non-null assertions.
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts (2)
89-91: Avoid non-null assertion onsignals.The non-null assertion
result.signals!.lengthviolates coding guidelines. Consider using a conditional check or asserting with a more descriptive message.expect(result.signals).toBeDefined(); - expect(result.signals!.length).toBeGreaterThan(0); + expect(result.signals?.length).toBeGreaterThan(0);
144-156: Multiple non-null assertions on cache stats.Lines 146-147 and 155 use non-null assertions on
stats. Consider using optional chaining or adding proper null checks.const stats = gate.getCacheStats(); expect(stats).not.toBeNull(); - expect(stats!.totalHits).toBe(1); - expect(stats!.totalMisses).toBe(1); + expect(stats?.totalHits).toBe(1); + expect(stats?.totalMisses).toBe(1);libs/enclave/src/lib/scoring/scorers/external-api.scorer.ts (1)
119-129: Consider stricter validation of API response.The type assertion
as ApiResponseat line 119 is followed by partial validation (onlyscoreis checked). If the API returns an invalidriskvalue or malformedsignals, runtime errors could occur downstream.Consider adding validation for
riskandsignalsstructure:const data = (await response.json()) as ApiResponse; if (data.error) { throw new Error(`API error: ${data.error}`); } if (typeof data.score !== 'number') { throw new Error('Invalid API response: missing score'); } + // Validate risk level if present + if (data.risk !== undefined) { + const validRisks = ['none', 'low', 'medium', 'high', 'critical']; + if (!validRisks.includes(data.risk)) { + throw new Error(`Invalid API response: invalid risk level '${data.risk}'`); + } + } return data;libs/enclave/src/lib/scoring/scoring-gate.ts (1)
265-271: Minor redundancy in dispose: clear before nullifying.
dispose()callsthis.cache?.clear()then immediately setsthis.cache = null. The clear is unnecessary since the cache is about to be dereferenced anyway.dispose(): void { this.scorer?.dispose?.(); - this.cache?.clear(); this.scorer = null; this.cache = null; this.initialized = false; }This is a minor optimization; the current code is functionally correct.
libs/enclave/src/lib/scoring/cache.ts (1)
42-49: Consider validatingmaxEntriesconfiguration.If
maxEntriesis set to0or a negative value, the cache will always evict entries immediately or behave unexpectedly. Consider adding validation in the constructor.constructor(config?: Partial<ScoringCacheConfig>) { this.cache = new Map(); + const maxEntries = config?.maxEntries ?? DEFAULT_SCORING_CONFIG.cache.maxEntries; + if (maxEntries < 1) { + throw new Error('ScoringCache maxEntries must be at least 1'); + } this.config = { enabled: config?.enabled ?? DEFAULT_SCORING_CONFIG.cache.enabled, ttlMs: config?.ttlMs ?? DEFAULT_SCORING_CONFIG.cache.ttlMs, - maxEntries: config?.maxEntries ?? DEFAULT_SCORING_CONFIG.cache.maxEntries, + maxEntries, }; }libs/enclave/src/lib/scoring/scorers/rule-based.scorer.ts (1)
71-77: UnusedcustomRulesparameter - dead code or incomplete feature.The
customRulesparameter is accepted in the constructor and stored, but it's never used in any of the rule check methods. Either remove it or implement the intended customization logic.If this is planned for future use, consider adding a TODO comment or removing it until implemented:
- private customRules: Record<string, number>; - - constructor(customRules?: Record<string, number>) { + constructor() { super(); - this.customRules = customRules ?? {}; this.ready = true; // Rule-based scorer is always ready }libs/enclave/src/lib/scoring/feature-extractor.ts (2)
222-232: Silent parse failure may mask issues.When parsing fails, the code silently returns an empty AST, resulting in zero extracted features. The caller cannot distinguish between "code has no tool calls" and "code failed to parse." Consider tracking parse success in
ExtractionMetaor logging a warning.
264-266: Minor: Non-null assertion is safe here but could use nullish coalescing for consistency.The assertion at line 266 is technically safe due to the prior
typeofcheck, but using nullish coalescing would be more consistent with the codebase style.- const isStaticName = toolNameNode?.type === 'Literal' && typeof toolNameNode.value === 'string'; - const toolName = isStaticName ? toolNameNode.value! : '<dynamic>'; + const isStaticName = toolNameNode?.type === 'Literal' && typeof toolNameNode.value === 'string'; + const toolName = isStaticName ? (toolNameNode.value ?? '<dynamic>') : '<dynamic>';libs/enclave/src/lib/enclave.ts (1)
482-485: Minor: Non-null assertion after assignment.Line 484 uses
this.adapter!immediately after assignment. While safe, consider returningthis.adapterdirectly since TypeScript should infer the type correctly after assignment, or use a local variable.case 'vm': { const { VmAdapter } = await import('./adapters/vm-adapter.js'); - this.adapter = new VmAdapter(); - return this.adapter!; + const adapter = new VmAdapter(); + this.adapter = adapter; + return adapter; }
📜 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 (19)
docs/draft/blog/11-2025/codecall-plugin.mdx(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/enclave/SECURITY-AUDIT.md(7 hunks)libs/enclave/src/index.ts(1 hunks)libs/enclave/src/lib/enclave.ts(10 hunks)libs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.ts(1 hunks)libs/enclave/src/lib/scoring/__tests__/rule-based-scorer.spec.ts(1 hunks)libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts(1 hunks)libs/enclave/src/lib/scoring/cache.ts(1 hunks)libs/enclave/src/lib/scoring/feature-extractor.ts(1 hunks)libs/enclave/src/lib/scoring/index.ts(1 hunks)libs/enclave/src/lib/scoring/scorer.interface.ts(1 hunks)libs/enclave/src/lib/scoring/scorers/disabled.scorer.ts(1 hunks)libs/enclave/src/lib/scoring/scorers/external-api.scorer.ts(1 hunks)libs/enclave/src/lib/scoring/scorers/index.ts(1 hunks)libs/enclave/src/lib/scoring/scorers/rule-based.scorer.ts(1 hunks)libs/enclave/src/lib/scoring/scoring-gate.ts(1 hunks)libs/enclave/src/lib/scoring/types.ts(1 hunks)libs/enclave/src/lib/types.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/lib/scoring/scorers/index.tslibs/enclave/src/lib/scoring/scorers/disabled.scorer.tslibs/enclave/src/lib/scoring/__tests__/rule-based-scorer.spec.tslibs/enclave/src/lib/scoring/scoring-gate.tslibs/enclave/src/lib/scoring/cache.tslibs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/types.tslibs/enclave/src/lib/scoring/scorer.interface.tslibs/enclave/src/lib/scoring/scorers/external-api.scorer.tslibs/enclave/src/lib/scoring/index.tslibs/enclave/src/index.tslibs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.tslibs/enclave/src/lib/scoring/feature-extractor.tslibs/enclave/src/lib/scoring/types.tslibs/enclave/src/lib/scoring/scorers/rule-based.scorer.tslibs/enclave/src/lib/enclave.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/enclave/src/lib/scoring/scorers/index.tslibs/enclave/src/lib/scoring/scorers/disabled.scorer.tslibs/enclave/src/lib/scoring/__tests__/rule-based-scorer.spec.tslibs/enclave/src/lib/scoring/scoring-gate.tslibs/enclave/src/lib/scoring/cache.tslibs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/types.tslibs/enclave/src/lib/scoring/scorer.interface.tslibs/enclave/src/lib/scoring/scorers/external-api.scorer.tslibs/enclave/src/lib/scoring/index.tslibs/enclave/src/index.tslibs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.tslibs/enclave/SECURITY-AUDIT.mdlibs/enclave/src/lib/scoring/feature-extractor.tslibs/enclave/src/lib/scoring/types.tslibs/enclave/src/lib/scoring/scorers/rule-based.scorer.tslibs/enclave/src/lib/enclave.ts
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/blog/11-2025/codecall-plugin.mdxdocs/draft/docs/plugins/codecall/security.mdx
**/{libs,apps}/**/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/{libs,apps}/**/src/index.ts: Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Files:
libs/enclave/src/index.ts
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document known limitations clearly and write comprehensive security documentation for validation libraries
Files:
libs/enclave/SECURITY-AUDIT.md
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/codecall/security.mdx
🧠 Learnings (9)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/enclave/src/lib/scoring/scorers/index.tslibs/enclave/src/lib/scoring/index.tslibs/enclave/src/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Applied to files:
libs/enclave/src/lib/scoring/scorers/index.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and edge conditions
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.tslibs/enclave/SECURITY-AUDIT.md
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Include constructor validation tests and error class instanceof checks in all test suites
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/enclave/src/lib/types.tslibs/enclave/src/lib/scoring/index.tslibs/enclave/src/index.tslibs/enclave/src/lib/scoring/types.tslibs/enclave/src/lib/enclave.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/enclave/SECURITY-AUDIT.md
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
🧬 Code graph analysis (4)
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts (2)
libs/enclave/src/lib/scoring/scoring-gate.ts (1)
ScoringGate(28-286)libs/enclave/src/lib/scoring/cache.ts (1)
ScoringCache(36-205)
libs/enclave/src/lib/scoring/scorers/external-api.scorer.ts (1)
libs/enclave/src/lib/scoring/types.ts (4)
RiskLevel(23-23)ExternalApiConfig(367-395)ExtractedFeatures(192-217)ScoringResult(252-277)
libs/enclave/src/lib/scoring/feature-extractor.ts (1)
libs/enclave/src/lib/scoring/types.ts (7)
SensitiveCategory(28-32)ExtractedToolCall(50-91)PatternSignals(96-127)NumericSignals(132-152)SensitiveAccess(157-167)ExtractionMeta(172-187)RULE_THRESHOLDS(505-525)
libs/enclave/src/lib/scoring/scorers/rule-based.scorer.ts (2)
libs/enclave/src/lib/scoring/types.ts (6)
RuleId(37-45)ExtractedFeatures(192-217)ScoringResult(252-277)RiskSignal(222-247)RULE_THRESHOLDS(505-525)RiskLevel(23-23)libs/enclave/src/lib/scoring/feature-extractor.ts (1)
FeatureExtractor(57-482)
🪛 ESLint
libs/enclave/src/lib/scoring/feature-extractor.ts
[error] 77-77: 'loopDepth' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 LanguageTool
docs/draft/blog/11-2025/codecall-plugin.mdx
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ructions. It forgets what it was doing. It hallucinates. Sound familiar? --- ##...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~427-~427: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ur agent uses code to orchestrate them. Your token costs drop by 90%. --- ## The P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/draft/docs/plugins/codecall/security.mdx
[style] ~90-~90: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Extremely large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
[style] ~91-~91: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...aracters can make code appear different than it executes ### Mandatory Limits (Cann...
(DIFFERENT_THAN)
⏰ 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
🔇 Additional comments (48)
docs/draft/docs/plugins/codecall/security.mdx (4)
167-597: ✅ Comprehensive and well-structured security layer documentation.Layers 1–5 are clearly explained with concrete examples, threat scenarios, and implementation details. The self-reference guard section (lines 510–545) effectively conveys the security importance. Sanitization rules and error categories provide good visibility. The content aligns with the coding guideline to write comprehensive security documentation. Based on learnings, this security documentation meets the standard for production use.
601-672: ✅ Clear error categorization and actionable security checklist.The error categories table (lines 605–614) provides good clarity on which error details can be safely exposed. The six-step security checklist is practical and well-ordered for production deployment.
676-723: ✅ Balanced threat model with realistic scope boundaries.The dual framing—what CodeCall protects against and what it does not—sets appropriate expectations. The section on tool abuse, algorithmic complexity, and business logic bugs ensures users understand CodeCall's role within a broader defense-in-depth strategy.
726-740: ✅ Good cross-documentation links.The related documentation cards provide natural entry points for readers wanting to explore AST Guard, Enclave, and security audit details.
docs/draft/blog/11-2025/codecall-plugin.mdx (3)
124-180: ✅ Fair and clear competitive positioning.The comparison to Anthropic's approach is balanced—acknowledges their innovation while clearly stating limitations (Claude-only, not self-hosted, beta). The positioning of CodeCall as an open-source alternative is well-reasoned.
184-355: ✅ Clear meta-API design and security narrative.The 4-tool architecture is concisely explained. The mermaid sequence diagram effectively shows the flow. Security sections (Layers 0–6) are well-integrated into the blog narrative and align with security.mdx. Accordion structure aids readability.
487-559: ✅ Pragmatic use case guidance and compelling closing.The "Use CodeCall When / Skip When" guidance is honest and practical. The vision statement effectively ties back to Anthropic's philosophy while reframing for open-source/self-hosted context. Get Started cards provide clear next steps.
libs/enclave/src/lib/scoring/__tests__/rule-based-scorer.spec.ts (1)
1-329: Comprehensive test coverage for the RuleBasedScorer.The test suite provides thorough coverage of all rule types, risk levels, score capping, and metadata. The structure follows good testing practices with clear positive and negative test cases for each rule.
libs/enclave/src/lib/scoring/types.ts (1)
1-525: Well-structured type definitions with comprehensive documentation.The type system is cleanly organized with clear hierarchies (ScorerType, RiskLevel, RuleId). The use of
as constfor DEFAULT_SCORING_CONFIG and RULE_THRESHOLDS ensures immutability. JSDoc comments provide good API documentation.libs/enclave/src/lib/scoring/scorers/index.ts (1)
1-9: Clean barrel export following conventions.The barrel file exports all scorer implementations without legacy aliases, following the coding guidelines. Based on learnings, this is the expected pattern for
index.tsfiles.libs/enclave/src/lib/scoring/scorers/disabled.scorer.ts (1)
16-34: Clean null-object pattern implementation.The DisabledScorer correctly implements a pass-through scorer with zero latency overhead. The unused parameter is properly prefixed with
_, and the implementation is appropriately minimal.libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts (1)
296-444: Comprehensive ScoringCache test coverage.The cache tests thoroughly cover constructor defaults, get/set operations, TTL expiration, LRU eviction, statistics tracking, and clear/prune functionality. Good use of timing-based tests for TTL verification.
libs/enclave/src/lib/scoring/scorers/external-api.scorer.ts (1)
54-88: Well-implemented retry logic with exponential backoff.The scoring method properly implements retry with exponential backoff, tracks the last error, and throws a descriptive error after all attempts fail. The error wrapping preserves the cause chain.
libs/enclave/src/index.ts (1)
36-83: Clean public API surface for scoring subsystem.The barrel exports follow conventions: value exports and type exports are properly separated, no legacy aliases, and the API surface is comprehensive for consumers of the scoring functionality. Based on learnings, this pattern is correct for library barrel files.
libs/enclave/src/lib/scoring/scoring-gate.ts (2)
127-239: Well-structured evaluate method with comprehensive error handling.The
evaluate()method properly handles all paths: disabled mode fast-path, auto-initialization, cache lookup, scoring, caching results, and fail-open/fail-closed error semantics. The timing metrics (latencyMs, extractionTimeMs, scoringTimeMs) provide good observability.
99-104: Local-LLM fallback documented with warning.The fallback to rule-based scoring when
local-llmis selected is appropriate for the current implementation state. The verbose warning informs developers of the behavior.Consider adding a TODO comment or tracking issue for the local-llm implementation:
case 'local-llm': - // For now, fall back to rule-based until local LLM is implemented + // TODO: Implement local-llm scorer (https://github.com/agentfront/frontmcp/issues/XXX) + // For now, fall back to rule-based until local LLM is implemented if (this.config.verbose) { console.warn('[ScoringGate] local-llm scorer not yet implemented, using rule-based'); } return new RuleBasedScorer(this.config.customRules);libs/enclave/src/lib/scoring/cache.ts (2)
36-113: LGTM!The LRU cache implementation is well-structured with proper MRU ordering via delete-and-reinsert, TTL-based expiration, and correct eviction logic. The use of
Required<ScoringCacheConfig>ensures strict typing.
137-205: LGTM!The helper methods (
has,delete,clear,prune) and statistics are well-implemented. Theprune()method safely iterates and deletes from the Map, andgetStats()properly handles the division-by-zero edge case.libs/enclave/src/lib/scoring/__tests__/feature-extractor.spec.ts (4)
7-12: LGTM!Good test setup with
beforeEachcreating a fresh extractor instance for isolation between tests.
14-95: LGTM!Comprehensive test coverage for tool call extraction including static/dynamic tool names,
__safe_callToolwrapper detection, and argument extraction (keys, string literals, numeric literals).
97-280: LGTM!Thorough testing of loop context detection (depth, nesting), sensitive field categorization, pattern signals, and numeric signals (limit values, density, fan-out risk).
318-399: LGTM!Good edge case coverage including graceful handling of invalid syntax. Static helper tests verify both positive detection and safe negatives for exfiltration patterns, bulk operations, and send operations.
libs/enclave/src/lib/scoring/scorer.interface.ts (2)
16-52: LGTM!The
Scorerinterface is well-designed with clear separation of concerns: required identification (type,name), core functionality (score), and optional lifecycle hooks (initialize,dispose,isReady).
57-96: LGTM!The
BaseScorerabstract class provides a solid foundation with sensible defaults for lifecycle management and helper methods. The risk level thresholds (80/60/40/20) are appropriate for overall score assessment.libs/enclave/src/lib/scoring/index.ts (1)
1-77: LGTM!Well-organized barrel file with clear categorization of exports. The documentation example demonstrates typical usage, and the separation of type exports using
export typefollows TypeScript best practices. Based on learnings, this follows the pattern of exporting everything users need without legacy aliases.libs/enclave/SECURITY-AUDIT.md (3)
124-139: LGTM!Excellent documentation of the new AI Scoring Gate capabilities including all 8 detection rules, risk levels, and configurable thresholds. The section clearly describes what semantic patterns are detected. As per coding guidelines, this provides comprehensive security documentation.
141-157: LGTM!Well-documented side channel attack prevention with clear explanation of why Spectre-class attacks are not applicable due to blocked prerequisites (SharedArrayBuffer, Atomics.wait, high-precision timing). This follows the coding guideline to document known limitations clearly.
197-245: LGTM!Clear documentation of the 6-layer defense architecture with the new AI Scoring Gate (Layer 3) positioned appropriately between code transformation and runtime sandbox. The layer descriptions are concise yet comprehensive.
libs/enclave/src/lib/scoring/scorers/rule-based.scorer.ts (5)
79-122: LGTM!The
score()method follows a clear pipeline: execute all 8 rule checks, aggregate scores with clamping, and determine overall risk level. The timing instrumentation provides good observability.
124-158: LGTM!The sensitive fields rule correctly calculates base score with category bonuses, scales for multiple fields, and caps the contribution at 60 to prevent single-rule dominance.
160-265: LGTM!Rules for excessive limits, wildcard queries, and loop tool calls are well-implemented with appropriate scaling based on severity (e.g., limit magnitude, loop nesting depth).
311-401: LGTM!Rules for extreme values, dynamic tool names, and bulk operations correctly cap their contributions and provide useful context in the signals. The
getLevelForScorehelper appropriately uses different thresholds for per-signal severity assessment.
270-309: Based on my verification of the pattern definitions, I can now provide the rewritten review comment:
Pattern sets are intentionally different; early return prevents duplicates, but verify the intent of both checks.
The method uses two different pattern matching strategies:
Local
EXFILTRATION_SEQUENCES(lines 285-296): Defines specific fetch→send pairs:
[list|get|query|search|find]→[send|email|post|webhook][fetch|read|select]→[export|upload|transfer][dump|backup]→[send|post|upload]
FeatureExtractor.detectExfiltrationPattern()(lines 300-308): Uses unified patterns:
FETCH_PATTERNS:/list|get|query|search|find|fetch|read|select/iSEND_PATTERNS:/send|email|notify|post|webhook|export|upload|transfer/iThe early
returnat line 294 prevents duplicate signals if local patterns match. However, note thatdump|backupappears only in local sequences—they are not inFETCH_PATTERNS, so the static helper cannot detect exfiltration sequences starting with dump/backup tools. This may be intentional, but clarify whether the static helper is meant to supplement the local checks or serve a different detection purpose.libs/enclave/src/lib/scoring/feature-extractor.ts (4)
27-52: Well-structured sensitive field detection patterns.The regex patterns for sensitive data categories are comprehensive and align well with the
SensitiveCategorytype definition. The separation of concerns (authentication, pii, financial, internal) provides good categorization for downstream scoring.
379-405: LGTM!The fan-out risk calculation uses appropriate weighting factors and shared constants. Capping at 100 ensures a consistent score range for downstream processing.
445-447: LGTM!SHA-256 truncated to 16 hex characters provides sufficient uniqueness for caching purposes while keeping keys compact.
452-467: LGTM!The exfiltration pattern detection correctly identifies fetch→send sequences. The reset on non-matching tools prevents false positives from unrelated intermediate calls.
libs/enclave/src/lib/types.ts (4)
87-99: Well-designed I/O flood protection configuration.The new
maxConsoleOutputBytesandmaxConsoleCallsfields provide appropriate controls for preventing console-based I/O flood attacks. The documentation clearly explains the security purpose.
123-136: Console limits follow hierarchical security pattern correctly.The console I/O limits scale appropriately across security levels (STRICT→PERMISSIVE), following the established preset pattern. Based on learnings, this aligns with the hierarchical configuration approach used in the codebase.
Also applies to: 155-168, 186-199, 219-232
258-263: LGTM!The optional
scoringResultfield cleanly extendsExecutionResultto include scoring data when available, maintaining backward compatibility.
698-737: Excellent documentation for scoring gate configuration.The JSDoc includes practical examples for both rule-based and external API scorers, clearly documents the execution timing (after AST validation, before execution), and the re-export provides convenient type access.
libs/enclave/src/lib/enclave.ts (7)
203-226: LGTM!The expanded
allowedGlobalscorrectly includes the newparallel,console,__safe_parallel, and__safe_consoleidentifiers to support the parallel execution helper and console rate limiting features.
235-238: LGTM!The conditional initialization correctly avoids creating a
ScoringGateinstance when the scorer is disabled, keeping resource usage minimal.
243-251: LGTM!The
initialize()method provides a clean pattern for async component setup, with proper documentation noting when it should be called (e.g., for local-llm model downloads).
327-358: Well-structured scoring gate integration.The scoring gate correctly runs after AST validation but before execution. The error response includes comprehensive details (score, risk level, signals) and the full
scoringResultfor debugging. TheSCORING_BLOCKEDerror code is descriptive and distinguishable from other error types.
385-390: LGTM!The conditional inclusion of
scoringResultin successful executions maintains backward compatibility while providing scoring insights when available.
539-545: LGTM!Proper resource cleanup pattern with optional chaining for the scoring gate disposal.
547-552: LGTM!The
getScoringStats()method provides a clean API for accessing cache statistics with proper null handling when scoring is not configured.
…ion and rate limiting
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
docs/draft/docs/plugins/codecall/security.mdx (1)
101-108: Clarify the 100 MB vs 50 KB preset limit in the mandatory limits table.As flagged in the previous review, the table on line 103 states "Max Input Size: 100 MB" without qualification, but line 163 shows the AgentScript preset uses
maxInputSize: 50_000(50 KB). Readers see the 100 MB first and may miss that it's the absolute cap, not the default applied by CodeCall.Consider updating the table entry to distinguish the preset default from the absolute maximum:
-| Max Input Size | 100 MB | Prevents memory exhaustion | +| Max Input Size | 100 MB (absolute) / 50 KB (AgentScript default) | Prevents memory exhaustion |This aligns with the detail provided in the "Input Size DoS" accordion (lines 140–145) and makes the table self-contained for readers who don't scroll through all sections.
🧹 Nitpick comments (11)
libs/enclave/src/lib/adapters/worker-pool/worker-script.ts (1)
487-529: Console output limiting is well implemented.Minor note: The byte calculation uses
s?.length(character count), which approximates bytes. For strict byte-level enforcement, considerBuffer.byteLength(s, 'utf8'), but the current approach is reasonable for limiting output size.libs/enclave/README.md (1)
178-190: Add language specifier to fenced code block.The ASCII diagram block should have a language specifier for proper markdown linting. Use
textorplaintextfor ASCII art diagrams.Apply this diff:
-``` +```text ┌─────────────────────────────────────────────────┐libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts (1)
114-141: Consider throwing an error instead of silently returningundefinedwhen depth is exceeded.When
sanitizeObjectexceedsMAX_DEPTH, it silently returnsundefined, which could lead to data loss without any indication. For consistency with the security-focused design and the error-throwing behavior documented insafeDeserialize, consider throwing aMessageValidationErrorinstead.export function sanitizeObject(value: unknown, depth = 0): unknown { if (depth > MAX_DEPTH) { - return undefined; + throw new MessageValidationError(`Object exceeds maximum depth of ${MAX_DEPTH}`); }libs/enclave/src/lib/adapters/worker-pool/worker-slot.ts (4)
185-198: Potential timeout leak if worker crashes before emitting 'ready'.If the worker crashes or exits before emitting
'ready', theonReadylistener remains registered and the timeout will still reject. However, the timeout is properly cleared only when'ready'is emitted. If the worker exits unexpectedly,handleExitwill emit an error, but thewaitForReadypromise may still be waiting.Consider also listening for error/exit during startup:
private waitForReady(): Promise<void> { return new Promise((resolve, reject) => { const timeout = setTimeout(() => { + this.off('ready', onReady); + this.off('error', onError); reject(new WorkerStartupError(`Worker ${this.id} startup timeout`)); }, this.options.startupTimeoutMs); const onReady = () => { clearTimeout(timeout); + this.off('error', onError); resolve(); }; + const onError = (error: Error) => { + clearTimeout(timeout); + this.off('ready', onReady); + reject(new WorkerStartupError(`Worker ${this.id} failed to start`, error)); + }; this.once('ready', onReady); + this.once('error', onError); }); }
230-236: Unusedreasonparameter inmarkForRecycle.The
reasonparameter is declared but never used. Consider logging it for debugging or removing it if not needed.markForRecycle(reason: string): void { if (this._status === 'terminated' || this._status === 'terminating') { return; } - + // Could log: console.debug(`Worker ${this.id} marked for recycle: ${reason}`); this.setStatus('recycling'); }
241-268: Hardcoded 5000ms graceful shutdown timeout should use configuration.The graceful shutdown timeout is hardcoded to 5000ms, but
WorkerPoolConfighas agracefulShutdownTimeoutMssetting. Consider passing this throughWorkerSlotOptions.export interface WorkerSlotOptions { /** Memory limit in MB for --max-old-space-size */ memoryLimitMB: number; /** Startup timeout in milliseconds */ startupTimeoutMs?: number; + /** Graceful shutdown timeout in milliseconds */ + gracefulShutdownTimeoutMs?: number; }Then use it in
terminate():- new Promise<void>((resolve) => setTimeout(resolve, 5000)), + new Promise<void>((resolve) => setTimeout(resolve, this.options.gracefulShutdownTimeoutMs)),
308-322: Consider rate-limiting error logging to prevent log flooding.If a worker sends many malformed messages,
console.errorwill be called for each one. This could flood logs. Consider rate-limiting or using a counter.libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts (3)
164-174: Consider logging swallowed errors during disposal.The empty catch block silently swallows all termination errors. While fire-and-forget is appropriate during disposal, completely silent failures could hide important issues during debugging.
// Don't await - just trigger termination - Promise.allSettled(terminatePromises).catch(() => {}); + Promise.allSettled(terminatePromises).catch((error) => { + // Log but don't throw - disposal should complete regardless + console.debug('Error during worker pool disposal:', error); + });
294-300: Consider logging termination failures for idle slot cleanup.Similar to the disposal case, silent error swallowing during idle slot cleanup could hide debugging information.
if (now - slot.lastActiveAt > this.config.idleTimeoutMs) { - slot.terminate(true).catch(() => {}); + slot.terminate(true).catch((error) => { + console.debug('Failed to terminate idle worker:', error); + }); this.slots.delete(slot.id); }
198-226: Recursive retry inacquireSlotcould be refactored to a loop.Lines 219-222 use recursion when a slot becomes unavailable between dequeue and acquire. In pathological scenarios with frequent slot recycling, this could lead to deep recursion. Consider using a loop pattern instead.
- private async acquireSlot(signal?: AbortSignal): Promise<WorkerSlot> { + private async acquireSlot(signal?: AbortSignal, retries = 0): Promise<WorkerSlot> { + const MAX_RETRIES = 10; + if (retries >= MAX_RETRIES) { + throw new Error('Failed to acquire worker slot after max retries'); + } + // Try to find an idle slot for (const slot of this.slots.values()) { if (slot.isIdle) { slot.acquire(`exec-${Date.now()}`); return slot; } } // Try to create a new slot if under limit if (this.slots.size < this.config.maxWorkers) { const slot = await this.createAndAddSlot(); slot.acquire(`exec-${Date.now()}`); return slot; } // Queue the request const slotId = await this.executionQueue.enqueue(signal); // Double check slot is still available const slot = this.slots.get(slotId); if (!slot || !slot.isIdle) { // Slot was removed/recycled, try again - return this.acquireSlot(signal); + return this.acquireSlot(signal, retries + 1); } slot.acquire(`exec-${Date.now()}`); return slot; }libs/enclave/src/lib/enclave.ts (1)
492-500: Worker threads adapter initialization works but has awkward type assertion.The type assertion
(this.adapter as { initialize?: () => Promise<void> })is functional but could be cleaner. Consider updatingSandboxAdapterinterface to include an optionalinitializemethod, or use a type guard.case 'worker_threads': { const { WorkerPoolAdapter } = await import('./adapters/worker-pool/index.js'); - this.adapter = new WorkerPoolAdapter( + const adapter = new WorkerPoolAdapter( this.config.workerPoolConfig, this.securityLevel ); - await (this.adapter as { initialize?: () => Promise<void> }).initialize?.(); + await adapter.initialize(); + this.adapter = adapter; return this.adapter!; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/draft/docs/plugins/codecall/overview.mdx(1 hunks)docs/draft/docs/plugins/codecall/scaling.mdx(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/enclave/README.md(4 hunks)libs/enclave/SECURITY-AUDIT.md(8 hunks)libs/enclave/src/index.ts(2 hunks)libs/enclave/src/lib/adapters/worker-pool/config.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/errors.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/execution-queue.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/index.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/memory-monitor.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/protocol.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/rate-limiter.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/worker-script.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/worker-slot.ts(1 hunks)libs/enclave/src/lib/enclave.ts(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/draft/docs/plugins/codecall/scaling.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/draft/docs/plugins/codecall/overview.mdx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/lib/adapters/worker-pool/errors.tslibs/enclave/src/lib/adapters/worker-pool/worker-slot.tslibs/enclave/src/lib/adapters/worker-pool/safe-deserialize.tslibs/enclave/src/lib/adapters/worker-pool/execution-queue.tslibs/enclave/src/lib/adapters/worker-pool/config.tslibs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/lib/adapters/worker-pool/rate-limiter.tslibs/enclave/src/lib/adapters/worker-pool/protocol.tslibs/enclave/src/lib/adapters/worker-pool/memory-monitor.tslibs/enclave/src/lib/adapters/worker-pool/worker-script.tslibs/enclave/src/index.tslibs/enclave/src/lib/adapters/worker-pool/index.tslibs/enclave/src/lib/enclave.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/enclave/src/lib/adapters/worker-pool/errors.tslibs/enclave/src/lib/adapters/worker-pool/worker-slot.tslibs/enclave/src/lib/adapters/worker-pool/safe-deserialize.tslibs/enclave/src/lib/adapters/worker-pool/execution-queue.tslibs/enclave/src/lib/adapters/worker-pool/config.tslibs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/SECURITY-AUDIT.mdlibs/enclave/src/lib/adapters/worker-pool/rate-limiter.tslibs/enclave/src/lib/adapters/worker-pool/protocol.tslibs/enclave/src/lib/adapters/worker-pool/memory-monitor.tslibs/enclave/src/lib/adapters/worker-pool/worker-script.tslibs/enclave/src/index.tslibs/enclave/src/lib/adapters/worker-pool/index.tslibs/enclave/src/lib/enclave.tslibs/enclave/README.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document known limitations clearly and write comprehensive security documentation for validation libraries
Files:
libs/enclave/SECURITY-AUDIT.mdlibs/enclave/README.md
**/{libs,apps}/**/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/{libs,apps}/**/src/index.ts: Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Files:
libs/enclave/src/index.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/codecall/security.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/codecall/security.mdx
🧠 Learnings (8)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/enclave/src/lib/adapters/worker-pool/errors.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Applied to files:
libs/enclave/src/lib/adapters/worker-pool/errors.tslibs/enclave/src/lib/adapters/worker-pool/execution-queue.tslibs/enclave/src/lib/adapters/worker-pool/protocol.tslibs/enclave/src/lib/adapters/worker-pool/worker-script.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/adapters/src/**/*.ts : Do not hardcode capabilities in adapters - use registry.getCapabilities() for dynamic capability exposure
Applied to files:
libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/lib/adapters/worker-pool/index.tslibs/enclave/src/lib/enclave.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/enclave/SECURITY-AUDIT.md
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Centralize record type definitions in libs/sdk/src/common/records/ instead of module-specific files
Applied to files:
libs/enclave/src/lib/adapters/worker-pool/protocol.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/enclave/src/index.tslibs/enclave/src/lib/adapters/worker-pool/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/enclave/src/index.tslibs/enclave/src/lib/enclave.tslibs/enclave/README.md
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
🧬 Code graph analysis (7)
libs/enclave/src/lib/adapters/worker-pool/errors.ts (2)
libs/enclave/src/index.ts (13)
WorkerPoolError(123-123)WorkerTimeoutError(124-124)WorkerMemoryError(125-125)WorkerCrashedError(126-126)WorkerPoolDisposedError(127-127)QueueFullError(128-128)QueueTimeoutError(129-129)ExecutionAbortedError(130-130)MessageFloodError(131-131)MessageValidationError(132-132)MessageSizeError(133-133)WorkerStartupError(134-134)TooManyPendingCallsError(135-135)libs/enclave/src/lib/adapters/worker-pool/index.ts (13)
WorkerPoolError(30-30)WorkerTimeoutError(31-31)WorkerMemoryError(32-32)WorkerCrashedError(33-33)WorkerPoolDisposedError(34-34)QueueFullError(35-35)QueueTimeoutError(36-36)ExecutionAbortedError(37-37)MessageFloodError(38-38)MessageValidationError(39-39)MessageSizeError(40-40)WorkerStartupError(41-41)TooManyPendingCallsError(42-42)
libs/enclave/src/lib/adapters/worker-pool/worker-slot.ts (4)
libs/enclave/src/lib/adapters/worker-pool/protocol.ts (2)
WorkerToMainMessage(204-209)MainToWorkerMessage(129-134)libs/enclave/src/lib/adapters/worker-pool/config.ts (2)
WorkerSlotStatus(101-107)ResourceUsage(112-123)libs/enclave/src/lib/adapters/worker-pool/errors.ts (2)
WorkerStartupError(163-172)WorkerCrashedError(55-64)libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts (2)
safeSerialize(89-97)safeDeserialize(37-79)
libs/enclave/src/lib/adapters/worker-pool/execution-queue.ts (2)
libs/enclave/src/lib/adapters/worker-pool/config.ts (1)
WorkerPoolConfig(15-96)libs/enclave/src/lib/adapters/worker-pool/errors.ts (3)
QueueFullError(79-91)ExecutionAbortedError(110-115)QueueTimeoutError(96-105)
libs/enclave/src/lib/adapters/worker-pool/config.ts (1)
libs/enclave/src/lib/types.ts (1)
SecurityLevel(24-24)
libs/enclave/src/lib/adapters/worker-pool/rate-limiter.ts (3)
libs/enclave/src/lib/adapters/worker-pool/index.ts (3)
RateLimiter(62-62)MessageFloodError(38-38)createRateLimiter(62-62)libs/enclave/src/index.ts (1)
MessageFloodError(131-131)libs/enclave/src/lib/adapters/worker-pool/errors.ts (1)
MessageFloodError(120-129)
libs/enclave/src/lib/adapters/worker-pool/worker-script.ts (2)
libs/enclave/src/lib/adapters/worker-pool/protocol.ts (7)
WorkerExecutionStats(31-42)SerializedConfig(48-67)MainToWorkerMessage(129-134)ExecuteMessage(76-84)ToolResponseMessage(89-99)WorkerToMainMessage(204-209)SerializedError(17-26)libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts (3)
safeDeserialize(37-79)sanitizeObject(114-141)safeSerialize(89-97)
libs/enclave/src/lib/enclave.ts (3)
libs/enclave/src/lib/adapters/worker-pool/config.ts (1)
WorkerPoolConfig(15-96)libs/enclave/src/lib/scoring/index.ts (2)
ScoringGate(28-28)ScoringGateResult(67-67)libs/enclave/src/lib/scoring/types.ts (1)
ScoringGateResult(282-332)
🪛 ESLint
libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts
[error] 170-170: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 271-271: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 297-297: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 334-334: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
🪛 LanguageTool
docs/draft/docs/plugins/codecall/security.mdx
[style] ~94-~94: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Extremely large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
[style] ~95-~95: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...aracters can make code appear different than it executes ### Mandatory Limits (Cann...
(DIFFERENT_THAN)
🪛 markdownlint-cli2 (0.18.1)
libs/enclave/README.md
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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
🔇 Additional comments (45)
libs/enclave/src/lib/adapters/worker-pool/rate-limiter.ts (3)
14-19: LGTM!The
RateLimiterConfiginterface is well-defined with clear property names and appropriate JSDoc comments.
36-119: Well-structured rate limiter implementation.The sliding window logic is correct. A few observations:
checkLimitincrements before checking, so the limit is exclusive (throws whencount > maxMessagesPerWindow).prune()uses2x windowMsfor retention, which is a sensible grace period to avoid race conditions with in-flight checks.Ensure
prune()is called periodically (e.g., via a timer in the adapter) to prevent unbounded memory growth from long-lived slot IDs.
124-129: LGTM!Clean factory function with sensible defaults (1-second window for per-second rate limiting).
libs/enclave/src/lib/adapters/worker-pool/worker-script.ts (6)
39-62: Good security hardening.Removing dangerous globals (worker thread primitives, SharedArrayBuffer, Atomics) before user code runs is essential. The empty catch is acceptable since some globals may already be undefined or non-configurable.
189-198: LGTM!The cleanup of pending tool calls in the
finallyblock is correct. Deleting from aMapduringfor...ofiteration is safe in JavaScript—the iterator continues correctly.
340-387: LGTM!The proxied tool call implementation is well-designed:
- Proper abort checking before proceeding
- Correct limit enforcement (throws when
toolCallCount > maxToolCalls)- Good input validation for
toolNameandargs- Unique
callIdgeneration usingcrypto.randomUUID()
389-485: Solid iteration safety guards.All safe loop functions correctly:
- Check abort state before each iteration
- Enforce configurable iteration limits with sensible defaults (10,000)
- Track iteration counts in execution stats
This effectively prevents infinite loops from runaway scripts.
277-334: Verify intentional omission ofDatefrom sandbox globals.The sandbox whitelist excludes
Date, which prevents timing attacks but also blocks legitimate date operations in user scripts. If this is intentional (security hardening), document it. If scripts need date functionality, consider adding a safe wrapper.
168-188: LGTM!Error handling is robust with proper stats tracking and fallback. The error serialization respects the
sanitizeStackTracesconfiguration.libs/enclave/src/lib/adapters/worker-pool/errors.ts (2)
9-21: Well-designed base error class.Proper stack trace capture via
Error.captureStackTracewhen available, and correctnameproperty assignment. This ensures clean stack traces in V8 environments.
23-189: Comprehensive and consistent error hierarchy.All specialized errors follow best practices:
- Readonly contextual fields for debugging
- Human-readable messages (MB/KB formatting for size errors)
- Consistent
nameproperty assignment- Error chaining support via
causeinWorkerStartupErrorThis aligns with the pattern of using specific error classes rather than generic
Errorobjects. Based on learnings from the codebase guidelines.libs/enclave/README.md (2)
13-30: LGTM!The updated security metrics and feature list accurately reflect the 6-layer defense model with the new Pre-Scanner (Layer 0) and AI Scoring Gate (Layer 3) additions.
124-173: Comprehensive Worker Pool documentation.The new Worker Pool Adapter section provides clear guidance on:
- When to use Worker Pool vs standard VM
- Security presets with specific resource limits
- Configuration options with sensible defaults
This helps developers make informed decisions about isolation levels.
libs/enclave/src/lib/adapters/worker-pool/protocol.ts (2)
230-313: Well-designed validation schemas.The Zod schemas provide robust runtime validation:
.strict()rejects unexpected keys (defense against injection)- Reasonable limits on string lengths (IDs: 100, tool names: 256, messages: 10K)
z.discriminatedUnionenables efficient parsing of worker messages- Numeric fields use
.nonnegative()and.int()where appropriate
315-350: LGTM!Type guards are simple, correct, and efficient for message type narrowing. They complement the Zod schemas—use guards for quick dispatch and schemas for full validation when needed.
libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts (2)
89-97: LGTM!The
safeSerializefunction correctly uses a replacer to strip dangerous keys during serialization.
99-104: LGTM!Simple and effective utility for checking dangerous keys.
libs/enclave/src/lib/adapters/worker-pool/execution-queue.ts (4)
97-143: Potential memory leak: timeout timer not cleared if promise is garbage collected.If the caller abandons the promise returned by
enqueue()without aborting, the timeout timer will keep running and hold references to the execution object. While this is unlikely in practice, consider whether this edge case matters for your use case.Additionally, the
abortHandlerclosure captures theexecutionobject. If the signal is never aborted and the request is fulfilled, the cleanup viaabortCleanup()innotifySlotAvailableproperly handles this.The implementation correctly handles the common cases. The abort and timeout cleanup paths are well-structured.
151-177: LGTM!The
notifySlotAvailablemethod correctly cleans up timers and abort listeners before fulfilling the request, and properly tracks wait time statistics.
200-211: LGTM!The
clear()method properly cleans up all pending requests by clearing timers, removing abort listeners, and rejecting with appropriate errors.
216-227: Division by zero is safely handled.The ternary check
this._totalFulfilled > 0prevents division by zero foravgWaitMs. Good defensive coding.libs/enclave/src/lib/adapters/worker-pool/config.ts (3)
166-180: Consider consistency between DEFAULT_WORKER_POOL_CONFIG and STANDARD preset.The
DEFAULT_WORKER_POOL_CONFIGusesos.cpus().lengthformaxWorkers, but theSTANDARDpreset overrides it to16. This means the effective default forSTANDARDsecurity level is always 16, even on machines with more CPUs. This is likely intentional for security, but worth documenting.The configuration structure is well-designed with clear defaults and preset hierarchy.
275-285: LGTM!The
buildWorkerPoolConfigfunction correctly implements a three-tier merge: defaults → preset → overrides. This allows security presets to override defaults while still permitting user customization.
193-206: Based on my investigation, I now have enough context. Let me analyze what I've found:Key Findings:
- STRICT preset is well-documented and intentional - It explicitly states "Maximum security, minimal resources" with tight limits (64MB memory, 100 executions, 1MB messages)
- Tiered approach is clear - STRICT (64MB) < SECURE (128MB) < STANDARD (256MB), showing intentional gradation
- Production docs recommend 128-256MB - But explicitly for "reasonable workloads", not security-critical scenarios
- STRICT is for untrusted AI code - The security level documentation confirms STRICT is meant for "AI-generated code from external sources"
- WorkerPoolAdapter is production code - It's actively used and properly integrated with the Enclave system
- Memory monitoring is enforced - Workers exceeding limits are recycled
Analysis of the Review Comment:
The comment asks to "verify STRICT preset values align with security requirements" and questions if they're "sufficient for legitimate use cases." However:
- The preset is clearly documented as maximum security (not maximum functionality)
- The values appear intentionally restrictive as a security feature
- There's no evidence these limits cause issues in practice
- The tiered approach (STRICT < SECURE < STANDARD) provides alternatives
The review comment's concern seems based on the assumption that STRICT should balance security AND functionality, but the design clearly prioritizes security for STRICT mode. The documentation and code structure make this intention explicit.
Conclusion: The original review comment's premise—that STRICT limits should be verified for "legitimate use cases"—is actually misaligned with the design intent. STRICT is intentionally restrictive for maximum security with untrusted code. For legitimate general use cases, SECURE or STANDARD presets are provided.
Rewritten Review Comment:
STRICT preset values are intentionally restrictive for maximum security; consider SECURE or STANDARD presets for legitimate use cases.
The STRICT preset (64MB memory, 100 executions, 1MB messages) is documented as "Maximum security, minimal resources" for untrusted AI-generated code. This is by design, not a limitation to verify. The tiered presets provide alternatives:
- STRICT: Security-first (64MB, 100 execs, 1MB messages)
- SECURE: Balanced (128MB, 500 execs, 4MB messages)
- STANDARD: Performance-optimized (256MB, 1000 execs, 16MB messages)
If your legitimate use cases require more resources or throughput, SECURE or STANDARD presets are appropriate choices. No action needed unless you're targeting untrusted code execution specifically.
libs/enclave/SECURITY-AUDIT.md (4)
212-264: LGTM!The security architecture documentation is comprehensive and well-structured, clearly explaining the 6-layer defense model. The new Layer 3 (AI Scoring Gate) and Layer 4 (Worker Pool) additions are well-documented with appropriate detail.
370-378: LGTM!The Worker Pool security attack vectors (ATK-WORKER-01 through ATK-WORKER-06) are clearly documented with their mitigations. This provides good traceability between security requirements and implementation.
520-531: LGTM!The version history entry for v0.5.0 is comprehensive, documenting all major changes including the Worker Pool Adapter, security hardening features, and new attack vector mitigations.
102-107: Acknowledged: Stack trace sanitization is marked for review.The documentation correctly notes that error information leakage prevention needs improvement. This aligns with the "Low Priority" recommendation on lines 427-430. Consider creating a tracking issue for this work.
libs/enclave/src/lib/adapters/worker-pool/worker-slot.ts (3)
334-343: LGTM!The
handleExitmethod correctly handles unexpected exits by cleaning up, setting status to terminated, and emitting an error with theWorkerCrashedError.
355-363: LGTM!The
cleanupmethod properly removes all event listeners and nullifies the worker reference to prevent memory leaks.
176-180: I'll verify the review comment about the__dirnameESM compatibility issue. Let me start by examining the build configuration and file context.
<function_calls>
#!/bin/bashCheck package.json for ESM type configuration
echo "=== Checking root package.json for ESM ==="
cat package.json | grep -A2 -B2 '"type"' 2>/dev/null || echo "No type field found in root"echo ""
echo "=== Checking libs/enclave/package.json for ESM ==="
cat libs/enclave/package.json 2>/dev/null | grep -A2 -B2 '"type"' || echo "No type field in enclave package.json"echo ""
echo "=== Checking libs/enclave/tsconfig.json module settings ==="
cat libs/enclave/tsconfig.json 2>/dev/null | grep -i 'module|target' || echo "No module/target config found"echo ""
echo "=== Checking root tsconfig.json module settings ==="
cat tsconfig.json 2>/dev/null | grep -i 'module|target' | head -20 || echo "No module/target config found"
</function_calls>Let me also examine the actual file and its context:
<function_calls>
#!/bin/bashRead the worker-slot.ts file around the getWorkerScriptPath method
echo "=== Examining worker-slot.ts around lines 176-180 ==="
sed -n '170,185p' libs/enclave/src/lib/adapters/worker-pool/worker-slot.tsecho ""
echo "=== Checking imports at top of file ==="
head -30 libs/enclave/src/lib/adapters/worker-pool/worker-slot.tsecho ""
echo "=== Checking if worker-script.js is located nearby ==="
ls -la libs/enclave/src/lib/adapters/worker-pool/
</function_calls>Now let me check the broader packaging and export strategy:
<function_calls>
#!/bin/bashCheck how the library is exported and packaged
echo "=== Checking exports in libs/enclave/package.json ==="
cat libs/enclave/package.jsonecho ""
echo "=== Checking build config in libs/enclave ==="
ls -la libs/enclave/echo ""
echo "=== Checking for any build scripts ==="
cat libs/enclave/package.json | grep -A5 '"scripts"'
</function_calls><function_calls>
#!/bin/bashCheck the full tsconfig.json files to understand compilation target
echo "=== Full libs/enclave/tsconfig.json ==="
cat libs/enclave/tsconfig.jsonecho ""
echo "=== Full root tsconfig.json ==="
cat tsconfig.json
</function_calls>libs/enclave/src/lib/adapters/worker-pool/index.ts (1)
1-63: Well-organized barrel file with clear categorization.The exports are cleanly organized by category (main adapter, configuration, errors, protocol types, utilities) and use type-only exports appropriately for protocol types. This follows the barrel file guidelines.
libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts (3)
328-336: Watchdog timeout handling is appropriate.The empty catch on line 334 is acceptable here since this is a force-terminate scenario during timeout. The error is already being propagated via
reject(new WorkerTimeoutError()), so logging the termination failure would be redundant.
441-473: Result building handles both success and failure cases correctly.The
buildResultmethod properly maps worker execution results to theExecutionResult<T>type, including all stat fields and error information.
433-439: Console message handling is safe.The existence check
if (console[level])on line 436 safely handles any invalid console levels that might be received from the worker.libs/enclave/src/lib/adapters/worker-pool/memory-monitor.ts (3)
58-80: Well-structured MemoryMonitor with clear lifecycle.The class properly extends EventEmitter, uses typed configuration, and initializes defaults correctly. The
memoryReportTimeoutMsfallback to 2000ms is reasonable.
123-138: Correct use ofPromise.allSettledfor resilient slot checking.Individual slot check failures won't prevent other slots from being checked, and results are properly aggregated before emitting the
checkCompleteevent.
64-70: Statistics tracking is appropriate for monitoring use case.While async operations could theoretically interleave when updating stats, this is acceptable for monitoring/observability data where exact precision is not critical.
libs/enclave/src/index.ts (2)
37-83: Well-organized scoring gate exports with clear categorization.The exports are cleanly organized with comments indicating purpose (orchestrator, feature extraction, cache, scorer implementations, constants, types).
111-142: Worker pool exports are comprehensive and well-organized.All necessary types, error classes, and utility classes are exported with clear categorization. This provides a complete public API surface for the worker pool adapter.
libs/enclave/src/lib/enclave.ts (5)
237-240: Scoring gate initialization is conditional and correct.The scoring gate is only created when
scoringGateconfig is provided and scorer is not 'disabled', preventing unnecessary overhead.
329-360: Scoring gate integration is well-positioned in the execution pipeline.The scoring evaluation correctly runs after AST validation but before code execution. Blocked scripts return detailed scoring information (score, risk level, signals) for debugging and monitoring.
548-554: Dispose properly handles scoring gate cleanup.The optional chaining
this.scoringGate?.dispose()correctly handles the case when scoring gate is not configured.
205-228: Allowed globals correctly include new runtime utilities.The addition of
parallel,console,__safe_parallel, and__safe_consoleto allowed globals enables the new safe parallel execution and console output features while maintaining security through the transform pipeline.
245-253: Based on my verification, I can now provide the rewritten review comment. The original review comment was based on a misunderstanding of how the code works.The
initialize()method is already optional due to auto-initialization inScoringGate.evaluate(); clarify this in the docstring.
ScoringGate.evaluate()(lines 141-143 inscoring-gate.ts) automatically initializes the scorer if it hasn't been initialized yet using optional chaining:await this.scorer.initialize?.(). This means users don't need to callEnclave.initialize()explicitly—it will be triggered automatically whenrun()callsevaluate(). Consider updating the docstring to reflect thatinitialize()is optional and mainly useful for explicit control over initialization timing, not a requirement.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docs/draft/docs/plugins/codecall/security.mdx (1)
94-95: Duplicate: Address previously-flagged language issues.These two lines still contain the grammar and style issues flagged in prior review comments:
- Line 94: "Extremely large inputs" uses an over-intensified adjective. Replace with "Large inputs" or "Very large inputs".
- Line 95: "different than" should be "different from" per formal technical writing conventions.
Apply this diff to fix both issues:
-3. **Memory Exhaustion**: Extremely large inputs can exhaust memory before validation -4. **Trojan Source Attacks**: Unicode BiDi characters can make code appear different than it executes +3. **Memory Exhaustion**: Large inputs can exhaust memory before validation +4. **Trojan Source Attacks**: Unicode BiDi characters can make code appear different from it executeslibs/enclave/src/lib/scoring/feature-extractor.ts (1)
264-267: Remove non-null assertion; the type guard already ensures safety.Line 267 uses a non-null assertion (
toolNameNode.value!) which violates the coding guideline. TheisStaticNamecheck already ensurestoolNameNode.valueis a string, so the assertion is redundant.Based on learnings, avoid non-null assertions and use explicit error handling or type guards instead.
Apply this diff to remove the non-null assertion:
- const toolName = isStaticName ? toolNameNode.value! : '<dynamic>'; + const toolName = isStaticName && typeof toolNameNode.value === 'string' + ? toolNameNode.value + : '<dynamic>';libs/ast-guard/src/pre-scanner/pre-scanner.ts (1)
124-130: Semantically incorrect error code for empty input.This is the same issue flagged in the previous review. Using
PRESCANNER_ERROR_CODES.INPUT_TOO_LARGEfor empty input is misleading. The comment on Line 127 acknowledges this but doesn't resolve it. A dedicatedINVALID_INPUTorINPUT_EMPTYerror code should be added.
🧹 Nitpick comments (4)
libs/enclave/src/lib/scoring/feature-extractor.ts (1)
88-103: Consider defining type guards for common acorn node patterns.The
as unknown as { type: string }pattern is repeated throughout the file when working with acorn nodes. While this works and is safe with optional chaining, you could improve type safety by defining type guard functions or interfaces for common node shapes.Example type guard approach:
type AcornNodeWithType = { type: string }; function hasType(node: unknown): node is AcornNodeWithType { return typeof (node as AcornNodeWithType)?.type === 'string'; }However, given acorn's limited type definitions, the current approach is acceptable and commonly used when working with AST libraries.
libs/enclave/src/lib/adapters/vm-adapter.ts (2)
161-225: Type-safe genericwrapfor console methods (and alignment with existing wrap helper)The rate-limited console wrapper looks good and the logic for counting calls/bytes is solid. The only thing I’d tighten is the typing of
wrapso it exactly preserves the signature of the underlying console methods and matches the pattern you already use inlibs/sdk/src/errors/error-handler.ts.Right now
wrapis typed as(...args: unknown[]) => void, whilecreateSafeConsoleadvertisestypeof console.log/etc. For strict TS, it’s cleaner to makewrapgeneric over the method type:-function createSafeConsole( - config: { maxConsoleOutputBytes: number; maxConsoleCalls: number }, - stats: ConsoleStats -): { log: typeof console.log; error: typeof console.error; warn: typeof console.warn; info: typeof console.info } { - const wrap = (method: (...args: unknown[]) => void) => (...args: unknown[]) => { +function createSafeConsole( + config: { maxConsoleOutputBytes: number; maxConsoleCalls: number }, + stats: ConsoleStats +): { log: typeof console.log; error: typeof console.error; warn: typeof console.warn; info: typeof console.info } { + const wrap = <T extends (...args: any[]) => void>(method: T): T => + ((...args: Parameters<T>) => { // Check call count limit BEFORE doing any work stats.callCount++; if (stats.callCount > config.maxConsoleCalls) { throw new Error( `Console call limit exceeded (max: ${config.maxConsoleCalls}). ` + `This limit prevents I/O flood attacks.` ); } // Calculate output size - const output = args.map(a => { + const output = args.map(a => { if (a === undefined) return 'undefined'; if (a === null) return 'null'; if (typeof a === 'object') { try { return JSON.stringify(a); } catch { return String(a); } } return String(a); - }).join(' '); + }).join(' '); // Check output size limit stats.totalBytes += output.length; if (stats.totalBytes > config.maxConsoleOutputBytes) { throw new Error( `Console output size limit exceeded (max: ${config.maxConsoleOutputBytes} bytes). ` + `This limit prevents I/O flood attacks.` ); } // Safe to call the real console method - method(...args); - }; + method(...args); + }) as T; + };This way the returned
log/error/warn/infoare guaranteed to match the exact console signatures, and you stay consistent with the existingErrorHandler.wraphelper.
327-345: Validate / default console limit config to avoid accidental unlimited loggingThe __safe_console injection and non-writable, non-configurable definition look good and match the rest of the sandbox hardening.
One behavioural detail to double‑check:
createSafeConsolerelies onconfig.maxConsoleOutputBytesandconfig.maxConsoleCallsbeing valid positive numbers. If either of these isundefined/NaN, the comparisons (stats.callCount > config.maxConsoleCalls,stats.totalBytes > config.maxConsoleOutputBytes) will always be false, effectively disabling the rate limiting. If they’re0or negative, you’ll get immediate errors on the first call/output.Can you confirm that:
- The Enclave config/types make both fields required and non‑optional at the type level, and
- You validate / clamp them to sane minimums upstream (e.g., during Enclave initialization)?
If not, I’d either (a) enforce those invariants at the config boundary, or (b) add a small guard here (throwing on invalid values or applying explicit defaults) so the rate limiter can’t be accidentally bypassed.
Separately, if you want observability on console abuse, it might be worth threading
consoleStatsinto the returnedstatsobject (e.g.,stats.console = { bytes, calls }) so callers can log/score on it, but that’s optional.libs/ast-guard/src/pre-scanner/checks/regex-check.ts (1)
69-155: Regex detection pipeline and scoring integration look sound; consider enriching error contextThe
detectRegexLiteralsflow respectsregexMode(allowshort‑circuit,blockfatal,analyzewith ReDoS scoring) and correctly wiresREDOS_THRESHOLDSinto error vs warning behavior. One small UX improvement: forREGEX_TOO_LONGyou only attachpositionbut notline, whereas other regex fatals carry both. IffindRegexLiteralscan surface line info here as well, adding it would make diagnostics more consistent for callers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/draft/docs/plugins/codecall/scaling.mdx(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/ast-guard/src/interfaces.ts(3 hunks)libs/ast-guard/src/pre-scanner/checks/regex-check.ts(1 hunks)libs/ast-guard/src/pre-scanner/pre-scanner.ts(1 hunks)libs/enclave/src/lib/adapters/vm-adapter.ts(2 hunks)libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts(1 hunks)libs/enclave/src/lib/scoring/feature-extractor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ast-guard/src/interfaces.ts
- libs/enclave/src/lib/scoring/tests/scoring-gate.spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/lib/scoring/feature-extractor.tslibs/enclave/src/lib/adapters/vm-adapter.tslibs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/checks/regex-check.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/enclave/src/lib/scoring/feature-extractor.tslibs/enclave/src/lib/adapters/vm-adapter.tslibs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/checks/regex-check.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/codecall/scaling.mdxdocs/draft/docs/plugins/codecall/security.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/codecall/scaling.mdxdocs/draft/docs/plugins/codecall/security.mdx
libs/ast-guard/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Files:
libs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/checks/regex-check.ts
🧠 Learnings (3)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Applied to files:
libs/enclave/src/lib/scoring/feature-extractor.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/ast-guard/src/pre-scanner/pre-scanner.tslibs/ast-guard/src/pre-scanner/checks/regex-check.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
🧬 Code graph analysis (4)
libs/enclave/src/lib/scoring/feature-extractor.ts (1)
libs/enclave/src/lib/scoring/types.ts (7)
SensitiveCategory(28-32)ExtractedToolCall(50-91)PatternSignals(96-127)NumericSignals(132-152)SensitiveAccess(157-167)ExtractionMeta(172-187)RULE_THRESHOLDS(505-525)
libs/enclave/src/lib/adapters/vm-adapter.ts (1)
libs/sdk/src/errors/error-handler.ts (1)
wrap(80-94)
libs/ast-guard/src/pre-scanner/pre-scanner.ts (8)
libs/ast-guard/src/pre-scanner/config.ts (4)
PreScannerPresetLevel(137-137)PreScannerConfig(25-127)createPreScannerConfig(291-314)getPreScannerPreset(283-285)libs/ast-guard/src/pre-scanner/scan-state.ts (2)
PreScanResult(78-89)ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)libs/ast-guard/src/pre-scanner/checks/size-check.ts (1)
performSizeChecks(121-132)libs/ast-guard/src/pre-scanner/checks/unicode-check.ts (1)
performUnicodeChecks(265-281)libs/ast-guard/src/pre-scanner/checks/nesting-check.ts (1)
performNestingChecks(246-251)libs/ast-guard/src/pre-scanner/checks/regex-check.ts (1)
performRegexChecks(524-526)libs/ast-guard/src/pre-scanner/checks/string-check.ts (1)
performStringChecks(173-175)
libs/ast-guard/src/pre-scanner/checks/regex-check.ts (3)
libs/ast-guard/src/pre-scanner/config.ts (1)
PreScannerConfig(25-127)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)
🪛 LanguageTool
docs/draft/docs/plugins/codecall/security.mdx
[style] ~94-~94: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Extremely large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
[style] ~95-~95: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...aracters can make code appear different than it executes ### Mandatory Limits (Cann...
(DIFFERENT_THAN)
⏰ 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
🔇 Additional comments (17)
docs/draft/docs/plugins/codecall/security.mdx (1)
101-108: Table clarification now resolves prior inconsistency.The mandatory limits table has been updated to clarify both the absolute maximum and the AgentScript preset default:
| Max Input Size | 100 MB (absolute) / 50 KB (AgentScript preset) | Prevents memory exhaustion |This resolves the prior inconsistency flagged between the 100 MB table entry and the 50KB AgentScript preset configuration.
docs/draft/docs/plugins/codecall/scaling.mdx (2)
485-490: Well fixed: Non-existentparallel()utility replaced with standard pattern.The troubleshooting section has been corrected. The prior comment flagged a reference to a non-existent
parallel()utility; the guidance now appropriately recommendsPromise.all()for independent tool calls, which is a standard JavaScript pattern and directly available in AgentScript execution contexts.
139-162: Good guidance: Direct invoke optimization clearly explained.The recommendation to use
codecall:invokeinstead ofcodecall:executefor single-tool operations is well-justified, with concrete savings estimate (15-20ms per call). Example clearly shows the difference and makes the choice actionable.libs/enclave/src/lib/scoring/feature-extractor.ts (5)
52-52: Good fix on the case normalization.The
LIMIT_FIELD_NAMESconstant is now consistently lowercase, which aligns with thekey.toLowerCase()comparison at line 420. This resolves the past review concern about case mismatch.
77-78: Good fix on removing the unused variable.The unused outer
loopDepthvariable mentioned in the previous review has been removed. TheloopDepthis now only declared within theCallExpressionhandler scope (line 108) where it's actually used.
127-142: Good fix on the non-null assertion guard.The explicit guard at line 136 (
if (varNode.id.name)) now ensures the name exists before adding it to the set at line 137, eliminating the non-null assertion that was flagged in the previous review.
64-218: Well-structured feature extraction implementation.The main
extractmethod is comprehensive and well-organized:
- Graceful parse failure handling
- Proper ancestor tracking for loop depth calculation
- Thorough extraction of tool calls, literals, and sensitive fields
- Clear separation of pattern signals, numeric signals, and sensitive access
- Good use of helper methods for modularity
453-482: Clean static utility methods.The static helper methods provide useful pattern-matching utilities that are appropriately stateless and reusable across instances.
libs/ast-guard/src/pre-scanner/pre-scanner.ts (8)
1-19: LGTM! Clear module documentation and clean imports.The module documentation effectively communicates the Layer 0 defense purpose, and the imports are well-organized with proper type-only imports.
21-36: LGTM! Clean interface design.The interface properly uses
Partial<PreScannerConfig>for overrides and provides clear documentation for both options.
38-97: LGTM! Well-designed constructor with good defaults.The constructor properly delegates to configuration utility functions and uses
readonlyproperties to prevent accidental mutation. The default 'standard' preset provides a sensible middle ground.
132-161: LGTM! Well-structured multi-phase scan.The scan phases are correctly ordered (size → Unicode → nesting → regex → string) with proper early termination via
state.shouldStop(). This design prioritizes fast checks and critical security issues.
164-187: LGTM! Good performance optimization.The
quickValidatemethod provides a sensible fast path by running only critical checks (size, Unicode, nesting). The documentation clearly communicates the trade-off between speed and thoroughness.
189-209: LGTM! Proper immutability pattern.Both methods correctly maintain immutability.
getConfig()returns a defensive copy, andwithConfig()creates a new instance while preserving the original. The overrides are properly passed throughcreatePreScannerConfig, ensuring mandatory limits are respected.
211-233: LGTM! Clean factory methods following preset hierarchy.The static factory methods provide a convenient API and correctly follow the hierarchical preset pattern (AGENTSCRIPT > STRICT > SECURE > STANDARD > PERMISSIVE) as specified in the coding guidelines.
235-265: LGTM! Convenient stateless wrappers.These convenience functions provide a simple API for one-off scans without requiring explicit scanner instantiation. The stateless design keeps the API simple and predictable.
libs/ast-guard/src/pre-scanner/checks/regex-check.ts (1)
372-474: ReDoS scoring logic is coherent and uses thresholds consistentlyThe
analyzeForReDoSimplementation cleanly aggregates a singlemaxScoreacross multiple pattern checks and maps it toREDOS_THRESHOLDS(BLOCK/WARN) viavulnerable: maxScore >= REDOS_THRESHOLDS.WARN. Catastrophic vs polynomial behavior is clearly separated by thelevelparameter, and the use of named entries fromREDOS_PATTERNSkeeps the public API (score +vulnerabilityType+explanation) predictable for callers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
docs/draft/docs/plugins/codecall/security.mdx (1)
94-94: Consider simplifying "Very large inputs" to "Large inputs."The phrase "Very large inputs can exhaust memory before validation" uses a soft intensifier. For conciseness, you could remove "Very" and say simply "Large inputs can exhaust memory before validation." This is a minor stylistic suggestion; current phrasing is acceptable in security documentation context.
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts (2)
8-317: Consider adding error case tests and constructor validation.The test suite provides good coverage of happy paths and basic error handling (fail-open/fail-closed), but according to the learnings, test files should include constructor validation tests and error class instanceof checks. Consider adding:
- Tests that verify
ScoringGateErroris thrown and can be caught with properinstanceofchecks- Tests for invalid configurations (e.g., negative thresholds, invalid scorer type, blockThreshold < warnThreshold)
- Edge cases like empty code strings or malformed code
Example test to add:
describe('error handling', () => { it('should throw ScoringGateError for invalid configuration', () => { expect(() => { new ScoringGate({ scorer: 'rule-based', blockThreshold: 30, warnThreshold: 50 // Invalid: warn > block }); }).toThrow(ScoringGateError); }); it('should handle instanceof checks for ScoringGateError', async () => { const gate = new ScoringGate({ scorer: 'rule-based', blockThreshold: -1 // Invalid }); try { await gate.initialize(); fail('Should have thrown'); } catch (error) { expect(error instanceof ScoringGateError).toBe(true); } finally { gate.dispose(); } }); });Based on learnings, this would improve coverage of all code paths including error cases.
160-317: Wrap dispose() calls in finally blocks for reliable cleanup.Several tests manually call
dispose()without wrapping in finally blocks (e.g., lines 181, 212, 232, 250, 262, 272, 284-285, 304, 313). If assertions fail before dispose() is reached, resources may leak in the test environment.Consider this pattern:
it('should respect custom block threshold', async () => { const gate = new ScoringGate({ scorer: 'rule-based', blockThreshold: 30, }); - await gate.initialize(); - - const result = await gate.evaluate(` - const data = await callTool('data:search', { query: '*' }); - `); - - // Assertions... - gate.dispose(); + try { + await gate.initialize(); + + const result = await gate.evaluate(` + const data = await callTool('data:search', { query: '*' }); + `); + + // Assertions... + } finally { + gate.dispose(); + } });Alternatively, use
beforeEach/afterEachhooks as done in lines 35-46 and 107-117.libs/enclave/src/lib/adapters/worker-pool/memory-monitor.ts (1)
176-197: Consider enforcing memory limits and emitting events incheckSlotImmediate.Unlike
checkSlot, this method doesn't check if memory usage exceeds the limit, doesn't emitmemoryExceededorcheckFailedevents, and doesn't mark the slot for recycling. This creates inconsistent behavior between periodic and on-demand checks.If immediate checks are for diagnostic purposes only, document this clearly. Otherwise, consider applying the same enforcement logic.
Apply this diff to align behavior with
checkSlot:async checkSlotImmediate(slotId: string): Promise<ResourceUsage | null> { const slot = this.slots.get(slotId); if (!slot || !slot.worker) { return null; } try { const usage = await slot.requestMemoryReport(this.config.memoryReportTimeoutMs); // Track statistics this._totalMemoryBytes += usage.rss; this._totalSamples++; if (usage.rss > this._peakMemoryBytes) { this._peakMemoryBytes = usage.rss; } + // Check limit + if (usage.rss > this.config.memoryLimitPerWorker) { + this._memoryExceededCount++; + this.emit('memoryExceeded', slotId, usage, this.config.memoryLimitPerWorker); + slot.markForRecycle('memory-exceeded'); + } return usage; - } catch { + } catch (error) { this._checkFailureCount++; + this.emit('checkFailed', slotId, error as Error); return null; } }libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts (1)
62-151: Consider consolidatingsanitizeObjectWithDepthCheckandsanitizeObjectto reduce duplication.Both functions implement nearly identical recursive sanitization logic; the only difference is that
sanitizeObjectWithDepthCheckthrows on depth exceeded (line 64) whilesanitizeObjectreturns undefined (line 126). This creates maintenance overhead—any logic change must be applied twice.Refactor to a shared implementation:
+/** + * Internal recursive sanitizer + */ +function sanitizeObjectInternal( + value: unknown, + depth: number, + throwOnDepthExceeded: boolean +): unknown { + if (depth > MAX_DEPTH) { + if (throwOnDepthExceeded) { + throw new MessageValidationError(`Message exceeds maximum depth of ${MAX_DEPTH}`); + } + return undefined; + } + + if (value === null || value === undefined) { + return value; + } + + if (typeof value !== 'object') { + return value; + } + + if (Array.isArray(value)) { + return value.map((item) => sanitizeObjectInternal(item, depth + 1, throwOnDepthExceeded)); + } + + const result = Object.create(null); + for (const key of Object.keys(value as object)) { + if (!DANGEROUS_KEYS.has(key)) { + result[key] = sanitizeObjectInternal( + (value as Record<string, unknown>)[key], + depth + 1, + throwOnDepthExceeded + ); + } + } + return result; +} + function sanitizeObjectWithDepthCheck(value: unknown, depth: number): unknown { - // ... current implementation + return sanitizeObjectInternal(value, depth, true); } export function sanitizeObject(value: unknown, depth = 0): unknown { - // ... current implementation + return sanitizeObjectInternal(value, depth, false); }libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts (4)
169-170: Remove unnecessary.catch()afterPromise.allSettled.
Promise.allSettlednever rejects, so the empty.catch()on line 170 is unnecessary and triggers an ESLint warning.Apply this diff:
// Don't await - just trigger termination - Promise.allSettled(terminatePromises).catch(() => {}); + Promise.allSettled(terminatePromises);
304-310: Log termination errors instead of silently ignoring them.The empty
.catch()on line 307 suppresses termination errors during idle slot cleanup. These errors should be logged for observability.Apply this diff:
for (let i = 0; i < excessCount; i++) { const slot = idleSlots[i]; if (now - slot.lastActiveAt > this.config.idleTimeoutMs) { - slot.terminate(true).catch(() => {}); + slot.terminate(true).catch((error) => { + console.error('Failed to terminate idle worker:', error); + }); this.slots.delete(slot.id); } }
339-346: Log watchdog termination errors for debugging.The empty
.catch()on line 344 suppresses errors when force-terminating a timed-out worker. These should be logged to aid debugging.Apply this diff:
const watchdogId = setTimeout(() => { this._timeoutExecutions++; this._forcedTerminations++; - slot.terminate(false).catch(() => {}); + slot.terminate(false).catch((error) => { + console.error('Failed to terminate timed-out worker:', error); + }); reject(new WorkerTimeoutError()); }, watchdogTimeout);
520-520: Consider tracking execution times foravgExecutionTimeMsmetric.The
avgExecutionTimeMsfield is currently hardcoded to 0 with a comment indicating it needs additional tracking. While not critical, this reduces the usefulness of the metrics API.If you'd like to implement execution time tracking, I can help generate a solution that:
- Maintains a running sum and count of execution durations
- Updates these in the execute method's finally block
- Computes the average in getMetrics
Would you like me to open a new issue to track this enhancement?
📜 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 (7)
docs/draft/docs.json(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/ast-guard/src/pre-scanner/checks/regex-check.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/memory-monitor.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts(1 hunks)libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/codecall/security.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/codecall/security.mdxdocs/draft/docs.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/adapters/worker-pool/safe-deserialize.tslibs/enclave/src/lib/adapters/worker-pool/memory-monitor.tslibs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.tslibs/ast-guard/src/pre-scanner/checks/regex-check.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/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/lib/adapters/worker-pool/safe-deserialize.tslibs/enclave/src/lib/adapters/worker-pool/memory-monitor.tslibs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.tslibs/ast-guard/src/pre-scanner/checks/regex-check.ts
libs/ast-guard/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Files:
libs/ast-guard/src/pre-scanner/checks/regex-check.ts
🧠 Learnings (7)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and edge conditions
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Include constructor validation tests and error class instanceof checks in all test suites
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.tslibs/ast-guard/src/pre-scanner/checks/regex-check.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Applied to files:
libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts
🧬 Code graph analysis (4)
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts (5)
libs/enclave/src/lib/scoring/scoring-gate.ts (1)
ScoringGate(28-286)libs/enclave/src/lib/scoring/scorers/disabled.scorer.ts (1)
score(25-33)libs/enclave/src/lib/scoring/scorers/external-api.scorer.ts (1)
score(54-88)libs/enclave/src/lib/scoring/scorers/rule-based.scorer.ts (1)
score(79-122)libs/enclave/src/lib/scoring/cache.ts (1)
ScoringCache(36-205)
libs/enclave/src/lib/adapters/worker-pool/safe-deserialize.ts (1)
libs/enclave/src/lib/adapters/worker-pool/errors.ts (2)
MessageSizeError(144-158)MessageValidationError(134-139)
libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts (9)
libs/enclave/src/index.ts (13)
WorkerPoolAdapter(114-114)SandboxAdapter(27-27)WorkerPoolConfig(31-31)SecurityLevel(28-28)WorkerSlot(137-137)ExecutionQueue(138-138)MemoryMonitor(140-140)buildWorkerPoolConfig(121-121)ExecutionContext(26-26)ExecutionResult(19-19)WorkerPoolDisposedError(127-127)WorkerTimeoutError(124-124)TooManyPendingCallsError(135-135)libs/enclave/src/lib/types.ts (5)
SandboxAdapter(530-547)WorkerPoolConfig(766-766)SecurityLevel(24-24)ExecutionContext(483-524)ExecutionResult(238-263)libs/enclave/src/lib/adapters/worker-pool/config.ts (2)
WorkerPoolConfig(15-96)buildWorkerPoolConfig(275-285)libs/enclave/src/lib/adapters/worker-pool/worker-slot.ts (1)
WorkerSlot(52-364)libs/enclave/src/lib/adapters/worker-pool/execution-queue.ts (1)
ExecutionQueue(58-240)libs/enclave/src/lib/adapters/worker-pool/memory-monitor.ts (1)
MemoryMonitor(58-255)libs/enclave/src/lib/adapters/worker-pool/rate-limiter.ts (1)
RateLimiter(36-119)libs/enclave/src/lib/adapters/worker-pool/errors.ts (3)
WorkerPoolDisposedError(69-74)WorkerTimeoutError(26-31)TooManyPendingCallsError(177-189)libs/enclave/src/lib/adapters/worker-pool/protocol.ts (7)
SerializedConfig(48-67)WorkerToMainMessage(204-209)isToolCallMessage(318-320)isExecutionResultMessage(325-327)isConsoleMessage(341-343)ToolCallMessage(150-160)ExecutionResultMessage(165-177)
libs/ast-guard/src/pre-scanner/checks/regex-check.ts (3)
libs/ast-guard/src/pre-scanner/config.ts (1)
PreScannerConfig(25-127)libs/ast-guard/src/pre-scanner/scan-state.ts (1)
ScanState(95-263)libs/ast-guard/src/pre-scanner/errors.ts (1)
PRESCANNER_ERROR_CODES(14-45)
🪛 ESLint
libs/enclave/src/lib/adapters/worker-pool/worker-pool-adapter.ts
[error] 170-170: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 307-307: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 344-344: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
🪛 LanguageTool
docs/draft/docs/plugins/codecall/security.mdx
[style] ~94-~94: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Very large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
⏰ 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
🔇 Additional comments (17)
docs/draft/docs.json (1)
119-135: Navigation restructuring looks good.The CodeCall hierarchy is well-organized under a nested group, improving discoverability. The structure is valid and aligns with the broader CodeCall documentation expansion.
Note: Please ensure that all five CodeCall documentation pages (overview, security, api-reference, configuration, scaling) exist in
docs/draft/docs/plugins/codecall/when this PR is merged, as they are now referenced in the navigation tree.docs/draft/docs/plugins/codecall/security.mdx (7)
84-168: Layer 0 Pre-Scanner section is well-documented.The mandatory limits table correctly reflects both the absolute maximum (100 MB) and the AgentScript preset default (50 KB), resolving the previous inconsistency. The attack patterns are clearly explained with concrete examples.
171-287: Layer 1 and AgentScript preset are comprehensive.Clear documentation of blocked constructs with solid rationale, and the AgentScript preset configuration is accurate and well-contextualized for LLM-generated code scenarios.
290-335: Layer 2 Code Transformation is well-explained.The transformation mapping table and example clearly show how user code is rewritten for safe execution. Reserved prefixes and their purpose are documented.
338-460: Layer 3 AI Scoring Gate is thorough.The semantic analysis approach is well-motivated, the 8 detection rules are clearly described with concrete examples, and the exfiltration detection example demonstrates the scoring model end-to-end. Risk level thresholds are explicit and actionable.
463-585: Layer 4 Runtime Sandbox and Worker Pool are comprehensive.Resource limits are sensible and well-documented. The VM presets table provides graduated options, and the optional Worker Pool feature is well-motivated with clear guidance on when to use it. The dual-layer sandbox diagram and worker pool configuration are detailed and actionable.
588-706: Self-Reference Guard, Output Sanitization, and Error Categorization are well-designed.The self-reference guard rationale is sound, sanitization rules address information leakage, and the error categorization system is systematic. All sections provide clear examples and are actionable.
710-793: Security Checklist and Threat Model are balanced and professional.The 6-step checklist is practical with concrete examples for each step. The threat model appropriately distinguishes what CodeCall protects against (10 scenarios) versus what it does not (5 scenarios with mitigations), and the disclaimer about defense-in-depth is properly positioned.
libs/enclave/src/lib/scoring/__tests__/scoring-gate.spec.ts (2)
159-214: Threshold tests improved with comprehensive assertions.These tests now properly verify threshold behavior with conditional assertions based on the actual score, addressing the concern from the previous review. The logic correctly validates:
- Block threshold: blocks when score >= threshold
- Warn threshold: warns when score is in the [warnThreshold, blockThreshold) range
320-469: ScoringCache tests provide comprehensive coverage.The test suite thoroughly exercises ScoringCache functionality:
- Constructor and configuration options
- Get/set operations with enabled/disabled states
- TTL expiration with proper async delays
- LRU eviction at capacity
- Statistics tracking (hits, misses, hit rate)
- Clear and prune operations
All assertions are appropriate and verify expected behavior.
libs/ast-guard/src/pre-scanner/checks/regex-check.ts (7)
1-76: LGTM!The constants, types, and keyword set are well-structured. The REGEX_KEYWORD_PREFIXES properly addresses regex vs division detection for keywords like
return,throw, etc.
87-174: LGTM!The detection logic correctly handles all three regex modes and properly enforces limits. The early return after blocking regexes (line 117) ensures no unnecessary analysis is performed.
180-325: LGTM!The identifier buffer mechanism (lines 197-310) correctly addresses the past review concern about distinguishing keywords like
returnfrom regular identifiers when determining if/starts a regex or division operator.
330-398: LGTM!Both past review concerns are properly addressed:
- Unclosed regex detection now uses
source[i - 1] !== '/'(line 386)- Flag parsing includes ES2024 flags
dandv(line 392)
404-506: LGTM!The ReDoS analysis uses simple, safe regex patterns to detect dangerous patterns. The two-level approach (catastrophic vs polynomial) provides appropriate granularity for security policies.
517-558: Request clarification on star height calculation for deeply nested patterns.The current implementation correctly calculates star height for simple nested patterns like
(a+)+(returns 2). However, for more deeply nested patterns like((a+)+)+, it returns 2 instead of 3.The algorithm counts the number of ancestor groups containing quantified content (line 532, 546), but doesn't account for the full depth of quantifier nesting. For
((a+)+)+:
- Three levels of quantification exist
- Algorithm returns maxHeight = 2
While this doesn't affect ReDoS detection (both 2 and 3 are > 1, indicating vulnerability), if
calculateStarHeightis intended to return the true star height for external use or future enhancements, this could be refined.Is the current behavior intentional, or should the algorithm track maximum quantified depth within each group rather than just a boolean flag?
Based on the web search results, the star height of a regular expression equals the maximum nesting depth of stars appearing in that expression. For
((a+)+)+, the theoretical star height should indeed be 3, not 2, since there are three nested levels of quantifiers.The current implementation under-reports this value. While this doesn't affect the practical ReDoS detection (both 2 and 3 indicate vulnerability > 1), the function name and documentation suggest it should return the true star height.
560-586: LGTM!The helper functions are simple and correct. The
performRegexChecksfunction serves as a clean entry point for the pre-scanner orchestration.
1. Removed the lib/ folder layer - all core files are now directly under src/ 2. Kept __tests__ folders colocated with their modules (as per your preference) 3. Updated all import paths in index.ts and test files
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
libs/enclave/src/adapters/worker-pool/protocol.ts (1)
150-160: Type mismatch between interface and schema forargs.The
ToolCallMessageinterface declaresargs: unknown, buttoolCallMessageSchemaat line 236 usesz.record(z.unknown())which expects an object. This mismatch could cause validation failures for valid messages or allow invalid types at the TypeScript level.Align the interface with the schema:
export interface ToolCallMessage { type: 'tool-call'; /** Execution request identifier */ requestId: string; /** Unique identifier for this tool call */ callId: string; /** Name of the tool to call */ toolName: string; /** Arguments for the tool */ - args: unknown; + args: Record<string, unknown>; }
🧹 Nitpick comments (22)
libs/enclave/src/scoring/scorers/rule-based.scorer.ts (1)
65-69: Consider removing or implementingcustomRules.The
customRulesfield is declared and initialized but never used in the scoring logic. If this is intended for future extensibility, consider adding a TODO comment or removing it to avoid dead code.export class RuleBasedScorer extends BaseScorer { readonly type = 'rule-based' as const; readonly name = 'RuleBasedScorer'; - private customRules: Record<string, number>; - - constructor(customRules?: Record<string, number>) { + constructor() { super(); - this.customRules = customRules ?? {}; this.ready = true; // Rule-based scorer is always ready }libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (3)
158-164: Replace empty catch handler with error logging.Line 160 has an empty catch handler which silently swallows errors. While errors during cleanup may not be fatal, logging them aids debugging.
// Don't await - just trigger termination - Promise.allSettled(terminatePromises).catch(() => {}); + Promise.allSettled(terminatePromises).catch((error) => { + console.error('Error during worker pool disposal:', error); + });
294-301: Replace empty catch handler with error logging.Line 297 has an empty catch handler which could hide termination failures during idle slot cleanup.
if (now - slot.lastActiveAt > this.config.idleTimeoutMs) { - slot.terminate(true).catch(() => {}); + slot.terminate(true).catch((error) => { + console.error(`Failed to terminate idle worker ${slot.id}:`, error); + }); this.slots.delete(slot.id); }
329-337: Replace empty catch handler with error logging.Line 334 has an empty catch handler on watchdog-triggered termination. Logging these errors helps diagnose timeout-related issues.
const watchdogId = setTimeout(() => { this._timeoutExecutions++; this._forcedTerminations++; - slot.terminate(false).catch(() => {}); + slot.terminate(false).catch((error) => { + console.error('Watchdog termination failed:', error); + }); reject(new WorkerTimeoutError()); }, watchdogTimeout);libs/enclave/src/adapters/worker-pool/worker-slot.ts (1)
303-317: Error in message deserialization silently swallowed.When
safeDeserializethrows (e.g., due to a malformed message), the error is logged to console but not propagated. Depending on the protocol, this could mask serious issues like corrupted worker state.Consider emitting a protocol error event or marking the worker for recycling on deserialization failures:
private handleMessage(raw: string): void { try { const msg = safeDeserialize(raw) as WorkerToMainMessage; if (msg.type === 'ready') { this.emit('ready'); } else if (msg.type === 'memory-report-result') { this._memoryUsage = msg.usage; } this.emit('message', msg); } catch (error) { console.error(`Worker ${this.id} message parse error:`, error); + // Consider marking for recycle on protocol errors + this.markForRecycle('protocol-error'); } }libs/enclave/src/scoring/__tests__/scoring-gate.spec.ts (1)
370-387: TTL test relies on real timers with tight margins.The test uses a 50ms TTL and waits 60ms. This 10ms margin may be flaky on slow CI runners or under system load.
Consider using fake timers or increasing the margin:
describe('TTL', () => { it('should expire entries after TTL', async () => { - const cache = new ScoringCache({ enabled: true, ttlMs: 50 }); + const cache = new ScoringCache({ enabled: true, ttlMs: 100 }); const result = { /* ... */ }; cache.set('hash1', result); expect(cache.get('hash1')).toBeDefined(); - await new Promise((resolve) => setTimeout(resolve, 60)); + await new Promise((resolve) => setTimeout(resolve, 150)); expect(cache.get('hash1')).toBeUndefined(); }); });Alternatively, use Jest's fake timers for deterministic testing.
libs/enclave/src/scoring/__tests__/rule-based-scorer.spec.ts (1)
264-290: Risk level tests cover boundary cases well.Tests verify
'none'for safe code (score < 20) and'critical'for high-risk code (score >= 80).Consider adding tests for the intermediate risk levels (
'low','medium','high') to ensure complete coverage of thegetLevelForScorelogic:it('should return medium for moderate-risk code', async () => { const code = ` const users = await callTool('users:list', { limit: 15000 }); `; const features = extractor.extract(code); const result = await scorer.score(features); expect(result.riskLevel).toBe('medium'); expect(result.totalScore).toBeGreaterThanOrEqual(40); expect(result.totalScore).toBeLessThan(60); });libs/enclave/src/adapters/worker-pool/rate-limiter.ts (1)
106-118: Pruning uses 2x window duration threshold.The prune logic removes windows older than
windowMs * 2. This is a reasonable buffer to avoid pruning windows that might still be relevant for rate tracking, but consider documenting why the 2x multiplier is used./** * Prune expired windows * Should be called periodically to prevent memory leaks + * + * Uses 2x window duration as threshold to ensure windows + * aren't pruned while they might still be relevant for rate decisions. */ prune(): number {libs/enclave/src/scoring/scorer.interface.ts (1)
77-86: Consider using theRiskLeveltype from./typesfor consistency.The return type uses a literal union
'none' | 'low' | 'medium' | 'high' | 'critical'instead of theRiskLeveltype that's likely already defined in./types.ts. This could lead to type drift ifRiskLevelis ever updated.+import type { ExtractedFeatures, ScoringResult, ScorerType, RiskLevel } from './types'; - protected calculateRiskLevel(score: number): 'none' | 'low' | 'medium' | 'high' | 'critical' { + protected calculateRiskLevel(score: number): RiskLevel {libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (2)
41-57: Size check uses string length, not byte length.The
maxSizeBytesparameter name suggests bytes, butraw.lengthreturns the number of UTF-16 code units (characters), not bytes. For strings with multi-byte UTF-8 characters, this could underestimate the actual size. Consider usingBuffer.byteLength(raw, 'utf-8')for accurate byte counting, or rename the parameter to clarify.export function safeDeserialize(raw: string, maxSizeBytes?: number): unknown { // Check size limit - if (maxSizeBytes !== undefined && raw.length > maxSizeBytes) { - throw new MessageSizeError(raw.length, maxSizeBytes); + if (maxSizeBytes !== undefined) { + const byteLength = Buffer.byteLength(raw, 'utf-8'); + if (byteLength > maxSizeBytes) { + throw new MessageSizeError(byteLength, maxSizeBytes); + } }
124-151: Document the behavioral difference fromsanitizeObjectWithDepthCheck.This function silently returns
undefinedwhen depth is exceeded (Line 126), whilesanitizeObjectWithDepthCheckthrows. While the JSDoc says "internal", this could lead to subtle bugs if callers expect an error. Consider adding a brief note explaining this design choice.libs/enclave/src/scoring/cache.ts (1)
93-113: Consider checking if key exists before eviction.The eviction loop runs before checking if the key already exists in the cache. If
codeHashis already present (an update), this could unnecessarily evict an entry when no new slot is needed.set(codeHash: string, result: ScoringResult): void { if (!this.config.enabled) { return; } + // If updating existing entry, no eviction needed + const isUpdate = this.cache.has(codeHash); + // Evict oldest entries if at capacity - while (this.cache.size >= this.config.maxEntries) { + while (!isUpdate && this.cache.size >= this.config.maxEntries) { const oldestKey = this.cache.keys().next().value; if (oldestKey) { this.cache.delete(oldestKey); } else { break; } }libs/enclave/src/safe-runtime.ts (1)
595-647: Serialized__safe_parallellacks abort checking.The runtime version checks
context.abortedat the start and before each work item, but the serialized version doesn't reference__internal_aborted. This is consistent with other serialized functions in this file (like__safe_forOf,__safe_for), so it may be intentional. However, for long-running parallel operations, abort support would be valuable.If abort support is needed in the serialized runtime, consider adding:
async function runNext() { while (currentIndex < fns.length) { if (typeof __internal_aborted === 'boolean' && __internal_aborted) { throw new Error('Execution aborted'); } // ... rest of implementation } }libs/enclave/src/scoring/feature-extractor.ts (3)
230-240: Silent failure on parse errors may hide malformed code.When parsing fails, returning an empty AST causes the extractor to report zero features. Consider logging the parse error or including a flag in
ExtractionMetato indicate parse failure, so callers can distinguish between "no risky patterns" and "unparseable code."private parseCode(code: string): acorn.Node { try { return acorn.parse(code, { ecmaVersion: 'latest', sourceType: 'module', }); - } catch { + } catch (error) { // If parsing fails, return an empty program + // Consider: could set a flag or log for observability return acorn.parse('', { ecmaVersion: 'latest' }); } }
448-450: Hash truncation to 16 characters may be insufficient for cache collision resistance.A 16-character hex substring provides 64 bits of entropy. For high-volume caching scenarios, consider using a longer substring (e.g., 32 chars / 128 bits) to reduce collision probability, or document the acceptable collision rate for your use case.
private hashCode(code: string): string { - return createHash('sha256').update(code).digest('hex').substring(0, 16); + return createHash('sha256').update(code).digest('hex').substring(0, 32); }
455-470: Exfiltration detection resets on non-matching tools, potentially missing patterns.The
lastWasFetchflag resets tofalsewhen a tool doesn't match fetch or send patterns (line 465). This means a sequence likelist → process → sendwon't trigger detection. If intermediate processing steps are common, consider tracking any prior fetch rather than requiring consecutive fetch→send.libs/enclave/src/adapters/worker-pool/errors.ts (1)
163-172: Consider using ES2022causeproperty instead of custom property.The
WorkerStartupErrordefines a customcauseproperty, but ES2022 Error constructor supports{ cause }options natively. Using the standard approach improves interoperability with error handling utilities.export class WorkerStartupError extends WorkerPoolError { - /** Original error that caused startup failure */ - readonly cause?: Error; - constructor(message: string, cause?: Error) { - super(message); + super(message, { cause }); this.name = 'WorkerStartupError'; - this.cause = cause; } }libs/enclave/src/adapters/worker-pool/worker-script.ts (1)
469-511: Console implementation duplicates logic from vm-adapter.ts createSafeConsole.Both
vm-adapter.ts(lines 168-218) and this file implement similar rate-limited console functionality. Consider extracting a shared utility to reduce duplication and ensure consistent behavior.libs/enclave/src/scoring/scorers/external-api.scorer.ts (1)
135-143: Consider using native Error cause option for consistency.Similar to the earlier suggestion for
WorkerStartupError, theExternalApiScorerErrorcould use ES2022's nativecauseoption for better error chain handling.export class ExternalApiScorerError extends Error { - readonly cause?: Error; - constructor(message: string, cause?: Error) { - super(message); + super(message, { cause }); this.name = 'ExternalApiScorerError'; - this.cause = cause; } }libs/enclave/src/adapters/worker-pool/execution-queue.ts (1)
108-108: Consider using crypto.randomUUID() for queue IDs.The current ID generation uses
Math.random().toString(36)which has limited entropy. For consistency with other parts of the codebase (e.g.,worker-script.tsline 363), consider usingcrypto.randomUUID().-const id = `queue-${Date.now()}-${Math.random().toString(36).slice(2)}`; +const id = `queue-${Date.now()}-${crypto.randomUUID()}`;libs/enclave/src/adapters/worker-pool/memory-monitor.ts (1)
16-23: Consider typed EventEmitter for stronger type safety.The
MemoryMonitorEventsinterface defines the event signatures, butEventEmitterdoesn't enforce these at compile time. Consider using a typed event emitter pattern.+import { EventEmitter } from 'events'; +import type TypedEmitter from 'typed-emitter'; + +type MemoryMonitorEmitter = TypedEmitter<{ + memoryExceeded: (slotId: string, usage: ResourceUsage, limit: number) => void; + checkFailed: (slotId: string, error: Error) => void; + checkComplete: (results: Map<string, ResourceUsage>) => void; +}>; + -export class MemoryMonitor extends EventEmitter { +export class MemoryMonitor extends (EventEmitter as new () => MemoryMonitorEmitter) {Alternatively, add runtime type checks for event arguments if external type-safety libraries aren't desired.
libs/enclave/src/enclave.ts (1)
474-488: Avoid non-null assertions - refactor adapter initialization.Lines 477 and 487 use non-null assertions (
this.adapter!) which the coding guidelines advise against. The adapter is assigned in the line above, so TypeScript should infer this, but the pattern could be cleaner.case 'vm': { const { VmAdapter } = await import('./adapters/vm-adapter.js'); - this.adapter = new VmAdapter(); - return this.adapter!; + const adapter = new VmAdapter(); + this.adapter = adapter; + return adapter; } case 'isolated-vm': throw new Error('isolated-vm adapter not yet implemented'); case 'worker_threads': { const { WorkerPoolAdapter } = await import('./adapters/worker-pool/index.js'); - this.adapter = new WorkerPoolAdapter(this.config.workerPoolConfig, this.securityLevel); - await (this.adapter as { initialize?: () => Promise<void> }).initialize?.(); - return this.adapter!; + const adapter = new WorkerPoolAdapter(this.config.workerPoolConfig, this.securityLevel); + await (adapter as { initialize?: () => Promise<void> }).initialize?.(); + this.adapter = adapter; + return adapter; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
docs/draft/docs/style.css(1 hunks)libs/enclave/src/__tests__/enclave.ai-context.spec.ts(1 hunks)libs/enclave/src/__tests__/enclave.attack-matrix.spec.ts(3 hunks)libs/enclave/src/__tests__/enclave.combined-attacks.spec.ts(5 hunks)libs/enclave/src/__tests__/enclave.helper-escape.spec.ts(2 hunks)libs/enclave/src/__tests__/enclave.host-api.spec.ts(1 hunks)libs/enclave/src/__tests__/enclave.infinite-loop-attacks.spec.ts(1 hunks)libs/enclave/src/__tests__/enclave.io-flood.spec.ts(1 hunks)libs/enclave/src/__tests__/enclave.multi-tenant.spec.ts(2 hunks)libs/enclave/src/__tests__/enclave.security-levels.spec.ts(2 hunks)libs/enclave/src/__tests__/enclave.security.spec.ts(3 hunks)libs/enclave/src/__tests__/enclave.serialization-attacks.spec.ts(1 hunks)libs/enclave/src/__tests__/enclave.serialization.spec.ts(1 hunks)libs/enclave/src/__tests__/enclave.spec.ts(1 hunks)libs/enclave/src/__tests__/safe-runtime-reference.spec.ts(6 hunks)libs/enclave/src/__tests__/stack-trace-sanitization.spec.ts(2 hunks)libs/enclave/src/adapters/vm-adapter.ts(9 hunks)libs/enclave/src/adapters/worker-pool/config.ts(1 hunks)libs/enclave/src/adapters/worker-pool/errors.ts(1 hunks)libs/enclave/src/adapters/worker-pool/execution-queue.ts(1 hunks)libs/enclave/src/adapters/worker-pool/index.ts(1 hunks)libs/enclave/src/adapters/worker-pool/memory-monitor.ts(1 hunks)libs/enclave/src/adapters/worker-pool/protocol.ts(1 hunks)libs/enclave/src/adapters/worker-pool/rate-limiter.ts(1 hunks)libs/enclave/src/adapters/worker-pool/safe-deserialize.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-script.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-slot.ts(1 hunks)libs/enclave/src/enclave.ts(15 hunks)libs/enclave/src/globals-validator.ts(12 hunks)libs/enclave/src/index.ts(3 hunks)libs/enclave/src/safe-runtime.ts(12 hunks)libs/enclave/src/scoring/__tests__/feature-extractor.spec.ts(1 hunks)libs/enclave/src/scoring/__tests__/rule-based-scorer.spec.ts(1 hunks)libs/enclave/src/scoring/__tests__/scoring-gate.spec.ts(1 hunks)libs/enclave/src/scoring/cache.ts(1 hunks)libs/enclave/src/scoring/feature-extractor.ts(1 hunks)libs/enclave/src/scoring/index.ts(1 hunks)libs/enclave/src/scoring/scorer.interface.ts(1 hunks)libs/enclave/src/scoring/scorers/disabled.scorer.ts(1 hunks)libs/enclave/src/scoring/scorers/external-api.scorer.ts(1 hunks)libs/enclave/src/scoring/scorers/index.ts(1 hunks)libs/enclave/src/scoring/scorers/rule-based.scorer.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- libs/enclave/src/tests/enclave.serialization-attacks.spec.ts
- libs/enclave/src/tests/enclave.multi-tenant.spec.ts
- libs/enclave/src/tests/enclave.ai-context.spec.ts
- libs/enclave/src/globals-validator.ts
- libs/enclave/src/tests/enclave.spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/scoring/scorers/index.tslibs/enclave/src/__tests__/enclave.infinite-loop-attacks.spec.tslibs/enclave/src/scoring/scorers/disabled.scorer.tslibs/enclave/src/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/scoring/scorers/rule-based.scorer.tslibs/enclave/src/__tests__/enclave.host-api.spec.tslibs/enclave/src/adapters/worker-pool/worker-slot.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.helper-escape.spec.tslibs/enclave/src/adapters/worker-pool/rate-limiter.tslibs/enclave/src/adapters/worker-pool/errors.tslibs/enclave/src/__tests__/enclave.security-levels.spec.tslibs/enclave/src/scoring/cache.tslibs/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/__tests__/enclave.security.spec.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/__tests__/enclave.combined-attacks.spec.tslibs/enclave/src/adapters/worker-pool/config.tslibs/enclave/src/scoring/__tests__/feature-extractor.spec.tslibs/enclave/src/scoring/scorer.interface.tslibs/enclave/src/scoring/scorers/external-api.scorer.tslibs/enclave/src/__tests__/enclave.serialization.spec.tslibs/enclave/src/adapters/vm-adapter.tslibs/enclave/src/scoring/index.tslibs/enclave/src/__tests__/enclave.io-flood.spec.tslibs/enclave/src/__tests__/enclave.attack-matrix.spec.tslibs/enclave/src/adapters/worker-pool/execution-queue.tslibs/enclave/src/safe-runtime.tslibs/enclave/src/adapters/worker-pool/index.tslibs/enclave/src/scoring/__tests__/rule-based-scorer.spec.tslibs/enclave/src/scoring/feature-extractor.tslibs/enclave/src/adapters/worker-pool/memory-monitor.tslibs/enclave/src/index.tslibs/enclave/src/adapters/worker-pool/protocol.tslibs/enclave/src/enclave.tslibs/enclave/src/__tests__/safe-runtime-reference.spec.tslibs/enclave/src/adapters/worker-pool/safe-deserialize.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/enclave/src/scoring/scorers/index.tslibs/enclave/src/__tests__/enclave.infinite-loop-attacks.spec.tslibs/enclave/src/scoring/scorers/disabled.scorer.tslibs/enclave/src/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/scoring/scorers/rule-based.scorer.tslibs/enclave/src/__tests__/enclave.host-api.spec.tslibs/enclave/src/adapters/worker-pool/worker-slot.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.helper-escape.spec.tslibs/enclave/src/adapters/worker-pool/rate-limiter.tslibs/enclave/src/adapters/worker-pool/errors.tslibs/enclave/src/__tests__/enclave.security-levels.spec.tslibs/enclave/src/scoring/cache.tslibs/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/__tests__/enclave.security.spec.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/__tests__/enclave.combined-attacks.spec.tslibs/enclave/src/adapters/worker-pool/config.tslibs/enclave/src/scoring/__tests__/feature-extractor.spec.tslibs/enclave/src/scoring/scorer.interface.tslibs/enclave/src/scoring/scorers/external-api.scorer.tslibs/enclave/src/__tests__/enclave.serialization.spec.tslibs/enclave/src/adapters/vm-adapter.tslibs/enclave/src/scoring/index.tslibs/enclave/src/__tests__/enclave.io-flood.spec.tslibs/enclave/src/__tests__/enclave.attack-matrix.spec.tslibs/enclave/src/adapters/worker-pool/execution-queue.tslibs/enclave/src/safe-runtime.tslibs/enclave/src/adapters/worker-pool/index.tslibs/enclave/src/scoring/__tests__/rule-based-scorer.spec.tslibs/enclave/src/scoring/feature-extractor.tslibs/enclave/src/adapters/worker-pool/memory-monitor.tslibs/enclave/src/index.tslibs/enclave/src/adapters/worker-pool/protocol.tslibs/enclave/src/enclave.tslibs/enclave/src/__tests__/safe-runtime-reference.spec.tslibs/enclave/src/adapters/worker-pool/safe-deserialize.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/style.css
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/style.css
**/{libs,apps}/**/src/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/{libs,apps}/**/src/index.ts: Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Files:
libs/enclave/src/index.ts
🧠 Learnings (15)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Export everything users need from barrel files (index.ts), without legacy aliases or deprecated names
Applied to files:
libs/enclave/src/scoring/scorers/index.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.security-levels.spec.tslibs/enclave/src/scoring/index.tslibs/enclave/src/adapters/worker-pool/index.tslibs/enclave/src/index.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/{libs,apps}/**/src/index.ts : Do not add backwards compatibility exports or aliases (legacy exports) in new libraries
Applied to files:
libs/enclave/src/scoring/scorers/index.tslibs/enclave/src/__tests__/enclave.security-levels.spec.tslibs/enclave/src/index.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/enclave/src/__tests__/enclave.infinite-loop-attacks.spec.tslibs/enclave/src/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/__tests__/enclave.host-api.spec.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.security-levels.spec.tslibs/enclave/src/__tests__/enclave.security.spec.tslibs/enclave/src/__tests__/enclave.combined-attacks.spec.tslibs/enclave/src/scoring/__tests__/feature-extractor.spec.tslibs/enclave/src/__tests__/enclave.io-flood.spec.tslibs/enclave/src/__tests__/enclave.attack-matrix.spec.tslibs/enclave/src/scoring/__tests__/rule-based-scorer.spec.tslibs/enclave/src/__tests__/safe-runtime-reference.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
libs/enclave/src/__tests__/enclave.infinite-loop-attacks.spec.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.helper-escape.spec.tslibs/enclave/src/__tests__/enclave.security.spec.tslibs/enclave/src/__tests__/enclave.combined-attacks.spec.tslibs/enclave/src/__tests__/enclave.io-flood.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and edge conditions
Applied to files:
libs/enclave/src/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/enclave/src/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/scoring/__tests__/feature-extractor.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Include constructor validation tests and error class instanceof checks in all test suites
Applied to files:
libs/enclave/src/scoring/__tests__/scoring-gate.spec.tslibs/enclave/src/__tests__/enclave.host-api.spec.tslibs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.io-flood.spec.tslibs/enclave/src/scoring/__tests__/rule-based-scorer.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
libs/enclave/src/__tests__/stack-trace-sanitization.spec.tslibs/enclave/src/__tests__/enclave.security-levels.spec.tslibs/enclave/src/scoring/index.tslibs/enclave/src/enclave.tslibs/enclave/src/__tests__/safe-runtime-reference.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.test.{ts,tsx} : Do not use prefixes like PT-001 in test names - use descriptive test names instead
Applied to files:
libs/enclave/src/__tests__/stack-trace-sanitization.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Applied to files:
libs/enclave/src/__tests__/stack-trace-sanitization.spec.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/enclave/src/__tests__/stack-trace-sanitization.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific MCP error classes with error codes from MCP_ERROR_CODES instead of generic Error classes
Applied to files:
libs/enclave/src/adapters/worker-pool/errors.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Applied to files:
libs/enclave/src/adapters/worker-pool/errors.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/adapters/vm-adapter.tslibs/enclave/src/safe-runtime.tslibs/enclave/src/__tests__/safe-runtime-reference.spec.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/adapters/src/**/*.ts : Do not hardcode capabilities in adapters - use registry.getCapabilities() for dynamic capability exposure
Applied to files:
libs/enclave/src/adapters/worker-pool/index.tslibs/enclave/src/enclave.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types must use typed protocol definitions (GetPromptResult, ReadResourceResult) instead of `unknown` for SDK methods
Applied to files:
libs/enclave/src/adapters/worker-pool/protocol.ts
🧬 Code graph analysis (13)
libs/enclave/src/scoring/scorers/disabled.scorer.ts (4)
libs/enclave/src/index.ts (4)
DisabledScorer(50-50)BaseScorer(48-48)ExtractedFeatures(68-68)ScoringResult(76-76)libs/enclave/src/scoring/index.ts (4)
DisabledScorer(41-41)BaseScorer(37-37)ExtractedFeatures(52-52)ScoringResult(61-61)libs/enclave/src/scoring/scorers/index.ts (1)
DisabledScorer(7-7)libs/enclave/src/scoring/types.ts (2)
ExtractedFeatures(192-217)ScoringResult(252-277)
libs/enclave/src/scoring/scorers/rule-based.scorer.ts (3)
libs/enclave/src/scoring/index.ts (8)
RuleId(49-49)RuleBasedScorer(41-41)ExtractedFeatures(52-52)ScoringResult(61-61)RiskSignal(60-60)RULE_THRESHOLDS(72-72)FeatureExtractor(31-31)RiskLevel(47-47)libs/enclave/src/scoring/types.ts (6)
RuleId(37-45)ExtractedFeatures(192-217)ScoringResult(252-277)RiskSignal(222-247)RULE_THRESHOLDS(505-525)RiskLevel(23-23)libs/enclave/src/scoring/feature-extractor.ts (1)
FeatureExtractor(57-485)
libs/enclave/src/__tests__/enclave.security.spec.ts (1)
libs/enclave/src/enclave.ts (1)
Enclave(140-550)
libs/enclave/src/adapters/worker-pool/worker-script.ts (4)
libs/enclave/src/adapters/worker-pool/protocol.ts (6)
WorkerExecutionStats(31-42)SerializedConfig(48-67)MainToWorkerMessage(129-134)ExecuteMessage(76-84)ToolResponseMessage(89-99)SerializedError(17-26)libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (3)
safeDeserialize(41-57)sanitizeObject(124-151)safeSerialize(99-107)libs/enclave/src/adapters/worker-pool/worker-slot.ts (1)
sendMessage(268-274)libs/enclave/src/sidecar/reference-resolver.ts (1)
resolve(171-217)
libs/enclave/src/scoring/scorer.interface.ts (4)
libs/enclave/src/scoring/types.ts (3)
ScorerType(18-18)ExtractedFeatures(192-217)ScoringResult(252-277)libs/enclave/src/scoring/scorers/disabled.scorer.ts (1)
score(25-33)libs/enclave/src/scoring/scorers/external-api.scorer.ts (1)
score(49-83)libs/enclave/src/scoring/scorers/rule-based.scorer.ts (1)
score(73-114)
libs/enclave/src/adapters/vm-adapter.ts (3)
libs/sdk/src/errors/error-handler.ts (1)
wrap(80-94)libs/enclave/src/index.ts (2)
ExecutionContext(26-26)ExecutionResult(19-19)libs/enclave/src/types.ts (2)
ExecutionContext(475-516)ExecutionResult(238-263)
libs/enclave/src/__tests__/enclave.io-flood.spec.ts (2)
libs/enclave/src/enclave.ts (1)
Enclave(140-550)libs/enclave/src/index.ts (1)
Enclave(15-15)
libs/enclave/src/safe-runtime.ts (3)
libs/enclave/src/index.ts (4)
createSafeRuntime(86-86)ExecutionContext(26-26)SafeRuntimeOptions(86-86)ReferenceResolver(105-105)libs/enclave/src/types.ts (1)
ExecutionContext(475-516)libs/enclave/src/sidecar/reference-resolver.ts (1)
ReferenceResolver(75-314)
libs/enclave/src/scoring/__tests__/rule-based-scorer.spec.ts (2)
libs/enclave/src/scoring/scorers/rule-based.scorer.ts (1)
RuleBasedScorer(61-364)libs/enclave/src/scoring/feature-extractor.ts (1)
FeatureExtractor(57-485)
libs/enclave/src/scoring/feature-extractor.ts (1)
libs/enclave/src/scoring/types.ts (8)
SensitiveCategory(28-32)ExtractedFeatures(192-217)ExtractedToolCall(50-91)PatternSignals(96-127)NumericSignals(132-152)SensitiveAccess(157-167)ExtractionMeta(172-187)RULE_THRESHOLDS(505-525)
libs/enclave/src/adapters/worker-pool/protocol.ts (1)
libs/enclave/src/adapters/worker-pool/config.ts (1)
ResourceUsage(106-117)
libs/enclave/src/__tests__/safe-runtime-reference.spec.ts (2)
libs/enclave/src/index.ts (1)
ExecutionContext(26-26)libs/enclave/src/types.ts (1)
ExecutionContext(475-516)
libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (1)
libs/enclave/src/adapters/worker-pool/errors.ts (2)
MessageSizeError(146-158)MessageValidationError(136-141)
🪛 ESLint
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
[error] 160-160: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 297-297: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 334-334: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/enclave/src/scoring/scorers/external-api.scorer.ts (1)
85-136: TightenApiResponsevalidation, especiallysignalsentriesThe extra checks for
score,riskvalue, andsignalsbeing an array are a good improvement, but malformed signal objects can still slip through (e.g., missingid, non‑numericscore, invalidlevel), which may surface as harder‑to‑debug issues later.You can extend the runtime validation a bit further:
- // Validate risk level if present - const validRiskLevels = ['none', 'low', 'medium', 'high', 'critical']; + // Validate risk level if present + const validRiskLevels: readonly RiskLevel[] = ['none', 'low', 'medium', 'high', 'critical']; if (data.risk !== undefined && !validRiskLevels.includes(data.risk)) { throw new Error(`Invalid API response: invalid risk level "${data.risk}"`); } // Validate signals array if present if (data.signals !== undefined && !Array.isArray(data.signals)) { throw new Error('Invalid API response: signals must be an array'); } + + if (Array.isArray(data.signals)) { + data.signals.forEach((signal, index) => { + if (typeof signal.id !== 'string' || typeof signal.description !== 'string') { + throw new Error(`Invalid API response: signals[${index}] must have string id and description`); + } + if (typeof signal.score !== 'number') { + throw new Error(`Invalid API response: signals[${index}].score must be a number`); + } + if (!validRiskLevels.includes(signal.level)) { + throw new Error(`Invalid API response: signals[${index}].level "${signal.level}" is invalid`); + } + }); + }This keeps the external API boundary robust and fails fast with clear messages when the upstream contract is violated.
🧹 Nitpick comments (9)
libs/enclave/src/scoring/scorers/external-api.scorer.ts (2)
16-26: AlignApiResponse.risktyping with runtime fallback behaviorAt runtime you treat
riskas optional (fallback tocalculateRiskLevelwhen missing and validate only when defined), but the interface declares it as required. This mismatch can mislead future callers ofApiResponse.Consider updating the schema to reflect the actual contract:
-interface ApiResponse { - score: number; - risk: RiskLevel; +interface ApiResponse { + score: number; + risk?: RiskLevel; signals?: Array<{ id: string; score: number; description: string; level: RiskLevel; }>; error?: string; }This keeps the types honest while preserving the current behavior of deriving a risk level when the API omits it.
49-76: Consider differentiating retriable vs. non‑retriable failures inscoreThe retry loop currently reattempts on any thrown error, including likely non‑transient cases such as 4xx responses (bad request, auth issues) from the API. That can waste latency and load on a permanently failing configuration.
You could classify errors in
callApi(e.g., based on HTTP status) and skip retries for clearly non‑retriable conditions, while keeping the current behavior for network/5xx/timeout cases.libs/enclave/src/adapters/worker-pool/worker-slot.ts (2)
180-194: Previous memory leak concern has been addressed.The listener is now properly removed on timeout (line 188:
this.off('ready', onReady)). However, there's still a potential edge case: if the worker crashes (emitting 'error' or 'exit') before emitting 'ready', thewaitForReadypromise will hang until the timeout fires rather than rejecting immediately.Consider also listening for worker exit/error to reject early:
private waitForReady(): Promise<void> { return new Promise((resolve, reject) => { + const cleanup = () => { + clearTimeout(timeout); + this.off('ready', onReady); + this.off('exit', onExit); + }; + const onReady = () => { - clearTimeout(timeout); + cleanup(); resolve(); }; + const onExit = (code: number | null) => { + cleanup(); + reject(new WorkerStartupError(`Worker ${this.id} exited during startup with code ${code}`)); + }; const timeout = setTimeout(() => { - this.off('ready', onReady); + cleanup(); reject(new WorkerStartupError(`Worker ${this.id} startup timeout`)); }, this.options.startupTimeoutMs); this.once('ready', onReady); + this.once('exit', onExit); }); }
250-259: Minor:once('exit')listener not cleaned up after timeout.If the 5-second timeout wins the race, the
once('exit')listener remains attached until the worker eventually exits. This is a minor issue since the worker will likely terminate shortly after, but for cleanliness:// Wait for graceful exit (with timeout) + let exitListener: (() => void) | null = null; await Promise.race([ new Promise<void>((resolve) => { if (worker) { - worker.once('exit', () => resolve()); + exitListener = () => resolve(); + worker.once('exit', exitListener); } else { resolve(); } }), - new Promise<void>((resolve) => setTimeout(resolve, 5000)), + new Promise<void>((resolve) => setTimeout(() => { + if (exitListener && worker) { + worker.off('exit', exitListener); + } + resolve(); + }, 5000)), ]);libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (3)
206-218: Consider adding recursion depth limit for slot acquisition.The recursive call on line 213 could theoretically lead to stack overflow if slots keep getting recycled/removed in rapid succession (e.g., during a memory pressure scenario). While unlikely in practice, adding a depth limit would make this more robust.
- private async acquireSlot(signal?: AbortSignal): Promise<WorkerSlot> { + private async acquireSlot(signal?: AbortSignal, retries = 3): Promise<WorkerSlot> { // ... existing code ... // Double check slot is still available const slot = this.slots.get(slotId); if (!slot || !slot.isIdle) { // Slot was removed/recycled, try again + if (retries <= 0) { + throw new Error('Failed to acquire worker slot after multiple attempts'); + } - return this.acquireSlot(signal); + return this.acquireSlot(signal, retries - 1); }
296-305: Potential race: slot removed from map before termination completes.The slot is deleted from the map immediately (line 302) while
terminateruns asynchronously. If the termination process emits events or the slot is looked up by other code paths during this window, inconsistent state could occur.Consider either awaiting termination or at least ensuring the slot's status is checked before removal:
for (let i = 0; i < excessCount; i++) { const slot = idleSlots[i]; if (now - slot.lastActiveAt > this.config.idleTimeoutMs) { - slot.terminate(true).catch((error) => { - console.error(`Failed to terminate idle worker ${slot.id}:`, error); - }); this.slots.delete(slot.id); + slot.terminate(true).catch((error) => { + console.error(`Failed to terminate idle worker ${slot.id}:`, error); + }); } }This ensures the slot is removed from the map first, preventing other code from acquiring it during termination.
159-163:Promise.allSettlednever rejects, so.catchis unreachable.
Promise.allSettledalways resolves (never rejects), so the.catchhandler on line 160-162 will never execute.// Don't await - just trigger termination - Promise.allSettled(terminatePromises).catch((error) => { - console.error('Error during worker pool disposal:', error); - }); + Promise.allSettled(terminatePromises).then((results) => { + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + console.error(`${failures.length} worker(s) failed to terminate during disposal`); + } + });libs/enclave/src/adapters/worker-pool/worker-script.ts (1)
489-495: Byte counting uses string length instead of actual byte length.Line 490 uses
s?.lengthwhich counts characters, not bytes. For strings with multi-byte UTF-8 characters, this would undercount the actual size. While this is a minor discrepancy (security limit is still enforced, just slightly loose), it could be made more accurate.- const bytes = serialized.reduce((sum, s) => sum + (s?.length ?? 0), 0); + const bytes = serialized.reduce((sum, s) => sum + Buffer.byteLength(s ?? '', 'utf-8'), 0);Note: If
Bufferis not available in the sandbox, the current implementation is acceptable as a conservative approximation.libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (1)
65-92: Consider consolidating duplicate sanitization logic.
sanitizeObjectWithDepthCheck(lines 65-92) andsanitizeObject(lines 127-154) contain nearly identical logic, differing only in how they handle depth exceeded (throw vs. return undefined). This duplication could lead to inconsistencies if one is updated but not the other.Consider unifying with a parameter:
function sanitizeObjectCore( value: unknown, depth: number, throwOnDepthExceeded: boolean ): unknown { if (depth > MAX_DEPTH) { if (throwOnDepthExceeded) { throw new MessageValidationError(`Message exceeds maximum depth of ${MAX_DEPTH}`); } return undefined; } // ... rest of shared logic }Also applies to: 127-154
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/enclave/src/adapters/worker-pool/protocol.ts(1 hunks)libs/enclave/src/adapters/worker-pool/safe-deserialize.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-script.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-slot.ts(1 hunks)libs/enclave/src/scoring/scorers/external-api.scorer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/enclave/src/adapters/worker-pool/protocol.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/adapters/worker-pool/worker-slot.tslibs/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/scoring/scorers/external-api.scorer.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/adapters/worker-pool/safe-deserialize.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/enclave/src/adapters/worker-pool/worker-slot.tslibs/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/scoring/scorers/external-api.scorer.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/adapters/worker-pool/safe-deserialize.ts
🧠 Learnings (1)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Applied to files:
libs/enclave/src/adapters/worker-pool/worker-script.ts
🧬 Code graph analysis (3)
libs/enclave/src/adapters/worker-pool/worker-slot.ts (4)
libs/enclave/src/adapters/worker-pool/protocol.ts (2)
WorkerToMainMessage(204-209)MainToWorkerMessage(129-134)libs/enclave/src/adapters/worker-pool/config.ts (2)
WorkerSlotStatus(101-101)ResourceUsage(106-117)libs/enclave/src/adapters/worker-pool/errors.ts (2)
WorkerStartupError(163-172)WorkerCrashedError(57-66)libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (2)
safeSerialize(102-110)safeDeserialize(41-60)
libs/enclave/src/scoring/scorers/external-api.scorer.ts (1)
libs/enclave/src/scoring/types.ts (4)
RiskLevel(23-23)ExternalApiConfig(367-395)ExtractedFeatures(192-217)ScoringResult(252-277)
libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (1)
libs/enclave/src/adapters/worker-pool/errors.ts (2)
MessageSizeError(146-158)MessageValidationError(136-141)
⏰ 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
🔇 Additional comments (10)
libs/enclave/src/adapters/worker-pool/worker-slot.ts (3)
289-308: Previous memory leak concern has been addressed.The handler is now properly removed on timeout (line 301). The implementation correctly handles both success and timeout paths.
314-329: LGTM: Defensive message handling with protocol error recovery.The implementation correctly:
- Uses
safeDeserializeto prevent prototype pollution- Handles the
readymessage to signal worker startup- Marks the slot for recycling on parse errors (defensive)
362-370: LGTM: Proper cleanup of event handlers.The cleanup method correctly removes all bound handlers and nulls the worker reference, preventing memory leaks and dangling listeners.
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (2)
256-275: Previous memory cleanup concern has been addressed.The
handleMemoryExceededmethod now properly removes the slot from the pool and resets the rate limiter in both success and error paths.
395-437: LGTM: Proper tool call handling with sanitization.The implementation correctly:
- Limits pending tool calls to prevent resource exhaustion
- Sanitizes arguments before passing to the handler
- Sanitizes results before sending back to the worker
- Uses
finallyto ensure pending call cleanuplibs/enclave/src/adapters/worker-pool/worker-script.ts (3)
186-196: Previous prefix matching issue has been addressed.Line 190 now correctly uses delimiter-based matching (
${msg.requestId}-) to prevent false positives when one requestId is a prefix of another.
275-329: LGTM: Well-designed sandbox with security considerations.The sandbox implementation:
- Uses
Object.create(null)to prevent prototype chain attacks- Intentionally excludes timing APIs (
Date,setTimeout, etc.) to prevent timing attacks- Only allows serializable (non-function) custom globals, which are sanitized
- Provides controlled versions of all exposed functionality
366-377: Based on my verification, the review comment is accurate and valid. I've confirmed:
- Tool calls have no individual timeout mechanism -
SerializedConfigcontains only antimeoutfield for execution-level timeout, no per-tool-call timeout field- Cleanup only happens when tool responses arrive, execution completes, or abort is called - not on any tool-call-specific timeout
- Worker blocking risk is real - if a tool call hangs without responding, the pending promise remains in
pendingToolCallsand blocks the worker thread until the overall execution timeout fires- No documented justification - no comments or documentation explaining this design choice
The concern is legitimate and reflects a potential resilience gap. The design relies entirely on the execution-level timeout to eventually free blocked tool calls.
Pending tool calls have no individual timeout.
Tool call promises in
pendingToolCallsare cleaned up only when: (1) the execution timeout fires, (2) a tool response is received, or (3) execution is aborted. If a tool call hangs on the main thread without responding, the worker blocks indefinitely until the VM timeout expires. This design choice is not documented. Consider adding per-tool-call timeouts for improved resilience, or document why execution-level timeout is considered sufficient.libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (2)
14-20: LGTM: Core security constants are well-defined.The
DANGEROUS_KEYSset covers the primary prototype pollution vectors (__proto__,constructor,prototype), andMAX_DEPTH = 50is a reasonable limit to prevent stack overflow from deeply nested objects.
102-110: LGTM: Safe serialization with dangerous key filtering.The implementation correctly uses
JSON.stringify's replacer function to strip dangerous keys during serialization. Circular references will throw naturally fromJSON.stringify, which is appropriate behavior.
…rker pool error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
docs/draft/docs/plugins/codecall/security.mdx (1)
78-78: Minor: Replace intensifier "Very" with a more specific descriptor.Line 78 uses "Very large inputs" which LanguageTool flags as an over-used intensifier. Consider a more measured or specific alternative:
-| Max Nesting Depth | 200 levels | Prevents stack overflow | -| Max Line Length | 100,000 chars | Handles minified code safely | +| Max Line Length | 100,000 chars | Handles minified code safely |Or more specifically:
-3. **Memory Exhaustion**: Very large inputs can exhaust memory before validation +3. **Memory Exhaustion**: Large inputs can exhaust memory before validationThis is a minor style improvement for technical documentation consistency.
libs/enclave/src/scoring/scorers/external-api.scorer.ts (2)
79-82: Simplify redundant null coalescing.The expression
lastError ?? undefinedis redundant becauselastErroris typed asError | null, and thecauseparameter in the constructor is already optional. WhenlastErrorisnull, passingnullorundefinedhas the same effect.Apply this diff:
throw new ExternalApiScorerError( `Failed to score after ${maxAttempts} attempts: ${lastError?.message}`, - lastError ?? undefined, + lastError || undefined, );
121-125: Based on my verification, I have all the information needed to rewrite the review comment.The hardcoded risk level validation is maintainable as-is, but a type guard or validation schema would improve consistency.
The
RiskLeveltype is defined as a union inlibs/enclave/src/scoring/types.tsand the validation array at lines 122-125 matches it exactly. Since this validation appears only once and zod is already available in the project, the suggested refactoring is optional. However, extracting the validation to a type guard or zod schema would prevent accidental divergence ifRiskLevelchanges:// Option 1: Type guard function isValidRiskLevel(level: unknown): level is RiskLevel { return ['none', 'low', 'medium', 'high', 'critical'].includes(level as string); } // Option 2: Zod schema (already used elsewhere in libs/enclave) const riskLevelSchema = z.enum(['none', 'low', 'medium', 'high', 'critical']);libs/enclave/src/adapters/worker-pool/worker-script.ts (1)
336-513: LGTM! Comprehensive safety enforcement.The safe runtime functions effectively enforce security constraints through input validation, abort checking, and limit enforcement. The callId format at line 365 correctly uses the delimiter pattern for cleanup. The duplicate logic across the three safe loop constructs could be extracted to a shared helper function for maintainability, but the current implementation is clear and correct.
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (2)
175-191: Consider using full UUID for slot IDs.Line 176 generates slot IDs using only 8 characters of the UUID (
crypto.randomUUID().slice(0, 8)), providing only 32 bits of entropy. While collisions are unlikely in most scenarios, under high load where multiple slots are created within the same millisecond, the birthday paradox could cause ID collisions, potentially causing slot confusion in the maps and rate limiter.Consider using the full UUID for maximum collision resistance:
- const slotId = `worker-${Date.now()}-${crypto.randomUUID().slice(0, 8)}`; + const slotId = `worker-${Date.now()}-${crypto.randomUUID()}`;
314-398: LGTM! Comprehensive execution orchestration with proper cleanup.The executeInSlot method correctly implements the full execution flow with watchdog timeout, message routing, and cleanup. The error handler is now properly removed on successful completion (line 362), addressing previous review feedback.
Consider using the full UUID for request IDs on line 319 for the same collision resistance reasons noted in the slot ID comment:
- const requestId = `exec-${Date.now()}-${crypto.randomUUID().slice(0, 8)}`; + const requestId = `exec-${Date.now()}-${crypto.randomUUID()}`;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/draft/assets/banners/codecall-plugin/security-layers.dark.pngis excluded by!**/*.pngdocs/draft/assets/banners/codecall-plugin/security-layers.light.pngis excluded by!**/*.png
📒 Files selected for processing (5)
docs/draft/docs/plugins/codecall/overview.mdx(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)libs/enclave/src/adapters/worker-pool/worker-script.ts(1 hunks)libs/enclave/src/scoring/scorers/external-api.scorer.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/codecall/security.mdxdocs/draft/docs/plugins/codecall/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/codecall/security.mdxdocs/draft/docs/plugins/codecall/overview.mdx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/scoring/scorers/external-api.scorer.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/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/adapters/worker-pool/worker-script.tslibs/enclave/src/scoring/scorers/external-api.scorer.ts
🧠 Learnings (3)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
📚 Learning: 2025-11-16T22:47:05.139Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 50
File: docs/docs/v/0.1/getting-started/welcome.mdx:2-3
Timestamp: 2025-11-16T22:47:05.139Z
Learning: In the agentfront/frontmcp repository, the docs/docs/v/** directory governance rule prohibiting manual edits applies to already-frozen releases, but does not preclude initial setup and bootstrap commits that establish the versioned docs structure.
Applied to files:
docs/draft/docs/plugins/codecall/overview.mdx
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/sdk/src/**/*.ts : Create shared base classes (ExecutionContextBase) for common execution context functionality across different context types
Applied to files:
libs/enclave/src/adapters/worker-pool/worker-script.ts
🪛 LanguageTool
docs/draft/docs/plugins/codecall/security.mdx
[style] ~78-~78: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Very large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
⏰ 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
🔇 Additional comments (22)
docs/draft/docs/plugins/codecall/overview.mdx (1)
1-304: Excellent overview documentation with clear narrative and practical examples.The documentation provides a well-structured introduction to CodeCall that effectively communicates the problem (tool explosion), solution (4 meta-tools), and implementation details. The quick start example is practical, the architecture section clearly explains the underlying libraries, and the decision tree (Use CodeCall When/Skip CodeCall When) helps users understand applicability. Internal links to guides and supporting documentation are correctly structured.
docs/draft/docs/plugins/codecall/security.mdx (1)
1-810: Comprehensive security documentation with excellent coverage of defense-in-depth architecture.The security documentation effectively explains all six layers of the CodeCall security pipeline, with particular strength in the new Layer 0 Pre-Scanner and Layer 3 AI Scoring Gate sections. The documentation balances detail with accessibility—accordions hide implementation specifics while keeping key concepts visible. Tables clearly specify limits, presets, and configuration options. The threat model section appropriately sets expectations about what CodeCall does and does not protect against. Prior inconsistencies regarding input size limits and terminology have been resolved.
libs/enclave/src/scoring/scorers/external-api.scorer.ts (3)
138-140: LGTM!The delay helper is correctly implemented for the exponential backoff mechanism.
143-154: LGTM!The custom error class properly extends
Error, sets the error name, and handles the optional cause parameter correctly.
37-47: Fix authorization header logic to handle missing API keys correctly.The default value of empty string for
apiKeyon line 41 will cause the conditional check on lines 94-96 to incorrectly add anAuthorization: Bearerheader (with no token) when no API key is provided. Empty strings are truthy in JavaScript.Apply this diff to fix the authorization logic:
this.config = { endpoint: config.endpoint, - apiKey: config.apiKey ?? '', + apiKey: config.apiKey, timeoutMs: config.timeoutMs ?? 5000, headers: config.headers ?? {}, retries: config.retries ?? 1, };Then update the type on line 35 to reflect that
apiKeycan be undefined:- private readonly config: Required<ExternalApiConfig>; + private readonly config: Required<Omit<ExternalApiConfig, 'apiKey'>> & Pick<ExternalApiConfig, 'apiKey'>;Or more simply, adjust the conditional on lines 94-96 to check for non-empty strings:
- ...(this.config.apiKey && { + ...(this.config.apiKey && this.config.apiKey.trim() !== '' && { Authorization: `Bearer ${this.config.apiKey}`, }),Likely an incorrect or invalid review comment.
libs/enclave/src/adapters/worker-pool/worker-script.ts (8)
27-62: LGTM! Solid security-first initialization.The approach of capturing
parentPortbefore removing dangerous globals, combined with non-writable and non-configurable property definitions, effectively prevents user code from accessing worker threading APIs or shared memory primitives. The try-catch handling for already-constrained properties is good defensive programming.
68-113: LGTM! Clean message-driven architecture.The execution state management and message dispatching are well-structured. The error handling in the message handler prevents the worker from crashing on unexpected messages while still logging for debugging.
119-197: LGTM! Robust execution lifecycle with proper cleanup.The execute handler correctly manages the full lifecycle from initialization through cleanup. The delimiter-based pending tool call cleanup (line 190) properly prevents false positive matches, as fixed in previous reviews.
203-256: LGTM! Consistent cleanup patterns across handlers.The tool response handler properly manages the pending call lifecycle, and the abort handler uses the same delimiter-based cleanup pattern as the execute handler, ensuring consistency and correctness.
262-270: LGTM! Appropriate termination semantics.The distinction between graceful termination (allowing current execution to finish) and forced termination (immediate exit) provides the necessary control for different shutdown scenarios. The use of
process.exit(0)is acceptable in a worker thread context.
276-330: LGTM! Well-designed sandbox with appropriate restrictions.The sandbox construction uses
Object.create(null)for prototype isolation and carefully filters out functions from custom globals while sanitizing values. The safe runtime function injection provides controlled access to iteration and tool-calling capabilities.
519-548: LGTM! Appropriate error serialization with configurable sanitization.The utility functions correctly handle message serialization and error formatting. The stack trace sanitization at lines 540-544 intentionally limits detail when the security flag is set, which is the correct tradeoff between debuggability and information disclosure.
550-554: LGTM! Proper initialization signal.The ready signal correctly indicates that the worker has completed initialization and is ready to receive execute messages.
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (9)
46-82: LGTM! Clean architecture with proper component integration.The constructor appropriately initializes all subsystems (queue, memory monitor, rate limiter) and establishes event-driven communication for memory management. The metrics counters are properly initialized.
88-139: LGTM! Robust initialization and execution lifecycle.The execute method properly manages the full lifecycle from slot acquisition through cleanup, with appropriate error handling and metrics tracking. The abort signal integration allows for execution cancellation.
144-169: LGTM! Appropriate disposal semantics.The dispose method correctly tears down all components and triggers worker termination. Not awaiting termination promises is acceptable for disposal scenarios where immediate cleanup is desired, and failure logging provides visibility.
193-221: LGTM! Proper slot acquisition with queue fallback.The slot acquisition logic correctly implements the priority: idle slots → new slots → queued requests. The recursive retry at line 216 handles the edge case where a slot is removed between queue resolution and acquisition. The recursion is bounded by the abort signal and should rarely exceed depth 1 in practice.
223-257: LGTM! Clean slot lifecycle management.The release and recycling logic properly manages the slot lifecycle, checking execution counts and creating replacements to maintain the minimum pool size.
259-278: LGTM! Memory exceeded handling now properly cleans up.The memory exceeded handler correctly addresses the previous review feedback by deleting the slot from the map in both success and error paths (lines 269 and 274), preventing dangling terminated slots.
280-308: LGTM! Efficient idle worker management.The idle slot cleanup correctly maintains the minimum pool size while removing workers that have been idle beyond the timeout threshold.
400-450: LGTM! Secure tool call handling with proper sanitization.The tool call handler implements defense in depth by sanitizing both inputs (line 422) and outputs (line 439), checking pending call limits to prevent resource exhaustion, and properly handling errors. The console forwarding provides visibility into worker output.
452-546: LGTM! Complete metrics and result handling.The result builder properly shapes responses for both success and error cases, preserving execution statistics. The metrics aggregation provides comprehensive visibility into pool health and performance across all subsystems.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (3)
193-221: Consider adding recursion guard in acquireSlot.The double-check pattern at lines 212-217 recursively calls
acquireSlotif the slot is no longer available. While unlikely, rapid slot recycling could theoretically cause deep recursion.Consider adding a recursion counter or a loop-based retry:
- private async acquireSlot(signal?: AbortSignal): Promise<WorkerSlot> { + private async acquireSlot(signal?: AbortSignal, retryCount = 0): Promise<WorkerSlot> { + if (retryCount > 10) { + throw new Error('Failed to acquire slot after multiple retries'); + } + // Try to find an idle slot for (const slot of this.slots.values()) { if (slot.isIdle) { slot.acquire(`exec-${Date.now()}`); return slot; } } // Try to create a new slot if under limit if (this.slots.size < this.config.maxWorkers) { const slot = await this.createAndAddSlot(); slot.acquire(`exec-${Date.now()}`); return slot; } // Queue the request const slotId = await this.executionQueue.enqueue(signal); // Double check slot is still available const slot = this.slots.get(slotId); if (!slot || !slot.isIdle) { // Slot was removed/recycled, try again - return this.acquireSlot(signal); + return this.acquireSlot(signal, retryCount + 1); } slot.acquire(`exec-${Date.now()}`); return slot; }
259-278: Consider notifying queue when memory-exceeded worker is terminated.When a worker is force-terminated due to memory limits, queued requests aren't notified that a slot may become available after replacement. While they'll eventually timeout or get other slots, notifying the queue could improve responsiveness.
Consider calling
this.executionQueue.notifySlotAvailable()after creating a replacement slot:private handleMemoryExceeded(slotId: string, usage: ResourceUsage, limit: number): void { const slot = this.slots.get(slotId); if (!slot) return; // If slot is executing, we need to force terminate if (slot.isExecuting) { this._forcedTerminations++; slot .terminate(false) .then(() => { this.slots.delete(slotId); this.rateLimiter.reset(slotId); + // Create replacement and notify queue if possible + if (this.slots.size < this.config.minWorkers && !this.disposed) { + this.createAndAddSlot().then(newSlot => { + this.executionQueue.notifySlotAvailable(newSlot.id); + }).catch(error => { + console.error('Failed to create replacement after memory kill:', error); + }); + } }) .catch((error) => { console.error('Failed to terminate memory-exceeded worker:', error); this.slots.delete(slotId); this.rateLimiter.reset(slotId); }); } }
314-398: Abort signal not monitored during execution.The abort signal from
context.abortControlleris used when acquiring a slot (line 124), but once execution starts inexecuteInSlot, the signal is not monitored. If the caller aborts during execution, the worker will continue running until completion or timeout.Consider monitoring the abort signal during execution:
private async executeInSlot<T>( slot: WorkerSlot, code: string, context: ExecutionContext, ): Promise<ExecutionResult<T>> { const requestId = `exec-${Date.now()}-${crypto.randomUUID()}`; const startTime = Date.now(); // Serialize config const serializedConfig: SerializedConfig = { timeout: context.config.timeout, maxIterations: context.config.maxIterations, maxToolCalls: context.config.maxToolCalls, maxConsoleOutputBytes: context.config.maxConsoleOutputBytes, maxConsoleCalls: context.config.maxConsoleCalls, sanitizeStackTraces: context.config.sanitizeStackTraces, maxSanitizeDepth: context.config.maxSanitizeDepth, maxSanitizeProperties: context.config.maxSanitizeProperties, globals: context.config.globals, }; return new Promise<ExecutionResult<T>>((resolve, reject) => { + // Monitor abort signal + const abortHandler = () => { + clearTimeout(watchdogId); + slot.off('message', messageHandler); + slot.off('error', errorHandler); + slot.terminate(false).catch(() => {}); + reject(new Error('Execution aborted')); + }; + + if (context.abortController?.signal.aborted) { + abortHandler(); + return; + } + + context.abortController?.signal.addEventListener('abort', abortHandler); + // Set up watchdog timeout (VM timeout + buffer) const watchdogTimeout = context.config.timeout + 5000; const watchdogId = setTimeout(() => { this._timeoutExecutions++; this._forcedTerminations++; + context.abortController?.signal.removeEventListener('abort', abortHandler); slot.terminate(false).catch((error) => { console.error('Watchdog termination failed:', error); }); reject(new WorkerTimeoutError()); }, watchdogTimeout); // ... rest of handlers ... const messageHandler = async (msg: WorkerToMainMessage) => { try { // Rate limiting this.rateLimiter.checkLimit(slot.id); if (isToolCallMessage(msg) && msg.requestId === requestId) { await this.handleToolCall(slot, msg, context, pendingToolCalls); } else if (isExecutionResultMessage(msg) && msg.requestId === requestId) { // Execution complete clearTimeout(watchdogId); + context.abortController?.signal.removeEventListener('abort', abortHandler); slot.off('message', messageHandler); slot.off('error', errorHandler); const duration = Date.now() - startTime; const result = this.buildResult<T>(msg, duration); resolve(result); } else if (isConsoleMessage(msg) && msg.requestId === requestId) { // Forward console output this.handleConsoleMessage(msg); } } catch (error) { clearTimeout(watchdogId); + context.abortController?.signal.removeEventListener('abort', abortHandler); slot.off('message', messageHandler); slot.off('error', errorHandler); reject(error); } }; slot.on('message', messageHandler); // Handle slot error const errorHandler = (error: Error) => { clearTimeout(watchdogId); + context.abortController?.signal.removeEventListener('abort', abortHandler); slot.off('message', messageHandler); slot.off('error', errorHandler); reject(error); }; slot.once('error', errorHandler); // Send execute message slot.sendMessage({ type: 'execute', requestId, code, config: serializedConfig, }); }); }libs/enclave/src/scoring/scorers/external-api.scorer.ts (4)
35-47: Normalize and harden config values at construction time.Consider clamping/sanitizing
timeoutMsandretries, and copyingheadersto avoid external mutation:constructor(config: ExternalApiConfig) { super(); this.config = { endpoint: config.endpoint, apiKey: config.apiKey ?? '', - timeoutMs: config.timeoutMs ?? 5000, - headers: config.headers ?? {}, - retries: config.retries ?? 1, + timeoutMs: config.timeoutMs && config.timeoutMs > 0 ? config.timeoutMs : 5000, + headers: { ...(config.headers ?? {}) }, + retries: Math.max(0, config.retries ?? 1), }; this.ready = true; }This prevents odd cases like negative retries or zero/negative timeouts and makes the stored headers immutable from the outside.
49-83: Differentiate retriable vs non‑retriable failures in the retry loop.Right now all errors from
callApiare treated as retriable, including 4xx HTTP errors and schema/validation errors. Repeating those calls adds latency with no benefit and may worsen rate limiting.Consider marking certain failures as non‑retriable (e.g., via a custom error type or flag from
callApi) and short‑circuiting the loop when encountered so you fail fast on configuration or contract problems but still retry on timeouts/transient 5xx/network issues.
111-149: Response validation looks solid; consider hoistingvalidRiskLevelsto a shared constant.The added checks for
score,risk,signals, and each signal’s fields/level close the gaps raised in earlier reviews and make downstream usage inscore()safe.You might want to define
validRiskLevelsonce at module scope and reuse it here and in the scorer mapping to avoid re‑allocating the array on every call and keep risk‑level handling centralized:+const VALID_RISK_LEVELS = ['none', 'low', 'medium', 'high', 'critical'] as const; + // ... - // Validate risk level if present - const validRiskLevels = ['none', 'low', 'medium', 'high', 'critical']; - if (data.risk !== undefined && !validRiskLevels.includes(data.risk)) { + // Validate risk level if present + if (data.risk !== undefined && !VALID_RISK_LEVELS.includes(data.risk)) { throw new Error(`Invalid API response: invalid risk level "${data.risk}"`); } // ... - if (!validRiskLevels.includes(signal.level)) { + if (!VALID_RISK_LEVELS.includes(signal.level)) { throw new Error(`Invalid API response: invalid signal level "${signal.level}"`); }Optionally annotate
VALID_RISK_LEVELSwithRiskLevel[]or aSet<RiskLevel>if you want stronger typing around future changes to the union.
163-170: Leverage nativeErrorcause support (if your runtime/TS target allows).Instead of manually storing
cause, you can use the standardErrorOptions.causefield, which many tools understand:-export class ExternalApiScorerError extends Error { - readonly cause?: Error; - - constructor(message: string, cause?: Error) { - super(message); - this.name = 'ExternalApiScorerError'; - this.cause = cause; - } -} +export class ExternalApiScorerError extends Error { + constructor(message: string, cause?: Error) { + super(message, { cause }); + this.name = 'ExternalApiScorerError'; + } +}This keeps the type simple and integrates better with standard error‑inspection tooling, assuming your TS lib and runtime versions support
ErrorOptions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)libs/enclave/src/scoring/scorers/external-api.scorer.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/scoring/scorers/external-api.scorer.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/enclave/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave/src/scoring/scorers/external-api.scorer.ts
🧬 Code graph analysis (1)
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (10)
libs/enclave/src/adapters/worker-pool/index.ts (17)
WorkerPoolAdapter(15-15)WorkerPoolConfig(19-19)WorkerSlot(59-59)ExecutionQueue(60-60)MemoryMonitor(61-61)RateLimiter(62-62)buildWorkerPoolConfig(25-25)createRateLimiter(62-62)WorkerPoolDisposedError(34-34)WorkerTimeoutError(31-31)SerializedConfig(48-48)WorkerToMainMessage(51-51)ToolCallMessage(53-53)TooManyPendingCallsError(42-42)SerializedError(47-47)sanitizeObject(63-63)ExecutionResultMessage(54-54)libs/enclave/src/types.ts (5)
SandboxAdapter(522-536)WorkerPoolConfig(755-755)SecurityLevel(24-24)ExecutionContext(475-516)ExecutionResult(238-263)libs/enclave/src/adapters/worker-pool/config.ts (2)
WorkerPoolConfig(15-96)buildWorkerPoolConfig(269-279)libs/enclave/src/adapters/worker-pool/worker-slot.ts (1)
WorkerSlot(47-371)libs/enclave/src/adapters/worker-pool/execution-queue.ts (1)
ExecutionQueue(58-240)libs/enclave/src/adapters/worker-pool/memory-monitor.ts (1)
MemoryMonitor(58-251)libs/enclave/src/adapters/worker-pool/rate-limiter.ts (2)
RateLimiter(36-119)createRateLimiter(124-129)libs/enclave/src/adapters/worker-pool/errors.ts (3)
WorkerPoolDisposedError(71-76)WorkerTimeoutError(26-31)TooManyPendingCallsError(177-189)libs/enclave/src/adapters/worker-pool/protocol.ts (8)
SerializedConfig(48-67)WorkerToMainMessage(204-209)isToolCallMessage(318-320)isExecutionResultMessage(325-327)isConsoleMessage(339-341)ToolCallMessage(150-160)SerializedError(17-26)ExecutionResultMessage(165-177)libs/enclave/src/adapters/worker-pool/safe-deserialize.ts (1)
sanitizeObject(127-154)
⏰ 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
🔇 Additional comments (6)
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (5)
1-55: LGTM! Well-structured class declaration.The imports, type declarations, and private fields are properly organized and typed.
64-106: LGTM! Proper initialization flow.The constructor correctly initializes all components, and the
initializemethod has appropriate guards and async handling for warming the pool.
111-169: LGTM! Robust lifecycle management.The
executemethod properly manages resources with a finally block, anddisposesafely cleans up all workers usingPromise.allSettledto handle termination failures gracefully.
400-450: LGTM! Secure tool call handling with proper sanitization.Tool call arguments and results are sanitized before processing (lines 422, 439), and the pending call limit prevents resource exhaustion. Error handling is appropriate with cleanup in the finally block.
486-547: LGTM! Comprehensive metrics collection.The metrics gathering is thorough, collecting data from all subsystems. The limitation of
avgExecutionTimeMsbeing 0 is acknowledged in the comment (line 521) and would require additional tracking to implement.libs/enclave/src/scoring/scorers/external-api.scorer.ts (1)
85-104: Confirm runtime support forfetch,AbortController, andperformance.This module assumes these browser‑style globals are available. In Node.js they’re only present in newer runtimes (or via polyfills/whatwg‑fetch/perf_hooks shims).
Please confirm your target environment (or bundler) guarantees these globals; otherwise, consider importing from the appropriate Node modules or centralizing a polyfilled HTTP/clock abstraction so scorers don’t depend directly on ambient globals.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/draft/docs/plugins/codecall/security.mdx (1)
78-78: Minor: Simplify "Very large" to "Large" for more measured language.Line 78 uses "Very large inputs" which static analysis flags as an over-used intensifier. For more direct technical language, simply use:
-4. **Memory Exhaustion**: Very large inputs can exhaust memory before validation +4. **Memory Exhaustion**: Large inputs can exhaust memory before validationThis maintains clarity while using more measured technical prose (consistent with the security documentation tone elsewhere).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/draft/docs/plugins/codecall/api-reference.mdx(1 hunks)docs/draft/docs/plugins/codecall/configuration.mdx(1 hunks)docs/draft/docs/plugins/codecall/scaling.mdx(1 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/draft/docs/plugins/codecall/api-reference.mdx
🧰 Additional context used
📓 Path-based instructions (4)
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/codecall/scaling.mdxdocs/draft/docs/plugins/codecall/configuration.mdxdocs/draft/docs/plugins/codecall/security.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/codecall/scaling.mdxdocs/draft/docs/plugins/codecall/configuration.mdxdocs/draft/docs/plugins/codecall/security.mdx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.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/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
🧠 Learnings (4)
📚 Learning: 2025-11-16T22:47:05.139Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 50
File: docs/docs/v/0.1/getting-started/welcome.mdx:2-3
Timestamp: 2025-11-16T22:47:05.139Z
Learning: In the agentfront/frontmcp repository, the docs/docs/v/** directory governance rule prohibiting manual edits applies to already-frozen releases, but does not preclude initial setup and bootstrap commits that establish the versioned docs structure.
Applied to files:
docs/draft/docs/plugins/codecall/configuration.mdx
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Build must complete without TypeScript warnings using strict mode
Applied to files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Applied to files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdx
🧬 Code graph analysis (1)
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (8)
libs/enclave/src/adapters/worker-pool/index.ts (13)
WorkerPoolAdapter(15-15)WorkerPoolConfig(19-19)WorkerSlot(59-59)ExecutionQueue(60-60)MemoryMonitor(61-61)buildWorkerPoolConfig(25-25)WorkerTimeoutError(31-31)QueueFullError(35-35)SerializedConfig(48-48)WorkerToMainMessage(51-51)ToolCallMessage(53-53)TooManyPendingCallsError(42-42)ExecutionResultMessage(54-54)libs/enclave/src/types.ts (4)
SandboxAdapter(522-536)WorkerPoolConfig(755-755)ExecutionContext(475-516)ExecutionResult(238-263)libs/enclave/src/adapters/worker-pool/config.ts (2)
WorkerPoolConfig(15-96)buildWorkerPoolConfig(269-279)libs/enclave/src/adapters/worker-pool/worker-slot.ts (1)
WorkerSlot(47-371)libs/enclave/src/adapters/worker-pool/execution-queue.ts (1)
ExecutionQueue(58-240)libs/enclave/src/adapters/worker-pool/memory-monitor.ts (1)
MemoryMonitor(58-251)libs/enclave/src/adapters/worker-pool/errors.ts (3)
WorkerTimeoutError(26-31)QueueFullError(81-93)TooManyPendingCallsError(177-189)libs/enclave/src/adapters/worker-pool/protocol.ts (7)
SerializedConfig(48-67)WorkerToMainMessage(204-209)isToolCallMessage(318-320)isExecutionResultMessage(325-327)isConsoleMessage(339-341)ToolCallMessage(150-160)ExecutionResultMessage(165-177)
🪛 LanguageTool
docs/draft/docs/plugins/codecall/security.mdx
[style] ~78-~78: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ng the parser 3. Memory Exhaustion: Very large inputs can exhaust memory before valida...
(EN_WEAK_ADJECTIVE)
⏰ 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
…with new sections and examples
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/draft/docs.json (1)
178-184: Blog entry for CodeCall plugin does not exist.The file
docs/draft/docs/blog/11-2025/codecall-plugin.mdxis missing. The navigation indocs/draft/docs.jsonreferences this blog entry, but the corresponding MDX file has not been created. Create the blog post file to resolve this broken reference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/draft/docs.json(2 hunks)docs/draft/docs/guides/ast-guard.mdx(4 hunks)docs/draft/docs/guides/enclave.mdx(6 hunks)docs/draft/docs/guides/vectoriadb.mdx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/draft/docs/guides/vectoriadb.mdx
🧰 Additional context used
📓 Path-based instructions (2)
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/guides/ast-guard.mdxdocs/draft/docs/guides/enclave.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/guides/ast-guard.mdxdocs/draft/docs/guides/enclave.mdxdocs/draft/docs.json
🧠 Learnings (2)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/guides/ast-guard.mdx
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
docs/draft/docs/guides/ast-guard.mdxdocs/draft/docs/guides/enclave.mdx
⏰ 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
🔇 Additional comments (9)
docs/draft/docs.json (2)
150-159: All three new guide pages exist at their expected locations.The verification confirms that the documentation files referenced in
docs/draft/docs.json(lines 150-159) are present:
- docs/draft/docs/guides/ast-guard.mdx ✓
- docs/draft/docs/guides/enclave.mdx ✓
- docs/draft/docs/guides/vectoriadb.mdx ✓
119-135: All referenced CodeCall documentation pages exist and migration is complete.Verification confirms:
- ✓ All 5 CodeCall pages exist (
overview.mdx,security.mdx,api-reference.mdx,configuration.mdx,scaling.mdx)- ✓ Old
codecall-plugin.mdxsuccessfully removed- ✓ No inbound references to the old
codecall-pluginpath remainThe CodeCall plugin restructuring from flat to nested group has been executed without breaking changes.
docs/draft/docs/guides/ast-guard.mdx (5)
521-554: Fine-grained rule composition example demonstrates practical validator configuration.The custom validator example at lines 535-553 shows a realistic composition of five rules (DisallowedIdentifier, ForbiddenLoop, RequiredFunctionCall, CallArgumentValidation, UnknownGlobal) with sensible defaults. This is helpful for users building custom security policies.
577-579: Known limitation is clearly documented.The note about
UnknownGlobalRuleusing a flat symbol table (without lexical scoping) is good transparency. This is helpful context for developers who may enable user functions in future versions.
464-520: All three rules are properly exported and available in the ast-guard API. Verification confirms StaticCallTargetRule, NoRegexLiteralRule, and NoRegexMethodsRule are implemented in their respective files, exported through the rules barrel file, and included in the main package index exports. The documentation accurately reflects the actual implementation.
12-13: Documentation is accurate — 16 built-in rules verified.The ast-guard library exports exactly 16 ValidationRule implementations, confirming the documentation claim is correct.
75-146: All Pre-Scanner documentation claims verified and accurate.Verification confirms:
✓ Rule count (16): All 16 built-in rules verified to exist
✓ Mandatory limits: All values match implementation (100MB input, 200 nesting, 100K line length, 1M lines, 5MB strings, 1000 char regex)
✓ ReDoS pattern detection: All documented patterns with correct scores verified (nested_quantifier 90, star_in_repetition 85, repetition_in_star 85, overlapping_alternation 80, greedy_backtracking 75, multiple_greedy 70)
✓ Preset hierarchy: All five preset levels confirmed (agentscript > strict > secure > standard > permissive)Minor observation: Implementation includes an additional
overlapping_classesReDoS pattern (score 50) not documented in the guide. This appears intentional (focusing docs on high-risk patterns) but could be noted for completeness if desired.docs/draft/docs/guides/enclave.mdx (2)
393-453: Worker Pool Adapter documentation is accurate and fully implemented.Verification confirms all documented presets and configuration defaults are correctly implemented in
libs/enclave/src/adapters/worker-pool/config.ts:
- Presets match exactly: STRICT (4 maxWorkers, 64MB), SECURE (8, 128MB), STANDARD (16, 256MB), PERMISSIVE (32, 512MB)
- Configuration defaults match: minWorkers=2, maxWorkers=os.cpus().length, memoryLimitPerWorker=128MB, maxExecutionsPerWorker=1000, maxQueueSize=100, maxMessagesPerSecond=1000
- Implementation structure: WorkerPoolConfig interface, DEFAULT_WORKER_POOL_CONFIG, WORKER_POOL_PRESETS record, and buildWorkerPoolConfig function all present and correct
89-126: Security Level Presets section is accurate and all documented values are properly implemented.Verification confirms that every preset value in the Security Level Comparison table (lines 111-121) matches the implementation in
SECURITY_LEVEL_CONFIGSatlibs/enclave/src/types.ts:
- Timeouts: 5s → 15s → 30s → 60s ✓
- maxIterations: 1,000 → 5,000 → 10,000 → 100,000 ✓
- maxToolCalls: 10 → 50 → 100 → 1,000 ✓
- maxConsoleCalls: 100 → 500 → 1,000 → 10,000 ✓
- maxConsoleOutputBytes: 64KB → 256KB → 1MB → 10MB ✓
- Security boolean flags (sanitizeStackTraces, blockTimingAPIs, allowUnboundedLoops, unicodeSecurityCheck) all match the documented YES/NO values ✓
Additional confirmed features:
scoringGatewith both 'rule-based' and 'external-api' scorers implementedworkerPoolConfigpresets follow the same hierarchical pattern (STRICT > SECURE > STANDARD > PERMISSIVE)- Configuration override mechanism (
getConfigFromSecurityLevel) properly merges preset values with explicit options
… detail; enhance worker pool error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/draft/docs/guides/enclave.mdx (1)
181-181: Enhancement: Line mentions console rate limiting but warrants clarification.Line 181 mentions "captured for logging with rate limiting" when describing Safe console transformation, which aligns with the new
maxConsoleCallslimit. However, users might benefit from a brief note that the default rate limits vary by security level. Consider cross-referencing the nearbymaxConsoleCallsParamField (line 241) to make this more discoverable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/draft/docs/guides/enclave.mdx(6 hunks)docs/draft/docs/plugins/codecall/security.mdx(1 hunks)libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/codecall/security.mdxdocs/draft/docs/guides/enclave.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/codecall/security.mdxdocs/draft/docs/guides/enclave.mdx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript - avoidanytypes without strong justification, preferunknownfor generic type defaults
Useunknowninstead ofanyfor generic type parameters with no constraint or default fallback
Use type parameters with constraints instead of unconstrained generic types
Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Use clean, descriptive names for all exports, types, classes, functions, and properties
Files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.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/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation libraries
Applied to files:
docs/draft/docs/plugins/codecall/security.mdxdocs/draft/docs/guides/enclave.mdx
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Build must complete without TypeScript warnings using strict mode
Applied to files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to **/*.{ts,tsx} : Avoid non-null assertions (!) - throw specific errors with proper error handling instead
Applied to files:
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
📚 Learning: 2025-11-26T17:22:58.085Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T17:22:58.085Z
Learning: Applies to libs/ast-guard/src/**/*.ts : Follow the preset pattern for hierarchical configurations (STRICT > SECURE > STANDARD > PERMISSIVE)
Applied to files:
docs/draft/docs/guides/enclave.mdx
🪛 ESLint
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts
[error] 104-104: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
[error] 364-364: 'messageHandler' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 365-365: 'errorHandler' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 366-366: 'watchdogId' is never reassigned. Use 'const' instead.
(prefer-const)
⏰ 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
🔇 Additional comments (13)
libs/enclave/src/adapters/worker-pool/worker-pool-adapter.ts (6)
1-83: LGTM!The imports, class declaration, and constructor are well-structured. All components are properly initialized with appropriate configurations, and the memory monitor event handler is correctly bound.
85-178: LGTM!The lifecycle methods properly handle initialization, execution, and disposal. The error handling in
initialize()correctly cleans up partially spawned workers, addressing the previous review concern. Thedispose()method gracefully shuts down all workers usingPromise.allSettled.
180-332: LGTM!Pool management methods are robust and handle all edge cases properly. Previous review concerns have been addressed:
handleMemoryExceedednow properly cleans up slots and notifies the queue in both success and error pathsreleaseSlotchecks slot status before operations to avoid operating on terminated slotsThe retry logic in
acquireSlotwithMAX_ACQUIRE_RETRIESprevents infinite recursion scenarios.
334-428: LGTM!The execution flow properly manages event handlers and cleanup. Previous review concerns have been addressed:
- The success path (line 385) now calls
cleanup()to remove all handlers- The watchdog timeout (line 410) calls
cleanup()before terminating the worker- All error paths properly clean up resources
The centralized
cleanupfunction ensures consistent handler removal across all code paths.
430-522: LGTM!Tool call handling properly sanitizes inputs and outputs. The
buildResultmethod includes clear documentation explaining the type assertion limitation at the serialization boundary, which addresses the previous review concern. The console message handler safely checks for the existence of the console method before calling it.
524-585: LGTM!The metrics and getter methods provide comprehensive observability into the worker pool state. The implementation is clean and straightforward.
docs/draft/docs/guides/enclave.mdx (6)
362-376: Cross-file consistency: Verify detection rule scores match codecall/security.mdx.The Detection Rules table here shows specific scores. Please verify these match the equivalent table in
docs/draft/docs/plugins/codecall/security.mdx(lines 339-348), as there appear to be discrepancies in the reported scores for several rules (EXFIL_PATTERN, LOOP_TOOL_CALL, DYNAMIC_TOOL, BULK_OPERATION). See the parallel review comment in codecall/security.mdx for details.
317-392: Well-documented addition: AI Scoring Gate section is comprehensive.The new AI Scoring Gate section clearly explains:
- Purpose of semantic analysis beyond AST validation ✓
- Specific detection rules with examples ✓
- Caching mechanism and latency implications ✓
- Configuration examples for rule-based and external-api scorers ✓
The section integrates well with the CodeCall security model and provides actionable guidance for configuration.
89-127: Well-structured: Security Level Presets section clarifies configuration options.The new Security Level Presets section provides:
- Clear introduction to preset concept ✓
- Practical examples showing
securityLevelparameter usage ✓- Security Level Comparison table with detailed defaults ✓
- Warning callout emphasizing STRICT for untrusted code ✓
However, verify that the timeout and resource limit values in the Security Level Comparison table (lines 111-121) align with actual implementation values. See parallel comment on this file for details.
236-244: Improvement: Per-level console output defaults now documented clearly.The new
maxConsoleOutputBytesandmaxConsoleCallsparameter fields now explicitly list security-level-specific defaults:
- Lines 236-238: maxConsoleOutputBytes defaults per level ✓
- Lines 241-243: maxConsoleCalls defaults per level ✓
This directly addresses the previous documentation gap (past review noted defaults were unclear). Users can now understand what limits apply to their chosen security level.
396-456: Well-documented addition: Worker Pool Adapter section is comprehensive.The new Worker Pool Adapter section covers:
- Configuration example with minWorkers/maxWorkers/memoryLimit ✓
- Feature overview (pool management, memory monitoring, hard halt, rate limiting) ✓
- Worker Pool Presets table with security level variants ✓
- Detailed ParamField documentation for all configuration options ✓
Clear explanation of dual-layer sandbox and when to use Worker Pool. Integration with security levels is well-explained.
111-121: Critical issue verified and confirmed—codecall/security.mdx misrepresents actual Enclave Security Level values.The table labeled "Enclave Security Level Defaults" (lines 613–621 of
docs/draft/docs/plugins/codecall/security.mdx) shows CodeCall VM Preset timeouts, not Enclave's actual values:
Level codecall/security.mdx shows Actual Enclave implementation STRICT 2,000ms 5,000ms SECURE 3,500ms 15,000ms STANDARD 5,000ms 30,000ms PERMISSIVE 10,000ms 60,000ms The mapping commentary correctly identifies that CodeCall VM Presets (
locked_down,secure,balanced,experimental) map to Enclave Security Levels (STRICT,SECURE,STANDARD,PERMISSIVE). However, the subsequent table displays CodeCall preset values while claiming they are "Enclave Security Level Defaults"—which is factually incorrect.The
docs/draft/docs/guides/enclave.mdxtable (lines 111–121) is accurate and matches the authoritative implementation inlibs/enclave/src/types.ts.Action: Clarify or replace the codecall/security.mdx table to either (1) accurately document Enclave's actual security level defaults or (2) clearly label it as CodeCall VM Preset configuration with a note that values differ from mapped Enclave levels. Ensure developers are not misled about actual timeout behavior.
⛔ Skipped due to learnings
Learnt from: CR Repo: agentfront/frontmcp PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T17:22:58.085Z Learning: Applies to **/*.md : Document known limitations clearly and write comprehensive security documentation for validation librariesdocs/draft/docs/plugins/codecall/security.mdx (1)
613-621: I'll help you verify this critical documentation inconsistency. Let me start by examining both files mentioned to confirm the claimed conflicts.
<function_calls>
#!/bin/bashFirst, verify the existence and read the security.mdx file
echo "=== Reading security.mdx (lines 610-625) ==="
sed -n '610,625p' docs/draft/docs/plugins/codecall/security.mdxecho -e "\n=== Reading enclave.mdx (lines 108-125) ==="
sed -n '108,125p' docs/draft/docs/guides/enclave.mdx
</function_calls>
Summary by CodeRabbit
New Features
Security Enhancements
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.