Adopt CI + linter fixes from PR #1 on top of current main#4
Draft
ameijer wants to merge 6 commits into
Draft
Conversation
Adopts CI from #1: - build-test.yaml: go build + go test with coverage artifact - codeql.yaml: CodeQL scanning - format-check.yaml: gofmt enforcement - golangci-lint.yaml: static analysis - scorecard.yml: OSSF Scorecard - sonar.yaml: SonarQube integration - update-go-version.yaml: automated Go toolchain bumps - vulnerability-scan.yaml: Trivy scanner - .gitignore, .golangci.yml, sonar-project.properties Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
Adopts from #1: - README: document GOPRIVATE for private module access, update buffer.go link to point at main instead of develop - go.mod: set go directive to 1.23 (minimum required for iter package use) so the module remains buildable with the current Go toolchain advertised in the golangci-lint / vulnerability workflows Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
Adopts the linter fixes from #1 plus additional follow-ups so golangci-lint v2 (default linter set: errcheck, govet, ineffassign, staticcheck, unused) passes cleanly: - cmd/bingen: check error from os.Remove when clearing stale codec files. - pkg/generator/bingen: replace deprecated strings.Title with a local titleName helper; avoid passing a constant as a format string; tidy redundant 'var ... string = ""' declarations. - pkg/generator/context: drop the unused 'version' field and route through titleName. - pkg/generator/streaming, generator: drop redundant typed zero-value decls caught by ineffassign/staticcheck (S1021). - pkg/generator/support: fix a doubly-escaped %%s format directive that hid the actual type name in the returned error. - pkg/meta/annotationset: downcase error-string capitalisation (ST1005) and collapse the for/append into a single append(slice, slice...). Guard the legitimate use of parser.ParseDir's deprecated ast.Package map with a nolint:staticcheck directive. - pkg/types/types: same ST1005 / append(range) fixes for field annotation parsing. - pkg/util/bufferpool: add a nolint directive documenting why sync.Pool here intentionally stores []byte by value rather than by pointer. - pkg/util/stringutil: drop the math/rand.Seed init - it is unused by this file and is a no-op on Go 1.20+. Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
The Read*/Write* helpers on Buffer previously swallowed errors returned by the low-level encoding helpers, which tripped errcheck and (more importantly) meant silent data corruption on short reads / encoder failures. We now: - Route every Write* error through a panic, matching the existing read-only / writable contract checks on Buffer. - Introduce an internal 'must(error)' helper and use it on every Read* site so short reads, io errors, and oversize payloads surface immediately rather than returning a zero value. - Rename NonPrimitiveTypeError to ErrNonPrimitiveType (ST1012, exported error naming) and rewrap its message in lowercase (ST1005). - Downcase the Peek error message (ST1005). - Add pkg/util/buffer_test.go covering the full round-trip matrix for both the bytes.Buffer and bufio.Reader backings, confirming ReadString truncation, Peek rejection on rw buffers, Bytes drain-then-empty semantics, and that every Read* panics on an empty source. Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
Brings in the streaming test fixture package added in #1. Used as an input package for upcoming streaming-codec generator tests. Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
Raises the module's go directive to 1.25 to match the 'stable' toolchain pinned in the CI workflows. Verified locally with go 1.25.0: - go build ./... clean - go vet ./... clean - go test ./pkg/... ./tests/... -count=1 passes - golangci-lint v2.6.1 (built with go1.25.3) run ./... reports 0 issues. Co-authored-by: Alex Meijer <ameijer@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Re-applies CI/lint infrastructure and related code cleanups from the previously closed PR #1 onto current main, including new workflows/configs and additional lint/test fixes (notably around pkg/util.Buffer read/write error handling).
Changes:
- Added multiple GitHub Actions workflows (build/test, lint, CodeQL, scorecard, SonarCloud, Trivy) plus repo configs (
.golangci.yml,sonar-project.properties,.gitignore). - Performed lint-driven refactors across generator/meta/types/util (error strings, append simplifications, deprecation fixes) and tightened
Bufferread/write paths with explicit error propagation. - Added new tests for
pkg/util.Bufferand introduced atests/streamingfixture package.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/streaming/types.go | Adds streaming test fixture model types. |
| sonar-project.properties | Adds Sonar configuration for sources/tests/coverage. |
| pkg/util/stringutil/stringutil.go | Removes global RNG seeding (lint-driven change). |
| pkg/util/bufferpool.go | Adds //nolint note for sync.Pool slice storage. |
| pkg/util/buffer_test.go | Adds Buffer round-trip, underflow panic, and semantics tests. |
| pkg/util/buffer.go | Standardizes error names/messages; panics on read/write errors via helper. |
| pkg/types/types.go | Simplifies appends; standardizes error strings. |
| pkg/meta/annotationset.go | Standardizes error strings; simplifies appends; adds lint suppression note. |
| pkg/generator/support.go | Fixes fmt string typo in stream factory error. |
| pkg/generator/streaming.go | Simplifies local variable declaration. |
| pkg/generator/context.go | Replaces deprecated strings.Title usage with helper. |
| pkg/generator/bingen.go | Adds titleName helper; improves template printing and minor refactors. |
| cmd/bingen/bingen.go | Checks os.Remove error when deleting existing generated file. |
| README.md | Adds GOPRIVATE guidance and updates buffer.go link to main. |
| .golangci.yml | Adds GolangCI-Lint configuration and generated/tests exclusions. |
| .gitignore | Adds baseline ignores for IDE/build/test outputs. |
| .github/workflows/vulnerability-scan.yaml | Adds Trivy FS scan + SARIF upload and artifacting. |
| .github/workflows/update-go-version.yaml | Adds scheduled automation to bump Go version via PR. |
| .github/workflows/sonar.yaml | Adds SonarCloud scan triggered by successful Build/Test. |
| .github/workflows/scorecard.yml | Adds OSSF Scorecard workflow configuration. |
| .github/workflows/golangci-lint.yaml | Adds GolangCI-Lint workflow. |
| .github/workflows/format-check.yaml | Adds gofmt verification workflow. |
| .github/workflows/codeql.yaml | Adds CodeQL scanning workflow. |
| .github/workflows/build-test.yaml | Adds Go build/test with coverage + artifact upload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: cp sonar-artifacts/coverage.out coverage.out | ||
|
|
||
| - name: SonarCloud Scan | ||
| uses: sonarsource/sonarcloud-github-action@master |
| analysis: | ||
| name: Scorecard analysis | ||
| runs-on: ubuntu-latest | ||
| if: github.event.repository.default_branch == github.ref_name || github.event_name == 'pull_request' |
| if b.bw != nil { | ||
| readUint16(b.bw, &l) | ||
| must(readUint16(b.bw, &l)) | ||
| return bytesToString(b.bw.Next(int(l))) |
Comment on lines
+16
to
+21
| func titleName(s string) string { | ||
| if s == "" { | ||
| return s | ||
| } | ||
| return strings.ToUpper(s[:1]) + s[1:] | ||
| } |
| {"ReadUInt32", func(b *Buffer) { b.ReadUInt32() }}, | ||
| {"ReadUInt64", func(b *Buffer) { b.ReadUInt64() }}, | ||
| {"ReadFloat32", func(b *Buffer) { b.ReadFloat32() }}, | ||
| {"ReadFloat64", func(b *Buffer) { b.ReadFloat64() }}, |
Comment on lines
+226
to
+229
|
|
||
| // Ensure Buffer value type satisfies expectations against io.Reader-returning | ||
| // helpers. | ||
| var _ io.Reader = (*bytes.Reader)(nil) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-applies the CI and linter fixes from the now-closed #1 on top of the current
mainbranch, without replaying its oldercmd//pkg//tests/core sources. Core source continues to come frommain; only the CI infra, README tweaks, lint fixes, and a new streaming test fixture come from #1. Additional linter fixes and tests were added untilgolangci-lintreports zero issues.What's in here
Six logical commits:
ci: add GitHub Actions workflows, golangci-lint and sonar config— adds all eight workflows from initial commit of bingen #1 (build-test,codeql,format-check,golangci-lint,scorecard,sonar,update-go-version,vulnerability-scan), plus.gitignore,.golangci.yml, andsonar-project.properties.docs: note GOPRIVATE, update buffer.go reference, and lower go directive— README gains theGOPRIVATEinstructions and thebuffer.golink moves fromdeveloptomain.lint: fix errcheck, staticcheck, ineffassign across pkg/ and cmd/— carries over the PR initial commit of bingen #1 lint fixes tocmd/bingen,pkg/generator,pkg/meta,pkg/types,pkg/util/bufferpool, andpkg/util/stringutil(deprecatedstrings.Title,%%stypo,ST1005error-string casing,S1011range-append, unused fields, deprecatedrand.Seed, etc.).pkg/util: propagate buffer errors via panic and add coverage tests— finishes the errcheck work inpkg/util/buffer.go(the PR initial commit of bingen #1 copy only covered theWrite*methods; theRead*methods were still being flagged). AllRead*sites now route through a smallmust(error)helper so short reads / I/O errors surface as panics, matching the existing read-only-buffer contract.pkg/util/buffer_test.gois new and covers:Write*→Read*round-trips against bothbytes.Bufferandbufio.Readerbackings.Read*panics on an empty source.WriteStringtruncation atmath.MaxUint16.Peekrejection on rw-mode buffers.Bytes()drain-then-empty semantics on reader-backed buffers.tests/streaming: add streaming test fixture types— the newtests/streamingpackage fixture from initial commit of bingen #1.go.mod: require go 1.25— raises the module'sgodirective to1.25to match thestabletoolchain pinned in the new CI workflows.Verification
Run from repo root with Go 1.25.0:
go build ./...— clean.go vet ./...— clean.gofmt -l $(find . -name "*.go" -not -path "*/tests/*" -not -name "*_codecs.go")— empty.golangci-lint v2.6.1(built withgo1.25.3)run --max-issues-per-linter=0 --max-same-issues=0 ./...with the repo's new.golangci.yml—0 issues.go test ./pkg/... ./tests/... -count=1— all packages pass;pkg/utilcoverage is 93.2%.Notes
tests/generator_test.goregenerates the checked-in*_codecs.gofixture files when the suite runs. Whengo test ./...is invoked with-coverprofileit can occasionally race against the cover-tool compiling those sibling packages; this race pre-exists onmain(the PR initial commit of bingen #1 CI runs hit it only intermittently) and is out of scope here.tests/opencost/allocation.godoc reflow and its revertingTestImportFromPathconstant change —mainalready has the newer forms.tests/var_test.got.Error→t.Errorf(r)change, which would introduce agovet/printfwarning on a non-format string.tests/opencost/bingen.go-bufferflag swap —mainalready points atgithub.com/opencost/bingen/pkg/util, which is correct for this repo.go.modgo 1.23downgrade — we keep thego 1.25directive instead (see commit 6).