Skip to content

ci(lint): forbid endian-unsafe patterns to protect s390x build#933

Merged
dangrondahl merged 1 commit into
mainfrom
add_lint_of_endianess
Jun 4, 2026
Merged

ci(lint): forbid endian-unsafe patterns to protect s390x build#933
dangrondahl merged 1 commit into
mainfrom
add_lint_of_endianess

Conversation

@dangrondahl
Copy link
Copy Markdown
Contributor

@dangrondahl dangrondahl commented Jun 4, 2026

Summary

  • Add .golangci.yml enabling forbidigo with rules that block binary.NativeEndian and unsafe.Pointer.
  • Both patterns are byte-order sensitive and would silently break on the s390x target added in feat: add linux/s390x build target to release distribution #930 (s390x is big-endian, all our CI runs on little-endian).
  • The codebase has zero usages today, so this is a regression guardrail rather than a fix.

A reviewer who wants to confirm the rule actually fires can drop _ = binary.NativeEndian into any .go file and re-run make lint.

Test plan

  • make lint passes locally with the new config (0 issues, 6 linters active including forbidigo)
  • CI lint job passes

Enable forbidigo with rules blocking binary.NativeEndian and
unsafe.Pointer, both of which are byte-order sensitive and would
silently break on the s390x build target (big-endian). The codebase
currently has zero usages; this is a guardrail against regressions.
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 4, 2026

Claude finished @dangrondahl's task in 3m 0s —— View job


PR Review: ci(lint): forbid endian-unsafe patterns

  • Run static analysis (build, vet, mod tidy)
  • Read changed files and existing config
  • Review code quality and correctness
  • Post review feedback

Static Analysis

All clean — go build ./..., go vet ./..., and go mod tidy pass with no issues.

Review

Overall: This is a clean, well-scoped guardrail. One new file, zero existing violations, clear error messages. LGTM.

Config correctness:

  • version: "2" matches the golangci-lint v2 format used by golangci/golangci-lint-action@v9 in CI (.github/workflows/test.yml:98).
  • enable: [forbidigo] adds to the default linter set — existing linters remain active. Good.
  • analyze-types: true is essential here — without it, forbidigo only does naive string matching against source text and would miss aliased imports (e.g., import bin "encoding/binary"bin.NativeEndian). With it, forbidigo resolves to the real package name regardless of alias.

Pattern review:

  • '^binary\.NativeEndian$' — anchored, escaped dot, exact match. Correct.
  • '^unsafe\.Pointer$' — same. The breadth of banning all unsafe.Pointer is worth noting: it catches more than just endian-sensitive reinterpretation (e.g., cgo interop, reflect hacks). However, since the codebase has zero usages today and the error message explicitly documents the //nolint:forbidigo escape hatch, this works well as a "stop and think" speed bump rather than a hard ban. Reasonable trade-off.

Error messages are clear and actionable — they explain why it's forbidden and what to do instead. This is what good lint messages look like.

CI integration is already wired: the lint job in test.yml runs golangci-lint-action@v9 which auto-discovers .golangci.yml. No CI changes needed.

Suggestions

No blocking issues. One minor thought:

  • Consider adding a comment at the top of .golangci.yml linking to the PR or issue that motivated these rules (e.g., # s390x portability guardrails — see #930). This helps future contributors understand why these seemingly unusual rules exist. Not required though — the lint messages themselves carry the context.

Good work

  • Smart use of analyze-types: true to catch alias imports
  • Differentiated messaging: NativeEndian says "use explicit endianness" (no escape), unsafe.Pointer says "prefer encoding/binary, or nolint with justification" (escape documented) — appropriate severity for each
  • Proactive guardrail before any violations exist, not reactive cleanup after a bug

Comment thread .golangci.yml
@dangrondahl dangrondahl enabled auto-merge (squash) June 4, 2026 07:22
@dangrondahl
Copy link
Copy Markdown
Contributor Author

Breaking it will produce this lint message:

cmd/kosli/assertSnapshot.go:64:6: use of `binary.NativeEndian` forbidden because "binary.NativeEndian is not portable; s390x is big-endian. Use binary.BigEndian or binary.LittleEndian explicitly." (forbidigo)
        _ = binary.NativeEndian
            ^
1 issues:
* forbidigo: 1

@dangrondahl dangrondahl merged commit 2c57076 into main Jun 4, 2026
20 checks passed
@dangrondahl dangrondahl deleted the add_lint_of_endianess branch June 4, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants