Python SDK: §6.4 mandatory leading ack

[renerocksai]

Summary

• Spec §6.4 was clarified: agents MUST emit exactly one {"type":"status","data":"ack"} chunk as the first message on the reply subject, before any response/query chunk and before any work that introduces observable latency. The ack resets the caller's §6.6 inactivity timeout ahead of any warm-up gap and makes the stream observable to generic NATS tooling (nats req --wait-for-empty).
• Implements the §6.4 invariant at a single chokepoint: AgentService._on_prompt_request now publishes the leading ack unconditionally after a successful envelope decode and before invoking the user handler. Every Python agent in the repo (reference agent, demo_echo, in-tree test handlers) becomes spec-compliant on upgrade with no per-agent code change.
• Leading ack is independent of keepalive_interval_s — passing None only disables the periodic keep-alive cadence. Malformed envelopes still produce error(400) → terminator with no spurious ack (ack lives after decode validation).

Changes

• agent-sdk: 5-line edit to service.py inserting the ack between decode and handler; one comment-block tweak documenting the relationship to keep-alive.
• agent-sdk tests: new test_leading_ack_e2e.py covers the wire-shape ordering and the no-ack-before-400 invariant. Existing tests rewritten for the new invariant — see commit-by-commit history.
• client-sdk: no functional change. 06-chat.py spinner trigger now waits for the first ResponseChunk/Query rather than the first chunk overall, so the "thinking…" spinner keeps spinning through the leading ack instead of stopping prematurely.
• docs: CHANGELOG.md entries on both packages + a 2026-05-11 alignment milestone in client-sdk/python/CLAUDE.md.

Pairing with TS

This is the Python half. The TS half is being landed in parallel by Mario (branch sdk-mandatory-initial-ack). The client side of either SDK already tolerates ack absence, so the cross-SDK interop test stays green throughout the rollout — there's a brief asymmetry on the wire while only one SDK has shipped, but no regression.

Test plan

• cd agent-sdk/python && uv run ruff check --no-cache .
• cd agent-sdk/python && uv run ruff format --check .
• cd agent-sdk/python && uv run mypy --no-incremental src tests examples
• cd agent-sdk/python && uv run pytest -v — 40 passed
• cd client-sdk/python && uv run ruff check --no-cache .
• cd client-sdk/python && uv run mypy --no-incremental src tests examples
• cd client-sdk/python && uv run pytest -v — 212 passed (including tests/test_interop_e2e.py against the unchanged TS reference agent — confirms the client receive side tolerates ack absence in the asymmetric window before the TS fix lands)
• Wire trace evidence: agent-sdk/python/tests/_evidence/tests__test_leading_ack_e2e.py__test_prompt_emits_leading_ack/frames.jsonl shows status=ack first, then the handler's response, then the §6.5 terminator.

🤖 Generated with Claude Code

[synadia-claude-reviewer]

Claude finished @renerocksai's task in 4m 8s —— View job

---

PR Review: Python SDK §6.4 mandatory leading ack

• Gather PR diff and context
• Review core implementation (service.py)
• Review new test file (test_leading_ack_e2e.py)
• Review updated existing tests
• Review client-side changes (06-chat.py)
• Check protocol compliance and cross-SDK wire compatibility
• Post inline comments and final summary

---

Summary

Implementation is correct and the approach is sound. The leading ack is placed at exactly the right point in _on_prompt_request — after decode validation (so malformed envelopes don't get a spurious ack) and before the keepalive task starts (so no ordering race). Error handling is consistent with the terminator pattern. Two findings below, both minor.

---

Finding 1 — test_malformed_envelope_emits_400_then_terminator not refactored to use _collect_replies

test_error_completion_e2e.py has two tests. The 500-path test was updated to use the shared _collect_replies helper (which correctly stops on the first empty-body/no-header frame). The 400-path test (test_malformed_envelope_emits_400_then_terminator, line 141) was not — it still uses inline polling with the old if len(collected) >= 2 and msg.data == b"" and not msg.headers: break guard on line 174.

This is not a bug (the 400 path is correctly 2 frames, so >= 2 is satisfied before the terminator), but it means the two tests in the same file use inconsistent collection logic. If the 400-path wire shape ever gains a frame prefix in a future spec revision, this guard would silently mask it (the loop would never stop on the terminator until 2+ frames have been collected, which would still be satisfied). Using _collect_replies would eliminate that fragility.

Finding 2 — AgentService docstring for keepalive_interval_s=None is incomplete

The class docstring (service.py line 211) reads:

Pass None to disable — for example when the handler emits its own status chunks at a finer cadence.

After this change, passing None does not suppress all acks — it only disables the periodic cadence. The mandatory §6.4 leading ack still fires unconditionally. An agent author who reads the class docstring (via help(AgentService) or an IDE hover) and passes None intending to send zero acks will be surprised.

The module-level comment near DEFAULT_KEEPALIVE_INTERVAL_S was updated correctly, but that's not surfaced to users. The class docstring should say something like: "Pass None to disable the periodic cadence — the §6.4 leading ack is emitted unconditionally regardless."

---

Everything else

• Ack placement: post-decode, pre-keepalive-task, pre-handler — exactly right.
• No keepalive race: _stop_keepalive awaits cancellation before respond_error is called, so the keepalive loop can't slip an extra ack between the error frame and the terminator.
• Malformed envelope path: ack correctly absent before a 400 error; test_malformed_prompt_no_ack_before_400 pins this invariant.
• 06-chat.py: first_visible_chunk flag correctly keeps the spinner alive through leading/keepalive acks; finally: thinking.stop() ensures cleanup on every exit path.
• Client SDK unchanged: StatusChunk was already yielded through Agent.prompt; the per-chunk inactivity timer resets naturally on the ack; no client-side behavioral change needed.
• Wire compatibility: protocol version stays at "0.3"; the leading ack is a valid StatusChunk shape the TS client already tolerates.

[synadia-claude-reviewer]

This comment correctly documents the invariant at the module level, but the class docstring for keepalive_interval_s (a few lines further down) says only "Pass None to disable" — without mentioning that the §6.4 leading ack still fires unconditionally. An agent author reading help(AgentService) or an IDE hover will get an incomplete picture. Worth adding a note there too, e.g. "Pass None to disable the periodic cadence; the §6.4 leading ack is emitted unconditionally regardless."

[renerocksai]

@claude, please review again — addressed both of your findings plus three more from a post-PR audit.

Your Finding 1 (consolidate _collect_replies): extended the helper to take an optional payload, refactored test_malformed_envelope_emits_400_then_terminator onto it. Both error-completion tests now use the same collection logic and stop on the first §6.5 terminator (no len >= 2 guard).

Your Finding 2 (AgentService docstring): updated the class docstring for keepalive_interval_s=None to make the leading-vs-periodic distinction explicit — agent authors reading help(AgentService) or an IDE hover now get the full story, not just the periodic-cadence-only story.

Three additional fixes from the audit:

• client-sdk/python/tests/test_interop_e2e.py::test_python_client_prompts_ts_reference_agent asserted isinstance(msg, ResponseChunk) on every chunk in the TS reference agent's stream. Passes today (TS reference agent hasn't been updated yet) but would fail the moment the parallel sdk-mandatory-initial-ack branch lands. Accept StatusChunk in the iterator and filter ResponseChunks for the content assertion.
• test_keepalive_emits_only_leading_ack_for_fast_handler counted acks but didn't assert the ack was the FIRST chunk under keep-alive-enabled. Pinned received[0] to mirror the keep-alive-disabled sibling so a regression that emits the ack AFTER a response chunk can't slip through the count assertion.
• Doc drift in client-sdk/python/docs/protocol-mapping.md (parenthetical "§6.4 status chunks are at the agent's discretion" — stale for the leading ack) and README.md (characterised ack as "for keep-alive" only). Both reworded to make the leading-vs-periodic split explicit.

Two commits on top of the original five:

• 35918de tests: tighten coverage + fix interop test for incoming TS leading ack
• 0620a23 docs: clarify leading vs periodic ack across user-facing surfaces

Pre-push gate green on both SDKs: ruff (no-cache) ✓ · ruff format ✓ · mypy (no-incremental) ✓ · pytest 40 ✓ (agent-sdk) · pytest 212 ✓ (client-sdk, incl. interop).

[synadia-claude-reviewer]

Claude finished @renerocksai's task in 1m 36s —— View job

---

Re-review: Python SDK §6.4 mandatory leading ack

• Read test_error_completion_e2e.py — verify Finding 1 fix (_collect_replies refactor)
• Read service.py — verify Finding 2 fix (docstring)
• Read test_interop_e2e.py — verify StatusChunk tolerance fix
• Read test_keepalive_e2e.py — verify first-chunk ordering assertion
• Read protocol-mapping.md and README.md — verify doc drift fixes

---

All five items are correctly resolved. Detailed notes follow.

Finding 1 — _collect_replies refactor ✅

_collect_replies now accepts payload: bytes = b"hi" (line 45–46 of test_error_completion_e2e.py) and test_malformed_envelope_emits_400_then_terminator calls it with payload=b'{"no_prompt":true}' (lines 171–176). Both error-completion tests now share the same terminator-detection logic — the old len >= 2 guard is gone. The helper's docstring is clear about the default value and why the 400 path wants a different payload.

Finding 2 — AgentService docstring ✅

Lines 210–216 of service.py now read:

Pass None to disable the periodic cadence — for example when the handler emits its own status chunks at a finer cadence. Note: the §6.4 leading ack (emitted before the handler runs) is mandatory and fires unconditionally regardless of this flag; None disables only the periodic mid-stream cadence, not the leading ack.

This is exactly the distinction that was missing. An agent author reading help(AgentService) now gets the full story.

Additional fix 1 — interop test StatusChunk tolerance ✅

test_python_client_prompts_ts_reference_agent now accepts ResponseChunk | StatusChunk in the iterator (line 207), filters ResponseChunk separately for the content assertion (line 215), and carries a clear comment explaining the rationale for the asymmetric-rollout window. The import on line 37 is correct. This is forward-compat done right.

Additional fix 2 — first-chunk ordering assertion in keepalive test ✅

test_keepalive_emits_only_leading_ack_for_fast_handler now pins received[0] (lines 319–328), mirroring the keepalive_disabled sibling. The comment explains exactly why a count assertion alone isn't enough — a regression that emits the ack after a response chunk would satisfy the count while breaking the spec invariant. Good catch.

Additional fix 3 — doc drift ✅

protocol-mapping.md row 7 (lines 253–255) now correctly separates the mandatory leading ack from the optional periodic cadence. README.md line 49 now reads "mandatory §6.4 leading chunk (and optionally again as periodic keep-alive)" — accurate, reader-friendly phrasing.

---

One minor observation (non-blocking)

test_malformed_prompt_no_ack_before_400 in test_leading_ack_e2e.py lines 209–213 checks:

decoded = decode_chunk(error_msg.data) if error_msg.data else None
assert not isinstance(decoded, StatusChunk) or decoded.status != "ack", ...

The assertion is logically correct — it's belt-and-suspenders given that len(frames) == 2 already guarantees no ack was emitted. In practice error_msg.data will be b"" or error JSON (not an ack payload), so decoded will be None and the assertion trivially passes. There's no bug here; the intent is clear from the failure message. I'd consider a direct assert decoded is None or not (isinstance(decoded, StatusChunk) and decoded.status == "ack") marginally easier to read, but this is stylistic and doesn't warrant a change.

---

Summary

All five addressed items are correctly implemented — both original findings and the three self-identified follow-ups. The PR is clean and ready to merge.

[renerocksai]

@claude, please review again — one more follow-up commit (554257f) addressing two items.

Latency budget on the leading ack (independent audit gap). test_prompt_emits_leading_ack now captures wall-clock latency between request publish and the first frame's arrival, asserts < 500 ms. The §6.4 rationale ("ack precedes the §6.6 warm-up gap") isn't enforced by wire ordering alone — a regression that wedged synchronous work between decode and the ack await would still produce ack-first ordering but defeat the spec invariant. 500 ms is generous (healthy local-NATS round-trip is ~5 ms here, so ~100× headroom) so CI flake risk is negligible. Evidence in tests/_evidence/.../ack-latency.json.

Your minor observation (test_malformed_prompt_no_ack_before_400 assertion readability). Pulled the De Morgan into a named is_leading_ack predicate and extended the comment to make explicit that the len(frames) == 2 + error-code-header pair already pins the §6.4 invariant — the explicit ack check is documentation-grade belt-and-braces.

Pre-push gate green on both SDKs: ruff (no-cache) ✓ · ruff format ✓ · mypy (no-incremental) ✓ · pytest 40 ✓ (agent-sdk) · pytest 212 ✓ (client-sdk).

[synadia-claude-reviewer]

Claude finished @renerocksai's task in 1m 27s —— View job

---

Re-review: Python SDK §6.4 mandatory leading ack (commit 554257f)

• Gather context
• Read test_leading_ack_e2e.py — verify latency budget assertion and is_leading_ack predicate refactor
• Check evidence structure for ack-latency
• Post review summary

---

Both items from the previous minor observation are correctly addressed. Notes below.

Latency budget — test_prompt_emits_leading_ack ✅

The test now inlines its own publish+drain loop to capture published_at and first_arrival, then asserts ack_latency_s < 0.5. The motivation is well-documented in both the commit message and the inline comment: a regression that wedges synchronous work between decode and the ack await would preserve ack-first wire ordering while defeating the §6.4 warm-up-gap invariant. The latency bound is the only guard for that case.

One micro-observation (non-blocking): the latency assertion appears after frame collection (lines 152–157 come after the while-loop that collects all three frames). The collection deadline is published_at + 2.0, so if the ack arrived at e.g. 1.5 s (well past budget), the remaining-frames deadline would be only 0.5 s away, and an incomplete frames list could cause a len(frames) == 3 failure before the latency assertion fires, producing a slightly confusing diagnostic. This scenario is purely hypothetical in a healthy environment — local NATS round-trip is ~1 ms — so there's no correctness concern. If it ever becomes a real diagnostic annoyance, hoisting the latency assertion immediately after first_arrival = asyncio.get_event_loop().time() (before collecting the remaining frames) would give a cleaner failure message. Not worth a change now.

Evidence written to ack-latency.json alongside frames.jsonl — the structure (published_at_s, first_arrival_s, ack_latency_s, budget_s) is self-explanatory and easy to parse in CI artifact reviews.

is_leading_ack predicate — test_malformed_prompt_no_ack_before_400 ✅

The De Morgan form is gone. The named predicate reads naturally: is_leading_ack = isinstance(decoded, StatusChunk) and decoded.status == "ack" followed by assert not is_leading_ack. The extended comment now explains the redundancy explicitly — len(frames) == 2 + the error-code header already pin the invariant; the explicit ack check is documentation-grade belt-and-braces. This is exactly the improvement the minor observation asked for.

---

Summary

The PR is complete. All findings (two original, three self-identified follow-ups, one minor observation) are correctly addressed across the nine commits. Implementation is spec-compliant, test coverage is thorough, and the latency budget assertion is a meaningful addition — not just belt-and-braces.

Ready to merge.

[renerocksai]

Alright, as signaled in OOB comms, I'm going to merge this on the Python side.

#100 also provides the §6.4 leading-ack fix, for both TypeScript and Python SDKs. After this PR has been merged, @M64GitHub amends #100 to drop all agent-sdk/python/** changes and keeps only the TypeScript side (agent-sdk/typescript/** + TS CHANGELOG). Zero TS overlap from this PR, so #100's TS work is unaffected.