Skip to content

Commit dff43cf

Browse files
fix: address P1 issues from security commands code review
- Fix SECURITY_SCAN_DAYS to avoid NaN (clamp to positive integer, default 7) - Remove instructions to commit threat model to PR branch during review - Remove instructions to commit patches to PR branch - Align security review with JSON output pattern (no direct inline comments) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 parent f607dfd commit dff43cf

File tree

2 files changed

+6
-9
lines changed

2 files changed

+6
-9
lines changed

src/create-prompt/templates/security-review-prompt.ts

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,8 @@ You have access to these Factory security skills (installed in ~/.factory/skills
4949
5050
### Step 1: Threat Model Check
5151
- Check if \`.factory/threat-model.md\` exists in the repository
52-
- If missing: Invoke the **threat-model-generation** skill to create one, then commit it to the PR branch
53-
- If exists: Check the file's last modified date
54-
- If >90 days old: Post a warning comment suggesting regeneration, but proceed with scan
55-
- If current: Use it as context for the security scan
52+
- If missing: Note this in the summary (threat model generation is done separately, not during PR review)
53+
- If exists: Use it as context for the security scan
5654
5755
### Step 2: Security Scan
5856
- Invoke the **commit-security-scan** skill on the PR diff
@@ -71,12 +69,11 @@ You have access to these Factory security skills (installed in ~/.factory/skills
7169
- Impact: What's the potential damage?
7270
- Filter out false positives and findings below the severity threshold (${severityThreshold})
7371
74-
### Step 4: Report & Patch
72+
### Step 4: Report Findings
7573
- For each confirmed finding at or above ${severityThreshold} severity:
76-
- Post inline comment using \`github_inline_comment___create_inline_comment\`
74+
- Record in the JSON output file
7775
- Include: severity, STRIDE category, CWE ID, clear explanation, suggested fix
78-
- For auto-fixable issues: Invoke **security-review** skill to generate patches
79-
- Commit any generated patches to the PR branch
76+
- For auto-fixable issues: Include the suggested patch in the finding's suggestion field
8077
8178
## Security Scope (STRIDE Categories)
8279

src/github/context.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ export function parseGitHubContext(): GitHubContext {
157157
securityBlockOnHigh: process.env.SECURITY_BLOCK_ON_HIGH === "true",
158158
securityNotifyTeam: process.env.SECURITY_NOTIFY_TEAM ?? "",
159159
securityScanSchedule: process.env.SECURITY_SCAN_SCHEDULE === "true",
160-
securityScanDays: parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10),
160+
securityScanDays: Math.max(1, parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10) || 7),
161161
},
162162
};
163163

0 commit comments

Comments
 (0)