Skip to content

Conversation

@laststylebender14
Copy link
Contributor

@laststylebender14 laststylebender14 commented Dec 30, 2025

fixes: #1789

@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Dec 30, 2025
@laststylebender14 laststylebender14 marked this pull request as ready for review December 30, 2025 12:46
Comment on lines +2637 to +2641
(Some(acc), None) | (None, Some(acc)) => [
acc.cost.map(|c| format!("${:.3}", c)),
Some(format!("{}/{}", *acc.prompt_tokens, *acc.completion_tokens)),
acc.cache_hit_rate().map(|p| format!("{:.2}%", p)),
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic bug in pattern matching. When accumulated is None and last_request is Some, the pattern (None, Some(acc)) binds acc to last_request, not accumulated. This contradicts the comment on line 2627 which states "Use accumulated for cost and cache rate". In this case, cost and cache_hit_rate will be calculated from last_request instead of accumulated.

The second match arm should only handle (Some(acc), None) to use accumulated data. The (None, Some(last)) case should be handled separately:

(Some(acc), None) => [
    acc.cost.map(|c| format!("${:.3}", c)),
    Some(format!("{}/{}", *acc.prompt_tokens, *acc.completion_tokens)),
    acc.cache_hit_rate().map(|p| format!("{:.2}%", p)),
],
(None, Some(last)) => [
    None, // No accumulated cost
    Some(format!("{}/{}", *last.prompt_tokens, *last.completion_tokens)),
    None, // No accumulated cache rate
],
Suggested change
(Some(acc), None) | (None, Some(acc)) => [
acc.cost.map(|c| format!("${:.3}", c)),
Some(format!("{}/{}", *acc.prompt_tokens, *acc.completion_tokens)),
acc.cache_hit_rate().map(|p| format!("{:.2}%", p)),
],
(Some(acc), None) => [
acc.cost.map(|c| format!("${:.3}", c)),
Some(format!("{}/{}", *acc.prompt_tokens, *acc.completion_tokens)),
acc.cache_hit_rate().map(|p| format!("{:.2}%", p)),
],
(None, Some(last)) => [
None, // No accumulated cost
Some(format!("{}/{}", *last.prompt_tokens, *last.completion_tokens)),
None, // No accumulated cache rate
],

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +159 to +169
pub fn set_message(&mut self, message: &str) -> Result<()> {
if let Some(pb) = &self.progress_bar {
let idx = *self
.word_index
.get_or_insert_with(|| rand::rng().random_range(0..THINKING_WORDS.len()));
let word = THINKING_WORDS[idx].to_string();
self.current_message = Some(format!("{} {}", word, message));
pb.set_message(message.green().bold().to_string());
}
Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistency between current_message and the displayed message. Line 165 stores the full message including the word prefix (format!("{} {}", word, message)), but line 166 only displays the message parameter without the word prefix. This creates a mismatch between the internal state and what's displayed.

The displayed message should match the stored state:

pub fn set_message(&mut self, message: &str) -> Result<()> {
    if let Some(pb) = &self.progress_bar {
        let idx = *self
            .word_index
            .get_or_insert_with(|| rand::rng().random_range(0..THINKING_WORDS.len()));
        let word = THINKING_WORDS[idx].to_string();
        let full_message = format!("{} {}", word, message);
        self.current_message = Some(full_message.clone());
        pb.set_message(full_message.green().bold().to_string());
    }
    Ok(())
}
Suggested change
pub fn set_message(&mut self, message: &str) -> Result<()> {
if let Some(pb) = &self.progress_bar {
let idx = *self
.word_index
.get_or_insert_with(|| rand::rng().random_range(0..THINKING_WORDS.len()));
let word = THINKING_WORDS[idx].to_string();
self.current_message = Some(format!("{} {}", word, message));
pb.set_message(message.green().bold().to_string());
}
Ok(())
}
pub fn set_message(&mut self, message: &str) -> Result<()> {
if let Some(pb) = &self.progress_bar {
let idx = *self
.word_index
.get_or_insert_with(|| rand::rng().random_range(0..THINKING_WORDS.len()));
let word = THINKING_WORDS[idx].to_string();
let full_message = format!("{} {}", word, message);
self.current_message = Some(full_message.clone());
pb.set_message(full_message.green().bold().to_string());
}
Ok(())
}

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@laststylebender14 laststylebender14 marked this pull request as draft January 2, 2026 04:11
@github-actions
Copy link

github-actions bot commented Jan 7, 2026

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. type: feature Brand new functionality, features, pages, workflows, endpoints, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: show token and cost with each log line

3 participants