Add atomic batch publish headers and PubAck fields

[caspervonb]

Backport of ADR-50 atomic batch publish completion fields (PR #922 in nats-jetstream) so legacy users of allow_atomic/allow_batched streams can read the commit ack.

[synadia-claude-reviewer]

Claude finished @caspervonb's task in 1m 57s —— View job

---

PR Review

• Gather context (read changed files and diff)
• Review nats/src/nats/js/api.py changes
• Review nats/tests/test_js.py changes
• Post inline comments for any issues
• Write final review summary

---

Summary

The implementation is correct and the backport is sound. A few observations:

api.py

• The Header enum additions (BATCH_COMMIT, BATCH_ID, BATCH_SEQUENCE) correctly expose the ADR-50 wire headers for callers constructing batch-publish requests.
• PubAck.from_response correctly translates the server wire names (batch / count) to the Python API names (batch_id / batch_size). The guard conditions ("batch_id" not in resp) are safely defensive, though the server will never send both forms simultaneously.
• PubAck.as_dict correctly reverses the mapping. Note that PubAck is a receive-only type — it is never serialized and sent to the server in the current codebase — so this override is unused in production but harmless, and the round-trip tests are a good correctness check.
• One minor observation left as an inline comment: from_response mutates the caller's resp dict via pop(), which is consistent with existing base-class helpers but is a side effect worth being aware of. A shallow copy (resp = {**resp}) would avoid it.

test_js.py

• Unit tests cover the three relevant cases (fields present, fields absent, round-trip via as_dict). These are appropriate unittest.TestCase tests that don't require a live server, which is correct for pure data-transformation logic.
• There is no integration test exercising the full publish path against a real NATS server with allow_atomic=True. That is acceptable for a backport of a data-mapping feature, but an integration test would give stronger confidence that PubAck.batch_id and PubAck.batch_size are populated as expected end-to-end.

No blocking issues. The single inline comment is a suggestion, not a requirement.

[caspervonb]

@claude please review again

[synadia-claude-reviewer]

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

---

PR Review

• Gather context
• Read nats/src/nats/js/api.py changes
• Read nats/tests/test_js.py changes
• Analyze diff
• Post review findings

---

Summary

The implementation is correct and the backport is sound. No blocking issues.

---

nats/src/nats/js/api.py

Header enum (api.py:26-28)

The three new entries are alphabetically ordered before CONSUMER_STALLED and match the ADR-50 wire header names exactly. Correct.

PubAck fields (api.py:169-170)

batch_id: Optional[str] and batch_size: Optional[int] are appended after existing optional fields (domain, duplicate), which is the right placement for a backwards-compatible extension of a dataclass.

from_response (api.py:173-179)

The rename logic is correct. Base.from_response matches fields by name, so remapping batch → batch_id and count → batch_size before delegating is the right approach. The guard conditions ("batch_id" not in resp) are overly defensive (the server will never send both the old and new key names simultaneously), but they are harmless and future-proof.

Regarding the pop() mutation: the author's rationale for keeping it — consistency with the base-class helpers _convert and _convert_nanoseconds which all mutate in-place — is reasonable. Changing it here alone would be inconsistent.

as_dict (api.py:181-187)

The inverse mapping (batch_id → batch, batch_size → count) correctly reverses from_response. As previously noted, PubAck is receive-only in the current codebase so this override is exercised only in tests, but having it makes the object a correct round-trippable representation of the wire format.

---

nats/tests/test_js.py

Three unit tests in PubAckBatchTest:

Test	What it checks	Verdict
test_pub_ack_from_response_with_batch_fields	Wire names mapped to Python names	✓
test_pub_ack_from_response_without_batch_fields	Fields default to None when absent	✓
test_pub_ack_as_dict_maps_batch_fields	Reverse mapping and exclusion of Python names	✓

These are correct unittest.TestCase tests that don't require a live server, appropriate for pure data-transformation logic.

Not covered (acceptable for this scope):

• Partially-present batch fields (e.g., batch without count) — would be an invalid server response per ADR-50 anyway
• Integration test against a real allow_atomic=True stream — useful for end-to-end confidence but out of scope for a data-mapping backport

---

Verdict: LGTM. No changes required.