[codex] Restore Windows --dev source selection#364
Merged
Merged
Conversation
Keep Windows --dev usable by resolving the selected device to a source IP and clearing SourceDevice before the tracer reaches Windows send paths. Update docs and project notes to describe that Windows routing may still choose the actual egress interface.
Contributor
Walkthrough本次变更移除了对 Windows 上 TCP + Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 诗句
🚥 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Restores Windows --dev behavior so TCP traces no longer fail early and instead derive a source IP from the selected device, aligning Windows behavior across ICMP/TCP/UDP and updating documentation to reflect that Windows --dev is source-address selection (not strict egress-interface binding).
Changes:
- Remove the Windows+TCP
SourceDevicerejection inNormalizeExplicitSourceConfig, allowing device→source-IP resolution. - Update/extend tests to validate Windows TCP resolves
--devtoSrcAddrand clearsSourceDevice. - Update CLI help text and READMEs to clarify Windows
--devand standalone--mtusemantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
trace/source_config.go |
Removes Windows TCP --dev rejection; normalizes Windows --dev into SrcAddr while clearing SourceDevice. |
trace/source_config_test.go |
Updates test to assert Windows TCP resolves device to a source IP instead of erroring. |
cmd/cmd.go |
Adjusts --dev help text and related comments to match restored Windows behavior. |
README.md |
Documents Windows --dev as source-IP selection for ICMP/TCP/UDP and clarifies --mtu behavior. |
README_zh_CN.md |
Same documentation updates as README.md (Chinese). |
AGENTS.md |
Records the Windows compatibility rule: resolve --dev to SrcAddr and clear SourceDevice (v1.6.2-compatible behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+124
| func NormalizeExplicitSourceConfig(method Method, config Config) (Config, error) { | ||
| _ = method | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
trace/source_config_test.go (1)
303-330: 建议把 Windows 协议覆盖做成表驱动。当前新增用例覆盖了这次 TCP 回归；可以顺手把
ICMPTrace/TCPTrace/UDPTrace都放进同一个表，锁住 PR 描述里的三种协议行为，避免后续重新引入 method-specific 分支时漏掉 UDP。🧪 可选测试重构
-func TestNormalizeExplicitSourceConfigWindowsTCPResolvesDeviceToSourceAddress(t *testing.T) { +func TestNormalizeExplicitSourceConfigWindowsResolvesDeviceToSourceAddress(t *testing.T) { restore := stubSourceDeviceResolver(t, func(device string) (*net.Interface, error) { if device != "Ethernet0" { t.Fatalf("ResolveSourceDevice device = %q, want Ethernet0", device) } return &net.Interface{Name: device}, nil }, func(_ *net.Interface) ([]net.Addr, error) { return []net.Addr{&net.IPNet{IP: net.ParseIP("192.0.2.44"), Mask: net.CIDRMask(24, 32)}}, nil }) defer restore() - cfg := Config{ - OSType: osTypeWindows, - DstIP: net.ParseIP("1.1.1.1"), - SourceDevice: "Ethernet0", - } - - got, err := NormalizeExplicitSourceConfig(TCPTrace, cfg) - if err != nil { - t.Fatalf("NormalizeExplicitSourceConfig() error = %v", err) - } - if got.SrcAddr != "192.0.2.44" { - t.Fatalf("SrcAddr = %q, want 192.0.2.44", got.SrcAddr) - } - if got.SourceDevice != "" { - t.Fatalf("SourceDevice = %q, want empty", got.SourceDevice) + for _, method := range []Method{ICMPTrace, TCPTrace, UDPTrace} { + t.Run(string(method), func(t *testing.T) { + cfg := Config{ + OSType: osTypeWindows, + DstIP: net.ParseIP("1.1.1.1"), + SourceDevice: "Ethernet0", + } + + got, err := NormalizeExplicitSourceConfig(method, cfg) + if err != nil { + t.Fatalf("NormalizeExplicitSourceConfig() error = %v", err) + } + if got.SrcAddr != "192.0.2.44" { + t.Fatalf("SrcAddr = %q, want 192.0.2.44", got.SrcAddr) + } + if got.SourceDevice != "" { + t.Fatalf("SourceDevice = %q, want empty", got.SourceDevice) + } + }) } }As per coding guidelines,
**/cmd/cmd.go: For Windows ICMP/TCP/UDP, use source IP to influence routing selection, not actual per-device binding; document in README that device specification may be inaccurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trace/source_config_test.go` around lines 303 - 330, The test TestNormalizeExplicitSourceConfigWindowsTCPResolvesDeviceToSourceAddress currently only covers TCP; change it to a table-driven test iterating over trace types (use TCPTrace, ICMPTrace, UDPTrace) and assert the same expectations for Windows: when SourceDevice resolves to an IP, NormalizeExplicitSourceConfig(traceType, cfg) returns SrcAddr == resolved IP and clears SourceDevice; update the test function name (e.g. TestNormalizeExplicitSourceConfigWindowsProtocolResolvesDeviceToSourceAddress) and loop over the trace types, calling NormalizeExplicitSourceConfig for each and running the same assertions, and add a short README note where Windows behavior is documented (per-cmd behavior in cmd.go) that device spec affects routing via source IP rather than guaranteed device binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@trace/source_config_test.go`:
- Around line 303-330: The test
TestNormalizeExplicitSourceConfigWindowsTCPResolvesDeviceToSourceAddress
currently only covers TCP; change it to a table-driven test iterating over trace
types (use TCPTrace, ICMPTrace, UDPTrace) and assert the same expectations for
Windows: when SourceDevice resolves to an IP,
NormalizeExplicitSourceConfig(traceType, cfg) returns SrcAddr == resolved IP and
clears SourceDevice; update the test function name (e.g.
TestNormalizeExplicitSourceConfigWindowsProtocolResolvesDeviceToSourceAddress)
and loop over the trace types, calling NormalizeExplicitSourceConfig for each
and running the same assertions, and add a short README note where Windows
behavior is documented (per-cmd behavior in cmd.go) that device spec affects
routing via source IP rather than guaranteed device binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecef484f-d74c-43f5-982d-ca57aff80c09
📒 Files selected for processing (6)
AGENTS.mdREADME.mdREADME_zh_CN.mdcmd/cmd.gotrace/source_config.gotrace/source_config_test.go
Remove the in-body unused method assignment and extend the Windows source-device regression test across ICMP, TCP, and UDP so future method-specific gating is caught.
Member
Author
|
@coderabbitai run pre-merge checks |
Contributor
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
Member
Author
|
@codex run pre-merge checks |
|
To use Codex here, create an environment for this repo. |
Member
Author
|
@codex run pre-merge checks |
Summary
Testing
If you want, I can also run any repo-specific CI scripts/workflows that mirror GitHub Actions more closely (if present), but the core Go checks are all passing locally. |
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.
Summary
--devhandling so ICMP/TCP/UDP resolve the selected device to a source IP instead of rejecting TCP--devas source-address selection, not real egress-interface binding--mtufollows the same source-address behavior and only uses the device name for local MTU lookupWhy
A previous change made Windows
TCP + --devfail before reaching the tracer. That regressed the v1.6.2-compatible behavior where Windows derives a source IP from the selected device and lets Windows routing choose the actual egress path.Validation
go test ./trace ./cmdgo test ./...Summary by CodeRabbit
Bug Fixes
Documentation
Tests