fix(nodes): add rate limiting to tool_http_request node#560
Conversation
charliegillet
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a token-bucket rate limiter with concurrency control to the HTTP Request node, initializes it from global configuration, enforces limits around outbound requests, exposes three new config knobs, and includes unit tests for limiter behavior and concurrency. Changes
Sequence Diagram(s)sequenceDiagram
participant Instance as IInstance
participant Global as IGlobal
participant Limiter as RateLimiter
participant Client as HTTP Client
Instance->>Global: read rate_limiter
alt rate_limiter exists
Instance->>Limiter: acquire()
note right of Limiter: semaphore + token-buckets\nper-second & per-minute checks
Limiter-->>Instance: success / RateLimitError
alt success
Instance->>Client: execute_request(...)
Client-->>Instance: response / error
Instance->>Limiter: release()
else RateLimitError
Instance-->>Client: propagate error (no request sent)
end
else no rate_limiter
Instance->>Client: execute_request(...) (no rate limiting)
Client-->>Instance: response / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Review: PR #560 — Rate limiting for tool_http_request
The feature is well-motivated and the overall structure (token bucket + semaphore, config-driven via services.json) is solid. However there is a logic bug in the core algorithm that must be fixed before merge, plus a mandatory project rule violation.
No unit tests: the refill math and three-way acquire() logic are non-trivial. The bug above is exactly what a unit test would catch. Please add tests for: normal acquire/release, per-second enforcement, per-minute enforcement, semaphore exhaustion, and token restoration on semaphore rejection.
…miter - Fix critical bug: check semaphore BEFORE consuming tokens so rejected requests don't wastefully consume rate-limit tokens. Release semaphore if token check fails afterward. - Extract _config_int() utility with min_value/max_value clamping params from inline _int_or_default helper in IGlobal._build_rate_limiter. - Allow _build_rate_limiter to return None when all three rate-limit knobs are explicitly set to 0 (opt-out path). - Remove unused RateLimitError import with noqa:F401 from http_driver.py. - Add unit tests covering: normal acquire/release, per-second enforcement, per-minute enforcement, semaphore exhaustion, token restoration on semaphore rejection, and thread safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/tool_http_request/IGlobal.py`:
- Around line 135-162: The current _build_rate_limiter() implements an
"all-or-nothing" opt-out via the helper _is_zero (all three raw_* must be
explicitly 0) while _config_int silently falls back to defaults when a user
supplies a non-None value <= 0; update _build_rate_limiter to warn when a raw
value was provided but clamped/reverted by _config_int: inspect
raw_ps/raw_pm/raw_mc, and for any raw that is not None and parses to an int <= 0
(use the same parsing logic as _is_zero), emit a clear warning message via the
module logger mentioning the offending field name (e.g. 'rateLimitPerSecond')
and that it will be treated as the default; keep the overall opt-out behavior
and continue using _config_int/RateLimiter/DEFAULT_* as currently implemented.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6dcf87de-05a3-497c-ba9f-70eeceff1ca4
📒 Files selected for processing (6)
nodes/src/nodes/tool_http_request/IGlobal.pynodes/src/nodes/tool_http_request/http_client.pynodes/src/nodes/tool_http_request/http_driver.pynodes/src/nodes/tool_http_request/rate_limiter.pynodes/src/nodes/tool_http_request/services.jsonnodes/test/test_rate_limiter.py
|
Addressed all review feedback:
Thanks for the review! |
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @charliegillet.
However, since this was authored, PR #599 landed and replaced the tool node architecture that this PR was built on.
This PR needs to be rebuilt against the new pattern before it can merge.
…miter - Fix critical bug: check semaphore BEFORE consuming tokens so rejected requests don't wastefully consume rate-limit tokens. Release semaphore if token check fails afterward. - Extract _config_int() utility with min_value/max_value clamping params from inline _int_or_default helper in IGlobal._build_rate_limiter. - Allow _build_rate_limiter to return None when all three rate-limit knobs are explicitly set to 0 (opt-out path). - Remove unused RateLimitError import with noqa:F401 from http_driver.py. - Add unit tests covering: normal acquire/release, per-second enforcement, per-minute enforcement, semaphore exhaustion, token restoration on semaphore rejection, and thread safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charliegillet
force-pushed
the
fix/http-request-rate-limiting
branch
from
a0c7f46 to
c173873
Compare
|
Rebased on develop and migrated to the @tool_function decorator pattern from PR #599. Removed the HttpDriver(ToolsBase) approach and integrated rate limiting directly into the @tool_function |
|
Addressing @asclearuc's inline feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/test/test_rate_limiter.py`:
- Around line 18-23: The test currently mutates sys.path in test_rate_limiter.py
using sys.path.insert(...) to import rate_limiter; move this path setup into a
reusable pytest fixture or conftest.py so multiple tests can reuse it: create a
conftest.py that defines a session-scoped fixture (or module import-time setup)
which adds Path(__file__).resolve().parent.parent / 'src' / 'nodes' /
'tool_http_request' to sys.path before tests run, and update
test_rate_limiter.py to remove the inline sys.path modification and simply
import RateLimiter and RateLimitError; reference the existing sys.path.insert
call and the imports from rate_limiter to locate where to remove and replace
with the conftest-managed setup.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 688b4aa2-1f12-4235-a573-6e9462a106eb
📒 Files selected for processing (6)
nodes/src/nodes/tool_http_request/IGlobal.pynodes/src/nodes/tool_http_request/IInstance.pynodes/src/nodes/tool_http_request/http_client.pynodes/src/nodes/tool_http_request/rate_limiter.pynodes/src/nodes/tool_http_request/services.jsonnodes/test/test_rate_limiter.py
Senior Review — PR #560: fix(nodes): add rate limiting to tool_http_request@charliegillet, great initiative here — proactively rebasing onto the new Status: With the rebase complete and tests added, this looks ready for a re-review. One suggestion: Now that the tests are in place, it would be worth verifying the token-bucket math explicitly. Specifically:
If the existing tests already cover these edge cases, great — just flagging it as a sanity check given that rate limiting bugs tend to be subtle and only surface under load. This is looking close. A re-review should be able to move this forward. |
🚀 Merge RequestAuthor has proactively rebased onto the new @tool_function architecture from PR #599 and added tests. Status: Looks ready for re-review and merge. Please verify the token-bucket implementation against the new pattern. @asclearuc |
|
No description provided. |
|
All review feedback has been addressed. Here's a summary: Rebase on #599: The branch was already rebased on develop (which includes #599). The rate limiter integrates cleanly with the new Previously addressed (commits
New commit (
All tests pass, |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/test/test_rate_limiter.py`:
- Around line 163-171: Add a return type annotation to the helper function
worker (declare it as returning None) and suppress the BLE001 warning for the
broad exception handler instead of changing behavior: keep the existing except
Exception as exc: errors.append(exc) but append a noqa comment for BLE001 on
that except line; locate the worker function wrapping rl.acquire()/rl.release()
and the except RateLimitError/except Exception as exc handlers to apply these
edits.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97777778-b54b-43ce-86d4-c1ce91fa58de
📒 Files selected for processing (3)
nodes/src/nodes/tool_http_request/services.jsonnodes/test/conftest.pynodes/test/test_rate_limiter.py
Latest fixes
|
Without rate limiting, the HTTP request node can be abused to flood external APIs or exhaust server resources. This adds a token-bucket rate limiter with three configurable limits: per-second, per-minute, and max concurrent requests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…miter - Fix critical bug: check semaphore BEFORE consuming tokens so rejected requests don't wastefully consume rate-limit tokens. Release semaphore if token check fails afterward. - Extract _config_int() utility with min_value/max_value clamping params from inline _int_or_default helper in IGlobal._build_rate_limiter. - Allow _build_rate_limiter to return None when all three rate-limit knobs are explicitly set to 0 (opt-out path). - Remove unused RateLimitError import with noqa:F401 from http_driver.py. - Add unit tests covering: normal acquire/release, per-second enforcement, per-minute enforcement, semaphore exhaustion, token restoration on semaphore rejection, and thread safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stepmikhaylov
force-pushed
the
fix/http-request-rate-limiting
branch
from
cdd2fdb to
d66d1c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/src/nodes/tool_http_request/services.json`:
- Around line 114-130: Update the three schema entries
"http_request.rateLimitPerSecond", "http_request.rateLimitPerMinute", and
"http_request.maxConcurrentRequests" so their description strings explicitly
document the opt-out behavior; add the sentence "Set to 0 to disable (all three
rate-limit fields must be 0 to fully opt out)." to each description (or add it
to one description and reference the requirement from the other two) so users
know setting a single field to 0 does not disable that limit unless all three
are 0.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4eb122dd-6a3e-423a-aad6-9526448a1ef1
📒 Files selected for processing (7)
nodes/src/nodes/tool_http_request/IGlobal.pynodes/src/nodes/tool_http_request/IInstance.pynodes/src/nodes/tool_http_request/http_client.pynodes/src/nodes/tool_http_request/rate_limiter.pynodes/src/nodes/tool_http_request/services.jsonnodes/test/tool_http_request/conftest.pynodes/test/tool_http_request/test_rate_limiter.py
stepmikhaylov
force-pushed
the
fix/http-request-rate-limiting
branch
from
d66d1c8 to
d7ba8e9
Compare
stepmikhaylov
force-pushed
the
fix/http-request-rate-limiting
branch
from
d7ba8e9 to
cffaa9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/test/tool_http_request/test_rate_limiter.py`:
- Around line 1-4: Update the file header to match the repository standard by
replacing the existing "Copyright (c) 2024 RocketRide Inc." line with "Copyright
(c) 2026 Aparavi Software AG" (keep the surrounding MIT License header lines
intact); search for the exact string "Copyright (c) 2024 RocketRide Inc." in
this file and replace it so the header matches the other module (__init__.py) in
the same directory.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e227ecc-2469-41eb-9abc-688d8e3936dc
📒 Files selected for processing (3)
nodes/test/tool_http_request/__init__.pynodes/test/tool_http_request/conftest.pynodes/test/tool_http_request/test_rate_limiter.py
* fix(nodes): add configurable rate limiting to tool_http_request node Without rate limiting, the HTTP request node can be abused to flood external APIs or exhaust server resources. This adds a token-bucket rate limiter with three configurable limits: per-second, per-minute, and max concurrent requests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(nodes): address PR #560 review feedback for rate limiter - Fix critical bug: check semaphore BEFORE consuming tokens so rejected requests don't wastefully consume rate-limit tokens. Release semaphore if token check fails afterward. - Extract _config_int() utility with min_value/max_value clamping params from inline _int_or_default helper in IGlobal._build_rate_limiter. - Allow _build_rate_limiter to return None when all three rate-limit knobs are explicitly set to 0 (opt-out path). - Remove unused RateLimitError import with noqa:F401 from http_driver.py. - Add unit tests covering: normal acquire/release, per-second enforcement, per-minute enforcement, semaphore exhaustion, token restoration on semaphore rejection, and thread safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore(nodes): move tool_http_request tests --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: stepmik <stepmikhaylov@yandex.ru>
4 participants
Summary
tool_http_requestnode using a thread-safe token-bucket algorithmservices.jsonwith sensible defaults (10/s, 100/m, 5 concurrent)RateLimitErrormessages returned when any limit is breachedType
Security Fix
Why this feature fits this codebase
Without rate limiting, an AI agent using
tool_http_requestcan fire unlimited outbound HTTP requests. This creates several abuse vectors:The existing guardrails (URL whitelist + method allowlist) control where requests go but not how fast. Rate limiting closes this gap.
What changed
rate_limiter.pythreading.Semaphorefor concurrency control. Enforces per-second, per-minute, and max-concurrent limits.http_driver.py_tool_invokenow callsrate_limiter.acquire()before the request andrelease()in afinallyblock.HttpDriver.__init__accepts an optionalRateLimiter.IGlobal.py_build_rate_limiter()readsrateLimitPerSecond,rateLimitPerMinute, andmaxConcurrentRequestsfrom node config and constructs the limiter. Passed intoHttpDriver.services.jsonhttp_client.pyValidation
ruff formatandruff checkpass on all changed files (only pre-existing D107 inhttp_driver.pyremains)acquire(blocking=False)to fail fast rather than queueHow to extend
RateLimiterwith a distributed implementation behind the sameacquire()/release()interfaceRateLimiterinstances keyed on the whitelist patternCloses: N/A — new contribution, no pre-existing issue
#Hack-with-bay-2
🤖 Generated with Claude Code
Summary by CodeRabbit