fix: return stream ID when closing streamed activity

[Alex Bitar (AlxBit)]

Fixes #592

Summary

ctx.stream.close() now returns the original stream activity ID instead of DO_NOT_USE_PLACEHOLDER_ID when Teams returns an empty response for the final streaming request.

The stream already retains the ID returned by the initial streaming request. This change uses that retained ID for the final SentActivity, including the value emitted to on_close handlers and cached for repeated close() calls.

Tests

• Added regression coverage for a final streaming response with no ID.

• Verified that close() and the close event handler both receive the original
  stream ID.

• Run:

  uv run --python python3 pytest -q packages/apps/tests/test_http_stream.py

[Alex Bitar (AlxBit)]

@microsoft-github-policy-service agree

[Copilot]

🟡 Changes recommended

The new assert self._id is not None is unsafe for library runtime behavior (can be stripped with -O and lead to invalid id updates), and should be replaced with an explicit guard.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes HttpStream.close() to return the original streamed activity ID when the final streaming request gets an empty/placeholder response from Teams, aligning the SDK behavior with Teams’ streaming contract.

Changes:

• Overwrites the final SentActivity.id with the stream’s retained _id before emitting the close event and caching the result.
• Adds a regression test to ensure close() (and on_close handlers) receive the original stream ID when the final response has the placeholder ID.
• Strengthens an existing close/flush synchronization test to assert the returned SentActivity.id is the retained activity id.

File summaries

File	Description
packages/apps/src/microsoft_teams/apps/http_stream.py	Ensures close() returns a stable stream activity ID even when the final API response is represented with a placeholder ID.
packages/apps/tests/test_http_stream.py	Adds/updates tests to validate close() retains the initial stream ID and propagates it to close handlers.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new close() logic currently overwrites the returned activity ID unconditionally when _id is set, which can clobber a real final-response ID and should be gated to placeholder-only responses.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Corina (corinagum)]

Alex Bitar (@AlxBit) Thanks for filing a PR, and apologies for the delay in getting back to you! I think in the end this looks correct, but the fix is a layer too late and not fixing on_chunk.

The root cause is SentActivity.merge in sent_activity.py:21. {**activity_params.model_dump(), **curr_activity.model_dump()} runs, and because SentActivity.id is required and the api client fills it with DO_NOT_USE_PLACEHOLDER_ID on an empty body, the response id always clobbers the request id. Worth noting TS never hits this: send() there returns { ...payload, ...res }, so an empty res leaves payload.id intact.

Because the fix is in close(), the chunk path is unfixed. Chunks 2 - N also route through create_activity (the streaminfo entity forces the create branch), also get an empty body, and still emit the placeholder at http_stream.py:382. That isn't internal: app_process.py:226 forwards on_chunk into event_manager.on_activity_sent, so every plugin sees DO_NOT_USE_PLACEHOLDER_ID as the activity id for most of a stream.

Could you make the correction in _send instead?

if to_send.id:
    res = res.model_copy(update={"id": to_send.id})
return SentActivity.merge(to_send, res)

That covers the final activity, the chunk events, and any future caller in one place, and it reproduces the TS semantics rather than special-casing one call site. I tried it locally with the close() hunk reverted and the full apps suite passes.

Btw, the sentinel is hardcoded rather than imported, and the real one (_PLACEHOLDER_ACTIVITY_ID, activity.py:24) is private and unexported. LilyDu what are your thoughts? the value is "DO_NOT_USE_PLACEHOLDER_ID", I feel it's ok not to include a and res.id == PLACEHOLDER_ACTIVITY_ID: check.

Lastly, I think the code at test_http_stream.py:48 could use updating. it returns a fresh real id on every call, which which doesn't match the api client, so no unit tests would catch it. Could we make it return an id on the first call and the placeholder afterwards to reproduce the client's behavior? It makes this whole class of bug fail by default instead of needing a mock each time, and it makes the override in test_close_waits_for_flush_to_complete unneeded.

[Copilot]

🟢 Approval recommended

No unresolved blocking issues were identified.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite