Quiet unresolved-variable skips and summarize at scan end - #7572
Open
Mzack9999 wants to merge 1 commit into
Open
Quiet unresolved-variable skips and summarize at scan end#7572Mzack9999 wants to merge 1 commit into
Mzack9999 wants to merge 1 commit into
Conversation
Contributor
WalkthroughChangesUnresolved Variable Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RequestProtocol
participant unresolvedvars
participant Progress
participant Runner
RequestProtocol->>unresolvedvars: Detect unresolved variable error
unresolvedvars->>Progress: Increment skipped count
RequestProtocol-->>Runner: Skip request
Runner->>Progress: Read skipped count
Runner-->>Runner: Log aggregate summary
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/progress/progress.go`:
- Around line 35-39: The exported Progress interface must remain
source-compatible for existing custom implementations. Remove
IncrementSkippedUnresolved and SkippedUnresolved from Progress, and introduce a
separate optional capability interface for unresolved-skip tracking; update
callers to use a type assertion before invoking those methods.
In `@pkg/protocols/common/unresolvedvars/unresolvedvars.go`:
- Around line 11-19: Replace the message-substring check in unresolvedvars.Is
with identity-based classification using a sentinel or typed
unresolved-variables error and errors.Is/errors.As. Update the producers and
wrapped-error paths to preserve that identity, while keeping nil and unrelated
errors returning false. Add a regression test proving an unrelated error
containing “unresolved variables” is not classified as a skip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa1910cf-60bd-48ea-a3d0-d43943a2ce00
📒 Files selected for processing (11)
cmd/nuclei/main.gointernal/runner/runner.gointernal/tests/testutils/testutils.gopkg/progress/progress.gopkg/progress/progress_unresolved_test.gopkg/protocols/common/unresolvedvars/unresolvedvars.gopkg/protocols/common/unresolvedvars/unresolvedvars_test.gopkg/protocols/dns/request.gopkg/protocols/http/request.gopkg/protocols/network/request.gopkg/types/types.go
Comment on lines
+35
to
+39
| // IncrementSkippedUnresolved increments the count of requests skipped because | ||
| // template variables could not be resolved. | ||
| IncrementSkippedUnresolved(count int64) | ||
| // SkippedUnresolved returns how many requests were skipped for unresolved variables. | ||
| SkippedUnresolved() uint64 |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)progress\.go$|pkg/progress|progress' | head -100
echo "== inspect progress.go =="
if [ -f pkg/progress/progress.go ]; then
wc -l pkg/progress/progress.go
cat -n pkg/progress/progress.go
fi
echo "== search Progress usages =="
rg -n "type Progress|interface \{\n|Progress\)|Progress\." . | head -200
echo "== go fmt/vet dry checks if tools present =="
if command -v go >/dev/null 2>&1; then
go fmt ./pkg/progress/... || true
go vet ./pkg/progress/... || true
else
echo "go not available"
fiRepository: projectdiscovery/nuclei
Length of output: 12013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go command availability =="
if command -v go >/dev/null 2>&1; then
go version
go fmt ./pkg/progress/...
output="$(go vet ./pkg/progress/... 2>&1)" || true
echo "$output"
else
echo "go not available"
fi
echo "== Progress/progress related references =="
rg -n --multiline 'type Progress interface|func New.*Progress|Progress\)|\.New.*Progress|IncrementSkippedUnresolved|SkippedUnresolved|var _ Progress' . | head -300Repository: projectdiscovery/nuclei
Length of output: 3298
Keep the public Progress contract source-compatible.
Progress is exported and progress.Progress is exposed through library paths, so adding IncrementSkippedUnresolved and SkippedUnresolved will make existing SDK/custom implementations fail to compile. Use an optional capability interface/type assertion, or document this as a breaking API change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/progress/progress.go` around lines 35 - 39, The exported Progress
interface must remain source-compatible for existing custom implementations.
Remove IncrementSkippedUnresolved and SkippedUnresolved from Progress, and
introduce a separate optional capability interface for unresolved-skip tracking;
update callers to use a type assertion before invoking those methods.
Comment on lines
+11
to
+19
| const errSubstring = "unresolved variables" | ||
|
|
||
| // Is reports whether err is (or wraps) an unresolved-variables skip. | ||
| func Is(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
| return strings.Contains(strings.ToLower(err.Error()), errSubstring) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not classify errors by a free-form message substring.
Is turns any error whose text contains "unresolved variables" into a successful skip. Because callers suppress that error and increment only the skip metric, unrelated wrapped errors can be hidden and misreported. Use a sentinel or typed error with errors.Is/errors.As, and add a false-positive regression case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/protocols/common/unresolvedvars/unresolvedvars.go` around lines 11 - 19,
Replace the message-substring check in unresolvedvars.Is with identity-based
classification using a sentinel or typed unresolved-variables error and
errors.Is/errors.As. Update the producers and wrapped-error paths to preserve
that identity, while keeping nil and unrelated errors returning false. Add a
regression test proving an unrelated error containing “unresolved variables” is
not classified as a skip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mutes per-request unresolved-variable warnings by default, counts skips, and prints one end-of-scan summary. Use -log-unresolved to restore per-request details.
Closes #3386
Summary by CodeRabbit
New Features
-log-unresolved(-lu) to optionally display warnings for requests skipped بسبب unresolved template variables.Bug Fixes