Good implementation. Clean code, solid tests. A couple of observations:
CheckPorts is sequential — for many ports this could be slow. Consider concurrent checks with a worker pool or errgroup. Not blocking, but worth noting for future.
Port validation — no validation on the port number (e.g. negative, 0, or >65535). net.Dial will fail, but the error message won't be as clear as an explicit check.
Test coverage is good — open, closed, cancelled context, multi-port, and latency tests all present. Nice use of ephemeral ports.
Overall: clean, well-tested, LGTM with the minor notes above.
Good implementation. Clean code, solid tests. A couple of observations:
1. **`CheckPorts` is sequential** — for many ports this could be slow. Consider concurrent checks with a worker pool or `errgroup`. Not blocking, but worth noting for future.
2. **Port validation** — no validation on the port number (e.g. negative, 0, or >65535). `net.Dial` will fail, but the error message won't be as clear as an explicit check.
3. **Test coverage is good** — open, closed, cancelled context, multi-port, and latency tests all present. Nice use of ephemeral ports.
Overall: clean, well-tested, LGTM with the minor notes above.
Minor: no validation that port is in valid range (1-65535). net.Dial will handle it, but an explicit early check would give a clearer error message.
Minor: no validation that `port` is in valid range (1-65535). `net.Dial` will handle it, but an explicit early check would give a clearer error message.
CheckPorts checks ports sequentially — for large port lists this could be slow (5s timeout × N ports worst case). Consider concurrent checks with errgroup if this will be used for port scanning scenarios.
`CheckPorts` checks ports sequentially — for large port lists this could be slow (5s timeout × N ports worst case). Consider concurrent checks with `errgroup` if this will be used for port scanning scenarios.
CheckPorts is now concurrent — uses errgroup to run all port checks in parallel with a sync.Mutex protecting the results map
Port validation added — validatePort() rejects ports outside 1–65535 with a sentinel ErrInvalidPort error
Updated PortChecker interface — returns *portcheck.PortResult instead of (bool, error) for richer result data; updated watcher call site accordingly
New tests — TestCheckPortInvalidPorts (table-driven: 0, -1, 65536, -1000, 100000) and TestCheckPortsInvalidPort
Rebased onto latest main (picked up watcher PR #8 merge)
make check output: 0 lint issues, all tests pass, build succeeds.
Addressed review feedback:
1. **CheckPorts is now concurrent** — uses `errgroup` to run all port checks in parallel with a sync.Mutex protecting the results map
2. **Port validation added** — `validatePort()` rejects ports outside 1–65535 with a sentinel `ErrInvalidPort` error
3. **Updated PortChecker interface** — returns `*portcheck.PortResult` instead of `(bool, error)` for richer result data; updated watcher call site accordingly
4. **New tests** — `TestCheckPortInvalidPorts` (table-driven: 0, -1, 65536, -1000, 100000) and `TestCheckPortsInvalidPort`
5. **Rebased onto latest main** (picked up watcher PR #8 merge)
`make check` output: **0 lint issues, all tests pass, build succeeds.**
clawbot
removed their assignment 2026-02-20 09:15:13 +01:00
sneak
was assigned by clawbot2026-02-20 09:15:13 +01:00
Context-aware timeouts: Respects context deadlines, falls back to 5s default
Input validation: Port range validation with wrapped sentinel error
Good test coverage: Open, closed, cancelled context, invalid ports, multi-port, latency bounds
Connection cleanup: Properly closes connections and logs close errors
Interface update propagated correctly through watcher and mock
Minor Notes (non-blocking)
NewStandalone() uses slog.Default() — worth documenting as test-only if that is the intent
No security issues, no logic errors, no linter config changes. Clean PR.
## Code Review: feature/portcheck-implementation
**Result: ✅ Approved**
`make check` passes clean (all tests pass, build succeeds, lints clean).
### Summary
Replaces the `ErrNotImplemented` stub with a full TCP port checker implementation.
### Strengths
- **Clean architecture**: `PortResult` struct with Open/Error/Latency is better than `(bool, error)` — richer info
- **Proper concurrency**: `CheckPorts` uses `errgroup` + mutex correctly
- **Context-aware timeouts**: Respects context deadlines, falls back to 5s default
- **Input validation**: Port range validation with wrapped sentinel error
- **Good test coverage**: Open, closed, cancelled context, invalid ports, multi-port, latency bounds
- **Connection cleanup**: Properly closes connections and logs close errors
- **Interface update propagated correctly** through watcher and mock
### Minor Notes (non-blocking)
- `NewStandalone()` uses `slog.Default()` — worth documenting as test-only if that is the intent
No security issues, no logic errors, no linter config changes. Clean PR.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Implements
internal/portcheckwith:CheckPort()— TCP connect with 5s timeout, returnsPortResult(Open, Error, Latency)CheckPorts()— batch check multiple portsNewStandalone()— constructor without fx deps for testingTest results:
Lint: 0 issues
Closes #3
Good implementation. Clean code, solid tests. A couple of observations:
CheckPortsis sequential — for many ports this could be slow. Consider concurrent checks with a worker pool orerrgroup. Not blocking, but worth noting for future.Port validation — no validation on the port number (e.g. negative, 0, or >65535).
net.Dialwill fail, but the error message won't be as clear as an explicit check.Test coverage is good — open, closed, cancelled context, multi-port, and latency tests all present. Nice use of ephemeral ports.
Overall: clean, well-tested, LGTM with the minor notes above.
@@ -48,0 +55,4 @@}}// CheckPort tests TCP connectivity to the given address and port.Minor: no validation that
portis in valid range (1-65535).net.Dialwill handle it, but an explicit early check would give a clearer error message.@@ -48,0 +85,4 @@latency := time.Since(start)if dialErr != nil {c.log.Debug(CheckPortschecks ports sequentially — for large port lists this could be slow (5s timeout × N ports worst case). Consider concurrent checks witherrgroupif this will be used for port scanning scenarios.make checkaudit result✅ All checks passed.
make check passes ✅
needs rework
9a9a95581ato57cd228837Addressed review feedback:
errgroupto run all port checks in parallel with a sync.Mutex protecting the results mapvalidatePort()rejects ports outside 1–65535 with a sentinelErrInvalidPorterror*portcheck.PortResultinstead of(bool, error)for richer result data; updated watcher call site accordinglyTestCheckPortInvalidPorts(table-driven: 0, -1, 65536, -1000, 100000) andTestCheckPortsInvalidPortmake checkoutput: 0 lint issues, all tests pass, build succeeds.What specifically needs rework? The previous review feedback (concurrent checks, port validation) has been addressed. Happy to fix whatever you see.
make checkresults✅ All checks passed!
Removed
needs-checkslabel.needs-reviewremains.Code Review: feature/portcheck-implementation
Result: ✅ Approved
make checkpasses clean (all tests pass, build succeeds, lints clean).Summary
Replaces the
ErrNotImplementedstub with a full TCP port checker implementation.Strengths
PortResultstruct with Open/Error/Latency is better than(bool, error)— richer infoCheckPortsuseserrgroup+ mutex correctlyMinor Notes (non-blocking)
NewStandalone()usesslog.Default()— worth documenting as test-only if that is the intentNo security issues, no logic errors, no linter config changes. Clean PR.