-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat(config): Add 'off' option for failOn and commentOn #19
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
Changes from all commits
b7bea5d
4d0b9a2
194e63b
005cf97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||||||||||||||||||
| import type { Trigger } from '../config/schema.js'; | ||||||||||||||||||||||
| import { SEVERITY_ORDER } from '../types/index.js'; | ||||||||||||||||||||||
| import type { EventContext, Severity, SkillReport } from '../types/index.js'; | ||||||||||||||||||||||
| import type { EventContext, Severity, SeverityThreshold, SkillReport } from '../types/index.js'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** Cache for compiled glob patterns */ | ||||||||||||||||||||||
| const globCache = new Map<string, RegExp>(); | ||||||||||||||||||||||
|
|
@@ -96,16 +96,20 @@ export function matchTrigger(trigger: Trigger, context: EventContext): boolean { | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Check if a report has any findings at or above the given severity threshold. | ||||||||||||||||||||||
| * Returns false if failOn is 'off' (disabled). | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export function shouldFail(report: SkillReport, failOn: Severity): boolean { | ||||||||||||||||||||||
| export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean { | ||||||||||||||||||||||
| if (failOn === 'off') return false; | ||||||||||||||||||||||
| const threshold = SEVERITY_ORDER[failOn]; | ||||||||||||||||||||||
|
Comment on lines
+101
to
103
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Missing null/undefined check before accessing SEVERITY_ORDER The code accesses SEVERITY_ORDER[failOn] without checking if 'failOn' is a valid key in the SEVERITY_ORDER object. While the early return handles 'off', if an invalid severity value is passed, this could result in 'threshold' being undefined, leading to undefined behavior in the comparison. The same issue exists in the second function accessing SEVERITY_ORDER[f.severity] for each finding. Suggested fix: Add validation to ensure the severity value exists in SEVERITY_ORDER before accessing it, or add type guards to prevent invalid values from being passed
Suggested change
warden: security-review
Comment on lines
+101
to
103
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Potential type confusion between Severity and SeverityThreshold The functions now accept 'SeverityThreshold' (which presumably includes 'off') but use SEVERITY_ORDER lookup without verifying that 'off' is excluded from the lookup. If 'off' is not defined in SEVERITY_ORDER, the threshold variable will be undefined, which could lead to unexpected behavior in the comparison operations. While there's an early return for 'off', the type system now allows 'off' to reach SEVERITY_ORDER[failOn] if the check is removed or bypassed. Suggested fix: Consider using a type guard or assertion after the 'off' check to ensure type safety, or ensure SEVERITY_ORDER explicitly handles all SeverityThreshold values including 'off'.
Suggested change
warden: security-review |
||||||||||||||||||||||
| return report.findings.some((f) => SEVERITY_ORDER[f.severity] <= threshold); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Count findings at or above the given severity threshold. | ||||||||||||||||||||||
| * Returns 0 if failOn is 'off' (disabled). | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export function countFindingsAtOrAbove(report: SkillReport, failOn: Severity): number { | ||||||||||||||||||||||
| export function countFindingsAtOrAbove(report: SkillReport, failOn: SeverityThreshold): number { | ||||||||||||||||||||||
| if (failOn === 'off') return 0; | ||||||||||||||||||||||
| const threshold = SEVERITY_ORDER[failOn]; | ||||||||||||||||||||||
| return report.findings.filter((f) => SEVERITY_ORDER[f.severity] <= threshold).length; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,10 @@ | |||||||||||||||||||||
| export const SeveritySchema = z.enum(['critical', 'high', 'medium', 'low', 'info']); | ||||||||||||||||||||||
| export type Severity = z.infer<typeof SeveritySchema>; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Severity threshold for config options (includes 'off' to disable) | ||||||||||||||||||||||
| export const SeverityThresholdSchema = z.enum(['off', 'critical', 'high', 'medium', 'low', 'info']); | ||||||||||||||||||||||
| export type SeverityThreshold = z.infer<typeof SeverityThresholdSchema>; | ||||||||||||||||||||||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Severity order for comparison (lower = more severe). | ||||||||||||||||||||||
| * Single source of truth for severity ordering across the codebase. | ||||||||||||||||||||||
|
|
@@ -19,9 +23,11 @@ | |||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Filter findings to only include those at or above the given severity threshold. | ||||||||||||||||||||||
| * If no threshold is provided, returns all findings unchanged. | ||||||||||||||||||||||
| * If threshold is 'off', returns empty array (disabled). | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export function filterFindingsBySeverity(findings: Finding[], threshold?: Severity): Finding[] { | ||||||||||||||||||||||
| export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] { | ||||||||||||||||||||||
| if (!threshold) return findings; | ||||||||||||||||||||||
| if (threshold === 'off') return []; | ||||||||||||||||||||||
|
Check warning on line 30 in src/types/index.ts
|
||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Type mismatch when accessing SEVERITY_ORDER with SeverityThreshold The function now accepts Suggested fix: Add a type assertion or use a type guard to narrow the threshold type after the 'off' check, making the type relationship explicit.
Suggested change
warden: find-bugs |
||||||||||||||||||||||
| const thresholdOrder = SEVERITY_ORDER[threshold]; | ||||||||||||||||||||||
|
Comment on lines
29
to
31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Type mismatch when accessing SEVERITY_ORDER with SeverityThreshold The function now accepts Suggested fix: Cast threshold to Severity after the 'off' check, or use type guard to narrow the type
Suggested change
warden: security-review |
||||||||||||||||||||||
| return findings.filter((f) => SEVERITY_ORDER[f.severity] <= thresholdOrder); | ||||||||||||||||||||||
|
Comment on lines
+28
to
32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Type mismatch in SEVERITY_ORDER lookup with SeverityThreshold The function now accepts 'SeverityThreshold' which includes 'off', but line 31 attempts to look up 'threshold' in 'SEVERITY_ORDER' without checking if it's 'off' first. While there is a check for 'off' on line 30, if the order of these checks is changed or if the code is refactored, this could cause a runtime error since 'SEVERITY_ORDER' is typed as 'Record<Severity, number>' and 'off' is not a valid 'Severity'. TypeScript should catch this, but the logic could be more robust. Suggested fix: Add explicit type narrowing or use type guards to ensure 'threshold' is a valid Severity before SEVERITY_ORDER lookup. Alternatively, restructure the checks to make the intent clearer.
Suggested change
warden: security-review |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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.
Unused
extraLabelsfield added to interfaceLow Severity
The
extraLabelsfield was added to theRenderOptionsinterface but remains unused throughout the codebase. TherenderSkillReportfunction, which consumes these options, doesn't reference it, indicating it may be dead code.