skipped status checks #2109

motatoes · 2025-08-23T01:43:32Z

update status check to use new api
moving to modern checked status

greptile-apps

Greptile Summary

This PR modernizes Digger's GitHub status check system by migrating from GitHub's legacy Status API to the newer Check Runs API. The changes encompass several key improvements:

Core API Migration: The GitHub integration in libs/ci/github/github.go has been completely rewritten to use GitHub's Check Runs API instead of the deprecated Status API. This removes the 140-character description limit and provides better UI integration with GitHub's pull request interface.

Status Mapping Improvements: Critical fixes were made to status mappings in libs/scheduler/models.go, where BatchJobFailed was incorrectly returning 'success' instead of 'failure', and BatchJobInvalidated now returns 'cancelled' instead of 'failure' for better semantic accuracy.

Enhanced Status Granularity: The new system introduces more nuanced status states like 'neutral' and 'skipped' that better represent different job scenarios. When only plan jobs are running, the apply status is set to 'neutral', and when no jobs are present, both plan and apply are marked as 'skipped' rather than 'success'.

Centralized Status Management: Status check logic has been moved from individual CLI components to centralized backend functions (UpdateCheckStatusForBatch and UpdateCheckStatusForJob), providing better consistency and control over status reporting.

Database Persistence Fix: A critical bug was fixed in backend/models/storage.go where batch status updates were being set in memory but not persisted to the database, potentially causing inconsistencies in job tracking.

Legacy Code Cleanup: Individual project-level status checks in the CLI have been commented out as the system transitions to the new centralized approach, reducing status check clutter in the GitHub PR interface.

The changes maintain backward compatibility while providing a foundation for more sophisticated status reporting that aligns with modern GitHub integration best practices.

Confidence score: 3/5

This PR has potential breaking changes and incomplete implementation that could impact production status reporting
Score reflects concerns about commented-out error handling, missing status mappings, and potential race conditions in concurrent scenarios
Pay close attention to GitHub integration files and status check logic for proper error handling and complete status coverage

Context used:

Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)

_{9 files reviewed, 7 comments}

_{Edit Code Review Bot Settings | Greptile}

greptile-apps · 2025-08-23T01:44:02Z

backend/models/storage.go

@@ -856,6 +856,7 @@ func (db *Database) UpdateBatchStatus(batch *DiggerBatch) error {
 	}
 	if allJobsSucceeded == true {
 		batch.Status = scheduler.BatchJobSucceeded
+		db.GormDB.Save(batch)


style: Consider adding error handling for the save operation to log any potential database errors.

Suggested change

db.GormDB.Save(batch)

if err := db.GormDB.Save(batch).Error; err != nil {

slog.Error("failed to update batch status to succeeded",

"batchId", batchId,

"error", err)

return err

}

greptile-apps · 2025-08-23T01:44:06Z

backend/models/scheduler.go

+	case orchestrator_scheduler.DiggerJobFailed:
+		return "failed", nil
+	}
+	return "", fmt.Errorf("unknown job status: %s", job.Status)


syntax: The error message uses %s format specifier but job.Status is an int8, should use %d instead for proper formatting.

Suggested change

return "", fmt.Errorf("unknown job status: %s", job.Status)

return "", fmt.Errorf("unknown job status: %d", job.Status)

greptile-apps · 2025-08-23T01:44:17Z

libs/ci/github/github.go

+		Name:    statusContext, // similar to "Context" in Status API
+		HeadSHA: pr.GetHead().GetSHA(),
+		Status:  github.String(checkStatus), // required when setting a conclusion
+		//Conclusion: github.String(conclusion),


style: Commented-out conclusion suggests incomplete implementation - should this be removed or is there a reason it's commented?

greptile-apps · 2025-08-23T01:44:26Z

backend/utils/github.go

@@ -210,6 +210,7 @@ func SetPRStatusForJobs(prService ci.PullRequestService, prNumber int, jobs []sc
 		if scheduler.IsPlanJobs(jobs) {
 			slog.Debug("Setting aggregate plan status", "prNumber", prNumber)
 			err = prService.SetStatus(prNumber, "pending", "digger/plan")
+			err = prService.SetStatus(prNumber, "neutral", "digger/apply")


logic: Error handling issue: the error from setting 'digger/apply' status overwrites the error from setting 'digger/plan' status. If the first call succeeds but the second fails, the original error is lost.

greptile-apps · 2025-08-23T01:44:27Z

backend/utils/github.go

@@ -224,13 +225,13 @@ func SetPRStatusForJobs(prService ci.PullRequestService, prNumber int, jobs []sc
 	} else {
 		slog.Debug("Setting success status for empty job list", "prNumber", prNumber)

-		err := prService.SetStatus(prNumber, "success", "digger/plan")
+		err := prService.SetStatus(prNumber, "skipped", "digger/plan")
 		if err != nil {
 			slog.Error("Failed to set success plan status", "prNumber", prNumber, "error", err)


style: Log message still says 'success plan status' but now setting 'skipped' status - should update message to match

Suggested change

slog.Error("Failed to set success plan status", "prNumber", prNumber, "error", err)

slog.Error("Failed to set skipped plan status", "prNumber", prNumber, "error", err)

greptile-apps · 2025-08-23T01:44:28Z

backend/utils/github.go

 		if err != nil {
 			slog.Error("Failed to set success plan status", "prNumber", prNumber, "error", err)
 			return fmt.Errorf("error setting pr status: %v", err)
 		}

-		err = prService.SetStatus(prNumber, "success", "digger/apply")
+		err = prService.SetStatus(prNumber, "skipped", "digger/apply")
 		if err != nil {
 			slog.Error("Failed to set success apply status", "prNumber", prNumber, "error", err)


style: Log message still says 'success apply status' but now setting 'skipped' status - should update message to match

Suggested change

slog.Error("Failed to set success apply status", "prNumber", prNumber, "error", err)

slog.Error("Failed to set skipped apply status", "prNumber", prNumber, "error", err)

greptile-apps · 2025-08-23T01:44:46Z

cli/pkg/digger/digger.go

@@ -275,7 +275,7 @@ func run(command string, job orchestrator.Job, policyChecker policy.Checker, org
 		if err != nil {
 			msg := fmt.Sprintf("Failed to Run digger plan command. %v", err)
 			slog.Error("Failed to Run digger plan command", "error", err)
-			err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/plan")
+			//err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/plan")


syntax: Same issue - error handling logic remains for commented status check call, creating unreachable code.

Suggested change

//err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/plan")

//err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/plan")

if err != nil {

msg := fmt.Sprintf("Failed to set PR status. %v", err)

return nil, msg, fmt.Errorf("%s", msg)

}

bismuthdev · 2025-08-23T02:15:22Z

backend/models/scheduler.go

+func GetStatusCheckForJob(job *DiggerJob) (string, error) {
+	switch job.Status {
+	case orchestrator_scheduler.DiggerJobStarted:
+		return "pending", nil
+	case orchestrator_scheduler.DiggerJobTriggered:
+		return "pending", nil
+	case orchestrator_scheduler.DiggerJobCreated:
+		return "pending", nil
+	case orchestrator_scheduler.DiggerJobSucceeded:
+		return "success", nil
+	case orchestrator_scheduler.DiggerJobFailed:
+		return "failed", nil
+	}
+	return "", fmt.Errorf("unknown job status: %s", job.Status)
+}


The GetStatusCheckForJob function in backend/models/scheduler.go is missing a case for the DiggerJobQueuedForRun status that is defined in libs/scheduler/models.go. Additionally, it doesn't have a proper default case to handle any future job statuses that might be added.

The current implementation has a fallback return statement outside the switch statement, but this is not as clear as having an explicit default case within the switch. If a new job status is added in the future, the function will return an error rather than providing a sensible default behavior.

I've added the missing case for DiggerJobQueuedForRun and converted the fallback to a proper default case within the switch statement to make the code more maintainable and future-proof.

Suggested change

func GetStatusCheckForJob(job *DiggerJob) (string, error) {

switch job.Status {

case orchestrator_scheduler.DiggerJobStarted:

return "pending", nil

case orchestrator_scheduler.DiggerJobTriggered:

return "pending", nil

case orchestrator_scheduler.DiggerJobCreated:

return "pending", nil

case orchestrator_scheduler.DiggerJobSucceeded:

return "success", nil

case orchestrator_scheduler.DiggerJobFailed:

return "failed", nil

}

return "", fmt.Errorf("unknown job status: %s", job.Status)

}

func GetStatusCheckForJob(job *DiggerJob) (string, error) {

switch job.Status {

case orchestrator_scheduler.DiggerJobStarted:

return "pending", nil

case orchestrator_scheduler.DiggerJobTriggered:

return "pending", nil

case orchestrator_scheduler.DiggerJobCreated:

return "pending", nil

case orchestrator_scheduler.DiggerJobSucceeded:

return "success", nil

case orchestrator_scheduler.DiggerJobFailed:

return "failed", nil

case orchestrator_scheduler.DiggerJobQueuedForRun:

return "pending", nil

default:

return "", fmt.Errorf("unknown job status: %s", job.Status)

}

}

bismuthdev · 2025-08-23T02:15:27Z

backend/models/scheduler.go

+	case orchestrator_scheduler.DiggerJobFailed:
+		return "failed", nil


The GetStatusCheckForJob function in backend/models/scheduler.go returns "failed" for failed jobs, but GitHub's status API expects "failure" as the status value.

This is confirmed by examining the GitHub implementation in libs/ci/github/github.go where the SetStatus method maps status values to GitHub's check run API. The interface in libs/ci/ci.go also documents that the status should be one of "pending", "failure", or "success".

This mismatch could cause issues when reporting job statuses to GitHub, as the status value "failed" is not properly handled by GitHub's API.

Suggested change

case orchestrator_scheduler.DiggerJobFailed:

return "failed", nil

case orchestrator_scheduler.DiggerJobFailed:

return "failure", nil

bismuthdev · 2025-08-23T02:15:36Z

cli/pkg/digger/digger.go

+				//err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/apply")
 				if err != nil {
 					msg := fmt.Sprintf("Failed to set PR status. %v", err)
 					return nil, msg, fmt.Errorf("%s", msg)
 				}


This is another instance of the same issue in the file. The call to prService.SetStatus() on line 424 is commented out, but the error check for this call is still active. This creates a logical inconsistency since err is being checked but never set by the commented-out function call.

This is part of the same pattern of issues where status setting calls have been commented out but their error handling remains active. This could lead to confusion during maintenance and potentially cause runtime issues if the variable err happens to have a non-nil value from a previous operation.

Suggested change

//err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/apply")

if err != nil {

msg := fmt.Sprintf("Failed to set PR status. %v", err)

return nil, msg, fmt.Errorf("%s", msg)

}

// SetStatus calls have been commented out but error checks remain

// Commenting out the error check as well since there's no error to check

// if err != nil {

// msg := fmt.Sprintf("Failed to set PR status. %v", err)

// return nil, msg, fmt.Errorf("%s", msg)

// }

bismuthdev · 2025-08-23T02:15:44Z

libs/ci/github/github.go

+	var conclusion string
+	if status == "pending" {
+		checkStatus = "in_progress"
+		conclusion = ""
+	} else {
+		checkStatus = "completed"
+		conclusion = status
+	}


The GitHub Checks API doesn't directly support the "skipped" status value that was previously used with the Status API. In the current implementation, when the status is "skipped", it's being mapped to the same conclusion value, which isn't a valid conclusion for the Checks API.

According to GitHub's documentation, the valid conclusion values for the Checks API are: "action_required", "cancelled", "failure", "neutral", "success", "skipped", "stale", or "timed_out".

While "skipped" is a valid conclusion value, the code is directly using the status value as the conclusion, which could lead to issues if other non-standard status values are used. For the "neutral" status, it should also be mapped to the "neutral" conclusion.

This fix ensures that both "neutral" and "skipped" status values are properly mapped to valid conclusion values in the GitHub Checks API.

Suggested change

var conclusion string

if status == "pending" {

checkStatus = "in_progress"

conclusion = ""

} else {

checkStatus = "completed"

conclusion = status

}

var conclusion string

if status == "pending" {

checkStatus = "in_progress"

conclusion = ""

} else if status == "neutral" || status == "skipped" {

checkStatus = "completed"

conclusion = "neutral"

} else {

checkStatus = "completed"

conclusion = status

}

motatoes added 2 commits August 22, 2025 15:57

update status check to use new api

0c2a383

moving to modern checked status

bd80c19

This comment has been minimized.

Sign in to view

greptile-apps bot reviewed Aug 23, 2025

View reviewed changes

bismuthdev bot reviewed Aug 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

skipped status checks #2109

skipped status checks #2109

Uh oh!

motatoes commented Aug 23, 2025

Uh oh!

This comment has been minimized.

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

greptile-apps bot Aug 23, 2025

Uh oh!

bismuthdev bot Aug 23, 2025

Uh oh!

bismuthdev bot Aug 23, 2025

Uh oh!

bismuthdev bot Aug 23, 2025

Uh oh!

bismuthdev bot Aug 23, 2025

Uh oh!

Uh oh!

-		db.GormDB.Save(batch)
+		if err := db.GormDB.Save(batch).Error; err != nil {
+			slog.Error("failed to update batch status to succeeded",
+				"batchId", batchId,
+				"error", err)
+			return err
+		}

	return "", fmt.Errorf("unknown job status: %s", job.Status)
	return "", fmt.Errorf("unknown job status: %d", job.Status)

	slog.Error("Failed to set success plan status", "prNumber", prNumber, "error", err)
	slog.Error("Failed to set skipped plan status", "prNumber", prNumber, "error", err)

	slog.Error("Failed to set success apply status", "prNumber", prNumber, "error", err)
	slog.Error("Failed to set skipped apply status", "prNumber", prNumber, "error", err)

		case orchestrator_scheduler.DiggerJobFailed:
		return "failed", nil

-				//err := prService.SetStatus(*job.PullRequestNumber, "failure", job.GetProjectAlias()+"/apply")
-				if err != nil {
-					msg := fmt.Sprintf("Failed to set PR status. %v", err)
-					return nil, msg, fmt.Errorf("%s", msg)
-				}
+				// SetStatus calls have been commented out but error checks remain
+				// Commenting out the error check as well since there's no error to check
+				// if err != nil {
+				// 	msg := fmt.Sprintf("Failed to set PR status. %v", err)
+				// 	return nil, msg, fmt.Errorf("%s", msg)
+				// }

skipped status checks #2109

Are you sure you want to change the base?

skipped status checks #2109

Uh oh!

Conversation

motatoes commented Aug 23, 2025

Uh oh!

This comment has been minimized.

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Summary

Confidence score: 3/5

Context used:

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

bismuthdev bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

bismuthdev bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

bismuthdev bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

bismuthdev bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!