fix: raise ValueError on append to nonexistent artifact#1040
fix: raise ValueError on append to nonexistent artifact#1040naveenkumarbaskaran wants to merge 1 commit intoa2aproject:mainfrom
Conversation
When `append_artifact_to_task()` receives `append=True` for an `artifact_id` that doesn't exist on the task, it previously logged a warning and silently dropped the chunk. This hid real bugs — callers saw a successful response with no content. Now raises `ValueError` with a clear message pointing at the bad call, making the misuse impossible to miss. Updated the existing test to assert the new ValueError behavior. Closes a2aproject#1038
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the task manager to raise a ValueError when attempting to append data to a non-existent artifact, replacing the previous behavior of silently logging a warning. The test suite has been updated to verify this new error-handling logic. The review feedback recommends using the project-specific InvalidParamsError instead of the built-in ValueError to ensure correct protocol mapping. Furthermore, the feedback highlights a repository rule requiring exception classes to be raised directly without instantiation, which requires adjustments to the implementation and the corresponding test assertions.
| raise ValueError( | ||
| f'append=True but no artifact with id {artifact_id!r} exists on ' | ||
| f'task {task.id!r}. Create the artifact first (append=False) ' | ||
| f'before appending to it.' | ||
| ) |
There was a problem hiding this comment.
Consider using InvalidParamsError instead of the built-in ValueError. Since append_artifact_to_task is part of the task event processing pipeline, using the project's custom A2AError subclasses ensures that the error is correctly caught and mapped to a structured protocol response. Note that per repository rules, exception classes must be raised directly without instantiation.
raise InvalidParamsErrorReferences
- Raise exception classes directly, without instantiating them (e.g., raise MyError instead of raise MyError()).
| assert len(task.artifacts[1].parts) == 1 | ||
|
|
||
| # Test appending part to a task that does not have a matching artifact | ||
| # raises ValueError instead of silently dropping the chunk (#1038) |
There was a problem hiding this comment.
Update the comment to reflect the change to InvalidParamsError.
| # raises ValueError instead of silently dropping the chunk (#1038) | |
| # raises InvalidParamsError instead of silently dropping the chunk (#1038) |
| context_id='123', | ||
| ) | ||
| append_artifact_to_task(task, append_event_5) | ||
| with pytest.raises(ValueError, match='append=True but no artifact with id'): |
There was a problem hiding this comment.
Update the test to assert InvalidParamsError instead of ValueError to align with the implementation change. Note that the exception is raised without a message per repository rules.
| with pytest.raises(ValueError, match='append=True but no artifact with id'): | |
| with pytest.raises(InvalidParamsError): |
References
- Raise exception classes directly, without instantiating them (e.g., raise MyError instead of raise MyError()).
🧪 Code Coverage (vs
|
1 participant
Summary
When
append_artifact_to_task()receivesappend=Truefor anartifact_idthat doesn't exist on the task, it previously logged a warning and silently dropped the chunk. This hid real bugs — callers saw a successful response with no content.Now raises
ValueErrorwith a clear message pointing at the bad call.Closes #1038
Changes
task_manager.py: Replacelogger.warning+ silent ignore withValueErrorcontaining a descriptive messagetest_task_manager.py: Updated existing test to assert the newValueErrorbehaviorRationale
As described in #1038, the silent drop is a footgun for any server-side executor that doesn't already know the
artifact_id-pinning pattern. This is Option 1 from the issue — the minimal/safest change. Loud failure beats silent drop.Testing
Updated the existing
test_append_artifact_to_tasktest case — the "append to nonexistent artifact" path now assertspytest.raises(ValueError)and verifies artifacts remain unchanged.