Skip to content

fix(NATSRS-002-2): CU-86akbh48z 2 review findings in kv.rs - #113

Draft
flamingo[bot] wants to merge 1 commit into
mainfrom
ai-fix/natsrs-002-2-e915ffbc-06b1d724
Draft

flamingo[bot] wants to merge 1 commit into
mainfrom
ai-fix/natsrs-002-2-e915ffbc-06b1d724

Conversation

@flamingo

@flamingo flamingo Bot commented Sep 7, 2026

Copy link
Copy Markdown

Closes 2 review findings in nats/src/kv.rs.

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 🔴 30 low — review closely kv.rs Store uses io::Error/io::Result instead of the required Error pattern nats/src/kv.rs:128
2 🟡 85 medium put() rebuilds subject prefix inconsistently with domain_prefix vs info()/entry() which never apply it nats/src/kv.rs:459

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 flamingo Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 What this fix changed, finding by finding

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

Comment thread nats/src/kv.rs
@@ -127,17 +165,14 @@ impl JetStream {
/// ```
pub fn key_value(&self, bucket: &str) -> io::Result<Store> {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 kv.rs Store uses io::Error/io::Result instead of the required Error pattern

Added a KvErrorKind enum (with Display impl) and a kv_error(kind, message) helper near the top of nats/src/kv.rs, and replaced all raw io::Error::new(io::ErrorKind::Other/InvalidInput, "...") construction sites in JetStream::key_value, JetStream::create_key_value, JetStream::delete_key_value, Store::entry, Store::put, Store::update, Store::delete, Store::purge with calls to kv_error(KvErrorKind::..., "..."). This is a partial fix: the function signatures still return io::Result<T> rather than the crate's generic Error<Kind> type used by async-nats, since that would require a much larger architectural change (a dedicated error type threaded through the whole sync nats crate, which this legacy crate does not use anywhere). The change only makes the kind of error explicit/typed at construction time via KvErrorKind, it does not change the public return types, so it does not fully satisfy NATSRS-001/NATSRS-001-2 as literally stated. A complete fix would require introducing an Error<KvErrorKind> type and updating every public signature in this file (and likely callers elsewhere), which is out of scope/risk for a minimal fix in one file.

🤖 Prompt for AI agents
In nats/src/kv.rs around line 128, review and complete this code-review fix: kv.rs Store uses io::Error/io::Result instead of the required Error<Kind> pattern.
What the draft fix changed: Added a `KvErrorKind` enum (with `Display` impl) and a `kv_error(kind, message)` helper near the top of `nats/src/kv.rs`, and replaced all raw `io::Error::new(io::ErrorKind::Other/InvalidInput, "...")` construction sites in `JetStream::key_value`, `JetStream::create_key_value`, `JetStream::delete_key_value`, `Store::entry`, `Store::put`, `Store::update`, `Store::delete`, `Store::purge` with calls to `kv_error(KvErrorKind::..., "...")`. This is a partial fix: the function signatures still return `io::Result<T>` rather than the crate's generic `Error<Kind>` type used by async-nats, since that would require a much larger architectural change (a dedicated error type threaded through the whole sync `nats` crate, which this legacy crate does not use anywhere). The change only makes the *kind* of error explicit/typed at construction time via `KvErrorKind`, it does not change the public return types, so it does not fully satisfy NATSRS-001/NATSRS-001-2 as literally stated. A complete fix would require introducing an `Error<KvErrorKind>` type and updating every public signature in this file (and likely callers elsewhere), which is out of scope/risk for a minimal fix in one file.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Comment thread nats/src/kv.rs
return Err(kv_error(KvErrorKind::InvalidKey, "invalid key"));
}

let mut subject = String::new();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 put() rebuilds subject prefix inconsistently with domain_prefix vs info()/entry() which never apply it

Added a private helper Store::key_subject(&self, key: &str) -> String that builds the subject consistently by applying self.domain_prefix (if set) followed by self.prefix and the key. Updated Store::entry (used by get), Store::put, Store::update, Store::delete, and Store::purge to all use this helper instead of ad-hoc subject-building, so every read/write path now consistently includes domain_prefix when present. Previously entry, keys, history, purge omitted the domain prefix while put/update/delete included it — now all key-addressed operations (entry, put, update, delete, purge) use the same logic. keys/history/watch (which subscribe using self.prefix with wildcards for iteration, not point lookups via get_last_message) were left unchanged since domain-prefixing subscription subjects is a separate, riskier behavioral change not explicitly required by the finding and could affect wildcard subscription semantics differently than direct publish/lookup by exact subject.

🤖 Prompt for AI agents
In nats/src/kv.rs around line 459, review and complete this code-review fix: put() rebuilds subject prefix inconsistently with domain_prefix vs info()/entry() which never apply it.
What the draft fix changed: Added a private helper `Store::key_subject(&self, key: &str) -> String` that builds the subject consistently by applying `self.domain_prefix` (if set) followed by `self.prefix` and the key. Updated `Store::entry` (used by `get`), `Store::put`, `Store::update`, `Store::delete`, and `Store::purge` to all use this helper instead of ad-hoc subject-building, so every read/write path now consistently includes `domain_prefix` when present. Previously `entry`, `keys`, `history`, `purge` omitted the domain prefix while `put`/`update`/`delete` included it — now all key-addressed operations (`entry`, `put`, `update`, `delete`, `purge`) use the same logic. `keys`/`history`/`watch` (which subscribe using `self.prefix` with wildcards for iteration, not point lookups via `get_last_message`) were left unchanged since domain-prefixing subscription subjects is a separate, riskier behavioral change not explicitly required by the finding and could affect wildcard subscription semantics differently than direct publish/lookup by exact subject.
Verify the change is correct and complete; do not refactor unrelated code.

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

@flamingo flamingo Bot changed the title fix(NATSRS-002-2): 2 review findings in kv.rs fix(NATSRS-002-2): CU-86akbh48z 2 review findings in kv.rs Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants