fix: address sonnet review MINOR findings (#2916)
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 59s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m6s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 59s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m6s
- client.go: fix misleading timer.Stop() comment (finding #1) - pr.go: document all-or-nothing semantics for GetCommitStatuses when check-runs endpoint fails after statuses succeed (finding #2) - files.go: include both array and object unmarshal errors in ListContents fallback error message (finding #3) - pr.go: expand mapCheckRunStatus comment to explain non-blocking policy decision (finding #4)
This commit is contained in:
+1
-1
@@ -225,7 +225,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
|
|||||||
timer := time.NewTimer(delay)
|
timer := time.NewTimer(delay)
|
||||||
select {
|
select {
|
||||||
case <-timer.C:
|
case <-timer.C:
|
||||||
timer.Stop() // no-op after fire, releases runtime resources promptly
|
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
|
||||||
case <-ctx.Done():
|
case <-ctx.Done():
|
||||||
timer.Stop()
|
timer.Stop()
|
||||||
return nil, ctx.Err()
|
return nil, ctx.Err()
|
||||||
|
|||||||
+1
-1
@@ -81,7 +81,7 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
|
|||||||
if err := json.Unmarshal(body, &entries); err != nil {
|
if err := json.Unmarshal(body, &entries); err != nil {
|
||||||
var single entry
|
var single entry
|
||||||
if err2 := json.Unmarshal(body, &single); err2 != nil {
|
if err2 := json.Unmarshal(body, &single); err2 != nil {
|
||||||
return nil, fmt.Errorf("parse contents JSON: %w", err2)
|
return nil, fmt.Errorf("parse contents JSON: as array: %v; as object: %w", err, err2)
|
||||||
}
|
}
|
||||||
// Guard against empty objects ({}) or unexpected shapes that
|
// Guard against empty objects ({}) or unexpected shapes that
|
||||||
// unmarshal successfully but carry no useful data.
|
// unmarshal successfully but carry no useful data.
|
||||||
|
|||||||
+4
-1
@@ -127,6 +127,9 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
|
|||||||
// Returns nil (not an empty slice) when there are no statuses or check runs.
|
// Returns nil (not an empty slice) when there are no statuses or check runs.
|
||||||
// If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
|
// If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
|
||||||
// function returns immediately without attempting the check-runs endpoint.
|
// function returns immediately without attempting the check-runs endpoint.
|
||||||
|
// If the check-runs endpoint fails after statuses were fetched successfully,
|
||||||
|
// the function returns an error (not a partial result) so callers always get
|
||||||
|
// either a complete view or a clear error signal.
|
||||||
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
|
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
|
||||||
var result []vcs.CommitStatus
|
var result []vcs.CommitStatus
|
||||||
|
|
||||||
@@ -192,7 +195,7 @@ func mapCheckRunStatus(conclusion *string) string {
|
|||||||
case "failure", "action_required", "timed_out":
|
case "failure", "action_required", "timed_out":
|
||||||
return "failure"
|
return "failure"
|
||||||
case "cancelled", "skipped", "neutral":
|
case "cancelled", "skipped", "neutral":
|
||||||
return "success" // non-blocking
|
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
|
||||||
case "stale", "waiting":
|
case "stale", "waiting":
|
||||||
return "pending"
|
return "pending"
|
||||||
default:
|
default:
|
||||||
|
|||||||
Reference in New Issue
Block a user