Skip to content

Commit e95ef38

Browse files
DavidS-ovmactions-user
DavidS-ovm
authored and
actions-user
committed
fix, cli uses get change's changeAnalysisStatus (#3842)
https://github.com/user-attachments/assets/a56fe17e-cb1a-4520-9368-76b965a998e7 <!-- CURSOR_SUMMARY --> > [!NOTE] > **Medium Risk** > Touches end-to-end polling/control-flow for change analysis completion and risk retrieval in both CLI UX and server-side run-task execution; mistakes could cause hangs, premature exits, or incorrect failure/retry behavior. > > **Overview** > **Stops using `GetChangeTimelineV2` to detect change-analysis completion** in multiple CLI commands and the API server run-task worker, and instead polls `GetChange` and inspects `change.metadata.change_analysis_status` (handling DONE/SKIPPED/ERROR/in-progress states). > > In `terraform plan` and the run-task flow, **risk extraction is decoupled from timeline entries** by calling `GetChangeRisks` after analysis completes, with added nil checks and updated error handling/messages (including retry vs. fail semantics in the worker). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit cbdf9ebb49ce6eeb6b981499960b665d2c525329. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> GitOrigin-RevId: fa48bdae414cbba13288d9f798d506abe0017728
1 parent 1e3ae17 commit e95ef38

4 files changed

Lines changed: 112 additions & 141 deletions

File tree

‎cmd/changes_get_change.go‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -72,47 +72,43 @@ func GetChange(cmd *cobra.Command, args []string) error {
7272
}
7373

7474
client := AuthenticatedChangesClient(ctx, oi)
75-
var timeLine *sdp.GetChangeTimelineV2Response
7675
fetch:
7776
for {
78-
rawTimeLine, timelineErr := client.GetChangeTimelineV2(ctx, &connect.Request[sdp.GetChangeTimelineV2Request]{
79-
Msg: &sdp.GetChangeTimelineV2Request{
80-
ChangeUUID: changeUuid[:],
77+
changeRes, err := client.GetChange(ctx, &connect.Request[sdp.GetChangeRequest]{
78+
Msg: &sdp.GetChangeRequest{
79+
UUID: changeUuid[:],
8180
},
8281
})
83-
if timelineErr != nil || rawTimeLine.Msg == nil {
82+
if err != nil || changeRes.Msg == nil || changeRes.Msg.GetChange() == nil {
8483
return loggedError{
85-
err: timelineErr,
84+
err: err,
8685
fields: lf,
87-
message: "failed to get timeline",
86+
message: "failed to get change",
8887
}
8988
}
90-
timeLine = rawTimeLine.Msg
91-
for _, entry := range timeLine.GetEntries() {
92-
// ENG-1993: This is temporary to still track the auto tagging entry in the timeline. this is to prevent the cli from hanging
93-
if entry.GetName() == sdp.ChangeTimelineEntryV2IDAutoTagging.Name && entry.GetStatus() == sdp.ChangeTimelineEntryStatus_DONE {
94-
break fetch
89+
ch := changeRes.Msg.GetChange()
90+
md := ch.GetMetadata()
91+
if md == nil || md.GetChangeAnalysisStatus() == nil {
92+
return loggedError{
93+
err: fmt.Errorf("change metadata or change analysis status is nil"),
94+
fields: lf,
95+
message: "failed to get change",
9596
}
9697
}
97-
// display the running entry
98-
runningEntry, contentDescription, status, err := sdp.TimelineFindInProgressEntry(timeLine.GetEntries())
99-
if err != nil {
98+
status := md.GetChangeAnalysisStatus().GetStatus()
99+
switch status {
100+
case sdp.ChangeAnalysisStatus_STATUS_DONE, sdp.ChangeAnalysisStatus_STATUS_SKIPPED:
101+
break fetch
102+
case sdp.ChangeAnalysisStatus_STATUS_ERROR:
100103
return loggedError{
101-
err: err,
104+
err: fmt.Errorf("change analysis completed with error status"),
102105
fields: lf,
103-
message: "failed to find running entry",
106+
message: "change analysis failed",
104107
}
108+
case sdp.ChangeAnalysisStatus_STATUS_UNSPECIFIED, sdp.ChangeAnalysisStatus_STATUS_INPROGRESS:
109+
log.WithContext(ctx).WithFields(lf).WithField("status", status.String()).Info("Waiting for change analysis to complete")
105110
}
106-
// find the running timeline entry
107-
log.WithContext(ctx).WithFields(log.Fields{
108-
"status": status.String(),
109-
"running": runningEntry,
110-
"content": contentDescription,
111-
}).Info("Waiting for change analysis to complete")
112-
// retry
113111
time.Sleep(3 * time.Second)
114-
115-
// check if the context is cancelled
116112
if ctx.Err() != nil {
117113
return loggedError{
118114
err: ctx.Err(),

‎cmd/changes_get_signals.go‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -55,47 +55,43 @@ func GetSignals(cmd *cobra.Command, args []string) error {
5555
}
5656

5757
client := AuthenticatedChangesClient(ctx, oi)
58-
var timeLine *sdp.GetChangeTimelineV2Response
5958
fetch:
6059
for {
61-
rawTimeLine, timelineErr := client.GetChangeTimelineV2(ctx, &connect.Request[sdp.GetChangeTimelineV2Request]{
62-
Msg: &sdp.GetChangeTimelineV2Request{
63-
ChangeUUID: changeUuid[:],
60+
changeRes, err := client.GetChange(ctx, &connect.Request[sdp.GetChangeRequest]{
61+
Msg: &sdp.GetChangeRequest{
62+
UUID: changeUuid[:],
6463
},
6564
})
66-
if timelineErr != nil || rawTimeLine.Msg == nil {
65+
if err != nil || changeRes.Msg == nil || changeRes.Msg.GetChange() == nil {
6766
return loggedError{
68-
err: timelineErr,
67+
err: err,
6968
fields: lf,
70-
message: "failed to get timeline",
69+
message: "failed to get change",
7170
}
7271
}
73-
timeLine = rawTimeLine.Msg
74-
for _, entry := range timeLine.GetEntries() {
75-
// ENG-1993: This is temporary to still track the auto tagging entry in the timeline. this is to prevent the cli from hanging
76-
if entry.GetName() == sdp.ChangeTimelineEntryV2IDAutoTagging.Name && entry.GetStatus() == sdp.ChangeTimelineEntryStatus_DONE {
77-
break fetch
72+
ch := changeRes.Msg.GetChange()
73+
md := ch.GetMetadata()
74+
if md == nil || md.GetChangeAnalysisStatus() == nil {
75+
return loggedError{
76+
err: fmt.Errorf("change metadata or change analysis status is nil"),
77+
fields: lf,
78+
message: "failed to get change",
7879
}
7980
}
80-
// display the running entry
81-
runningEntry, contentDescription, status, err := sdp.TimelineFindInProgressEntry(timeLine.GetEntries())
82-
if err != nil {
81+
status := md.GetChangeAnalysisStatus().GetStatus()
82+
switch status {
83+
case sdp.ChangeAnalysisStatus_STATUS_DONE, sdp.ChangeAnalysisStatus_STATUS_SKIPPED:
84+
break fetch
85+
case sdp.ChangeAnalysisStatus_STATUS_ERROR:
8386
return loggedError{
84-
err: err,
87+
err: fmt.Errorf("change analysis completed with error status"),
8588
fields: lf,
86-
message: "failed to find running entry",
89+
message: "change analysis failed",
8790
}
91+
case sdp.ChangeAnalysisStatus_STATUS_UNSPECIFIED, sdp.ChangeAnalysisStatus_STATUS_INPROGRESS:
92+
log.WithContext(ctx).WithFields(lf).WithField("status", status.String()).Info("Waiting for change analysis to complete")
8893
}
89-
// find the running timeline entry
90-
log.WithContext(ctx).WithFields(log.Fields{
91-
"status": status.String(),
92-
"running": runningEntry,
93-
"content": contentDescription,
94-
}).Info("Waiting for change analysis to complete")
95-
// retry
9694
time.Sleep(3 * time.Second)
97-
98-
// check if the context is cancelled
9995
if ctx.Err() != nil {
10096
return loggedError{
10197
err: ctx.Err(),

‎cmd/changes_start_change.go‎

Lines changed: 24 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
package cmd
22

33
import (
4-
"regexp"
4+
"fmt"
55
"time"
66

77
"connectrpc.com/connect"
@@ -43,56 +43,45 @@ func StartChange(cmd *cobra.Command, args []string) error {
4343
"ticket-link": viper.GetString("ticket-link"),
4444
}
4545

46-
// poll the timeline for the Calculated Blast Radius to be complete
46+
// wait for change analysis to complete (poll GetChange by change_analysis_status)
4747
client := AuthenticatedChangesClient(ctx, oi)
4848
fetch:
4949
for {
50-
rawTimeLine, timelineErr := client.GetChangeTimelineV2(ctx, &connect.Request[sdp.GetChangeTimelineV2Request]{
51-
Msg: &sdp.GetChangeTimelineV2Request{
52-
ChangeUUID: changeUuid[:],
50+
changeRes, err := client.GetChange(ctx, &connect.Request[sdp.GetChangeRequest]{
51+
Msg: &sdp.GetChangeRequest{
52+
UUID: changeUuid[:],
5353
},
5454
})
55-
if timelineErr != nil || rawTimeLine.Msg == nil {
55+
if err != nil || changeRes.Msg == nil || changeRes.Msg.GetChange() == nil {
5656
return loggedError{
57-
err: timelineErr,
57+
err: err,
5858
fields: lf,
59-
message: "failed to get timeline",
59+
message: "failed to get change",
6060
}
6161
}
62-
timeLine := rawTimeLine.Msg
63-
// Use a case-insensitive regex to match any entry containing "blast radius"
64-
blastRadiusRegex := regexp.MustCompile(`(?i)blast\s+radius`)
65-
for _, entry := range timeLine.GetEntries() {
66-
if blastRadiusRegex.MatchString(entry.GetName()) {
67-
if entry.GetStatus() == sdp.ChangeTimelineEntryStatus_DONE {
68-
break fetch
69-
}
70-
if entry.GetStatus() == sdp.ChangeTimelineEntryStatus_ERROR {
71-
// the api server will retry the blast radius calculation, so lets wait for the retry
72-
log.WithContext(ctx).WithFields(lf).Warn("Blast radius calculation failed, waiting for retry")
73-
break
74-
}
62+
ch := changeRes.Msg.GetChange()
63+
md := ch.GetMetadata()
64+
if md == nil || md.GetChangeAnalysisStatus() == nil {
65+
return loggedError{
66+
err: fmt.Errorf("change metadata or change analysis status is nil"),
67+
fields: lf,
68+
message: "failed to get change",
7569
}
7670
}
77-
// display the running entry
78-
runningEntry, contentDescription, status, err := sdp.TimelineFindInProgressEntry(timeLine.GetEntries())
79-
if err != nil {
71+
status := md.GetChangeAnalysisStatus().GetStatus()
72+
switch status {
73+
case sdp.ChangeAnalysisStatus_STATUS_DONE, sdp.ChangeAnalysisStatus_STATUS_SKIPPED:
74+
break fetch
75+
case sdp.ChangeAnalysisStatus_STATUS_ERROR:
8076
return loggedError{
81-
err: err,
77+
err: fmt.Errorf("change analysis completed with error status"),
8278
fields: lf,
83-
message: "failed to find running entry",
79+
message: "change analysis failed",
8480
}
81+
case sdp.ChangeAnalysisStatus_STATUS_UNSPECIFIED, sdp.ChangeAnalysisStatus_STATUS_INPROGRESS:
82+
log.WithContext(ctx).WithFields(lf).WithField("status", status.String()).Info("Waiting for change analysis to complete")
8583
}
86-
// log progress while waiting for blast radius calculation
87-
log.WithContext(ctx).WithFields(log.Fields{
88-
"status": status.String(),
89-
"running": runningEntry,
90-
"content": contentDescription,
91-
}).Info("Waiting for blast radius to be calculated")
92-
// retry
9384
time.Sleep(3 * time.Second)
94-
95-
// check if the context is cancelled
9685
if ctx.Err() != nil {
9786
return loggedError{
9887
err: ctx.Err(),

‎cmd/terraform_plan.go‎

Lines changed: 44 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -336,73 +336,63 @@ func TerraformPlanImpl(ctx context.Context, cmd *cobra.Command, oi sdp.OvermindI
336336
log.WithField("change-url", changeUrl.String()).Info("Change ready")
337337

338338
///////////////////////////////////////////////////////////////////
339-
// wait for change analysis to complete
339+
// wait for change analysis to complete (poll GetChange by change_analysis_status)
340340
///////////////////////////////////////////////////////////////////
341341
changeAnalysisSpinner, _ := pterm.DefaultSpinner.WithWriter(multi.NewWriter()).Start("Change Analysis")
342342

343-
var timeLine *sdp.GetChangeTimelineV2Response
344-
milestoneSpinners := []*pterm.SpinnerPrinter{}
345343
retryLoop:
346344
for {
347-
rawTimeLine, timelineErr := client.GetChangeTimelineV2(ctx, &connect.Request[sdp.GetChangeTimelineV2Request]{
348-
Msg: &sdp.GetChangeTimelineV2Request{
349-
ChangeUUID: changeUuid[:],
345+
changeRes, err := client.GetChange(ctx, &connect.Request[sdp.GetChangeRequest]{
346+
Msg: &sdp.GetChangeRequest{
347+
UUID: changeUuid[:],
350348
},
351349
})
352-
if timelineErr != nil || rawTimeLine.Msg == nil {
353-
changeAnalysisSpinner.Fail(fmt.Sprintf("Change Analysis failed to get timeline: %v", timelineErr))
354-
return nil
350+
if err != nil {
351+
changeAnalysisSpinner.Fail(fmt.Sprintf("Change Analysis failed to get change: %v", err))
352+
return fmt.Errorf("failed to get change during change analysis: %w", err)
355353
}
356-
timeLine = rawTimeLine.Msg
357-
358-
// display the status for the timeline entries
359-
for i, entry := range timeLine.GetEntries() {
360-
// populate the spinner list on the first run
361-
if i <= len(milestoneSpinners) {
362-
milestoneSpinners = append(milestoneSpinners, pterm.DefaultSpinner.
363-
WithWriter(multi.NewWriter()).
364-
WithIndentation(IndentSymbol()).
365-
WithText(entry.GetName()))
366-
}
367-
// render the spinner for this entry
368-
switch entry.GetStatus() {
369-
case sdp.ChangeTimelineEntryStatus_PENDING:
370-
continue
371-
case sdp.ChangeTimelineEntryStatus_IN_PROGRESS:
372-
if !milestoneSpinners[i].IsActive {
373-
milestoneSpinners[i], _ = milestoneSpinners[i].Start()
374-
}
375-
case sdp.ChangeTimelineEntryStatus_ERROR:
376-
milestoneSpinners[i].Fail()
377-
case sdp.ChangeTimelineEntryStatus_DONE:
378-
milestoneSpinners[i].Success()
379-
case sdp.ChangeTimelineEntryStatus_UNSPECIFIED:
380-
// do nothing
381-
default:
382-
milestoneSpinners[i].Fail(fmt.Sprintf("Unknown status: %v", entry.GetStatus()))
383-
}
384-
385-
// ENG-1993: This is temporary to still track the auto tagging entry in the timeline. this is to prevent the cli from hanging
386-
// check if change analysis is done
387-
if entry.GetName() == sdp.ChangeTimelineEntryV2IDAutoTagging.Name && entry.GetStatus() == sdp.ChangeTimelineEntryStatus_DONE {
388-
changeAnalysisSpinner.Success()
389-
break retryLoop
390-
}
354+
if changeRes.Msg == nil || changeRes.Msg.GetChange() == nil {
355+
changeAnalysisSpinner.Fail("Change Analysis failed: received empty change response")
356+
return fmt.Errorf("change analysis failed: received empty change response")
357+
}
358+
ch := changeRes.Msg.GetChange()
359+
md := ch.GetMetadata()
360+
if md == nil || md.GetChangeAnalysisStatus() == nil {
361+
changeAnalysisSpinner.Fail("Change Analysis failed: change metadata or analysis status missing")
362+
return fmt.Errorf("change analysis failed: change metadata or change analysis status is nil")
363+
}
364+
status := md.GetChangeAnalysisStatus().GetStatus()
365+
switch status {
366+
case sdp.ChangeAnalysisStatus_STATUS_DONE, sdp.ChangeAnalysisStatus_STATUS_SKIPPED:
367+
changeAnalysisSpinner.Success()
368+
break retryLoop
369+
case sdp.ChangeAnalysisStatus_STATUS_ERROR:
370+
changeAnalysisSpinner.Fail("Change analysis failed")
371+
return fmt.Errorf("change analysis completed with error status")
372+
case sdp.ChangeAnalysisStatus_STATUS_UNSPECIFIED, sdp.ChangeAnalysisStatus_STATUS_INPROGRESS:
373+
// keep polling
391374
}
392-
// retry
393375
time.Sleep(3 * time.Second)
394-
}
395-
var calculateRiskStep *sdp.ChangeTimelineEntryV2
396-
for _, entry := range timeLine.GetEntries() {
397-
if entry.GetName() == sdp.ChangeTimelineEntryV2IDCalculatedRisks.Name {
398-
calculateRiskStep = entry
399-
break
376+
if ctx.Err() != nil {
377+
changeAnalysisSpinner.Fail("Cancelled")
378+
return ctx.Err()
400379
}
401380
}
402-
if calculateRiskStep == nil || calculateRiskStep.GetCalculatedRisks() == nil {
403-
return fmt.Errorf("Failed to get calculated risks")
381+
risksRes, err := client.GetChangeRisks(ctx, &connect.Request[sdp.GetChangeRisksRequest]{
382+
Msg: &sdp.GetChangeRisksRequest{
383+
UUID: changeUuid[:],
384+
},
385+
})
386+
if err != nil {
387+
return fmt.Errorf("failed to get calculated risks: %w", err)
388+
}
389+
if risksRes.Msg == nil {
390+
return fmt.Errorf("failed to get calculated risks: response message was nil")
391+
}
392+
if risksRes.Msg.GetChangeRiskMetadata() == nil {
393+
return fmt.Errorf("failed to get calculated risks: change risk metadata was nil")
404394
}
405-
calculatedRisks := calculateRiskStep.GetCalculatedRisks().GetRisks()
395+
calculatedRisks := risksRes.Msg.GetChangeRiskMetadata().GetRisks()
406396
// Submit milestone for tracing
407397
if cmdSpan != nil {
408398
cmdSpan.AddEvent("Change Analysis finished", trace.WithAttributes(

0 commit comments

Comments
 (0)