Skip to content

Tolerate empty context IDs when updating A2A session state - #606

Merged
Quim Muntal (qmuntal) merged 2 commits into
microsoft:mainfrom
PratikDhanaveFork:tolerate-empty-context-id
Jul 27, 2026
Merged

Tolerate empty context IDs when updating A2A session state#606
Quim Muntal (qmuntal) merged 2 commits into
microsoft:mainfrom
PratikDhanaveFork:tolerate-empty-context-id

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

updateSessionContextID in provider/a2aprovider/a2a.go reconciles the A2A response's context ID with the one stored in the session. Two problems when the incoming context ID is empty:

  1. The mismatch guard (currentContextID != "" && currentContextID != contextID) fires whenever the session already holds a context ID and the incoming one is empty, returning mismatched context ID and erroring the whole run.
  2. Even past the guard, setContextID unconditionally overwrote the stored context ID with the empty string.

A bare streamed Message legitimately carries an empty ContextID (sendMsg reads taskInfo.ContextID), so both paths are reachable in normal streaming.

This is fixed by adding contextID != "" to the mismatch condition and adding an early if contextID == "" { return } guard inside setContextID, so empty streamed values neither error nor clobber the stored context ID.

Why

This aligns with .NET, where the context ID is only assigned when currently unset (ContextId ??= contextId), and restores internal consistency: setTaskID already guards if taskID == "" { return }, while setContextID did not. The asymmetry is what let the empty case slip through.

Testing

Added two black-box tests in a2a_test.go driven through the existing streaming harness (extended the shared mock with a rawStreamingResponse flag so a bare message's empty context ID is not backfilled from the request):

  • TestRunStreamingWithEmptyContextIDKeepsSessionContext: session holds ctx-1, a streamed message with empty ContextID produces no error and leaves ServiceID() as ctx-1. Fails before the fix (mismatched context ID: session has "ctx-1" but A2A response has ""), passes after.
  • TestRunStreamingWithEmptyInitialContextStoresResponseContext: session starts with no context ID, first event carries ctx-1, which is stored.

go build ./..., go vet ./provider/a2aprovider/..., and go test ./provider/a2aprovider/... all pass.

Copilot AI review requested due to automatic review settings July 23, 2026 05:34
@PratikDhanave
PratikDhanave (PratikDhanave) requested a review from a team as a code owner July 23, 2026 05:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the A2A provider’s session reconciliation logic to treat empty streamed ContextID values as “no update” (instead of an error or a clobber), matching the expected streaming behavior and aligning with the existing setTaskID empty-guard pattern.

Changes:

  • Prevent updateSessionContextID from erroring on context mismatches when the incoming contextID is empty.
  • Prevent setContextID from overwriting an existing session context ID with an empty string.
  • Add streaming-focused black-box tests that cover empty-context streamed messages and initial context assignment, via an extended transport mock.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
provider/a2aprovider/session.go Adds an early return so empty context IDs do not clobber the session’s stored context.
provider/a2aprovider/a2a.go Updates the mismatch guard to ignore empty incoming context IDs during session reconciliation.
provider/a2aprovider/a2a_test.go Extends the streaming transport mock and adds tests validating empty-context streaming behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread provider/a2aprovider/a2a_test.go
@PratikDhanave
PratikDhanave (PratikDhanave) force-pushed the tolerate-empty-context-id branch 2 times, most recently from d1bfa91 to 32d8a9f Compare July 23, 2026 15:43
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 23, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

A bare streamed message can carry an empty ContextID. The mismatch guard
in updateSessionContextID fired whenever the session already held a
context ID and the incoming one was empty, spuriously erroring the whole
run, and setContextID then clobbered the stored value with the empty
string. Skip the mismatch check when the incoming context ID is empty and
avoid overwriting a stored context ID with an empty value, matching the
.NET semantics where ContextId is only assigned when unset
(ContextId ??= contextId) and mirroring the existing empty-guard in
setTaskID.
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review: ✅ Approved

This PR makes internal corrections to updateSessionContextID and setContextID in provider/a2aprovider/ — both are unexported helpers with no public API surface change.

Parity assessment: This fix improves alignment with the upstream .NET implementation. As noted in the PR description, the .NET SDK uses ContextId ??= contextId (null-coalescing assignment, only stores when currently unset), so an empty incoming contextID must neither error nor overwrite an existing stored value. The Go fix reproduces exactly that semantic:

  • The mismatch guard gains contextID != "" so a bare streamed Message with no context ID never triggers the error.
  • setContextID gains an early-return guard when contextID == "", mirroring the existing setTaskID guard and the .NET null-coalescing behaviour.

No exported API changesupdateSessionContextID, setContextID, and setTaskID are all unexported. TaskIDsFromSession (the only exported symbol in session.go) is unchanged.

The public-api-change label is not needed. parity-approved label is appropriate and retained.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 57 AIC · ⌖ 5.57 AIC · ⊞ 5.9K ·

@qmuntal
Quim Muntal (qmuntal) added this pull request to the merge queue Jul 27, 2026
Merged via the queue into microsoft:main with commit 89d1550 Jul 27, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants