feat: add security review feature with @droid security command #10
Conversation
Add comprehensive security review functionality: ## New Commands - `@droid security` - Security-focused code review using STRIDE methodology - `@droid review security` or `@droid security review` - Run both code and security reviews in parallel - `@droid security --full` - Full repository security scan ## New Inputs (action.yml) - automatic_security_review - Auto-run security review on PRs - security_model - Override model for security reviews - security_severity_threshold - Filter by severity (critical/high/medium/low) - security_block_on_critical - Block PR on critical findings - security_block_on_high - Block PR on high findings - security_notify_team - Team to @mention on critical findings - security_scan_schedule - Enable scheduled scans - security_scan_days - Days of commits to scan ## New Files - security/action.yml - Security review sub-action - src/tag/commands/security-review.ts - Security review command handler - src/tag/commands/security-scan.ts - Full repo scan handler - src/create-prompt/templates/security-review-prompt.ts - STRIDE-based prompt - src/create-prompt/templates/security-report-prompt.ts - Scan report prompt - src/entrypoints/combine-reviews.ts - Combine code + security results ## Security Skills Integration Auto-installs Factory security skills from Factory-AI/skills: - threat-model-generation - commit-security-scan - vulnerability-validation - security-review Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
Droid finished @shashank-factory's task —— View job 🔍 Code Review SummaryReviewed the security review feature PR and found 3 potential issues worth discussing:
See inline comments for details. |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| needs: [prepare, code-review] | ||
| if: always() && needs.prepare.outputs.run_code_review == 'true' | ||
| needs: [prepare, code-review, security-review] | ||
| # Only run combine when BOTH code review AND security review were executed |
There was a problem hiding this comment.
This condition requires both run_code_review AND run_security_review to be true. However, when an existing security review is detected (hasExistingSecurityReview returns true in src/tag/index.ts), run_security_review is set to false while run_code_review is true. In that case, the combine job will be skipped and the tracking comment won't be updated with the code review results.
Should this use OR logic instead (||) to run combine when either review was executed?
| core.setOutput("run_code_review", "true"); | ||
| core.setOutput("run_security_review", "true"); | ||
| return { | ||
| skipped: false, |
There was a problem hiding this comment.
When handling the explicit @droid security command, this returns early without calling prepareSecurityReviewMode(). As a result, droid_args and mcp_tools outputs won't be set, and downstream workflow steps that depend on these outputs will fail or behave unexpectedly.
Should this call prepareSecurityReviewMode() similar to how automaticSecurityReview is handled above?
| const command = extractCommandFromContext(context); | ||
| if (command && ['fill', 'review'].includes(command.command)) { | ||
| console.log(`Detected @droid ${command.command} command, triggering action`); | ||
| if (command && ["fill", "review", "security"].includes(command.command)) { |
There was a problem hiding this comment.
The security-full and review-security commands are missing from this trigger check array. Since parseDroidCommand() in command-parser.ts recognizes these commands, users invoking @droid security --full or @droid review security will not trigger the action.
Could we update this to include all security-related commands?
| if (command && ["fill", "review", "security"].includes(command.command)) { | |
| if (command && ["fill", "review", "security", "security-full", "review-security"].includes(command.command)) { |
Summary
Add comprehensive security review functionality with new commands, configuration options, and Factory security skills integration.
New Commands
@droid security@droid review security@droid security review@droid security --fullNew Configuration Options
automatic_security_reviewfalsesecurity_model""security_severity_thresholdmediumsecurity_block_on_criticaltruesecurity_block_on_highfalsesecurity_notify_team""security_scan_schedulefalsesecurity_scan_days7New Sub-Action
Security Skills Integration
Auto-installs Factory security skills from
Factory-AI/skills:Parallel Workflow
When both
automatic_reviewandautomatic_security_revieware enabled, reviews run in parallel:The combine step only runs when BOTH reviews are enabled, ensuring deduplicated inline comments.
Testing
Depends On
Part of
This is PR 3 of 3 for the security review feature: