feat: add security review skill to plan implementation#482
Open
dotuananh0712 wants to merge 3 commits intoobra:mainfrom
Open
feat: add security review skill to plan implementation#482dotuananh0712 wants to merge 3 commits intoobra:mainfrom
dotuananh0712 wants to merge 3 commits intoobra:mainfrom
Conversation
Adds guidance to run git diff commands separately instead of chaining with && to avoid consent prompts. Fixes obra#476
After generating a detailed implementation plan, clear context to free up memory for implementation and debugging. Fixes obra#478
- Enhanced code-reviewer.md with comprehensive security checklist - Added new security-reviewer.md template with OWASP Top 10 checks - Added new security-reviewer-prompt.md for dispatching security subagents - Updated subagent-driven-development workflow to include optional security review This addresses the issue that code review skill was not fully realized, and adds dedicated security review to the implementation process. Fixes obra#479
📝 WalkthroughWalkthroughThis PR integrates a dedicated security review stage into the development workflow, positioned after code quality checks. It includes new security review documentation with comprehensive checklists, improves existing code review guidance, adds memory management instructions for implementation, and updates the task workflow to dispatch security reviewers and handle security fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Implementer
participant CodeQualityReviewer as Code Quality<br/>Reviewer
participant SecurityReviewer as Security<br/>Reviewer
participant TaskCompletion as Task<br/>Completion
Implementer->>CodeQualityReviewer: Submit implementation
CodeQualityReviewer->>CodeQualityReviewer: Review code quality
alt Code Quality Issues
CodeQualityReviewer-->>Implementer: Request changes
Implementer->>CodeQualityReviewer: Resubmit
else Code Quality Approved
CodeQualityReviewer->>SecurityReviewer: Dispatch security review
SecurityReviewer->>SecurityReviewer: Perform security review
alt Security Issues Found
SecurityReviewer-->>Implementer: Report vulnerabilities
Implementer->>Implementer: Fix security issues
Implementer->>SecurityReviewer: Resubmit for review
else Security Approved
SecurityReviewer->>TaskCompletion: Approve
TaskCompletion->>TaskCompletion: Complete task
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/subagent-driven-development/SKILL.md (1)
56-83:⚠️ Potential issue | 🟡 MinorFix DOT node/edge shapes for valid diagram semantics.
“Dispatch security reviewer” is an action (box), and
shapeon an edge is not valid DOT—this can break rendering or mislead readers.🧩 Suggested DOT fixes
- "Dispatch security reviewer subagent (./security-reviewer-prompt.md)?" [shape=diamond]; + "Dispatch security reviewer subagent (./security-reviewer-prompt.md)" [shape=box]; ... - "Dispatch security reviewer subagent (./security-reviewer-prompt.md)?" -> "Security reviewer subagent approves?" [shape=diamond]; + "Dispatch security reviewer subagent (./security-reviewer-prompt.md)" -> "Security reviewer subagent approves?";
🤖 Fix all issues with AI agents
In `@skills/executing-plans/SKILL.md`:
- Around line 23-25: The "Clear context" step may remove plan details created by
TodoWrite needed for Step 2; update SKILL.md to instruct preserving the plan
before clearing by saving the plan state (e.g., persist the TodoWrite output or
store a plan identifier) or explicitly reloading/reopening the plan after
clearing the window so Step 2 can access it; reference the TodoWrite action and
the Step 2 usage in the text and add a short sentence explaining how to
persist/reopen the plan (persist to storage or emit a plan token) immediately
before the "Tell Claude Code to clear its context/window" instruction.
In `@skills/requesting-code-review/security-reviewer.md`:
- Around line 45-51: Reword the "Input Validation:" checklist to remove
repetitive "No ..." starters for readability by changing each bullet to concise
negative-check phrases (e.g., "SQL injection risks?" -> "SQL injection risks
present?" or "Resistant to SQL injection?"; "command injection risks?" ->
"Resistant to command injection?"; "path traversal vulnerabilities?" -> "Path
traversal vulnerabilities present?"; "XSS prevention in place?" -> "XSS
prevention implemented?") while keeping the original intent and question format;
update the bullets under the "Input Validation" heading so they read
consistently and scan easily.
In `@skills/subagent-driven-development/SKILL.md`:
- Around line 194-200: The phrase "Two-stage review" under the "Quality gates:"
section is now incorrect; update the wording to "Three-stage review" and adjust
the bulleted sequence that currently reads "Two-stage review: spec compliance,
then code quality, then security (optional)" to explicitly reflect three stages
(spec compliance, code quality, security) and remove "(optional)" so it matches
the added security review; edit the "Quality gates:" block (the heading and its
bullets) to replace "Two-stage review" with "Three-stage review" and ensure the
bullets list the three stages in order.
Comment on lines
+23
to
+25
| 5. **Clear context** to free up memory for implementation: | ||
| - Tell Claude Code to clear its context/window | ||
| - This ensures more memory available during implementation and debugging |
There was a problem hiding this comment.
Clarify how task context is preserved before clearing context.
Clearing the window right after TodoWrite could drop plan details needed for Step 2. Consider adding a note to persist or re-open the plan after clearing context.
✏️ Suggested wording tweak
-5. **Clear context** to free up memory for implementation:
- - Tell Claude Code to clear its context/window
- - This ensures more memory available during implementation and debugging
+5. **Clear context** to free up memory for implementation:
+ - Ensure task details are persisted (TodoWrite + saved plan reference) before clearing
+ - Tell Claude Code to clear its context/window
+ - This ensures more memory available during implementation and debugging📝 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
| 5. **Clear context** to free up memory for implementation: | |
| - Tell Claude Code to clear its context/window | |
| - This ensures more memory available during implementation and debugging | |
| 5. **Clear context** to free up memory for implementation: | |
| - Ensure task details are persisted (TodoWrite + saved plan reference) before clearing | |
| - Tell Claude Code to clear its context/window | |
| - This ensures more memory available during implementation and debugging |
🤖 Prompt for AI Agents
In `@skills/executing-plans/SKILL.md` around lines 23 - 25, The "Clear context"
step may remove plan details created by TodoWrite needed for Step 2; update
SKILL.md to instruct preserving the plan before clearing by saving the plan
state (e.g., persist the TodoWrite output or store a plan identifier) or
explicitly reloading/reopening the plan after clearing the window so Step 2 can
access it; reference the TodoWrite action and the Step 2 usage in the text and
add a short sentence explaining how to persist/reopen the plan (persist to
storage or emit a plan token) immediately before the "Tell Claude Code to clear
its context/window" instruction.
Comment on lines
+45
to
+51
| **Input Validation:** | ||
| - All inputs validated? | ||
| - No SQL injection risks? | ||
| - No command injection risks? | ||
| - No path traversal vulnerabilities? | ||
| - XSS prevention in place? | ||
|
|
There was a problem hiding this comment.
Reduce repeated “No …” starters for readability.
LanguageTool flag is valid here; minor rephrasing will improve scanability.
✏️ Suggested wording tweak
-**Input Validation:**
-- All inputs validated?
-- No SQL injection risks?
-- No command injection risks?
-- No path traversal vulnerabilities?
-- XSS prevention in place?
+**Input Validation:**
+- All inputs validated?
+- SQL injection risks addressed?
+- Command injection risks addressed?
+- Path traversal protections in place?
+- XSS prevention in place?📝 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
| **Input Validation:** | |
| - All inputs validated? | |
| - No SQL injection risks? | |
| - No command injection risks? | |
| - No path traversal vulnerabilities? | |
| - XSS prevention in place? | |
| **Input Validation:** | |
| - All inputs validated? | |
| - SQL injection risks addressed? | |
| - Command injection risks addressed? | |
| - Path traversal protections in place? | |
| - XSS prevention in place? | |
🧰 Tools
🪛 LanguageTool
[style] ~49-~49: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... risks? - No command injection risks? - No path traversal vulnerabilities? - XSS p...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
In `@skills/requesting-code-review/security-reviewer.md` around lines 45 - 51,
Reword the "Input Validation:" checklist to remove repetitive "No ..." starters
for readability by changing each bullet to concise negative-check phrases (e.g.,
"SQL injection risks?" -> "SQL injection risks present?" or "Resistant to SQL
injection?"; "command injection risks?" -> "Resistant to command injection?";
"path traversal vulnerabilities?" -> "Path traversal vulnerabilities present?";
"XSS prevention in place?" -> "XSS prevention implemented?") while keeping the
original intent and question format; update the bullets under the "Input
Validation" heading so they read consistently and scan easily.
Comment on lines
194
to
+200
| **Quality gates:** | ||
| - Self-review catches issues before handoff | ||
| - Two-stage review: spec compliance, then code quality | ||
| - Two-stage review: spec compliance, then code quality, then security (optional) | ||
| - Review loops ensure fixes actually work | ||
| - Spec compliance prevents over/under-building | ||
| - Code quality ensures implementation is well-built | ||
| - Security review catches vulnerabilities in security-sensitive changes |
There was a problem hiding this comment.
Update wording to reflect three-stage review.
“Two-stage” is now inaccurate with the added security review.
✏️ Suggested wording tweak
-- Two-stage review: spec compliance, then code quality, then security (optional)
+- Three-stage review: spec compliance, then code quality, then security (optional)📝 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
| **Quality gates:** | |
| - Self-review catches issues before handoff | |
| - Two-stage review: spec compliance, then code quality | |
| - Two-stage review: spec compliance, then code quality, then security (optional) | |
| - Review loops ensure fixes actually work | |
| - Spec compliance prevents over/under-building | |
| - Code quality ensures implementation is well-built | |
| - Security review catches vulnerabilities in security-sensitive changes | |
| **Quality gates:** | |
| - Self-review catches issues before handoff | |
| - Three-stage review: spec compliance, then code quality, then security (optional) | |
| - Review loops ensure fixes actually work | |
| - Spec compliance prevents over/under-building | |
| - Code quality ensures implementation is well-built | |
| - Security review catches vulnerabilities in security-sensitive changes |
🤖 Prompt for AI Agents
In `@skills/subagent-driven-development/SKILL.md` around lines 194 - 200, The
phrase "Two-stage review" under the "Quality gates:" section is now incorrect;
update the wording to "Three-stage review" and adjust the bulleted sequence that
currently reads "Two-stage review: spec compliance, then code quality, then
security (optional)" to explicitly reflect three stages (spec compliance, code
quality, security) and remove "(optional)" so it matches the added security
review; edit the "Quality gates:" block (the heading and its bullets) to replace
"Two-stage review" with "Three-stage review" and ensure the bullets list the
three stages in order.
|
That would be amazing tool! |
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
2 participants
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
This addresses the issue that code review skill was not fully realized,
and adds dedicated security review to the implementation process.
Fixes #479
Summary by CodeRabbit
New Features
Documentation