fix: preserve explicit zero-length limits in handlers and storage#440
Open
Subhajitdas99 wants to merge 6 commits intoGetBindu:mainfrom
Open
fix: preserve explicit zero-length limits in handlers and storage#440Subhajitdas99 wants to merge 6 commits intoGetBindu:mainfrom
Subhajitdas99 wants to merge 6 commits intoGetBindu:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughHandlers now validate and normalize non-negative integer Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Scheduler
participant Storage
Client->>Handler: Request (params include length / history_length)
Handler->>Handler: _parse_non_negative_length(value, field_name)\n(reject bool/non-int/neg, accept 0)
alt Submit & Schedule flow
Handler->>Scheduler: run_task(..., history_length maybe 0)
Scheduler->>Storage: create/load task
Storage-->>Scheduler: task (history truncated if history_length provided)
else Listing flow
Handler->>Storage: list_contexts/list_tasks(length)
Note right of Storage: if length == 0 -> return []
Storage-->>Handler: result list
end
Handler-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
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:
|
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/message_handlers.py`:
- Around line 149-154: The ValueError thrown by
_parse_non_negative_history_length (propagating through
_submit_and_schedule_task) is currently reaching the top-level RPC handler and
being mapped to InternalError; catch that validation error earlier and convert
it to the RPC-level invalid-parameters error instead. Specifically, in the call
site where you set scheduler_params["history_length"] (the block using
_parse_non_negative_history_length and TaskSendParams["history_length"]), wrap
the parse in a try/except that catches ValueError and raises the RPC
InvalidParamsError (or translate into the same error type the a2a_protocol
handler would use for invalid params) so explicit bad input yields code
-32602/status 400 rather than an InternalError. Ensure you reference
_parse_non_negative_history_length and _submit_and_schedule_task when making the
change so the conversion happens before the exception reaches a2a_protocol's
top-level mapper.
In `@tests/unit/server/handlers/test_context_handlers.py`:
- Around line 59-75: The test test_list_contexts_preserves_explicit_zero_length
asserts the mock_storage.list_contexts call without the default owner_did
keyword; update the assertion to include owner_did=None because
ContextHandlers.list_contexts forwards owner_did=caller_did (caller_did defaults
to None when omitted). Change the final assertion on mock_storage.list_contexts
to assert it was called with length 0 and owner_did=None (use the exact keyword
name owner_did) so it matches the actual call shape from handler.list_contexts.
🪄 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: 0a7a5a9f-3905-4835-8f31-7652f953cbeb
📒 Files selected for processing (5)
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.py
🚧 Files skipped from review as they are similar to previous changes (3)
- bindu/server/storage/postgres_storage.py
- bindu/server/storage/memory_storage.py
- bindu/server/handlers/context_handlers.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindu/server/handlers/message_handlers.py (1)
135-158:⚠️ Potential issue | 🟠 MajorValidate
history_lengthbefore creating the task.Invalid external input is currently rejected only after
storage.submit_task(...), so a badconfiguration.history_lengthcan persist an unscheduled/orphan task. Parse the config before any storage side effect.As per coding guidelines, "Validate all external input and use type hints for input validation in Python files".
🐛 Proposed fix: move validation before task creation
"""Submit task to storage and schedule it with shared send/stream logic.""" message = request_params["message"] context_id = self.context_id_parser(message.get("context_id")) + config = request_params.get("configuration", {}) + history_length: int | None = None + if "history_length" in config: + try: + history_length = self._parse_non_negative_history_length( + config["history_length"] + ) + except ValueError as exc: + from bindu.common.protocol.types import InvalidParamsError + + raise InvalidParamsError(str(exc)) from exc + task: Task = await self.storage.submit_task( context_id, message, caller_did=caller_did ) scheduler_params: TaskSendParams = TaskSendParams( @@ message=message, ) - config = request_params.get("configuration", {}) - if "history_length" in config: - try: - history_length = self._parse_non_negative_history_length( - config["history_length"] - ) - except ValueError as exc: - from bindu.common.protocol.types import InvalidParamsError - raise InvalidParamsError(str(exc)) from exc - if history_length is not None: - scheduler_params["history_length"] = history_length + if history_length is not None: + scheduler_params["history_length"] = history_length🤖 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 135 - 158, The history_length validation must run before persisting a Task to avoid creating orphan/unscheduled tasks: move the config parsing block that calls self._parse_non_negative_history_length and raises InvalidParamsError (using InvalidParamsError from bindu.common.protocol.types) to run before calling self.storage.submit_task; construct/validate history_length from request_params.get("configuration", {}) first, then call self.context_id_parser and await self.storage.submit_task, and finally populate TaskSendParams (TaskSendParams, task["id"], scheduler_params) with the validated history_length if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bindu/server/handlers/message_handlers.py`:
- Around line 135-158: The history_length validation must run before persisting
a Task to avoid creating orphan/unscheduled tasks: move the config parsing block
that calls self._parse_non_negative_history_length and raises InvalidParamsError
(using InvalidParamsError from bindu.common.protocol.types) to run before
calling self.storage.submit_task; construct/validate history_length from
request_params.get("configuration", {}) first, then call self.context_id_parser
and await self.storage.submit_task, and finally populate TaskSendParams
(TaskSendParams, task["id"], scheduler_params) with the validated history_length
if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08ee825d-4fbc-46c6-9479-3b7ce76a78c7
📒 Files selected for processing (2)
bindu/server/handlers/message_handlers.pytests/unit/server/handlers/test_context_handlers.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/server/handlers/test_context_handlers.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
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