Skip to content

fix: raise ValueError on append to nonexistent artifact #1040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
naveenkumarbaskaran wants to merge 3 commits into from
+12 −7
Closed

fix: raise ValueError on append to nonexistent artifact #1040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a72442
fix: raise ValueError on append to nonexistent artifact
naveenkumarbaskaran May 2, 2026
209da6c
Merge branch 'main' into fix/raise-on-append-unknown-artifact
sokoliva May 26, 2026
7c1abdb
Merge branch 'main' into fix/raise-on-append-unknown-artifact
sokoliva May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/a2a/server/tasks/task_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,14 @@ def append_artifact_to_task(task: Task, event: TaskArtifactUpdateEvent) -> None:
dict(new_artifact_data.metadata.items())
)
else:
# We received a chunk to append, but we don't have an existing artifact.
# we will ignore this chunk
logger.warning(
'Received append=True for nonexistent artifact index %s in task %s. Ignoring chunk.',
artifact_id,
task.id,
# We received a chunk to append, but there is no existing artifact
# with this id. Silently dropping the chunk would hide a real bug
# in the caller (e.g. generating a fresh artifact_id on every
# add_artifact call instead of pinning one), so we raise.
Comment on lines +77 to +80
Copy link
Copy Markdown
Member

@sokoliva sokoliva May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider reframing this description. Future readers will have no context of the chunk being dropped so mentioning it seems redundant. Let's just describe current behaviour without mentioning the prior.

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.'
)
Comment on lines +81 to 85
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 InvalidParamsError
References
  1. Raise exception classes directly, without instantiating them (e.g., raise MyError instead of raise MyError()).

Copy link
Copy Markdown
Member

@sokoliva sokoliva May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naveenkumarbaskaran



Expand Down
5 changes: 4 additions & 1 deletion tests/server/tasks/test_task_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ def test_append_artifact_to_task():
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)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

Update the comment to reflect the change to InvalidParamsError.

Suggested change
# raises ValueError instead of silently dropping the chunk (#1038)
# raises InvalidParamsError instead of silently dropping the chunk (#1038)

Copy link
Copy Markdown
Member

@sokoliva sokoliva May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naveenkumarbaskaran

non_existing_artifact_with_parts = Artifact(
artifact_id='artifact-456', parts=[Part(text='Part 1')]
)
Expand All @@ -438,7 +439,9 @@ def test_append_artifact_to_task():
task_id='123',
context_id='123',
)
append_artifact_to_task(task, append_event_5)
with pytest.raises(ValueError, match='append=True but no artifact with id'):
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

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.

Suggested change
with pytest.raises(ValueError, match='append=True but no artifact with id'):
with pytest.raises(InvalidParamsError):
References
  1. Raise exception classes directly, without instantiating them (e.g., raise MyError instead of raise MyError()).

Copy link
Copy Markdown
Member

@sokoliva sokoliva May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naveenkumarbaskaran

append_artifact_to_task(task, append_event_5)
# Artifacts unchanged — the bad chunk was not silently added
assert len(task.artifacts) == 2
assert len(task.artifacts[0].parts) == 2
assert len(task.artifacts[1].parts) == 1
Loading