fix: refresh in-progress workflow handling - #3
Merged
Conversation
📝 WalkthroughWalkthroughAdds detection and handling for in-progress workflows across models, reporting, and CLI fetch logic; report renderers treat running workflows specially and exclude them from "all passed" counts; CI build step now runs Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Main
participant Cache as ci-status.json
participant GH as GitHub API (Octocrab)
participant Models as WorkflowStatus
participant Report as report.rs
CLI->>Cache: read cached statuses
alt cache missing or empty
CLI->>GH: fetch_current_branch_workflows(owner, repo, branch, sha)
GH-->>CLI: workflow runs
CLI->>Models: map runs -> WorkflowStatus
else cache present
CLI->>Models: parse cached JSON -> Vec<WorkflowStatus>
CLI->>Models: should_refresh_workflows(cached)?
alt needs refresh
CLI->>GH: fetch_current_branch_workflows(...)
GH-->>CLI: workflow runs
CLI->>Models: map runs -> WorkflowStatus
end
end
CLI->>Report: render_human_report(statuses)
CLI->>Report: render_llm_report(statuses)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/report.rs (1)
68-70:⚠️ Potential issue | 🟠 Major
all workflows passedcan be emitted while workflows are still running.At Line 68 and Line 69, success/failure are counted from conclusion-only checks. Combined with Line 107, a running workflow with stale success conclusion can produce a false “all workflows passed”.
Suggested fix
- let success = workflows.iter().filter(|w| w.is_success()).count(); - let failure = workflows.iter().filter(|w| w.is_failure()).count(); + let success = workflows + .iter() + .filter(|w| !w.is_in_progress() && w.is_success()) + .count(); + let failure = workflows + .iter() + .filter(|w| !w.is_in_progress() && w.is_failure()) + .count(); let other = total.saturating_sub(success + failure); @@ - for workflow in workflows.iter().filter(|w| w.is_failure()) { + for workflow in workflows + .iter() + .filter(|w| !w.is_in_progress() && w.is_failure()) + { @@ - if failure == 0 && other == 0 { + let all_passed = workflows + .iter() + .all(|w| !w.is_in_progress() && w.is_success()); + if all_passed { lines.push("all workflows passed".to_string()); }Also applies to: 77-78, 100-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` around lines 68 - 70, The code counts successes/failures using conclusion-only predicates (is_success()/is_failure()) and can report "all workflows passed" while some workflows are still running; update all places that compute success/failure (the usages around workflows.iter() at the top and the similar blocks at lines ~77-78 and ~100-108) to first restrict to concluded workflows—e.g., filter with w.is_concluded() or w.status == WorkflowStatus::Completed before calling is_success()/is_failure()—so counts and the “all workflows passed” decision only consider workflows that have finished.
🧹 Nitpick comments (1)
src/report.rs (1)
133-157: Add a regression test for conflicting status/conclusion.Please add a case like
status="in_progress"withconclusion=Some("success")to ensure report rendering keeps treating it as running.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` around lines 133 - 157, Add a regression test that supplies a workflow with status "in_progress" but conclusion Some("success") using the existing helper workflow(...) and assert the renderer treats it as running; for example, update or add to the tests using render_human_report and render_llm_report to include workflow("FreeBSD","in_progress",Some("success")) and assert the human report contains "- FreeBSD [RUNNING] (in_progress)" and the LLM report does not contain "all workflows passed" and does contain "running workflow=FreeBSD status=in_progress". Use the existing test names (e.g., renders_in_progress_workflow_as_running or llm_report_does_not_mark_running_workflows_as_passed) or add a new test function to keep behavior coverage for conflicting status/conclusion cases.
🤖 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/report.rs`:
- Around line 32-53: The status check for in-progress workflows is currently
after conclusion checks, causing workflows with status == in_progress but stale
conclusion == success to render as passed; update the branching in report
generation so that workflow.is_in_progress() (or checking workflow.status ==
"in_progress") is evaluated before workflow.is_success() and
workflow.is_failure() — move the block that formats the running state (the code
using workflow.is_in_progress(), icon_running(), and workflow.status) to occur
prior to the success/failure blocks (or change the conditional order) so
in-progress workflows are rendered correctly.
---
Outside diff comments:
In `@src/report.rs`:
- Around line 68-70: The code counts successes/failures using conclusion-only
predicates (is_success()/is_failure()) and can report "all workflows passed"
while some workflows are still running; update all places that compute
success/failure (the usages around workflows.iter() at the top and the similar
blocks at lines ~77-78 and ~100-108) to first restrict to concluded
workflows—e.g., filter with w.is_concluded() or w.status ==
WorkflowStatus::Completed before calling is_success()/is_failure()—so counts and
the “all workflows passed” decision only consider workflows that have finished.
---
Nitpick comments:
In `@src/report.rs`:
- Around line 133-157: Add a regression test that supplies a workflow with
status "in_progress" but conclusion Some("success") using the existing helper
workflow(...) and assert the renderer treats it as running; for example, update
or add to the tests using render_human_report and render_llm_report to include
workflow("FreeBSD","in_progress",Some("success")) and assert the human report
contains "- FreeBSD [RUNNING] (in_progress)" and the LLM report does not contain
"all workflows passed" and does contain "running workflow=FreeBSD
status=in_progress". Use the existing test names (e.g.,
renders_in_progress_workflow_as_running or
llm_report_does_not_mark_running_workflows_as_passed) or add a new test function
to keep behavior coverage for conflicting status/conclusion cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/report.rs (1)
145-155: Make running-icon tests terminal-agnostic.The test assertions at lines 148 and 154 check only for
[RUNNING], buticon_running()returns⏳whensupports_emoji()is true—which occurs when stdout is a terminal and theNO_EMOJIenvironment variable is unset. These tests will fail if run in an interactive terminal withoutNO_EMOJIset, despite correct behavior.Harden the assertions to accept both patterns:
Proposed fix
assert!(report.contains("- FreeBSD [RUNNING] (in_progress)")); + assert!( + report.contains("- FreeBSD [RUNNING] (in_progress)") + || report.contains("- FreeBSD ⏳ (in_progress)") + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` around lines 145 - 155, The two tests renders_in_progress_workflow_as_running and renders_in_progress_with_success_conclusion_as_running assert only on the literal "[RUNNING]" but icon_running() may return an emoji when supports_emoji() is true; update these test assertions in src/report.rs (the two test functions calling render_human_report and workflow) so they accept either the text marker "[RUNNING]" or the emoji from icon_running() (call or reproduce both possible outputs) — i.e., check the rendered report contains either "- FreeBSD [RUNNING] (in_progress)" OR "- FreeBSD ⏳ (in_progress)" (or use icon_running() to build the expected string) so the tests are terminal-agnostic.
🤖 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/report.rs`:
- Around line 116-120: The current computation of all_passed (let all_passed =
workflows.iter().all(|w| !w.is_in_progress() && w.is_success());) wrongly
returns true for an empty workflows iterator and causes lines.push("all
workflows passed".to_string()) to run on empty lists; change the guard so it
only pushes that message when there is at least one workflow and all of them
passed — e.g., require workflows.iter().next().is_some() (or
workflows.is_empty() == false) in conjunction with the existing all() check
before calling lines.push("all workflows passed".to_string()).
---
Nitpick comments:
In `@src/report.rs`:
- Around line 145-155: The two tests renders_in_progress_workflow_as_running and
renders_in_progress_with_success_conclusion_as_running assert only on the
literal "[RUNNING]" but icon_running() may return an emoji when supports_emoji()
is true; update these test assertions in src/report.rs (the two test functions
calling render_human_report and workflow) so they accept either the text marker
"[RUNNING]" or the emoji from icon_running() (call or reproduce both possible
outputs) — i.e., check the rendered report contains either "- FreeBSD [RUNNING]
(in_progress)" OR "- FreeBSD ⏳ (in_progress)" (or use icon_running() to build
the expected string) so the tests are terminal-agnostic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main.rs`:
- Around line 83-88: The current logic treats any non-empty
cached_workflow_statuses as authoritative, causing run status/conclusion to stay
stale if a cached workflow is still "in_progress"; change the conditional so
that you call fetch_current_branch_workflows(&octocrab, &owner, &repo, &branch,
&commit_sha).await? not only when cached_workflow_statuses.is_empty() but also
when any cached workflow has status "in_progress" (e.g.,
cached_workflow_statuses.iter().any(|w| w.status == "in_progress")). This
ensures full run metadata (status/conclusion) is refreshed, keeping it in sync
with the separate jobs-refresh logic that currently updates only jobs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main.rs`:
- Around line 81-85: The code currently fails hard when deserializing
cached_workflow_statuses via serde_json::from_str, but a malformed cache at
workflow_status_json_path should be treated as a cache miss; change the logic
around cached_workflow_statuses/WorkflowStatus to handle parse errors by
catching the serde_json::from_str result (e.g., match or Result::or_else) and on
Err log a warning and return an empty Vec<WorkflowStatus> so the tool refetches
instead of exiting (keep file read errors as fatal but recover from JSON parse
failures).
Comment on lines
+81
to
+85
| let cached_workflow_statuses: Vec<WorkflowStatus> = serde_json::from_str( | ||
| &fs::read_to_string(&workflow_status_json_path) | ||
| .context("Failed to read existing ci-status.json")?, | ||
| ) | ||
| .context("Failed to parse existing ci-status.json")? | ||
| .context("Failed to parse existing ci-status.json")?; |
There was a problem hiding this comment.
Recover from malformed cache instead of failing hard.
At Line 81-85, JSON parse failure exits the tool immediately. Since this file is a local cache artifact, invalid JSON should be treated as a cache miss and trigger a refetch.
💡 Proposed fix
- let cached_workflow_statuses: Vec<WorkflowStatus> = serde_json::from_str(
- &fs::read_to_string(&workflow_status_json_path)
- .context("Failed to read existing ci-status.json")?,
- )
- .context("Failed to parse existing ci-status.json")?;
-
- if should_refresh_workflows(&cached_workflow_statuses) {
- output_mode.emit_verbose(
- "Cached ci-status.json requires workflow refresh; refreshing workflows...",
- );
- fetch_current_branch_workflows(&octocrab, &owner, &repo, &branch, &commit_sha).await?
- } else {
- cached_workflow_statuses
- }
+ let cached_raw = fs::read_to_string(&workflow_status_json_path)
+ .context("Failed to read existing ci-status.json")?;
+ match serde_json::from_str::<Vec<WorkflowStatus>>(&cached_raw) {
+ Ok(cached_workflow_statuses) => {
+ if should_refresh_workflows(&cached_workflow_statuses) {
+ output_mode.emit_verbose(
+ "Cached ci-status.json requires workflow refresh; refreshing workflows...",
+ );
+ fetch_current_branch_workflows(&octocrab, &owner, &repo, &branch, &commit_sha)
+ .await?
+ } else {
+ cached_workflow_statuses
+ }
+ }
+ Err(err) => {
+ output_mode.emit_verbose(format!(
+ "Cached ci-status.json is invalid ({err}); refreshing workflows..."
+ ));
+ fetch_current_branch_workflows(&octocrab, &owner, &repo, &branch, &commit_sha)
+ .await?
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 81 - 85, The code currently fails hard when
deserializing cached_workflow_statuses via serde_json::from_str, but a malformed
cache at workflow_status_json_path should be treated as a cache miss; change the
logic around cached_workflow_statuses/WorkflowStatus to handle parse errors by
catching the serde_json::from_str result (e.g., match or Result::or_else) and on
Err log a warning and return an empty Vec<WorkflowStatus> so the tool refetches
instead of exiting (keep file read errors as fatal but recover from JSON parse
failures).
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
1 participant
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.
Summary
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation