feat!: add validation for AppendSessionConfig

[quettabit]

No description provided.

[quettabit]

@greptileai review this draft

[greptile-apps]

Greptile Summary

Added validation to AppendSessionConfig builder methods to enforce minimum constraints: max_inflight_bytes must be at least 1 MiB and max_inflight_batches must be at least 1. The builder methods now return Result<Self, ValidationError> instead of Self, which is a breaking API change (correctly marked with feat!:).

Key changes:

• Added ONE_MIB constant (1048576 bytes) for clearer validation logic
• Updated with_max_inflight_bytes() to validate minimum 1 MiB requirement
• Updated with_max_inflight_batches() to validate minimum value of 1
• Updated documentation to reflect these constraints
• Consistent with existing validation pattern used in BatchingConfig

Note: As discussed in the previous thread, validation can still be bypassed by directly mutating the public fields after construction, but this is an intentional design choice by the maintainers.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The validation logic is correct and straightforward, checking for minimum values before construction. The breaking change is appropriately marked with feat!: in the commit message. The implementation follows existing patterns in the codebase (similar to BatchingConfig). No logical errors or edge cases detected. The previous thread's concern about public field mutation is acknowledged as an intentional design choice.
• No files require special attention

Important Files Changed

Filename	Overview
src/session/append.rs	adds validation to builder methods for minimum 1 MiB bytes and minimum 1 batch, changes return type to Result, consistent with existing patterns

Sequence Diagram

sequenceDiagram
    participant User
    participant AppendSessionConfig
    participant ValidationError
    participant AppendSession

    User->>AppendSessionConfig: new()
    AppendSessionConfig-->>User: config (with defaults)
    
    User->>AppendSessionConfig: with_max_inflight_bytes(bytes)
    alt bytes < 1 MiB
        AppendSessionConfig->>ValidationError: create error
        ValidationError-->>User: Err(ValidationError)
    else bytes >= 1 MiB
        AppendSessionConfig-->>User: Ok(config)
    end
    
    User->>AppendSessionConfig: with_max_inflight_batches(batches)
    alt batches < 1
        AppendSessionConfig->>ValidationError: create error
        ValidationError-->>User: Err(ValidationError)
    else batches >= 1
        AppendSessionConfig-->>User: Ok(config)
    end
    
    User->>AppendSession: new(client, stream, config)
    AppendSession-->>User: session

Loading

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

logic: validation can be bypassed by directly mutating public fields

the fields max_inflight_bytes and max_inflight_batches are public, allowing users to bypass validation after construction:

let mut config = AppendSessionConfig::new();
config.max_inflight_bytes = 100; // bypasses 1 MiB minimum!
config.max_inflight_batches = Some(0); // bypasses minimum of 1!

consider making these fields private and adding getter methods, or add validation at the point of use in AppendSession::new

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/session/append.rs
Line: 90:103

Comment:
**logic:** validation can be bypassed by directly mutating public fields

the fields `max_inflight_bytes` and `max_inflight_batches` are public, allowing users to bypass validation after construction:

```rust
let mut config = AppendSessionConfig::new();
config.max_inflight_bytes = 100; // bypasses 1 MiB minimum!
config.max_inflight_batches = Some(0); // bypasses minimum of 1!
```

consider making these fields private and adding getter methods, or add validation at the point of use in `AppendSession::new`

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

[quettabit]

yeah i'm aware of the possibility of this misuse, not just for this type but for few other types. the choice i made was to just prevent direct struct inits, and let constructors and builders be the only way to construct instances. private fields and providing accessors for em is something that i'm not leaning towards at the moment.

[quettabit]

looks like readonly crate might fit the bill. will check that out.

[quettabit]

one big downside w/ readonly crate is that the readonly fields are not getting shown in IDE's autocomplete options.

[greptile-apps]

That's a valid concern with the readonly crate - losing IDE autocomplete for read-only fields definitely hurts developer experience.

A few alternative approaches you could consider:

1. Runtime validation in AppendSession::new() - Keep the fields public but add validation where the config is actually used, as you mentioned in your original comment. This preserves the current API while catching invalid values at the point of use.

2. Builder pattern with private fields - Make fields private and provide a builder that validates on build, but this changes the API more significantly.

3. Custom readonly implementation - Create your own read-only wrapper that's more IDE-friendly, though that's probably overkill for this use case.

Given your preference to avoid private fields + accessors and the IDE issue with readonly, the runtime validation approach in AppendSession::new() might be the most practical solution. It catches the misuse where it actually matters (when the session is created) while preserving the current ergonomics.