feat: write resume file specified by flag#6616
feat: write resume file specified by flag#6616Mzack9999 merged 2 commits intoprojectdiscovery:devfrom
Conversation
WalkthroughThe resume file handling in nuclei's main entry point is refactored. The Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/nuclei/main.go(2 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). (1)
- GitHub Check: Lint
🔇 Additional comments (2)
cmd/nuclei/main.go (2)
261-261: Add a changelog entry documenting this breaking change.The updated help text accurately describes the new behavior. However, web search confirms there is no dedicated migration guide for breaking changes—Project Discovery documents changes in GitHub release notes and CLI/docs. For a breaking change to the resume flag behavior, add an entry to the project's changelog (typically in
CHANGELOG.mdor GitHub release notes) explicitly documenting:
- The old behavior (resume via
resume.cfg)- The new behavior (resume from and save to specified file)
- The side effect (clustering disabled)
- Migration guidance for affected users
198-201: Remove directory creation suggestion—it's already implemented inSaveResumeConfig().The code at lines 198-201 correctly implements the feature. The concern about missing directory creation is invalid: the
SaveResumeConfigmethod already handles this atinternal/runner/runner.golines 918-920 withos.MkdirAll(). Path validation for resume file loading is also handled viaShouldLoadResume(), which checks file existence before attempting to read. The implementation properly integrates with existing resume infrastructure across the codebase.Likely an incorrect or invalid review comment.
cmd/nuclei/main.go
Outdated
| resumeFileName := options.Resume | ||
| if resumeFileName == "" { | ||
| resumeFileName = types.DefaultResumeFilePath() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add tests for the new resume file behavior.
As noted in the PR objectives, tests proving the feature are not yet completed. Given this is a breaking change, comprehensive testing is essential to ensure:
- Resume file is created at the user-specified path when
-resumeflag is provided. - Resume file falls back to default path when flag is not provided.
- Error handling works correctly for invalid paths.
- Successful cleanup removes the correct resume file.
🤖 Prompt for AI Agents
In cmd/nuclei/main.go around lines 198 to 201, tests are missing for the new
resume file behavior; add unit/integration tests that (1) invoke the CLI or call
the underlying resume-file logic with the -resume flag set and assert the resume
file is created at the provided path, (2) call the logic without the flag and
assert the resume file is created at types.DefaultResumeFilePath(), (3) pass an
invalid/unwritable path and assert the code returns an error (and that no file
is created), and (4) simulate successful cleanup and assert the correct resume
file is removed; ensure tests create and teardown temporary directories/files to
avoid side effects and run reliably in CI.
Mzack9999
left a comment
There was a problem hiding this comment.
lgtm! Now it should behave like nmap resume
|
GH actions errors eeems unrelated to this PR - merging |
Proposed changes
-resumewill use the provided flag as path to save the resume config, instead of only used as loading. Possibly a breaking changes.Checklist
Summary by CodeRabbit
Bug Fixes
Documentation