From d0707d585bd7e8b4157139d7096cc2f8e50ee384 Mon Sep 17 00:00:00 2001 From: Peter Honeder Date: Mon, 31 Aug 2026 10:26:55 +0300 Subject: [PATCH] Harden CI and make its lint step actually lint The check job ran pull request code with a writable token, no time limit, and actions on mutable tags. Its lint step ran no linter at all. .github/workflows/ci.yml: - Read-only token. - persist-credentials: false, so the token is not left in .git/config. - timeout-minutes on both jobs, instead of the 6 hour default. - Actions pinned to commit SHAs. - Force-pushing a PR cancels the superseded run. - Installs golangci-lint from a pinned, checksummed tarball. The runner image does not ship one. Makefile: make lint now fails when golangci-lint is missing and CI is set, instead of falling back to go vet. .golangci.yml: new. Removes the default output limits, which cap findings at 3 of a kind and 50 per linter and keep only one per line. .github/dependabot.yml: new. Keeps the pins current. 23 lint fixes, so the job passes. Mostly unchecked error returns from Close and Fprintf. Three others: tint.NewHandler is deprecated, replaced with tint.NewTextHandler; a deliberately discarded config.Load return now uses _; one switch is now tagged. Fork PRs are gated by a repository setting rather than this file. It is set to require approval for all outside collaborators. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnZG8whBis7HMNVNAWFxDr --- .github/dependabot.yml | 28 +++++++++++++++++++ .github/workflows/ci.yml | 46 ++++++++++++++++++++++++++++--- .golangci.yml | 10 +++++++ Makefile | 6 +++- cmd/demo/main.go | 12 ++++---- cmd/llm-proxy/main.go | 8 +++--- internal/config/main_test.go | 2 +- internal/obs/logging.go | 2 +- internal/proxy/auth_test.go | 4 +-- internal/proxy/handler.go | 8 +++--- internal/proxy/main_test.go | 2 +- internal/proxy/routing_test.go | 4 +-- internal/testutil/fakeupstream.go | 8 +++--- 13 files changed, 110 insertions(+), 30 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .golangci.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..e590722 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,28 @@ +version: 2 + +updates: + # The workflow pins actions to commit SHAs, which is the point — a mutable + # tag is a stranger's push away from running in CI. Pinning without this + # file just trades a supply-chain hole for a stale one, so let Dependabot + # move the pins; it rewrites the SHA and the trailing version comment + # together. + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + day: monday + # Two first-party actions. Separate PRs for them is noise, not signal. + groups: + actions: + patterns: ["*"] + + - package-ecosystem: gomod + directory: / + schedule: + interval: weekly + day: monday + groups: + # Patch and minor bumps land as one PR that CI either passes or does + # not. Majors stay separate, because those are the ones worth reading. + go-minor: + update-types: [minor, patch] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 43f5e19..1300f8d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,32 +11,70 @@ on: - cron: "0 6 * * 1" workflow_dispatch: +# go test runs arbitrary code from the PR, so its token gets read and nothing +# else. Also set in repo settings; this copy is the one that shows up in a diff. +permissions: + contents: read + +# Cancel a superseded PR run on force-push. Not main or the weekly sweep: +# each of those runs is the record for one commit. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: check: runs-on: ubuntu-latest + # The default is 6 hours. The race suite takes a couple of minutes. + timeout-minutes: 15 + env: + # The runner image ships no golangci-lint, and `make lint` used to fall + # back to `go vet` when it was missing, so this step passed for months + # without linting. Pinned like gitleaks, and to the version developers + # run locally, so CI and a laptop disagree only when the code differs. + GOLANGCI_VERSION: 2.13.2 + GOLANGCI_SHA256: 2277d43b98ec0054280f2ac26b53268bae97682444678a59a657dd565da021d6 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Without this the job's token is left behind in .git/config, where + # the pull request's own test code can read it. + persist-credentials: false - - uses: actions/setup-go@v7 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod - # fmt + vet + lint + the full suite under -race. + - name: Install golangci-lint + run: | + set -euo pipefail + tarball="golangci-lint-${GOLANGCI_VERSION}-linux-amd64.tar.gz" + curl -sSfL -o "$tarball" \ + "https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_VERSION}/${tarball}" + echo "${GOLANGCI_SHA256} ${tarball}" | sha256sum -c - + tar -xzf "$tarball" --strip-components=1 \ + "golangci-lint-${GOLANGCI_VERSION}-linux-amd64/golangci-lint" + sudo install golangci-lint /usr/local/bin/golangci-lint + + # fmt + vet + lint + the full suite under -race. CI is set by Actions, so + # a missing linter fails here instead of quietly downgrading to go vet. - run: make check secrets: runs-on: ubuntu-latest + timeout-minutes: 10 env: # Pinned and checksummed. An unpinned scanner is a supply-chain hole in # the one job whose purpose is supply-chain hygiene. GITLEAKS_VERSION: 8.30.1 GITLEAKS_SHA256: 551f6fc83ea457d62a0d98237cbad105af8d557003051f41f3e7ca7b3f2470eb steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: # gitleaks walks real history; the default shallow clone gives it one # commit and nothing to walk. fetch-depth: 0 + persist-credentials: false - name: Install gitleaks run: | diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..f8553c8 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,10 @@ +version: "2" + +issues: + # The defaults stop at 3 of a kind and 50 per linter, which quietly turns a + # full run into a sample. A gate has to report everything it found. + max-issues-per-linter: 0 + max-same-issues: 0 + # One issue per line by default, so a second linter's finding on an already + # flagged line is dropped. That hid an SA1019 deprecation behind a govet hint. + uniq-by-line: false diff --git a/Makefile b/Makefile index 2d57761..815079b 100644 --- a/Makefile +++ b/Makefile @@ -64,11 +64,15 @@ bench: vet: go vet ./... -## lint: golangci-lint when installed, otherwise go vet +## lint: golangci-lint when installed, otherwise go vet (an error under CI) .PHONY: lint lint: ifdef GOLANGCI golangci-lint run ./... +else ifdef CI + @echo "golangci-lint is not installed; the go vet fallback would report a" >&2 + @echo "green lint step that linted almost nothing. Install it in the workflow." >&2 + @exit 1 else @echo "golangci-lint not installed; falling back to go vet (make tools to install)" @go vet ./... diff --git a/cmd/demo/main.go b/cmd/demo/main.go index ec9a2e9..ec999cb 100644 --- a/cmd/demo/main.go +++ b/cmd/demo/main.go @@ -252,14 +252,14 @@ func streamHandler(n int, end ending, finish string) http.HandlerFunc { if err != nil { return } - defer conn.Close() + defer func() { _ = conn.Close() }() writeHead(rw) for i := 0; i < n; i++ { frame := fmt.Sprintf( `data: {"id":"chatcmpl-demo","model":"demo-model","choices":[{"index":0,"delta":{"content":"tok%d"}}]}`+"\n\n", i) if end == endTruncate && i == n-1 { - fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) + _, _ = fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) _ = rw.Flush() return } @@ -291,7 +291,7 @@ func slowFirstToken(think time.Duration) http.HandlerFunc { if err != nil { return } - defer conn.Close() + defer func() { _ = conn.Close() }() writeHead(rw) time.Sleep(think) @@ -311,7 +311,7 @@ func errorMidStream() http.HandlerFunc { if err != nil { return } - defer conn.Close() + defer func() { _ = conn.Close() }() writeHead(rw) writeChunk(rw, `data: {"choices":[{"index":0,"delta":{"content":"partial answer"}}]}`+"\n\n") writeChunk(rw, `data: {"error":{"message":"the model backend crashed","type":"server_error"}}`+"\n\n") @@ -358,7 +358,7 @@ func writeHead(rw *bufio.ReadWriter) { } func writeChunk(rw *bufio.ReadWriter, data string) { - fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) + _, _ = fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) _ = rw.Flush() } @@ -370,7 +370,7 @@ func hangUpMidStream(addr string) { return } body := `{"model":"demo-model","stream":true,"messages":[{"role":"user","content":"hello"}]}` - fmt.Fprintf(conn, "POST /demo/v1/chat/completions HTTP/1.1\r\nHost: demo\r\n"+ + _, _ = fmt.Fprintf(conn, "POST /demo/v1/chat/completions HTTP/1.1\r\nHost: demo\r\n"+ "Content-Type: application/json\r\nContent-Length: %d\r\n\r\n%s", len(body), body) buf := make([]byte, 4096) diff --git a/cmd/llm-proxy/main.go b/cmd/llm-proxy/main.go index fe928b8..632fbc5 100644 --- a/cmd/llm-proxy/main.go +++ b/cmd/llm-proxy/main.go @@ -57,7 +57,7 @@ func run() error { fs.BoolVar(&f.check, "check", false, "validate the config, print the route table and exit") fs.BoolVar(&f.showVersion, "version", false, "print the version and exit") fs.Usage = func() { - fmt.Fprintf(fs.Output(), "llm-proxy — an OpenAI-compatible wire-level debugging proxy\n\nusage: llm-proxy [flags]\n\n") + _, _ = fmt.Fprintf(fs.Output(), "llm-proxy — an OpenAI-compatible wire-level debugging proxy\n\nusage: llm-proxy [flags]\n\n") fs.PrintDefaults() } if err := fs.Parse(os.Args[1:]); err != nil { @@ -75,7 +75,7 @@ func run() error { " see llm-proxy.example.yaml for a starting point") } - cfg, warnings, err := config.Load(path) + cfg, _, err := config.Load(path) if err != nil { return fmt.Errorf("config %s:\n%w", path, err) } @@ -83,7 +83,7 @@ func run() error { // Re-validate after the flags, and keep *these* warnings: -listen can move // the proxy onto a public address, and the warning about an unguarded // listener has to reflect where it will actually listen. - warnings, err = cfg.Validate() + warnings, err := cfg.Validate() if err != nil { return fmt.Errorf("config %s:\n%w", path, err) } @@ -187,7 +187,7 @@ func printRouteTable(c *config.Config, path string, warnings []config.Warning) { } for _, r := range c.Routes { - auth := "none" + var auth string switch { case r.APIKeyEnv == "": auth = "client-supplied only" diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 37686ea..0fd68ef 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -10,6 +10,6 @@ import ( // llm-proxy is likely to have it exported — and without this every test that // assumes an unguarded proxy would fail on their machine and pass in CI. func TestMain(m *testing.M) { - os.Unsetenv("LLM_PROXY_TOKENS") + _ = os.Unsetenv("LLM_PROXY_TOKENS") os.Exit(m.Run()) } diff --git a/internal/obs/logging.go b/internal/obs/logging.go index 3b58d2e..93d5060 100644 --- a/internal/obs/logging.go +++ b/internal/obs/logging.go @@ -60,7 +60,7 @@ func NewLogger(o Options) *Logger { var consoleHandler slog.Handler if pretty { - consoleHandler = tint.NewHandler(out, &tint.Options{ + consoleHandler = tint.NewTextHandler(out, &tint.Options{ Level: level, TimeFormat: "15:04:05.000", NoColor: !color, diff --git a/internal/proxy/auth_test.go b/internal/proxy/auth_test.go index ab0b261..e6bfa66 100644 --- a/internal/proxy/auth_test.go +++ b/internal/proxy/auth_test.go @@ -32,7 +32,7 @@ func TestUnauthenticatedRequestIsRejected(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusUnauthorized { t.Fatalf("Status = %d, want 401", resp.StatusCode) @@ -154,7 +154,7 @@ func TestWrongTokenIsRejected(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusUnauthorized { t.Errorf("Status = %d, want 401", resp.StatusCode) diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go index 45eabd0..b19fdca 100644 --- a/internal/proxy/handler.go +++ b/internal/proxy/handler.go @@ -360,7 +360,7 @@ func (h *routeHandler) deliver( cancelUp context.CancelCauseFunc, prefix []byte, ) { - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() streaming := isEventStream(resp.Header) analyzer := analyze.New(h.srv.now) @@ -540,8 +540,8 @@ func (h *routeHandler) postmortem(rec *record.Request, status int, a *record.Att if status >= 400 && rec.Fault() == nil { env := oaierr.Parse(errBody) - switch { - case status == http.StatusTooManyRequests: + switch status { + case http.StatusTooManyRequests: rec.SetFault(statusFault(fault.KindRateLimited, status, env)) default: if which, ok := h.srv.ctxMatchers.Match(env, errBody); ok { @@ -776,7 +776,7 @@ func decodeForDisplay(body []byte, encoding string) []byte { if err != nil { return body } - defer zr.Close() + defer func() { _ = zr.Close() }() // A capped peek is usually a partial gzip stream, so a read error still // leaves whatever decoded successfully. out, _ := io.ReadAll(io.LimitReader(zr, maxErrorBody)) diff --git a/internal/proxy/main_test.go b/internal/proxy/main_test.go index 3676125..a16e187 100644 --- a/internal/proxy/main_test.go +++ b/internal/proxy/main_test.go @@ -8,6 +8,6 @@ import ( // See the note in internal/config: the suite must not inherit the operator's // LLM_PROXY_TOKENS, or every test would get a 401 from its own proxy. func TestMain(m *testing.M) { - os.Unsetenv("LLM_PROXY_TOKENS") + _ = os.Unsetenv("LLM_PROXY_TOKENS") os.Exit(m.Run()) } diff --git a/internal/proxy/routing_test.go b/internal/proxy/routing_test.go index 9ce9fe0..794ade7 100644 --- a/internal/proxy/routing_test.go +++ b/internal/proxy/routing_test.go @@ -147,7 +147,7 @@ func TestUnknownRouteReturnsParseableError(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusNotFound { t.Errorf("Status = %d, want 404", resp.StatusCode) @@ -189,7 +189,7 @@ func TestHealthAndRoutesEndpoints(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() var routes struct { Routes []struct { diff --git a/internal/testutil/fakeupstream.go b/internal/testutil/fakeupstream.go index 8de9d87..4ebd09e 100644 --- a/internal/testutil/fakeupstream.go +++ b/internal/testutil/fakeupstream.go @@ -91,7 +91,7 @@ func NewUpstream(t *testing.T, scripts ...Script) *Upstream { t.Helper() u := &Upstream{scripts: scripts} u.Server = httptest.NewServer(http.HandlerFunc(u.serve)) - t.Cleanup(u.Server.Close) + t.Cleanup(u.Close) return u } @@ -164,7 +164,7 @@ func (u *Upstream) serveStream(w http.ResponseWriter, s Script) { if err != nil { panic(err) } - defer conn.Close() + defer func() { _ = conn.Close() }() var head bytes.Buffer fmt.Fprintf(&head, "HTTP/1.1 %d %s\r\n", statusOr(s.Status), http.StatusText(statusOr(s.Status))) @@ -204,7 +204,7 @@ func (u *Upstream) serveStream(w http.ResponseWriter, s Script) { // The final frame is where a truncation has to happen, so it is // written as a partial chunk rather than a complete one. if s.Ending == EndTruncateMidFrame && i == len(frames)-1 { - fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) + _, _ = fmt.Fprintf(rw, "%x\r\n%s", len(frame), frame[:len(frame)/2]) _ = rw.Flush() resetOrClose(conn, false) return @@ -266,7 +266,7 @@ func splitFrames(payload string) []string { } func writeChunk(rw *bufio.ReadWriter, data string) { - fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) + _, _ = fmt.Fprintf(rw, "%x\r\n%s\r\n", len(data), data) _ = rw.Flush() }