fix: replace panic with proper error handling in catalog loader#7023
Open
Pitrat-wav wants to merge 2 commits intoprojectdiscovery:devfrom
Open
fix: replace panic with proper error handling in catalog loader#7023Pitrat-wav wants to merge 2 commits intoprojectdiscovery:devfrom
Pitrat-wav wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
- Change Load() and LoadTemplates() to return errors instead of panicking - Handle errors properly in runner, SDK, and automaticscan packages - Fix compilation error in lazy.go (variable redeclaration) - Update benchmark tests to handle new error return values Fixes: projectdiscovery#6674 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Neo - PR Security ReviewNo security issues found Highlights
Comment |
Contributor
WalkthroughThe pull request adds comprehensive error handling to template loading operations across the codebase. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
♻️ Duplicate comments (2)
REQUIREMENTS.md (1)
1-14: Same concern asSTATE.md— this project-management artifact should not be committed.See the comment on
STATE.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@REQUIREMENTS.md` around lines 1 - 14, REQUIREMENTS.md is a project-management artifact that shouldn't be committed (same issue as STATE.md); remove it from the PR by deleting or moving it out of source control and update the commit set (e.g., remove the file from the branch or add it to .gitignore/private docs), then amend/rebase or create a new commit that removes REQUIREMENTS.md (and ensure STATE.md is handled the same way); confirm the PR no longer contains REQUIREMENTS.md and run a quick build/tests to ensure no code references the file remain.task.md (1)
1-12: Same concern asSTATE.md— this project-management artifact should not be committed.See the comment on
STATE.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@task.md` around lines 1 - 12, This project-management artifact (task.md) should not be committed; remove task.md from the commit (or delete it from the repo), revert the addition if already committed, and add the filenames like task.md and STATE.md to the repository ignore list (e.g., .gitignore or .git/info/exclude) so they remain local only; ensure any CI/PR contents no longer reference task.md and amend the PR to remove this file before final submission.
🧹 Nitpick comments (2)
cmd/integration-test/library.go (1)
135-137: LGTM — error fromstore.Load()is now properly propagated.Minor style note: other error returns in this function use
errors.Wrap(e.g., lines 100 and 133), but this one is returned raw. Consider wrapping for consistency:♻️ Optional: wrap the error for consistency
- if err := store.Load(); err != nil { - return nil, err - } + if err := store.Load(); err != nil { + return nil, errors.Wrap(err, "could not load templates") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/integration-test/library.go` around lines 135 - 137, The return of the raw error from store.Load() should be wrapped for consistency with other error returns; change the error return in the block around store.Load() to wrap the error using errors.Wrap (or the same wrapping helper used elsewhere in this file) and include context like "loading store" so it matches the style of the other returns (see the existing errors.Wrap usages near library.go around the earlier returns).pkg/catalog/loader/loader_bench_test.go (1)
74-74: Consider checking the error return in benchmarks to avoid silently benchmarking failure paths.All benchmark loops discard the error from
LoadTemplates. If the setup is ever misconfigured (e.g., missingprotocolstateinitialization — the exact scenario this PR fixes), benchmarks will silently measure error-return performance instead of actual template loading.A lightweight guard like
b.Helper(); if err != nil { b.Fatal(err) }on the first iteration would catch this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader_bench_test.go` at line 74, The benchmark loop discards the error returned by store.LoadTemplates which can hide setup failures; modify the benchmark (the function calling store.LoadTemplates) to check the error on the first iteration by calling b.Helper() and if err != nil call b.Fatal(err) (or b.Fatalf) so the benchmark fails fast instead of measuring error paths; reference the call to store.LoadTemplates / LoadTemplates and the b.Fatal/b.Helper helpers to locate where to add the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@implementation_plan.md`:
- Around line 8-12: Remove the developer-local absolute URIs from
implementation_plan.md: replace the file:///home/... entries referencing lazy.go
and automaticscan.go with neutral identifiers (just the filenames or
project-relative paths) or remove the tracking lines entirely so no local
username or tooling paths are present; ensure the modified entries refer only to
lazy.go and automaticscan.go (or a relative repo path) and do not include any
absolute local filesystem URIs, and avoid committing state/tracking files that
contain such paths.
In `@STATE.md`:
- Around line 1-15: Remove the personal project-tracking files (STATE.md,
REQUIREMENTS.md, task.md, implementation_plan.md) from the PR by deleting them
from the repository or staging them for removal; if you still need them locally,
add them to .gitignore and remove them from the index (e.g., stop tracking with
git rm --cached) so they aren't committed, update the PR to reflect the
deletions, and ensure no code references these filenames (STATE.md is shown in
the diff) before pushing the updated commit.
---
Duplicate comments:
In `@REQUIREMENTS.md`:
- Around line 1-14: REQUIREMENTS.md is a project-management artifact that
shouldn't be committed (same issue as STATE.md); remove it from the PR by
deleting or moving it out of source control and update the commit set (e.g.,
remove the file from the branch or add it to .gitignore/private docs), then
amend/rebase or create a new commit that removes REQUIREMENTS.md (and ensure
STATE.md is handled the same way); confirm the PR no longer contains
REQUIREMENTS.md and run a quick build/tests to ensure no code references the
file remain.
In `@task.md`:
- Around line 1-12: This project-management artifact (task.md) should not be
committed; remove task.md from the commit (or delete it from the repo), revert
the addition if already committed, and add the filenames like task.md and
STATE.md to the repository ignore list (e.g., .gitignore or .git/info/exclude)
so they remain local only; ensure any CI/PR contents no longer reference task.md
and amend the PR to remove this file before final submission.
---
Nitpick comments:
In `@cmd/integration-test/library.go`:
- Around line 135-137: The return of the raw error from store.Load() should be
wrapped for consistency with other error returns; change the error return in the
block around store.Load() to wrap the error using errors.Wrap (or the same
wrapping helper used elsewhere in this file) and include context like "loading
store" so it matches the style of the other returns (see the existing
errors.Wrap usages near library.go around the earlier returns).
In `@pkg/catalog/loader/loader_bench_test.go`:
- Line 74: The benchmark loop discards the error returned by store.LoadTemplates
which can hide setup failures; modify the benchmark (the function calling
store.LoadTemplates) to check the error on the first iteration by calling
b.Helper() and if err != nil call b.Fatal(err) (or b.Fatalf) so the benchmark
fails fast instead of measuring error paths; reference the call to
store.LoadTemplates / LoadTemplates and the b.Fatal/b.Helper helpers to locate
where to add the guard.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
PROJECT.mdREQUIREMENTS.mdROADMAP.mdSTATE.mdcmd/integration-test/library.goimplementation_plan.mdinternal/runner/lazy.gointernal/runner/runner.gointernal/server/nuclei_sdk.golib/multi.golib/sdk.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_bench_test.gopkg/protocols/common/automaticscan/util.gotask.md
STATE.md
Outdated
Comment on lines
1
to
15
| # State - Nuclei Panic Fix | ||
|
|
||
| ## Current Phase | ||
| Phase 1: Core Loader Modification | ||
|
|
||
| ## Status | ||
| - Project Initialized | ||
| - Vibe Kanban Sync: Done | ||
| - Codebase Mapped: Done (Found panics at lines 711, 720) | ||
|
|
||
| ## Blockers | ||
| None | ||
|
|
||
| ## Decision Log | ||
| - 2026-02-25: Decided to focus on error propagation instead of just removing panic to ensure robust error handling. |
Contributor
There was a problem hiding this comment.
Remove project-tracking files from the repository.
STATE.md, along with REQUIREMENTS.md, task.md, and implementation_plan.md, appear to be personal/AI-agent project-management artifacts rather than project documentation. They should not be committed to the repository — they add noise for other contributors and will go stale immediately.
Consider adding them to .gitignore or removing them from this PR entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@STATE.md` around lines 1 - 15, Remove the personal project-tracking files
(STATE.md, REQUIREMENTS.md, task.md, implementation_plan.md) from the PR by
deleting them from the repository or staging them for removal; if you still need
them locally, add them to .gitignore and remove them from the index (e.g., stop
tracking with git rm --cached) so they aren't committed, update the PR to
reflect the deletions, and ensure no code references these filenames (STATE.md
is shown in the diff) before pushing the updated commit.
- Remove GSD project management files from git index (keep locally) - Add error checking in benchmark loops with b.Helper() guard - Wrap raw error from store.Load() with proper context Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/catalog/loader/loader_bench_test.go (1)
240-252:⚠️ Potential issue | 🟡 MinorPre-warm call silently ignores errors.
LoadTemplatesOnlyMetadata()now returns an error per this PR. Ignoring the pre-warm error with_ =means a failure here goes undiagnosed; the benchmark will only surface the error on the first timed iteration (inside the loop), with no indication that the failure originated during warm-up. Useb.Fatalon the pre-warm call too for clearer diagnostics.🛡️ Proposed fix
- // Pre-warm the cache - _ = store.LoadTemplatesOnlyMetadata() + // Pre-warm the cache + if err := store.LoadTemplatesOnlyMetadata(); err != nil { + b.Fatalf("pre-warm failed: %s", err) + }Apply the same fix to the
WithSeverityFiltersub-benchmark (line 266).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/loader/loader_bench_test.go` around lines 240 - 252, The pre-warm call currently ignores errors by assigning `_ = store.LoadTemplatesOnlyMetadata()`: change this to call store.LoadTemplatesOnlyMetadata() and if it returns an error call b.Fatal(err) so warm-up failures are reported immediately; apply the same change to the WithSeverityFilter sub-benchmark pre-warm so both pre-warm invocations of LoadTemplatesOnlyMetadata() use b.Fatal on error (referencing the store variable and the LoadTemplatesOnlyMetadata function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/catalog/loader/loader_bench_test.go`:
- Around line 73-79: Move the b.Helper() call out of the timed benchmark loop so
it doesn't skew measurements: remove b.Helper() from inside each for b.Loop()
block and place a single b.Helper() call immediately before entering the loop
(or at the top of the sub-benchmark closure) for each benchmark that calls
store.LoadTemplates (and similar loops), ensuring only the body that should be
measured (e.g., _, err := store.LoadTemplates(...); if err != nil { b.Fatal(err)
}) remains inside for b.Loop().
---
Outside diff comments:
In `@pkg/catalog/loader/loader_bench_test.go`:
- Around line 240-252: The pre-warm call currently ignores errors by assigning
`_ = store.LoadTemplatesOnlyMetadata()`: change this to call
store.LoadTemplatesOnlyMetadata() and if it returns an error call b.Fatal(err)
so warm-up failures are reported immediately; apply the same change to the
WithSeverityFilter sub-benchmark pre-warm so both pre-warm invocations of
LoadTemplatesOnlyMetadata() use b.Fatal on error (referencing the store variable
and the LoadTemplatesOnlyMetadata function).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/integration-test/library.gopkg/catalog/loader/loader_bench_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/integration-test/library.go
Comment on lines
73
to
79
| for b.Loop() { | ||
| _ = store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory}) | ||
| b.Helper() | ||
| _, err := store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory}) | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Move b.Helper() out of the benchmark loop — it pollutes measurement.
b.Helper() should be called once at the top of the function body (or sub-benchmark closure), not inside the for b.Loop() body. Every statement between the braces of a b.Loop() loop is timed; calling b.Helper() on each iteration adds overhead that skews the benchmark results. While the effect is idempotent on the helper registration, the function call itself executes every iteration.
♻️ Proposed fix (representative example — same pattern for all sub-benchmarks)
b.Run("NoFilter", func(b *testing.B) {
+ b.Helper()
loaderCfg := loader.NewConfig(options, catalog, executerOpts)
store, err := loader.New(loaderCfg)
if err != nil {
b.Fatalf("could not create store: %s", err)
}
b.ResetTimer()
b.ReportAllocs()
for b.Loop() {
- b.Helper()
_, err := store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory})
if err != nil {
b.Fatal(err)
}
}
})Also applies to: 95-101, 117-123, 139-145, 161-167, 183-189, 207-213, 246-252, 271-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/catalog/loader/loader_bench_test.go` around lines 73 - 79, Move the
b.Helper() call out of the timed benchmark loop so it doesn't skew measurements:
remove b.Helper() from inside each for b.Loop() block and place a single
b.Helper() call immediately before entering the loop (or at the top of the
sub-benchmark closure) for each benchmark that calls store.LoadTemplates (and
similar loops), ensuring only the body that should be measured (e.g., _, err :=
store.LoadTemplates(...); if err != nil { b.Fatal(err) }) remains inside for
b.Loop().
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.
Proposed changes
This PR replaces panic statements with proper error handling in the catalog loader package and related code.
Changes
Load()andLoadTemplates()functions to return errors instead of panickingLoadTemplatesWithTagswith proper error returnsinternal/runner/lazy.go(variable redeclaration)Proof
pkg/catalog/loaderandinternal/runnerChecklist
Fixes #6674
Summary by CodeRabbit
Release Notes