Conversation
📝 WalkthroughWalkthroughThe changes modify GitHub check title generation for deployment plans to report added and deleted line counts derived from unified diffs of proposed changes, replacing the previous format that summarized counts of changed, unchanged, and unsupported resources. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go (1)
57-74: Coverage gap: the new diff-counting logic isn't directly tested.The updated
TestAggregate_CheckTitlecases only assert on already-populatedAdditions/Deletionsvalues;countDiffLinesand theaggregateResultspath that populates those fields fromCurrent/Proposedaren't exercised. Consider extendingTestAggregateResults_Countsto assertagg.Additions/agg.Deletionsfor thecompletedResult("a", true, "old", "new")case, and/or adding a focusedTestCountDiffLineswith table cases (identical input → 0/0, pure additions, pure deletions, mixed, empty strings, content lines that happen to start with---or+++— which in a unified diff would appear as+---/-+++and must still be counted correctly).Also applies to: 140-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go` around lines 57 - 74, Extend TestAggregateResults_Counts to assert that aggregateResults populates Additions and Deletions for the completedResult("a", true, "old", "new") case (use agg.Additions and agg.Deletions) so countDiffLines is exercised, and/or add a new focused TestCountDiffLines table-driven test that calls countDiffLines with cases: identical inputs -> 0/0, pure additions, pure deletions, mixed changes, empty strings, and lines that begin with diff markers like "+---" or "-+++" to ensure those are counted correctly; locate the logic in aggregateResults and the countDiffLines function to wire inputs via completedResult helpers and assert expected numeric results.apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go (1)
157-182: Redundant unified-diff computation per changed agent.For every result with
HasChanges=true,difflib.GetUnifiedDiffStringis now invoked twice against the sameCurrent/Proposedinputs: once here withContext: 0to count lines, and again informatAgentSection(line 295) withContext: 3to render the markdown diff. For large manifests this doubles the diff work. Consider counting+/-lines directly from theContext: 3diff that's already produced informatAgentSectionand caching the totals onagentResult, or sharing a single diff through the aggregate pipeline.Also applies to: 295-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go` around lines 157 - 182, The countDiffLines function is redundantly recomputing a unified diff (Context:0) while formatAgentSection already generates a unified diff (Context:3); refactor so the unified diff is computed once and its +/− line counts are reused: have formatAgentSection (or the diff-producing step) return or attach the unified-diff string and computed adds/dels to the agentResult (e.g., add fields like AddedLines/DeletedLines or a CachedDiff string), compute adds/dels by scanning the already-produced Context:3 diff instead of calling difflib.GetUnifiedDiffString again, and update callers that currently call countDiffLines to read the cached totals; remove the duplicate GetUnifiedDiffString invocation in countDiffLines or inline its logic to use the cached diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`:
- Around line 264-269: The diff summary currently returns "+0 -0" when there are
no changes; update the logic in the formatting function that produces
diffSummary (the block using a.Additions, a.Deletions, a.Errored) to first
detect the all-clean case (a.Additions == 0 && a.Deletions == 0 && a.Changed ==
0) and return "No changes" in that case, otherwise build the "+%d -%d" summary
as before and still append the "(%d errored)" suffix when a.Errored > 0; keep
references to the same variables (a.Additions, a.Deletions, a.Changed,
a.Errored) so the change is a simple conditional early-return.
---
Nitpick comments:
In
`@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go`:
- Around line 57-74: Extend TestAggregateResults_Counts to assert that
aggregateResults populates Additions and Deletions for the completedResult("a",
true, "old", "new") case (use agg.Additions and agg.Deletions) so countDiffLines
is exercised, and/or add a new focused TestCountDiffLines table-driven test that
calls countDiffLines with cases: identical inputs -> 0/0, pure additions, pure
deletions, mixed changes, empty strings, and lines that begin with diff markers
like "+---" or "-+++" to ensure those are counted correctly; locate the logic in
aggregateResults and the countDiffLines function to wire inputs via
completedResult helpers and assert expected numeric results.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`:
- Around line 157-182: The countDiffLines function is redundantly recomputing a
unified diff (Context:0) while formatAgentSection already generates a unified
diff (Context:3); refactor so the unified diff is computed once and its +/− line
counts are reused: have formatAgentSection (or the diff-producing step) return
or attach the unified-diff string and computed adds/dels to the agentResult
(e.g., add fields like AddedLines/DeletedLines or a CachedDiff string), compute
adds/dels by scanning the already-produced Context:3 diff instead of calling
difflib.GetUnifiedDiffString again, and update callers that currently call
countDiffLines to read the cached totals; remove the duplicate
GetUnifiedDiffString invocation in countDiffLines or inline its logic to use the
cached diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a1aec19-417f-4c96-83c3-f7d414e2bb70
📒 Files selected for processing (2)
apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.goapps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go
|
|
||
| diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions) | ||
| if a.Errored > 0 { | ||
| return fmt.Sprintf( | ||
| "%d errored, %d changed, %d unchanged, %d unsupported", | ||
| a.Errored, a.Changed, a.Unchanged, a.Unsupported, | ||
| ) | ||
| return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored) | ||
| } | ||
| if a.Changed > 0 { | ||
| return fmt.Sprintf( | ||
| "%d changed, %d unchanged, %d unsupported", | ||
| a.Changed, a.Unchanged, a.Unsupported, | ||
| ) | ||
| } | ||
| if a.Unsupported > 0 { | ||
| return fmt.Sprintf( | ||
| "No changes (%d unsupported)", | ||
| a.Unsupported, | ||
| ) | ||
| } | ||
| return "No changes" | ||
| return diffSummary |
There was a problem hiding this comment.
UX: "+0 -0" replaces the old "No changes" summary.
When every agent reports HasChanges=false, the title now reads +0 -0 (confirmed by the "final no changes shows zero diff counts" and "final all clean" test cases). That's technically correct, but arguably less informative than the prior "No changes" / human-readable summary. If intentional per the linked issue (#1033 asks specifically for line totals), feel free to ignore; otherwise consider falling back to "No changes" when a.Additions == 0 && a.Deletions == 0 && a.Changed == 0.
Optional fallback
diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions)
if a.Errored > 0 {
return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored)
}
+ if a.Additions == 0 && a.Deletions == 0 && a.Changed == 0 {
+ return "No changes"
+ }
return diffSummary📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions) | |
| if a.Errored > 0 { | |
| return fmt.Sprintf( | |
| "%d errored, %d changed, %d unchanged, %d unsupported", | |
| a.Errored, a.Changed, a.Unchanged, a.Unsupported, | |
| ) | |
| return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored) | |
| } | |
| if a.Changed > 0 { | |
| return fmt.Sprintf( | |
| "%d changed, %d unchanged, %d unsupported", | |
| a.Changed, a.Unchanged, a.Unsupported, | |
| ) | |
| } | |
| if a.Unsupported > 0 { | |
| return fmt.Sprintf( | |
| "No changes (%d unsupported)", | |
| a.Unsupported, | |
| ) | |
| } | |
| return "No changes" | |
| return diffSummary | |
| diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions) | |
| if a.Errored > 0 { | |
| return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored) | |
| } | |
| if a.Additions == 0 && a.Deletions == 0 && a.Changed == 0 { | |
| return "No changes" | |
| } | |
| return diffSummary |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`
around lines 264 - 269, The diff summary currently returns "+0 -0" when there
are no changes; update the logic in the formatting function that produces
diffSummary (the block using a.Additions, a.Deletions, a.Errored) to first
detect the all-clean case (a.Additions == 0 && a.Deletions == 0 && a.Changed ==
0) and return "No changes" in that case, otherwise build the "+%d -%d" summary
as before and still append the "(%d errored)" suffix when a.Errored > 0; keep
references to the same variables (a.Additions, a.Deletions, a.Changed,
a.Errored) so the change is a simple conditional early-return.
There was a problem hiding this comment.
Pull request overview
Updates the GitHub check run title for deployment plan results to show total line additions/deletions (per issue #1033) instead of changed/unchanged/unsupported counts.
Changes:
- Add diff line aggregation fields (
Additions,Deletions) to the resultaggregate. - Compute total added/deleted line counts across agents with changes and render as
+A -DincheckTitle(). - Update unit tests to assert the new title formatting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go | Adds diff line counting and updates check title formatting to show +additions -deletions. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go | Updates checkTitle test cases to match the new diff-count-based summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ | ||
| A: difflib.SplitLines(current), | ||
| B: difflib.SplitLines(proposed), | ||
| FromFile: "current", | ||
| ToFile: "proposed", | ||
| Context: 0, | ||
| }) | ||
| if err != nil { | ||
| return 0, 0 | ||
| } | ||
| var adds, dels int | ||
| for line := range strings.SplitSeq(diff, "\n") { | ||
| if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(line, "+") { | ||
| adds++ | ||
| continue | ||
| } | ||
| if strings.HasPrefix(line, "-") { | ||
| dels++ | ||
| } | ||
| } |
There was a problem hiding this comment.
countDiffLines generates a full unified diff string and then scans it to count additions/deletions. This introduces a second diff computation per changed agent (the diff is already computed again in formatAgentSection when rendering output), which can materially increase CPU/memory use for large plans. Consider counting via difflib opcodes (without building a unified diff string), or computing the diff once per agent and reusing it for both the rendered diff and the line counts.
| diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ | |
| A: difflib.SplitLines(current), | |
| B: difflib.SplitLines(proposed), | |
| FromFile: "current", | |
| ToFile: "proposed", | |
| Context: 0, | |
| }) | |
| if err != nil { | |
| return 0, 0 | |
| } | |
| var adds, dels int | |
| for line := range strings.SplitSeq(diff, "\n") { | |
| if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") { | |
| continue | |
| } | |
| if strings.HasPrefix(line, "+") { | |
| adds++ | |
| continue | |
| } | |
| if strings.HasPrefix(line, "-") { | |
| dels++ | |
| } | |
| } | |
| currentLines := difflib.SplitLines(current) | |
| proposedLines := difflib.SplitLines(proposed) | |
| if len(currentLines) == 0 { | |
| return len(proposedLines), 0 | |
| } | |
| if len(proposedLines) == 0 { | |
| return 0, len(currentLines) | |
| } | |
| // Count additions/deletions without building a unified diff string. | |
| // The number of deleted lines is the number of lines in current that are | |
| // not part of the longest common subsequence (LCS); similarly for added | |
| // lines in proposed. This preserves line-based counting semantics while | |
| // avoiding the extra allocation and scan of a rendered diff. | |
| prev := make([]int, len(proposedLines)+1) | |
| curr := make([]int, len(proposedLines)+1) | |
| for i := 1; i <= len(currentLines); i++ { | |
| for j := 1; j <= len(proposedLines); j++ { | |
| if currentLines[i-1] == proposedLines[j-1] { | |
| curr[j] = prev[j-1] + 1 | |
| } else if prev[j] >= curr[j-1] { | |
| curr[j] = prev[j] | |
| } else { | |
| curr[j] = curr[j-1] | |
| } | |
| } | |
| prev, curr = curr, prev | |
| clear(curr) | |
| } | |
| lcs := prev[len(proposedLines)] | |
| adds := len(proposedLines) - lcs | |
| dels := len(currentLines) - lcs |
| adds, dels := countDiffLines(r.Current, r.Proposed) | ||
| a.Additions += adds | ||
| a.Deletions += dels |
There was a problem hiding this comment.
New behavior aggregates Additions/Deletions based on diffs, but the tests currently only assert the final title string and do not verify that aggregateResults computes Additions/Deletions correctly across multiple agents / multi-line changes. Add a focused unit test (and/or tests for countDiffLines) that asserts expected additions/deletions counts for representative diffs and ensures unchanged/unsupported/errored agents don’t affect the totals.
| adds, dels := countDiffLines(r.Current, r.Proposed) | |
| a.Additions += adds | |
| a.Deletions += dels | |
| if r.Status == db.DeploymentPlanTargetStatusCompleted { | |
| adds, dels := countDiffLines(r.Current, r.Proposed) | |
| a.Additions += adds | |
| a.Deletions += dels | |
| } |
fixes #1033
Summary by CodeRabbit
New Features
Tests