Repository navigation
Feat: Exit Codes - #18
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CLI policy enforcement feature (--fail-on) with parsing, evaluation, reporting, and exit-on-violation behavior; new failOn module and tests; README updates documenting new flags; new CLI tests; and VS Code workbench color customizations in .vscode/settings.json. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as RGBA(52,105,153,0.5)
participant Eval as RGBA(16,124,75,0.5)
participant Out as RGBA(153,102,51,0.5)
User->>CLI: run scan --fail-on <rules>
CLI->>CLI: parseFailOnRules(<rules>)
alt invalid or empty rules
CLI->>Out: print error & help
Out->>User: exit non-zero
else valid rules
CLI->>Eval: evaluatePolicyViolations(aggregatedData, rules)
Eval->>Eval: compute violation counts
Eval->>CLI: return PolicyViolation[]
CLI->>Out: printPolicyViolations(violations)
alt violations present
Out->>User: print banner & violations
Out->>User: exit 1
else no violations
Out->>User: continue / exit 0
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli.test.ts (1)
9-65: Good integration test for CLI summary output.The test validates the summary format and CTA placement using deterministic conditions (
--offline,--no-report,NO_COLOR=1). The 30-second timeout is appropriate for spawning a full CLI process.Consider adding a test case that exercises
--fail-onto verify the exit code behavior when policy violations are detected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.test.ts` around lines 9 - 65, Add a new integration test in src/cli.test.ts that exercises the CLI's --fail-on behavior: use the same spawnSync pattern (process.execPath, tsNodeBin, cliPath) with arguments ['scan','--project', repoRoot,'--offline','--no-report','--fail-on','<policy>'] (or a policy that will definitely trigger in the repo) and NO_COLOR=1; assert the process exit code (result.status) is non-zero when a policy violation is detected and that stdout/stderr contains the policy violation message; keep setup consistent with the existing test (repoRoot, tsNodeBin, cliPath, env) so the new test integrates cleanly..vscode/settings.json (1)
5-13: Editor theme customizations noted.These VSCode color settings are unrelated to the exit codes feature. Consider whether personal editor preferences should be committed to the repo, or if they belong in a user-local settings file instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/settings.json around lines 5 - 13, This commit adds user-specific VSCode color keys (e.g., "titleBar.activeForeground", "statusBar.background", "statusBar.noFolderForeground") that are unrelated to the feature; remove these personal editor settings from the committed JSON and either (a) move them to a local-only settings file (user settings) or (b) add the workspace .vscode settings file to your local .gitignore so it isn’t committed, ensuring only project-relevant editor settings remain in the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.vscode/settings.json:
- Around line 5-13: This commit adds user-specific VSCode color keys (e.g.,
"titleBar.activeForeground", "statusBar.background",
"statusBar.noFolderForeground") that are unrelated to the feature; remove these
personal editor settings from the committed JSON and either (a) move them to a
local-only settings file (user settings) or (b) add the workspace .vscode
settings file to your local .gitignore so it isn’t committed, ensuring only
project-relevant editor settings remain in the repo.
In `@src/cli.test.ts`:
- Around line 9-65: Add a new integration test in src/cli.test.ts that exercises
the CLI's --fail-on behavior: use the same spawnSync pattern (process.execPath,
tsNodeBin, cliPath) with arguments ['scan','--project',
repoRoot,'--offline','--no-report','--fail-on','<policy>'] (or a policy that
will definitely trigger in the repo) and NO_COLOR=1; assert the process exit
code (result.status) is non-zero when a policy violation is detected and that
stdout/stderr contains the policy violation message; keep setup consistent with
the existing test (repoRoot, tsNodeBin, cliPath, env) so the new test integrates
cleanly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.test.ts`:
- Around line 108-109: The test currently asserts a non-zero exit with
expect(result.status).not.toBe(0); which is too lax—change the assertion to
require the specific failure code 1 by replacing that call with
expect(result.status).toBe(1); so the test (in the same test block where result
is produced) validates the exact intended fail-on exit code rather than any
non-zero value.
- Around line 5-6: The inline regex in stripAnsi contains a literal control
character and trips Biome lint; extract the pattern into a top-level RegExp
constant (e.g. ANSI_ESCAPE_REGEX = new RegExp('\\\\x1b\\[[0-9;]*[A-Za-z]', 'g'))
and replace the inline pattern in function stripAnsi to use that constant
(return value.replace(ANSI_ESCAPE_REGEX, '')). This moves the escaped control
sequence into a string literal for the RegExp constructor and fixes the lint
rule while keeping stripAnsi behavior unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.test.ts`:
- Around line 41-64: The merged output used for assertions includes stderr
noise; keep the combined stripAnsi(`${result.stdout}\n${result.stderr}`) for the
general expectations but for the final-line CTA assertion use only the cleaned
stdout (e.g. stripAnsi(result.stdout).replace(/\r/g, '')) so the
.trim().endsWith(...) check is not affected by stderr; update the assertion that
currently calls expect(output.trim().endsWith(...)).toBe(true) to operate on the
cleaned stdout string instead while leaving all other checks against the
existing combined `output`.
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JosephMaynard. The following files were modified: * `src/cli.ts` * `src/failOn.ts` These files were ignored: * `src/cli.test.ts` * `src/failOn.test.ts` These file types are not supported: * `.vscode/settings.json` * `README.md`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/failOn.ts`:
- Line 88: Update the JSDoc for the parameter "value" in src/failOn.ts so the
example uses the correct rule names; replace the incorrect
"production-vulnerabilities,high-severity" with
"production-vuln,high-severity-vuln" in the docstring so it matches the actual
rules (refer to the "value" param in the failOn function/JSDoc).
| /** | ||
| * Parse a comma-separated list of fail-on rule names into a validated set. | ||
| * | ||
| * @param value - Comma-separated rule names (e.g., "production-vulnerabilities,high-severity") |
Contributor
There was a problem hiding this comment.
Docstring example uses incorrect rule name.
The example shows "production-vulnerabilities,high-severity" but the actual rule names are production-vuln and high-severity-vuln. This could confuse developers reading the documentation.
📝 Proposed fix
- * `@param` value - Comma-separated rule names (e.g., "production-vulnerabilities,high-severity")
+ * `@param` value - Comma-separated rule names (e.g., "production-vuln,high-severity-vuln")📝 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.
Suggested change
| * @param value - Comma-separated rule names (e.g., "production-vulnerabilities,high-severity") | |
| * `@param` value - Comma-separated rule names (e.g., "production-vuln,high-severity-vuln") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/failOn.ts` at line 88, Update the JSDoc for the parameter "value" in
src/failOn.ts so the example uses the correct rule names; replace the incorrect
"production-vulnerabilities,high-severity" with
"production-vuln,high-severity-vuln" in the docstring so it matches the actual
rules (refer to the "value" param in the failOn function/JSDoc).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Style