Lint + test fixes after utils dep update #6393
Conversation
- Replace deprecated errorutil with modern errkit - Convert error declarations from var to func for better compatibility - Fix all SA1019 deprecation warnings - Maintain error chain support and stack traces
- Configure test to use Google DNS (8.8.8.8) for stability - Fix nil pointer issue in DNS client initialization - Keep production defaults unchanged
ehsandeep
changed the title
WalkthroughMigrates repository-wide error handling from github.com/projectdiscovery/utils/errors → github.com/projectdiscovery/utils/errkit, converting many exported error variables into constructor functions and updating call sites; additionally adds lint, lint-strict, and test-with-lint targets to the Makefile. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Make as Makefile
participant Lint as golangci-lint
participant Test as go test
Dev->>Make: make test-with-lint
Make->>Lint: ensure/install golangci-lint
Make->>Lint: golangci-lint run
Lint-->>Make: lint status
alt lint success
Make->>Test: make test
Test-->>Make: test results
else lint failed
Make-->>Dev: exit with lint errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (11)
pkg/input/formats/openapi/generator.go (1)
400-423: Bug: incorrect AND/OR handling for security requirements and leaking state across iterationsTwo issues here:
- found is not reset per requirement, so a match in an earlier requirement incorrectly marks later ones as found.
- For requirements with multiple schemes (AND semantics per OAS3), the loop continues after the first match and never enforces that all schemes are present.
This can lead to silently accepting incomplete/invalid security requirements and generating incorrect params. Fix by evaluating each requirement atomically (collecting all required schemes), marking success only when all schemes in that requirement are present, and breaking on the first satisfied requirement (OR semantics across requirements).
Apply this diff to correct the logic:
- found := false - // this api is protected for each security scheme pull its corresponding scheme -schemaLabel: - for _, security := range *requirement { - for name := range security { - if scheme, ok := schema.Components.SecuritySchemes[name]; ok { - found = true - param, err := GenerateParameterFromSecurityScheme(scheme) - if err != nil { - return nil, err - } - globalParams = append(globalParams, &openapi3.ParameterRef{Value: param}) - continue schemaLabel - } - } - if (!found) && len(security) > 1 { - // if this is case then both security schemes are required - return nil, errkit.New(fmt.Sprintf("openapi: security requirement (%+v) not found in openapi file", security)).Build() - } - } - if !found { - return nil, errkit.New(fmt.Sprintf("openapi: security requirement (%+v) not found in openapi file", requirement)).Build() - } + anySatisfied := false + // Evaluate each requirement (OR across requirements). Within a requirement, + // all listed schemes are required (AND). + for _, security := range *requirement { + allPresent := true + localParams := openapi3.NewParameters() + for name := range security { + scheme, ok := schema.Components.SecuritySchemes[name] + if !ok { + allPresent = false + continue + } + param, err := GenerateParameterFromSecurityScheme(scheme) + if err != nil { + return nil, err + } + localParams = append(localParams, &openapi3.ParameterRef{Value: param}) + } + if allPresent { + // Prefer the first fully-satisfied requirement and ignore alternatives. + globalParams = append(globalParams, localParams...) + anySatisfied = true + break + } + // If a multi-scheme requirement is partially present, it is invalid. + if len(security) > 1 && !allPresent { + return nil, errkit.New(fmt.Sprintf("openapi: security requirement (%+v) not found in openapi file", security)).Build() + } + } + if !anySatisfied { + return nil, errkit.New(fmt.Sprintf("openapi: security requirement (%+v) not found in openapi file", requirement)).Build() + }pkg/protocols/network/network.go (1)
174-181: Fix TLS flag leakage across multiple addresses (once true, always true bug)
shouldUseTLSis shared across iterations. If any address has tls://, all subsequent addresses inherit TLS=true even if they don’t have the prefix. Compute TLS per-address to avoid incorrect behavior.Apply this diff:
- var shouldUseTLS bool request.options = options - for _, address := range request.Address { - // check if the connection should be encrypted - if strings.HasPrefix(address, "tls://") { - shouldUseTLS = true - address = strings.TrimPrefix(address, "tls://") - } - request.addresses = append(request.addresses, addressKV{address: address, tls: shouldUseTLS}) - } + for _, addr := range request.Address { + // compute TLS per address to avoid leakage across entries + tls := strings.HasPrefix(addr, "tls://") + if tls { + addr = strings.TrimPrefix(addr, "tls://") + } + request.addresses = append(request.addresses, addressKV{address: addr, tls: tls}) + }pkg/installer/util.go (1)
49-56: Close response body and enrich error context (prevents resource leak)resp.Body is never closed, which can leak connections/descriptors. Also, including status details helps debugging.
Apply:
func getNewAdditionsFileFromGitHub(version string) ([]string, error) { resp, err := retryableHttpClient.Get(fmt.Sprintf("https://raw.githubusercontent.com/projectdiscovery/nuclei-templates/%s/.new-additions", version)) if err != nil { return nil, err } + defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, errkit.New("version not found").Build() + return nil, errkit.New(fmt.Sprintf("version not found (status: %s) for version %s", resp.Status, version)).Build() }internal/pdcp/writer.go (1)
187-195: Bug: lines are dropped when buffer flushes at chunk boundaryWhen buff.Len()+len(line) > MaxChunkSize, you flush but never append the current line to the new buffer, effectively losing that line. Also handle the case where a single line exceeds MaxChunkSize.
Apply this diff to preserve the current line after flush and handle oversized lines:
- if buff.Len()+len(line) > MaxChunkSize { - // flush existing buffer - if err := u.uploadChunk(buff); err != nil { - u.Logger.Error().Msgf("Failed to upload scan results on cloud: %v", err) - } - } else { - buff.WriteString(line) - } + if buff.Len()+len(line) > MaxChunkSize { + // flush existing buffer + if err := u.uploadChunk(buff); err != nil { + u.Logger.Error().Msgf("Failed to upload scan results on cloud: %v", err) + } + // write the current line after flush; if line itself is huge, upload directly + if len(line) > MaxChunkSize { + tmp := bytes.NewBufferString(line) + if err := u.uploadChunk(tmp); err != nil { + u.Logger.Error().Msgf("Failed to upload scan results on cloud: %v", err) + } + } else { + buff.WriteString(line) + } + } else { + buff.WriteString(line) + }pkg/protocols/code/code.go (1)
426-433: Bug: incorrect handling of regex submatches for python env vars
FindAllStringSubmatchreturns an array wherematch[0]is the full match andmatch[1]is the first capture group. The code uses the whole match in both the pattern and the map lookup, and formats a slice with%s. This leads to incorrect replacements and potential panics.Apply this diff:
- for _, match := range matches { - if len(match) == 0 { - continue - } - source = strings.ReplaceAll(source, fmt.Sprintf("os.getenv('%s')", match), fmt.Sprintf("'%s'", vars[match[0]])) - } + for _, match := range matches { + // match[0] is full match, match[1] is the captured env var name + if len(match) < 2 { + continue + } + name := match[1] + source = strings.ReplaceAll( + source, + fmt.Sprintf("os.getenv('%s')", name), + fmt.Sprintf("'%v'", vars[name]), + ) + }pkg/js/libs/ssh/ssh.go (1)
160-165: Guard Close against nil connection to avoid panicClose dereferences c.connection without a nil check. If Close is called before a successful Connect, this will panic.
Apply this diff:
func (c *SSHClient) Close() (bool, error) { - if err := c.connection.Close(); err != nil { + if c.connection == nil { + return true, nil + } + if err := c.connection.Close(); err != nil { return false, err } return true, nil }cmd/tmc/main.go (4)
241-247: Close HTTP response body to prevent leaks (enhanceTemplate)resp.Body is never closed. Add a defer immediately after a successful Post to avoid descriptor leaks.
Apply this diff:
func enhanceTemplate(data string) (string, bool, error) { resp, err := retryablehttp.DefaultClient().Post(fmt.Sprintf("%s/enhance", tmBaseUrl), "application/x-yaml", strings.NewReader(data)) if err != nil { return data, false, err } + defer resp.Body.Close() if resp.StatusCode != 200 { return data, false, errkit.New(fmt.Sprintf("unexpected status code: %v", resp.Status)).Build() }
275-281: Close HTTP response body to prevent leaks (formatTemplate)Same leak here; close the body after Post succeeds.
Apply this diff:
func formatTemplate(data string) (string, bool, error) { resp, err := retryablehttp.DefaultClient().Post(fmt.Sprintf("%s/format", tmBaseUrl), "application/x-yaml", strings.NewReader(data)) if err != nil { return data, false, err } + defer resp.Body.Close() if resp.StatusCode != 200 { return data, false, errkit.New(fmt.Sprintf("unexpected status code: %v", resp.Status)).Build() }
309-315: Close HTTP response body to prevent leaks (lintTemplate)Add a defer to close the response body.
Apply this diff:
func lintTemplate(data string) (bool, error) { resp, err := retryablehttp.DefaultClient().Post(fmt.Sprintf("%s/lint", tmBaseUrl), "application/x-yaml", strings.NewReader(data)) if err != nil { return false, err } + defer resp.Body.Close() if resp.StatusCode != 200 { return false, errkit.New(fmt.Sprintf("unexpected status code: %v", resp.Status)).Build() }
331-337: Close HTTP response body to prevent leaks (validateTemplate)Add a defer to close resp.Body here as well.
Apply this diff:
func validateTemplate(data string) (bool, error) { resp, err := retryablehttp.DefaultClient().Post(fmt.Sprintf("%s/validate", tmBaseUrl), "application/x-yaml", strings.NewReader(data)) if err != nil { return false, err } + defer resp.Body.Close() if resp.StatusCode != 200 { return false, errkit.New(fmt.Sprintf("unexpected status code: %v", resp.Status)).Build() }cmd/integration-test/http.go (1)
631-647: Bug: returning the wrong error variable (err vs errx) hides validation failuresIf parameter validation fails, the function returns the unrelated ‘err’ (which is nil on success), causing silent test pass. Return errx instead.
if errx != nil { - return err + return errx }
| executionId := ctx.Value("executionId").(string) | ||
| if !protocolstate.IsHostAllowed(executionId, host) { | ||
| // host is not valid according to network policy | ||
| return false, protocolstate.ErrHostDenied.Msgf(host) | ||
| return false, protocolstate.ErrHostDenied(host) | ||
| } | ||
| // create a new client |
There was a problem hiding this comment.
Return nil (not false/"") for interface{} result on errors
RunLuaScript returns (interface{}, error). On error paths you currently return false or "", which produces non-nil interface values and can confuse callers. Return nil for the value when an error is returned.
Apply this diff:
func RunLuaScript(ctx context.Context, host string, port int, password string, script string) (interface{}, error) {
executionId := ctx.Value("executionId").(string)
if !protocolstate.IsHostAllowed(executionId, host) {
// host is not valid according to network policy
- return false, protocolstate.ErrHostDenied(host)
+ return nil, protocolstate.ErrHostDenied(host)
}
// create a new client
client := redis.NewClient(&redis.Options{
@@
// Ping the Redis server
- _, err := client.Ping(context.TODO()).Result()
+ _, err := client.Ping(ctx).Result()
if err != nil {
- return "", err
+ return nil, err
}
@@
- infoCmd := client.Eval(context.Background(), script, []string{})
+ infoCmd := client.Eval(ctx, script, []string{})
if infoCmd.Err() != nil {
- return "", infoCmd.Err()
+ return nil, infoCmd.Err()
}
return infoCmd.Val(), nil
}Also applies to: 196-207
🤖 Prompt for AI Agents
In pkg/js/libs/redis/redis.go around lines 181-186 (and also apply the same
change at lines 196-207), the function RunLuaScript currently returns concrete
zero values like false or "" alongside an error which produces non-nil
interface{} results; change those error return paths to return nil for the
interface{} result and the corresponding error instead (i.e., replace returns
like 'return false, err' or 'return "", err' with 'return nil, err') so callers
receive a nil value when an error occurs.
| options := &types.Options{} | ||
| err := dnsclientpool.Init(options) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dnsClient, err := dnsclientpool.Get(nil, &dnsclientpool.Configuration{}) | ||
| dnsClient, err := dnsclientpool.Get(options, &dnsclientpool.Configuration{}) | ||
| if err != nil { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid re-initializing the DNS pool and mismatching options; use a shared, injectable options instance
Creating a fresh types.Options{} and calling dnsclientpool.Init inside the DSL resolve handler can conflict with upstream/test initialization and cause the pool used by Get to diverge from the one the test initialized (different options pointer/values). This may ignore test-provided resolvers or create multiple pools/configs depending on the underlying implementation.
Recommend using a shared options instance in this package that callers/tests can set, then always pass that same instance to both Init and Get. This keeps behavior deterministic across tests and production.
Proposed change within this range:
- options := &types.Options{}
- err := dnsclientpool.Init(options)
+ // Initialize DNS client with shared options to support resolver injection from tests.
+ err := dnsclientpool.Init(dnsOptions)
if err != nil {
return nil, err
}
- dnsClient, err := dnsclientpool.Get(options, &dnsclientpool.Configuration{})
+ dnsClient, err := dnsclientpool.Get(dnsOptions, &dnsclientpool.Configuration{})
if err != nil {
return nil, err
}And add these supporting pieces outside this range in the same file:
// At package scope (near other vars)
var dnsOptions = &types.Options{}
// Exported setter to allow tests or callers to inject custom resolvers/options.
func SetDNSResolvers(resolvers []string) {
dnsOptions.InternalResolversList = resolvers
}Optionally, if you want to avoid repeated Init calls on every resolve invocation, you can guard with a sync.Once at package scope; but this is not strictly necessary if dnsclientpool.Init is idempotent.
To verify whether this is a correctness risk in your codebase (idempotency and pool selection logic), run:
Expected:
- If Init is guarded with sync.Once or otherwise idempotent, subsequent calls should be no-ops.
- Confirm whether Get resolves the pool by pointer identity or by options’ content; using a shared instance avoids both pitfalls.
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
# Inspect Init/Get implementations and any once/idempotency guards
rg -nP -C3 'package\s+dnsclientpool\b|func\s+Init\s*\(|func\s+Get\s*\(|sync\.Once|once\.Do|initialized' --type=go
Length of output: 43187
🏁 Script executed:
#!/bin/bash
# Display the DNS client pool implementation to inspect Init and Get logic
sed -n '1,120p' pkg/protocols/dns/dnsclientpool/clientpool.goLength of output: 2931
🏁 Script executed:
#!/bin/bash
# Show DSL resolve handler context including DNS initialization
sed -n '1,200p' pkg/operators/common/dsl/dsl.goLength of output: 3633
🏁 Script executed:
#!/bin/bash
# Search for types.Options struct definition
rg -nP 'type Options\s+struct' -C5 pkg/typesLength of output: 793
Use a shared, injectable types.Options instance for DNS to ensure deterministic resolver configuration across tests and production
The current DSL resolve function always creates a new &types.Options{} on each call, causing the first invocation to initialize the DNS pool with the default resolvers and ignoring any test-injected settings on subsequent calls (because dnsclientpool.Init is guarded by a global normalClient and is a no-op after the first init). To fix this, introduce a package-scoped dnsOptions and an exported setter, then pass that shared instance to both Init and Get.
Locations to update:
- pkg/operators/common/dsl/dsl.go (around lines 64–70)
- pkg/operators/common/dsl/dsl.go (add package scope and setter near top of file)
Proposed changes:
--- a/pkg/operators/common/dsl/dsl.go
+++ b/pkg/operators/common/dsl/dsl.go
@@
// package-scope shared options for dnsclientpool
+var dnsOptions = &types.Options{}
// SetDNSResolvers allows tests or callers to inject custom resolvers before the first Init.
+func SetDNSResolvers(resolvers []string) {
+ dnsOptions.InternalResolversList = resolvers
+}
func init() {
_ = dsl.AddFunction(dsl.NewWithMultipleSignatures("resolve", []string{
@@
- options := &types.Options{}
- err := dnsclientpool.Init(options)
+ // Use shared dnsOptions to support resolver injection
+ err := dnsclientpool.Init(dnsOptions)
if err != nil {
return nil, err
}
- dnsClient, err := dnsclientpool.Get(options, &dnsclientpool.Configuration{})
+ dnsClient, err := dnsclientpool.Get(dnsOptions, &dnsclientpool.Configuration{})
if err != nil {
return nil, err
}Note: dnsclientpool.Init is already idempotent (it returns early if normalClient is non-nil), so no additional sync.Once guard is required unless you prefer to avoid repeated method calls.
📝 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.
| options := &types.Options{} | |
| err := dnsclientpool.Init(options) | |
| if err != nil { | |
| return nil, err | |
| } | |
| dnsClient, err := dnsclientpool.Get(nil, &dnsclientpool.Configuration{}) | |
| dnsClient, err := dnsclientpool.Get(options, &dnsclientpool.Configuration{}) | |
| if err != nil { | |
| // Use shared dnsOptions to support resolver injection | |
| err := dnsclientpool.Init(dnsOptions) | |
| if err != nil { | |
| return nil, err | |
| } | |
| dnsClient, err := dnsclientpool.Get(dnsOptions, &dnsclientpool.Configuration{}) | |
| if err != nil { |
🤖 Prompt for AI Agents
In pkg/operators/common/dsl/dsl.go around lines 64–70, the code currently
creates a new &types.Options{} per call which prevents test-injected DNS
settings from taking effect; add a package-scoped variable (e.g. var dnsOptions
*types.Options) and an exported setter function (e.g. SetDNSOptions(o
*types.Options)) near the top of the file, then change the resolve function to
use that shared instance: use options := dnsOptions; if options == nil { options
= &types.Options{} } and pass that same options to dnsclientpool.Init and
dnsclientpool.Get so Init/Get use the injected, deterministic resolver
configuration across tests and production.
| // ErrURLDenied returns an error when a URL is denied by network policy | ||
| func ErrURLDenied(url, rule string) error { | ||
| return errkit.New(fmt.Sprintf("headless: url %v dropped by rule: %v", url, rule)).Build() | ||
| } | ||
|
|
||
| // ErrHostDenied returns an error when a host is denied by network policy | ||
| func ErrHostDenied(host string) error { | ||
| return errkit.New(fmt.Sprintf("host %v dropped by network policy", host)).Build() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Solid move to function-based error constructors; consider quoting for clarity and verify external usages
Switching from sentinel variables with Msgf to functions returning errors built via errkit is clean. As a tiny readability improvement, quote dynamic values.
Apply:
-func ErrURLDenied(url, rule string) error {
- return errkit.New(fmt.Sprintf("headless: url %v dropped by rule: %v", url, rule)).Build()
-}
+func ErrURLDenied(url, rule string) error {
+ return errkit.New(fmt.Sprintf("headless: url %q dropped by rule: %q", url, rule)).Build()
+}
-func ErrHostDenied(host string) error {
- return errkit.New(fmt.Sprintf("host %v dropped by network policy", host)).Build()
-}
+func ErrHostDenied(host string) error {
+ return errkit.New(fmt.Sprintf("host %q dropped by network policy", host)).Build()
+}Also, since the exported API changed from variables to functions, please ensure no remaining usages rely on the old .Msgf helpers or on errors.Is/As with old sentinels. Run:
🏁 Script executed:
#!/bin/bash
# Find legacy formatting usage
rg -nP 'Err(URL|Host)Denied\.Msgf\(' -C2
# Find potential identity checks against old sentinels
rg -nP 'errors\.(Is|As)\(.*Err(URL|Host)Denied' -C3Length of output: 370
Quote dynamic values and update docs example
To improve clarity and align examples with the new function-based constructors:
• In pkg/protocols/common/protocolstate/headless.go, switch from %v to %q:
func ErrURLDenied(url, rule string) error {
- return errkit.New(fmt.Sprintf("headless: url %v dropped by rule: %v", url, rule)).Build()
+ return errkit.New(fmt.Sprintf("headless: url %q dropped by rule: %q", url, rule)).Build()
}
func ErrHostDenied(host string) error {
- return errkit.New(fmt.Sprintf("host %v dropped by network policy", host)).Build()
+ return errkit.New(fmt.Sprintf("host %q dropped by network policy", host)).Build()
}• In pkg/js/CONTRIBUTE.md, update the example to call the new constructor rather than .Msgf:
- return false, protocolstate.ErrHostDenied.Msgf(host)
+ return false, protocolstate.ErrHostDenied(host)No remaining errors.Is/errors.As checks against the old sentinels were found. Ensure any other documentation or examples are updated similarly.
📝 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.
| // ErrURLDenied returns an error when a URL is denied by network policy | |
| func ErrURLDenied(url, rule string) error { | |
| return errkit.New(fmt.Sprintf("headless: url %v dropped by rule: %v", url, rule)).Build() | |
| } | |
| // ErrHostDenied returns an error when a host is denied by network policy | |
| func ErrHostDenied(host string) error { | |
| return errkit.New(fmt.Sprintf("host %v dropped by network policy", host)).Build() | |
| } | |
| // ErrURLDenied returns an error when a URL is denied by network policy | |
| func ErrURLDenied(url, rule string) error { | |
| return errkit.New(fmt.Sprintf("headless: url %q dropped by rule: %q", url, rule)).Build() | |
| } | |
| // ErrHostDenied returns an error when a host is denied by network policy | |
| func ErrHostDenied(host string) error { | |
| return errkit.New(fmt.Sprintf("host %q dropped by network policy", host)).Build() | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/common/protocolstate/headless.go around lines 21 to 29, the
formatted error messages use %v for dynamic values; change the formatting to use
%q so the URL and host are quoted in the error strings (e.g., "headless: url %q
dropped by rule: %q" and "host %q dropped by network policy"). Also update the
docs example in pkg/js/CONTRIBUTE.md to call the new function-based error
constructors (ErrURLDenied/ErrHostDenied) instead of using the old .Msgf pattern
so examples reflect the current API.
| return nil, errkit.New(fmt.Sprintf("http: failed to parse url %v while creating http request", reqData)).Build() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve cause: use Append instead of embedding err string
Use Append to carry the underlying parse error.
- return nil, errkit.New(fmt.Sprintf("http: failed to parse url %v while creating http request", reqData)).Build()
+ return nil, errkit.Append(errkit.New(fmt.Sprintf("http: failed to parse url %v while creating http request", reqData)), err)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/protocols/http/build_request.go around lines 228-229, the error
construction currently embeds the parse error string into the message; replace
that with errkit.Append to preserve the original error as the cause. Change the
New(fmt.Sprintf(... with err string)) pattern to create the contextual message
with errkit.New("http: failed to parse url while creating http request") and
then call .Append(err) before .Build(), keeping the same descriptive text and
returning the built error.
| if err != nil { | ||
| return nil, errorutil.NewWithErr(err).Msgf("failed to parse %v in self contained request", data).WithTag("self-contained") | ||
| return nil, errkit.New(fmt.Sprintf("self-contained: failed to parse %v in self contained request: %s", data, err)).Build() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve cause: Append underlying parse error for self-contained URL
Don’t stringify the cause; keep it chained.
- return nil, errkit.New(fmt.Sprintf("self-contained: failed to parse %v in self contained request: %s", data, err)).Build()
+ return nil, errkit.Append(errkit.New(fmt.Sprintf("self-contained: failed to parse %v in self contained request", data)), err)📝 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.
| if err != nil { | |
| return nil, errorutil.NewWithErr(err).Msgf("failed to parse %v in self contained request", data).WithTag("self-contained") | |
| return nil, errkit.New(fmt.Sprintf("self-contained: failed to parse %v in self contained request: %s", data, err)).Build() | |
| } | |
| if err != nil { | |
| return nil, errkit.Append( | |
| errkit.New(fmt.Sprintf("self-contained: failed to parse %v in self contained request", data)), | |
| err, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/http/build_request.go around lines 309-311, the code
stringifies the underlying parse error into the message; instead, create the
errkit error and attach the original err as its cause (e.g.,
errkit.New("self-contained: failed to parse %v in self contained request",
data).WithCause(err).Build()) so the original error is preserved in the error
chain rather than being formatted into the message.
| result, err := request.options.JsCompiler.ExecuteWithOptions(initCompiled, args, opts) | ||
| if err != nil { | ||
| return errorutil.NewWithTag(request.TemplateID, "could not execute pre-condition: %s", err) | ||
| return errkit.New(fmt.Sprintf("%s: could not execute pre-condition: %s", request.TemplateID, err)).Build() | ||
| } |
There was a problem hiding this comment.
Wrong error message: init code execution labeled as pre-condition
This block executes Init, not PreCondition. The error text is misleading.
Apply this diff to correct the message and preserve the cause via Append:
- result, err := request.options.JsCompiler.ExecuteWithOptions(initCompiled, args, opts)
- if err != nil {
- return errkit.New(fmt.Sprintf("%s: could not execute pre-condition: %s", request.TemplateID, err)).Build()
- }
+ result, err := request.options.JsCompiler.ExecuteWithOptions(initCompiled, args, opts)
+ if err != nil {
+ return errkit.Append(errkit.New(fmt.Sprintf("%s: could not execute init code", request.TemplateID)), err)
+ }📝 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.
| result, err := request.options.JsCompiler.ExecuteWithOptions(initCompiled, args, opts) | |
| if err != nil { | |
| return errorutil.NewWithTag(request.TemplateID, "could not execute pre-condition: %s", err) | |
| return errkit.New(fmt.Sprintf("%s: could not execute pre-condition: %s", request.TemplateID, err)).Build() | |
| } | |
| result, err := request.options.JsCompiler.ExecuteWithOptions(initCompiled, args, opts) | |
| if err != nil { | |
| return errkit.Append( | |
| errkit.New(fmt.Sprintf("%s: could not execute init code", request.TemplateID)), | |
| err, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/javascript/js.go around lines 223-226, the error message
incorrectly refers to "pre-condition" even though this block executes Init;
change the message to indicate Init (e.g., "could not execute init") and
preserve the original error as the cause by appending it to the errkit before
building the final error (use errkit's Append with the original err and then
Build), so the returned errkit contains the corrected message and the original
error as the cause.
| return errkit.New(fmt.Sprintf("%s: port %v is not in valid range", request.TemplateID, portInt)).Build() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use options.TemplateID and avoid .Build() for consistency (confirm errkit API)
request.TemplateIDis likely empty at this point;options.TemplateIDis set. Preferoptions.TemplateIDfor better diagnostics.- The mixed usage of
errkit.New(...).Build()here vs plainerrkit.New(...)elsewhere is inconsistent. IfNewalready returns anerror,.Build()is likely unnecessary.
Run this script to confirm idiomatic errkit usage in the repo and whether .Build() is expected after New:
If .Build() is not a common pattern after New, apply:
- return errkit.New(fmt.Sprintf("%s: port %v is not in valid range", request.TemplateID, portInt)).Build()
+ return errkit.New(fmt.Sprintf("%s: port %d is not in valid range", options.TemplateID, portInt))🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Occurrences of errkit.New(...).Build(...):"
rg -nP -C2 'errkit\.New\([^)]*\)\.Build\('
echo
echo "General occurrences of errkit.New(...):"
rg -nP -C2 'errkit\.New\('
echo
echo "Append usage patterns:"
rg -nP -C2 'errkit\.Append\('Length of output: 134970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Occurrences of request.TemplateID in pkg/protocols/network/network.go:"
rg -nnC2 'request\.TemplateID' pkg/protocols/network/network.go || true
echo
echo "Occurrences of options.TemplateID in pkg/protocols/network/network.go:"
rg -nnC2 'options\.TemplateID' pkg/protocols/network/network.go || trueLength of output: 1321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Context around port parsing/validation in network.go (lines 190–210):"
sed -n '190,210p' pkg/protocols/network/network.go || true
echo
echo "Occurrences of options.TemplateID in network.go:"
rg -n -C3 'options\.TemplateID' pkg/protocols/network/network.go || trueLength of output: 1381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All errkit.New usages in network.go around port validation:"
rg -n 'errkit\.New' -C2 pkg/protocols/network/network.go || trueLength of output: 613
Use options.TemplateID in port‐range error
The template ID for diagnostics should come from options.TemplateID (which is always set) rather than request.TemplateID. Also keep .Build() here to match the file’s existing pattern for simple errkit.New(...).Build() errors.
Locations to update:
- pkg/protocols/network/network.go:203
Diff:
- return errkit.New(fmt.Sprintf("%s: port %v is not in valid range", request.TemplateID, portInt)).Build()
+ return errkit.New(fmt.Sprintf("%s: port %v is not in valid range", options.TemplateID, portInt)).Build()📝 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.
| return errkit.New(fmt.Sprintf("%s: port %v is not in valid range", request.TemplateID, portInt)).Build() | |
| } | |
| return errkit.New(fmt.Sprintf("%s: port %v is not in valid range", options.TemplateID, portInt)).Build() | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/network/network.go around lines 203-204, the error message uses
request.TemplateID but should use options.TemplateID (which is always set);
change the fmt.Sprintf to include options.TemplateID instead of
request.TemplateID and retain the .Build() call so the line remains
errkit.New(fmt.Sprintf("%s: port %v is not in valid range", options.TemplateID,
portInt)).Build().
| tlsxService, err := tlsx.New(tlsxOptions) | ||
| if err != nil { | ||
| return errorutil.NewWithTag(request.TemplateID, "could not create tlsx service") | ||
| return errkit.New(fmt.Sprintf("%s: could not create tlsx service", request.TemplateID)).Build() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t drop the underlying error from tlsx.New
Switch to Append to keep the original cause; Build here hides crucial diagnostics.
- if err != nil {
- return errkit.New(fmt.Sprintf("%s: could not create tlsx service", request.TemplateID)).Build()
- }
+ if err != nil {
+ return errkit.Append(errkit.New(fmt.Sprintf("%s: could not create tlsx service", request.TemplateID)), err)
+ }📝 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.
| tlsxService, err := tlsx.New(tlsxOptions) | |
| if err != nil { | |
| return errorutil.NewWithTag(request.TemplateID, "could not create tlsx service") | |
| return errkit.New(fmt.Sprintf("%s: could not create tlsx service", request.TemplateID)).Build() | |
| } | |
| tlsxService, err := tlsx.New(tlsxOptions) | |
| if err != nil { | |
| return errkit.Append(errkit.New(fmt.Sprintf("%s: could not create tlsx service", request.TemplateID)), err) | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/ssl/ssl.go around lines 170 to 173, the error returned when
tlsx.New fails is built without preserving the original error; change the err
construction to include the underlying err (for example use
errkit.New(fmt.Sprintf("%s: could not create tlsx service",
request.TemplateID)).Append(err).Build() or the equivalent wrap/append method
provided by errkit) so the original cause is retained in the returned error.
| return nil, errkit.Append(errkit.New(fmt.Sprintf("failed to parse %s", template.Path)), err) | ||
| } |
There was a problem hiding this comment.
Bug: error message uses template.Path before it’s set (loses file context)
At this point template.Path is empty. The resulting error becomes “failed to parse ” with no path, hurting debuggability. Use options.TemplatePath (copied from srcOptions) instead.
Apply this diff:
- return nil, errkit.Append(errkit.New(fmt.Sprintf("failed to parse %s", template.Path)), err)
+ return nil, errkit.Append(errkit.New(fmt.Sprintf("failed to parse %s", options.TemplatePath)), err)📝 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.
| return nil, errkit.Append(errkit.New(fmt.Sprintf("failed to parse %s", template.Path)), err) | |
| } | |
| return nil, errkit.Append(errkit.New(fmt.Sprintf("failed to parse %s", options.TemplatePath)), err) | |
| } |
🤖 Prompt for AI Agents
In pkg/templates/compile.go around lines 483-484, the error message currently
formats using template.Path which is still empty at this point; replace the use
of template.Path with options.TemplatePath (the value copied from srcOptions) so
the error reads the actual template path — update the fmt.Sprintf call to
include options.TemplatePath and ensure options is the variable in scope that
holds the copied srcOptions.TemplatePath.
| stats.Increment(SyntaxWarningStats) | ||
| checkOpenFileError(validationWarning) | ||
| return ret, ErrCouldNotLoadTemplate.Msgf(templatePath, validationWarning) | ||
| return ret, ErrCouldNotLoadTemplate(templatePath, validationWarning.Error()) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use the dedicated warning helper instead of “Could not load …”
Here ret can be true (template matched) and we’re emitting a warning. Using “Could not load” is misleading. Prefer the dedicated ErrLoadedWithWarnings for clarity.
Apply this diff:
- return ret, ErrCouldNotLoadTemplate(templatePath, validationWarning.Error())
+ return ret, ErrLoadedWithWarnings(templatePath, validationWarning.Error())Optionally confirm current usage of the warning helper and avoid regressions:
🏁 Script executed:
#!/bin/bash
# Inspect current call sites for the warning helper
rg -n -C2 -g '!**/vendor/**' '\bErrLoadedWithWarnings\s*\('
# Also inspect how LoadTemplate's warning path is handled elsewhere
rg -n -C3 -g '!**/vendor/**' 'LoadTemplate\('Length of output: 7802
Use the dedicated warning helper for syntax warnings
Here, ret may be true (template loaded) despite a syntax warning, so “Could not load…” is misleading. Swap in the ErrLoadedWithWarnings helper for clarity.
• File: pkg/templates/parser.go
• Lines: 111–114
- stats.Increment(SyntaxWarningStats)
- checkOpenFileError(validationWarning)
- return ret, ErrCouldNotLoadTemplate(templatePath, validationWarning.Error())
+ stats.Increment(SyntaxWarningStats)
+ checkOpenFileError(validationWarning)
+ return ret, ErrLoadedWithWarnings(templatePath, validationWarning.Error())📝 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.
| stats.Increment(SyntaxWarningStats) | |
| checkOpenFileError(validationWarning) | |
| return ret, ErrCouldNotLoadTemplate.Msgf(templatePath, validationWarning) | |
| return ret, ErrCouldNotLoadTemplate(templatePath, validationWarning.Error()) | |
| } | |
| stats.Increment(SyntaxWarningStats) | |
| checkOpenFileError(validationWarning) | |
| return ret, ErrLoadedWithWarnings(templatePath, validationWarning.Error()) | |
| } |
🤖 Prompt for AI Agents
In pkg/templates/parser.go around lines 111 to 114, the code logs a syntax
warning but then returns ErrCouldNotLoadTemplate which is misleading when the
template actually loaded; replace the error helper with the dedicated one for
loaded-with-warnings by returning
ErrLoadedWithWarnings(validationWarning.Error()) (keep the stats.Increment and
checkOpenFileError calls and still return ret) so the return correctly reflects
a successful load with warnings.
|
failing test - #6398 |
Proposed changes
Related utils update - projectdiscovery/utils#666
Checklist
Summary by CodeRabbit
Chores
Refactor
Tests