chore(ci): add go linter #5910
chore(ci): add go linter #5910
Conversation
WalkthroughThis change introduces a Go FFI layer for interoperability with Rust-generated C code, including new Go interfaces, exported C-callable functions, and utilities for type conversion and memory management. It also adds a Go linter workflow, a linter configuration file, a Makefile target for linting, and updates to environment variable error handling and build output paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Rust
participant C
participant Go_FFI (ffi_gen.go)
participant Go_Impl
Rust->>C: Expose interface via C FFI
C->>Go_FFI: Call exported Go function (e.g., CGoKadNode_run)
Go_FFI->>Go_Impl: Call Go interface method (e.g., GoKadNode.run)
Go_Impl-->>Go_FFI: Return result
Go_FFI->>C: Return result (with type conversion)
C->>Rust: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-07T13:39:36.941ZApplied to files:
📚 Learning: 2025-08-07T13:39:15.080ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9a55655 to
9b8395b
Compare
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.golangci.yml (1)
1-3: Consider expanding the linter configuration for better coverage.While the basic configuration is functional, consider adding more specific settings such as:
- Specific linters to enable/disable based on project needs
- Custom rules or severity levels
- File exclusions if needed
- Timeout settings for large codebases
Example enhanced configuration:
version: "2" linters: - default: standard + enable: + - gofmt + - golint + - govet + - ineffassign + - misspell + - errcheck +run: + timeout: 5m + skip-dirs: + - vendorinterop-tests/src/tests/go_app/gen.go (2)
48-56: Complex unsafe callback mechanism requires careful handlingThis function uses low-level unsafe operations and assembly calls through
asmcall.CallFuncG0P2. While theruntime.KeepAlivecalls are correctly placed to prevent premature garbage collection, this pattern is fragile and difficult to maintain.Since this is generated code, ensure the generation tool (rust2go) is kept up-to-date and well-tested, as bugs in this layer could cause memory corruption or crashes.
135-158: Variable shadows built-in functionLine 152 declares a variable named
lenwhich shadows the built-inlenfunction. While this works, it can cause confusion and potential bugs.- len := unsafe.Sizeof(child) - copy(children, unsafe.Slice((*byte)(unsafe.Pointer(&child)), len)) - children = children[len:] + size := unsafe.Sizeof(child) + copy(children, unsafe.Slice((*byte)(unsafe.Pointer(&child)), size)) + children = children[size:]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/go-lint.yml(1 hunks).golangci.yml(1 hunks)Makefile(2 hunks)f3-sidecar/ffi_impl.go(1 hunks)interop-tests/src/tests/go_app/.gitignore(0 hunks)interop-tests/src/tests/go_app/common.go(1 hunks)interop-tests/src/tests/go_app/gen.go(1 hunks)
💤 Files with no reviewable changes (1)
- interop-tests/src/tests/go_app/.gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
interop-tests/src/tests/go_app/common.go (1)
16-17: LGTM! Excellent improvement to error handling.The explicit error handling for
os.Setenvis a great improvement. Previously, setting the GODEBUG environment variable could fail silently, but now any errors will be caught and handled appropriately via thecheckErrorfunction.f3-sidecar/ffi_impl.go (1)
60-61: LGTM! Consistent error handling improvement.The explicit error handling for
os.Setenvmatches the improvement made in the test application and follows Go best practices. This ensures that environment variable setup failures are properly detected rather than silently ignored.Makefile (2)
82-84: LGTM! Well-structured Go linting target.The
lint-gotarget is well-designed with:
- Version pinning for reproducibility
- Reasonable timeout and concurrency settings
- Proper directory targeting for the Go code locations
The approach of using
go runwith version specification ensures consistent tooling across environments.
1-1: Golangci-lint version up to dateVersion v2.3.1 is the latest stable release as of August 2025. No change needed.
.github/workflows/go-lint.yml (3)
2-3: LGTM! Proper permissions configuration.The workflow correctly sets read-only permissions for contents, which addresses security best practices and the previous security scanning comment.
6-8: LGTM! Good concurrency control.The concurrency configuration properly cancels in-progress runs for feature branches while allowing main branch runs to complete, which optimizes CI resource usage.
26-29: LGTM! Excellent Go setup and integration.The workflow properly:
- Uses the latest actions versions (v4, v5)
- Configures Go version from
go.workfile for consistency- Integrates cleanly with the Makefile target
This provides a robust and maintainable linting pipeline.
interop-tests/src/tests/go_app/gen.go (1)
29-35: No nil-pointer risk: globals are initialized in init()Both
kad_impl.goandbitswap_impl.godefinefunc init()that assigns non-nil implementations toGoKadNodeImplandGoBitswapNodeImplbefore any exported C functions are callable. No additional nil checks are needed.
| package main | ||
|
|
||
| /* | ||
| // Generated by rust2go. Please DO NOT edit this C part manually. | ||
| #include <stdarg.h> | ||
| #include <stdbool.h> | ||
| #include <stdint.h> | ||
| #include <stdlib.h> | ||
| typedef struct ListRef { | ||
| const void *ptr; | ||
| uintptr_t len; | ||
| } ListRef; | ||
| typedef struct StringRef { | ||
| const uint8_t *ptr; | ||
| uintptr_t len; | ||
| } StringRef; | ||
| */ | ||
| import "C" | ||
| import ( | ||
| "runtime" | ||
| "unsafe" | ||
|
|
||
| "github.com/ihciah/rust2go/asmcall" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider excluding generated files from version control and linting
This file is marked as generated by rust2go with an explicit "DO NOT edit" warning. Generated files should typically be:
- Excluded from version control (regenerated as needed)
- Excluded from linting to avoid noise from auto-generated code
The AI summary indicates a .gitignore entry for gen.go was removed. Consider keeping generated files out of version control and adding this path to .golangci.yml exclusions.
🏁 Script executed:
#!/bin/bash
# Check if other generated files are tracked and if linting exclusions are configured
# Check for other generated files in the repository
echo "=== Searching for other generated files ==="
rg -l "Generated by|DO NOT edit|Code generated" --type go
# Check if gen.go is excluded from linting in .golangci.yml
echo -e "\n=== Checking .golangci.yml for exclusions ==="
if [ -f ".golangci.yml" ]; then
cat .golangci.yml | grep -A 10 -B 2 "skip-files\|skip-dirs\|exclude"
fi
# Check if there's a generation script or Makefile target
echo -e "\n=== Checking for generation commands ==="
if [ -f "Makefile" ]; then
grep -n "gen\.go\|rust2go" Makefile
fiLength of output: 578
Exclude generated Go files from version control and linting
We discovered two tracked generated files without any lint-exclusion rules:
interop-tests/src/tests/go_app/gen.gof3-sidecar/ffi_gen.go
And .golangci.yml contains no skip-files, skip-dirs, or exclude entries for them.
Action items:
- Add these paths (or a regex) to
.gitignoreso they aren’t accidentally committed in the future. - Configure GolangCI to skip linting on all
*_gen.gofiles.
Suggested diff:
# .gitignore
+# Ignore Rust2Go-generated Go code
+**/*_gen.go
# .golangci.yml
run:
+ skip-files:
+ - '.*_gen\.go$'Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In interop-tests/src/tests/go_app/gen.go lines 1 to 27, the file is a generated
Go file but is currently tracked in version control and not excluded from
linting. To fix this, add the path interop-tests/src/tests/go_app/gen.go or a
suitable pattern matching generated files to the .gitignore file to prevent
accidental commits. Additionally, update the .golangci.yml configuration to
include a skip-files or exclude rule that matches all *_gen.go files, ensuring
these generated files are excluded from linting.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@LesnyRumcajs Fixed. (The suggested change does not match the config syntax though)
| func newString(s_ref C.StringRef) string { | ||
| return unsafe.String((*byte)(unsafe.Pointer(s_ref.ptr)), s_ref.len) | ||
| } | ||
| func refString(s *string, _ *[]byte) C.StringRef { | ||
| return C.StringRef{ | ||
| ptr: (*C.uint8_t)(unsafe.StringData(*s)), | ||
| len: C.uintptr_t(len(*s)), | ||
| } | ||
| } | ||
|
|
||
| func ownString(s_ref C.StringRef) string { | ||
| return string(unsafe.Slice((*byte)(unsafe.Pointer(s_ref.ptr)), int(s_ref.len))) | ||
| } |
There was a problem hiding this comment.
Memory safety concern with string conversion functions
The newString function (line 89) creates a Go string that aliases the C memory without copying, while ownString (line 99) creates a proper copy. Using newString in the exported functions (lines 44, 73, 79) could lead to use-after-free if the C side deallocates the memory while Go still holds references.
Consider using ownString instead of newString in the exported functions for safety, unless you're certain the C memory remains valid for the entire duration of the Go function calls.
🤖 Prompt for AI Agents
In interop-tests/src/tests/go_app/gen.go around lines 88 to 100, the newString
function returns a Go string referencing C memory without copying, risking
use-after-free if C deallocates it. To fix this, replace calls to newString in
the exported functions at lines 44, 73, and 79 with ownString, which safely
copies the C memory into a Go string, ensuring memory safety.
| @@ -0,0 +1,29 @@ | |||
| name: Go code linters | |||
There was a problem hiding this comment.
Does it make sense to invoke this exclusively for changes in the Go source code directories + go.work etc?
There was a problem hiding this comment.
Good idea, fixed.
26900a5 to
302ab09
Compare
Makefile
Outdated
| @@ -1,3 +1,5 @@ | |||
| GOLANGCI_LINT_VERSION=v2.3.1 | |||
There was a problem hiding this comment.
does it have to be global for the makefile? can it be just part of the go run... command?
There was a problem hiding this comment.
It's easier to upgrade when it's at the top, what do you think?
There was a problem hiding this comment.
Yeah, but it's not used anywhere else. It adds unnecessary noise for anyone NOT upgrading it.
There was a problem hiding this comment.
That makes sense, fixed.
9c98004 to
26b7ac0
Compare
|
no green checkmark! |
There was a problem hiding this comment.
So, could we not add it to the source?
There was a problem hiding this comment.
Go linter does not work without it. Generating it requires running rust build
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignorefor generated files, allowing them to be tracked.