vpn: treat the server-driven proxyless rule-set detour as infrastructure#549
Merged
Conversation
Remote rule-sets (geosite-cn, geosite-ir, geoip-ru, ...) are fetched at cold start over download_detour:direct, which censors throttle at the SNI layer. Repoint every direct-fetched remote rule-set to a proxyless "mirror" outbound — the outline-sdk smart dialer with DoH resolution + TLS-record fragmentation — so the fetch survives the throttle without a proxy server. Detour-only (not a selectable proxy); the outbound is injected only when a rule-set was actually repointed, and its first strategy is plain direct so uncensored fetches are unaffected. Region-agnostic: replaces the earlier geosite-cn-only scoping, since IR/RU rule-sets fetch from the same throttled CDNs (jsDelivr, raw.githubusercontent). Includes cmd/geosite-throttle-test, the CN-residential harness used to validate that fragmentation defeats the SNI-keyed throttle (fastly.jsdelivr + tlsfrag: full .srs in 3-5s; s3 excluded — its throttle is rate-based). engineering#3657 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds server-declared non-selectable outbound tags to VPN box options, wires them from backend config and tests the filtering, updates one dependency, and introduces a CLI that measures HTTPS fetch behavior through an Oxylabs CONNECT tunnel under multiple dialing strategies. ChangesNon-selectable outbound tags
Geosite throttle test CLI
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR routes sing-box remote rule-set downloads through a new proxyless “mirror” detour outbound (outline smart dialer with DoH + TLS record fragmentation) to make cold-start rule-set fetches more resilient to SNI-based DPI throttling, and adds a validation harness for CN-residential testing.
Changes:
- Add a
mirroroutbound (Outline smart dialer) and rewrite remote rule-sets withdownload_detour: direct/empty to use it. - Inject the outbound into built options only when a rule-set was repointed.
- Add unit tests for the repointing logic and a standalone throttle-test command.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| vpn/mirror.go | Introduces the mirror outbound definition and rule-set repointing logic. |
| vpn/mirror_test.go | Adds tests covering rule-set repointing behavior. |
| vpn/boxoptions.go | Applies repointing post-merge and conditionally injects the mirror outbound. |
| cmd/geosite-throttle-test/main.go | Adds a CLI harness to validate fragmentation effectiveness via a CN residential CONNECT proxy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
buildOptions now repoints direct-fetched remote rule-sets to the mirror download_detour, so the golden assertions must expect that detour. Add an expectMirrorDetour helper mirroring repointRuleSetsToMirror and apply it to the smart-routing and ad-block expected rule-sets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/geosite-throttle-test/main.go (3)
53-53: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnchecked type assertion.
c.(*net.TCPConn)panics if the dial ever returns a differentnet.Connimplementation. Givennet.Dialer.DialContextwith network"tcp", this is currently safe, but a comma-ok assertion avoids a hard panic if this is ever refactored.🤖 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 `@cmd/geosite-throttle-test/main.go` at line 53, The type assertion in the geosite throttle test is unchecked and can panic if DialContext ever returns a non-TCPConn implementation. Update the connection handling in main.go around the tcp variable assignment to use a comma-ok assertion and handle the failure path gracefully instead of assuming c is always a *net.TCPConn.
82-92: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMissing
MinVersionon thetls.Config.Static analysis flags the absence of
tls.MinVersionhere. For this diagnostic tool the risk is minimal (fetches a public rule-set file, no sensitive data), but setting a floor is a trivial hardening.🔧 Proposed fix
- TLSClientConfig: &tls.Config{NextProtos: []string{"http/1.1"}}, + TLSClientConfig: &tls.Config{NextProtos: []string{"http/1.1"}, MinVersion: tls.VersionTLS12},🤖 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 `@cmd/geosite-throttle-test/main.go` around lines 82 - 92, The fetch helper’s TLS setup in fetch uses a tls.Config without an explicit minimum version, which triggers the static analysis warning. Update the tls.Config passed to http.Transport so it sets a MinVersion floor alongside NextProtos, keeping the existing client behavior unchanged while hardening the TLS handshake.Source: Linters/SAST tools
43-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicates the CONNECT-tunnel pattern from
bypass.go.
connectDialer/bufConnreimplement the handshake and buffered-read wrapping already present ashttpConnect/bufferedConninbypass/bypass.go. Since this is a throwaway diagnostic command kept independent of the main module's internal packages, this is optional rather than essential.🤖 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 `@cmd/geosite-throttle-test/main.go` around lines 43 - 77, The CONNECT tunnel logic in connectDialer.DialStream and bufConn duplicates the existing httpConnect/bufferedConn implementation from bypass.go. Replace the ad hoc handshake and buffered-read wrapper with the shared helper pattern if it can be reused cleanly, or otherwise keep the diagnostic copy intentionally isolated and minimal so it does not drift from the main implementation.
🤖 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 `@cmd/geosite-throttle-test/main.go`:
- Around line 48-68: DialStream is missing a read deadline for the CONNECT
handshake, so the http.ReadResponse call can block forever after the TCP
connection is established. Update connectDialer.DialStream to mirror the timeout
handling used by bypass.go’s httpConnect helper: set a deadline on the TCPConn
before reading the CONNECT response, then clear it after the response is read
successfully or on error. Keep the existing error handling around fmt.Errorf,
tcp.Close, and resp.Body.Close intact.
In `@vpn/boxoptions.go`:
- Line 448: Reserve the mirror tag before calling mergeAndCollectTags for the
injected outbound in boxoptions.go, since bOptions.Options is merged first and a
user-defined entry tagged mirror can collide with the appended outbound. Update
the tag collection logic around mergeAndCollectTags/opts/bOptions.Options so
mirror is pre-reserved or checked for duplicates before appending the outbound,
preventing sing-box startup rejection.
---
Nitpick comments:
In `@cmd/geosite-throttle-test/main.go`:
- Line 53: The type assertion in the geosite throttle test is unchecked and can
panic if DialContext ever returns a non-TCPConn implementation. Update the
connection handling in main.go around the tcp variable assignment to use a
comma-ok assertion and handle the failure path gracefully instead of assuming c
is always a *net.TCPConn.
- Around line 82-92: The fetch helper’s TLS setup in fetch uses a tls.Config
without an explicit minimum version, which triggers the static analysis warning.
Update the tls.Config passed to http.Transport so it sets a MinVersion floor
alongside NextProtos, keeping the existing client behavior unchanged while
hardening the TLS handshake.
- Around line 43-77: The CONNECT tunnel logic in connectDialer.DialStream and
bufConn duplicates the existing httpConnect/bufferedConn implementation from
bypass.go. Replace the ad hoc handshake and buffered-read wrapper with the
shared helper pattern if it can be reused cleanly, or otherwise keep the
diagnostic copy intentionally isolated and minimal so it does not drift from the
main implementation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13b20c9b-2f56-40c0-8aa9-1e11b70f842c
📒 Files selected for processing (4)
cmd/geosite-throttle-test/main.govpn/boxoptions.govpn/mirror.govpn/mirror_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
vpn/boxoptions.go (1)
448-456: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReserve the
mirrortag before injecting the outbound.
mergeAndCollectTags(Line 448) merges user-supplied outbounds/endpoints intoopts.OutboundsbeforemirrorOutbound()is appended (Lines 454-456). If a user config already defines an outbound or endpoint tagged"mirror", sing-box will reject the whole config at startup due to a duplicate outbound tag. This was already flagged in a prior review round on this line and remains unfixed.🛡️ Proposed fix: guard against tag collision
tags := mergeAndCollectTags(&opts, &bOptions.Options) // Route remote rule-set fetches through the proxyless "mirror" outbound so a // cold-start fetch survives DPI throttling of the CDNs they're served from. // Detour-only — deliberately not added to the selectable `tags`. Injected // only when a rule-set was actually repointed (engineering#3657). if repointRuleSetsToMirror(&opts) { + if slices.Contains(tags, mirrorOutboundTag) { + return O.Options{}, fmt.Errorf("config uses reserved outbound/endpoint tag %q", mirrorOutboundTag) + } opts.Outbounds = append(opts.Outbounds, mirrorOutbound()) }🤖 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 `@vpn/boxoptions.go` around lines 448 - 456, The `mergeAndCollectTags` plus `mirrorOutbound` flow can still collide with a user-defined `"mirror"` tag and make sing-box fail at startup. Reserve or guard the `"mirror"` tag before appending the outbound in this block so `opts.Outbounds` never contains a duplicate tag, and keep the fix centered around `mergeAndCollectTags`, `repointRuleSetsToMirror`, and `mirrorOutbound`.
🧹 Nitpick comments (1)
cmd/geosite-throttle-test/main.go (1)
83-92: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMissing
MinVersionon TLS client config.Static analysis flags the
tls.Configused for fetches as missing an explicit minimum TLS version. Low risk here since this is a throwaway diagnostic tool and Go's client default is already TLS 1.2, but worth setting explicitly for hygiene.🔧 Proposed fix
- TLSClientConfig: &tls.Config{NextProtos: []string{"http/1.1"}}, + TLSClientConfig: &tls.Config{NextProtos: []string{"http/1.1"}, MinVersion: tls.VersionTLS12},🤖 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 `@cmd/geosite-throttle-test/main.go` around lines 83 - 92, The TLS client setup in main uses a tls.Config with only NextProtos, so add an explicit MinVersion there to make the intended TLS floor clear. Update the http.Transport construction where tr is built so the tls.Config includes a minimum version (for example TLS 1.2) alongside the existing TLS settings, keeping the change localized to the client used by the geosite throttle test.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@vpn/boxoptions.go`:
- Around line 448-456: The `mergeAndCollectTags` plus `mirrorOutbound` flow can
still collide with a user-defined `"mirror"` tag and make sing-box fail at
startup. Reserve or guard the `"mirror"` tag before appending the outbound in
this block so `opts.Outbounds` never contains a duplicate tag, and keep the fix
centered around `mergeAndCollectTags`, `repointRuleSetsToMirror`, and
`mirrorOutbound`.
---
Nitpick comments:
In `@cmd/geosite-throttle-test/main.go`:
- Around line 83-92: The TLS client setup in main uses a tls.Config with only
NextProtos, so add an explicit MinVersion there to make the intended TLS floor
clear. Update the http.Transport construction where tr is built so the
tls.Config includes a minimum version (for example TLS 1.2) alongside the
existing TLS settings, keeping the change localized to the client used by the
geosite throttle test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9e35ced-eb04-4253-8ab2-16b0e3ec34eb
📒 Files selected for processing (5)
cmd/geosite-throttle-test/main.govpn/boxoptions.govpn/boxoptions_test.govpn/mirror.govpn/mirror_test.go
…rness - Remove engineering#3657 references from code comments (AGENTS.md: keep rationale in git history, not in-code). - Guard the mirror-outbound append: skip when the merged config already defines the "mirror" tag, so it can't produce a duplicate outbound tag. - Replace the throttle-test harness with its reviewed-final version (TLS- wrapped proxy hop so creds aren't sent in cleartext, checked CONNECT write, bounded read deadline, non-2xx treated as an error, no external script ref). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/geosite-throttle-test/main.go (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Go doc comments for the interface methods.
DialStreamand the intentionally no-opCloseReadshould document their contracts where declared.Proposed doc comments
+// DialStream establishes a TLS-wrapped CONNECT tunnel through the Oxylabs gateway. func (d connectDialer) DialStream(ctx context.Context, addr string) (transport.StreamConn, error) {+// CloseRead is a no-op because tls.Conn does not expose read-side half-close. func (tlsStreamConn) CloseRead() error { return nil }As per coding guidelines,
**/*.go: Use Go doc comments (// Foo ...) for exported identifiers and any unexported ones with non-obvious contracts.Also applies to: 90-90
🤖 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 `@cmd/geosite-throttle-test/main.go` at line 49, The `connectDialer` methods need Go doc comments where they are declared: add a comment on `DialStream` describing that it returns a `transport.StreamConn` for the given address and context, and document `CloseRead` as an intentional no-op so its contract is clear. Keep the comments in the `connectDialer` method block and use standard `// Name ...` doc style for these non-obvious method behaviors.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cmd/geosite-throttle-test/main.go`:
- Line 49: The `connectDialer` methods need Go doc comments where they are
declared: add a comment on `DialStream` describing that it returns a
`transport.StreamConn` for the given address and context, and document
`CloseRead` as an intentional no-op so its contract is clear. Keep the comments
in the `connectDialer` method block and use standard `// Name ...` doc style for
these non-obvious method behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cebe6ef5-51f5-4455-8f0f-49b4b1c102f6
📒 Files selected for processing (3)
cmd/geosite-throttle-test/main.govpn/boxoptions.govpn/mirror.go
🚧 Files skipped from review as they are similar to previous changes (2)
- vpn/mirror.go
- vpn/boxoptions.go
…repoint - tagInUse checks both outbounds and endpoints before injecting the mirror outbound, since sing-box tags must be unique across both. - expectMirrorDetour now calls repointRuleSetsToMirror instead of duplicating the rewrite predicate, so the test can't drift from production logic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/geosite-throttle-test/main.go`:
- Around line 16-17: The command comment is misleading about the expected proxy
username format, since sessUser builds the auth username from a bare customer ID
by prepending customer-. Update the documentation near main and sessUser to
explicitly state that OXY_USER must be the customer ID only, not a full
username, and that sessUser will add the customer- prefix automatically.
- Around line 86-90: The tlsStreamConn.CloseRead method currently returns nil
without changing the connection state, which incorrectly reports a successful
read-half close for *tls.Conn. Update tlsStreamConn so that CloseRead falls back
to closing the full connection when half-close is not supported, and keep the
adaptation logic consistent with transport.StreamConn expectations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01da301d-02e2-4880-b528-05690bc93168
📒 Files selected for processing (5)
cmd/geosite-throttle-test/main.govpn/boxoptions.govpn/boxoptions_test.govpn/mirror.govpn/mirror_test.go
Previously repointRuleSetsToMirror ran unconditionally and only the outbound injection was guarded, so a config already using the "mirror" tag (e.g. on an endpoint) would leave rule-sets pointing at a foreign tag. Check tagInUse before repointing (short-circuit), so rule-sets are only rewritten when our mirror outbound will back the tag. Adds TestTagInUse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
vpn/boxoptions.go:561
- mergeAndCollectTags() filters reserved/non-selectable tags only for src.Outbounds, but not for src.Endpoints. Since the returned tag list is used to populate the auto/manual selector groups, an endpoint tagged "direct"/"block"/"auto"/"manual" (or tagged with a server-declared non-selectable tag) would still become user-selectable, which contradicts the function comment and the PR intent of excluding these tags from the selectable set.
for _, ep := range src.Endpoints {
tags = append(tags, ep.Tag)
}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/geosite-throttle-test/main.go (1)
54-55: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet an explicit TLS
MinVersion.Both
tls.Configinstances (proxy handshake at Line 55, and the target-fetch transport at Line 120) omitMinVersion, defaulting to TLS 1.2. Since this dials a real external endpoint over the public internet, pin a modern floor to avoid silent downgrade.🔧 Proposed fix
- conn := tls.Client(raw, &tls.Config{ServerName: oxyHost}) + conn := tls.Client(raw, &tls.Config{ServerName: oxyHost, MinVersion: tls.VersionTLS12})- TLSClientConfig: &tls.Config{NextProtos: []string{"http/1.1"}}, + TLSClientConfig: &tls.Config{NextProtos: []string{"http/1.1"}, MinVersion: tls.VersionTLS12},Also applies to: 119-121
🤖 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 `@cmd/geosite-throttle-test/main.go` around lines 54 - 55, Set an explicit TLS minimum version for all outbound TLS connections in this test utility, since both the proxy handshake in tls.Client and the target-fetch transport in http.Transport currently rely on the default. Update the tls.Config used in the connection setup around tls.Client and the tls.Config inside the transport setup in the target-fetch path to include a modern MinVersion (for example TLS 1.2 or higher) so both the initial proxy handshake and the external endpoint fetch are pinned to a secure floor.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@cmd/geosite-throttle-test/main.go`:
- Around line 54-55: Set an explicit TLS minimum version for all outbound TLS
connections in this test utility, since both the proxy handshake in tls.Client
and the target-fetch transport in http.Transport currently rely on the default.
Update the tls.Config used in the connection setup around tls.Client and the
tls.Config inside the transport setup in the target-fetch path to include a
modern MinVersion (for example TLS 1.2 or higher) so both the initial proxy
handshake and the external endpoint fetch are pinned to a secure floor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ecbb6b3-fcaf-4ea2-968d-7d7ae8d43b70
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
backend/radiance.gocmd/geosite-throttle-test/main.gogo.modvpn/boxoptions.govpn/nonselectable_test.go
…bounds doc Spell out that excluded tags are kept out of BOTH the auto (URLTest) group (auto-selection won't route traffic through them) and the manual selector, matching the getlantern/common#27 field doc. Comment-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
vpn/boxoptions.go:564
- mergeAndCollectTags filters reserved/non-selectable tags only for outbounds; endpoint tags are appended unconditionally. Because endpoints share the same tag namespace (and these tags feed the auto/manual selectors), a server-provided endpoint tagged "auto"/"manual"/"direct"/"block" (or a tag listed in NonSelectableOutbounds) would still become selectable, contradicting the function’s doc comment and potentially reintroducing unsafe selection (e.g., selectable "direct"). Filter endpoints the same way as outbounds.
for _, ep := range src.Endpoints {
tags = append(tags, ep.Tag)
}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/geosite-throttle-test/main.go`:
- Line 50: Add a Go doc comment for the exported connectDialer.DialStream
method. The method currently lacks its own comment even though it is exported;
add a concise // DialStream ... comment immediately above the method declaration
that describes its tunnel dialing behavior and keep it consistent with the
existing connectDialer type documentation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 003e02bf-d317-49a7-bfcd-a02b409ac8bc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
backend/radiance.gocmd/geosite-throttle-test/main.gogo.modvpn/boxoptions.govpn/nonselectable_test.go
…ctable set mergeAndCollectTags filtered outbound tags but returned endpoint tags unfiltered, so an infra-tagged endpoint (reserved or server-declared non-selectable) would have been offered as a selectable proxy. Apply the same filter to endpoints via a shared skip closure; cover it in the test. Also add a doc comment on the throttle-test DialStream method. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
common#27 merged; re-point from the branch pseudo-version to merged main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/geosite-throttle-test/main.go (1)
112-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Go doc comment for
fetch.
fetchhas a non-obvious measurement contract: it forces HTTP/1.1, disables redirects/compression, drains the body, and returns measured bytes plus duration. Put that contract immediately above the declaration.📝 Suggested doc comment
+// fetch downloads url through dialer, measuring the original HTTP/1.1 response +// body without redirects or transparent gzip decoding. func fetch(dialer transport.StreamDialer, url string) (int64, time.Duration, error) {As per coding guidelines,
**/*.go: Use Go doc comments (// Foo ...) for exported identifiers and any unexported ones with non-obvious contracts.🤖 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 `@cmd/geosite-throttle-test/main.go` around lines 112 - 144, Add a Go doc comment immediately above fetch that describes its non-obvious measurement contract: it uses HTTP/1.1 only, disables redirects and compression, drains the response body, and returns the byte count plus elapsed duration. Keep the comment tied to the fetch function so readers understand the behavior without inspecting the implementation, and make sure it follows the Go doc comment style used elsewhere in the package.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cmd/geosite-throttle-test/main.go`:
- Around line 112-144: Add a Go doc comment immediately above fetch that
describes its non-obvious measurement contract: it uses HTTP/1.1 only, disables
redirects and compression, drains the response body, and returns the byte count
plus elapsed duration. Keep the comment tied to the fetch function so readers
understand the behavior without inspecting the implementation, and make sure it
follows the Go doc comment style used elsewhere in the package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e3f32cf7-2b6c-4f7c-8ca7-19a3dbbb7047
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
backend/radiance.gocmd/geosite-throttle-test/main.gogo.modvpn/boxoptions.govpn/nonselectable_test.go
…reserved set Follow-up to filtering endpoint tags: the BoxOptions field, mergeAndCollectTags doc, and test header said "outbounds" though the filter applies to endpoints too; say "tags". Also list the full reserved set (auto/manual/direct/block) in the skip comment. Comment-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
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 (1)
vpn/boxoptions.go (1)
527-569: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExclude reserved tags before merging src outbounds/endpoints
mergeAndCollectTagsstill appends everysrc.Outbounds/src.Endpointsentry intodst, whilebuildOptionslater injectsautoandmanualoutbounds. sing-box requires outbound tags to be unique, so a src entry usingauto,manual,direct, orblockcan make the final config invalid at startup. Filter reserved tags out of the merge, not just out of the selectable tag list.🤖 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 `@vpn/boxoptions.go` around lines 527 - 569, mergeAndCollectTags still copies every src.Outbounds and src.Endpoints entry into dst, so reserved tags like auto, manual, direct, and block can collide with the later buildOptions injection and break sing-box startup. Update mergeAndCollectTags to filter these reserved tags out before appending to dst, while keeping the existing selectable-tag collection logic for non-reserved entries. Use the existing reservedTags helper and the mergeAndCollectTags/buildOptions flow to keep both the merged config and the returned tag list consistent.
🤖 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.
Outside diff comments:
In `@vpn/boxoptions.go`:
- Around line 527-569: mergeAndCollectTags still copies every src.Outbounds and
src.Endpoints entry into dst, so reserved tags like auto, manual, direct, and
block can collide with the later buildOptions injection and break sing-box
startup. Update mergeAndCollectTags to filter these reserved tags out before
appending to dst, while keeping the existing selectable-tag collection logic for
non-reserved entries. Use the existing reservedTags helper and the
mergeAndCollectTags/buildOptions flow to keep both the merged config and the
returned tag list consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39bbdf46-1a5c-4f1b-baf6-4973bc8b0c1d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
backend/radiance.gocmd/geosite-throttle-test/main.gogo.modvpn/boxoptions.govpn/nonselectable_test.go
Contributor
Author
|
Pulling this in. Actually a small change that allows us to specify the proxyless outbound for rule-sets. |
This was referenced Jul 6, 2026
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.
What
Let the server introduce optional/infrastructure outbounds (like a proxyless rule-set-fetch detour) without a client release. The client merges any outbound the server sends into its box config so references resolve, but excludes the ones the server marks non-selectable from the auto/manual proxy groups.
Why
The companion server change (getlantern/lantern-cloud#2936) sends a
proxylessoutbound — the Outline smart dialer (DoH + TLS-record fragmentation) — and points remote rule-set fetches at it viadownload_detour, so a cold-start fetch survives SNI-based DPI throttling (the CN cold-start failure, FD-179514).But a server-sent outbound would otherwise become a user-selectable proxy — and worse,
auto(urltest) could pick the low-latency direct+frag dialer and route traffic directly, exposing the user's IP. So the client must treat such outbounds as infrastructure. Rather than hardcode theproxylesstag, the client honors a generic server-declared list, so future optional outbounds need no client change.Change
vpn/boxoptions.go—mergeAndCollectTagsnow excludes the base reserved tags (auto/manual/direct/block) and any tag inBoxOptions.NonSelectableOutboundsfrom the returned selectable set (they're still merged into the config).backend/radiance.go— populatesBoxOptions.NonSelectableOutboundsfromConfigResponse.NonSelectableOutbounds.vpn/proxyless.go— the client no longer knows any specific infra tag.cmd/geosite-throttle-test(CN-residential validation harness).Dependencies / rollout
NonSelectableOutboundstoConfigResponse) is merged;go.modpoints at the mergedcommonmain.config.client_versionto the release carrying this change — older clients (without this filter) would surface the proxyless outbound as selectable. Context: getlantern/engineering#3657.Testing
go test ./vpn/—TestMergeAndCollectTags_ExcludesNonSelectable(server-declared + reserved tags excluded, but still merged into the config), plus the existing build tests.go build ./vpn/... ./backend/... ./cmd/geosite-throttle-test/,go vet, gofmt clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01WGpKfgkbfdJwwsTaouMDoR
Summary by CodeRabbit