From af56f2176a3045d2574d1c28dccb557b3270c528 Mon Sep 17 00:00:00 2001 From: Rehmanali Momin Date: Tue, 26 May 2026 23:32:27 +0530 Subject: [PATCH] Fix silent exit when NIGHTFALL_API_KEY is invalid (INTM-4909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, scanAllFiles logged but swallowed scan errors, so a 401 from the Nightfall API (bad/expired API key) caused the GitHub Action to print an error line and then exit 0 — letting PRs through with sensitive data unscanned. ReviewDiff now propagates the first scan error to main, which exits 1. 401/403 responses are wrapped with an explicit hint to check NIGHTFALL_API_KEY so the failure surface is obvious in CI logs. Linear: https://linear.app/nightfall/issue/INTM-4909 Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/clients/nightfall/nightfall.go | 64 ++++++++++++++++++-- internal/clients/nightfall/nightfall_test.go | 59 ++++++++++++++++++ 2 files changed, 117 insertions(+), 6 deletions(-) diff --git a/internal/clients/nightfall/nightfall.go b/internal/clients/nightfall/nightfall.go index ca899a0c..adac9180 100644 --- a/internal/clients/nightfall/nightfall.go +++ b/internal/clients/nightfall/nightfall.go @@ -4,8 +4,10 @@ import ( "bufio" "bytes" "context" + "errors" "fmt" "io" + "net/http" "regexp" "strings" "sync" @@ -266,6 +268,28 @@ func (n *Client) buildScanRequest(items []string) *nf.ScanTextRequest { } } +// isAuthError reports whether err represents a Nightfall API authentication +// failure (HTTP 401 or 403). The Nightfall Go SDK returns *nf.Error for any +// non-2xx response, with Code set to the HTTP status. +func isAuthError(err error) bool { + var apiErr *nf.Error + if errors.As(err, &apiErr) { + return apiErr.Code == http.StatusUnauthorized || apiErr.Code == http.StatusForbidden + } + return false +} + +// describeScanError returns a user-facing error message for a failed scan +// request. Authentication failures are called out explicitly so the action +// fails loudly when NIGHTFALL_API_KEY is missing/expired/revoked, instead of +// silently passing the PR through (see INTM-4909). +func describeScanError(err error) error { + if isAuthError(err) { + return fmt.Errorf("nightfall API authentication failed (check NIGHTFALL_API_KEY is valid and not expired): %w", err) + } + return err +} + func (n *Client) scanFileContent( ctx context.Context, cts []*fileToScan, @@ -282,7 +306,7 @@ func (n *Client) scanFileContent( resp, err := n.Scan(ctx, items) if err != nil { logger.Debug(fmt.Sprintf("Error sending request number %d with %d items: %v", requestNum, len(items), err)) - return nil, err + return nil, describeScanError(err) } // Determine findings from response and create comments @@ -296,8 +320,10 @@ func (n *Client) scanAllFiles( logger logger.Logger, cts []*fileToScan, commentCh chan<- []*diffreviewer.Comment, + errCh chan<- error, ) { defer close(commentCh) + defer close(errCh) blockingCh := make(chan struct{}, n.MaxNumberRoutines) var wg sync.WaitGroup requestBatches := make([][]*fileToScan, 0) @@ -338,17 +364,24 @@ func (n *Client) scanAllFiles( blockingCh <- struct{}{} go func(loopCount int, cts []*fileToScan) { defer wg.Done() + defer func() { <-blockingCh }() if ctx.Err() != nil { return } c, err := n.scanFileContent(ctx, cts, loopCount+1, logger) if err != nil { - logger.Error(fmt.Sprintf("Unable to scan %d content items", len(cts))) - } else { - commentCh <- c + logger.Error(fmt.Sprintf("Unable to scan %d content items: %v", len(cts), err)) + // Propagate error so the action exits non-zero. We do a + // non-blocking send: the consumer only needs the first + // failure to fail the run (see INTM-4909). + select { + case errCh <- err: + default: + } + return } - <-blockingCh + commentCh <- c }(i, requestBatches[i]) } wg.Wait() @@ -379,19 +412,38 @@ func (n *Client) ReviewDiff(ctx context.Context, logger logger.Logger, fileDiffs } commentCh := make(chan []*diffreviewer.Comment) + // Buffered so a worker can record an error without blocking; we only + // surface the first error we observe (see INTM-4909). + errCh := make(chan error, 1) newCtx, cancel := context.WithDeadline(ctx, time.Now().Add(defaultTimeout)) defer cancel() - go n.scanAllFiles(newCtx, logger, fileToScanList, commentCh) + go n.scanAllFiles(newCtx, logger, fileToScanList, commentCh, errCh) comments := make([]*diffreviewer.Comment, 0) + var scanErr error for { select { case c, chOpen := <-commentCh: if !chOpen { + // commentCh is closed once scanAllFiles is done. Drain any + // pending error before returning so we don't lose a failure + // that came in alongside the close. + if scanErr == nil { + if err, ok := <-errCh; ok { + scanErr = err + } + } + if scanErr != nil { + return nil, scanErr + } return comments, nil } comments = append(comments, c...) + case err, ok := <-errCh: + if ok && scanErr == nil { + scanErr = err + } case <-newCtx.Done(): return nil, newCtx.Err() } diff --git a/internal/clients/nightfall/nightfall_test.go b/internal/clients/nightfall/nightfall_test.go index c3781395..4b60884b 100644 --- a/internal/clients/nightfall/nightfall_test.go +++ b/internal/clients/nightfall/nightfall_test.go @@ -331,6 +331,65 @@ func TestReviewDiffHasFindingMetadata(t *testing.T) { assert.Equal(t, expectedComments, comments, "Received incorrect response from ReviewDiff") } +// TestReviewDiffPropagatesScanError verifies that a scan-time failure (such +// as a bad NIGHTFALL_API_KEY producing a 401) bubbles up from ReviewDiff +// rather than being silently swallowed. See INTM-4909: previously the +// worker logged the error and the action exited 0, letting unscanned PRs +// merge. +func TestReviewDiffPropagatesScanError(t *testing.T) { + mockAPIClient := &mockNightfall{ + scanFn: func(ctx context.Context, request *nf.ScanTextRequest) (*nf.ScanTextResponse, error) { + return nil, &nf.Error{Code: 401, Message: "Unauthorized"} + }, + } + client := Client{ + APIClient: mockAPIClient, + DetectionRules: testDetectionRules, + MaxNumberRoutines: 1, + } + + input := []*diffreviewer.FileDiff{ + { + PathNew: "test/data", + Hunks: []*diffreviewer.Hunk{ + { + Lines: []*diffreviewer.Line{ + {LnumNew: 1, Content: "some content"}, + }, + }, + }, + }, + } + + comments, err := client.ReviewDiff(context.Background(), githublogger.NewDefaultGithubLogger(), input) + assert.Error(t, err, "ReviewDiff must surface scan-time errors so the action exits non-zero") + assert.Nil(t, comments) + assert.Contains(t, err.Error(), "authentication failed", + "401 errors should be wrapped with a clear NIGHTFALL_API_KEY hint") +} + +// TestIsAuthError covers the 401/403 detection used to produce a clear +// error message when NIGHTFALL_API_KEY is invalid (INTM-4909). +func TestIsAuthError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "nil", err: nil, want: false}, + {name: "generic error", err: errors.New("boom"), want: false}, + {name: "500 from API", err: &nf.Error{Code: 500}, want: false}, + {name: "401 from API", err: &nf.Error{Code: 401}, want: true}, + {name: "403 from API", err: &nf.Error{Code: 403}, want: true}, + {name: "wrapped 401", err: fmt.Errorf("outer: %w", &nf.Error{Code: 401}), want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isAuthError(tt.err)) + }) + } +} + func TestScanPaths(t *testing.T) { client := Client{ DetectionRules: testDetectionRules,