Skip to content

fix(NATSRS-004): CU-86akbgq0z 3 review findings in object_store.rs - #166

Draft
flamingo[bot] wants to merge 1 commit into
mainfrom
ai-fix/natsrs-004-5c1a96a9-015e5738
Draft

flamingo[bot] wants to merge 1 commit into
mainfrom
ai-fix/natsrs-004-5c1a96a9-015e5738

Conversation

@flamingo

@flamingo flamingo Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Closes 3 review findings in nats/src/object_store.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 🟢 90 high object_store::Config.max_age Duration field lacks serde_nanos adapter nats/src/object_store.rs:60
2 🟡 85 medium object_store::Config.description Option missing skip_serializing_if guard nats/src/object_store.rs:60
3 🔴 55 low — review closely ObjectStore::put swallows the real error when checking for an existing object nats/src/object_store.rs:436

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: 015e5738-6626-4eeb-8abb-b9341fb8eb33

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-86akbgq0z NATS server TcpStream panic fixes (11 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

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

Comment thread nats/src/object_store.rs
Comment on lines 61 to 70
/// Name of the storage bucket.
pub bucket: String,
/// A short description of the purpose of this storage bucket.
#[serde(skip_serializing_if = "Option::is_none")]
pub description: Option<String>,
/// Maximum age of any value in the bucket, expressed in nanoseconds
#[serde(with = "serde_nanos")]
pub max_age: Duration,
/// The type of storage backend, `File` (default) and `Memory`
pub storage: StorageType,

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.

🦩 🔴 object_store::Config.max_age Duration field lacks serde_nanos adapter

Added #[serde(with = "serde_nanos")] to Config.max_age in the Config struct (near line 60), matching the wire format used elsewhere (e.g. StreamConfig::max_age). serde_nanos is already an external crate dependency pattern used in this codebase's wire types; assuming it's available as a dependency in nats crate (consistent with how other Duration fields like StreamConfig's are expected to be handled). If the crate is not already a Cargo dependency, Cargo.toml must add serde_nanos — this cannot be verified from the single file shown, so flatten risk to that dependency existing.

🤖 Prompt for AI agents
In nats/src/object_store.rs around line 60, review and complete this code-review fix: object_store::Config.max_age Duration field lacks serde_nanos adapter.
What the draft fix changed: Added `#[serde(with = "serde_nanos")]` to `Config.max_age` in the `Config` struct (near line 60), matching the wire format used elsewhere (e.g. `StreamConfig::max_age`). `serde_nanos` is already an external crate dependency pattern used in this codebase's wire types; assuming it's available as a dependency in `nats` crate (consistent with how other Duration fields like StreamConfig's are expected to be handled). If the crate is not already a Cargo dependency, `Cargo.toml` must add `serde_nanos` — this cannot be verified from the single file shown, so flatten risk to that dependency existing.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Comment thread nats/src/object_store.rs
Comment on lines 61 to 70
/// Name of the storage bucket.
pub bucket: String,
/// A short description of the purpose of this storage bucket.
#[serde(skip_serializing_if = "Option::is_none")]
pub description: Option<String>,
/// Maximum age of any value in the bucket, expressed in nanoseconds
#[serde(with = "serde_nanos")]
pub max_age: Duration,
/// The type of storage backend, `File` (default) and `Memory`
pub storage: StorageType,

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.

🦩 🔵 object_store::Config.description Option missing skip_serializing_if guard

Added #[serde(skip_serializing_if = "Option::is_none")] to description field on Config, ObjectInfo, and ObjectMeta structs, consistent with the suggested fix and the finding's note that ObjectInfo.description and ObjectMeta.description have the same issue.

🤖 Prompt for AI agents
In nats/src/object_store.rs around line 60, review and complete this code-review fix: object_store::Config.description Option<String> missing skip_serializing_if guard.
What the draft fix changed: Added `#[serde(skip_serializing_if = "Option::is_none")]` to `description` field on `Config`, `ObjectInfo`, and `ObjectMeta` structs, consistent with the suggested fix and the finding's note that `ObjectInfo.description` and `ObjectMeta.description` have the same issue.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment thread nats/src/object_store.rs
// A "not found" condition is treated as "no prior object"; any other
// error is propagated since it may indicate a real I/O or server
// failure rather than a missing object.
let maybe_existing_object_info = match self.info(&object_name) {

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.

🦩 🔵 ObjectStore::put swallows the real error when checking for an existing object

In ObjectStore::put, changed the match on self.info(&object_name) to only treat ErrorKind::NotFound as "no existing object", propagating all other errors via return Err(err). This relies on self.info() surfacing a NotFound io::ErrorKind for missing objects (via get_last_message presumably returning such an error) — this behavior is not verified from the given file alone, so if the underlying JetStream API returns a different error kind for "no message found", this fix would incorrectly propagate what should be treated as "object does not exist" and break put for first-time uploads. A complete fix would require confirming the exact error kind returned by get_last_message/stream_info for a missing message.

🤖 Prompt for AI agents
In nats/src/object_store.rs around line 436, review and complete this code-review fix: ObjectStore::put swallows the real error when checking for an existing object.
What the draft fix changed: In `ObjectStore::put`, changed the match on `self.info(&object_name)` to only treat `ErrorKind::NotFound` as "no existing object", propagating all other errors via `return Err(err)`. This relies on `self.info()` surfacing a `NotFound` io::ErrorKind for missing objects (via `get_last_message` presumably returning such an error) — this behavior is not verified from the given file alone, so if the underlying JetStream API returns a different error kind for "no message found", this fix would incorrectly propagate what should be treated as "object does not exist" and break `put` for first-time uploads. A complete fix would require confirming the exact error kind returned by `get_last_message`/`stream_info` for a missing message.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

@flamingo flamingo Bot changed the title fix(NATSRS-004): 3 review findings in object_store.rs fix(NATSRS-004): CU-86akbgq0z 3 review findings in object_store.rs Oct 12, 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