Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughIntroduces a ChangesTemplate Capability System and Stats Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/templates/stats.go`:
- Around line 36-38: In the ExcludedSelfContainedTemplateStats description
field, replace the short flag reference `-esc` with the appropriate long flag
name to maintain consistency with the long-flag messaging standard and ensure
the user-facing message aligns with the actual capability metadata for
self-contained template execution.
🪄 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
Run ID: 0f3a3751-2b51-4aa3-b4c9-6d2a2a2dfc98
⛔ Files ignored due to path filters (3)
pkg/templates/tests/workflow-capability-gates.yamlis excluded by!**/*.yamlpkg/templates/tests/workflow-file-template.yamlis excluded by!**/*.yamlpkg/templates/tests/workflow-self-contained-template.yamlis excluded by!**/*.yaml
📒 Files selected for processing (13)
internal/runner/runner.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_test.gopkg/templates/capability.gopkg/templates/compile.gopkg/templates/compile_test.gopkg/templates/parser.gopkg/templates/parser_stats.gopkg/templates/stats.gopkg/templates/templates.gopkg/templates/templates_test.gopkg/templates/templates_utils.gopkg/templates/workflows.go
d566585 to
a0e4eca
Compare
Move template execution requirements into a shared capability table that maps each capability to its flag, stats key, and template predicate. Use the shared model from template loading, workflow parsing, request compilation, and runner stats display so file, self-contained, headless, code, DAST, and global matcher gates stay in sync. Signed-off-by: Dwi Siswanto <git@dw1.io>
a0e4eca to
ed759b7
Compare
|
@coderabbitai create an issue for this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue create \
--repo "projectdiscovery/nuclei" \
--title "Follow-up: Address vocabulary inconsistencies, user-facing message quality, and capability flag naming" \
--body "## Overview
This issue tracks follow-up improvements identified during the capability gates refactor in `#7489`. The refactor centralized capability definitions into a shared table, but several broader code quality and UX concerns were noted that are out of scope for that PR.
---
## 1. Vocabulary inconsistency
Terms like **\"signed\"**, **\"tampered\"**, **\"verified\"**, **\"skipped\"**, and **\"excluded\"** are used interchangeably across the codebase despite having distinct meanings. This contributes to confusion in code, documentation, and observable behavior.
**Goal:** Establish clear, consistent definitions for each term and apply them uniformly across the codebase, docs, and user-facing output.
---
## 2. User-facing message quality
User-facing messages (warnings, errors, skip notices) lack consistency in wording, tone, and spelling across similar scenarios. Messages appear ad-hoc rather than intentional.
**Goal:** Audit and standardize user-facing messages for consistency in tone, spelling, and format.
---
## 3. Flag naming in user-facing messages
User-facing output currently uses short CLI flags (e.g., \`-c\`) in error messages and skip notices. Short flags are efficient for command-line typing but create confusion in error messages and documentation.
**Goal:** Replace short flags with full long flag names (e.g., \`--code\`) in all user-facing messages to provide clear, actionable guidance.
---
## 4. Single source of truth for capability flag names
Capability flag names are hardcoded in multiple places (messages, stats, checks). This increases the risk of drift when flags are renamed or refactored.
**Goal:** Ensure capability definitions (introduced in `#7489`) are the single authoritative source for associated flag names, so that renaming a flag only requires a change in one place.
---
## References
- PR `#7489` — Refactor template capability gates into a centralized capability table
/cc `@dwisiswant0`" \
--assignee "dwisiswant0"Length of output: 2394
The issue has been assigned to you and links back to this PR. |
nuclei 3.10.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## What's Changed ### 🎉 New Features * Added per-host HTTP client pooling by @Mzack9999 in projectdiscovery/nuclei#7301 ### 🐞 Bug Fixes * Fixed handling in hosterrorscache to automatically skip hosts that consistently time out by @knakul853 in projectdiscovery/nuclei#7455 * Fixed preservation of explicit target port in network templates (fixes #7323) by @XananasX7 in projectdiscovery/nuclei#7465 * Fixed connection reuse and improved port pre-flight handling by @Mzack9999 in projectdiscovery/nuclei#6715 * Fixed code template signature validation before DAST loading by @dogancanbakir in projectdiscovery/nuclei#7472 * Fixed gating of MySQL allowAllFiles option to require `-lfa` flag by @dogancanbakir in projectdiscovery/nuclei#7473 * Fixed ASCII-section regex to properly escape literal `.` by @snicket2100 in projectdiscovery/nuclei#7476 * Fixed recording of decoded bytes for debug dumps by @snicket2100 in projectdiscovery/nuclei#7478 * Fixed proper escaping of dbname in lib/pq URLs by @dwisiswant0 in projectdiscovery/nuclei#7479 * Fixed network policy enforcement prior to LDAP dialing by @dwisiswant0 in projectdiscovery/nuclei#7494 * Fixed normalization and rejection of trace file DSN options in Oracle by @dwisiswant0 in projectdiscovery/nuclei#7480 * Fixed proper escaping of MSSQL database names in connection URLs by @dwisiswant0 in projectdiscovery/nuclei#7481 * Fixed krbforge to reject unsandboxed ccache writes by @dwisiswant0 in projectdiscovery/nuclei#7482 * Fixed rejection of request-condition(s) during fuzzing by @dwisiswant0 in projectdiscovery/nuclei#7466 * Fixed: YAML now correctly rejects recursive include chains by @dwisiswant0 in projectdiscovery/nuclei#7492 * Fixed resource leaks by @Mzack9999 in projectdiscovery/nuclei#7502 ### Other Changes * Updated govaluate dependency to prevent slice-bounds panic on invalid UTF-8 input by @XananasX7 in projectdiscovery/nuclei#7464 * Updated goja dependency by @Mzack9999 in projectdiscovery/nuclei#7467 * Updated dependencies to remove unused packages by @dwisiswant0 in projectdiscovery/nuclei#7457 * Refactored templates to centralize opt-in capability gating by @dwisiswant0 in projectdiscovery/nuclei#7489 * Added fuzzing parser harnesses for raw requests and templates by @dwisiswant0 in projectdiscovery/nuclei#7459 * Refactored template rendering boundary by @dwisiswant0 in projectdiscovery/nuclei#7499 ## New Contributors * @XananasX7 made their first contribution in projectdiscovery/nuclei#7465 * @snicket2100 made their first contribution in projectdiscovery/nuclei#7476 **Full Changelog**: https://github.com/projectdiscovery/nuclei/compare/v3.9.0...v3.10.0</pre> <p>View the full release notes at <a href="https://github.com/projectdiscovery/nuclei/releases/tag/v3.10.0">https://github.com/projectdiscovery/nuclei/releases/tag/v3.10.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!13187
Proposed changes
Move template execution requirements into a shared
capability table that maps each capability to its
flag, stats key, and template predicate. Use the
shared model from template loading, workflow
parsing, request compilation, and runner stats
display so file, self-contained, headless, code,
DAST, and global matcher gates stay in sync.
Prev. unverified code templates were being counted in overlapping and/or inconsistent ways. Sometimes the top-level loader would increment both the generic unverified counter and the code-template counter for the same skip, and workflow subtemplates did their own thing. The numbers were basically meaningless. Here, that the counters are mutually exclusive now, they actually mean something clear, much easier to reason about.
A few things are worth calling out when I'm working on this:
We have got a vocab mess. The actual terms we use in the code & docs like signed vs. tampered vs. verified, skipped vs. excluded, and similar labels. These words are not interchangeable, but we have been treating them like they are. That's how we end up with confusing stats, contradictory comments, and weird behavior down the line. We should probably just write down clear definitions once (a tiny
GLOSSARY dot mdwould do it) and then actually stick to them.The user-facing messages are a separate concern. A lot of them feel like they were added randomly over time, slightly different wording, tone, and even spelling for basically the same situation, making the whole thing feel sloppy, even when the logic underneath is solid. We should clean those up and make them consistent, calm, and predictable.
Hence, anything shown to users should use the full, long flag name instead of short flags. I mean they are great for typing a command, but they are terrible for doc or error messages. If the error tells user to do something, it should spell out exactly what they need to do. Users be like "wtf is -duc? -esc?" and then they have to go look it up, which is a bad experience. If it says "you need to enable [INSERT SOME LONG FLAG NAME]" then they know exactly what to do.
I also don't think we should twist ourselves into knots to keep compatibility for internal names that are not part of official lib SDK-facing API. If no external user is supposed to be consuming it, hanging onto the old name just weighs the code down. Compatibility should protect real users, not preserve technical debt forever.
And, currently, [I'm still not sure about this but, for the shake of future-proofing,] a capability needs a certain flag, the capability definition should be the single source of truth for that flag name. So hardcoding the same flag string in messages, stats, checks, everywhere else is just begging for things to get out of sync. The next time we rename a flag or add a new type, we shouldn't have to go on a
grephunt.Proof
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests