Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
📝 WalkthroughWalkthroughGoridge is bumped from v3 to v4 with Go 1.26; CI workflows and actions updated; frame protocol gains extensive header/flag/options APIs and CRC handling; msgpack codec support removed; new package docs, buffer pool preallocation, many tests and fuzzers added across frame, internal, and rpc packages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/bpool.go (2)
46-61:⚠️ Potential issue | 🟠 Major
get()panics ifPreallocate()was not called — self-initialize instead.
sync.Map.Loadreturns(nil, false)for a missing key. The subsequent type assertionval.(*sync.Pool)panics with "interface conversion: interface is nil, not *sync.Pool" in all three pool branches. Callers ofReceiveFramewho forget to callPreallocate()will hit a non-recoverable panic.The idiomatic fix is to call
preallocate.Do(internalAllocate)at the top ofget()(andput()), making initialization automatic and the explicitPreallocate()function either a no-op warm-up hint or removable.🛡️ Proposed fix — make get() self-initializing
func get(size uint32) *[]byte { + preallocate.Do(internalAllocate) switch { case size <= OneMB:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bpool.go` around lines 46 - 61, The get() function can panic because frameChunkedPool.Load may return nil if Preallocate() wasn't called; call preallocate.Do(internalAllocate) at the start of get() (and symmetrically at the start of put()) to ensure frameChunkedPool is initialized before using frameChunkedPool.Load and type-asserting to *sync.Pool; this makes Preallocate() optional (a warm-up hint) and prevents ReceiveFrame callers from triggering the nil interface type assertion panic.
63-78:⚠️ Potential issue | 🟠 Major
put()default case funnels oversized (>10 MB) buffers into the TenMB pool — pool contamination.
get()allocates a fresh, un-pooled slice forsize > TenMB. Butput()'sdefaultbranch coverssize > FiveMB, which includes both theFiveMB < size ≤ TenMBtier (correct) andsize > TenMB(wrong). Those oversized slices are stored in the TenMB pool, and a subsequentget(size ≤ TenMB)call can return a buffer much larger than 10 MB, breaking the pool's size invariant and causing unbounded memory growth.The fix is to add an explicit
case size <= TenMBand letdefaultsimply drop the oversized buffer (it was never from the pool).Additionally,
put()has the same nil-panic risk asget()— addpreallocate.Do(internalAllocate)here as well.🐛 Proposed fix — explicit TenMB case, discard oversized
func put(size uint32, data *[]byte) { + preallocate.Do(internalAllocate) switch { case size <= OneMB: pool, _ := frameChunkedPool.Load(OneMB) pool.(*sync.Pool).Put(data) return case size <= FiveMB: pool, _ := frameChunkedPool.Load(FiveMB) pool.(*sync.Pool).Put(data) return + case size <= TenMB: + pool, _ := frameChunkedPool.Load(TenMB) + pool.(*sync.Pool).Put(data) + return default: - pool, _ := frameChunkedPool.Load(TenMB) - pool.(*sync.Pool).Put(data) - return + // size > TenMB: was freshly allocated in get(), not pooled — discard. + return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bpool.go` around lines 63 - 78, The put function currently funnels buffers >TenMB into the TenMB pool causing pool contamination; update put(size uint32, data *[]byte) to call preallocate.Do(internalAllocate) at the start (same as get), add an explicit case for size <= TenMB that loads frameChunkedPool for TenMB and only then puts the buffer, make the default branch drop/return (do not put oversized buffers back), and guard the pool Put with a nil-check on data and the loaded pool from frameChunkedPool to avoid nil panics.
🧹 Nitpick comments (7)
.github/workflows/macos.yml (2)
30-34: Consider consolidating tests to avoid package list drift.Explicit package lists can miss new packages over time. If there’s no strong reason to split, a single
./...keeps coverage in sync.♻️ Possible consolidation
- go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./pkg/frame - go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./pkg/pipe - go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./pkg/rpc - go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./pkg/socket - go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./internal + go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/macos.yml around lines 30 - 34, The workflow repeats go test for specific packages (./pkg/frame, ./pkg/pipe, ./pkg/rpc, ./pkg/socket, ./internal) which can drift when packages are added; replace these multiple lines with a single consolidated invocation that runs go test -v -race -cover -tags=debug -coverpkg=./... -covermode=atomic ./... to cover all packages, preserving the existing flags and coverage settings; if there was an intentional reason to test those folders separately (e.g., different tags or environment), keep that rationale in a comment next to the commands instead of hard-coding an incomplete package list.
11-17: Pin Go version to avoid "stable" drift.Using
stablewill pull the latest stable Go toolchain, which may differ from the repo's declared Go version (1.26 in go.mod). This can cause CI behavior to change unexpectedly when new Go versions are released. Pin to a specific version or usego-version-file: go.modfor reproducible builds.Note: This pattern appears in linux.yml, windows.yml, and linters.yml as well; codeql-analysis.yml already uses the recommended
go-version-fileapproach.♻️ Suggested pinning (example)
- go: [stable] + go: ['1.26.x']Or alternatively:
- name: Set up Go ${{ matrix.go }} uses: actions/setup-go@v5 with: - go-version: ${{ matrix.go }} + go-version-file: go.mod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/macos.yml around lines 11 - 17, The workflow currently uses "stable" for the Go matrix and passes it into the actions/setup-go@v5 step via the go-version input (go-version: ${{ matrix.go }}), which can drift; change the setup step to pin the Go toolchain by either replacing the matrix value "stable" with the specific version from go.mod (e.g., "1.26") or by using the actions/setup-go input go-version-file: go.mod so setup-go reads the repo's go.mod; apply the same change pattern to the other workflow files that use the "stable" matrix entry (linux.yml, windows.yml, linters.yml) to ensure reproducible CI builds.internal/bpool.go (1)
7-8: Consider replacingsync.Map+sync.Oncewith plain package-levelsync.Poolvars.The three pools have fixed keys written exactly once. A
sync.Mapadds indirection, a load per call, and the nil-panic risk above. Three package-level*sync.Poolvariables initialized viavarorinit()are simpler, eliminate the uninitialized-panic class entirely, and have better cache locality.♻️ Suggested simpler alternative
-var frameChunkedPool = &sync.Map{} -var preallocate = &sync.Once{} +var ( + pool1MB = &sync.Pool{New: func() any { b := make([]byte, OneMB); return &b }} + pool5MB = &sync.Pool{New: func() any { b := make([]byte, FiveMB); return &b }} + pool10MB = &sync.Pool{New: func() any { b := make([]byte, TenMB); return &b }} +)
getandputthen switch directly on the pool vars, no map lookup needed.Preallocate()can be kept as a warm-up hint (callingGet+Putto pre-populate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bpool.go` around lines 7 - 8, Replace the package-level sync.Map (frameChunkedPool) and sync.Once (preallocate) with explicit package-level *sync.Pool variables initialized via var or init(), then update the get/put code paths to switch directly on those pool vars (e.g., replace lookups of frameChunkedPool with the concrete pool variable) and adjust Preallocate() to warm pools by calling pool.Get()/pool.Put() instead of using sync.Once; ensure functions named get, put and Preallocate reference the new *sync.Pool symbols and remove the sync.Map and sync.Once usage to eliminate indirection and nil-panic risks.pkg/frame/frame.go (1)
78-86:WriteVersionOR's the version into byte 0 without clearing existing version bits.If
WriteVersionis called more than once (e.g., after aResetthat doesn't zero the upper nibble, or if a caller sets the version twice), the old version bits are not cleared before OR-ing the new value, potentially corrupting the version field. This is documented behavior ("OR'd with the existing byte"), but worth noting that callers must not call it on a header that already has a version set — or the method should mask off the upper bits first.Defensive version: clear upper bits before writing
func (*Frame) WriteVersion(header []byte, version byte) { _ = header[0] if version > 15 { panic("version is only 4 bits") } - header[0] = version<<4 | header[0] + header[0] = (header[0] & 0x0F) | (version << 4) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 78 - 86, WriteVersion currently ORs the new version into header[0] without clearing the old upper nibble, so repeated calls can corrupt the version; update Frame.WriteVersion to first validate version as before, then clear the upper 4 bits of header[0] (mask with 0x0F) and OR in version<<4 (e.g., header[0] = (header[0] & 0x0F) | (version<<4)) to ensure the upper nibble is replaced rather than accumulated.pkg/rpc/codec_edge_test.go (1)
133-135: Bare type assertion onval.(byte)— prefer the two-value form.If
storeCodecever stores the codec under a different underlying type, this assertion panics and yields a confusing test failure rather than a descriptive one.♻️ Proposed fix
- val, ok := c.codec.Load(req.Seq) - assert.True(t, ok) - assert.Equal(t, tc.flag, val.(byte)) + val, ok := c.codec.Load(req.Seq) + assert.True(t, ok) + storedByte, ok := val.(byte) + assert.True(t, ok, "expected codec value to be stored as byte") + assert.Equal(t, tc.flag, storedByte)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/codec_edge_test.go` around lines 133 - 135, The test currently uses a bare type assertion val.(byte) which can panic; change it to the two-value form to safely detect type mismatches: after retrieving val via c.codec.Load(req.Seq) use b, ok := val.(byte) then assert.True(t, ok) and assert.Equal(t, tc.flag, b) so the test fails with a clear assertion instead of a panic (referencing c.codec.Load, req.Seq, and the val type assertion).pkg/frame/frame_fuzz_test.go (2)
96-131:payloadLenis a declared-but-unused fuzz parameter — wastes fuzzer entropy.
payloadLen uint32is accepted as a fuzz dimension butuint32(len(payload))is always used instead (line 115). The fuzzer will explore the fulluint32space for this parameter with no effect on code paths, diluting coverage of the other dimensions.♻️ Suggested fix — drop the unused parameter
- f.Add(byte(1), byte(0x08), uint32(5), []byte("hello")) - f.Add(byte(0), byte(0x20), uint32(0), []byte{}) - f.Add(byte(15), byte(0x04), uint32(3), []byte{0xFF, 0xFE, 0xFD}) + f.Add(byte(1), byte(0x08), []byte("hello")) + f.Add(byte(0), byte(0x20), []byte{}) + f.Add(byte(15), byte(0x04), []byte{0xFF, 0xFE, 0xFD}) - f.Fuzz(func(t *testing.T, version byte, flags byte, payloadLen uint32, payload []byte) { + f.Fuzz(func(t *testing.T, version byte, flags byte, payload []byte) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame_fuzz_test.go` around lines 96 - 131, The fuzz test's closure currently accepts an unused parameter payloadLen which wastes fuzzer entropy; update the f.Fuzz closure signature to remove the payloadLen parameter and any references to it (leave the rest of the body using uint32(len(payload)) for the payload length), and remove any now-unnecessary nolint/gosec comments tied to that parameter; locate the test by the f.Fuzz call in pkg/frame/frame_fuzz_test.go and modify the anonymous function signature and related declarations (NewFrame/WritePayloadLen/ReadPayloadLen/WritePayloadLen usage remains unchanged).
30-31:recover()in fuzz targets swallows panics and prevents the Go fuzzer from identifying panic-inducing corpus entries.Go's fuzzer marks inputs that cause an unrecovered panic as interesting corpus entries and tries to minimize them. Wrapping the entire fuzz body with
recover()prevents this, so a newly introduced panic inReadFrame,ReadOptions, orVerifyCRCwould silently pass the fuzz run. Consider removingrecover()from targets where panics are not expected (e.g.,FuzzFrameRoundTrip), or at minimum scoping recovery to known-safe panic sites so unexpected panics surface.Also applies to: 62-63, 79-88, 96-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame_fuzz_test.go` around lines 30 - 31, The fuzz target currently defers an unconditional recover() inside the f.Fuzz anonymous function which swallows panics and prevents the Go fuzzer from recording inputs that trigger real crashes; remove the top-level defer func() { recover() }() from the fuzz body in frame_fuzz_test.go (and similar occurrences at the other fuzz targets noted) so panics from ReadFrame, ReadOptions, VerifyCRC, etc. surface to the fuzzer, or if you must handle only known-safe panics, limit the recover to a narrow block around that specific call (not the whole Fuzz function) and keep FuzzFrameRoundTrip free of recovery.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/receive_test.go`:
- Around line 121-133: The test TestReceiveFrame_PayloadEOF currently uses
assert.Error(t, err, "unexpected EOF") which only checks for non-nil error and
its TODO suggests io.ErrUnexpectedEOF incorrectly; inspect ReceiveFrame (and
receive.go around the io.ReadFull call) and update the test to assert the
correct error type for a zero-byte read — replace the weak assert with
assert.ErrorIs(t, err, io.EOF) and update the comment/TODO to expect io.EOF
instead of io.ErrUnexpectedEOF so the test fails only for the correct EOF
behavior.
In `@pkg/frame/doc.go`:
- Around line 7-8: The package documentation incorrectly lists CodecMsgpack as a
supported codec even though v4 removed msgpack support; update the doc comment
that enumerates codecs (the line mentioning "CodecRaw, CodecJSON, CodecMsgpack,
CodecGob, CodecProto") to either remove CodecMsgpack or explicitly mark it as
"unsupported in v4 / removed" so the docs match runtime behavior for
CodecMsgpack.
In `@pkg/frame/frame.go`:
- Around line 159-162: The IsStop method omits the bounds-check hint used by its
siblings, causing inconsistent behavior for short header slices; update the
function Frame.IsStop to include the same BCE hint by adding the statement _ =
header[11] before accessing header[10], ensuring the early panic on short slices
and matching other methods like SetStreamFlag, IsStream, IsPing, SetPingBit,
IsPong, SetPongBit, and SetStopBit.
In `@pkg/rpc/codec_edge_test.go`:
- Around line 39-40: The inline comment is misleading: update the comment near
c.WriteResponse(resp, "some body") to state that this used to panic at
codec.(byte) because LoadAndDelete returned nil, but the test now asserts
recover() == nil (no panic) as a regression guard; reference c.WriteResponse,
codec.(byte), and LoadAndDelete in the comment and phrase it like "Previously
panicked at codec.(byte) due to LoadAndDelete returning nil; ensure no panic
now."
In `@pkg/rpc/codec.go`:
- Around line 96-98: The handle: WriteResponse currently calls handleError which
returns errors.E(op, errors.Str(r.Error)) losing the original err when r.Error
is empty; modify the handleError function (the function named handleError) so it
returns the actual error string passed in (err) wrapped via errors.E(op,
errors.Str(err)) instead of always using r.Error, preserving the detailed error
sent to the remote and ensuring callers of WriteResponse and other call sites
(the calls at lines around the codec nil check and other handleError
invocations) receive the original error context.
---
Outside diff comments:
In `@internal/bpool.go`:
- Around line 46-61: The get() function can panic because frameChunkedPool.Load
may return nil if Preallocate() wasn't called; call
preallocate.Do(internalAllocate) at the start of get() (and symmetrically at the
start of put()) to ensure frameChunkedPool is initialized before using
frameChunkedPool.Load and type-asserting to *sync.Pool; this makes Preallocate()
optional (a warm-up hint) and prevents ReceiveFrame callers from triggering the
nil interface type assertion panic.
- Around line 63-78: The put function currently funnels buffers >TenMB into the
TenMB pool causing pool contamination; update put(size uint32, data *[]byte) to
call preallocate.Do(internalAllocate) at the start (same as get), add an
explicit case for size <= TenMB that loads frameChunkedPool for TenMB and only
then puts the buffer, make the default branch drop/return (do not put oversized
buffers back), and guard the pool Put with a nil-check on data and the loaded
pool from frameChunkedPool to avoid nil panics.
---
Nitpick comments:
In @.github/workflows/macos.yml:
- Around line 30-34: The workflow repeats go test for specific packages
(./pkg/frame, ./pkg/pipe, ./pkg/rpc, ./pkg/socket, ./internal) which can drift
when packages are added; replace these multiple lines with a single consolidated
invocation that runs go test -v -race -cover -tags=debug -coverpkg=./...
-covermode=atomic ./... to cover all packages, preserving the existing flags and
coverage settings; if there was an intentional reason to test those folders
separately (e.g., different tags or environment), keep that rationale in a
comment next to the commands instead of hard-coding an incomplete package list.
- Around line 11-17: The workflow currently uses "stable" for the Go matrix and
passes it into the actions/setup-go@v5 step via the go-version input
(go-version: ${{ matrix.go }}), which can drift; change the setup step to pin
the Go toolchain by either replacing the matrix value "stable" with the specific
version from go.mod (e.g., "1.26") or by using the actions/setup-go input
go-version-file: go.mod so setup-go reads the repo's go.mod; apply the same
change pattern to the other workflow files that use the "stable" matrix entry
(linux.yml, windows.yml, linters.yml) to ensure reproducible CI builds.
In `@internal/bpool.go`:
- Around line 7-8: Replace the package-level sync.Map (frameChunkedPool) and
sync.Once (preallocate) with explicit package-level *sync.Pool variables
initialized via var or init(), then update the get/put code paths to switch
directly on those pool vars (e.g., replace lookups of frameChunkedPool with the
concrete pool variable) and adjust Preallocate() to warm pools by calling
pool.Get()/pool.Put() instead of using sync.Once; ensure functions named get,
put and Preallocate reference the new *sync.Pool symbols and remove the sync.Map
and sync.Once usage to eliminate indirection and nil-panic risks.
In `@pkg/frame/frame_fuzz_test.go`:
- Around line 96-131: The fuzz test's closure currently accepts an unused
parameter payloadLen which wastes fuzzer entropy; update the f.Fuzz closure
signature to remove the payloadLen parameter and any references to it (leave the
rest of the body using uint32(len(payload)) for the payload length), and remove
any now-unnecessary nolint/gosec comments tied to that parameter; locate the
test by the f.Fuzz call in pkg/frame/frame_fuzz_test.go and modify the anonymous
function signature and related declarations
(NewFrame/WritePayloadLen/ReadPayloadLen/WritePayloadLen usage remains
unchanged).
- Around line 30-31: The fuzz target currently defers an unconditional recover()
inside the f.Fuzz anonymous function which swallows panics and prevents the Go
fuzzer from recording inputs that trigger real crashes; remove the top-level
defer func() { recover() }() from the fuzz body in frame_fuzz_test.go (and
similar occurrences at the other fuzz targets noted) so panics from ReadFrame,
ReadOptions, VerifyCRC, etc. surface to the fuzzer, or if you must handle only
known-safe panics, limit the recover to a narrow block around that specific call
(not the whole Fuzz function) and keep FuzzFrameRoundTrip free of recovery.
In `@pkg/frame/frame.go`:
- Around line 78-86: WriteVersion currently ORs the new version into header[0]
without clearing the old upper nibble, so repeated calls can corrupt the
version; update Frame.WriteVersion to first validate version as before, then
clear the upper 4 bits of header[0] (mask with 0x0F) and OR in version<<4 (e.g.,
header[0] = (header[0] & 0x0F) | (version<<4)) to ensure the upper nibble is
replaced rather than accumulated.
In `@pkg/rpc/codec_edge_test.go`:
- Around line 133-135: The test currently uses a bare type assertion val.(byte)
which can panic; change it to the two-value form to safely detect type
mismatches: after retrieving val via c.codec.Load(req.Seq) use b, ok :=
val.(byte) then assert.True(t, ok) and assert.Equal(t, tc.flag, b) so the test
fails with a clear assertion instead of a panic (referencing c.codec.Load,
req.Seq, and the val type assertion).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/php_test_files/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.github/dependabot.yml.github/workflows/linux.yml.github/workflows/macos.yml.github/workflows/windows.yml.golangci.ymlMakefileREADME.mdbenchmarks/main.gogo.modinternal/bpool.gointernal/doc.gointernal/receive.gointernal/receive_test.gopkg/frame/doc.gopkg/frame/frame.gopkg/frame/frame_edge_test.gopkg/frame/frame_fuzz_test.gopkg/frame/frame_test.gopkg/pipe/doc.gopkg/pipe/pipe.gopkg/pipe/pipe_test.gopkg/relay/doc.gopkg/relay/interface.gopkg/rpc/client.gopkg/rpc/client_server_test.gopkg/rpc/codec.gopkg/rpc/codec_edge_test.gopkg/rpc/doc.gopkg/socket/doc.gopkg/socket/socket.gopkg/socket/socket_test.gotests/issues/issue_185_test.go
💤 Files with no reviewable changes (1)
- .github/dependabot.yml
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/frame/frame.go (4)
30-50:⚠️ Potential issue | 🟠 Major
ReadFramesilently drops stream/ping/pong/stop flags and mutates the caller's buffer.
f.headeris a slice alias ofdata, sof.header[10], f.header[11] = 0, 0also zeroesdata[10]anddata[11]in the caller's original buffer. More importantly, byte 10 is exactly whereSetStreamFlag,SetPingBit,SetPongBit, andSetStopBitwrite their bits — zeroing it on the read path means control-flag round-trips are broken: a sender that setsSTREAM|PING|PONG|STOPon byte 10 will have those bits silently cleared by the receiver. The with-options path (opt > 3) does not apply the same clearing, creating an asymmetry.🐛 Proposed fix
f := &Frame{ header: data[:12], payload: data[12:], } - - f.header[10], f.header[11] = 0, 0 - return f🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 30 - 50, ReadFrame mutates the caller's buffer and clears control flags by writing zeros to f.header[10] and f.header[11]; update ReadFrame so it does not modify the original data slice: allocate a new header copy when constructing the Frame (for both the default 12-byte header and the opt>3 path using opt*WORD) and clear bits on that copy only, preserving data[10]/data[11] in the caller buffer and keeping behavior symmetric between with-options and without-options paths; locate ReadFrame and replace the slice-aliasing header assignments that use data[:...] with a copied header buffer before zeroing bytes 10 and 11.
80-86:⚠️ Potential issue | 🟡 Minor
WriteVersioncorrupts the version field if any upper-nibble bits are already set.
header[0] = version<<4 | header[0]ORs the new version bits with whatever is already in the upper nibble. A second call — or any call after the upper nibble is non-zero — produces a wrong result (e.g., writing version2over version1yields version3).🐛 Proposed fix
- header[0] = version<<4 | header[0] + header[0] = (version << 4) | (header[0] & 0x0F)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 80 - 86, WriteVersion currently ORs the new version into header[0] which preserves any existing upper-nibble bits and corrupts the version on repeated writes; in Frame.WriteVersion clear the upper 4 bits of header[0] before setting the version (i.e., mask header[0] with 0x0F to keep the low nibble, then OR in version<<4) and keep the existing bounds check on version (>15) intact.
416-429:⚠️ Potential issue | 🟠 Major
AppendOptionshas two bugs: hardcoded write offset and missing HL update.
Hardcoded offset
j=12: same problem asWriteOptions— if the header already has options (length > 12), the loop overwrites bytes 12+ with the new data, corrupting existing options. The start offset should belen(*header).HL never updated: after the header is extended,
header[0]'s lower nibble still reflects the pre-call word count.ReadOptionsderives option count fromReadHL - 3; since HL is stale, the appended data is invisible to any reader.🐛 Proposed fix
-func (*Frame) AppendOptions(header *[]byte, options []byte) { +func (f *Frame) AppendOptions(header *[]byte, options []byte) { // make a new slice with the exact len (not doubled) newSl := make([]byte, len(options)+len(*header)) // copy old data copy(newSl, *header) - // j = 12 - first options byte - for i, j := 0, 12; i < len(options); i, j = i+1, j+1 { - newSl[j] = options[i] - } - + copy(newSl[len(*header):], options) // replace value *header = newSl + // update HL for each complete 4-byte word appended + for i := 0; i < len(options)/WORD; i++ { + f.incrementHL(*header) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 416 - 429, AppendOptions currently writes new options starting at a hardcoded offset j=12 and never updates the header length (HL), which corrupts existing options and leaves HL stale; modify AppendOptions to append options at the end of the current header by using start = len(*header) (or similar) instead of j=12, copy the options into the new slice starting at that computed start, and then update the header's HL (header[0] low nibble / word count) to reflect the new header length (so ReadHL/ReadOptions will see the appended bytes); keep the function name AppendOptions and ensure the same HL calculation logic used elsewhere (matching WriteOptions/ReadHL) is applied.
58-59:⚠️ Potential issue | 🟡 MinorStale comment: HL is initialised to
3, not2.
defaultHLcallswriteHl(header, 3), so the header length is 3 words (12 bytes).- // set default header len (2) + // set default header len (3 words = 12 bytes) f.defaultHL(f.header)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 58 - 59, The inline comment "// set default header len (2)" is stale — defaultHL(f.header) invokes writeHl(header, 3) so the header length is actually 3 words (12 bytes); update the comment next to the f.defaultHL(f.header) call to reflect "3" (e.g., "// set default header len (3) — 3 words / 12 bytes") or alternatively adjust defaultHL/writeHl if the intended default truly was 2, but be sure to reference defaultHL and writeHl consistently.
♻️ Duplicate comments (1)
pkg/frame/frame.go (1)
159-163:IsStopBCE hint correctly added — LGTM.The
_ = header[11]bounds-check hint is now present, consistent with all sibling methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 159 - 163, No change required: the bounds-check-elimination hint is already present in the IsStop method (function *Frame.IsStop) via the `_ = header[11]` statement and the method correctly returns `header[10]&STOP != 0`, so leave the implementation as-is.
🧹 Nitpick comments (1)
pkg/frame/frame.go (1)
96-102:incrementHL: misleading expression that is accidentally correct.
header[0] | hl + 1visually suggestsheader[0] | (hl + 1)(which would be wrong — it wouldn't clear old lower-nibble bits). In Go,|and+share precedence 4 and associate left-to-right, so it actually evaluates as(header[0] | hl) + 1. Becausehl = header[0] & 0x0F,header[0] | hlis alwaysheader[0], reducing the whole expression toheader[0] + 1— correct for valid inputs. Consider rewriting to make the intent clear:♻️ Proposed refactor
- header[0] = header[0] | hl + 1 + header[0]++🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/frame/frame.go` around lines 96 - 102, The expression in incrementHL is misleading and relies on operator precedence; instead explicitly preserve the high nibble of header[0] and replace only the low nibble with hl+1: compute hl via ReadHL(header), check the panic condition, then set header[0] to (high nibble of the original header[0]) OR (hl + 1) so the old low nibble is cleared before writing the incremented value; update the implementation in function incrementHL to perform those clear-and-set steps (use mask 0xF0 to keep the high nibble and combine with the new low nibble).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/frame/frame.go`:
- Around line 30-50: ReadFrame mutates the caller's buffer and clears control
flags by writing zeros to f.header[10] and f.header[11]; update ReadFrame so it
does not modify the original data slice: allocate a new header copy when
constructing the Frame (for both the default 12-byte header and the opt>3 path
using opt*WORD) and clear bits on that copy only, preserving data[10]/data[11]
in the caller buffer and keeping behavior symmetric between with-options and
without-options paths; locate ReadFrame and replace the slice-aliasing header
assignments that use data[:...] with a copied header buffer before zeroing bytes
10 and 11.
- Around line 80-86: WriteVersion currently ORs the new version into header[0]
which preserves any existing upper-nibble bits and corrupts the version on
repeated writes; in Frame.WriteVersion clear the upper 4 bits of header[0]
before setting the version (i.e., mask header[0] with 0x0F to keep the low
nibble, then OR in version<<4) and keep the existing bounds check on version
(>15) intact.
- Around line 416-429: AppendOptions currently writes new options starting at a
hardcoded offset j=12 and never updates the header length (HL), which corrupts
existing options and leaves HL stale; modify AppendOptions to append options at
the end of the current header by using start = len(*header) (or similar) instead
of j=12, copy the options into the new slice starting at that computed start,
and then update the header's HL (header[0] low nibble / word count) to reflect
the new header length (so ReadHL/ReadOptions will see the appended bytes); keep
the function name AppendOptions and ensure the same HL calculation logic used
elsewhere (matching WriteOptions/ReadHL) is applied.
- Around line 58-59: The inline comment "// set default header len (2)" is stale
— defaultHL(f.header) invokes writeHl(header, 3) so the header length is
actually 3 words (12 bytes); update the comment next to the
f.defaultHL(f.header) call to reflect "3" (e.g., "// set default header len (3)
— 3 words / 12 bytes") or alternatively adjust defaultHL/writeHl if the intended
default truly was 2, but be sure to reference defaultHL and writeHl
consistently.
---
Duplicate comments:
In `@pkg/frame/frame.go`:
- Around line 159-163: No change required: the bounds-check-elimination hint is
already present in the IsStop method (function *Frame.IsStop) via the `_ =
header[11]` statement and the method correctly returns `header[10]&STOP != 0`,
so leave the implementation as-is.
---
Nitpick comments:
In `@pkg/frame/frame.go`:
- Around line 96-102: The expression in incrementHL is misleading and relies on
operator precedence; instead explicitly preserve the high nibble of header[0]
and replace only the low nibble with hl+1: compute hl via ReadHL(header), check
the panic condition, then set header[0] to (high nibble of the original
header[0]) OR (hl + 1) so the old low nibble is cleared before writing the
incremented value; update the implementation in function incrementHL to perform
those clear-and-set steps (use mask 0xF0 to keep the high nibble and combine
with the new low nibble).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/receive_test.gopkg/frame/doc.gopkg/frame/frame.gopkg/rpc/codec.gopkg/rpc/codec_edge_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/frame/doc.go
- internal/receive_test.go
- pkg/rpc/codec_edge_test.go
- pkg/rpc/codec.go
1 participant
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s) or (git commit -S).CHANGELOG.md.Summary by CodeRabbit