From df154037f63d570a6e6009d3f4a1982b4d549fcd Mon Sep 17 00:00:00 2001 From: Christopher Plieger <917744+cplieger@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:44:37 +0200 Subject: [PATCH] refactor: drop unread repo fields and narrow config defaults --- .punused-ignore | 12 -------- internal/config/config.go | 34 +++++++++++----------- internal/config/config_test.go | 12 ++++---- internal/ghsignal/ghsignal.go | 6 ++-- internal/github/client.go | 32 +++++--------------- internal/github/client_conditional_test.go | 4 +-- 6 files changed, 34 insertions(+), 66 deletions(-) delete mode 100644 .punused-ignore diff --git a/.punused-ignore b/.punused-ignore deleted file mode 100644 index 1d149da..0000000 --- a/.punused-ignore +++ /dev/null @@ -1,12 +0,0 @@ -# punused findings adjudicated as false positives. One literal substring per -# line, matched against punused's output line; full-line `#` comments carry the -# reason. -# -# transientStatusError wraps a non-transient 5xx so the conditional-GET door -# keeps GetBytes retry parity. Both methods are reached only through an -# interface value: httpx.IsTransient does errors.As(err, &httpx.Transient) and -# calls IsTransient(), and the stdlib errors.Is/As walk calls Unwrap. punused -# resolves explicit call sites only, so cross-module interface dispatch is -# invisible to it. -method (transientStatusError).IsTransient -method (transientStatusError).Unwrap diff --git a/internal/config/config.go b/internal/config/config.go index f0e5a83..e14f897 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,16 +21,16 @@ import ( // Defaults for environment-backed fields. const ( - // DefaultScanInterval stays within GitHub's authenticated request budget. - DefaultScanInterval = 15 * time.Minute - // DefaultLookbackHours retains failures across a weekend. - DefaultLookbackHours = 72 - // DefaultPRExclude removes Renovate PR noise. - DefaultPRExclude = "-author:app/renovate" - // DefaultIssueExclude removes Renovate and generated issue noise. - DefaultIssueExclude = "-author:app/renovate -label:renovate -label:auto-generated" - // DefaultCodeScanningExcludeForks excludes inherited upstream alerts. - DefaultCodeScanningExcludeForks = true + // defaultScanInterval stays within GitHub's authenticated request budget. + defaultScanInterval = 15 * time.Minute + // defaultLookbackHours retains failures across a weekend. + defaultLookbackHours = 72 + // defaultPRExclude removes Renovate PR noise. + defaultPRExclude = "-author:app/renovate" + // defaultIssueExclude removes Renovate and generated issue noise. + defaultIssueExclude = "-author:app/renovate -label:renovate -label:auto-generated" + // defaultCodeScanningExcludeForks excludes inherited upstream alerts. + defaultCodeScanningExcludeForks = true // maxScanInterval prevents a cadence too slow to be actionable. maxScanInterval = 365 * 24 * time.Hour // minScanInterval prevents quota exhaustion. @@ -78,7 +78,7 @@ func Load() (Config, []Warning) { } scanInterval, scanWarns := ScanInterval() warns = append(warns, scanWarns...) - lookback, lookbackWarns := clampedInt("LOOKBACK_HOURS", DefaultLookbackHours, 1, maxLookbackHours) + lookback, lookbackWarns := clampedInt("LOOKBACK_HOURS", defaultLookbackHours, 1, maxLookbackHours) warns = append(warns, lookbackWarns...) return Config{ @@ -86,12 +86,12 @@ func Load() (Config, []Warning) { Owner: strings.TrimSpace(os.Getenv("GITHUB_OWNER")), ExcludeRepos: parseExcludes(os.Getenv("EXCLUDE_REPOS")), CodeScanningExcludeRepos: parseExcludes(os.Getenv("CODE_SCANNING_EXCLUDE_REPOS")), - PRExclude: cmp.Or(envx.String("PR_EXCLUDE_QUERY"), DefaultPRExclude), - IssueExclude: cmp.Or(envx.String("ISSUE_EXCLUDE_QUERY"), DefaultIssueExclude), + PRExclude: cmp.Or(envx.String("PR_EXCLUDE_QUERY"), defaultPRExclude), + IssueExclude: cmp.Or(envx.String("ISSUE_EXCLUDE_QUERY"), defaultIssueExclude), ScanInterval: scanInterval, Lookback: time.Duration(lookback) * time.Hour, LogLevel: lvl, - CodeScanningExcludeForks: envx.Bool("CODE_SCANNING_EXCLUDE_FORKS", DefaultCodeScanningExcludeForks), + CodeScanningExcludeForks: envx.Bool("CODE_SCANNING_EXCLUDE_FORKS", defaultCodeScanningExcludeForks), }, warns } @@ -105,13 +105,13 @@ func ScanInterval() (time.Duration, []Warning) { // parseScanInterval falls back to the default for invalid or external modes. func parseScanInterval(raw string) (time.Duration, []Warning) { - s := scheduler.ParseInterval(raw, DefaultScanInterval, + s := scheduler.ParseInterval(raw, defaultScanInterval, scheduler.WithBounds(minScanInterval, maxScanInterval), scheduler.WithName("SCAN_INTERVAL")) if s.Mode == scheduler.ModeExternal { - return DefaultScanInterval, []Warning{{ + return defaultScanInterval, []Warning{{ Msg: "invalid SCAN_INTERVAL, using default", - Attrs: []slog.Attr{slog.String("value", raw), slog.String("default", DefaultScanInterval.String())}, + Attrs: []slog.Attr{slog.String("value", raw), slog.String("default", defaultScanInterval.String())}, }} } return s.Interval, nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0ac8d64..07cb64c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -20,7 +20,7 @@ func TestLoadDefaults(t *testing.T) { cfg := loadCfg() // Expected values are written out rather than read back from the - // constants they check: an assertion against DefaultScanInterval moves + // constants they check: an assertion against defaultScanInterval moves // with any edit to it and so pins nothing. 15m and 72h are the cadence // and window the compose contract and the bundled dashboard assume. if cfg.ScanInterval != 15*time.Minute { @@ -384,12 +384,12 @@ func TestCodeScanningExcludeForks(t *testing.T) { raw string want bool }{ - {desc: "unset defaults to exclude", set: false, want: DefaultCodeScanningExcludeForks}, - {desc: "empty is treated as unset", set: true, raw: "", want: DefaultCodeScanningExcludeForks}, + {desc: "unset defaults to exclude", set: false, want: defaultCodeScanningExcludeForks}, + {desc: "empty is treated as unset", set: true, raw: "", want: defaultCodeScanningExcludeForks}, {desc: "false includes forks", set: true, raw: "false", want: false}, {desc: "true excludes forks", set: true, raw: "true", want: true}, {desc: "envx accepts 0", set: true, raw: "0", want: false}, - {desc: "unparseable falls back to the default", set: true, raw: "ture", want: DefaultCodeScanningExcludeForks}, + {desc: "unparseable falls back to the default", set: true, raw: "ture", want: defaultCodeScanningExcludeForks}, } { t.Run(tc.desc, func(t *testing.T) { if tc.set { @@ -401,7 +401,7 @@ func TestCodeScanningExcludeForks(t *testing.T) { } }) } - if !DefaultCodeScanningExcludeForks { - t.Error("DefaultCodeScanningExcludeForks = false, want true: a fork's alerts are the upstream project's, so the safe default is to skip them") + if !defaultCodeScanningExcludeForks { + t.Error("defaultCodeScanningExcludeForks = false, want true: a fork's alerts are the upstream project's, so the safe default is to skip them") } } diff --git a/internal/ghsignal/ghsignal.go b/internal/ghsignal/ghsignal.go index d18072a..b5b90df 100644 --- a/internal/ghsignal/ghsignal.go +++ b/internal/ghsignal/ghsignal.go @@ -20,10 +20,8 @@ var ( // Repo is a GitHub repository discovered for an owner. type Repo struct { - Owner string `json:"owner"` - Name string `json:"name"` - Private bool `json:"private"` - Archived bool `json:"archived"` + Owner string `json:"owner"` + Name string `json:"name"` // Fork alerts describe inherited upstream code; other signals remain in scope. Fork bool `json:"fork"` } diff --git a/internal/github/client.go b/internal/github/client.go index 22c2ffc..dd09621 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -93,7 +93,6 @@ type apiRepo struct { Owner struct { Login string `json:"login"` } `json:"owner"` - Private bool `json:"private"` Archived bool `json:"archived"` Fork bool `json:"fork"` } @@ -126,11 +125,9 @@ func (c *Client) ListRepos(ctx context.Context, owner string) ([]ghsignal.Repo, continue } repos = append(repos, ghsignal.Repo{ - Owner: r.Owner.Login, - Name: r.Name, - Private: r.Private, - Archived: r.Archived, - Fork: r.Fork, + Owner: r.Owner.Login, + Name: r.Name, + Fork: r.Fork, }) } if len(pageRepos) < perPage { @@ -276,8 +273,7 @@ func (c *Client) getJSONConditional(ctx context.Context, reqURL string, out any) // contract). A non-transient 5xx is wrapped transient so this door retries // every 5xx exactly as GetBytes does — DoConditional's CheckHTTPStatus // mapping classifies only 502/503/504 transient, and the repo listing is -// the scan's one health-flipping call, so it must not lose retries in the -// adoption. +// the scan's one health-flipping call, so it must not lose retries. func (c *Client) conditionalGet(ctx context.Context, reqURL string, v httpx.Validators) (httpx.ConditionalResult, error) { opts := make([]httpx.DoOption, 0, len(c.retryOpts)+2) for _, o := range c.retryOpts { @@ -292,25 +288,12 @@ func (c *Client) conditionalGet(ctx context.Context, reqURL string, v httpx.Vali c.setHeaders(req) res, err := httpx.DoConditional(c.http, req, v, bodyCap) if hse, ok := errors.AsType[*httpx.HTTPStatusError](err); ok && hse.IsServerError() && !hse.IsTransient() { - err = transientStatusError{err} + err = httpx.MarkTransient(err) } return res, err }, opts...) } -// transientStatusError marks a non-transient 5xx from the conditional door -// retryable, aligning it with the GetBytes door's all-5xx retry policy (the -// per-door divergence is deliberate in httpx; this client wants one policy -// across both of its paths). -type transientStatusError struct{ error } - -// IsTransient implements httpx.Transient. -func (transientStatusError) IsTransient() bool { return true } - -// Unwrap exposes the wrapped status error to errors.As chains -// (codeScanningNotFound, mapStatusError). -func (e transientStatusError) Unwrap() error { return e.error } - // mapStatusError maps only 401 and 429 to systemic domain errors; 403 remains per-repo. func mapStatusError(err error) error { if se, ok := errors.AsType[*httpx.StatusError](err); ok { @@ -449,9 +432,8 @@ func (c *Client) search(ctx context.Context, base, owner, exclude string) ([]api } // archived:false excludes archived repos from the cross-repo Search API, // which (unlike ListRepos) includes them by default. This aligns the - // snapshot path with the repo-loop path (ListRepos filters r.Archived) and - // with ghsignal.Repo's contract that archived repos are skipped: an archived - // repo's open PRs/issues are not actionable. + // snapshot path with the repo-loop path (ListRepos filters r.Archived): an + // archived repo's open PRs/issues are not actionable. q := base + " user:" + owner + " archived:false" if exclude = strings.TrimSpace(exclude); exclude != "" { q += " " + exclude diff --git a/internal/github/client_conditional_test.go b/internal/github/client_conditional_test.go index 9ffc115..793854b 100644 --- a/internal/github/client_conditional_test.go +++ b/internal/github/client_conditional_test.go @@ -259,8 +259,8 @@ func TestConditional_codeScanning404StillMapsToNoCodeScanning(t *testing.T) { } } -// TestConditional_500IsRetried pins the transientStatus wrapper: the -// conditional door retries a plain 500 exactly as the GetBytes door does +// TestConditional_500IsRetried pins the transient mark on conditional 5xx: +// the conditional door retries a plain 500 exactly as the GetBytes door does // (DoConditional's own classification would treat only 502/503/504 as // transient), so the scan's one health-flipping call keeps its retries. func TestConditional_500IsRetried(t *testing.T) {