Skip to content

feat(api): align basin info with stream info#338

Merged
shikhar merged 21 commits intomainfrom
codex/basin-info-alignment
Mar 20, 2026
Merged

feat(api): align basin info with stream info#338
shikhar merged 21 commits intomainfrom
codex/basin-info-alignment

Conversation

@shikhar
Copy link
Copy Markdown
Member

@shikhar shikhar commented Mar 20, 2026

Align basin lifecycle metadata with stream info while keeping the deprecated basin state field as a compatibility-only JSON field.

  • add created_at and deleted_at to basin info in the common, API, SDK, and lite layers
  • derive basin deletion state from deleted_at and stop publishing state in OpenAPI
  • update CLI and TUI basin presentation to follow the stream info patterns
  • tolerate missing basin created_at during deserialize by defaulting it to now_utc()

@shikhar shikhar marked this pull request as ready for review March 20, 2026 18:09
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 20, 2026

Greptile Summary

This PR replaces the single state: BasinState enum field on BasinInfo with explicit created_at and deleted_at timestamps across the common, API, SDK, and CLI/TUI layers, aligning basin lifecycle metadata with the existing StreamInfo shape. The deprecated state field is preserved as a compatibility-only JSON field (hidden from OpenAPI) and is re-derived from deleted_at during serialization.

Key changes:

  • common::types::BasinInfo drops BasinState and gains created_at: OffsetDateTime / deleted_at: Option<OffsetDateTime>.
  • api::v1::BasinInfo splits Serialize (derives state from deleted_at) from a hand-rolled Deserialize that tolerates missing timestamps by falling back to OffsetDateTime::now_utc() for backward compatibility with older servers.
  • sdk::BasinInfo switches from infallible From to TryFrom for the API→SDK conversion, consistent with StreamInfo.
  • The CLI auto-paginate paths for both basins and streams now pass .with_include_deleted(true), surfacing deleting resources with a red [deleting] badge.
  • BasinState is fully removed from the common and SDK layers; all deletion checks are replaced with deleted_at.is_some().
  • Tests are updated to assert on created_at/deleted_at fields rather than BasinState variants.

Confidence Score: 4/5

  • Safe to merge with minor caveats around misleading fallback timestamps and a small SDK breaking change.
  • The structural changes are well-scoped and consistent across all layers. The main concerns are (1) created_at/deleted_at defaulting to now_utc() during deserialization for older servers — intentional but will surface inaccurate timestamps in the CLI/TUI during a mixed-version rollout — and (2) BasinInfo silently losing the Eq trait, which is a minor semver-breaking change for SDK users. Neither is a correctness bug, but both are worth acknowledging before the release.
  • api/src/v1/basin.rs (custom deserializer with now_utc() fallbacks) and sdk/src/types.rs (removal of Eq from BasinInfo).

Important Files Changed

Filename Overview
api/src/v1/basin.rs Core change: adds created_at and deleted_at to BasinInfo, introduces a hand-rolled custom Deserialize that defaults missing timestamps to now_utc() for backward compat. The deprecated state field is kept in serialized output but hidden from OpenAPI. The now_utc() fallback is intentional but will show inaccurate timestamps when talking to older servers.
sdk/src/types.rs Removes BasinState enum and its From impl, replaces state: BasinState with created_at: S2DateTime and deleted_at: Option<S2DateTime> on BasinInfo. Drops Eq derive to match S2DateTime's PartialEq-only bound — a minor breaking change for downstream users that relies on BasinInfo: Eq.
lite/src/backend/basins.rs Propagates the created_at/deleted_at fields from stored metadata directly into BasinInfo, eliminating the intermediate BasinState derivation. Straightforward translation with no logic changes.
cli/src/ops.rs Both list_basins and list_streams auto-paginate paths now explicitly pass .with_include_deleted(true), showing deleted resources in listings. The no_auto_paginate paths do not set include_deleted explicitly, though current server behaviour makes this consistent. Minor asymmetry worth watching if server behaviour ever changes.
sdk/src/ops.rs Switches BasinInfo conversions from infallible Into to TryInto (propagating ValidationError on invalid timestamps) and replaces BasinState::Deleting check with deleted_at.is_some(). Clean and correct.

Sequence Diagram

sequenceDiagram
    participant CLI
    participant SDK
    participant API Layer
    participant Server

    CLI->>SDK: list_all_basins(include_deleted=true)
    SDK->>API Layer: list_basins(ListBasinsRequest)
    API Layer->>Server: HTTP GET /basins
    Server-->>API Layer: JSON { name, scope, created_at, deleted_at, state(deprecated) }
    Note over API Layer: Custom Deserialize:<br/>missing created_at → now_utc()<br/>state:deleting + no deleted_at → now_utc()
    API Layer-->>SDK: BasinInfo { created_at, deleted_at }
    Note over SDK: TryFrom: OffsetDateTime → S2DateTime<br/>filter: include_deleted=true → yield all
    SDK-->>CLI: Stream<BasinInfo>
    Note over CLI: print_listing_uri(name, deleted_at.is_some())<br/>print_listing_with_created_at for streams
Loading

Comments Outside Diff (1)

  1. cli/src/ops.rs, line 47-63 (link)

    P2 no_auto_paginate path does not explicitly request deleted basins

    When no_auto_paginate = true, the code constructs a plain ListBasinsInput::new() (and equivalently ListStreamsInput::new() for streams) without setting include_deleted. The auto-paginate path explicitly calls .with_include_deleted(true).

    In practice, both paths see deleted items because the server always returns them and the include_deleted flag is a client-side filter inside list_all_basins. However, if the server ever starts honouring an include_deleted query parameter, the no_auto_paginate path would silently stop showing deleted basins/streams, creating an inconsistency between the two modes.

    Consider aligning the single-page path by making the intent explicit, e.g., keeping a note or sharing the same flag source.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: cli/src/ops.rs
    Line: 47-63
    
    Comment:
    **`no_auto_paginate` path does not explicitly request deleted basins**
    
    When `no_auto_paginate = true`, the code constructs a plain `ListBasinsInput::new()` (and equivalently `ListStreamsInput::new()` for streams) without setting `include_deleted`. The auto-paginate path explicitly calls `.with_include_deleted(true)`.
    
    In practice, both paths see deleted items because the server always returns them and the `include_deleted` flag is a client-side filter inside `list_all_basins`. However, if the server ever starts honouring an `include_deleted` query parameter, the `no_auto_paginate` path would silently stop showing deleted basins/streams, creating an inconsistency between the two modes.
    
    Consider aligning the single-page path by making the intent explicit, e.g., keeping a note or sharing the same flag source.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: api/src/v1/basin.rs
Line: 113-116

Comment:
**Inaccurate fallback timestamps may mislead users**

Both `created_at` and `deleted_at` are set to `OffsetDateTime::now_utc()` when missing from older API responses. While this maintains backward compatibility, it means:

- A basin created weeks ago against an older server will appear to have been created "just now" in the CLI/TUI.
- A basin in `state: deleting` without a real `deleted_at` will show the current time as its deletion time, not the actual time deletion was requested.

These inaccuracies will be visible to users upgrading their SDK before their server is updated. Consider logging a warning when falling back to `now_utc()`, or surfacing this via an `Option<S2DateTime>` to let callers know the value is synthetic.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: sdk/src/types.rs
Line: 1124

Comment:
**Removing `Eq` is a breaking change for SDK users**

`BasinInfo` previously derived both `PartialEq` and `Eq`. Dropping `Eq` (because `S2DateTime` only derives `PartialEq`) is a semver-breaking change for any downstream code that required `BasinInfo: Eq` — e.g., using it as a value in `HashSet`, or in a generic bound `T: Eq`.

This is now consistent with `StreamInfo` (which was already `PartialEq`-only), so the alignment is good. However, since `time::OffsetDateTime` itself implements `Eq`, it would be trivial to add `Eq` to `S2DateTime`:

```rust
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct S2DateTime(time::OffsetDateTime);
```

This would restore `Eq` for both `BasinInfo` and `StreamInfo` and avoid a silent breaking change.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: cli/src/ops.rs
Line: 47-63

Comment:
**`no_auto_paginate` path does not explicitly request deleted basins**

When `no_auto_paginate = true`, the code constructs a plain `ListBasinsInput::new()` (and equivalently `ListStreamsInput::new()` for streams) without setting `include_deleted`. The auto-paginate path explicitly calls `.with_include_deleted(true)`.

In practice, both paths see deleted items because the server always returns them and the `include_deleted` flag is a client-side filter inside `list_all_basins`. However, if the server ever starts honouring an `include_deleted` query parameter, the `no_auto_paginate` path would silently stop showing deleted basins/streams, creating an inconsistency between the two modes.

Consider aligning the single-page path by making the intent explicit, e.g., keeping a note or sharing the same flag source.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "chore(diff): trim li..."

Comment on lines +113 to +116
let created_at = created_at.unwrap_or_else(OffsetDateTime::now_utc);
let deleted_at = match (deleted_at, state) {
(Some(deleted_at), _) => Some(deleted_at),
(None, Some(BasinState::Deleting)) => Some(OffsetDateTime::now_utc()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Inaccurate fallback timestamps may mislead users

Both created_at and deleted_at are set to OffsetDateTime::now_utc() when missing from older API responses. While this maintains backward compatibility, it means:

  • A basin created weeks ago against an older server will appear to have been created "just now" in the CLI/TUI.
  • A basin in state: deleting without a real deleted_at will show the current time as its deletion time, not the actual time deletion was requested.

These inaccuracies will be visible to users upgrading their SDK before their server is updated. Consider logging a warning when falling back to now_utc(), or surfacing this via an Option<S2DateTime> to let callers know the value is synthetic.

Prompt To Fix With AI
This is a comment left during a code review.
Path: api/src/v1/basin.rs
Line: 113-116

Comment:
**Inaccurate fallback timestamps may mislead users**

Both `created_at` and `deleted_at` are set to `OffsetDateTime::now_utc()` when missing from older API responses. While this maintains backward compatibility, it means:

- A basin created weeks ago against an older server will appear to have been created "just now" in the CLI/TUI.
- A basin in `state: deleting` without a real `deleted_at` will show the current time as its deletion time, not the actual time deletion was requested.

These inaccuracies will be visible to users upgrading their SDK before their server is updated. Consider logging a warning when falling back to `now_utc()`, or surfacing this via an `Option<S2DateTime>` to let callers know the value is synthetic.

How can I resolve this? If you propose a fix, please make it concise.

@shikhar shikhar merged commit 836db71 into main Mar 20, 2026
16 checks passed
@shikhar shikhar deleted the codex/basin-info-alignment branch March 20, 2026 20:59
@s2-release-plz s2-release-plz bot mentioned this pull request Mar 20, 2026
shikhar pushed a commit that referenced this pull request Mar 20, 2026
## 🤖 New release

* `s2-common`: 0.29.0 -> 0.30.0 (⚠ API breaking changes)
* `s2-api`: 0.27.12 -> 0.27.13 (✓ API compatible changes)
* `s2-lite`: 0.29.25 -> 0.29.26 (✓ API compatible changes)
* `s2-sdk`: 0.25.0 -> 0.26.0 (⚠ API breaking changes)
* `s2-cli`: 0.29.25 -> 0.29.26

### ⚠ `s2-common` breaking changes

```text
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field BasinInfo.created_at in /tmp/.tmpC8ehRw/s2/common/src/types/basin.rs:213
  field BasinInfo.deleted_at in /tmp/.tmpC8ehRw/s2/common/src/types/basin.rs:214

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/enum_missing.ron

Failed in:
  enum s2_common::types::basin::BasinState, previously in file /tmp/.tmpFp6uyJ/s2-common/src/types/basin.rs:203

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field state of struct BasinInfo, previously in file /tmp/.tmpFp6uyJ/s2-common/src/types/basin.rs:218
```

### ⚠ `s2-sdk` breaking changes

```text
--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/enum_missing.ron

Failed in:
  enum s2_sdk::types::BasinState, previously in file /tmp/.tmpFp6uyJ/s2-sdk/src/types.rs:1126

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field state of struct BasinInfo, previously in file /tmp/.tmpFp6uyJ/s2-sdk/src/types.rs:1151
```

<details><summary><i><b>Changelog</b></i></summary><p>

## `s2-common`

<blockquote>

## [0.30.0] - 2026-03-20

### Features

- Align basin info with stream info
([#338](#338))

<!-- generated by git-cliff -->
</blockquote>

## `s2-api`

<blockquote>

## [0.27.13] - 2026-03-20

### Features

- Align basin info with stream info
([#338](#338))

<!-- generated by git-cliff -->
</blockquote>

## `s2-lite`

<blockquote>

## [0.29.26] - 2026-03-20

### Features

- Align basin info with stream info
([#338](#338))

<!-- generated by git-cliff -->
</blockquote>

## `s2-sdk`

<blockquote>

## [0.26.0] - 2026-03-20

### Features

- Align basin info with stream info
([#338](#338))

<!-- generated by git-cliff -->
</blockquote>

## `s2-cli`

<blockquote>

## [0.29.26] - 2026-03-20

### Features

- Align basin info with stream info
([#338](#338))

### Miscellaneous Tasks

- Dep updates ([#340](#340))

<!-- generated by git-cliff -->
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: s2-release-plz[bot] <262023388+s2-release-plz[bot]@users.noreply.github.com>
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.

1 participant