feat(query): group findings by field with aggregation - #1304
Conversation
Add a --group option to `secator q` and `secator report show` that collapses
findings by one or more fields (processing-side, post-query) with
auto-aggregation of a per-type field.
- New _group_by / _group_aggregate class attrs on OutputType, set for
Vulnerability (name / matched_at), Tag (category,name / match) and
Technology (product / match).
- --group is an optional-value option: bare --group uses the per-type
defaults; --group "<field>[,<field>]" overrides the group fields.
- group_findings() helper collapses each group to the newest finding, keeps a
_group_count and a truncated list of aggregated values (".. and X more").
- Console exporter shows "(count: N)" next to grouped lines; an Info message
per grouped type is printed at the end.
- Types without _group_by (or non-finding results) pass through unchanged;
--group with an explicit field but no groupable types warns; --group is
ignored (with a warning) when --format is used.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VWZJdNgJKrn1XHkENKqVo7
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesFinding grouping
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
secator/query/utils.py (1)
558-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog load failures instead of silently swallowing them.
The
except Exception: passat lines 600-603 silently degrades grouping: ifcls.load(rep)fails, the representative stays a dict, so the aggregate field is never set (line 604 guards onnot isinstance(rep, dict)) and_group_countis never set (line 609). The finding appears ungrouped with no count or aggregated values, and no diagnostic is emitted.Ruff flags this as S110 (try-except-pass) and BLE001 (blind exception). Consider logging the failure at debug level so schema mismatches or data issues are traceable.
♻️ Proposed fix
try: rep = cls.load(rep) - except Exception: - pass + except Exception as exc: + logger.debug('group_findings: failed to load %s representative: %s', rep.get('_type'), exc)🤖 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 `@secator/query/utils.py` around lines 558 - 612, In group_findings, replace the silent broad exception around cls.load(rep) with a debug-level log that includes the exception and relevant finding/type context; use the module’s existing logger or add an appropriate logger without introducing a blind except-pass. Preserve the fallback behavior while making load failures traceable and satisfying Ruff S110/BLE001.Source: Linters/SAST tools
🤖 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 `@secator/cli.py`:
- Around line 1459-1481: Update the warning condition in the group-processing
block after grouped_types is populated so it triggers when grouping was
requested, including bare --group where group == '', and no groupable types were
found; preserve the existing warning message and avoid warning when --group was
not provided.
---
Nitpick comments:
In `@secator/query/utils.py`:
- Around line 558-612: In group_findings, replace the silent broad exception
around cls.load(rep) with a debug-level log that includes the exception and
relevant finding/type context; use the module’s existing logger or add an
appropriate logger without introducing a blind except-pass. Preserve the
fallback behavior while making load failures traceable and satisfying Ruff
S110/BLE001.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fbd523e-a333-42b1-9454-629ffde1cbe1
📒 Files selected for processing (8)
secator/cli.pysecator/exporters/console.pysecator/output_types/_base.pysecator/output_types/tag.pysecator/output_types/technology.pysecator/output_types/vulnerability.pysecator/query/utils.pytests/unit/test_query_utils.py
|
|
||
| # Group findings by field(s) with auto-aggregation (processing-side, post-query). | ||
| # `group is None` => disabled; `group == ''` => per-type defaults; else explicit field(s). | ||
| grouped_types = [] | ||
| if group is not None and not fmt: | ||
| from secator.query.utils import group_findings | ||
| user_group_by = [f.strip() for f in group.split(',') if f.strip()] | ||
| type_map = {cls.get_name(): cls for cls in FINDING_TYPES} | ||
| for type_name, items in report.data['results'].items(): | ||
| cls = type_map.get(type_name) | ||
| if not items or cls is None: | ||
| continue | ||
| group_by = user_group_by or list(getattr(cls, '_group_by', ()) or ()) | ||
| if not group_by: | ||
| continue | ||
| aggregate_field = getattr(cls, '_group_aggregate', None) | ||
| report.data['results'][type_name] = group_findings(items, group_by, aggregate_field) | ||
| grouped_types.append((type_name, ', '.join(group_by))) | ||
| if group and not grouped_types: | ||
| console.print(Warning(message=f'--group "{group}": no groupable finding types in results')) | ||
| elif group is not None and fmt: | ||
| console.print(Warning(message='--group is ignored when --format is used')) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bare --group with no groupable types produces no warning.
Line 1477 checks if group and not grouped_types, but when bare --group is used, group == '' (falsy), so the warning never fires. Users who run --group with only non-groupable result types (e.g., URLs, domains) get no feedback that grouping was a no-op.
The PR objectives state grouping should "warn when grouping is unavailable." Fix the condition to also cover the bare-flag case.
🐛 Proposed fix
- if group and not grouped_types:
- console.print(Warning(message=f'--group "{group}": no groupable finding types in results'))
+ if not grouped_types:
+ group_desc = f' "{group}"' if group else ''
+ console.print(Warning(message=f'--group{group_desc}: no groupable finding types in results'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Group findings by field(s) with auto-aggregation (processing-side, post-query). | |
| # `group is None` => disabled; `group == ''` => per-type defaults; else explicit field(s). | |
| grouped_types = [] | |
| if group is not None and not fmt: | |
| from secator.query.utils import group_findings | |
| user_group_by = [f.strip() for f in group.split(',') if f.strip()] | |
| type_map = {cls.get_name(): cls for cls in FINDING_TYPES} | |
| for type_name, items in report.data['results'].items(): | |
| cls = type_map.get(type_name) | |
| if not items or cls is None: | |
| continue | |
| group_by = user_group_by or list(getattr(cls, '_group_by', ()) or ()) | |
| if not group_by: | |
| continue | |
| aggregate_field = getattr(cls, '_group_aggregate', None) | |
| report.data['results'][type_name] = group_findings(items, group_by, aggregate_field) | |
| grouped_types.append((type_name, ', '.join(group_by))) | |
| if group and not grouped_types: | |
| console.print(Warning(message=f'--group "{group}": no groupable finding types in results')) | |
| elif group is not None and fmt: | |
| console.print(Warning(message='--group is ignored when --format is used')) | |
| # Group findings by field(s) with auto-aggregation (processing-side, post-query). | |
| # `group is None` => disabled; `group == ''` => per-type defaults; else explicit field(s). | |
| grouped_types = [] | |
| if group is not None and not fmt: | |
| from secator.query.utils import group_findings | |
| user_group_by = [f.strip() for f in group.split(',') if f.strip()] | |
| type_map = {cls.get_name(): cls for cls in FINDING_TYPES} | |
| for type_name, items in report.data['results'].items(): | |
| cls = type_map.get(type_name) | |
| if not items or cls is None: | |
| continue | |
| group_by = user_group_by or list(getattr(cls, '_group_by', ()) or ()) | |
| if not group_by: | |
| continue | |
| aggregate_field = getattr(cls, '_group_aggregate', None) | |
| report.data['results'][type_name] = group_findings(items, group_by, aggregate_field) | |
| grouped_types.append((type_name, ', '.join(group_by))) | |
| if not grouped_types: | |
| group_desc = f' "{group}"' if group else '' | |
| console.print(Warning(message=f'--group{group_desc}: no groupable finding types in results')) | |
| elif group is not None and fmt: | |
| console.print(Warning(message='--group is ignored when --format is used')) |
🤖 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 `@secator/cli.py` around lines 1459 - 1481, Update the warning condition in the
group-processing block after grouped_types is populated so it triggers when
grouping was requested, including bare --group where group == '', and no
groupable types were found; preserve the existing warning message and avoid
warning when --group was not provided.
|
@claude address coderabbit review comment |
|
Claude finished @ocervell's task in 3m 7s —— View job Addressing CodeRabbit review comments
Changes made (commit
|
- secator/cli.py: fire the "no groupable finding types" warning for bare --group too (group == '' was falsy, so the warning never fired when only per-type defaults were requested). - secator/query/utils.py: log load failures in group_findings via debug() instead of silently swallowing them (Ruff S110/BLE001). Co-authored-by: Olivier Cervello <9629314+ocervell@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
2 participants
Closes #1219.
Adds a
--groupoption tosecator qandsecator report showthat groups findings by one or more fields with auto-aggregation, done on the processing side after the query (no backend query changes), as the issue suggested.Usage
secator q vulnerability --group— group vulnerabilities by their default field(s).secator q vulnerability --group severity— group by an explicit field.--groupuses per-type defaults;--group "<field>[,<field>]"overrides.What it does
_group_by/_group_aggregateclass attrs onOutputType, set for the three types the issue lists:Vulnerability→ group byname, aggregatematched_atTag→ group bycategory,name, aggregatematchTechnology→ group byproduct, aggregatematchgroup_findings()collapses each group to the newest finding, keeping a_group_countand a truncated list of the aggregated field's values (.. and X morepast 5).(count: N)next to each grouped line.Infomessage per grouped type is printed at the end (<type> grouped by <field>. To show complete results, remove the --group option.).Degradation
_group_by(and non-finding results) pass through unchanged.--group <field>with no groupable finding types present → warning, no silent mis-answer.--groupcombined with--formatis ignored with a warning (format already reshapes output).Notes / assumptions
$grouppipeline needed, matching the issue's "processing side before displaying" guidance.Technologyis a typo —Technologyhas nomatched_atfield; its location field ismatch, so that is used (same asTag).--grouppath forsecator q/report show. The issue's hedged "secator r listshould probably default to grouped vuln counts" is a separate command/code path and was left out to keep scope tight — easy follow-up if wanted.Test
Added
TestGroupFindingsintests/unit/test_query_utils.py(group-by-name + aggregate + truncation asserts).secator test unitquery/report/output-type suites pass (240 tests).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--groupflag to query and report display commands.Tests