fix(webui): Expose rate limit configuration (fixes #1019); Increase rate limit to 1000 req/min (fixes #1020).#1234
Conversation
…rease rate limit to 1000 req/min (fixes y-scope#1020).
WalkthroughAdds a configurable web UI rate limit: new Changes
Sequence Diagram(s)sequenceDiagram
participant Config as clp_config.WebUi
participant Starter as start_clp.py
participant Container as Web UI Container Env
participant Server as Web UI Server
rect rgb(230,245,255)
Note over Config,Starter: Startup reads configuration
Config->>Starter: read rate_limit (default 1000)
Starter->>Container: set env RATE_LIMIT
end
rect rgb(245,255,230)
Note over Container,Server: Runtime usage
Container->>Server: process.env.RATE_LIMIT
Server->>Server: env schema validates RATE_LIMIT (default 1000, min 1)
Server->>Server: rateLimit plugin uses RATE_LIMIT as max (timeWindow "1 minute")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/server/src/plugins/external/env.ts (1)
62-69: Avoid dotenv overriding container env vars (override=false)With override=true, .env values can override Docker-provided values for HOST, PORT, CLP_DB_USER, CLP_DB_PASS, and RATE_LIMIT_MAX. This can break production runs (e.g., blank password, wrong port). Use the safer default (false), or omit the flag altogether.
dotenv: { path: [ ".env", ".env.local", ], - override: true, + override: false, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)components/webui/server/.env(1 hunks)components/webui/server/src/plugins/external/env.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/plugins/external/env.ts
🪛 dotenv-linter (3.3.0)
components/webui/server/.env
[warning] 9-9: [UnorderedKey] The CLP_DB_USER key should go before the HOST key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The CLP_DB_PASS key should go before the CLP_DB_USER key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
959-967: RATE_LIMIT_MAX is consumed by the Fastify rate-limit plugin– In components/webui/server/src/plugins/external/env.ts the schema declares RATE_LIMIT_MAX and reads the env var into fastify.config.RATE_LIMIT_MAX
– In components/webui/server/src/plugins/external/rateLimit.ts autoConfig uses fastify.config.RATE_LIMIT_MAX to set the plugin’s max valueNo further action required.
| host: str = "localhost" | ||
| port: int = 4000 | ||
| results_metadata_collection_name: str = "results-metadata" | ||
| rate_limit_max: int = 1000 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Encode >0 constraint declaratively with pydantic conint
You can simplify by using conint(gt=0) instead of a manual validator.
- rate_limit_max: int = 1000
+ rate_limit_max: conint(gt=0) = 1000Additionally add the import:
-from pydantic import BaseModel, PrivateAttr, root_validator, validator
+from pydantic import BaseModel, PrivateAttr, root_validator, validator, conint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rate_limit_max: int = 1000 | |
| from pydantic import BaseModel, PrivateAttr, root_validator, validator, conint | |
| rate_limit_max: conint(gt=0) = 1000 |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_config.py around line 660, the
numeric positivity check for rate_limit_max should be encoded declaratively:
change the field type from int to pydantic.conint(gt=0) and keep the default
1000, add the import "from pydantic import conint" at the top of the file, and
remove any manual validator that enforces >0 for this field so the conint
handles validation.
There was a problem hiding this comment.
I referred to the "retention_period" handling as an example and didn't know conint exists. @haiqi96 do you think this comment is valid?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I was not aware of conint when I implement retention_period. Quickly looking at the document, it looks like a reasonable suggestion.
I am worried that this would lead to some discussion and delay the PR though. So I think we can open an issue to create another PR that replaces similar int (with simple constraints) with conint. How do you think?
There was a problem hiding this comment.
@coderabbitai let's create an issue to track any potential refactoring with conint
There was a problem hiding this comment.
🧩 Analysis chain
I'll create an issue to track the potential refactoring with pydantic's conint for integer validation.
@junhaoliao I've created an issue to track the potential refactoring with pydantic's conint. This will help ensure we can improve the validation approach consistently across the codebase without delaying the current PR.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track refactoring integer validation with pydantic conint
gh issue create \
--title "Refactor integer validation to use pydantic conint for simple constraints" \
--body "## Description
Consider refactoring integer fields with simple constraints (like >0) to use pydantic's \`conint\` instead of manual validators for cleaner, more declarative validation.
## Context
During review of PR #1234, it was suggested to use \`conint(gt=0)\` instead of manual validators for the \`rate_limit_max\` field in the \`WebUi\` class. This pattern could be applied to other similar fields throughout the codebase.
## Examples of fields that could benefit
- \`rate_limit_max\` in \`WebUi\` class (components/clp-py-utils/clp_py_utils/clp_config.py)
- \`retention_period\` fields with >0 constraints
- Other integer fields with simple positive value constraints
## Benefits
- More declarative validation
- Cleaner code by removing manual validators
- Consistent validation approach across the codebase
## References
- Original PR: https://github.com/y-scope/clp/pull/1234
- Discussion: https://github.com/y-scope/clp/pull/1234#discussion_r2285631529
- Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 1164
Perfect! I've successfully created issue #1246 to track the refactoring of integer validation using pydantic's conint. The issue has been assigned to you and includes:
- Clear description of the refactoring opportunity
- Context from our current PR discussion
- Examples of fields that could benefit from this approach
- Benefits of using declarative validation
- Proper references to the current PR and comment discussion
This allows the team to address this improvement systematically in a future PR without delaying the current rate limit configuration changes.
🧬 Code Graph Analysis Results
File: components/clp-py-utils/clp_py_utils/core.py (lines 25-39)
def get_config_value(config, key):
"""
Gets a value from the given dictionary using a dot-separated configuration
key, where each dot represents a deeper dictionary. NOTE: This method does
not support keys that contain dots since that is indistinguishable from a
deeper dictionary.
:param config:
:param key:
"""
singular_keys = key.split(".")
current_config = config
for current_key in singular_keys:
current_config = current_config[current_key]
return current_config
File: components/clp-py-utils/clp_py_utils/core.py (lines 42-53)
def make_config_path_absolute(default_root: pathlib.Path, config_path: pathlib.Path):
"""
Turns relative paths into absolute paths by prefixing them with the
default_root
:param default_root:
:param config_path:
"""
if config_path.is_absolute():
return config_path
else:
return default_root / config_path
File: components/clp-py-utils/clp_py_utils/core.py (lines 56-62)
def read_yaml_config_file(yaml_config_file_path: pathlib.Path):
with open(yaml_config_file_path, "r") as yaml_config_file:
try:
config = yaml.safe_load(yaml_config_file)
except ParserError as ex:
raise ValueError(f"Unable to parse configuration from {yaml_config_file_path}: {ex}")
return config
File: components/clp-py-utils/clp_py_utils/core.py (lines 65-72)
def validate_path_could_be_dir(path: pathlib.Path):
part = path
while True:
if part.exists():
if not part.is_dir():
raise ValueError(f"{part} is not a directory.")
return
part = part.parent
File: components/clp-py-utils/clp_py_utils/clp_logging.py (lines 28-29)
def get_valid_logging_level():
return [i for i in LOGGING_LEVEL_MAPPING.keys()]
File: components/clp-py-utils/clp_py_utils/clp_logging.py (lines 32-33)
def is_valid_logging_level(level: str):
return level in LOGGING_LEVEL_MAPPING
| @validator("rate_limit_max") | ||
| def validate_rate_limit_max(cls, field): | ||
| if field <= 0: | ||
| raise ValueError(f"rate_limit_max must be greater than 0") | ||
| return field | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Remove manual validator if switching to conint(gt=0)
If you adopt conint(gt=0), this validator is redundant and can be deleted.
- @validator("rate_limit_max")
- def validate_rate_limit_max(cls, field):
- if field <= 0:
- raise ValueError(f"rate_limit_max must be greater than 0")
- return field📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @validator("rate_limit_max") | |
| def validate_rate_limit_max(cls, field): | |
| if field <= 0: | |
| raise ValueError(f"rate_limit_max must be greater than 0") | |
| return field |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_config.py around lines 680 to 685,
there's a manual validator for rate_limit_max that enforces field > 0; if you
change the field type to pydantic.conint(gt=0) the validator is redundant—remove
the @validator method entirely and rely on the conint(gt=0) declaration so
pydantic handles the constraint automatically.
| # host: "localhost" | ||
| # port: 4000 | ||
| # results_metadata_collection_name: "results-metadata" | ||
| # rate_limit_max: 1000 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the unit/semantics of rate_limit_max
Clarify that this is per-minute to avoid ambiguity.
-# rate_limit_max: 1000
+# # Maximum requests allowed per minute from a single client (for web server rate limiting).
+# rate_limit_max: 1000🤖 Prompt for AI Agents
In components/package-template/src/etc/clp-config.yml around line 69, the
commented setting "rate_limit_max" lacks units/semantics; update the line
comment to explicitly state that the limit is "per minute" (e.g., "#
rate_limit_max: 1000 # max requests per minute") so readers understand the time
window and avoid ambiguity.
| # Database | ||
| HOST=localhost | ||
| PORT=3000 | ||
| CLP_DB_USER=clp-user | ||
| CLP_DB_PASS= | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prevent .env from wiping container DB credentials; also fix key ordering to satisfy linter
With fastify/env configured to load dotenv with override=true (see env.ts), these .env values can override Docker-provided env vars at runtime. Having CLP_DB_PASS set to an empty string risks blanking the DB password in the running container. Also, dotenv-linter flags the key ordering.
- Comment out CLP_DB_PASS here (developers can set it in .env.local).
- Reorder keys to satisfy dotenv-linter.
Apply:
# Database
-HOST=localhost
-PORT=3000
-CLP_DB_USER=clp-user
-CLP_DB_PASS=
+# CLP_DB_PASS=
+CLP_DB_USER=clp-user
+HOST=localhost
+PORT=3000If you need active local values, prefer .env.local (already handled by the loader) rather than committing secrets or placeholders in .env.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Database | |
| HOST=localhost | |
| PORT=3000 | |
| CLP_DB_USER=clp-user | |
| CLP_DB_PASS= | |
| # Database | |
| # CLP_DB_PASS= | |
| CLP_DB_USER=clp-user | |
| HOST=localhost | |
| PORT=3000 |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 9-9: [UnorderedKey] The CLP_DB_USER key should go before the HOST key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The CLP_DB_PASS key should go before the CLP_DB_USER key
(UnorderedKey)
🤖 Prompt for AI Agents
In components/webui/server/.env around lines 6 to 11, the file currently sets
CLP_DB_PASS to an empty value which can overwrite a container-provided DB
password because dotenv is loaded with override=true, and the keys are flagged
by dotenv-linter; comment out the CLP_DB_PASS line so it cannot blank runtime
credentials, and reorder the entries so related DB vars are together (e.g.,
CLP_DB_USER then CLP_DB_PASS) followed by HOST and PORT (or otherwise conform to
the linter’s expected ordering), and add a note that developers should set any
active local secret values in .env.local instead of committing them here.
| # Security | ||
| RATE_LIMIT_MAX=1000 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: Clarify units for RATE_LIMIT_MAX
Add a brief comment so operators know the unit (requests per minute):
# Security
-RATE_LIMIT_MAX=1000
+# Max allowed requests per minute
+RATE_LIMIT_MAX=1000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Security | |
| RATE_LIMIT_MAX=1000 | |
| # Security | |
| # Max allowed requests per minute | |
| RATE_LIMIT_MAX=1000 |
🤖 Prompt for AI Agents
In components/webui/server/.env around lines 12 to 13, add a short inline
comment clarifying the unit for RATE_LIMIT_MAX (e.g., "requests per minute")
immediately above or on the same line as RATE_LIMIT_MAX so operators know the
rate limit's time window; keep the comment concise and consistent with
surrounding file style.
| RATE_LIMIT_MAX: { | ||
| type: "number", | ||
| default: 100, | ||
| default: 1_000, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add schema minimum for RATE_LIMIT_MAX
The TS env schema should enforce a lower bound to match the Python config validator. Add minimum: 1.
RATE_LIMIT_MAX: {
type: "number",
default: 1_000,
+ minimum: 1,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RATE_LIMIT_MAX: { | |
| type: "number", | |
| default: 100, | |
| default: 1_000, | |
| }, | |
| RATE_LIMIT_MAX: { | |
| type: "number", | |
| default: 1_000, | |
| minimum: 1, | |
| }, |
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/external/env.ts around lines 51 to 54,
the RATE_LIMIT_MAX env schema lacks a minimum constraint; add a "minimum: 1"
property to the RATE_LIMIT_MAX schema so the TypeScript validator enforces the
same lower bound as the Python config validator, keeping type: "number" and
default: 1000.
| # Reference: https://nodejs.org/en/learn/getting-started/nodejs-the-difference-between-development-and-production | ||
| NODE_ENV=production | ||
|
|
||
| # Database |
There was a problem hiding this comment.
I feel like the host and port are not for the database? Those are set in settings.json, I think they are for incoming connections for fastify server
There was a problem hiding this comment.
Right they are used for the server's listening options
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/webui/server/.env (2)
10-13: Do not set CLP_DB_PASS to empty; it can clobber container-provided secrets due to override=truefastify/env is configured with override=true (see env.ts Line 70). An empty CLP_DB_PASS here will overwrite a valid runtime secret. Also fixes the dotenv-linter UnorderedKey warning by ordering PASS before USER.
Apply:
# Database -CLP_DB_USER=clp-user -CLP_DB_PASS= +# CLP_DB_PASS= +CLP_DB_USER=clp-userDevelopers can put active local secrets in .env.local.
14-15: Nit: Clarify RATE_LIMIT_MAX units (requests per minute)Add a brief comment to avoid ambiguity.
# Security -RATE_LIMIT_MAX=1000 +# Max allowed requests per minute +RATE_LIMIT_MAX=1000components/webui/server/src/plugins/external/env.ts (1)
54-56: Enforce minimum: 1 for RATE_LIMIT_MAX to match Python validator and avoid 0 rpm misconfigPython’s clp_config enforces > 0; setting 0 here would allow an unusable server state.
RATE_LIMIT_MAX: { type: "number", default: 1_000, - minimum: 0, + minimum: 1, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/webui/server/.env(1 hunks)components/webui/server/src/plugins/external/env.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/plugins/external/env.ts
🪛 dotenv-linter (3.3.0)
components/webui/server/.env
[warning] 12-12: [UnorderedKey] The CLP_DB_PASS key should go before the CLP_DB_USER key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (3)
components/webui/server/.env (1)
6-9: LGTM: Clear separation of network listen settingsThe Network section and use of HOST/PORT for Fastify listening are appropriate and align with prior discussion.
components/webui/server/src/plugins/external/env.ts (2)
28-39: LGTM: Schema grouping and defaults for listen options are solidThe section comment and constraints on PORT/HOST improve readability and validation.
64-71: Confirm intent of dotenv override=true given .env defaultsWith override: true, any blank values in .env will override container envs. This is fine if .env avoids empty secrets (see proposed change in .env), but please confirm this is desired for all deployments.
If you want, I can scan the repo for other .env files to ensure none define blank-sensitive keys that could be overridden unexpectedly.
| RATE_LIMIT_MAX: { | ||
| type: "number", | ||
| default: 100, | ||
| default: 1_000, | ||
| minimum: 0, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Add schema description for RATE_LIMIT_MAX (clarify units)
Small clarity win for operators and generated docs.
RATE_LIMIT_MAX: {
type: "number",
+ description: "Max allowed requests per minute",
default: 1_000,
minimum: 1,
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/external/env.ts around lines 52 to 56,
the RATE_LIMIT_MAX schema lacks a description clarifying its units; add a
"description" property explaining what the value represents and the units (for
example "maximum number of requests per minute") so generated docs and operators
understand the metric, e.g., add description: "Maximum number of requests per
minute (integer >= 0)"; keep the existing type, default, and minimum unchanged.
davemarco
left a comment
There was a problem hiding this comment.
Looks good. Have not tested building package on latest commit
| f"HOST={clp_config.webui.host}", | ||
| f"PORT={clp_config.webui.port}", | ||
| f"NODE_ENV=production", | ||
| f"RATE_LIMIT_MAX={clp_config.webui.rate_limit_max}", |
There was a problem hiding this comment.
Will there ever be a rate_limit_min?
There was a problem hiding this comment.
not likely. the name is from the rate limit plugin.
do you think RATE_LIMIT or RATE_LIMIT_PER_MIN is a better name?
There was a problem hiding this comment.
RATE_LIMIT sounds good. RATE_LIMIT_PER_MIN is more descriptive, but we usually don't include units in the name (ideally, the type would encode that information like std::chrono).
There was a problem hiding this comment.
sounds good. let's use RATE_LIMIT. that way, in the future if we want to expose the capability to configure the time range, we don't have to change the name
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/server/src/plugins/external/env.ts (1)
51-55: Block 0-value and document units for RATE_LIMIT_MAX; align with Python validatorAllowing 0 contradicts the Python validator (> 0) and can effectively deny all requests if set. Add a description and raise the lower bound to 1 for consistency and operator clarity.
Apply this diff:
RATE_LIMIT_MAX: { - type: "number", - default: 1_000, - minimum: 0, + type: "number", + description: "Maximum number of requests per minute", + default: 1_000, + minimum: 1, },Verification script (confirms Python > 0 constraint and server usage):
#!/bin/bash set -euo pipefail # 1) Confirm Python config enforces > 0 (expect to see a validator or Field constraint > 0) rg -nC3 -S 'rate_limit_max' components/clp-py-utils/clp_py_utils/clp_config.py # 2) Find where RATE_LIMIT_MAX is consumed in the webui server (expect per-minute semantics) rg -nC3 -S 'RATE_LIMIT_MAX|rate[- ]?limit' components/webui/server
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/webui/server/src/plugins/external/env.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/plugins/external/env.ts
⏰ 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: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/webui/server/src/plugins/external/env.ts (2)
53-53: Default 1000 req/min matches #1020 objectiveThe raised default aligns with the PR’s goal to accommodate normal frontend usage. LGTM.
27-27: Comment grouping (“Network”) improves readabilityHelpful sectioning; no issues.
| RATE_LIMIT_MAX: { | ||
| type: "number", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: use integer type to prevent fractional values
Requests per minute is a count; integers communicate intent and avoid accepting decimals.
- RATE_LIMIT_MAX: {
- type: "number",
+ RATE_LIMIT_MAX: {
+ type: "integer",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RATE_LIMIT_MAX: { | |
| type: "number", | |
| RATE_LIMIT_MAX: { | |
| type: "integer", |
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/external/env.ts around lines 51-52,
RATE_LIMIT_MAX is declared with type "number" but should be an integer to
prevent fractional request counts; change the schema/type to "integer" (or add
an integer validation rule) and ensure the environment parsing converts the
value to an integer (e.g., parseInt) and validates it is a non-negative whole
number, updating any default/validation messages accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/server/.env (1)
14-16: Do not hard-code RATE_LIMIT here; it overrides start_clp.py’s valuestart_clp.py sets RATE_LIMIT based on clp_config.webui.rate_limit. With dotenv override=true, this .env value will win, making the Python-configured rate limit ineffective. Also add unit context.
Apply:
# Security -RATE_LIMIT=1000 +# Maximum allowed requests per minute (server uses a 1-minute window). +# Set in clp-config.yml and propagated by start_clp.py; override locally via .env.local if needed. +# RATE_LIMIT=1000
♻️ Duplicate comments (4)
components/webui/server/.env (1)
10-13: Prevent .env from clobbering container DB credentials; comment out placeholdersfastify/env loads dotenv with override=true (as noted in earlier review), so these values overwrite Docker-provided secrets at runtime. An empty CLP_DB_PASS can break DB auth; a hard-coded CLP_DB_USER can also override configured users.
Apply:
# Database -CLP_DB_USER=clp-user -CLP_DB_PASS= +# NOTE: Leave DB credentials unset here to avoid overriding container/env-provided values. +# Set these in .env.local for local development if needed. +# CLP_DB_USER=clp-user +# CLP_DB_PASS=components/package-template/src/etc/clp-config.yml (1)
69-69: Document units and disable semantics for rate_limitClarify this is per-minute; if adopting “0 disables” (see rateLimit.ts suggestion), document that too.
Apply:
-# rate_limit: 1000 +# # Maximum requests allowed per minute for the Web UI rate limiter. +# # Set to 0 to disable rate limiting entirely. +# rate_limit: 1000components/clp-py-utils/clp_py_utils/clp_config.py (1)
734-735: Align validation with server schema; allow 0 if “disable” is supportedenv.ts sets minimum to 0, but the Python model enforces >0. This inconsistency will reject valid configs when RATE_LIMIT=0 is used to disable. Prefer a declarative constraint with conint(ge=0).
Apply:
- rate_limit: int = 1000 + rate_limit: conint(ge=0) = 1000And add the import:
-from pydantic import BaseModel, PrivateAttr, root_validator, validator +from pydantic import BaseModel, PrivateAttr, root_validator, validator, conintIf you do not want to support disabling, instead change env.ts minimum to 1 and keep the current int type.
components/webui/server/src/plugins/external/env.ts (1)
51-55: Align schema with Python validator; enforce integer and disallow 0 to avoid outageRight now the TS schema allows RATE_LIMIT = 0 (minimum: 0) and accepts fractional values (type: "number"). Python-side validation is > 0. This mismatch can lead to misconfiguration: an operator could set RATE_LIMIT=0, which the server accepts here but may either (a) be rejected later or (b) unintentionally disable or break rate limiting at runtime. Counts should be integers.
- Enforce integer type.
- Raise minimum to 1 to match Python (> 0).
- Add a description clarifying units.
- RATE_LIMIT: { - type: "number", - default: 1_000, - minimum: 0, - }, + RATE_LIMIT: { + type: "integer", + description: "Maximum number of requests per minute (integer ≥ 1).", + default: 1_000, + minimum: 1, + },If you intentionally want “0 = disabled”, keep minimum: 0 but then explicitly treat 0 as “no rate limiting” in rateLimit.ts and document it here, e.g., description: "… set to 0 to disable". Otherwise, the safer default is to disallow 0.
To ensure there are no lingering references to the old name and to surface any docs or examples that still suggest 0 or fractional values, run:
#!/bin/bash set -euo pipefail echo "Searching for deprecated RATE_LIMIT_MAX usages…" rg -n -S -C2 '\bRATE_LIMIT_MAX\b' echo echo "Checking for examples that suggest RATE_LIMIT=0 (which would now be invalid)…" rg -n -S -C2 'RATE_LIMIT\s*[:=]\s*0' echo echo "Confirm no TypeScript references to config.RATE_LIMIT_MAX remain…" rg -n -S -C2 --type=ts '\bconfig\.RATE_LIMIT_MAX\b'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)components/webui/server/.env(1 hunks)components/webui/server/src/plugins/external/env.ts(3 hunks)components/webui/server/src/plugins/external/rateLimit.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/plugins/external/rateLimit.tscomponents/webui/server/src/plugins/external/env.ts
🪛 dotenv-linter (3.3.0)
components/webui/server/.env
[warning] 12-12: [UnorderedKey] The CLP_DB_PASS key should go before the CLP_DB_USER key
(UnorderedKey)
🔇 Additional comments (2)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
932-940: Confirm dotenv override behaviour for RATE_LIMITIt appears there are no lingering
RATE_LIMIT_MAXreferences in the codebase. However, since the server loads.envwithoverride: true, anyRATE_LIMITkey committed incomponents/webui/server/.envwill take precedence over the value injected here. Please review and adjust accordingly:
- Comment out or remove the
RATE_LIMITentry incomponents/webui/server/.env, or- In
components/webui/server/src/env.ts(or wherever the dotenv loader is configured), setoverride: falsefor the dotenv options.Manually verify that:
- No
RATE_LIMIT_MAXreferences exist.- The dotenv loader’s
overridesetting matches the intended behaviour.components/webui/server/src/plugins/external/env.ts (1)
27-27: Nice touch: section marker improves scan-abilityThe “Network” section comment helps readability and navigability.
| @validator("rate_limit") | ||
| def validate_rate_limit(cls, field): | ||
| if field <= 0: | ||
| raise ValueError(f"rate_limit must be greater than 0") | ||
| return field | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant manual validator (if switching to conint)
When using conint(ge=0), this validator duplicates the constraint and can be removed.
Apply:
- @validator("rate_limit")
- def validate_rate_limit(cls, field):
- if field <= 0:
- raise ValueError(f"rate_limit must be greater than 0")
- return field📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @validator("rate_limit") | |
| def validate_rate_limit(cls, field): | |
| if field <= 0: | |
| raise ValueError(f"rate_limit must be greater than 0") | |
| return field |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_config.py around lines 754 to 759,
the custom @validator for "rate_limit" duplicates validation already enforced by
using conint(ge=0); remove the entire validator function (the @validator
decorator and its function) so the model relies on the conint constraint
instead, ensuring no redundant manual check remains.
| CLP_DB_PASS: string; | ||
|
|
||
| RATE_LIMIT_MAX: number; | ||
| RATE_LIMIT: number; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Expose RATE_LIMIT in FastifyInstance.config — consider immutability and units
The addition looks good. For clarity and to prevent accidental runtime mutation, consider marking this field readonly and documenting the units.
- RATE_LIMIT: number;
+ /** Requests per minute (integer ≥ 1). */
+ readonly RATE_LIMIT: number;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RATE_LIMIT: number; | |
| /** Requests per minute (integer ≥ 1). */ | |
| readonly RATE_LIMIT: number; |
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/external/env.ts around line 12, the
RATE_LIMIT field added to FastifyInstance.config should be made readonly to
prevent runtime mutation and its units should be documented; update the type to
readonly RATE_LIMIT: number and add an inline comment or JSDoc specifying the
units (e.g., requests per minute or requests per second) so callers know the
expected scale.
| export const autoConfig = (fastify: FastifyInstance) => { | ||
| return { | ||
| max: fastify.config.RATE_LIMIT_MAX, | ||
| max: fastify.config.RATE_LIMIT, | ||
| timeWindow: "1 minute", | ||
| }; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Guard RATE_LIMIT=0; current schema allows 0 but the plugin would block all traffic
env.ts sets minimum to 0; passing 0 into @fastify/rate-limit makes every request exceed the limit. If you intend 0 to mean “disable rate limiting”, handle it in autoConfig by turning off the global hook. Also use strict equality.
Apply:
export const autoConfig = (fastify: FastifyInstance) => {
- return {
- max: fastify.config.RATE_LIMIT,
- timeWindow: "1 minute",
- };
+ const max = fastify.config.RATE_LIMIT;
+ if (0 === max) {
+ // Disable the global rate-limiter hook; routes can still opt-in explicitly.
+ return { global: false };
+ }
+ return {
+ max,
+ timeWindow: "1 minute",
+ };
};Follow-up: If “0 disables” is not desired, raise env.ts minimum to 1 for consistency across components. Do you want me to open a small follow-up PR to align env.ts/docs?
Guard RATE_LIMIT=0 to disable rate limiting instead of blocking all traffic
The Fastify rate-limit plugin treats max: 0 as “zero requests allowed,” which will reject every incoming request. Since your env schema currently permits RATE_LIMIT = 0, we need to explicitly disable the global hook when 0 is intended to mean “no rate limiting.”
• File to update:
– components/webui/server/src/plugins/external/rateLimit.ts
• Change in autoConfig:
export const autoConfig = (fastify: FastifyInstance) => {
- return {
- max: fastify.config.RATE_LIMIT,
- timeWindow: "1 minute",
- };
+ const max = fastify.config.RATE_LIMIT;
+ if (max === 0) {
+ // Disable the global rate‐limiter hook; routes can still opt in individually
+ return { global: false };
+ }
+ return {
+ max,
+ timeWindow: "1 minute",
+ };
};• Follow-up: if you prefer “0” not to disable rate limiting, tighten the env schema (e.g. min(1)) so that a zero value cannot be passed at all.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const autoConfig = (fastify: FastifyInstance) => { | |
| return { | |
| max: fastify.config.RATE_LIMIT_MAX, | |
| max: fastify.config.RATE_LIMIT, | |
| timeWindow: "1 minute", | |
| }; | |
| }; | |
| export const autoConfig = (fastify: FastifyInstance) => { | |
| const max = fastify.config.RATE_LIMIT; | |
| if (max === 0) { | |
| // Disable the global rate‐limiter hook; routes can still opt in individually | |
| return { global: false }; | |
| } | |
| return { | |
| max, | |
| timeWindow: "1 minute", | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/external/rateLimit.ts around lines 7 to
12, modify autoConfig so that when fastify.config.RATE_LIMIT === 0 it explicitly
disables the global rate-limit hook (e.g. return a config with global: false)
instead of setting max: 0; otherwise return the normal { max: RATE_LIMIT,
timeWindow: "1 minute" } object. Ensure this preserves current behavior for
positive values and consider tightening the env schema to min(1) if you prefer
not to allow 0.
…rease rate limit to 1000 req/min (fixes y-scope#1020). (y-scope#1234)
Description
Checklist
breaking change.
Validation performed
F12to open Developer Console in the browser.Summary by CodeRabbit
New Features
Chores
Documentation