add comma separated support for -l option#895
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/runner/options.go (1)
286-295: Remove stream-mode check from the multi-stdin exclusivity block.The inner
if options.Stream { ... "argument stdin not supported in stream mode" }only triggers when two flags use stdin and is redundant/confusing since stream-mode constraints are enforced below. Keep this block focused on exclusivity across flags.Apply this diff:
- // stdin can be set only on one flag - if (argumentHasStdin(options.Domains) && argumentHasStdin(options.WordList)) || - (argumentHasStdin(options.Domains) && argumentHasStdin(options.Hosts)) || - (argumentHasStdin(options.WordList) && argumentHasStdin(options.Hosts)) { - if options.Stream { - gologger.Fatal().Msgf("argument stdin not supported in stream mode") - } - gologger.Fatal().Msgf("stdin can be set for one flag") - } + // stdin can be set only on one flag + if (argumentHasStdin(options.Domains) && argumentHasStdin(options.WordList)) || + (argumentHasStdin(options.Domains) && argumentHasStdin(options.Hosts)) || + (argumentHasStdin(options.WordList) && argumentHasStdin(options.Hosts)) { + gologger.Fatal().Msgf("stdin can be set for one flag") + }
🧹 Nitpick comments (2)
internal/runner/options.go (1)
303-305: Clarify the stream-mode error for -l with a usage hint.Suggest making the message actionable so users know how to feed stdin in stream mode.
- gologger.Fatal().Msgf("hosts not supported in stream mode") + gologger.Fatal().Msgf("hosts (-l) not supported in stream mode; pipe input instead (e.g., dnsx -stream < hosts.txt)")internal/runner/runner.go (1)
256-259: Add empty-item guard and clearer empty-input error
- In the
if sc == nilblock, distinguishHosts == ""and return
errors.New("no input provided: use -l <file|comma-separated values> or pipe via stdin")instead of the opaque"empty argument".- In the host loop, immediately skip blank entries after trimming:
for item := range sc { item := normalize(item) if item == "" { continue } … }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/runner/options.go(3 hunks)internal/runner/runner.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Functional Test (windows-latest)
🔇 Additional comments (1)
internal/runner/options.go (1)
97-97: Help text update for -l looks good.Accurately reflects comma-separated support and aligns with -d/-w descriptions.
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
breaks stdin import + failing tests
Neo - PR Security ReviewCaution Review could not be completed Task aborted before completion Suggestion: Try again with Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runner/runner_test.go (1)
130-137: Duplicate test: "file" subtest duplicatesTestRunner_fileInput_prepareInput.This subtest is identical to the existing
TestRunner_fileInput_prepareInputtest at lines 107-127—same file path, same expected values. Consider removing this subtest to avoid redundant test execution.♻️ Suggested removal
func TestRunner_hostsInput_prepareInput(t *testing.T) { - t.Run("file", func(t *testing.T) { - hm, err := hybrid.New(hybrid.DefaultDiskOptions) - require.NoError(t, err) - r := Runner{options: &Options{Hosts: "tests/file_input.txt"}, hm: hm} - require.NoError(t, r.prepareInput()) - got := scanHMap(t, r.hm) - require.ElementsMatch(t, []string{"one.one.one.one", "example.com"}, got) - }) - t.Run("stdin", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner_test.go` around lines 130 - 137, The "file" subtest in runner_test.go duplicates the existing TestRunner_fileInput_prepareInput test; remove the redundant subtest block (the t.Run("file", ... ) that creates hybrid.New, constructs Runner{options: &Options{Hosts: "tests/file_input.txt"}, hm: hm}, calls r.prepareInput(), and asserts the same ElementsMatch) so only TestRunner_fileInput_prepareInput remains; ensure no other references to that anonymous subtest remain and run tests to confirm coverage is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runner/runner_test.go`:
- Around line 130-137: The "file" subtest in runner_test.go duplicates the
existing TestRunner_fileInput_prepareInput test; remove the redundant subtest
block (the t.Run("file", ... ) that creates hybrid.New, constructs
Runner{options: &Options{Hosts: "tests/file_input.txt"}, hm: hm}, calls
r.prepareInput(), and asserts the same ElementsMatch) so only
TestRunner_fileInput_prepareInput remains; ensure no other references to that
anonymous subtest remain and run tests to confirm coverage is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9549c40d-5422-4b5e-af2e-a14b248f6564
📒 Files selected for processing (2)
internal/runner/runner.gointernal/runner/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runner/runner.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/runner/runner_test.go (2)
144-151: Consider removing duplicate test.This sub-test duplicates
TestRunner_fileInput_prepareInput(lines 118-141). Consider removing the older test to reduce maintenance overhead, or keep both if you want the redundancy for different failure contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner_test.go` around lines 144 - 151, The test "file" in runner_test.go duplicates the earlier TestRunner_fileInput_prepareInput; remove one to avoid redundant coverage — either delete the "file" sub-test (the t.Run block invoking hybrid.New and Runner{options: &Options{Hosts: "tests/file_input.txt"}, hm: hm} then calling r.prepareInput()) or delete the older TestRunner_fileInput_prepareInput, keeping only the desired case; ensure that the remaining test still exercises Runner.prepareInput and asserts the same expected hosts ("one.one.one.one", "example.com").
178-185: Good coverage for the core comma-separated feature.Consider adding edge-case tests for robustness (e.g.,
"host1, host2"with spaces,"host1,,host2"with empty segments, trailing/leading commas) if not already handled in integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner_test.go` around lines 178 - 185, Add unit tests around the Hosts parsing to cover edge cases: test inputs like "host1, host2" (spaces), "host1,,host2" (empty segments), and leading/trailing commas ",host1,host2," to ensure Runner.prepareInput() trims whitespace, ignores empty segments and still populates Runner.hm correctly; add subtests alongside the existing "comma separated" test that construct Runner{options: &Options{Hosts: ...}, hm: hm}, call r.prepareInput(), then assert the expected host list via scanHMap(t, r.hm).
🤖 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/runner/runner_test.go`:
- Around line 156-159: The deferred os.Remove(...) and the tmp.Close() call
currently ignore returned errors; capture their errors and assert they succeed
(e.g., err := os.Remove(tmp.Name()); require.NoError(t, err) inside a defer
wrapper, and err := tmp.Close(); require.NoError(t, err) after calling
tmp.Close()) so the test fails on unexpected IO errors. Update the code around
tmp (the temp file variable), replacing the raw defer os.Remove(...) with a
deferred func that checks the remove error and replace the bare tmp.Close() with
an error capture followed by require.NoError(t, err) (use the existing require
package).
---
Nitpick comments:
In `@internal/runner/runner_test.go`:
- Around line 144-151: The test "file" in runner_test.go duplicates the earlier
TestRunner_fileInput_prepareInput; remove one to avoid redundant coverage —
either delete the "file" sub-test (the t.Run block invoking hybrid.New and
Runner{options: &Options{Hosts: "tests/file_input.txt"}, hm: hm} then calling
r.prepareInput()) or delete the older TestRunner_fileInput_prepareInput, keeping
only the desired case; ensure that the remaining test still exercises
Runner.prepareInput and asserts the same expected hosts ("one.one.one.one",
"example.com").
- Around line 178-185: Add unit tests around the Hosts parsing to cover edge
cases: test inputs like "host1, host2" (spaces), "host1,,host2" (empty
segments), and leading/trailing commas ",host1,host2," to ensure
Runner.prepareInput() trims whitespace, ignores empty segments and still
populates Runner.hm correctly; add subtests alongside the existing "comma
separated" test that construct Runner{options: &Options{Hosts: ...}, hm: hm},
call r.prepareInput(), then assert the expected host list via scanHMap(t, r.hm).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42543bca-8fd6-4b0c-8ac0-28b772d449c3
📒 Files selected for processing (3)
internal/runner/options.gointernal/runner/runner.gointernal/runner/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/runner/options.go
- internal/runner/runner.go
| defer os.Remove(tmp.Name()) | ||
| _, err = tmp.WriteString("one.one.one.one\nexample.com\n") | ||
| require.NoError(t, err) | ||
| tmp.Close() |
There was a problem hiding this comment.
Fix unchecked error returns to pass pipeline.
The linter and pipeline are failing because os.Remove and tmp.Close return values are not checked.
🔧 Proposed fix
t.Run("stdin", func(t *testing.T) {
tmp, err := os.CreateTemp("", "dnsx-stdin-test")
require.NoError(t, err)
- defer os.Remove(tmp.Name())
+ defer func() { _ = os.Remove(tmp.Name()) }()
_, err = tmp.WriteString("one.one.one.one\nexample.com\n")
require.NoError(t, err)
- tmp.Close()
+ require.NoError(t, tmp.Close())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer os.Remove(tmp.Name()) | |
| _, err = tmp.WriteString("one.one.one.one\nexample.com\n") | |
| require.NoError(t, err) | |
| tmp.Close() | |
| defer func() { _ = os.Remove(tmp.Name()) }() | |
| _, err = tmp.WriteString("one.one.one.one\nexample.com\n") | |
| require.NoError(t, err) | |
| require.NoError(t, tmp.Close()) |
🧰 Tools
🪛 GitHub Actions: 🔨 Build Test
[error] 156-156: golangci-lint (errcheck): Error return value of os.Remove is not checked
🪛 GitHub Check: Lint Test
[failure] 159-159:
Error return value of tmp.Close is not checked (errcheck)
[failure] 156-156:
Error return value of os.Remove is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/runner/runner_test.go` around lines 156 - 159, The deferred
os.Remove(...) and the tmp.Close() call currently ignore returned errors;
capture their errors and assert they succeed (e.g., err :=
os.Remove(tmp.Name()); require.NoError(t, err) inside a defer wrapper, and err
:= tmp.Close(); require.NoError(t, err) after calling tmp.Close()) so the test
fails on unexpected IO errors. Update the code around tmp (the temp file
variable), replacing the raw defer os.Remove(...) with a deferred func that
checks the remove error and replace the bare tmp.Close() with an error capture
followed by require.NoError(t, err) (use the existing require package).
2 participants
closes #878
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation