chore/code-cleanup#173
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates build metadata injection and version parsing, canonicalizes BashExecutionMessage JSON output while keeping legacy decoding support, refactors skill stop-word matching, adds private-network checks to fetch, and caches parsed default ignore patterns. ChangesBuild metadata and version formatting
Bash execution message JSON compatibility
Skill stop-word matching
Fetch private-network validation
Ignore pattern cache
Sequence Diagram(s)sequenceDiagram
participant fetchURL
participant httpClientWithRedirectValidation
participant transportWithNetworkValidation
participant validateFetchDialedConnection
participant validatePublicFetchRemoteAddr
fetchURL->>httpClientWithRedirectValidation: get client and cleanup callback
httpClientWithRedirectValidation->>transportWithNetworkValidation: clone transport and wrap dialers
fetchURL->>httpClientWithRedirectValidation: client.Do(request)
transportWithNetworkValidation->>validateFetchDialedConnection: validate dialed connection
validateFetchDialedConnection->>validatePublicFetchRemoteAddr: inspect remote addr
validatePublicFetchRemoteAddr-->>validateFetchDialedConnection: allow or reject
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
==========================================
- Coverage 82.97% 82.88% -0.10%
==========================================
Files 287 287
Lines 22977 23112 +135
==========================================
+ Hits 19066 19156 +90
- Misses 2768 2799 +31
- Partials 1143 1157 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/vinfo/version_internal_test.go (1)
76-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse these fallback cases into one table-driven test.
These three tests exercise the same core path with different
debug.BuildInfoinputs. Converting them into one table will make it much easier to extend the fallback matrix without duplicating setup. As per coding guidelines,**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs in Go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vinfo/version_internal_test.go` around lines 76 - 143, The three tests around build metadata fallback duplicate the same setup and assertion path, so consolidate them into one table-driven test in version_internal_test.go. Refactor TestBuildMetadataFallsBackToBuildInfo, TestBuildMetadataUsesModuleVersion, and TestBuildMetadataPreservesInjectedValues into a single test that iterates over cases covering the different build info inputs and expected buildMetadata outputs, while keeping the fallback logic exercised through buildMetadata.withBuildInfoFallback.Source: Coding guidelines
.goreleaser.yaml (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
.ShortCommitexplicitly here.GoReleaser documents
.Commitas deprecated and exposes.ShortCommit/.FullCommitas the explicit replacements. Using.Commithere keeps release builds on deprecated behavior and can diverge from the short hash already injected byTaskfile.yml. (goreleaser.com)♻️ Suggested change
- - -X github.com/omarluq/librecode/internal/vinfo.version={{.Version}}|{{.Commit}}|{{.Date}} + - -X github.com/omarluq/librecode/internal/vinfo.version={{.Version}}|{{.ShortCommit}}|{{.Date}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.goreleaser.yaml at line 29, The GoReleaser version injection still uses the deprecated .Commit template value; update the linker flag in the release config to use .ShortCommit explicitly instead so the built version string matches the intended short hash behavior used elsewhere. Locate the ldflags entry that sets github.com/omarluq/librecode/internal/vinfo.version and replace the commit template reference with the explicit short-commit variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tool/fetch.go`:
- Around line 333-340: The fallback paths in fetchTransport creation currently
return intentionally empty cleanup closures, which Sonar flags as empty blocks.
In the fetch tool logic around allowPrivateNetworks and cloneFetchHTTPTransport,
add a brief inline comment inside each no-op func() {} to make it explicit that
no cleanup is needed for the baseTransport fallback, while keeping the existing
return behavior unchanged.
- Around line 342-347: The transport hardening in fetch.go only wraps
http.Transport DialContext and DialTLSContext, so legacy Dial and DialTLS
settings can still bypass the validation path. Update the transport setup in the
fetch logic that uses validatingFetchDialContext to either wrap the legacy
Dial/DialTLS hooks too or explicitly reject transports that set them, and keep
the remote-address check enforced consistently for both HTTP and HTTPS.
In `@internal/vinfo/version.go`:
- Around line 87-89: The built timestamp fallback in version metadata is using
vcs.time from build info, which is a revision time and not an actual binary
build time. Update the logic in metadata.buildDate handling to avoid assigning
buildInfoSetting(info.Settings, buildInfoTimeKey) to built=; keep the value as
unknown when no real build timestamp is injected, or rename the field/label in
the version output to make it explicitly VCS-based.
---
Nitpick comments:
In @.goreleaser.yaml:
- Line 29: The GoReleaser version injection still uses the deprecated .Commit
template value; update the linker flag in the release config to use .ShortCommit
explicitly instead so the built version string matches the intended short hash
behavior used elsewhere. Locate the ldflags entry that sets
github.com/omarluq/librecode/internal/vinfo.version and replace the commit
template reference with the explicit short-commit variant.
In `@internal/vinfo/version_internal_test.go`:
- Around line 76-143: The three tests around build metadata fallback duplicate
the same setup and assertion path, so consolidate them into one table-driven
test in version_internal_test.go. Refactor
TestBuildMetadataFallsBackToBuildInfo, TestBuildMetadataUsesModuleVersion, and
TestBuildMetadataPreservesInjectedValues into a single test that iterates over
cases covering the different build info inputs and expected buildMetadata
outputs, while keeping the fallback logic exercised through
buildMetadata.withBuildInfoFallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8d711ff-4d03-4cb2-b5a8-28053b9780da
📒 Files selected for processing (11)
.goreleaser.yamlTaskfile.ymlinternal/core/messages.gointernal/core/messages_test.gointernal/core/skills_activation.gointernal/core/skills_test.gointernal/tool/fetch.gointernal/tool/fetch_internal_test.gointernal/tool/ignore.gointernal/vinfo/version.gointernal/vinfo/version_internal_test.go
| if fetchTool.allowPrivateNetworks { | ||
| return baseTransport, func() {} | ||
| } | ||
|
|
||
| transport, ok := cloneFetchHTTPTransport(baseTransport) | ||
| if !ok { | ||
| return baseTransport, func() {} | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the intentional no-op closers.
CI is flagging both empty closures. Add a short comment inside each no-op so the fallback is explicit and Sonar passes.
Proposed fix
if fetchTool.allowPrivateNetworks {
- return baseTransport, func() {}
+ return baseTransport, func() {
+ // No cloned transport is owned here, so there are no idle connections to close.
+ }
}
transport, ok := cloneFetchHTTPTransport(baseTransport)
if !ok {
- return baseTransport, func() {}
+ return baseTransport, func() {
+ // Non-http transports are not cloned here, so cleanup remains with the owner.
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if fetchTool.allowPrivateNetworks { | |
| return baseTransport, func() {} | |
| } | |
| transport, ok := cloneFetchHTTPTransport(baseTransport) | |
| if !ok { | |
| return baseTransport, func() {} | |
| } | |
| if fetchTool.allowPrivateNetworks { | |
| return baseTransport, func() { | |
| // No cloned transport is owned here, so there are no idle connections to close. | |
| } | |
| } | |
| transport, ok := cloneFetchHTTPTransport(baseTransport) | |
| if !ok { | |
| return baseTransport, func() { | |
| // Non-http transports are not cloned here, so cleanup remains with the owner. | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 334-334: Add a nested comment explaining why this function is empty or complete the implementation.
[failure] 339-339: Add a nested comment explaining why this function is empty or complete the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tool/fetch.go` around lines 333 - 340, The fallback paths in
fetchTransport creation currently return intentionally empty cleanup closures,
which Sonar flags as empty blocks. In the fetch tool logic around
allowPrivateNetworks and cloneFetchHTTPTransport, add a brief inline comment
inside each no-op func() {} to make it explicit that no cleanup is needed for
the baseTransport fallback, while keeping the existing return behavior
unchanged.
Source: Linters/SAST tools
| transport.Proxy = nil | ||
| transport.DialContext = validatingFetchDialContext(fetchDialContext(transport)) | ||
|
|
||
| if transport.DialTLSContext != nil { | ||
| transport.DialTLSContext = validatingFetchDialContext(transport.DialTLSContext) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find custom http.Transport dial hooks that need preservation/validation.
# Expectation: Any Dial/DialTLS usage should remain behaviorally equivalent after wrapping.
rg -nP --type=go -C3 '\bDial(TLS|TLSContext|Context)?\s*:' .Repository: omarluq/librecode
Length of output: 748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation around the cited lines and related helpers/tests.
git ls-files 'internal/tool/fetch.go' 'internal/tool/fetch_internal_test.go'
wc -l internal/tool/fetch.go internal/tool/fetch_internal_test.go
sed -n '320,390p' internal/tool/fetch.go
printf '\n--- tests around transport setup ---\n'
sed -n '480,560p' internal/tool/fetch_internal_test.go
printf '\n--- search for legacy dial hooks in repo ---\n'
rg -n --type=go '\bDialTLSContext\b|\bDialTLS\b|\bDial\b' internal/toolRepository: omarluq/librecode
Length of output: 4752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for other cloning/sanitizing logic for http.Transport in the codebase.
rg -n --type=go 'Clone\(\)|\bProxy\s*=\s*nil|\bDialContext\s*=\s*validatingFetchDialContext|\bvalidateFetchDialedConnection\b' internalRepository: omarluq/librecode
Length of output: 1141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the implementation and helper definitions that matter for the transport wrapping logic.
sed -n '300,420p' internal/tool/fetch.go
printf '\n---\n'
sed -n '1,140p' internal/tool/fetch.goRepository: omarluq/librecode
Length of output: 7693
🌐 Web query:
Go net/http Transport Clone copies Dial and DialTLS legacy hooks DialTLSContext validation behavior
💡 Result:
The net/http.Transport.Clone method performs a shallow copy of the Transport's exported fields, which includes the legacy Dial, DialTLS, and modern DialTLSContext hooks [1][2]. Specifically regarding these hooks: - The fields Dial, DialTLS, and DialTLSContext are copied directly from the original Transport to the new one [2]. - Because they are function types, the cloned Transport will point to the exact same dialer functions as the original Transport [2]. - There is no special validation logic for these hooks during the cloning process [2]. Regarding DialTLSContext behavior specifically: - The Transport prioritizes DialTLSContext over the deprecated DialTLS if both are configured [3][2]. - When you clone a Transport, this priority logic remains preserved in the new instance because the fields themselves are copied as-is [2]. It is important to note that while Clone is intended to create a usable copy, it has historically been subject to various edge-case issues—particularly concerning internal state like HTTP/2 configuration (TLSNextProto and ForceAttemptHTTP2) [4][5]. Users should be aware that if the original Transport has not yet been initialized (via a request), the cloned Transport might behave differently than expected due to lazy initialization of default values [5]. Always ensure the Transport state is fully configured if you intend to rely on specific protocol or dialer behaviors after cloning [4][5].
Citations:
- 1: https://go.dev/src/net/http/transport.go?m=text
- 2: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 3: https://go.googlesource.com/go/+/master/src/net/http/transport.go
- 4: net/http: Transport.Clone breaks HTTP/2 golang/go#39302
- 5: net/http: Transport.Clone produces broken HTTP/2 state when original transport was auto-configured golang/go#79418
🌐 Web query:
Go net/http Transport DialTLS DialTLSContext precedence and Clone documentation
💡 Result:
In the Go net/http package, the DialTLSContext and DialTLS fields on the http.Transport struct manage TLS connection dialing for HTTPS requests, with specific precedence rules [1][2]. Precedence and Behavior: 1. If DialTLSContext is provided, it takes precedence [1][2]. 2. If DialTLSContext is nil, the transport falls back to the deprecated DialTLS [1][2]. 3. If both DialTLSContext and DialTLS are nil, the transport uses the configured DialContext (or the deprecated Dial) in combination with the Transport's TLSClientConfig to establish the connection [1][2]. 4. When DialTLSContext is set, the Dial and DialContext hooks are ignored for HTTPS requests, as are the TLSClientConfig and TLSHandshakeTimeout fields [1][2]. The DialTLS and Dial hooks are deprecated in favor of DialTLSContext and DialContext, respectively, because the context-aware versions allow the transport to cancel dials immediately when they are no longer needed [1][2]. Clone Documentation: The Clone method on http.Transport returns a deep copy of the Transport's exported fields [3]. This is useful for creating a modified version of an existing transport without affecting the original [4][3]. The clone process performs a shallow copy of most configuration fields (such as dialers and timeout durations) while creating new, independent instances for fields that require deep copying to prevent shared-state side effects, such as the TLSClientConfig [3]. Note that the internal state—such as connection pools or synchronization primitives—is managed appropriately to ensure the cloned transport is safe for use independently [3].
Citations:
- 1: https://go.dev/src/net/http/transport.go?m=text
- 2: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 3: https://github.com/golang/go/blob/go1.16.3/src/net/http/transport.go
- 4: https://pkg.go.dev/net/http?utm_source=godoc
🌐 Web query:
Go net/http Transport Clone copies Dial and DialTLS legacy hooks DialTLSContext validation behavior
💡 Result:
In Go's net/http package, the Transport.Clone method creates a shallow copy of the Transport structure, specifically copying exported fields, including the dial hooks [1][2][3]. When Clone is called, both the deprecated DialTLS and the modern DialTLSContext hooks are copied from the original Transport to the new instance [1][2]. Regarding their behavior and validation: 1. Prioritization: When both DialTLSContext and the deprecated DialTLS are set on a Transport (whether original or cloned), DialTLSContext takes precedence [1][4][2]. 2. Legacy Hook Handling: The Transport continues to support the legacy DialTLS hook for backward compatibility [1][2]. However, the documentation explicitly recommends using DialTLSContext instead, as it allows for better control, such as canceling dials when they are no longer needed [1][4]. 3. Validation and Fallback: If DialTLSContext is nil and DialTLS is also nil, the Transport falls back to using the DialContext (or Dial) hook combined with the TLSClientConfig to initiate TLS connections [1][4]. 4. Connection Behavior: If DialTLSContext is provided, the Transport ignores the standard Dial/DialContext hooks for HTTPS requests, as well as the TLSClientConfig and TLSHandshakeTimeout [1][4][2]. The returned net.Conn is expected to be already established past the TLS handshake [1][4]. Users should be aware that while Clone copies these hooks, it does not perform validation to ensure they are mutually compatible or optimal; it simply replicates the configuration [1][3]. Furthermore, while Clone is useful for creating variations of a Transport, users have historically reported issues regarding the cloning of internal state (such as HTTP/2 configuration) when the original transport was auto-configured [5][6], so it is generally best practice to ensure the cloned Transport is explicitly configured if specific behavior is required.
Citations:
- 1: https://go.dev/src/net/http/transport.go?m=text
- 2: https://go.googlesource.com/go/+/master/src/net/http/transport.go
- 3: https://github.com/golang/go/blob/go1.16.3/src/net/http/transport.go
- 4: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 5: net/http: Transport.Clone produces broken HTTP/2 state when original transport was auto-configured golang/go#79418
- 6: net/http: Transport.Clone breaks HTTP/2 golang/go#39302
Wrap legacy transport dial hooks too. http.Transport.Clone() preserves Dial/DialTLS, but this path only wraps DialContext and DialTLSContext. A transport that relies on legacy Dial loses its custom dialer here, and legacy DialTLS still bypasses the remote-address check for HTTPS. Wrap those hooks as well, or reject transports that use them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tool/fetch.go` around lines 342 - 347, The transport hardening in
fetch.go only wraps http.Transport DialContext and DialTLSContext, so legacy
Dial and DialTLS settings can still bypass the validation path. Update the
transport setup in the fetch logic that uses validatingFetchDialContext to
either wrap the legacy Dial/DialTLS hooks too or explicitly reject transports
that set them, and keep the remote-address check enforced consistently for both
HTTP and HTTPS.
| if metadata.buildDate == defaultBuildDate { | ||
| metadata.buildDate = metadataPart(buildInfoSetting(info.Settings, buildInfoTimeKey), defaultBuildDate) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files 'internal/vinfo/version.go' 'cmd/librecode/root.go' 'cmd/librecode/version.go' 'internal/terminal/welcome.go' | sed 's#^`#-` #'
printf '\n== outline internal/vinfo/version.go ==\n'
ast-grep outline internal/vinfo/version.go --view expanded || true
printf '\n== relevant symbols/strings ==\n'
rg -n "buildInfoTimeKey|buildDate|built=|vcs.time|defaultBuildDate|metadataPart|withBuildInfoFallback|buildInfoSetting" internal/vinfo/version.go cmd/librecode/root.go cmd/librecode/version.go internal/terminal/welcome.go
printf '\n== read internal/vinfo/version.go around relevant lines ==\n'
nl -ba internal/vinfo/version.go | sed -n '1,220p'Repository: omarluq/librecode
Length of output: 3408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files 'internal/vinfo/version.go' 'cmd/librecode/root.go' 'cmd/librecode/version.go' 'internal/terminal/welcome.go' | sed 's#^`#-` #'
printf '\n== outline internal/vinfo/version.go ==\n'
ast-grep outline internal/vinfo/version.go --view expanded || true
printf '\n== relevant symbols/strings ==\n'
rg -n "buildInfoTimeKey|buildDate|built=|vcs.time|defaultBuildDate|metadataPart|withBuildInfoFallback|buildInfoSetting" internal/vinfo/version.go cmd/librecode/root.go cmd/librecode/version.go internal/terminal/welcome.go
printf '\n== read internal/vinfo/version.go around relevant lines ==\n'
nl -ba internal/vinfo/version.go | sed -n '1,220p'Repository: omarluq/librecode
Length of output: 3408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files 'internal/vinfo/version.go' 'cmd/librecode/root.go' 'cmd/librecode/version.go' 'internal/terminal/welcome.go' | sed 's#^`#-` #'
printf '\n== outline internal/vinfo/version.go ==\n'
ast-grep outline internal/vinfo/version.go --view expanded || true
printf '\n== relevant symbols/strings ==\n'
rg -n "buildInfoTimeKey|buildDate|built=|vcs.time|defaultBuildDate|metadataPart|withBuildInfoFallback|buildInfoSetting" internal/vinfo/version.go cmd/librecode/root.go cmd/librecode/version.go internal/terminal/welcome.go
printf '\n== read internal/vinfo/version.go around relevant lines ==\n'
nl -ba internal/vinfo/version.go | sed -n '1,220p'Repository: omarluq/librecode
Length of output: 3408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba internal/vinfo/version.go | sed -n '1,220p'
printf '\n---\n'
rg -n "built=|buildDate|vcs.time|defaultBuildDate|withBuildInfoFallback|buildInfoTimeKey" -S internal cmdRepository: omarluq/librecode
Length of output: 195
Don’t use vcs.time as built=
runtime/debug’s vcs.time is the revision timestamp, not the binary build time. Falling back to it makes built= misleading whenever ldflags don’t inject a real build timestamp; keep unknown here or rename the field to something VCS-specific.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/vinfo/version.go` around lines 87 - 89, The built timestamp fallback
in version metadata is using vcs.time from build info, which is a revision time
and not an actual binary build time. Update the logic in metadata.buildDate
handling to avoid assigning buildInfoSetting(info.Settings, buildInfoTimeKey) to
built=; keep the value as unknown when no real build timestamp is injected, or
rename the field/label in the version output to make it explicitly VCS-based.
1 participant
No description provided.