Set maximum length for fields#1235
Open
samdoran wants to merge 4 commits intolightspeed-core:mainfrom
Open
Conversation
We do not want to accept unbounded amounts of input. Use base 2 numbers because they are cool and nerdy.
Contributor
WalkthroughAdded max_length string validators to four Rlsapi V1 request fields (attachment.contents, terminal.output, context.stdin: 65,536; infer.question: 10,240). New unit and integration tests cover boundary and oversized payload cases. Removed pytest.mark.asyncio decorators from many async unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/models/rlsapi/requests.py (1)
32-35: Centralize max-length values into shared constants.The limits are correct, but repeating raw numbers across fields makes drift likely. Move these values to shared constants (preferably in
constants.py) and reference them here and in tests.Suggested refactor
+import constants + class RlsapiV1Attachment(ConfigurationBase): @@ contents: str = Field( default="", - max_length=65_536, + max_length=constants.RLSAPI_CONTEXT_MAX_LENGTH, @@ class RlsapiV1Terminal(ConfigurationBase): @@ output: str = Field( default="", - max_length=65_536, + max_length=constants.RLSAPI_CONTEXT_MAX_LENGTH, @@ class RlsapiV1Context(ConfigurationBase): @@ stdin: str = Field( default="", - max_length=65_536, + max_length=constants.RLSAPI_CONTEXT_MAX_LENGTH, @@ class RlsapiV1InferRequest(ConfigurationBase): @@ question: str = Field( ..., min_length=1, - max_length=10_240, + max_length=constants.RLSAPI_QUESTION_MAX_LENGTH,As per coding guidelines "Check
constants.pyfor shared constants before defining new ones".Also applies to: 52-55, 132-135, 176-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/rlsapi/requests.py` around lines 32 - 35, Several Pydantic Field max_length literals (e.g., the contents Field with max_length=65_536 and the other occurrences at the noted ranges) should be replaced with shared constants: add appropriately named constants (e.g., MAX_FILE_CONTENTS_LENGTH, MAX_TITLE_LENGTH, MAX_SUMMARY_LENGTH, etc.) to constants.py, import those symbols into src/models/rlsapi/requests.py, and use them in the Field(..., max_length=...) calls (update the Field declarations for identifiers like contents and the other max_length fields referenced at 52-55, 132-135, 176-180); also update any tests to reference the new constants from constants.py instead of the raw numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/endpoints/test_rlsapi_v1_integration.py`:
- Around line 14-17: Reorder the imports so all FastAPI-related imports are
grouped together: make the lines with TestClient and HTTPException/status
adjacent (e.g., place from fastapi import HTTPException, status next to from
fastapi.testclient import TestClient) so TestClient, HTTPException, and status
are in the same import block and no other imports separate them; this will
satisfy the pylint C0412 import grouping rule.
In `@tests/unit/models/rlsapi/test_requests.py`:
- Around line 604-633: Add a local pylint suppression and missing docstring:
annotate the TestMaxLengthValidation class with a pylint-disable comment for
R0903 (too-few-public-methods) and add a short method docstring to
test_value_at_max_length describing the test purpose; reference the class name
TestMaxLengthValidation and the method test_value_at_max_length so the linter
warnings (R0903 and C0116) are suppressed and the CI will pass.
---
Nitpick comments:
In `@src/models/rlsapi/requests.py`:
- Around line 32-35: Several Pydantic Field max_length literals (e.g., the
contents Field with max_length=65_536 and the other occurrences at the noted
ranges) should be replaced with shared constants: add appropriately named
constants (e.g., MAX_FILE_CONTENTS_LENGTH, MAX_TITLE_LENGTH, MAX_SUMMARY_LENGTH,
etc.) to constants.py, import those symbols into src/models/rlsapi/requests.py,
and use them in the Field(..., max_length=...) calls (update the Field
declarations for identifiers like contents and the other max_length fields
referenced at 52-55, 132-135, 176-180); also update any tests to reference the
new constants from constants.py instead of the raw numbers.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/models/rlsapi/requests.pytests/integration/endpoints/test_rlsapi_v1_integration.pytests/unit/app/endpoints/test_rlsapi_v1.pytests/unit/models/rlsapi/test_requests.py
💤 Files with no reviewable changes (1)
- tests/unit/app/endpoints/test_rlsapi_v1.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/endpoints/test_rlsapi_v1_integration.py (1)
512-516: Assert status before parsingdetailto improve failure clarity.Line 513 dereferences
response.json()["detail"]before Line 515 validates the expected status, which can hide the root cause when the response shape differs.Proposed change
def test_infer_size_limit(integration_http_client: TestClient, json) -> None: """Test that a field exceeding limit is rejected.""" response = integration_http_client.post("/v1/infer", json=json) - detail = response.json()["detail"] - assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT + detail = response.json()["detail"] assert "string_too_long" in {item["type"] for item in detail}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_rlsapi_v1_integration.py` around lines 512 - 516, The test reads response.json()["detail"] into detail before asserting the response.status_code, which can mask schema errors; change the order in the test around the integration_http_client.post call so you first assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT and only then parse response.json() to extract detail, and keep the existing assertion that "string_too_long" is in {item["type"] for item in detail}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/models/rlsapi/test_requests.py`:
- Around line 607-612: The parametrized test ids array contains a truncated id
"infer-request-questio"; update the ids list in
tests/unit/models/rlsapi/test_requests.py (the ids used for the parametrized
test) to the full, descriptive value "infer-request-question" so test reports
and -k filtering work correctly.
---
Nitpick comments:
In `@tests/integration/endpoints/test_rlsapi_v1_integration.py`:
- Around line 512-516: The test reads response.json()["detail"] into detail
before asserting the response.status_code, which can mask schema errors; change
the order in the test around the integration_http_client.post call so you first
assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT and only
then parse response.json() to extract detail, and keep the existing assertion
that "string_too_long" is in {item["type"] for item in detail}.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/endpoints/test_rlsapi_v1_integration.pytests/unit/app/endpoints/test_rlsapi_v1.pytests/unit/models/rlsapi/test_requests.py
💤 Files with no reviewable changes (1)
- tests/unit/app/endpoints/test_rlsapi_v1.py
major
approved these changes
Feb 27, 2026
Contributor
major
left a comment
major
left a comment
There was a problem hiding this comment.
No objections here other than the typo that coderabbit found
tisnik
approved these changes
Feb 27, 2026
Contributor
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
Agree, LGTM, but please fix the typo.
The project configures pytest asyncio mode to auto so it is unnecessary to marke individual tests as async.
Contributor
Author
|
I corrected the test ID. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/endpoints/test_rlsapi_v1_integration.py (1)
500-516: Tighten failure matching to the specific oversized field.
Good boundary test. To prevent false positives, assert thelocfor the expected field in each case (not justtype == "string_too_long").Proposed refinement
`@pytest.mark.parametrize`( - "json", + ("json", "expected_loc"), ( - ({"question": "?" * 10_241}), - ({"question": "Q", "context": {"stdin": "a" * 65_537}}), - ({"question": "Q", "context": {"attachments": {"contents": "A" * 65_537}}}), - ({"question": "Q", "context": {"terminal": {"output": "T" * 65_537}}}), + ({"question": "?" * 10_241}, ["body", "question"]), + ({"question": "Q", "context": {"stdin": "a" * 65_537}}, ["body", "context", "stdin"]), + ( + {"question": "Q", "context": {"attachments": {"contents": "A" * 65_537}}}, + ["body", "context", "attachments", "contents"], + ), + ( + {"question": "Q", "context": {"terminal": {"output": "T" * 65_537}}}, + ["body", "context", "terminal", "output"], + ), ), ids=["question", "stdin", "attachment_contents", "terminal_output"], ) -def test_infer_size_limit(integration_http_client: TestClient, json) -> None: +def test_infer_size_limit( + integration_http_client: TestClient, json, expected_loc +) -> None: """Test that a field exceeding limit is rejected.""" response = integration_http_client.post("/v1/infer", json=json) detail = response.json()["detail"] assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT - assert "string_too_long" in {item["type"] for item in detail} + assert any( + item.get("type") == "string_too_long" and item.get("loc") == expected_loc + for item in detail + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_rlsapi_v1_integration.py` around lines 500 - 516, The test_infer_size_limit currently only checks for type == "string_too_long" which can produce false positives; update the parametrization to include the expected location for each payload case (e.g., ("question", ["body", "question"]), ("context.stdin", ["body", "context", "stdin"]), ("context.attachments.contents", ["body", "context", "attachments", "contents"]), ("context.terminal.output", ["body", "context", "terminal", "output"])), then send the request via integration_http_client.post("/v1/infer", json=json) and assert that response.status_code is HTTP_422_UNPROCESSABLE_CONTENT and that the response.json()["detail"] contains an item whose "type" == "string_too_long" and whose "loc" equals the expected location for that parametrized case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/endpoints/test_rlsapi_v1_integration.py`:
- Around line 500-516: The test_infer_size_limit currently only checks for type
== "string_too_long" which can produce false positives; update the
parametrization to include the expected location for each payload case (e.g.,
("question", ["body", "question"]), ("context.stdin", ["body", "context",
"stdin"]), ("context.attachments.contents", ["body", "context", "attachments",
"contents"]), ("context.terminal.output", ["body", "context", "terminal",
"output"])), then send the request via integration_http_client.post("/v1/infer",
json=json) and assert that response.status_code is
HTTP_422_UNPROCESSABLE_CONTENT and that the response.json()["detail"] contains
an item whose "type" == "string_too_long" and whose "loc" equals the expected
location for that parametrized case.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/endpoints/test_rlsapi_v1_integration.pytests/unit/app/endpoints/test_rlsapi_v1.pytests/unit/models/rlsapi/test_requests.py
💤 Files with no reviewable changes (1)
- tests/unit/app/endpoints/test_rlsapi_v1.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
/v1/inferAPI accepts arbitrary data in the post body. Set maximum limits on the question, stdin, attachment contents, and terminal output fields to avoid overwhelming the server.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Tests