chore: address code review comments

grafana-alertcheck/cmd/grafana-alertcheck/check.go → grafana-alertcheck/cmd/check.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -40,6 +41,13 @@ func runCheck(args []string, stdin io.Reader, stdout, stderr io.Writer) int {
 	output := fs.String("output", "", `"json" writes the machine-readable Result to stdout in addition to the table; default is the table alone`)
 
 	if err := fs.Parse(args); err != nil {
+		if errors.Is(err, flag.ErrHelp) {
+			return 0
+		}
+		return 2
+	}
+	if fs.NArg() != 0 {
+		fmt.Fprintf(stderr, "check: unexpected arguments %v\n", fs.Args())
 		return 2
 	}
 	if *output != "" && *output != "json" {
@@ -113,11 +121,10 @@ func runCheck(args []string, stdin io.Reader, stdout, stderr io.Writer) int {
 
 	result, checkErr := gate.Check(ctx, cfg)
 
-	if err := renderTable(stderr, result); err != nil {
-		fmt.Fprintln(stderr, err)
-	}
 	if checkErr != nil {
 		fmt.Fprintln(stderr, checkErr)
+	} else if err := renderTable(stderr, result); err != nil {
+		fmt.Fprintln(stderr, err)
 	}
 	if *output == "json" {
 		enc := json.NewEncoder(stdout)

grafana-alertcheck/cmd/grafana-alertcheck/main.go → grafana-alertcheck/cmd/main.go

@@ -12,7 +12,7 @@ func main() {
 	os.Exit(run(os.Args[1:], os.Stdout, os.Stderr))
 }
 
-const usage = "usage: grafana-alertcheck <list|watch|check|version>"
+const usage = "usage: grafana-alertcheck <list|watch|check>"
 
 // run is the whole of main's testable surface: parse the subcommand, dispatch,
 // return the process exit code. Exit codes below 2 (pass/violations) belong to
@@ -37,8 +37,6 @@ func run(args []string, stdout, stderr io.Writer) int {
 		return runWatch(args[1:], os.Stdin, stdout, stderr)
 	case "check":
 		return runCheck(args[1:], os.Stdin, stdout, stderr)
-	case "version":
-		return runVersion(args[1:], stdout, stderr)
 	case "-h", "-help", "--help":
 		fmt.Fprintln(stdout, usage)
 		return 0

grafana-alertcheck/cmd/grafana-alertcheck/watch.go → grafana-alertcheck/cmd/watch.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -49,6 +50,13 @@ func runWatch(args []string, stdin io.Reader, stdout, stderr io.Writer) int {
 	readyFD := fs.Int(gate.ReadyFDFlag[2:], 0, "")
 
 	if err := fs.Parse(args); err != nil {
+		if errors.Is(err, flag.ErrHelp) {
+			return 0
+		}
+		return 2
+	}
+	if fs.NArg() != 0 {
+		fmt.Fprintf(stderr, "watch: unexpected arguments %v\n", fs.Args())
 		return 2
 	}
 

grafana-alertcheck/docs/index.md

@@ -23,7 +23,7 @@ It **fails closed**: if it cannot get an answer, it stops the release. It never
 ## Install
 
 ```bash
-go install github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/cmd/grafana-alertcheck@latest
+go install github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/cmd@latest
 ```
 
 Connection details come from the environment — the token is env-only, never a flag:

grafana-alertcheck/internal/gate/classify.go

@@ -476,7 +476,7 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition,
 		Thresholds:     make(map[string]RuleThresholds),
 		Global: GlobalThresholds{
 			TransitionGrace: gt.transitionGrace,
-			GraceSource:     gt.graceSource,
+			GraceSource:     graceSourceOrNone(gt.graceSource),
 			DrainTimeout:    gt.drainTimeout,
 		},
 	}

grafana-alertcheck/internal/gate/coverage.go

@@ -146,7 +146,7 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d
 		// corrupted or hand-edited data (ReadLog does no field validation);
 		// GrafanaNow-LastEvaluation would go negative and silently read as
 		// fresh — fail-open. Treat it as unobservable instead.
-		if p.LastEvaluation.After(p.GrafanaNow) {
+		if p.LastEvaluation.Truncate(time.Second).After(p.GrafanaNow) {
 			fail(ReasonFutureEvaluation, fmt.Sprintf(
 				"lastEvaluation %s is after grafana_now %s (corrupted poll)",
 				p.LastEvaluation.Format(time.RFC3339), p.GrafanaNow.Format(time.RFC3339)))

grafana-alertcheck/internal/gate/parse_ruler.go

@@ -84,7 +84,7 @@ func ParseDefinitions(body []byte) ([]Definition, error) {
 func parseDefinition(raw json.RawMessage, folder, group string) (Definition, error) {
 	var m map[string]json.RawMessage
 	if err := json.Unmarshal(raw, &m); err != nil {
-		return Definition{}, fmt.Errorf("%w", err)
+		return Definition{}, err
 	}
 
 	var forStr string

grafana-alertcheck/internal/gate/parse_state_test.go

@@ -217,10 +217,6 @@ func TestInstanceKey(t *testing.T) {
 	require.Equal(t, "null", instanceKey(nil), "instanceKey(nil) should be \"null\"")
 }
 
-<<<<<<< HEAD
-// Label values may contain "\n" or "="; the JSON encoding must keep them distinct.
-=======
->>>>>>> c276546b (chore: use testify's require in tests)
 func TestInstanceKey_NoCollision(t *testing.T) {
 	require.NotEqual(t, instanceKey(map[string]string{"a": "1\nb=2"}), instanceKey(map[string]string{"a": "1", "b": "2"}), "instanceKey should not collide for sets {a:1\\nb=2} and {a:1,b:2}")
 

grafana-alertcheck/internal/gate/schedule.go

@@ -339,6 +339,16 @@ func CheckBudget(t map[string]ruleTimings, measured map[string]time.Duration, co
 	return fmt.Errorf("%s", b.String())
 }
 
+// graceSourceOrNone is the single "none" default for the grace-source field:
+// an empty source means no rule contributed a transitionGrace. Applied here so
+// StartupSummary and the human table print the same thing.
+func graceSourceOrNone(source string) string {
+	if source == "" {
+		return "none"
+	}
+	return source
+}
+
 // StartupSummary formats the pre-run print an operator sees before the wait:
 // the total planned run time and the rule (with its `for` value) that set
 // transitionGrace, plus a warning when the grace eats more than
@@ -347,10 +357,7 @@ func CheckBudget(t map[string]ruleTimings, measured map[string]time.Duration, co
 func StartupSummary(from, to time.Time, global globalTimings) (summary, warning string) {
 	window := to.Sub(from)
 	total := window + global.transitionGrace + global.drainTimeout
-	source := global.graceSource
-	if source == "" {
-		source = "none"
-	}
+	source := graceSourceOrNone(global.graceSource)
 	summary = fmt.Sprintf(
 		"planned run time: %s\n  window %s + transitionGrace %s + drainTimeout %s\n  transitionGrace source: %s",
 		total, window, global.transitionGrace, global.drainTimeout, source)

grafana-alertcheck/internal/gate/source.go

@@ -44,9 +44,10 @@ type Observation struct {
 	Latency    time.Duration // t_send through the full body read — see requestResult.Latency
 }
 
-// TransportError marks a failure worth retrying: a non-2xx response, a network
-// failure, or a body that failed to parse. Not a deleted rule (an authoritative
-// 2xx) and not a clock problem (a hard error — see doRequest).
+// TransportError marks a failure worth retrying: a 5xx/429 response, a network
+// failure, or a body that failed to parse. Not a 4xx (wrong auth, missing
+// resource), not a deleted rule (an authoritative 2xx) and not a clock problem
+// (a hard error — see doRequest).
 type TransportError struct {
 	Err    error
 	Status int // 0 when the failure never got a status (network/transport failure)
@@ -111,17 +112,7 @@ func parseGrafanaVersion(s string) (grafanaVersion, error) {
 	var v grafanaVersion
 	fields := [3]*int{&v.major, &v.minor, &v.patch}
 	for i, field := range fields {
-		// Trim any trailing non-digit suffix (prerelease/build metadata, e.g.
-		// "0+security") rather than requiring an exact numeric match.
-		digits := parts[i]
-		j := 0
-		for j < len(digits) && digits[j] >= '0' && digits[j] <= '9' {
-			j++
-		}
-		if j == 0 {
-			return grafanaVersion{}, fmt.Errorf("unparseable version %q", s)
-		}
-		n, err := strconv.Atoi(digits[:j])
+		n, err := strconv.Atoi(parts[i])
 		if err != nil {
 			return grafanaVersion{}, fmt.Errorf("unparseable version %q: %w", s, err)
 		}
@@ -259,10 +250,11 @@ type requestResult struct {
 	Latency time.Duration
 }
 
-// doRequest performs one HTTP GET and classifies the outcome: network failure,
-// non-2xx, or body-read failure is retryable (*TransportError); a missing or
-// unparseable Date header or a skew beyond SkewHardLimit is a hard error —
-// retrying can never fix either, so neither enters the backoff loop.
+// doRequest performs one HTTP GET and classifies the outcome: a 5xx/429, a
+// network failure, or a body-read failure is retryable (*TransportError); a
+// 4xx (wrong auth, missing resource — retrying cannot fix it), a missing or
+// unparseable Date header, or a skew beyond SkewHardLimit is a hard error, so
+// none of those enters the backoff loop.
 //
 // The Date/skew check runs on every endpoint (even /api/health): a skew only
 // noticed once RuleState starts polling has already masked earlier reads, so it
@@ -298,7 +290,11 @@ func (s *httpSource) doRequest(ctx context.Context, path string) (requestResult,
 	latency := tBodyRead.Sub(tSend)
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return requestResult{}, &TransportError{Err: fmt.Errorf("unexpected status %d", resp.StatusCode), Status: resp.StatusCode}
+		err := fmt.Errorf("unexpected status %d", resp.StatusCode)
+		if resp.StatusCode >= 400 && resp.StatusCode < 500 && resp.StatusCode != http.StatusTooManyRequests {
+			return requestResult{}, err
+		}
+		return requestResult{}, &TransportError{Err: err, Status: resp.StatusCode}
 	}
 
 	dateHeader := resp.Header.Get("Date")