Skip to content

Conversation

@laststylebender14
Copy link
Contributor

No description provided.

- Added `OutputPrinter` trait for thread-safe writing to stdout and stderr.
- Implemented `OutputPrinterInfra` in various services to utilize the new output printer.
- Refactored `ForgeCommandExecutorService` to use `OutputPrinter` for command output.
- Updated `SpinnerManager` to support output printing through the new trait.
- Removed acknowledgment mechanism from `ChatResponse` to simplify tool call handling.
- Enhanced `StreamWriter` to work with `OutputPrinter` for better output management.
- Updated dependencies in `Cargo.lock` to include `forge_domain`.
@laststylebender14 laststylebender14 changed the title feat: implement progressive rendering feat: stream markdown Jan 5, 2026
@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Jan 5, 2026
laststylebender14 and others added 19 commits January 6, 2026 14:09
@laststylebender14 laststylebender14 marked this pull request as ready for review January 7, 2026 08:56

/// Start the spinner with a message.
pub fn start(&self, message: Option<&str>) -> Result<()> {
self.0.lock().unwrap().start(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Panic Risk: Unwrapping mutex lock will panic if the mutex is poisoned (e.g., if a thread panicked while holding the lock). This will crash the application in production.

// Fix: Handle the poison error gracefully
pub fn start(&self, message: Option<&str>) -> Result<()> {
    self.0.lock()
        .map_err(|e| anyhow::anyhow!("Spinner mutex poisoned: {}", e))?
        .start(message)
}
Suggested change
self.0.lock().unwrap().start(message)
self.0.lock()
.map_err(|e| anyhow::anyhow!("Spinner mutex poisoned: {}", e))?
.start(message)

Spotted by Graphite Agent

Fix in Graphite


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

}
}));
});
*self.tracker.lock().unwrap() = Some(handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Panic Risk: Unwrapping mutex lock will panic if the mutex is poisoned. Since this is in the hot path of spinner operations, a poisoned mutex will crash the application.

// Fix: Handle the poison error gracefully
let mut tracker_guard = self.tracker.lock()
    .map_err(|e| anyhow::anyhow!("Tracker mutex poisoned: {}", e))?;
*tracker_guard = Some(handle);
Suggested change
*self.tracker.lock().unwrap() = Some(handle);
let mut tracker_guard = self.tracker.lock()
.map_err(|e| anyhow::anyhow!("Tracker mutex poisoned: {}", e))?;
*tracker_guard = Some(handle);

Spotted by Graphite Agent

Fix in Graphite


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

forge_select = { path = "crates/forge_select" }
forge_test_kit = { path = "crates/forge_test_kit" }

forge_markdown_stream = { git = "https://github.com/laststylebender14/forge_markdown_stream", branch = "master" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move the project into our repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feature Brand new functionality, features, pages, workflows, endpoints, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants