fix: enforce upper bound on history_length - #1177
Conversation
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/events/event_queue_v2.py | 91.79% | 91.28% | 🔴 -0.51% |
| src/a2a/utils/task.py | 95.16% | 95.59% | 🟢 +0.43% |
| src/a2a/utils/telemetry.py | 91.47% | 90.70% | 🔴 -0.78% |
| Total | 93.00% | 92.98% | 🔴 -0.02% |
Generated by coverage-comment.yml
mykytanetipa
left a comment
There was a problem hiding this comment.
The history_length cap is a reasonable idea, but it's a public-facing behavioral change, so it should be reviewed and accepted by the A2A spec maintainers first. Please open a spec proposal PR at https://github.com/a2aproject/A2A before implementing it in the SDK.
| @@ -25,9 +36,13 @@ def HasField(self, field_name: Literal['history_length']) -> bool: # noqa: N802 | |||
|
|
|||
|
|
|||
| def validate_history_length(config: HistoryLengthConfig | None) -> None: | |||
There was a problem hiding this comment.
The PR description states that right now server "allowing an unbounded-history read via an absurdly large parameter", but the change into validate_history_length doesn't affect this, full task history will still be loaded in memory.
example on_get_task:
# param validation happens here:
validate_history_length(params)
...
# task is loaded with full history here:
task: Task | None = await self.task_store.get(task_id, context)
...
# history cap is applied here:
return apply_history_length(task, params)
| from a2a.utils.errors import InvalidParamsError | ||
|
|
||
|
|
||
| MAX_HISTORY_LENGTH = 1000 |
There was a problem hiding this comment.
I'd avoid setting MAX_HISTORY_LENGTH just as a global constant. If this change lands it should be configurable, opt-in, with None default and set by some server config
What changed
1. Enforce an upper bound on
historyLengthProblem:
validate_history_length()insrc/a2a/utils/task.pyonly rejected negative values. A client could sendhistoryLength=999999999and the server would materialize the full task history for every matching task (intasks/get,tasks/list, andmessage/send), allowing an unbounded-history read via an absurdly large parameter.Fix (src/a2a/utils/task.py):
MAX_HISTORY_LENGTH = 1000with a comment explaining the cap is a pragmatic bound aligned with the other A2A SDKs' "large value ≈ return everything available" semantics.validate_history_length()now raisesInvalidParamsErrorwhenhistory_length > MAX_HISTORY_LENGTH(in addition to the existing negative-value check). The validation already runs at the entry ofon_get_task,on_list_tasks, andon_message_send(V1) and their V2 counterparts, so the cap applies to all JSON-RPC / REST / gRPC paths without further changes.Testing
./.venv/Scripts/python -m pytest tests/utils/test_task.py tests/server/request_handlers/test_default_request_handler.py tests/server/request_handlers/test_default_request_handler_v2.py -q→ 150 passed (includes 8 new regression tests:validate_history_lengthunit tests plus handler-levelhistory_lengthover-limit rejection foron_get_taskandon_message_send)../.venv/Scripts/python -m ruff checkon modified files: clean.historyLengthabove 1000 now returnInvalidParamsErrorinstead of returning full history. Requests at or below the cap are unaffected (existing tests all use values ≤ 10).