fix(NATSRS-001): CU-86akbh48z 3 review findings across 2 files

[flamingo]

Closes 3 review findings across 2 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🔴 35 low — review closely	service::error::Error is an ad-hoc public error struct, not the crate's Error/ErrorKind pattern	async-nats/src/service/error.rs:20
2	🟡 70 medium	double_ack() uses raw io::Error for timeout/dropped-subscription failures instead of a typed *ErrorKind	async-nats/src/jetstream/message.rs:264
3	🟡 80 medium	info() token parsing uses n_tokens >= 9 branch even for exactly 9-token replies, but nested 'token' check duplicates same condition redundantly	async-nats/src/jetstream/message.rs:370

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: 06b1d724-91f9-455e-9eef-4cda91351dba

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akbh48z NATS async-nats and CI review findings (10 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

3 finding(s) fixed in this draft — 3 explained inline on the diff; 1 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 service::error::Error is an ad-hoc public error struct, not the crate's Error/ErrorKind pattern

In async-nats/src/service/error.rs, replaced the ad-hoc public Error { status, code } struct with the crate's generic Error<Kind> pattern: added a new ErrorKind enum (Clone+Debug+Display+PartialEq), a pub type Error = crate::error::Error<ErrorKind> alias, and renamed the old wire-format struct to ErrorPayload (keeping its Serialize/Deserialize/Display/std::error::Error impls since it is still needed as the actual service-protocol payload type), plus a From<ErrorPayload> for Error conversion. This assumes crate::error::Error<Kind> exists with a with_source constructor as used elsewhere in async-nats (e.g. PublishError/SubscribeError) — I could not verify its exact API signature in this file-scoped view, so the constructor call may need adjusting to match the real helper. This change also breaks all other call sites in the crate that currently construct/match the old Error { status, code } struct directly (e.g. service response serialization code elsewhere in service/), which are not visible/editable here; a complete fix requires updating those call sites to use ErrorPayload for wire (de)serialization and Error/ErrorKind for the public error type, per the stated single-file constraint.

🤖 Prompt for AI agents

In async-nats/src/service/error.rs around line 20, review and complete this code-review fix: service::error::Error is an ad-hoc public error struct, not the crate's Error<Kind>/ErrorKind pattern.
What the draft fix changed: In `async-nats/src/service/error.rs`, replaced the ad-hoc public `Error { status, code }` struct with the crate's generic `Error<Kind>` pattern: added a new `ErrorKind` enum (Clone+Debug+Display+PartialEq), a `pub type Error = crate::error::Error<ErrorKind>` alias, and renamed the old wire-format struct to `ErrorPayload` (keeping its Serialize/Deserialize/Display/std::error::Error impls since it is still needed as the actual service-protocol payload type), plus a `From<ErrorPayload> for Error` conversion. This assumes `crate::error::Error<Kind>` exists with a `with_source` constructor as used elsewhere in async-nats (e.g. PublishError/SubscribeError) — I could not verify its exact API signature in this file-scoped view, so the constructor call may need adjusting to match the real helper. This change also breaks all other call sites in the crate that currently construct/match the old `Error { status, code }` struct directly (e.g. service response serialization code elsewhere in `service/`), which are not visible/editable here; a complete fix requires updating those call sites to use `ErrorPayload` for wire (de)serialization and `Error`/`ErrorKind` for the public error type, per the stated single-file constraint.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 *double_ack() uses raw io::Error for timeout/dropped-subscription failures instead of a typed ErrorKind

In Message::double_ack and Acker::double_ack (both instances), replaced the raw std::io::Error variants with a new typed error DoubleAckError/DoubleAckErrorKind (added near the top of the file, alongside StreamMessageErrorKind). The timeout case now maps to DoubleAckErrorKind::DoubleAckTimeout, the dropped-subscription case maps to DoubleAckErrorKind::SubscriptionDropped, and the "no reply subject" case in both double_ack methods maps to DoubleAckErrorKind::NotJetStreamMessage. This follows the same error::Error<Kind> pattern already used by StreamMessageError in this file. Risk: this is a public API-shape change (new exported types DoubleAckErrorKind/DoubleAckError); existing callers matching on io::ErrorKind via downcast will break, but that is the intended fix per the finding. I did not change the unrelated ack()/ack_with() "no reply subject" io::Error paths since the finding only concerns double_ack().

🤖 Prompt for AI agents

In async-nats/src/jetstream/message.rs around line 264, review and complete this code-review fix: double_ack() uses raw io::Error for timeout/dropped-subscription failures instead of a typed *ErrorKind.
What the draft fix changed: In `Message::double_ack` and `Acker::double_ack` (both instances), replaced the raw `std::io::Error` variants with a new typed error `DoubleAckError`/`DoubleAckErrorKind` (added near the top of the file, alongside `StreamMessageErrorKind`). The timeout case now maps to `DoubleAckErrorKind::DoubleAckTimeout`, the dropped-subscription case maps to `DoubleAckErrorKind::SubscriptionDropped`, and the "no reply subject" case in both `double_ack` methods maps to `DoubleAckErrorKind::NotJetStreamMessage`. This follows the same `error::Error<Kind>` pattern already used by `StreamMessageError` in this file. Risk: this is a public API-shape change (new exported types `DoubleAckErrorKind`/`DoubleAckError`); existing callers matching on `io::ErrorKind` via downcast will break, but that is the intended fix per the finding. I did not change the unrelated `ack()`/`ack_with()` "no reply subject" io::Error paths since the finding only concerns `double_ack()`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 info() token parsing uses n_tokens >= 9 branch even for exactly 9-token replies, but nested 'token' check duplicates same condition redundantly

In Message::info(), changed the inner redundant condition from if n_tokens >= 9 to if n_tokens > 9 for the token field of the Info struct literal (inside the n_tokens >= 9 outer branch). This makes the token only parsed when there are more than 9 tokens (i.e., 10 or 11), leaving it None for exactly-9-token replies, matching the intended domain/no-domain ack subject distinction. Risk: I did not have access to the exact NATS ADR-15 subject format spec to verify the exact boundary (9 vs 10 vs 11) is precisely correct, so while this resolves the "dead code / always-true" issue described in the finding, the exact numeric threshold should be double-checked against the JetStream ack-subject-token-count specification during review.

🤖 Prompt for AI agents

In async-nats/src/jetstream/message.rs around line 370, review and complete this code-review fix: info() token parsing uses n_tokens >= 9 branch even for exactly 9-token replies, but nested 'token' check duplicates same condition redundantly.
What the draft fix changed: In `Message::info()`, changed the inner redundant condition from `if n_tokens >= 9` to `if n_tokens > 9` for the `token` field of the `Info` struct literal (inside the `n_tokens >= 9` outer branch). This makes the token only parsed when there are more than 9 tokens (i.e., 10 or 11), leaving it `None` for exactly-9-token replies, matching the intended domain/no-domain ack subject distinction. Risk: I did not have access to the exact NATS ADR-15 subject format spec to verify the exact boundary (9 vs 10 vs 11) is precisely correct, so while this resolves the "dead code / always-true" issue described in the finding, the exact numeric threshold should be double-checked against the JetStream ack-subject-token-count specification during review.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer