Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough新增 Nali 模式：通过 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant CLI as CLI 解析 / 分发<br/>(cmd/cmd.go)
participant Nali as Nali 控制器<br/>(cmd/nali_mode.go)
participant Annot as 标注引擎<br/>(internal/nali)
participant Geo as 地理信息源 / Leo/ws / IPGeo
User->>CLI: 运行 nexttrace --nali [选项] [目标]
CLI->>CLI: 识别 --nali 标志
alt enableNali 为 true
CLI->>Nali: 调用 runNaliMode(...)
Nali->>Nali: validateNaliModeOptions(...)
Nali->>Nali: 初始化 GeoDNS / Leo websocket
alt 有目标参数
Nali->>Annot: Run(ctx, cfg, stdin, stdout, target)
else 无目标参数
Nali->>Annot: Run(ctx, cfg, stdin, stdout, "")
end
loop 每行或每个目标
Annot->>Annot: FindIPSpans / 过滤 family
loop 每个检测到的 IP
Annot->>Geo: 查询 IP 地理/提供商（带超时）
Geo-->>Annot: 返回 IPGeoData
Annot->>Annot: FormatGeo -> 在文本中插入 " [label]"
end
Annot-->>User: 输出已标注的行
end
else
CLI->>CLI: 继续默认追踪流程
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 分钟 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new full-build-only --nali mode to NextTrace to annotate IPv4/IPv6 literals in text (single target argument or stdin stream), reusing existing GeoIP providers and related runtime setup, and documents the feature in both English and Chinese READMEs.
Changes:
- Introduce
internal/naliannotator (IP span detection, Geo formatting, streaming runner) plus unit tests. - Add CLI integration for
--naliin the full flavor only, including option conflict validation and cmd-level tests. - Update README feature matrices and usage docs to include
--nali.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/nali/nali.go | Core IP-literal scanning + annotation logic and streaming runner (Run). |
| internal/nali/nali_test.go | Unit tests for span detection, geo formatting, caching, and stdin/target behavior. |
| cmd/nali_mode.go | Registers --nali, validates conflicts, and wires mode execution to internal/nali. |
| cmd/nali_mode_test.go | Tests for flag availability by flavor, parser behavior, conflict validation, and mode execution. |
| cmd/flavor_full.go | Enables --nali in the full build. |
| cmd/flavor_tiny.go | Explicitly disables --nali in tiny build. |
| cmd/flavor_ntr.go | Explicitly disables --nali in ntr build. |
| cmd/cmd.go | Hooks --nali into main Execute flow and routes to runNaliMode. |
| cmd/cmd_test.go | Adjusts help/output-flag tests for the ntr flavor behavior. |
| README.md | Documents --nali feature and updates the feature matrix + usage snippet. |
| README_zh_CN.md | Same documentation updates as README.md (Chinese). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type Annotator struct { | ||
| cfg Config | ||
| cache map[string]string | ||
| } | ||
|
|
There was a problem hiding this comment.
Annotator caches every looked-up IP label for the lifetime of the process, but the cache map is unbounded. When annotating large streams/logs with many unique IPs this can grow without limit and increase memory usage. Consider bounding the cache (size cap + eviction), or at least providing a way to disable caching / periodically clear it in streaming mode.
| if strings.HasPrefix(strings.ToUpper(asn), "AS") { | ||
| return asn |
There was a problem hiding this comment.
normalizeASN returns the original string unchanged when it already starts with "AS" (case-insensitive). This can lead to inconsistent formatting (e.g., "as15169" stays lowercase, and inputs like "AS 15169" keep the space). There is already a normalizeASN implementation in internal/speedtest/runner that strips any existing prefix and re-adds a canonical "AS"; mirroring that logic here would keep output consistent across the project.
| if strings.HasPrefix(strings.ToUpper(asn), "AS") { | |
| return asn | |
| asn = strings.TrimSpace(asn) | |
| if len(asn) >= 2 && strings.EqualFold(asn[:2], "AS") { | |
| asn = strings.TrimSpace(asn[2:]) |
| func isIPStart(b byte) bool { | ||
| return isHexByte(b) || b == ':' | ||
| } |
There was a problem hiding this comment.
FindIPSpans currently treats any hex letter (a-f/A-F) as a potential IP start (isIPStart uses isHexByte), which means many ordinary words will trigger netip.ParseAddr attempts. In text-stream mode this can add significant overhead on non-IP-heavy input. Consider adding a cheap pre-check before ParseAddr (e.g., require the candidate to contain '.' or ':'; and for hex-letter starts require at least one ':'), or tighten isIPStart to reduce false positives while still matching IPv6 like "fe80::1".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cmd/nali_mode_test.go (1)
59-63: 按仓库规则避免新增匿名 struct 表项。这里用匿名
struct定义测试用例 shape；如果要严格遵循当前仓库规则，请改为项目约定的interface{}/共享接口形式，或为测试文件明确豁免该规则。As per coding guidelines,
**/*.{ts,tsx,go}: Useinterface{}for defining object shapes instead of struct types in TypeScript/Go code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/nali_mode_test.go` around lines 59 - 63, The test slice currently uses an anonymous struct (tests := []struct{ name string; opts naliModeOptions; want string }) which violates the repo rule; replace it with a slice of interface{} (tests := []interface{}{}) where each case is a map[string]interface{} with keys "name", "opts", and "want", and update the test loop to type-assert each entry (e.g., m := tc.(map[string]interface{}); name := m["name"].(string); opts := m["opts"].(naliModeOptions); want := m["want"].(string)) so you avoid anonymous struct literals while keeping the same values and using the existing naliModeOptions type and the tests identifier.internal/nali/nali.go (1)
41-54: 提醒：Annotator.cache未加锁，不支持并发调用AnnotateLine。
Annotator是一个导出类型，AnnotateLine/lookupLabel会在同一个map[string]string上进行读/写。当前通过Run的单协程循环调用没有问题，但既然New/Annotator/AnnotateLine都是包外 API，一旦未来有并发使用场景（比如批量注解多条流）就会触发 data race。建议二选一：
- 在 doc comment 中显式声明"not safe for concurrent use"；
- 或给
cache加一把sync.RWMutex（查询走读锁，miss 后升级写锁），让类型本身并发安全。♻️ 建议的并发安全修改
type Annotator struct { cfg Config + mu sync.RWMutex cache map[string]string } @@ func (a *Annotator) lookupLabel(ctx context.Context, ip string) string { if ip == "" { return "" } - if label, ok := a.cache[ip]; ok { + a.mu.RLock() + label, ok := a.cache[ip] + a.mu.RUnlock() + if ok { return label } - label := "" + label = "" if geo, ok := ipgeo.Filter(ip); ok { label = FormatGeo(geo, a.cfg.Lang) } else if a.cfg.Source != nil && ctx.Err() == nil { if geo, err := a.cfg.Source(ip, a.cfg.Timeout, a.cfg.Lang, false); err == nil { label = FormatGeo(geo, a.cfg.Lang) } } - a.cache[ip] = label + a.mu.Lock() + a.cache[ip] = label + a.mu.Unlock() return label }需同步
import "sync"。Also applies to: 139-157
cmd/nali_mode.go (2)
132-187: 建议：buildNaliModeOptions参数过多，考虑用选项聚合结构体传参。当前
buildNaliModeOptions以位置参数形式接收近 20 个值（包含混合类型 bool/string/int/effectiveMTRModes），调用方很容易把顺序写错，编译器也无法帮助发现。可以把相关的源字段（outputPath、from、listen、file、srcAddr、srcPort、srcDev、各布尔 flag 与mtrModes）打包到一个已有的/新的"CLI 原始参数"结构里，或者让validateNaliModeOptions直接在Execute()里基于已经解析好的字段做检查，避免在此处重复展开。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/nali_mode.go` around lines 132 - 187, The buildNaliModeOptions function currently accepts ~20 positional params which is error-prone; refactor it to take a single options struct (e.g., CLIArgs or RawCLIOptions) that aggregates fields like outputPath, from, listen, file, srcAddr, srcPort, srcDev, the boolean flags, and mtrModes; update the call sites to construct and pass that struct (or pass the parser plus the struct) and move any validation logic into validateNaliModeOptions or into Execute() to operate on the aggregated struct, keeping buildNaliModeOptions signatures and internal mapping (icmpMode/port/queries/.../sourceDevice) unchanged except reading from the new struct.
27-78: 编码规范提醒：Go 文件应使用interface{}而非struct定义对象形状。新增的
naliModeOptions/naliRunOptions采用struct定义。若严格依据项目规范，这属于违规；不过这条规范对 Go 而言并不符合惯用风格，是否需要实际调整请与维护者确认后再定。As per coding guidelines: "Use
interface{}for defining object shapes instead of struct types in TypeScript/Go code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/nali_mode.go` around lines 27 - 78, The new concrete structs naliModeOptions and naliRunOptions violate the stated project guideline to use interface{}-shaped types; confirm with the maintainers and, if the guideline must be applied, replace those struct type definitions (naliModeOptions, naliRunOptions) with interface{}-based types (e.g., type naliModeOptions interface{} and type naliRunOptions interface{}) and update all call sites/constructors to supply map[string]interface{} or appropriate concrete maps/values that satisfy the interface{} shape so consumers expect a generic object rather than concrete struct fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/nali_mode_test.go`:
- Around line 3-8: Tests use testing.T.Context() which requires Go 1.24; replace
its usage with context.Background() to retain Go 1.22+ compatibility: add
"context" to the import block and change any call sites that pass t.Context()
(in the test functions in cmd/nali_mode_test.go) to use context.Background()
instead.
In `@internal/nali/nali.go`:
- Around line 222-253: Add a regression test that asserts how
parseCandidate/splitIPv4Port/parseAddr handle the IPv4-mapped-IPv6-with-port
case "::ffff:1.2.3.4:53" (e.g. in the same table as
TestAnnotateLineBracketPortCIDRZoneAndPunctuation); specifically encode the
expected behavior (currently the token is not parsed as host:port and is
dropped) so future refactors have a clear contract — add a test case comparing
input "::ffff:1.2.3.4:53" to the expected output (dropped/unchanged as per
existing behavior) and reference parseCandidate, splitIPv4Port, and parseAddr in
the test comment.
---
Nitpick comments:
In `@cmd/nali_mode_test.go`:
- Around line 59-63: The test slice currently uses an anonymous struct (tests :=
[]struct{ name string; opts naliModeOptions; want string }) which violates the
repo rule; replace it with a slice of interface{} (tests := []interface{}{})
where each case is a map[string]interface{} with keys "name", "opts", and
"want", and update the test loop to type-assert each entry (e.g., m :=
tc.(map[string]interface{}); name := m["name"].(string); opts :=
m["opts"].(naliModeOptions); want := m["want"].(string)) so you avoid anonymous
struct literals while keeping the same values and using the existing
naliModeOptions type and the tests identifier.
In `@cmd/nali_mode.go`:
- Around line 132-187: The buildNaliModeOptions function currently accepts ~20
positional params which is error-prone; refactor it to take a single options
struct (e.g., CLIArgs or RawCLIOptions) that aggregates fields like outputPath,
from, listen, file, srcAddr, srcPort, srcDev, the boolean flags, and mtrModes;
update the call sites to construct and pass that struct (or pass the parser plus
the struct) and move any validation logic into validateNaliModeOptions or into
Execute() to operate on the aggregated struct, keeping buildNaliModeOptions
signatures and internal mapping (icmpMode/port/queries/.../sourceDevice)
unchanged except reading from the new struct.
- Around line 27-78: The new concrete structs naliModeOptions and naliRunOptions
violate the stated project guideline to use interface{}-shaped types; confirm
with the maintainers and, if the guideline must be applied, replace those struct
type definitions (naliModeOptions, naliRunOptions) with interface{}-based types
(e.g., type naliModeOptions interface{} and type naliRunOptions interface{}) and
update all call sites/constructors to supply map[string]interface{} or
appropriate concrete maps/values that satisfy the interface{} shape so consumers
expect a generic object rather than concrete struct fields.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12a8acd4-fdcc-45a7-a03b-f45866dc6a85
📒 Files selected for processing (11)
README.mdREADME_zh_CN.mdcmd/cmd.gocmd/cmd_test.gocmd/flavor_full.gocmd/flavor_ntr.gocmd/flavor_tiny.gocmd/nali_mode.gocmd/nali_mode_test.gointernal/nali/nali.gointernal/nali/nali_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f85a28200
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| packetInterval: parsedFlag(parser, "send-time"), | ||
| packetSize: parsedFlag(parser, "psize"), | ||
| tos: parsedFlag(parser, "tos"), |
There was a problem hiding this comment.
--nali is documented to conflict with probe/traceroute tuning flags, but this option builder never records whether --ttl-time/-i was explicitly provided. In practice nexttrace --nali -i 500 ... is accepted and -i is silently ignored, which can hide misconfigured automation and produce unexpected behavior. Please track ttl-time as an explicit conflict so validateNaliModeOptions fails fast.
Useful? React with 👍 / 👎.
|
@codex review |
|
@coderabbitai review ，已针对review进行了修改 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(a.cacheOrder) >= maxCacheEntries { | ||
| oldest := a.cacheOrder[0] | ||
| delete(a.cache, oldest) | ||
| copy(a.cacheOrder, a.cacheOrder[1:]) | ||
| a.cacheOrder = a.cacheOrder[:len(a.cacheOrder)-1] | ||
| } |
There was a problem hiding this comment.
Cache eviction shifts the entire cacheOrder slice on every insert once maxCacheEntries is reached (copy(a.cacheOrder, a.cacheOrder[1:])). For large streams with mostly-unique IPs this becomes O(maxCacheEntries) work per new IP and can significantly slow --nali mode. Consider replacing cacheOrder with a ring buffer (maintaining an eviction index) or a small LRU/list implementation to make eviction O(1).
| ipv4Only bool | ||
| ipv6Only bool | ||
| tcp bool | ||
| udp bool | ||
| mtu bool | ||
| mtr bool | ||
| raw bool | ||
| table bool | ||
| classic bool | ||
| json bool | ||
| output bool | ||
| outputDefault bool | ||
| routePath bool | ||
| maptraceFlag bool | ||
| from bool | ||
| deploy bool | ||
| listen bool | ||
| fastTrace bool | ||
| file bool | ||
| disableMPLS bool | ||
| noRDNS bool | ||
| alwaysRDNS bool | ||
| init bool | ||
| icmpMode bool | ||
| port bool | ||
| queries bool | ||
| maxAttempts bool | ||
| parallelRequests bool | ||
| maxHops bool | ||
| beginHop bool | ||
| packetInterval bool | ||
| ttlInterval bool | ||
| packetSize bool | ||
| tos bool | ||
| source bool | ||
| sourcePort bool | ||
| sourceDevice bool |
There was a problem hiding this comment.
naliModeOptions.maptraceFlag is populated from disableMaptrace (the -M/--map flag that disables trace-map printing). The current field name reads like it represents enabling maptrace, which makes the option builder and conflict list harder to reason about. Renaming this field (and the corresponding input name) to something like disableMaptrace/disableMaptraceFlag would better reflect the actual semantics.
| ipv4Only bool | |
| ipv6Only bool | |
| tcp bool | |
| udp bool | |
| mtu bool | |
| mtr bool | |
| raw bool | |
| table bool | |
| classic bool | |
| json bool | |
| output bool | |
| outputDefault bool | |
| routePath bool | |
| maptraceFlag bool | |
| from bool | |
| deploy bool | |
| listen bool | |
| fastTrace bool | |
| file bool | |
| disableMPLS bool | |
| noRDNS bool | |
| alwaysRDNS bool | |
| init bool | |
| icmpMode bool | |
| port bool | |
| queries bool | |
| maxAttempts bool | |
| parallelRequests bool | |
| maxHops bool | |
| beginHop bool | |
| packetInterval bool | |
| ttlInterval bool | |
| packetSize bool | |
| tos bool | |
| source bool | |
| sourcePort bool | |
| sourceDevice bool | |
| ipv4Only bool | |
| ipv6Only bool | |
| tcp bool | |
| udp bool | |
| mtu bool | |
| mtr bool | |
| raw bool | |
| table bool | |
| classic bool | |
| json bool | |
| output bool | |
| outputDefault bool | |
| routePath bool | |
| disableMaptraceFlag bool | |
| from bool | |
| deploy bool | |
| listen bool | |
| fastTrace bool | |
| file bool | |
| disableMPLS bool | |
| noRDNS bool | |
| alwaysRDNS bool | |
| init bool | |
| icmpMode bool | |
| port bool | |
| queries bool | |
| maxAttempts bool | |
| parallelRequests bool | |
| maxHops bool | |
| beginHop bool | |
| packetInterval bool | |
| ttlInterval bool | |
| packetSize bool | |
| tos bool | |
| source bool | |
| sourcePort bool | |
| sourceDevice bool |
| {"--output-default", opts.outputDefault}, | ||
| {"--route-path", opts.routePath}, | ||
| {"--map", opts.maptraceFlag}, | ||
| {"--from", opts.from}, |
There was a problem hiding this comment.
validateNaliModeOptions currently treats --map (which in this CLI is the disable trace map output flag) as a hard conflict for --nali. Since --nali mode doesn't produce trace-map output at all, rejecting --map seems unnecessary and diverges from the documented list of --nali conflicts in the PR description. Consider allowing --map (ignore it in --nali mode) or update the documented behavior if this is intentionally disallowed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f92c4d4965
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return spans | ||
| } | ||
|
|
||
| func isIPStart(b byte) bool { |
There was a problem hiding this comment.
isIPStart treats any hex byte as a valid start, so FindIPSpans can begin in the middle of words and mis-parse common key:value text. For example, src:2001:db8::1 is scanned from the trailing c in src and interpreted as c:2001:db8::1, which produces incorrect annotations (often [INVALID]) or misses the actual IP entirely. This breaks --nali accuracy on typical log formats unless a left-boundary delimiter check is added before starting a candidate.
Useful? React with 👍 / 👎.
| packetSize: parsedFlag(input.parser, "psize"), | ||
| tos: parsedFlag(input.parser, "tos"), | ||
| source: strings.TrimSpace(input.srcAddr) != "", | ||
| sourcePort: input.srcPort != 0, |
There was a problem hiding this comment.
--source-port 0 as a nali conflict
Conflict detection for --source-port is based on input.srcPort != 0, so an explicitly provided --source-port 0 bypasses validation and --nali runs instead of rejecting an unsupported flag combination. Since validateNaliModeOptions declares --source-port incompatible with --nali, this should use parsed-state detection (like other probe flags) rather than value-based detection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cmd.go`:
- Around line 1184-1189: The current error handling after calling runNaliMode
treats context.Canceled as suppressed but still does os.Exit(1), causing
SIGINT/SIGTERM exits to appear as failures; change the branch so that if
errors.Is(err, context.Canceled) you return (or otherwise allow Execute to
return normally) instead of calling os.Exit(1) or printing the error, matching
how runTraceOnce/Execute handle cancellations and yielding exit code 0 for user
interrupts. Locate the block handling the error from runNaliMode and adjust it
to early-return on context.Canceled while preserving the existing behavior for
other errors.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 629dfb49-be70-4f9a-b673-c59308406927
📒 Files selected for processing (5)
cmd/cmd.gocmd/nali_mode.gocmd/nali_mode_test.gointernal/nali/nali.gointernal/nali/nali_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/nali/nali_test.go
- internal/nali/nali.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/nali_mode_test.go
- cmd/nali_mode.go
|
@coderabbitai help |
✅ Actions performedReview triggered.
|
|
@codex review |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai run pre-merge checks |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func validateNaliModeOptions(opts naliModeOptions) error { | ||
| if opts.json { | ||
| return fmt.Errorf("--nali 不支持 --json") | ||
| } | ||
| if opts.ipv4Only && opts.ipv6Only { | ||
| return fmt.Errorf("-4/--ipv4 不能与 -6/--ipv6 同时使用") | ||
| } | ||
| for _, conflict := range []struct { | ||
| name string | ||
| enabled bool | ||
| }{ | ||
| {"--mtu", opts.mtu}, | ||
| {"--mtr/-r/--report/-w/--wide", opts.mtr}, | ||
| {"--raw", opts.raw}, | ||
| {"--table", opts.table}, | ||
| {"--classic", opts.classic}, | ||
| {"--output", opts.output}, | ||
| {"--output-default", opts.outputDefault}, | ||
| {"--route-path", opts.routePath}, | ||
| {"--from", opts.from}, | ||
| {"--deploy", opts.deploy}, | ||
| {"--listen", opts.listen}, | ||
| {"--fast-trace", opts.fastTrace}, | ||
| {"--file", opts.file}, | ||
| {"--tcp", opts.tcp}, | ||
| {"--udp", opts.udp}, | ||
| {"--port", opts.port}, | ||
| {"--icmp-mode", opts.icmpMode}, | ||
| {"--queries", opts.queries}, | ||
| {"--max-attempts", opts.maxAttempts}, | ||
| {"--parallel-requests", opts.parallelRequests}, | ||
| {"--max-hops", opts.maxHops}, | ||
| {"--first", opts.beginHop}, | ||
| {"--send-time", opts.packetInterval}, | ||
| {"--ttl-time", opts.ttlInterval}, | ||
| {"--psize", opts.packetSize}, | ||
| {"--tos", opts.tos}, | ||
| {"--source", opts.source}, | ||
| {"--source-port", opts.sourcePort}, | ||
| {"--dev", opts.sourceDevice}, | ||
| {"--disable-mpls", opts.disableMPLS}, | ||
| {"--no-rdns", opts.noRDNS}, | ||
| {"--always-rdns", opts.alwaysRDNS}, | ||
| {"--init", opts.init}, | ||
| } { |
There was a problem hiding this comment.
validateNaliModeOptions doesn't reject some CLI flags that are still registered in the full build but are ignored in --nali mode (e.g. --map/-M, --show-ips, and --ipinfo). This means users can pass them with --nali and get silently ignored behavior, which conflicts with the documented “conflict with traceroute/MTR flags” behavior. Consider extending naliModeOptions/buildNaliModeOptions to detect these flags (via parsedFlag or passed pointers) and add them to the conflict list so they error out consistently.
| if geo, ok := ipgeo.Filter(ip); ok { | ||
| label = FormatGeo(geo, a.cfg.Lang) | ||
| } else if a.cfg.Source != nil && ctx.Err() == nil { | ||
| if geo, err := a.cfg.Source(ip, a.cfg.Timeout, a.cfg.Lang, false); err == nil { | ||
| label = FormatGeo(geo, a.cfg.Lang) | ||
| } | ||
| } | ||
| a.storeLabel(ip, label) |
There was a problem hiding this comment.
lookupLabel caches empty labels even when the GeoIP source returns an error. A transient lookup failure will therefore permanently suppress annotation for that IP for the lifetime of the process, because the empty string is treated as a cache hit. Consider only caching non-empty labels, or distinguishing error/negative-cache cases (e.g., skip storeLabel when Source returns a non-nil error, while still caching successful results and ipgeo.Filter hits).
| if geo, ok := ipgeo.Filter(ip); ok { | |
| label = FormatGeo(geo, a.cfg.Lang) | |
| } else if a.cfg.Source != nil && ctx.Err() == nil { | |
| if geo, err := a.cfg.Source(ip, a.cfg.Timeout, a.cfg.Lang, false); err == nil { | |
| label = FormatGeo(geo, a.cfg.Lang) | |
| } | |
| } | |
| a.storeLabel(ip, label) | |
| shouldCache := false | |
| if geo, ok := ipgeo.Filter(ip); ok { | |
| label = FormatGeo(geo, a.cfg.Lang) | |
| shouldCache = true | |
| } else if a.cfg.Source != nil && ctx.Err() == nil { | |
| if geo, err := a.cfg.Source(ip, a.cfg.Timeout, a.cfg.Lang, false); err == nil { | |
| label = FormatGeo(geo, a.cfg.Lang) | |
| shouldCache = true | |
| } | |
| } | |
| if shouldCache { | |
| a.storeLabel(ip, label) | |
| } |
| type Span struct { | ||
| Start int | ||
| End int | ||
| InsertEnd int | ||
| ScanEnd int | ||
| Text string | ||
| LookupIP string | ||
| Family Family | ||
| } |
There was a problem hiding this comment.
Span.ScanEnd is stored in parseAddr but is not used anywhere else in this package. Keeping unused fields increases surface area and makes the code harder to reason about. Consider removing it (and the corresponding parameter plumb-through) unless it’s needed for an imminent follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6a937ebbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } | ||
| line, err := reader.ReadString('\n') |
There was a problem hiding this comment.
In Run, the loop checks ctx.Err() before calling ReadString, but ReadString('\n') can block indefinitely on stdin and does not observe context cancellation. In nexttrace --nali interactive use (no immediate newline from stdin), pressing Ctrl-C cancels the context yet the process remains stuck waiting for input, so the mode cannot exit promptly on signal. This should be changed so the read path can be interrupted when the context is canceled.
Useful? React with 👍 / 👎.
tsosunchia
force-pushed
the
codex/nali-mode
branch
from
86c9bf9 to
20f0854
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func readLines(ctx context.Context, input io.Reader) <-chan lineReadResult { | ||
| results := make(chan lineReadResult, 1) | ||
| go func() { | ||
| defer close(results) | ||
| reader := bufio.NewReader(input) | ||
| for { | ||
| line, err := reader.ReadString('\n') | ||
| select { | ||
| case results <- lineReadResult{line: line, err: err}: | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| if err != nil { | ||
| return | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
readLines runs a goroutine that blocks on reader.ReadString('\n') and cannot be interrupted by ctx cancellation. When Run returns early (e.g., user types quit/exit or ctx is canceled), the goroutine can remain blocked indefinitely on readers that are not closed, leaking a goroutine.
Consider avoiding the extra goroutine and accepting that reads are blocking, or (if you want cancellation to work) detect io.ReadCloser and close it on exit/cancel, or pass a closer/stop hook into Run so the read can be unblocked deterministically.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/nali/nali.go`:
- Around line 263-266: The parse loop currently sets i = end when
parseCandidate(line[i:end], i) fails, which skips the rest of the candidate and
can miss literals like "dead:8.8.8.8"; change the failure path in the loop
around parseCandidate so it advances i by at least 1 byte (e.g., i++ or i = i+1)
instead of i = end to allow the scanner to resync on the separator and reattempt
matching, and add a regression test that feeds input like "dead:8.8.8.8" and
asserts the IP literal is detected; update the code paths referencing
parseCandidate, i and end accordingly.
- Around line 27-49: The three newly added concrete structs (Config, Span,
Annotator) should follow the repository convention of using interface-based
object shapes: replace type Config struct{...}, type Span struct{...}, and type
Annotator struct{...} with interface types (e.g., type Config interface {
Timeout() time.Duration; Source() ipgeo.Source; Lang() string; Family() Family }
and similar getter methods for Span and Annotator) and implement those
interfaces in a private concrete type (e.g., configImpl, spanImpl,
annotatorImpl); update all call sites to use the interface methods instead of
direct field access (or add the repo guideline exception to documentation if
changing to interfaces is not feasible). Ensure unique symbols to touch: Config,
Span, Annotator, and any methods that currently read fields like cfg.Timeout or
span.LookupIP are updated to use the new accessors.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc13bfa6-0d32-4eb0-ab85-ffbb91862624
📒 Files selected for processing (11)
README.mdREADME_zh_CN.mdcmd/cmd.gocmd/cmd_test.gocmd/flavor_full.gocmd/flavor_ntr.gocmd/flavor_tiny.gocmd/nali_mode.gocmd/nali_mode_test.gointernal/nali/nali.gointernal/nali/nali_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/flavor_ntr.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/flavor_tiny.go
- cmd/flavor_full.go
- cmd/nali_mode_test.go
- cmd/nali_mode.go
| type Config struct { | ||
| Source ipgeo.Source | ||
| Timeout time.Duration | ||
| Lang string | ||
| Family Family | ||
| } | ||
|
|
||
| type Span struct { | ||
| Start int | ||
| End int | ||
| InsertEnd int | ||
| Text string | ||
| LookupIP string | ||
| Family Family | ||
| } | ||
|
|
||
| type Annotator struct { | ||
| cfg Config | ||
| cacheMu sync.RWMutex | ||
| cache map[string]string | ||
| cacheRing []string | ||
| cacheNext int | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
按仓库约定处理新增对象形状类型。
Config、Span、Annotator 是本包新增的对象形状，但当前以 struct 定义；如果该规则适用于本包，请改成项目约定的 interface{}/接口抽象，或在规范中补充 Go 例外。As per coding guidelines, **/*.{ts,tsx,go}: Use interface{} for defining object shapes instead of struct types in TypeScript/Go code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/nali/nali.go` around lines 27 - 49, The three newly added concrete
structs (Config, Span, Annotator) should follow the repository convention of
using interface-based object shapes: replace type Config struct{...}, type Span
struct{...}, and type Annotator struct{...} with interface types (e.g., type
Config interface { Timeout() time.Duration; Source() ipgeo.Source; Lang()
string; Family() Family } and similar getter methods for Span and Annotator) and
implement those interfaces in a private concrete type (e.g., configImpl,
spanImpl, annotatorImpl); update all call sites to use the interface methods
instead of direct field access (or add the repo guideline exception to
documentation if changing to interfaces is not feasible). Ensure unique symbols
to touch: Config, Span, Annotator, and any methods that currently read fields
like cfg.Timeout or span.LookupIP are updated to use the new accessors.
Summary
--nalimode for annotating IPv4/IPv6 literals in text streamsBehavior
nexttrace --nali 1.1.1.1annotates one line and exitsquit/exitnexttrace-tinyandntrdo not register--nali--jsonand traceroute/MTR/MTU/Web/probe/source output flags conflict with--naliValidation
go test ./...go test -tags flavor_tiny ./cmdgo test -tags flavor_ntr ./cmdgo build ./...go build -tags flavor_tiny ./...go build -tags flavor_ntr ./...printf 'A 1.1.1.1\\n' | /tmp/nexttrace-nali --nali --data-provider disable-geoip\n-/tmp/nexttrace-nali --nali --data-provider IPInfo --language en --timeout 3000 1.1.1.1\n-/tmp/nexttrace-tiny-nali --helpand/tmp/ntr-nali --helpchecked for absence of--nali\n\n## Copyright\nNo nali source files, regex implementations, databases, config paths, or update logic were copied. The README credits zu1k/nali and links to its MIT license because this mode is feature-inspired by nali.Summary by CodeRabbit
--nali与对应“文本 IP 标注”功能（仅完整版 nexttrace 可用），并新增内部 nali 注释实现以在文本中为 IPv4/IPv6 字面量追加地理/提供者标注。--nali说明、示例与可用性约束。--nali注册、参数兼容性、运行时行为与注释输出的单元/集成测试。