[API-206] Add conversations top-level command to CLI#57
Merged
Conversation
📝 WalkthroughWalkthroughAdds conversation management: new CLI subcommand and handlers, client routing for /v1/conversations endpoints, conversation data models and responses, audio download support, and improved NotFound hinting in simulation commands. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant CLI as CLI Dispatcher
participant Handler as Conversations Handler
participant Client as Conversations Client
participant API as HTTP API
participant FS as Filesystem
User->>CLI: invoke "conversations" subcommand
CLI->>Handler: dispatch with parsed args
Handler->>Client: call (list|get|delete|audio|metrics)
Client->>API: HTTP request to /v1/conversations...
API-->>Client: JSON response
Client-->>Handler: parsed model/response
alt audio download
Handler->>API: fetch audio URL (or use returned URL)
API-->>Handler: audio bytes/stream
Handler->>FS: write file with progress
end
Handler-->>User: display output or save file
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
lorenjphillips
force-pushed
the
loren/api-206-conversations-command
branch
from
3da846c to
58f427b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/client/models/conversation.rs (2)
39-59: Consider using#[serde(alias)]to simplify the enum.The duplicate variants (
InQueue/InQueueSpace,InProgress/InProgressSpace) handle API inconsistency between underscore and space separators. Usingaliaswould consolidate them.♻️ Alternative using serde alias
#[derive(Debug, Clone, Copy, Serialize, Deserialize)] pub enum ConversationStatus { #[serde(rename = "PENDING")] Pending, - #[serde(rename = "IN_QUEUE")] + #[serde(rename = "IN_QUEUE", alias = "IN QUEUE")] InQueue, - #[serde(rename = "IN QUEUE")] - InQueueSpace, - #[serde(rename = "IN_PROGRESS")] + #[serde(rename = "IN_PROGRESS", alias = "IN PROGRESS")] InProgress, - #[serde(rename = "IN PROGRESS")] - InProgressSpace, #[serde(rename = "COMPLETED")] Completed, // ... rest unchanged }This would also simplify the
Displayimpl by removing the| Self::InQueueSpaceand| Self::InProgressSpacepatterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/models/conversation.rs` around lines 39 - 59, The ConversationStatus enum defines duplicate variants (InQueue/InQueueSpace and InProgress/InProgressSpace) to handle API variations; replace these duplicates by keeping a single variant for each logical state (InQueue, InProgress) and add serde aliases (e.g., #[serde(rename = "...", alias = "IN QUEUE")]) on those variants so both "IN_QUEUE" and "IN QUEUE" (and similarly for IN_PROGRESS/IN PROGRESS) deserialize to the same variant, then update the Display impl to remove patterns matching the removed duplicate variants and only match the consolidated variants (ConversationStatus::InQueue and ConversationStatus::InProgress).
103-110:ConversationAudioUrlResponseis missingSerializederive.Looking at
src/commands/conversations.rs, this type isn't passed toprint_one(only theaudio_urlstring is printed), so this isn't a compilation blocker. However, for consistency with other response types and to enable JSON output in the future, consider addingSerialize.♻️ Suggested change
-#[derive(Debug, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] pub struct ConversationAudioUrlResponse {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/models/conversation.rs` around lines 103 - 110, ConversationAudioUrlResponse only derives Deserialize; add Serialize so it can be serialized for JSON output. Update the struct's derives to include Serialize (e.g., #[derive(Debug, Serialize, Deserialize)]) and ensure serde::Serialize is in scope (adjust the use/import statements if necessary) so ConversationAudioUrlResponse matches other response types and can be printed/returned as JSON in the future.src/commands/conversations.rs (1)
154-176: Consider extractingdownload_audioto a shared module.This function is duplicated verbatim from
src/commands/simulations.rs. Extracting it to a shared utility module (e.g.,src/commands/common.rsorsrc/utils.rs) would reduce duplication.♻️ Example extraction
Create a shared module and import from both files:
// src/commands/common.rs (or src/utils.rs) use std::path::Path; use anyhow::Result; use indicatif::{ProgressBar, ProgressStyle}; pub async fn download_audio(url: &str, path: &Path) -> Result<()> { let client = reqwest::Client::new(); let resp = client.get(url).send().await?; if !resp.status().is_success() { anyhow::bail!("Failed to download audio: HTTP {}", resp.status()); } let total = resp.content_length().unwrap_or(0); let pb = ProgressBar::new(total); pb.set_style( ProgressStyle::default_bar() .template("{spinner:.green} [{bar:40.cyan/blue}] {bytes}/{total_bytes}")? .progress_chars("=>-"), ); let bytes = resp.bytes().await?; pb.set_position(bytes.len() as u64); pb.finish_and_clear(); std::fs::write(path, &bytes)?; Ok(()) }Then in both
simulations.rsandconversations.rs:use super::common::download_audio;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/conversations.rs` around lines 154 - 176, The download_audio function is duplicated; extract it into a shared module (e.g., create src/commands/common.rs or src/utils.rs) as a public async fn download_audio(url: &str, path: &Path) -> Result<()> and move the reqwest/indicatif/anyhow/std::path uses into that module; then replace the local download_audio definitions in both conversations.rs and simulations.rs with a use import (e.g., use crate::commands::common::download_audio or use crate::utils::download_audio) so both files call the single shared function, ensuring the function remains public and the module is declared in mod.rs or lib.rs so the imports resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/mod.rs`:
- Around line 305-311: The function formatting in the Conversation client method
audio (pub async fn audio(&self, id: &str) ->
Result<models::ConversationAudioUrlResponse, ApiError>) and the format! call
inside (self.0.url(&format!("/v1/conversations/{id}/audio"))) does not match the
project's rustfmt rules; reformat the function signature and the format!
invocation to match rustfmt (or simply run cargo fmt) so the signature,
parameter placement and the format! macro call adhere to the repository style
and CI will pass.
- Around line 326-328: The format! invocation should be split out so rustfmt can
reflow it: create a temporary variable (e.g. let path =
format!("/v1/conversations/{id}/metrics/{metric_output_id}");) and then call
self.0.url(&path) instead of embedding format! directly in the url(...) call;
update the code around the self.0.url and format! usage accordingly.
In `@src/commands/conversations.rs`:
- Around line 97-112: The block handling ConversationCommands::Delete is
misformatted; run rustfmt-style adjustments: break the chained call on
client.conversations().delete so the await is on its own line and align the
match arms, ensure commas and closing braces are properly spaced, and keep
print_success("Conversation deleted.") on its own line; also reformat the
print_not_found_hint function (the hint block around the NotFound arm) to follow
rustfmt conventions (consistent indentation, single spaces, and proper line
breaks). Locate ConversationCommands::Delete,
client.conversations().delete(&args.conversation_id).await, print_success and
print_not_found_hint to apply these formatting fixes.
In `@src/commands/simulations.rs`:
- Around line 145-149: The eprintln! in print_not_found_hint is split across
multiple lines and fails cargo fmt; collapse the eprintln! invocation into a
single line so the entire string interpolation is on one line (update function
print_not_found_hint to call eprintln!("hint: not found as a simulation. Try
`coval {try_command} get {id}` instead.");).
---
Nitpick comments:
In `@src/client/models/conversation.rs`:
- Around line 39-59: The ConversationStatus enum defines duplicate variants
(InQueue/InQueueSpace and InProgress/InProgressSpace) to handle API variations;
replace these duplicates by keeping a single variant for each logical state
(InQueue, InProgress) and add serde aliases (e.g., #[serde(rename = "...", alias
= "IN QUEUE")]) on those variants so both "IN_QUEUE" and "IN QUEUE" (and
similarly for IN_PROGRESS/IN PROGRESS) deserialize to the same variant, then
update the Display impl to remove patterns matching the removed duplicate
variants and only match the consolidated variants (ConversationStatus::InQueue
and ConversationStatus::InProgress).
- Around line 103-110: ConversationAudioUrlResponse only derives Deserialize;
add Serialize so it can be serialized for JSON output. Update the struct's
derives to include Serialize (e.g., #[derive(Debug, Serialize, Deserialize)])
and ensure serde::Serialize is in scope (adjust the use/import statements if
necessary) so ConversationAudioUrlResponse matches other response types and can
be printed/returned as JSON in the future.
In `@src/commands/conversations.rs`:
- Around line 154-176: The download_audio function is duplicated; extract it
into a shared module (e.g., create src/commands/common.rs or src/utils.rs) as a
public async fn download_audio(url: &str, path: &Path) -> Result<()> and move
the reqwest/indicatif/anyhow/std::path uses into that module; then replace the
local download_audio definitions in both conversations.rs and simulations.rs
with a use import (e.g., use crate::commands::common::download_audio or use
crate::utils::download_audio) so both files call the single shared function,
ensuring the function remains public and the module is declared in mod.rs or
lib.rs so the imports resolve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64b63993-1777-4a9c-89bd-c2dd4c7aefce
📒 Files selected for processing (7)
src/cli.rssrc/client/mod.rssrc/client/models/conversation.rssrc/client/models/mod.rssrc/commands/conversations.rssrc/commands/mod.rssrc/commands/simulations.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/commands/conversations.rs (1)
142-146:⚠️ Potential issue | 🟡 MinorFix formatting to pass CI.
The pipeline indicates the
eprintln!macro needs to be on a single line.🔧 Proposed fix
fn print_not_found_hint(id: &str, try_command: &str) { - eprintln!( - "hint: not found as a conversation. Try `coval {try_command} get {id}` instead." - ); + eprintln!("hint: not found as a conversation. Try `coval {try_command} get {id}` instead."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/conversations.rs` around lines 142 - 146, The eprintln! invocation in print_not_found_hint currently spans multiple lines and fails CI formatting; collapse the macro call into a single line so the entire eprintln!(...) is on one line (update the eprintln! in function print_not_found_hint to a single-line invocation with the same formatted string and variables).
🧹 Nitpick comments (1)
src/commands/conversations.rs (1)
148-169: Consider extractingdownload_audioto a shared module.This function is duplicated from
simulations.rs(see lines 151-173 in that file). While acceptable for this PR, consider extracting it to a shared utility module to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/conversations.rs` around lines 148 - 169, The download_audio function is duplicated; extract it into a shared utility module by moving async fn download_audio(url: &str, path: &Path) -> Result<()> into a new/common module (make it pub async fn download_audio(...)->anyhow::Result<()>), preserve its implementation and required imports (reqwest, ProgressBar, ProgressStyle, std::fs, Path, anyhow::Result), update both conversations.rs and simulations.rs to import and call the shared download_audio (replace the local definitions), and run cargo check to fix any visibility or use path issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/conversations.rs`:
- Around line 77-90: The match arm in ConversationCommands::Get (around
client.conversations().get and the Err(ApiError::NotFound { .. }) branch that
calls print_not_found_hint) is misformatted; run cargo fmt to reformat the
chained call and match arms so the Err(ApiError::NotFound { .. }) => { block and
its returned ApiError::NotFound construction follow the project's rustfmt rules,
then re-commit the formatted file.
- Around line 122-137: Reformat the chained async call in the
ConversationCommands::Audio arm so it satisfies rustfmt: break the chain on
separate lines instead of one long expression for
client.conversations().audio(&args.conversation_id).await?; adjust the let audio
= ... assignment accordingly in the ConversationCommands::Audio match arm (the
surrounding symbols are ConversationCommands::Audio, client.conversations(),
audio(), and the later uses download_audio and print_success remain unchanged).
---
Duplicate comments:
In `@src/commands/conversations.rs`:
- Around line 142-146: The eprintln! invocation in print_not_found_hint
currently spans multiple lines and fails CI formatting; collapse the macro call
into a single line so the entire eprintln!(...) is on one line (update the
eprintln! in function print_not_found_hint to a single-line invocation with the
same formatted string and variables).
---
Nitpick comments:
In `@src/commands/conversations.rs`:
- Around line 148-169: The download_audio function is duplicated; extract it
into a shared utility module by moving async fn download_audio(url: &str, path:
&Path) -> Result<()> into a new/common module (make it pub async fn
download_audio(...)->anyhow::Result<()>), preserve its implementation and
required imports (reqwest, ProgressBar, ProgressStyle, std::fs, Path,
anyhow::Result), update both conversations.rs and simulations.rs to import and
call the shared download_audio (replace the local definitions), and run cargo
check to fix any visibility or use path issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9987465a-6cfe-4f92-b4d7-2fc989e27606
📒 Files selected for processing (7)
src/cli.rssrc/client/mod.rssrc/client/models/conversation.rssrc/client/models/mod.rssrc/commands/conversations.rssrc/commands/mod.rssrc/commands/simulations.rs
✅ Files skipped from review due to trivial changes (2)
- src/commands/mod.rs
- src/client/models/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/models/conversation.rs
lorenjphillips
force-pushed
the
loren/api-206-conversations-command
branch
from
58f427b to
2b55ca7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/models/conversation.rs (1)
127-129: Clarify timezone inOCCURRED ATtable output.The
occurred_atfield isDateTime<Utc>, but the format string"%Y-%m-%d %H:%M"displays the timestamp without a timezone indicator, which can be misread as local time in CLI output. AppendingUTCto the format string clarifies the timezone.Proposed diff
- self.occurred_at - .map(|t| t.format("%Y-%m-%d %H:%M").to_string()) - .unwrap_or_else(|| "-".into()), + self.occurred_at + .map(|t| t.format("%Y-%m-%d %H:%M UTC").to_string()) + .unwrap_or_else(|| "-".into()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/models/conversation.rs` around lines 127 - 129, The CLI output for the occurred_at DateTime<Utc] lacks a timezone label; update the formatting call on the occurred_at field (the closure that calls t.format("%Y-%m-%d %H:%M").to_string()) to include a timezone indicator—e.g., append " UTC" to the format string or use a format specifier that emits the offset—so the table shows the timestamp with its UTC timezone; leave the unwrap_or_else fallback ("-") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/models/conversation.rs`:
- Around line 127-129: The CLI output for the occurred_at DateTime<Utc] lacks a
timezone label; update the formatting call on the occurred_at field (the closure
that calls t.format("%Y-%m-%d %H:%M").to_string()) to include a timezone
indicator—e.g., append " UTC" to the format string or use a format specifier
that emits the offset—so the table shows the timestamp with its UTC timezone;
leave the unwrap_or_else fallback ("-") unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e770a854-1571-45ca-bdb7-c736fa1832ab
📒 Files selected for processing (7)
src/cli.rssrc/client/mod.rssrc/client/models/conversation.rssrc/client/models/mod.rssrc/commands/conversations.rssrc/commands/mod.rssrc/commands/simulations.rs
✅ Files skipped from review due to trivial changes (2)
- src/client/models/mod.rs
- src/commands/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/mod.rs
- src/commands/conversations.rs
kdmelon
approved these changes
Apr 3, 2026
kdmelon
left a comment
kdmelon
left a comment
Contributor
There was a problem hiding this comment.
lil nit, thoughts on adding 404 hints not metrics/metric-detail? so if a user runs coval conversations metrics <sim_id>, they get the hint to try coval simulations metrics instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
coval conversationscommand with full subcommand parity tosimulations(list, get, delete, audio, metrics, metric-detail)ConversationsClient,Conversationmodel,ConversationAudioUrlResponse(proper response type, not reusing simulation's)ConversationStatusenum,ConversationProgressstruct,Tabularimplsimulationsandconversationsget/delete pointing users to the other resource type--monitoringflag + auto-detect approach from [API-206] Add --monitoring flag with auto-detect to simulations commands #56 with explicit, predictable commandsCloses Dakota's #56 (superseded by this approach).
ID: b10c01b3-82c2-4f6e-913a-df1c2a1f38b7
Summary by CodeRabbit
New Features
Bug Fixes