-
Notifications
You must be signed in to change notification settings - Fork 3k
fix: enable all template types for template list/display #6668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enable all template types for template list/display #6668
Conversation
WalkthroughCentralizes template enablement using new Capabilities + Template.IsEnabledFor, removes runner's early universal enabling, adds many Template HasXRequest/HasWorkflows predicates and fuzzing accessors, and introduces pre-append duplicate detection in the loader. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner
participant Loader
participant Template
participant Store
Runner->>Loader: create loader with executor options
Loader->>Loader: derive caps (Capabilities) and isListOrDisplay
loop each candidate template
Loader->>Template: parse/load template
alt template == nil
Loader-->>Loader: skip
else
alt isListOrDisplay
Loader-->>Loader: bypass IsEnabledFor gating
else
Loader->>Template: IsEnabledFor(caps)?
Template-->>Loader: enabled/disabled
end
alt ID already seen
Loader-->>Loader: log & skip (duplicate)
else
Loader->>Template: set Path
Loader->>Store: append to store.templates
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/runner/runner.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
internal/runner/runner.go
internal/runner/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Core scanning orchestration logic should be implemented in internal/runner
Files:
internal/runner/runner.go
🧬 Code graph analysis (1)
internal/runner/runner.go (1)
lib/config.go (4)
EnableCodeTemplates(394-400)EnableFileTemplates(427-432)EnableSelfContainedTemplates(403-408)EnableGlobalMatchersTemplates(411-416)
⏰ 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
Mzack9999
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposed minor change
17a7bbe to
da31540
Compare
0562b5e to
b0ab055
Compare
When using `-tl` or `-td`, explicitly enable all template types (`code`, `file`, `self-contained`, `global-matchers`, `headless`, `dast`) to ensure all templates are listed w/o requiring extra flags. Signed-off-by: Dwi Siswanto <[email protected]>
b0ab055 to
3d652a7
Compare
|
Drafting. I'll do more cleanup. |
|
tbh I think we've reached the stage where having a GLOSARRY would help a lot. We should probably start drafting one so everyone stays on the same page. |
| // if template list or template display is enabled, enable all templates | ||
| if options.TemplateList || options.TemplateDisplay { | ||
| options.EnableCodeTemplates = true | ||
| options.EnableFileTemplates = true | ||
| options.EnableSelfContainedTemplates = true | ||
| options.EnableGlobalMatchersTemplates = true | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced in #6343. Removed, since mutating global opts in the runner could affects the entire engine state (UB).
50773d5 to
df95d9e
Compare
df95d9e to
d506b40
Compare
Signed-off-by: Dwi Siswanto <[email protected]>
8a6e9c8 to
2fc2ed6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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.go (1)
187-221: Fix error handling when remote templates list is empty but no error occurredIn
ReadTemplateFromURI, the conditionif err != nil || len(remoteTemplates) == 0can result in callingerrkit.Wrapf(nil, ...)whengetRemoteTemplatesAndWorkflowsreturns an empty slice witherr == nil. SinceWrapfreturnsnilwhen wrapping anilerror, this causes the function to return(nil, nil)instead of a proper error.Separate the error cases:
if err != nil { return nil, errkit.Wrapf(err, "Could not load template %s", uri) } if len(remoteTemplates) == 0 { return nil, fmt.Errorf("Could not load template %s: empty response", uri) }
♻️ Duplicate comments (1)
pkg/catalog/loader/loader.go (1)
684-703: Executor options alias and dialer lookup are fine; dialer panic is already tracked separatelyAliasing
typesOpts := store.config.ExecutorOptions.Optionsimproves readability, and lazily initializingExecutionIdwhen empty is reasonable.The subsequent dialer lookup:
dialers := protocolstate.GetDialersWithId(typesOpts.ExecutionId) if dialers == nil { panic("dialers with executionId " + typesOpts.ExecutionId + " not found") }preserves the existing “hard fail” behaviour when dialers are missing. This is the same concern previously discussed and tracked in issue #6674, so no change is required in this PR, but it’s worth keeping in mind for future error-handling improvements.
If you want to revisit it in a separate PR, the existing issue already contains the suggested refactor to convert this panic into a proper error return.
🧹 Nitpick comments (5)
pkg/catalog/loader/loader.go (2)
410-477: Capabilities-based gating plus dedupe inLoadTemplatesOnlyMetadatalooks correct for -tl/-tdThe new
caps := templates.Capabilities{...}plustemplate.IsEnabledFor(caps)gate, combined with theisListOrDisplayshort-circuit, achieves the right balance:
- For normal runs, templates whose requirements (headless/code/DAST/self-contained/file) aren’t satisfied by options are filtered out early at metadata time.
- For template list/display (
TemplateListorTemplateDisplay),isListOrDisplaydisables this gating so all template types are included regardless of flags, matching the PR objective.- Pre-append de‑dupe via
loadedTemplateIDsand settingtemplate.Pathbefore appending keepsstore.templatesclean and deterministic.This aligns the metadata-only path with the new
Requirements/Capabilitiesmodel and should avoid the previous under-counting for-tl/-td+ filters.As a follow-up (not for this PR), you might consider reusing
IsEnabledForin the mainLoadTemplatesWithTagspath too, to reduce the chance of drift between metadata and full-parse gating.
751-808: Gating chain for unsigned/self-contained/file/headless/code/DAST templates is coherent with new helpersThe updated gating logic in
LoadTemplatesWithTags:
- Uses
typesOptsconsistently (avoid repeatedstore.config.ExecutorOptions.Options).- Applies unsigned, self-contained, and file gating before protocol/DAST logic.
- For DAST mode (
typesOpts.DAST && store.ID() != AuthStoreId), loads only fuzzable or global matchers templates, with a headless flag check.- Outside DAST mode, enforces:
- Headless flag for any headless requests (
parsed.HasHeadlessRequest()),EnableCodeTemplatesflag for code templates,- Code-template verification (skipping unsigned code templates),
- Excluding DAST-only templates unless
-dastis set.All of this now relies on the predicate helpers (
HasHeadlessRequest,HasCodeRequest,IsFuzzableRequest,IsGlobalMatchersTemplate,HasWorkflows), which centralizes protocol detection and should reduce subtle length-check bugs. Behaviourally this aligns with prior intent, while making the DAST / headless / code gating easier to reason about.Long term, you could consider expressing more of this logic in terms of
Template.Requirements()+IsEnabledFor, with an explicit “DAST-only view” instead of duplicating predicate checks here.pkg/templates/templates.go (3)
166-179: DeprecatedHasCodeProtocol/HasFileProtocolshims are a reasonable compatibility bridgeExposing
HasCodeProtocolandHasFileProtocolas thin, deprecated wrappers around the underlying slices keeps existing callers compiling while steering new code toward theHasCodeRequest/HasFileRequestpredicate family. This is a pragmatic way to evolve the API.Documenting planned removal (e.g., in a changelog or deprecation policy) will help downstream users schedule migrations off these helpers.
181-207: Centralizing protocol presence and request-ID logic onHasXRequest/HasRequestis soundThe changes in
Type,validateAllRequestIDs, andUnmarshalYAMLconsistently move from manual slice length checks to:
HasXRequest()/HasXRequest(min)methods onTemplate.- Generic
HasRequest(...)checks for HTTP vshttp/ network vstcpalias fields.This yields a few benefits:
- Single-point logic for “does this template have protocol X?” including future protocol extensions.
- Safer multi-protocol detection via
hasMultipleRequests()and YAML key order preservation (addRequestsToQueue).- Clearer conflict errors when both legacy and new fields are present (e.g.,
httpandrequeststogether).The ordering in
Type()still matches the old implicit priority (DNS, File, HTTP, ... , Javascript, Workflows), so multi-protocol templates preserve their principal type.You may eventually want to base
hasMultipleRequests()on the sameHasXRequesthelpers to avoid divergence with any future protocol-counting logic, but it’s fine as-is.Also applies to: 243-325, 335-405
592-647:Requirements/CapabilitiesandIsEnabledForform a solid basis for unified gatingThe new model:
Requirementsencodes what a template needs to run:
Headless: any headless requests.Code: any code requests.DAST: any fuzzable requests (viaIsFuzzableRequest()).SelfContained: template-level self-contained flag.File: any file requests.Capabilitiesmirrors this shape from execution options.IsEnabledForsimply checks that eachtruerequirement is satisfied by the corresponding capability.This is simple, composable, and matches how
LoadTemplatesOnlyMetadatabuildscaps. It should make future gating changes easier (e.g., adding new capability bits) without scattering checks across the codebase.Over time, consolidating more of the loader’s per-flag gating into
IsEnabledFor(with a DAST-specific view where needed) would further reduce duplicated logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/catalog/loader/loader.go(17 hunks)pkg/protocols/headless/headless.go(1 hunks)pkg/protocols/http/http.go(1 hunks)pkg/templates/compile.go(1 hunks)pkg/templates/templates.go(7 hunks)pkg/templates/templates_utils.go(1 hunks)pkg/templates/workflows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/protocols/headless/headless.go
- pkg/protocols/http/http.go
- pkg/templates/templates_utils.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
pkg/catalog/loader/loader.gopkg/templates/compile.gopkg/templates/templates.gopkg/templates/workflows.go
pkg/catalog/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Template discovery and loading from disk/remote sources should be implemented in pkg/catalog
Files:
pkg/catalog/loader/loader.go
pkg/templates/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Template parsing, compilation, and management should be implemented in pkg/templates
Files:
pkg/templates/compile.gopkg/templates/templates.gopkg/templates/workflows.go
🧬 Code graph analysis (1)
pkg/catalog/loader/loader.go (3)
pkg/templates/templates.go (2)
Capabilities(613-619)Template(35-164)pkg/types/types.go (2)
Options(34-472)DefaultTemplateLoadingConcurrency(23-23)pkg/protocols/common/protocolstate/state.go (1)
GetDialersWithId(40-46)
⏰ 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 (6)
pkg/templates/workflows.go (1)
95-111: UsingHasCodeRequest()here is consistent with new request predicatesSwitching from a raw slice length check to
template.HasCodeRequest()keeps behaviour the same while centralizing protocol detection logic; the workflow gating for code templates (flag + verification) remains intact and looks correct.pkg/catalog/loader/loader.go (3)
630-647:LoadWorkflowsbehaviour unchanged and still straightforwardThe workflow loading path remains simple: catalog resolution,
LoadWorkflow, and thentemplates.Parsewith warnings on errors. The surrounding changes don’t alter semantics; appending only parsed, non-nil workflows is still correct.
842-853: HTTP-based protocol detection viaHasHTTPRequest/HasHeadlessRequestis clearer
IsHTTPBasedProtocolUsednow uses the new predicates instead of raw slice length checks:
template.HasHTTPRequest() || template.HasHeadlessRequest()for top-level templates.- Workflows still use
TemplateTypecomparisons, which keeps the check inexpensive.This is a straightforward readability win with no behaviour change.
Also applies to: 858-877
573-587: Workflow-aware validation changes are reasonable but do narrow coverage; verify this matches intended-validatebehaviorIn
areWorkflowOrTemplatesValid(lines 573-587, 589-603, 606-617):
- The skip condition
!isWorkflow && template.HasWorkflows()prevents templates with workflows from being validated when passed as regular templates. This is sensible only if templates containing workflows never appear in the non-workflow set during normal operation.areWorkflowTemplatesValidnow recursively validates subtemplates via catalog lookups, which is reasonable.isParsingErrorexcludingErrCreateTemplateExecutormeans templates that parse but cannot produce an executor now pass validation, narrowing what counts as a validation failure.These changes reduce the validation scope compared to prior behavior. Confirm that:
- Templates with
.Workflowsfields are only ever loaded intofinalWorkflows, never intofinalTemplates.- Executor-creation failures should not cause
-validateto fail for templates in the current environment.If templates can legitimately appear in both sets (e.g., a file containing both standard and workflow templates), this skip logic creates a coverage gap.
pkg/templates/templates.go (1)
413-503:ImportFileRefsflow/multiprotocol handling viaIsFlowTemplateandHasRequestis clearerSwitching flow detection to
template.IsFlowTemplate()and multiprotocol detection toHasRequest(template.RequestsQueue)makes the control flow much easier to follow:
- Flow templates: if
Flowpoints to a.jsfile, it’s loaded viaLoadHelperFileandoptions.Flowis set from the inlined content.- Multiprotocol templates (non-flow) use
RequestsQueueto resolvecode/javascriptfile references consistently with single-protocol handling.This keeps flow and multiprotocol concerns mutually exclusive and reuses the new helper predicates cleanly.
pkg/templates/compile.go (1)
323-352: UsingHasXRequestincompileProtocolRequestskeeps semantics while centralizing checksReplacing direct length checks with
HasDNSRequest/HasFileRequest/HasNetworkRequest/HasHTTPRequest/HasHeadlessRequest/HasSSLRequest/HasWebsocketRequest/HasWHOISRequest/HasCodeRequest/HasJavascriptRequestkeeps the selection logic identical while reusing the new predicate helpers. The additional gating onoptions.Options.HeadlessandEnableCodeTemplatesremains unchanged.
* fix: enable all template types for template list/display When using `-tl` or `-td`, explicitly enable all template types (`code`, `file`, `self-contained`, `global-matchers`, `headless`, `dast`) to ensure all templates are listed w/o requiring extra flags. Signed-off-by: Dwi Siswanto <[email protected]> * chore: cleanup messy messy messy Signed-off-by: Dwi Siswanto <[email protected]> --------- Signed-off-by: Dwi Siswanto <[email protected]>
* fix: enable all template types for template list/display When using `-tl` or `-td`, explicitly enable all template types (`code`, `file`, `self-contained`, `global-matchers`, `headless`, `dast`) to ensure all templates are listed w/o requiring extra flags. Signed-off-by: Dwi Siswanto <[email protected]> * chore: cleanup messy messy messy Signed-off-by: Dwi Siswanto <[email protected]> --------- Signed-off-by: Dwi Siswanto <[email protected]>
* fix: enable all template types for template list/display When using `-tl` or `-td`, explicitly enable all template types (`code`, `file`, `self-contained`, `global-matchers`, `headless`, `dast`) to ensure all templates are listed w/o requiring extra flags. Signed-off-by: Dwi Siswanto <[email protected]> * chore: cleanup messy messy messy Signed-off-by: Dwi Siswanto <[email protected]> --------- Signed-off-by: Dwi Siswanto <[email protected]>
* fix: enable all template types for template list/display When using `-tl` or `-td`, explicitly enable all template types (`code`, `file`, `self-contained`, `global-matchers`, `headless`, `dast`) to ensure all templates are listed w/o requiring extra flags. Signed-off-by: Dwi Siswanto <[email protected]> * chore: cleanup messy messy messy Signed-off-by: Dwi Siswanto <[email protected]> --------- Signed-off-by: Dwi Siswanto <[email protected]>
Proposed changes
fix(runner): enable all template types for template list/display
When using
-tlor-td, explicitly enable alltemplate types (
code,file,self-contained,global-matchers,headless,dast) to ensureall templates are listed w/o requiring extra flags.
Fixes #6644.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.