fix: preserve explicit zero-length limits in handlers and storage#440
Open
Subhajitdas99 wants to merge 4 commits intoGetBindu:mainfrom
Open
fix: preserve explicit zero-length limits in handlers and storage#440Subhajitdas99 wants to merge 4 commits intoGetBindu:mainfrom
Subhajitdas99 wants to merge 4 commits intoGetBindu:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHandlers now strictly validate non-negative integer Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Scheduler
participant Storage
Client->>Handler: Request (params includes length/history_length)
Handler->>Handler: _parse_non_negative_* (reject bool/non-int/neg, accept 0)
alt submit & schedule
Handler->>Scheduler: run_task(scheduler_params with history_length maybe 0)
Scheduler->>Storage: create/load task
Storage-->>Scheduler: task (history truncated if history_length provided)
else list contexts/tasks
Handler->>Storage: list_contexts/list_tasks(length) -- length==0 returns []
Storage-->>Handler: result list
end
Handler-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/server/handlers/context_handlers.py`:
- Around line 46-49: The current assignment to length from params may accept
non-integer or negative values; update the logic in context_handlers.py where
length is derived from params (the length variable) to validate and normalize it
before forwarding to storage: ensure length is an int and >= 0 (accept 0),
otherwise clear/omit it (e.g., set to None) so invalid values like "", "0"
(string), or negatives are not forwarded; add a simple type-check/coercion path
only for safe numeric types if desired and add a type hint for the
params/handler signature to reflect an Optional[int] for history_length/length.
In `@bindu/server/handlers/message_handlers.py`:
- Around line 131-132: Validate and sanitize config["history_length"] before
adding it to scheduler_params: ensure the value is an integer and >= 0
(preserving 0), e.g., check isinstance(config["history_length"], int) and
config["history_length"] >= 0; if it's a numeric string you may attempt safe
coercion with int(...) inside a try/except, otherwise skip assignment or
raise/log a clear error; update the code that currently sets
scheduler_params["history_length"] from config to perform this validation and
only assign when the value is a valid non-negative int.
🪄 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: 77c9bf50-8518-451c-a7b3-bd4a4bd0dc97
📒 Files selected for processing (7)
bindu/server/handlers/context_handlers.pybindu/server/handlers/message_handlers.pybindu/server/storage/memory_storage.pybindu/server/storage/postgres_storage.pytests/unit/server/handlers/test_context_handlers.pytests/unit/server/handlers/test_message_handlers.pytests/unit/server/storage/test_memory_storage.py
Comment on lines
+46
to
+49
| if "length" in params: | ||
| length = params["length"] | ||
| else: | ||
| length = params.get("history_length") |
There was a problem hiding this comment.
Validate length type before forwarding to storage.
This now correctly preserves 0, but it also forwards invalid external values (e.g., "", "0", negative numbers), which can fail in storage calls.
Suggested hardening
params = request.get("params", {})
- if "length" in params:
- length = params["length"]
- else:
- length = params.get("history_length")
+ if "length" in params:
+ length = params["length"]
+ else:
+ length = params.get("history_length")
+
+ if length is not None:
+ if not isinstance(length, int) or length < 0:
+ raise TypeError("length must be a non-negative integer or null")As per coding guidelines: "Validate all external input and use type hints for input validation in Python files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/server/handlers/context_handlers.py` around lines 46 - 49, The current
assignment to length from params may accept non-integer or negative values;
update the logic in context_handlers.py where length is derived from params (the
length variable) to validate and normalize it before forwarding to storage:
ensure length is an int and >= 0 (accept 0), otherwise clear/omit it (e.g., set
to None) so invalid values like "", "0" (string), or negatives are not
forwarded; add a simple type-check/coercion path only for safe numeric types if
desired and add a type hint for the params/handler signature to reflect an
Optional[int] for history_length/length.
Contributor
Author
|
I also found that CI setup was failing because The remaining local pre-commit/pytest failure is still caused by the existing unrelated syntax issue in |
Contributor
Author
|
Good catch. I’ve updated this to validate external limit inputs before forwarding them, while still preserving explicit |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindu/server/handlers/message_handlers.py (1)
58-67: Consider extracting shared validation logic.This validation method duplicates
_parse_non_negative_lengthincontext_handlers.py. A shared utility (e.g., inbindu/utils/validation.py) could reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/handlers/message_handlers.py` around lines 58 - 67, Extract the duplicated validation logic into a shared utility function (e.g., validate_non_negative_int) in a new module bindu/utils/validation.py and update both _parse_non_negative_history_length (in message_handlers.py) and _parse_non_negative_length (in context_handlers.py) to delegate to that utility; the shared function should accept the raw value and a field name or message, perform the same checks (None -> None, reject bool, non-int, or negative -> raise ValueError with a clear message), and return the validated int so both handler methods become one-line wrappers calling validate_non_negative_int.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/server/handlers/message_handlers.py`:
- Around line 142-145: The code assigns the result of
_parse_non_negative_history_length (which may return int or None) directly into
scheduler_params["history_length"], causing a type error for TaskSendParams
which expects an int; change the logic in the block that handles
config["history_length"] so you call
self._parse_non_negative_history_length(config["history_length"]) and only set
scheduler_params["history_length"] when the parsed value is not None (i.e., skip
the assignment if parsing returns None) to ensure TaskSendParams.history_length
remains an int.
---
Nitpick comments:
In `@bindu/server/handlers/message_handlers.py`:
- Around line 58-67: Extract the duplicated validation logic into a shared
utility function (e.g., validate_non_negative_int) in a new module
bindu/utils/validation.py and update both _parse_non_negative_history_length (in
message_handlers.py) and _parse_non_negative_length (in context_handlers.py) to
delegate to that utility; the shared function should accept the raw value and a
field name or message, perform the same checks (None -> None, reject bool,
non-int, or negative -> raise ValueError with a clear message), and return the
validated int so both handler methods become one-line wrappers calling
validate_non_negative_int.
🪄 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: 0dda1381-be75-4b2f-95ab-5b61a7f52729
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
bindu/server/handlers/context_handlers.pybindu/server/handlers/message_handlers.pytests/unit/server/handlers/test_context_handlers.pytests/unit/server/handlers/test_message_handlers.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/server/handlers/test_context_handlers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/server/handlers/test_message_handlers.py
Contributor
Author
|
I checked the failing CI log. There was one PR-local typing issue in The remaining failures are still coming from unrelated repository-wide issues outside this PR, including:
|
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
1 participant
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.
Summary
Describe the problem and fix in 2–5 bullets:
length=0andhistory_length=0were treated as falsy/missing in some handlers and storage paths.Change Type (select all that apply)
Scope (select all touched areas)
Linked Issue/PR
User-Visible / Behavior Changes
length=0now return empty results instead of being treated as missing/falsy.history_length=0now return empty history consistently.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Verification
Environment
Steps to Test
length=0is preserved incontexts/listinstead of falling back tohistory_length.history_length=0returns empty history and zero-length list calls return empty results.Expected Behavior
length=0returns empty task/context results.history_length=0returns empty task history.Actual Behavior
79 passed.Evidence (attach at least one)
Human Verification (required)
What you personally verified (not just CI):
length=0in context listing, explicithistory_length=0in message scheduling and task history loading, zero-length list behavior in in-memory storage.__future__import syntax problem.Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. If none, write
None.Checklist
uv run pytest)uv run pre-commit run --all-files)Summary by CodeRabbit
Bug Fixes
Tests