Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the monolithic pkg/scanner package into modular domain packages (pkg/engine, pkg/results, pkg/targets, pkg/discovery, and pkg/ports), introducing a context-aware execution model and removing global state. A temporary backward-compatibility shim is also provided. The review feedback focuses on fully propagating context cancellation by making ports.ScanPorts context-aware, checking for cancellation before performing network operations in engine.StartScan, and fixing a concurrency bug in the shim's cancellation tracking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // ScanPorts varre uma lista de portas em um IP e retorna as abertas. | ||
| func ScanPorts(ip string, ports []int, timeoutMs int) []int { | ||
| if err := validateIPv4(ip); err != nil { | ||
| return nil | ||
| } | ||
| var openPorts []int | ||
| timeout := time.Duration(timeoutMs) * time.Millisecond | ||
| for _, port := range ports { | ||
| address := net.JoinHostPort(ip, strconv.Itoa(port)) | ||
| conn, err := net.DialTimeout("tcp", address, timeout) | ||
| if err == nil { | ||
| conn.Close() | ||
| openPorts = append(openPorts, port) | ||
| } | ||
| } | ||
| return openPorts | ||
| } |
There was a problem hiding this comment.
To make the port scanner truly context-aware and responsive to cancellation, we should accept a context.Context and use dialer.DialContext instead of net.DialTimeout. This prevents the scan from hanging if the context is cancelled during a long port scan.
// ScanPorts varre uma lista de portas em um IP e retorna as abertas, respeitando o contexto.
func ScanPorts(ctx context.Context, ip string, ports []int, timeoutMs int) []int {
if err := validateIPv4(ip); err != nil {
return nil
}
var openPorts []int
dialer := net.Dialer{Timeout: time.Duration(timeoutMs) * time.Millisecond}
for _, port := range ports {
select {
case <-ctx.Done():
return openPorts
default:
}
address := net.JoinHostPort(ip, strconv.Itoa(port))
conn, err := dialer.DialContext(ctx, "tcp", address)
if err == nil {
conn.Close()
openPorts = append(openPorts, port)
}
}
return openPorts
}| if di.IsAlive { | ||
| di.Hostname = ReverseDNS(ip) | ||
| di.MAC = GetMAC(ip) | ||
| di.OpenPorts = ScanPorts(ip, cfg.DefaultPorts, cfg.PortTimeoutMs) | ||
| di.Hostname = discovery.ReverseDNS(ip) | ||
| di.MAC = discovery.GetMAC(ip) | ||
| di.OpenPorts = ports.ScanPorts(ip, cfg.DefaultPorts, cfg.PortTimeoutMs) | ||
| di.OpenPortsCount = len(di.OpenPorts) | ||
| } |
There was a problem hiding this comment.
Pass the ctx to ports.ScanPorts to ensure that port scanning respects context cancellation. Additionally, we should check if the context has been cancelled before initiating subsequent network operations (like reverse DNS and MAC lookup) to avoid unnecessary work.
if di.IsAlive {
if ctx.Err() != nil {
return
}
di.Hostname = discovery.ReverseDNS(ip)
if ctx.Err() != nil {
return
}
di.MAC = discovery.GetMAC(ip)
di.OpenPorts = ports.ScanPorts(ctx, ip, cfg.DefaultPorts, cfg.PortTimeoutMs)
di.OpenPortsCount = len(di.OpenPorts)
}| var ( | ||
| shimMu sync.Mutex | ||
| shimCancel context.CancelFunc | ||
| ) | ||
|
|
||
| // StartScan is deprecated: Use engine.StartScan with a context.Context instead. | ||
| func StartScan(ips []string, cfg ScanConfig, onResult func(DeviceInfo), onProgress func(float64)) error { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| shimMu.Lock() | ||
| shimCancel = cancel | ||
| shimMu.Unlock() | ||
|
|
||
| defer func() { | ||
| shimMu.Lock() | ||
| shimCancel = nil | ||
| shimMu.Unlock() | ||
| cancel() | ||
| }() | ||
|
|
||
| return engine.StartScan(ctx, ips, cfg, onResult, onProgress) | ||
| } | ||
|
|
||
| // StopScan is deprecated: Caller should pass a cancelable context to engine.StartScan instead. | ||
| func StopScan() { | ||
| shimMu.Lock() | ||
| defer shimMu.Unlock() | ||
| if shimCancel != nil { | ||
| shimCancel() | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of the backward compatibility shim has a concurrency bug: if multiple scans are started concurrently, they will overwrite the global shimCancel variable, meaning StopScan() will only cancel the last started scan. To fix this while keeping the shim thread-safe and concurrent, we can store active cancellations in a map.
var (
shimMu sync.Mutex
shimCancels = make(map[context.CancelFunc]bool)
)
// StartScan is deprecated: Use engine.StartScan with a context.Context instead.
func StartScan(ips []string, cfg ScanConfig, onResult func(DeviceInfo), onProgress func(float64)) error {
ctx, cancel := context.WithCancel(context.Background())
shimMu.Lock()
shimCancels[cancel] = true
shimMu.Unlock()
defer func() {
shimMu.Lock()
delete(shimCancels, cancel)
shimMu.Unlock()
cancel()
}()
return engine.StartScan(ctx, ips, cfg, onResult, onProgress)
}
// StopScan is deprecated: Caller should pass a cancelable context to engine.StartScan instead.
func StopScan() {
shimMu.Lock()
defer shimMu.Unlock()
for cancel := range shimCancels {
cancel()
}
}| package ports | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net" | ||
| "strconv" | ||
| "time" | ||
| ) |
There was a problem hiding this comment.
To support context-aware port scanning, we need to import the context package.
| package ports | |
| import ( | |
| "fmt" | |
| "net" | |
| "strconv" | |
| "time" | |
| ) | |
| package ports | |
| import ( | |
| "context" | |
| "fmt" | |
| "net" | |
| "strconv" | |
| "time" | |
| ) |
| // ScanPorts is deprecated: Use ports.ScanPorts instead. | ||
| func ScanPorts(ip string, portsList []int, timeoutMs int) []int { | ||
| return ports.ScanPorts(ip, portsList, timeoutMs) | ||
| } |
There was a problem hiding this comment.
Update the deprecated ScanPorts wrapper to pass context.Background() to the updated ports.ScanPorts function.
| // ScanPorts is deprecated: Use ports.ScanPorts instead. | |
| func ScanPorts(ip string, portsList []int, timeoutMs int) []int { | |
| return ports.ScanPorts(ip, portsList, timeoutMs) | |
| } | |
| // ScanPorts is deprecated: Use ports.ScanPorts instead. | |
| func ScanPorts(ip string, portsList []int, timeoutMs int) []int { | |
| return ports.ScanPorts(context.Background(), ip, portsList, timeoutMs) | |
| } |
mendsec
left a comment
mendsec
left a comment
There was a problem hiding this comment.
Review — refactor: M1 Core Architecture Hardening
Overall impression
This is a strong, well-scoped architectural refactor that moves the project away from a monolithic pkg/scanner toward smaller, focused domain packages and a context-driven execution model.
The deprecation shim and API stability docs show good attention to downstream consumers and migration ergonomics.
What I like
Clear package boundaries: engine, results, targets, discovery, and ports each have a single, well-defined responsibility which makes the codebase easier to understand and maintain.
Context-aware execution: replacing global locking/state with context.Context in engine.StartScan is an important improvement for concurrency and cancellation semantics.
Backwards compatibility: the shim in pkg/scanner/shim.go preserves existing public names while encouraging consumers to migrate, reducing breakage risk.
Cross-platform testing: tests updated for both POSIX and Windows and confirmed with go test ./... and go build ./... — good CI hygiene.
Documentation: the milestone changes doc and api-stability contract help communicate the intent and migration plan to integrators.
Suggested improvements / questions
Shim lifetime and visibility: clarify how long the shim will remain supported and whether there’s a deprecation timeline or semantic versioning plan in docs/contracts/api-stability.md. Consumers will benefit from an expected removal version or timeframe.
Tests coverage gaps: please confirm whether existing unit/integration coverage for ScanPorts, Ping/GetMAC/ReverseDNS, and ParseRange remains the same after the move; if coverage dropped, consider adding focused tests for the new pkg/ports and pkg/discovery boundary cases (timeouts, partial failures, parsing invalid ranges).
Error handling contracts: recommend documenting error shapes (wrapped vs typed errors) returned by engine.StartScan, ScanPorts, and ParseRange so callers can programmatically handle retry/cancel/ignore scenarios. Consider returning sentinel errors or typed error values where appropriate.
Context cancellation semantics: in addition to accepting context.Context, consider documenting whether StartScan returns quickly on cancellation (and what cleanup happens), and whether long-running goroutines are guaranteed to exit within a bounded time. A short note in engine/scan.go or the README would help integrators.
Concurrency model doc: the removal of global scan state is great — consider adding a short section to ARCHITECTURE.md that describes the intended concurrency model (allowed parallel scans per process, resource limits, and how callers should coordinate multiple scans).
API stability examples: add a small migration example in docs (or README) showing the before/after call site for StartScan and for cancelling a running scan using context.WithCancel — this will help downstream projects adopt the new API faster.
Minor code/style suggestions
Package-level comments: add package doc comments for the new packages (pkg/engine, pkg/results, pkg/targets, pkg/discovery, pkg/ports) describing exported types and intents — this helps with godoc and onboarding.
Naming consistency: ensure exported names across packages share a consistent naming pattern (e.g., ScanConfig vs. DefaultConfig vs. StartScan) to make the public API intuitive.
Tests: where possible, prefer table-driven tests for ParseRange and discovery functions to make boundary cases explicit and easy to extend.
Logging: confirm logging expectations (structured logs, log levels) for engine.StartScan and discovery/ports functions, and document any logger interface or conventions so consumers can integrate with their logging stack.
|
This PR will be closed because it's currently targeting the main branch, but it should be merged into the develop branch instead. Please reopen a new PR with develop as the base branch. |
Description
This PR implements Milestone 1: Core Architecture Hardening.
Changes
Breaking Changes & Migration
pkg/scanner is now deprecated. Downstream consumers should migrate to the new packages. StopScan() is deprecated in favor of passing a cancelable context to engine.StartScan().