Skip to content

fix(server): omit task artifacts from ListTasks unless asked - #447

Merged
muscariello merged 1 commit into
stack/07-protojson-required-fieldsfrom
stack/08-list-tasks-include-artifacts
Nov 2, 2026
Merged

muscariello merged 1 commit into
stack/07-protojson-required-fieldsfrom
stack/08-list-tasks-include-artifacts

Conversation

@muscariello

Copy link
Copy Markdown
Member

Fixes #417. Replaces #418. Stacked on #446: review only the last commit.

Cause: includeArtifacts reaches ListTasksRequest on every binding, but nothing read it, so ListTasks returned every stored artifact. §3.1.4 sets the default to false, and then artifacts MUST be omitted.

Fix: DefaultRequestHandler::list_tasks clears artifacts unless the request sets includeArtifacts: true, in the loop that already applies historyLength. Doing it in the handler gives every TaskStore, not only InMemoryTaskStore, the behavior the spec requires.

Behavior change: callers that relied on ListTasks returning artifacts now have to ask for them with includeArtifacts: true.

Tests: new test_list_tasks_includes_artifacts_only_on_request: a stored task with an artifact is listed with the flag unset, false and true.

Unproven: that the test fails on main was not run locally. CI is the check.

@codecov

codecov Bot commented Nov 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@muscariello
muscariello force-pushed the stack/08-list-tasks-include-artifacts branch 2 times, most recently from 6f7895e to 07fee30 Compare November 2, 2026 12:02
ListTasks returned every stored artifact whatever includeArtifacts said.
Section 3.1.4 of the spec makes the default false and says the artifacts
field MUST be omitted entirely then. DefaultRequestHandler::list_tasks now
drops them unless the request sets includeArtifacts to true, next to the
historyLength trim, so any TaskStore gets the same behaviour.

Signed-off-by: gulldan <5641375+gulldan@users.noreply.github.com>
@muscariello
muscariello force-pushed the stack/08-list-tasks-include-artifacts branch from 07fee30 to 7a822de Compare November 2, 2026 14:28
@muscariello
muscariello removed this pull request from stack #442 November 2, 2026 15:00
@muscariello
muscariello added this pull request to stack #456 November 2, 2026 15:01
@muscariello
muscariello added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit c48ab8b Nov 2, 2026
43 of 45 checks passed
@muscariello
muscariello deleted the stack/08-list-tasks-include-artifacts branch November 2, 2026 17:44
@github-actions github-actions Bot mentioned this pull request Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ListTasks returns artifacts although includeArtifacts defaults to false

3 participants