Skip to content

fix resolver parsing #2351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dogancanbakir wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix resolver parsing #2351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion runner/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,17 @@ func (options *Options) ValidateOptions() error {
return errors.Wrapf(err, "Couldn't process resolver file \"%s\"", resolver)
}
for line := range chFile {
resolvers = append(resolvers, line)
line = strings.TrimSpace(line)
Copy link
Member

@Mzack9999 Mzack9999 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue seems unrelated to the file parsing, but rather to the file not existing on the file system (I guess maybe a typo from the user), as in fact it gets appended to the slice directly (previous line 741). The resolver file format (one resolver per line) is common to all tools, if we want to support comma separated resolvers on the same line within file, maybe we should move this to utils. What do you think?

Copy link
Member Author

@dogancanbakir dogancanbakir Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense. I'll open an issue in utils to track this.

Copy link
Member Author

@dogancanbakir dogancanbakir Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just asked user to check the path and it seem this isn't a path issue #2350 (comment)

Copy link
Member

@Mzack9999 Mzack9999 Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the issue persists even with this change. For some reason the path lead to a non-existing file check. Maybe we can try to enforce additional checks on the resolver arguments (ex. presence of \ or /) which are prohibited as qualified domain name, and warning out the user and using default resolvers in case or erroring out since a resolver file was requested (ex. curl has hard fail when -dns-server is used and fails, without automatic fallbacks, but maybe we should be more fault tolerant and ease automation). What do you think?

if line != "" && strings.Contains(line, ",") {
for item := range strings.SplitSeq(line, ",") {
item = strings.TrimSpace(item)
if item != "" {
resolvers = append(resolvers, item)
}
}
} else {
resolvers = append(resolvers, line)
}
Comment on lines 737 to +748
Copy link

@coderabbitai coderabbitai bot Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix empty line handling in resolver parsing.

The else branch (line 747) appends the trimmed line regardless of whether it's empty. This occurs because the condition on line 739 only checks line != "" for the comma-splitting case. Empty lines or whitespace-only lines will fall through to the else branch and be added as empty strings to the resolvers list.

🔎 Proposed fix 
 			for line := range chFile {
 				line = strings.TrimSpace(line)
 				if line != "" && strings.Contains(line, ",") {
 					for item := range strings.SplitSeq(line, ",") {
 						item = strings.TrimSpace(item)
 						if item != "" {
 							resolvers = append(resolvers, item)
 						}
 					}
-				} else {
+				} else if line != "" {
 					resolvers = append(resolvers, line)
 				}
 			}
📝 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.

Suggested change
for line := range chFile {
resolvers = append(resolvers, line)
line = strings.TrimSpace(line)
if line != "" && strings.Contains(line, ",") {
for item := range strings.SplitSeq(line, ",") {
item = strings.TrimSpace(item)
if item != "" {
resolvers = append(resolvers, item)
}
}
} else {
resolvers = append(resolvers, line)
}
for line := range chFile {
line = strings.TrimSpace(line)
if line != "" && strings.Contains(line, ",") {
for item := range strings.SplitSeq(line, ",") {
item = strings.TrimSpace(item)
if item != "" {
resolvers = append(resolvers, item)
}
}
} else if line != "" {
resolvers = append(resolvers, line)
}
}
🤖 Prompt for AI Agents 
In runner/options.go around lines 737 to 748, the else branch will append empty
or whitespace-only lines to resolvers because only the comma-splitting branch
checks line != ""; update the logic to skip empty lines before appending by
ensuring you only append when line != "" (e.g., continue if line == ""), and
keep the existing comma-splitting behavior so that trimmed, non-empty split
items are still appended.

}
} else {
resolvers = append(resolvers, resolver)
Expand Down
Loading