**Actionable comments posted: 8**

🧹 Nitpick comments (4)

packages/core/src/rate-limit/manager.ts (2)
`131-140`: **The `default` case returning `"unknown"` may mask configuration errors.** If an unexpected `scopeId.type` is passed, the function silently returns `"unknown"`, which could cause multiple unrelated scopes to share the same limiter. Consider logging a warning or throwing an error for unexpected types.

Suggested improvement
```diff private getScopeKey(scopeId: RateLimitScopeId): string { switch (scopeId.type) { case "global": return "global:llm"; case "tool": return `tool:${scopeId.agentId}:${scopeId.toolName}`; default: + this.logger?.warn(`Unknown rate limit scope type: ${(scopeId as RateLimitScopeId).type}`); return "unknown"; } } ```

🤖 Prompt for AI Agents
``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/rate-limit/manager.ts` around lines 131 - 140, The getScopeKey function currently returns "unknown" for unexpected RateLimitScopeId.type values which can mask config errors; update getScopeKey to explicitly handle invalid types by throwing an error (or at minimum logging a warning) that includes the unexpected scopeId.type and the full scopeId object so callers can identify the source, and keep the existing cases for "global" and "tool" intact; reference the getScopeKey method and the RateLimitScopeId type when making this change. ```
--- `31-42`: **Consider documenting the implicit rate limiting when `llm` config is present but incomplete.** When `config.llm` exists (even without `maxRequestsPerMinute`), a limiter is created with a default of 60 requests/min. This could be surprising - an empty `llm: {}` config silently enables rate limiting. Consider either: 1. Requiring `maxRequestsPerMinute` to be set for the limiter to be created 2. Documenting this behavior clearly

Option 1: Only create limiter when maxRequestsPerMinute is explicitly set
```diff async checkLLMRateLimit(): Promise { // Check global LLM limit (if configured) - if (this.config.llm) { + if (this.config.llm?.maxRequestsPerMinute) { const scopeId: RateLimitScopeId = { type: "global", }; const limiter = this.getLimiter(scopeId, this.config.llm); await limiter.acquire(); } // No rate limit configured - allow through } ```

🤖 Prompt for AI Agents
``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/rate-limit/manager.ts` around lines 31 - 42, The code in checkLLMRateLimit currently creates a limiter whenever config.llm is truthy, which silently enables a default 60rpm limiter; update checkLLMRateLimit to only create/await a limiter when config.llm.maxRequestsPerMinute is explicitly set (or non-null) to avoid implicit enforcement, i.e. check config.llm && typeof config.llm.maxRequestsPerMinute === "number" before constructing RateLimitScopeId and calling this.getLimiter/acquire; reference the checkLLMRateLimit method, config.llm, maxRequestsPerMinute, RateLimitScopeId and getLimiter to locate and change the logic accordingly. ```

packages/core/src/rate-limit/types.ts (1)
`43-64`: **Asymmetric optionality between LLM and tool rate limit configs.** `LLMRateLimitConfig.maxRequestsPerMinute` is optional (line 45), but `ToolRateLimitConfig.maxRequestsPerMinute` is required (line 59). This asymmetry may be intentional, but it means: - LLM rate limiting can be configured without an explicit limit (defaulting to 60/min in the manager) - Tool rate limiting requires an explicit limit If this is intentional, consider adding a JSDoc comment explaining why. If not, consider making them consistent.

🤖 Prompt for AI Agents
``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/rate-limit/types.ts` around lines 43 - 64, The two interfaces LLMRateLimitConfig and ToolRateLimitConfig have inconsistent optionality for maxRequestsPerMinute (LLMRateLimitConfig.maxRequestsPerMinute is optional, ToolRateLimitConfig.maxRequestsPerMinute is required); decide which behavior is intended and make them consistent: either change ToolRateLimitConfig.maxRequestsPerMinute to optional to match LLMRateLimitConfig, or make LLMRateLimitConfig.maxRequestsPerMinute required to match ToolRateLimitConfig, and update any callers/tests accordingly; if the asymmetry is intentional instead, add a clarifying JSDoc above the affected symbols (LLMRateLimitConfig and/or ToolRateLimitConfig) explaining why one requires an explicit limit while the other can rely on a manager default. ```

packages/core/src/rate-limit/manager.spec.ts (1)
`163-166`: **Misleading variable name.** The variable `openaiStats` suggests provider-specific (OpenAI) stats, but per the PR summary, provider-specific configurations were removed. This is global LLM stats.

Suggested fix
```diff const stats = manager.getAllStats(); expect(Object.keys(stats).length).toBe(1); // Only one limiter should exist - const openaiStats = Object.values(stats)[0]; - expect(openaiStats.current).toBe(2); // Both requests counted + const llmStats = Object.values(stats)[0]; + expect(llmStats.current).toBe(2); // Both requests counted }); ```

🤖 Prompt for AI Agents
``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/rate-limit/manager.spec.ts` around lines 163 - 166, The test uses a misleading variable name openaiStats in packages/core/src/rate-limit/manager.spec.ts; rename that variable to something reflecting global LLM stats (e.g., llmStats or globalStats) and update the inline comment that asserts its value (expect(...).toBe(2)) to reference "LLM/global stats" instead of "Both requests counted" being tied to OpenAI; locate the variable by the identifier openaiStats and the surrounding stats usage and change all occurrences to the new name to keep the test clear and consistent with the provider-agnostic design. ```

🤖 Prompt for all review comments with AI agents

``` Verify each finding against the current code and only fix it if needed. Inline comments: In `@examples/with-rate-limiting/.gitignore`: - Around line 1-4: The example's .gitignore is missing patterns to exclude local environment files, so add entries for ".env" and common variants like ".env.local" (and optionally ".env.*" if you want to exclude all env files) to the package's .gitignore to prevent accidental commits of real API keys; update the existing .gitignore file to include these patterns alongside the current entries (node_modules, dist, .DS_Store, .voltagent). In `@examples/with-rate-limiting/README.md`: - Around line 22-26: The README incorrectly tells users to "Set your OpenAI API key" while showing the environment variable GOOGLE_GENERATIVE_AI_API_KEY; update the setup instructions so the provider name and env var match: either change the descriptive text to "Set your Google Generative AI API key" to match GOOGLE_GENERATIVE_AI_API_KEY, or change the shown variable to the correct OpenAI variable (e.g., OPENAI_API_KEY) to match the OpenAI wording; ensure the text and the environment variable (GOOGLE_GENERATIVE_AI_API_KEY or OPENAI_API_KEY) are consistent. In `@examples/with-rate-limiting/src/index.ts`: - Around line 262-265: The console message after examples finishes is misleading; update the console.log call in the try block (the message that currently reads "Note: Uncomment examples in main() to run them.") to reflect reality (e.g., "Examples executed" or remove the note) so it no longer instructs to uncomment examples in main(); locate the final console.log in main() / the try block and replace the outdated string with an accurate status message. - Around line 65-67: The log incorrectly labels RateLimitExceededError.retryAfter as seconds while retryAfter is in milliseconds; update the logging in examples/with-rate-limiting/src/index.ts to convert error.retryAfter from milliseconds to seconds (e.g., Math.ceil(error.retryAfter / 1000)) or change the label to "milliseconds" so it matches the value returned by RateLimitExceededError.retryAfter in errors.ts; use the symbol RateLimitExceededError.retryAfter when locating the source of the mismatch. In `@packages/core/src/agent/agent.ts`: - Around line 542-555: The provider extracted by extractProviderFromModel(model) is only used for logging and never passed to RateLimitManager, so provider-specific limits (e.g., rateLimits.providers.openai) are ignored; update all places where rateLimitManager.checkLLMRateLimit() is called (including the blocks around extractProviderFromModel at the top-level LLM call, the other similar block later, and the long-running generation block) to pass the provider (or provider id) into checkLLMRateLimit(provider) and adjust the RateLimitManager.checkLLMRateLimit signature/implementation accordingly to accept and use the provider to select per-provider configs, keeping the existing debug log calls (methodLogger.debug) intact but now relying on the provider argument to enforce provider-specific limits. - Around line 542-549: generateObject() and streamObject() are bypassing the LLM quota because they don't call the rate limiter; add the same rate-limit check used in generateText()/streamText() to both generateObject and streamObject (and their streaming counterparts referenced around the other block), i.e., extract provider via extractProviderFromModel(model), set modelId = modelName, then await this.rateLimitManager.checkLLMRateLimit(provider, modelId) (or the same call signature used in generateText/streamText) before invoking the provider directly; update both functions to early-await the rateLimitManager when this.rateLimitManager exists so object-generation cannot skip the llm quota. In `@packages/core/src/rate-limit/limiters/fixed-window.ts`: - Around line 66-79: The delay branch in the acquire logic of fixed-window limiter (the method that checks this.count, this.windowStart and calls this.delay) can race because multiple waiters reset this.count and this.windowStart concurrently; modify the acquire() implementation in fixed-window.ts to serialize delayed waiters by introducing a per-window pending promise or FIFO queue (e.g., a shared Promise/resolve pair or waiter queue on the limiter instance) so only one waiter resets the window and allows the next waiter to proceed and update count; ensure the code that currently does "await this.delay(waitTime); this.count = 0; this.windowStart = Date.now();" is replaced with enqueuing/waiting on the shared waiter mechanism and that increments to this.count happen under that serialized path to enforce the limit. In `@packages/core/src/rate-limit/manager.spec.ts`: - Around line 132-143: The test incorrectly expects a rejection after 5 requests even though the test setup (beforeEach) sets maxRequestsPerMinute to 10; update the test that uses manager.checkLLMRateLimit() so it consumes all 10 allowed requests (for loop i < 10) and then assert that the 11th call throws RateLimitExceededError, keeping the manager.resetAll() call and final assertion that a post-reset checkLLMRateLimit() resolves; reference the methods manager.checkLLMRateLimit, manager.resetAll and the RateLimitExceededError when making the change. --- Nitpick comments: In `@packages/core/src/rate-limit/manager.spec.ts`: - Around line 163-166: The test uses a misleading variable name openaiStats in packages/core/src/rate-limit/manager.spec.ts; rename that variable to something reflecting global LLM stats (e.g., llmStats or globalStats) and update the inline comment that asserts its value (expect(...).toBe(2)) to reference "LLM/global stats" instead of "Both requests counted" being tied to OpenAI; locate the variable by the identifier openaiStats and the surrounding stats usage and change all occurrences to the new name to keep the test clear and consistent with the provider-agnostic design. In `@packages/core/src/rate-limit/manager.ts`: - Around line 131-140: The getScopeKey function currently returns "unknown" for unexpected RateLimitScopeId.type values which can mask config errors; update getScopeKey to explicitly handle invalid types by throwing an error (or at minimum logging a warning) that includes the unexpected scopeId.type and the full scopeId object so callers can identify the source, and keep the existing cases for "global" and "tool" intact; reference the getScopeKey method and the RateLimitScopeId type when making this change. - Around line 31-42: The code in checkLLMRateLimit currently creates a limiter whenever config.llm is truthy, which silently enables a default 60rpm limiter; update checkLLMRateLimit to only create/await a limiter when config.llm.maxRequestsPerMinute is explicitly set (or non-null) to avoid implicit enforcement, i.e. check config.llm && typeof config.llm.maxRequestsPerMinute === "number" before constructing RateLimitScopeId and calling this.getLimiter/acquire; reference the checkLLMRateLimit method, config.llm, maxRequestsPerMinute, RateLimitScopeId and getLimiter to locate and change the logic accordingly. In `@packages/core/src/rate-limit/types.ts`: - Around line 43-64: The two interfaces LLMRateLimitConfig and ToolRateLimitConfig have inconsistent optionality for maxRequestsPerMinute (LLMRateLimitConfig.maxRequestsPerMinute is optional, ToolRateLimitConfig.maxRequestsPerMinute is required); decide which behavior is intended and make them consistent: either change ToolRateLimitConfig.maxRequestsPerMinute to optional to match LLMRateLimitConfig, or make LLMRateLimitConfig.maxRequestsPerMinute required to match ToolRateLimitConfig, and update any callers/tests accordingly; if the asymmetry is intentional instead, add a clarifying JSDoc above the affected symbols (LLMRateLimitConfig and/or ToolRateLimitConfig) explaining why one requires an explicit limit while the other can rely on a manager default. ```

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR: - [ ] Push a commit to this branch (recommended) - [ ] Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

**Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `0894e402-5803-40df-9f49-99ab3ee6f2df`

📥 Commits

Reviewing files that changed from the base of the PR and between bac1f4992e3841b940c5d5bce4474c63257dbe63 and 59575ae297347c01a6c3d012b476135c084dbe49.

📒 Files selected for processing (16)

* `examples/with-rate-limiting/.env.example` * `examples/with-rate-limiting/.gitignore` * `examples/with-rate-limiting/README.md` * `examples/with-rate-limiting/package.json` * `examples/with-rate-limiting/src/index.ts` * `packages/core/src/agent/agent.ts` * `packages/core/src/agent/types.ts` * `packages/core/src/index.ts` * `packages/core/src/rate-limit/errors.ts` * `packages/core/src/rate-limit/index.ts` * `packages/core/src/rate-limit/limiters/fixed-window.spec.ts` * `packages/core/src/rate-limit/limiters/fixed-window.ts` * `packages/core/src/rate-limit/limiters/index.ts` * `packages/core/src/rate-limit/manager.spec.ts` * `packages/core/src/rate-limit/manager.ts` * `packages/core/src/rate-limit/types.ts`