feat(a2a): includeArtifacts on GetTask for lean status polls

[arcaputo3]

What

GetTask always returns the task's full artifacts array. The A2A spec defines an artifact switch only on ListTasks (include_artifacts, default exclude), so a client polling task status has no way to skip multi-MB inline base64 artifact payloads — on TJC's email-hook reconcile path that is every in-flight task, every 60s (tjc-agents TJC-1962/TJC-1967).

Contract

• A2ARequest.TasksGet gains includeArtifacts: Option[Boolean] = None. Absent or true = include — the opposite default of ListTasks, so every existing caller and the spec-shaped GetTask response are unchanged; false trims the artifacts array after applyHistoryLength. Read projection only — the stored task keeps its artifacts.
• JSON codec emits/reads includeArtifacts/include_artifacts; REST binding parses the query param on GET /tasks/{id}; proto GetTaskRequest gains optional bool include_artifacts = 4 (a local extension, same precedent as tenant).
• Parity suite updated: GetTaskRequest sample carries the field; REST query allowlist includes it.

Tests

./mill agent.test green on both platforms (326/326 targets), including A2AProtoParitySpec exact-set equality and a new A2AServerCoreSpec case: absent/true keep artifacts, false trims, stored task unaffected.

Release intent: tag v0.13.1 after merge (version literal bumped per repo convention); tjc-agents repins via RuntimeVersions.Scalagent.

🤖 Generated with Claude Code

[claude]

Review: includeArtifacts on GetTask

Nice, tightly-scoped change. The important calls all look right to me:

• Default direction is correct. Absent/true = include keeps tasks/get spec-shaped and every existing caller byte-identical, while ListTasks keeps its spec default of exclude. Encoding the divergence deliberately (rather than "consistency" for its own sake) is the right trade.
• Read projection only — taskStore.load → reconcileOrphaned → applyHistoryLength → trim, with the stored task untouched, and the new test asserts exactly that.
• The wire result really is lean: A2ATask.toJsonObject (A2ATask.scala:58) omits artifacts entirely when Nil, so includeArtifacts=false drops the key rather than emitting "artifacts": [] — matching the convention the spec doc states for excluded artifacts (test/resources/specification.md:240).
• Parity suite kept honest: because protoRestQueryFields derives the allowlist from the vendored proto, the extension had to be added in both places to stay green — the guard did its job.
• Field placement (before tenant) mirrors TasksList, and a positional caller passing a tenant string now gets a type error rather than a silent misbind. Good.

Findings below, roughly by value.

1. gRPC lean polls are untested, and the mechanism is more fragile than it looks

The gRPC path decodes proto → ProtoJSON → shared codec (A2AGrpcProtoCodec.decodeRequest, src/jvm/.../A2AGrpcProtoCodec.scala:50-53). include_artifacts=false survives that round trip only because the field is declared optional — proto3 explicit presence is what makes JsonFormat.printer() emit a field whose value equals the type default. Declare it non-optional, or re-vendor from an upstream proto that lacks the local extension, and setIncludeArtifacts(false) silently becomes include — a quiet regression straight back to multi-MB polls, with no test failing.

A2AGrpcProtoCodecSpec:13-29 already builds a GetTaskRequest with setHistoryLength(2). One line there pins the fragile case:

.setIncludeArtifacts(false)
...
assertEquals(request.includeArtifacts, Some(false))

false (not true) is the value worth asserting, precisely because it is the one presence-tracking protects.

2. No transport-level coverage of the payload win

The new test asserts trimmed.artifacts == Nil on the handler. What the motivating caller actually observes is the serialized body, which depends on the toJsonObject behavior above. Worth adding:

• GET /tasks/{id}?include_artifacts=false in A2AServerLiveSpec (and/or A2ARestTransportSpec), asserting the response JSON has no "artifacts" key while the default request does. Those specs already exercise include_artifacts parse failures for /tasks, so the shape is there.
• A JSON-RPC tasks/get case with params.includeArtifacts = false.
• A codec round-trip for TasksGet with snake_case include_artifacts and with null, mirroring the TasksList cases at A2ACodecSpec:1841 / :1150. The parity suite covers encoding field names, not decoding the alias on this message.

3. Trimming logic now duplicated with inverted defaults

A2AServerTypes.scala:464-468 (list) and A2ARequestHandler.scala:139-140 (get) do the same projection with opposite defaults — a natural spot for future drift. Consider a shared helper alongside applyHistoryLength, keeping the defaults explicit at the call sites:

def project(task: A2ATask, historyLength: Option[Int], includeArtifacts: Boolean): A2ATask
// get:  project(task, params.historyLength, params.includeArtifacts.getOrElse(true))
// list: project(task, params.historyLength, params.includeArtifacts.getOrElse(false))

Relatedly, params.includeArtifacts.contains(false) reads a bit obliquely for a defaulted flag; !params.includeArtifacts.getOrElse(true) states the contract in the code rather than only in the comment above it.

4. The switch is server-side only — the in-repo client cannot use it

A2AClient.getTask (src/js/.../A2AClient.scala:50,176) builds TasksGet with named args and no includeArtifacts, and A2AClientPolling.awaitTask / sendAndPoll take getTask: (TaskId, Option[Int]) => Task[A2ATask] — a poll loop over tasks/get is exactly the lean-poll use case, and it cannot opt in. If the motivating reconcile path goes through A2AClient, this PR does not reach it without hand-rolling the request.

Threading an includeArtifacts: Option[Boolean] = None through those signatures would be additive and behavior-preserving. Two caveats worth capturing if you do: the A2AV03 / JS-SDK path cannot carry the field (JsBuilders.taskQueryParams takes only history length), and a lean awaitTask must not be treated as returning authoritative final artifacts.

5. Documentation lives where users will not look

• The inverted default is documented in a handler comment and the proto comment, but not on TasksGet.includeArtifacts itself — the scaladoc on the case class field is what an API consumer sees.
• README's A2A Execution section enumerates the surface (getTask, listTasks, resubscribe, config knobs); one line noting GetTask accepts includeArtifacts, default include, opposite of ListTasks, would land where someone goes looking.
• proto/A2A_PROTO_SOURCE.txt records only the upstream commit — the local deltas (tenant, now include_artifacts = 4) are not listed, so a re-vendor drops them. The parity suite will catch it, but a two-line delta list turns a puzzling failure into an obvious fix. Also worth a thought: field 4 in GetTaskRequest collides if upstream ever adds one there. A high tag (or a documented reserved range for local extensions) sidesteps that wire-compat trap — tenant = 1 set the precedent, but it set it in the risky direction.

6. Excluded vs. empty are indistinguishable on the wire

Since the key is omitted either way, a client that caches or merges GetTask responses can clobber known artifacts with none. Inherent to the design and already true of ListTasks, but a sentence in the field doc would save someone a debugging session.

7. Version bump: 0.12.0 → 0.13.1 skips 0.13.0

If 0.13.0 was cut from another branch, ignore this. If not, an additive-field change would normally be 0.12.1 (or 0.13.0). Separately, README keeps per-release ### X Notes sections and the newest is 0.11.0 — 0.12.x/0.13.x are already missing, so if you are tagging 0.13.1 this is a decent moment to add a short entry recording the local extension.

8. Minor: the store still pays for the artifacts

Trimming happens after taskStore.load, so a durable backend continues to read and deserialize the full inline base64 on every poll — the wire saving is real, the store-I/O saving is not. Fine if the bottleneck is egress and client parsing; if that 60s reconcile path turns out to be store-bound, a projection hook on load (load(id, tenant, includeArtifacts)) is the follow-up.

---

Nothing above blocks merge in my view — #1 and #2 are the ones I would want before tagging a release, since both failure modes are silent.

[claude]

Review: includeArtifacts on GetTask

Nicely scoped change, and the hard parts are right: the trim happens after reconcileOrphaned and applyHistoryLength, so nothing lossy can be persisted (and the new test asserts exactly that); the proto field is optional bool, which is what makes false survive JsonFormat.printer() on the gRPC path (a non-optional field would be dropped as a proto3 default and silently disable the feature); and the parity suite was updated in lockstep across all three transports. The inverted default vs. ListTasks is well justified in both the proto and handler comments — the kind of note that saves the next reader.

A few things worth addressing, roughly in priority order.

1. Codec coverage — the repo already has the exact slots, they just weren't extended (medium)

The new field's encode/decode behavior is currently pinned only indirectly, via field-name-set equality in A2AProtoParitySpec. A2ACodecSpec has three ListTasks-shaped tests that should grow a TasksGet case:

• test("request encoders omit absent optional fields and preserve optional false presence") (~A2ACodecSpec.scala:1055) covers TasksList(includeArtifacts = Some(false)) but not TasksGet. Some(false) surviving the encoder is the feature — the invariant most worth locking.
• test("request decoders accept ProtoJSON original field names") (~:1827) asserts include_artifacts for TasksList only. The new snake_case alias on the TasksGet decoder has no direct coverage.
• The round-trip list at :1043 uses TasksGet(taskId, historyLength = Some(1), tenant = ...) — adding includeArtifacts = Some(false) is a one-word change.

Also A2AGrpcProtoCodecSpec.scala:13 ("decodes upstream binary GetTaskRequest…") builds setTenant/setId/setHistoryLength; adding .setIncludeArtifacts(false) and asserting Some(false) exercises the only path that actually goes proto-binary → JsonPrinter → shared decoder. Since the Java classes are regenerated from the vendored proto at build time, that test is what catches a future edit dropping optional and turning false into "field absent → include artifacts" — a silent inversion of the requested behavior.

2. No in-repo client can send the flag (medium, completeness)

Server-side this is wired on JSON-RPC, REST and gRPC, but nothing in scalagent can request it:

• A2AClient.getTask(taskId, historyLength) — src/js/com/tjclp/scalagent/a2a/A2AClient.scala:50 / :176
• A2AClientPolling.awaitTask / sendAndPoll — src/shared/com/tjclp/scalagent/a2a/A2AClientPolling.scala:36

That polling loop is precisely the "client polling task status every 60s" the PR motivates, and it can't opt out of artifacts. The natural completion is getTask(taskId, historyLength, includeArtifacts) plus awaitTask passing Some(false) while non-terminal and re-fetching with artifacts once task.isStreamEnding. If tjc-agents hits REST directly this is a follow-up rather than a blocker — but worth an explicit note/issue, otherwise the switch is invisible to scalagent's own consumers. (A2AV03 can stay as-is; JsBuilders.taskQueryParams is bounded by the upstream JS SDK.)

3. Suppressed vs. genuinely-empty is indistinguishable on the wire (low)

A2ATask.toJsonObject defaults includeEmptyArtifacts = false (A2ATask.scala:52-58), so includeArtifacts=false yields a response with no artifacts key — byte-identical to a task that simply has none. ListTasks deliberately avoids this by threading the flag into includeEmptyArtifacts (A2AResponse.scala:90); GetTask has no equivalent signal. Benign for the immediate caller, which knows what it asked for, but any pass-through/caching layer that persists a GetTask result would silently drop artifacts. Either mirror the ListTasks signal, or state it in the field's doc comment: suppressed artifacts are omitted, not emitted as an empty array — never write a suppressed GetTask result back to a store.

4. Artifact projection now lives in two places with opposite defaults (low, reuse)

A2AServerTypes.scala:464-468 (ListTasks, default exclude) and A2ARequestHandler.scala:139-140 (GetTask, default include). applyHistoryLength was deliberately made public on A2ATaskStore "so durable A2ATaskStore impls can stay byte-identical with the in-memory default's projection" — the same argument applies to artifacts. An A2ATaskStore.applyArtifactProjection(task, includeArtifacts, default) would keep both defaults in one auditable spot and give durable stores something to reuse.

5. Performance: the savings are serialization + network only (worth stating)

taskStore.load still materializes the full task including inline base64 artifacts; the trim happens after. On a durable store the read and heap allocation for those multi-MB payloads are unchanged — only encode + transfer are saved. That's likely the dominant cost for the 60s reconcile poll, so this is still a clear win, but if artifact blobs are expensive to fetch, the remaining win needs a store-level projection — another argument for #4's shared helper pushing the flag down into the store API.

6. Smaller notes

• sharedPublishVersion jumps 0.12.0 → 0.13.1, skipping 0.13.0, and uses a patch-level tag for an additive feature. Intentional (is 0.13.0 already published off another branch)? Otherwise 0.13.0 looks like the right tag.
• if params.includeArtifacts.contains(false) then trimmed.copy(artifacts = Nil) else trimmed is correct but reads negatively for a flag that defaults to include. if params.includeArtifacts.getOrElse(true) then trimmed else trimmed.copy(artifacts = Nil) states the default in the code and mirrors the ListTasks style (getOrElse(false)). Cosmetic — identical behavior.
• proto/A2A_PROTO_SOURCE.txt records the upstream commit but not the local deltas (tenant, now include_artifacts). Following its own instruction — "to track a new upstream proto, replace the file" — would silently drop both. The parity spec would fail loudly, which is good, but a two-line "local deltas" list in that file would make the re-vendor obvious instead of archaeological.
• end TasksGet is insertEndMarkerMinLines = 20 doing its job, not noise. 👍

Security

No new exposure: authorizeRequest and validateHistoryLength still run first, and the projection only removes data. query.bool rejects anything but true/false with invalidParams, consistent with the ListTasks parsing. If anything it mildly reduces the blast radius of an over-broad status poll.

Not a blocker overall — #1 is the one I'd want before merge, since it's a handful of one-line additions to tests that already exist and it guards the wire-level presence semantics the whole feature rests on.

🤖 Generated with Claude Code

[arcaputo3]

Addressed the review in 28f7cf9. What changed:

Adopted (both reviews' pre-merge asks):

• Optional-false presence pinned on every layer it can silently break — the gRPC binary decode test now sets include_artifacts=false and asserts Some(false) (the case optional protects: without it, JsonPrinter drops the proto3 default and a lean poll silently becomes a full one); A2ACodecSpec covers the TasksGet encoder's optional-false presence, the include_artifacts snake_case decode alias, and the round-trip sample; A2AServerLiveSpec gains a transport test asserting REST ?include_artifacts=false and JSON-RPC includeArtifacts:false both return bodies with no artifacts key while the default GET keeps it.
• Shared projection helper: A2ATaskStore.applyArtifactProjection now serves both handlers with the defaults stated at the call sites (getOrElse(true) for GetTask, getOrElse(false) for ListTasks) — no more duplicated trim with inverted defaults, and durable stores have something to reuse.
• Docs where consumers look: TasksGet scaladoc states the default direction and the omitted-not-empty caveat (never write a trimmed result back to a store); README gains a 0.13.1 notes entry + a line in A2A Execution; proto/A2A_PROTO_SOURCE.txt now lists the local deltas (tenant, include_artifacts = 4) a re-vendor must re-apply.

Deferred to #70 (out of this PR's scope): threading includeArtifacts through A2AClient.getTask / A2AClientPolling (with the lean-poll-then-full-fetch-on-terminal contract), and store-level projection so durable stores skip loading artifact bytes entirely. The motivating consumer (tjc-agents email hook) speaks JSON-RPC directly, so neither blocks the release.

Kept as-is, with reasoning:

• Field tag 4: staying. The parity suite fails loudly on a re-vendor, A2A_PROTO_SOURCE.txt now documents the delta, and if upstream ever adds an artifact switch to GetTaskRequest at tag 4 with matching semantics we get to drop the delta entirely; a high tag would forfeit that convergence.
• Version 0.13.1: v0.13.0 is already tagged and published — the build.mill literal was stale at 0.12.0 (it is only the local fallback; the tag drives PUBLISH_VERSION). 0.13.1 for an additive optional field follows the 0.x line's convention.

Full suite green on both platforms (326/326; A2AServerLiveSpec 31→32, A2AGrpcProtoCodecSpec 4→5, A2ACodecSpec 74→77 assertions across the touched tests).

🤖 Generated with Claude Code

[claude]

Review: includeArtifacts on GetTask

Solid, well-scoped change. The design decision that matters most — absent/true = include, so the spec-shaped GetTask response and every existing caller are unchanged — is the right call, and it's documented in three places a future reader will actually hit (TasksGet scaladoc, proto/A2A_PROTO_SOURCE.txt, README). A few things I liked specifically:

• Ordering is correct where it counts. reconcileOrphaned runs before the projection (A2ARequestHandler.scala:138-143), so the orphan self-heal still persists the full task; the trim is genuinely read-only. The A2AServerCoreSpec assertion on stored pins that.
• applyArtifactProjection de-duplicating the ListTasks branch (A2AServerTypes.scala:475-483) with the default stated at each call site rather than baked into the helper — right place to put the asymmetry.
• The proto optional comment in A2AGrpcProtoCodecSpec.scala:19-21 is the best test in the PR: it names the exact failure mode (drop the modifier → JsonPrinter elides the proto3 default → lean poll silently becomes full) that a re-vendor would otherwise reintroduce silently.

Notes below, roughly by value. Nothing I found is a correctness bug in the server path.

---

1. The feature is server-only — the client that motivated it can't send the flag

A2AClient.getTask is fixed at (TaskId, Option[Int]):

• src/js/com/tjclp/scalagent/a2a/A2AClient.scala:50 (trait) and :176-180 (impl, constructs TasksGet with no includeArtifacts)
• A2AClientPolling.awaitTask / sendAndPoll take getTask: (TaskId, Option[Int]) => Task[A2ATask] (A2AClientPolling.scala:24, :62)

The stated motivation is a poller — the email-hook reconcile path hitting every in-flight task every 60s. As shipped, a scalagent-based poller has to bypass A2AClient and hand-roll REST/JSON-RPC to get the benefit. Suggestion, mirroring how listTasks already takes full params:

def getTask(params: A2ARequest.TasksGet): Task[A2ATask]

That's additive for callers and avoids widening the existing 2-arg signature. And awaitTask is where the win compounds: poll lean, then do one full getTask once isStreamEnding — which also sidesteps hazard (3) below, since the task the caller finally receives is always complete. Fine as a follow-up if tjc-agents goes over the wire directly today, but worth saying so in the PR body, because "GetTask supports lean polls" reads as though client.getTask does.

2. The saving is wire-size only, not store I/O

taskStore.load returns the full task and the projection happens after (A2ARequestHandler.scala:136-143). For the in-memory store that's free, but for a durable store the multi-MB base64 artifacts are still read out of the backing store and JSON-decoded on every poll — exactly the cost profile the PR is trying to shed on a 60s loop. The response payload and client-side parse do shrink, which is real. Worth either stating that boundary in the README note, or (later) letting A2ATaskStore expose a projected load so implementations can push the projection down into the query.

3. Explicit includeArtifacts=true on a task with no artifacts is indistinguishable from a trim

The PR docs handle this honestly ("indistinguishable from a task that has none... must never be written back to a store"), but the codebase already has machinery to remove the ambiguity in the explicit case: A2ATask.toJsonObject(task, includeEmptyArtifacts = true) emits "artifacts":[], which is how ListTasks satisfies test/resources/specification.md:240 ("When includeArtifacts is true, the artifacts field should be included... which may be an empty array"). ListTasks threads it through ListTasksResult.includeArtifacts (A2AResponse.scala:90).

So after this PR the two endpoints disagree: ListTasks(includeArtifacts=true) on an artifact-less task emits "artifacts":[], GetTask(includeArtifacts=true) omits the key. Absent stays omitted either way, so nothing spec-shaped changes. Low priority — plumbing the flag into the renderer for a bare A2ATask is more surface than the ambiguity costs — but it's the one asymmetry a client author would trip over, and it's the principled fix for the "don't write it back" warning.

4. Versioning: patch bump for a case-class field insertion, and a skipped literal

build.mill goes 0.12.0 → 0.13.1, skipping 0.13.0 in the literal, and README gains a ### 0.13.1 Notes section with no 0.12.0/0.13.0 sections above it. If 0.13.0 shipped via PUBLISH_VERSION without touching the literal that's just a bookkeeping gap, but it leaves the README changelog non-contiguous — worth a line either way. (The install snippets at README.md:196-207 still say 0.11.0; pre-existing, but this is the version-bump PR.)

On bump level: includeArtifacts is inserted before tenant in TasksGet (A2ARequest.scala:153-157), which is binary-incompatible for a published artifact and breaks positional construction downstream. The compile-time risk is mild — TasksGet(id, Some(3), Some("tenant")) fails with a type error rather than silently rebinding, and the only silent shift is TasksGet(id, None, None), where both readings mean the same thing. Still, that's minor-bump-shaped, not patch-shaped. Either bump to 0.14.0, or append the field after tenant (at the cost of the tenant-last convention).

5. Proto tag 4 for a local extension

A2A_PROTO_SOURCE.txt already flags the collision ("tag collides if upstream ever adds a field 4"), and the vendored proto is already divergent — tenant = 1 shifted upstream's numbering in every request message — so the delta list is the real mitigation and it now exists. That said, since local extensions are apparently permanent here, a reserved high range (include_artifacts = 1001 plus reserved) would make future re-vendors mechanical instead of a judgment call. Not worth churning this PR for; worth deciding before the next local field.

6. Test coverage — good layering, three small gaps

Codec / routing / proto-parity / core-handler / JVM-wire is a thorough spread, and I like that the live spec covers ?include_artifacts=false (snake) on REST and includeArtifacts (camel) on JSON-RPC, and asserts the key is absent rather than []. Gaps:

• No malformed-boolean test for GetTask. ListTasks has one (A2ARestTransportSpec.scala:893-924 → "includeArtifacts must be a valid boolean"); GET /tasks/{id}?includeArtifacts=maybe now takes the same query.bool path (A2APathRouting.scala:42-49) and returns invalidParams, but nothing pins it.
• The trim isn't asserted to be artifacts-only. A2AServerCoreSpec checks artifacts on all three responses but not that status/history survive; an over-broad projection would pass. One assertEquals(trimmed.history, defaulted.history) closes it.
• The end-to-end wire test is JVM-only. A2APathRouting is shared and tested, so the JS server path is covered structurally — just noting the asymmetry with A2ARestTransportSpec, where equivalent JS coverage would live.

No security concerns: authorizeRequest still runs first, the flag only narrows what's returned, and a bad value is rejected as invalidParams rather than silently defaulting.

---

I read the diff and the surrounding A2A modules; I did not run ./mill agent.test in this environment, so I'm taking the 326/326 claim at face value. None of the above blocks merge — (1) is the one I'd want resolved (or explicitly deferred in the PR body) before calling the feature done, since the motivating consumer is a client.