fix(flare-output): don't fail RealLlm on a benign stdin broken-pipe - #234
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesCLI error handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2666b5b to
d4db010
Compare
call_via_cli() writes the prompt to the child's stdin on a separate thread (to avoid the write/read deadlock) but then treated ANY error from that write -- including a broken pipe from a child that already exited successfully -- as fatal, even before checking whether the child itself succeeded. A child closing its stdin read end early after a clean exit (a canned response, or any CLI that doesn't need to drain the full prompt) is the ordinary short-pipe case, not a real failure. This is exactly what made tests/caveman_cli.rs's stubbed-claude test flaky under CI's compile-load scheduling jitter: the stub echoes a canned response and exits immediately without reading stdin, so the write's completion raced the child's exit. Reorders the checks so the child's actual exit status is what decides success/failure; a stdin-write error only surfaces as a warning once we already know the child succeeded. Stress-tested 22 consecutive runs of the previously-flaky test, including under concurrent compile load.
d4db010 to
ca3ce35
Compare
…icate-item reuse, assignee freeze (#365) * feat(handoff): verified continuation commit, structured payload, duplicate-item reuse, assignee freeze Item #236 (handoff hardening, QuorumGit adoption): - HandoffRequest gains last_commit: verified via `git cat-file -e` and, when the item's task/<seq> branch already exists, `git merge-base --is-ancestor` -- a fabricated or unreachable OID is rejected, not trusted. - HandoffRequest gains required completed/remaining and optional blockers fields, stored in the asset's metadata. - A handoff without item_id now reuses an existing open item assigned to the recipient (matched by name or thread_id) instead of blindly creating a duplicate; genuinely new work still auto-creates. - agentflare_backend::item::claim gains a BlockedByAssignee outcome: a freshly handed-off, never-claimed item can only be claimed by its assignee until accepted, excluding completed/cancelled items. Deferred: shim-side branch-push denial for open handoffs (spec's own review already downgraded this to defense-in-depth, covered in large part by #234's scope enforcement and the opencode branch-guard plugin). * fix(handoff): fmt, canonicalize claim owner, validate commit oid/payload off the DB lock (#366) - cargo fmt (item.rs claim closure, types.rs schemars doc attr) - item::claim: canonicalize both sides of the assignee/owner comparison so an alias owner (claude:1) isn't wrongly BlockedByAssignee against its own canonical handoff assignee (claude-code) - verify_continuation_commit: resolve the target branch under the backend DB lock, then run all git subprocess checks after releasing it, matching the existing item_claim split; validate oid is a plain hex id and force commit-type resolution (oid^{commit}) so a non-hex/flag-like value or a blob/tree/tag can't pass as a continuation commit - handoff_impl: reject empty completed/remaining instead of silently accepting an empty structured payload - mcp_prompts: document completed/remaining as required handoff fields so generated requests don't fail deserialization * fix(test): stop with_temp_home_clears_the_override_env_var_after_returning from racing concurrent with_temp_home callers The assertion read AGENTFLARE_HOME_OVERRIDE outside GLOBAL_STATE_LOCK, the lock that guards every set/remove of it. A concurrent thread's own correctly-scoped with_temp_home call could be transiently holding the var set at the exact moment this test read it, flaking the check on a var this test was never entitled to observe in the first place. Fix: acquire the same lock before reading.
1 participant
Summary
Root-causes the
caveman_cli.rsflake that's been showing up asbroken pipefailures under CI's compile-load scheduling jitter (most recently cascading into a fail-fast cancellation of #230's Windows build).call_via_cli()writes the prompt to the child's stdin on a separate thread (correctly, to avoid the classic write/read deadlock), but then treated any error from that write — including a broken pipe from a child that already exited successfully — as fatal, checked before the child's own exit status.A child closing its stdin read end early after a clean exit (a canned response, a CLI that doesn't need to drain the full prompt) is the ordinary short-pipe case, not a real failure. The test's stubbed
claudescriptechos a canned response and exits immediately without touching stdin at all, so the write's completion races the child's exit — under compile-load scheduling pressure, the child sometimes wins the race.Fix
Reorder the checks: the child's actual exit status decides success/failure. A stdin-write error only surfaces (as a non-fatal warning) once we already know the child succeeded — it no longer masks the real failure/success signal.
Test plan
cargo test --workspace(567 passed)cargo fmt --checkcargo clippy -p agentflare-flare-output --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedanticSummary by CodeRabbit