Skip to content

feat(config): Add 'off' option for failOn and commentOn#19

Merged
dcramer merged 4 commits intomainfrom
feat/optional-checks
Jan 30, 2026
Merged

feat(config): Add 'off' option for failOn and commentOn#19
dcramer merged 4 commits intomainfrom
feat/optional-checks

Conversation

@dcramer
Copy link
Member

@dcramer dcramer commented Jan 29, 2026

Allow users to completely disable failure checks and PR comments by setting failOn and/or commentOn to "off".

  • failOn=off: Never fail the check, regardless of findings
  • commentOn=off: Never post PR comments/annotations

This is useful when users want to run Warden for informational purposes only without affecting CI status or creating PR noise.

Introduces a SeverityThreshold type that extends Severity with 'off', keeping the existing Severity type unchanged for findings themselves.

Usage:

In warden.toml:

[defaults.output]
failOn = "off"      # Never fail CI
commentOn = "off"   # No PR comments

Via CLI:

warden --fail-on off --comment-on off

Via GitHub Action:

- uses: getsentry/warden@main
  with:
    fail-on: 'off'
    comment-on: 'off'

@vercel
Copy link

vercel bot commented Jan 29, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
warden Ready Ready Preview, Comment Jan 30, 2026 0:11am

Request Review

Copy link
Contributor

@sentry-warden sentry-warden bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-review

security-review: Found 4 issues (3 medium, 1 low)

}

const validSeverities = ['critical', 'high', 'medium', 'low', 'info'] as const;
const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Potential logic issue: 'off' severity may bypass validation elsewhere

Adding 'off' to the validSeverities array allows it to pass the includes() check on line 66 and be assigned to failOn/commentOn. However, this may cause issues in downstream code that expects only actual severity levels (critical, high, medium, low, info). If 'off' is meant to disable the feature, it should be handled separately (e.g., checking for 'off' and setting the value to undefined), rather than treating it as a valid severity level that gets passed through the system.

Suggested fix: Handle 'off' as a special case that results in undefined, keeping it separate from actual severity levels

Suggested change
const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const;
const validSeverities = ['critical', 'high', 'medium', 'low', 'info'] as const;
const failOn = failOnInput === 'off' ? undefined : validSeverities.includes(failOnInput as typeof validSeverities[number])

warden: security-review

Comment on lines +101 to 103
export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean {
if (failOn === 'off') return false;
const threshold = SEVERITY_ORDER[failOn];
Copy link
Contributor

Choose a reason for hiding this comment

The 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
export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean {
if (failOn === 'off') return false;
const threshold = SEVERITY_ORDER[failOn];
if (threshold === undefined) return false;

warden: security-review

Comment on lines +28 to 32
export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] {
if (!threshold) return findings;
if (threshold === 'off') return [];
const thresholdOrder = SEVERITY_ORDER[threshold];
return findings.filter((f) => SEVERITY_ORDER[f.severity] <= thresholdOrder);
Copy link
Contributor

Choose a reason for hiding this comment

The 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
export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] {
if (!threshold) return findings;
if (threshold === 'off') return [];
const thresholdOrder = SEVERITY_ORDER[threshold];
return findings.filter((f) => SEVERITY_ORDER[f.severity] <= thresholdOrder);
export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] {
if (!threshold) return findings;
if (threshold === 'off') return [];
// At this point, threshold is guaranteed to be a valid Severity
const thresholdOrder = SEVERITY_ORDER[threshold as Severity];

warden: security-review

Copy link
Contributor

@sentry-warden sentry-warden bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-review

security-review: Found 4 issues (3 medium, 1 low)

}

const validSeverities = ['critical', 'high', 'medium', 'low', 'info'] as const;
const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Addition of 'off' severity may bypass security controls

The 'off' value added to validSeverities allows users to disable security checks via fail-on or comment-on inputs. While this may be intentional for flexibility, it creates a risk that security findings will be ignored. If 'fail-on' is set to 'off', the action will never fail on security findings regardless of their severity. This could allow critical vulnerabilities to pass through CI/CD pipelines unnoticed if misconfigured or intentionally disabled by malicious actors with repository write access.

Suggested fix: Consider: 1) Document this behavior clearly in user-facing documentation with security warnings, 2) Log a warning when 'off' is used to ensure visibility, 3) Consider whether 'off' should only be allowed for comment-on but not fail-on to maintain minimum security guardrails, or 4) Require explicit acknowledgment (e.g., 'disabled' instead of 'off') to prevent accidental misconfiguration.

Suggested change
const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const;
if (failOnInput === 'off') {
console.warn('WARNING: fail-on is set to "off" - security findings will not fail the build');
}

warden: security-review

Comment on lines +101 to 103
export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean {
if (failOn === 'off') return false;
const threshold = SEVERITY_ORDER[failOn];
Copy link
Contributor

Choose a reason for hiding this comment

The 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
export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean {
if (failOn === 'off') return false;
const threshold = SEVERITY_ORDER[failOn];
// Type assertion to ensure failOn is now Severity after 'off' check
const severityLevel: Severity = failOn as Severity;
const threshold = SEVERITY_ORDER[severityLevel];

warden: security-review

Comment on lines 29 to 31
if (!threshold) return findings;
if (threshold === 'off') return [];
const thresholdOrder = SEVERITY_ORDER[threshold];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Type mismatch when accessing SEVERITY_ORDER with SeverityThreshold

The function now accepts SeverityThreshold which includes 'off', but SEVERITY_ORDER is typed as Record<Severity, number> which does not include 'off'. When threshold is not 'off', the code accesses SEVERITY_ORDER[threshold] which could be 'off' at the type level, causing a type error. The runtime check for 'off' happens before this access, but TypeScript may not narrow the type correctly, potentially allowing undefined to be assigned to thresholdOrder.

Suggested fix: Cast threshold to Severity after the 'off' check, or use type guard to narrow the type

Suggested change
if (!threshold) return findings;
if (threshold === 'off') return [];
const thresholdOrder = SEVERITY_ORDER[threshold];
const thresholdOrder = SEVERITY_ORDER[threshold as Severity];

warden: security-review

@sentry-warden
Copy link
Contributor

sentry-warden bot commented Jan 29, 2026

security-review

security-review: No issues found

No findings to report.

@sentry-warden
Copy link
Contributor

sentry-warden bot commented Jan 29, 2026

security-review

security-review: No issues found

No findings to report.

dcramer and others added 3 commits January 29, 2026 16:03
Allow users to completely disable failure checks and PR comments by
setting failOn and/or commentOn to 'off'.

- failOn=off: Never fail the check, regardless of findings
- commentOn=off: Never post PR comments/annotations

Introduces SeverityThreshold type that extends Severity with 'off',
keeping the existing Severity type unchanged for findings.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When commentOn was set to 'off', a summary comment "No findings to report"
was still being posted because postReviewToGitHub was called unconditionally.
Now the commentOn value is tracked in TriggerResult and checked before posting.

Also moves misplaced import statement to top of output/types.ts.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instead of computing renderResult and then checking commentOn before
posting, skip the renderSkillReport call entirely when comments are
disabled. This removes unnecessary work and simplifies TriggerResult.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace duplicated inline union types and validSeverities array with
imports from src/types/index.ts. Uses SeverityThresholdSchema.safeParse
for validation instead of manual array inclusion check.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Contributor

@sentry-warden sentry-warden bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find-bugs

find-bugs: Found 2 issues (1 medium, 1 low)


429.6k in / 11.7k out · $1.49

export function filterFindingsBySeverity(findings: Finding[], threshold?: Severity): Finding[] {
export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] {
if (!threshold) return findings;
if (threshold === 'off') return [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Type mismatch when accessing SEVERITY_ORDER with SeverityThreshold

The function now accepts SeverityThreshold which includes 'off', but SEVERITY_ORDER is typed as Record<Severity, number> which does not include 'off'. While the code handles 'off' before accessing SEVERITY_ORDER[threshold], TypeScript's type system will flag this as an error because threshold is still typed as SeverityThreshold (not narrowed to Severity) when used as the key.

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
if (threshold === 'off') return [];
const thresholdOrder = SEVERITY_ORDER[threshold as Severity];

warden: find-bugs

@dcramer dcramer merged commit 2f20061 into main Jan 30, 2026
12 checks passed
@dcramer dcramer deleted the feat/optional-checks branch January 30, 2026 00:16
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

groupByFile?: boolean;
/** Only include findings at or above this severity level in rendered output */
commentOn?: 'critical' | 'high' | 'medium' | 'low' | 'info';
extraLabels?: string[];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused extraLabels field added to interface

Low Severity

The extraLabels field was added to the RenderOptions interface but remains unused throughout the codebase. The renderSkillReport function, which consumes these options, doesn't reference it, indicating it may be dead code.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant