Conversation
Reviewer's GuideThe PR refactors end-of-output handling to detect and silently swallow BrokenPipe errors during printing, updates print_end_banner to return a Result and use writeln!, and adds an e2e regression test to verify clean exit when output is piped to a short-lived reader. Sequence diagram for handling BrokenPipe during output in vk prsequenceDiagram
participant vk_pr as vk pr (main)
participant stdout as Stdout
vk_pr->>stdout: print_reviews()
alt BrokenPipe error
vk_pr-->>vk_pr: Return Ok(()) (exit silently)
else Other error
vk_pr-->>vk_pr: Log error
end
vk_pr->>stdout: print_thread()
alt BrokenPipe error
vk_pr-->>vk_pr: Return Ok(()) (exit silently)
else Other error
vk_pr-->>vk_pr: Log error
end
vk_pr->>stdout: print_end_banner()
alt BrokenPipe error
vk_pr-->>vk_pr: Return Ok(()) (exit silently)
else Other error
vk_pr-->>vk_pr: Log error
end
Class diagram for updated print_end_banner functionclassDiagram
class summary {
+print_end_banner() std::io::Result<()>
}
summary : print_end_banner() uses writeln! to write to stdout
summary : print_end_banner() returns error on write failure
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughExpand error handling for output operations in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant vk (run_pr)
participant OS Pipe
User->>vk (run_pr): Start process, pipe output
vk (run_pr)->>OS Pipe: Write output
OS Pipe-->>vk (run_pr): BrokenPipe error (pipe closed)
vk (run_pr)->>vk (run_pr): Detect BrokenPipe, return Ok(())
vk (run_pr)-->>User: Exit cleanly
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The
.chain().any(|c| c.downcast_ref::<io::Error>().map_or(false, |io| io.kind() == ErrorKind::BrokenPipe))logic is repeated in several spots – consider extracting it into a helper likeis_broken_pipe(err: &Error) -> boolto DRY up the code. - Instead of checking for BrokenPipe after each individual print call, you could wrap the entire printing flow in a single writer that gracefully ignores
BrokenPipeto keep the top-level logic simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `.chain().any(|c| c.downcast_ref::<io::Error>().map_or(false, |io| io.kind() == ErrorKind::BrokenPipe))` logic is repeated in several spots – consider extracting it into a helper like `is_broken_pipe(err: &Error) -> bool` to DRY up the code.
- Instead of checking for BrokenPipe after each individual print call, you could wrap the entire printing flow in a single writer that gracefully ignores `BrokenPipe` to keep the top-level logic simpler.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements graceful handling of BrokenPipe errors in the vk pr command to improve the user experience when piping output to utilities that may terminate early (like head, grep, etc.). The changes address a common CLI tool issue where error messages are logged when downstream processes close their input before consuming all output.
The implementation adds BrokenPipe detection at three critical output stages in the run_pr() function: when printing reviews, printing threads, and printing the end banner. Each error handler now checks if the error chain contains a BrokenPipe IO error and returns Ok(()) instead of logging an error message. The print_end_banner() function was refactored from using println! to writeln! with explicit error handling, allowing the caller to detect and handle BrokenPipe scenarios consistently.
The changes integrate well with the existing error handling patterns in the codebase, maintaining the use of anyhow::Error for most operations while specifically handling the std::io::Error cases where BrokenPipe detection is needed. A comprehensive regression test ensures the functionality works end-to-end by spawning vk pr piped to head -n 1 and verifying clean exit behavior.
Important Files Changed
File Changes
| Filename | Score | Overview |
|---|---|---|
| src/main.rs | 5/5 | Added BrokenPipe error detection and graceful handling in three output stages of run_pr() |
| src/summary.rs | 5/5 | Converted print_end_banner() from println! to writeln! with proper error propagation |
| tests/e2e.rs | 4/5 | Added regression test for BrokenPipe handling using process pipeline with head -n 1 |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it only improves error handling for a specific edge case
- Score reflects well-structured error handling with appropriate fallback behavior and comprehensive test coverage
- No files require special attention as all changes follow established patterns and include proper testing
3 files reviewed, no comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/main.rs(2 hunks)src/summary.rs(2 hunks)tests/e2e.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
src/main.rssrc/summary.rstests/e2e.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/main.rssrc/summary.rstests/e2e.rs
🔍 MCP Research (1 server)
Deepwiki:
-
Issue ENG-2040: The
run_prfunction insrc/main.rshandles output errors during printing of reviews, threads, and the end banner. It now specifically checks forErrorKind::BrokenPipeand returnsOk(())immediately to avoid logging errors when the output stream is closed prematurely. Other errors continue to be logged as warnings. This improves robustness in output handling and prevents unnecessary error logs related to broken pipe scenarios. (ENG-2040) -
Document [End-to-End Testing]: The
tests/e2e.rsfile includes an async Tokio testpr_exits_cleanly_on_broken_pipethat runs thevk prcommand piped intohead -n 1to simulate a broken pipe scenario. The test asserts that thevkprocess exits successfully within 5 seconds, verifying that the application handles broken pipe errors gracefully without crashing or logging errors. (tests/e2e.rs) -
Document [Supporting Components and Utilities]: The printing functions in
src/main.rsand related modules now propagate I/O errors including broken pipe detection, improving error handling in output rendering. (src/main.rs)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (5)
src/summary.rs (1)
7-7: Trait import is appropriate forwriteln!usage.Keep this;
writeln!on a locked stdout handle requiresstd::io::Writein scope.src/main.rs (2)
45-45: Import forErrorKindis correct and scoped.This supports BrokenPipe detection without broadening imports.
162-167: Confirm minimum supported Rust version forOption::is_some_andusage.No explicit MSRV declaration found in Cargo.toml or rust-toolchain files. Ensure this project’s MSRV is at least 1.62 (when
Option::is_some_andstabilised) before keeping the current code. Otherwise replace the call to avoid bumping MSRV.• Search CI configs or docs for an MSRV specification.
• If MSRV < 1.62, replace the snippet at lines 162–167 (and 173–178) with:if e.chain().any(|c| { c.downcast_ref::<std::io::Error>() .map(|io| io.kind() == ErrorKind::BrokenPipe) .unwrap_or(false) }) { return Ok(()); }tests/e2e.rs (2)
12-13: Add imports for binary path lookup.These are needed for the new end-to-end test.
17-18: Add process and timing imports.These are appropriate for spawning child processes and timeouts.
1 participant
Summary
writeln!and surface write errorsvk prexits cleanly when piped to a short-lived readerTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_6898985477208322a5415a24d283ad2d
Summary by Sourcery
Gracefully handle BrokenPipe errors during review output and end banner printing, surface write errors via Result, and add a regression test to ensure clean exits when piping output into a reader that closes early
Enhancements:
Documentation:
Tests: