mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
refactor: enhance software-development skills with Hermes integration
Improvements to all 5 skills adapted from obra/superpowers: - Restored anti-rationalization tables and red flags from originals (key behavioral guardrails that prevent LLMs from taking shortcuts) - Restored 'Rule of Three' for debugging (3+ failed fixes = question architecture, not keep fixing) - Restored Pattern Analysis and Hypothesis Testing phases in debugging - Restored 'Why Order Matters' rebuttals and verification checklist in TDD - Added proper Hermes delegate_task integration with real parameter examples and toolset specifications throughout - Added Hermes tool usage (search_files, read_file, terminal) for investigation and verification steps - Removed references to non-existent skills (brainstorming, finishing-a-development-branch, executing-plans, using-git-worktrees) - Removed generic language-specific sections (Go, Rust, Jest) that added bulk without agent value - Tightened prose — cut ~430 lines while adding more actionable content - Added execution handoff section to writing-plans - Consistent cross-references between the 5 skills
This commit is contained in:
parent
0e1723ef74
commit
de0af4df66
5 changed files with 879 additions and 1312 deletions
|
|
@ -1,8 +1,8 @@
|
|||
---
|
||||
name: requesting-code-review
|
||||
description: Use when completing tasks, implementing major features, or before merging. Validates work meets requirements through systematic review process.
|
||||
version: 1.0.0
|
||||
author: Hermes Agent (adapted from Superpowers)
|
||||
version: 1.1.0
|
||||
author: Hermes Agent (adapted from obra/superpowers)
|
||||
license: MIT
|
||||
metadata:
|
||||
hermes:
|
||||
|
|
@ -14,108 +14,102 @@ metadata:
|
|||
|
||||
## Overview
|
||||
|
||||
Systematic code review catches issues before they cascade. Review early, review often.
|
||||
Dispatch a reviewer subagent to catch issues before they cascade. Review early, review often.
|
||||
|
||||
**Core principle:** Fresh perspective finds issues you'll miss.
|
||||
|
||||
## When to Request Review
|
||||
|
||||
**Mandatory Reviews:**
|
||||
**Mandatory:**
|
||||
- After each task in subagent-driven development
|
||||
- After completing major features
|
||||
- After completing a major feature
|
||||
- Before merge to main
|
||||
- After bug fixes
|
||||
|
||||
**Optional but Valuable:**
|
||||
**Optional but valuable:**
|
||||
- When stuck (fresh perspective)
|
||||
- Before refactoring (baseline check)
|
||||
- After complex logic implementation
|
||||
- When touching critical code (auth, payments, data)
|
||||
|
||||
**Don't skip because:**
|
||||
- "It's simple" (simple bugs compound)
|
||||
- "I'm in a hurry" (reviews save time)
|
||||
- "I tested it" (you have blind spots)
|
||||
**Never skip because:**
|
||||
- "It's simple" — simple bugs compound
|
||||
- "I'm in a hurry" — reviews save time
|
||||
- "I tested it" — you have blind spots
|
||||
|
||||
## Review Process
|
||||
|
||||
### Step 1: Prepare Context
|
||||
### Step 1: Self-Review First
|
||||
|
||||
Gather:
|
||||
- What was implemented
|
||||
- Original requirements/plan
|
||||
- Files changed
|
||||
- Test results
|
||||
Before dispatching a reviewer, check yourself:
|
||||
|
||||
```bash
|
||||
# Get changed files
|
||||
git diff --name-only HEAD~1
|
||||
|
||||
# Get diff summary
|
||||
git diff --stat HEAD~1
|
||||
|
||||
# Get commit messages
|
||||
git log --oneline HEAD~5
|
||||
```
|
||||
|
||||
### Step 2: Self-Review First
|
||||
|
||||
Before requesting external review:
|
||||
|
||||
**Checklist:**
|
||||
- [ ] Code follows project conventions
|
||||
- [ ] All tests pass
|
||||
- [ ] No debug print statements
|
||||
- [ ] No hardcoded secrets
|
||||
- [ ] No debug print statements left
|
||||
- [ ] No hardcoded secrets or credentials
|
||||
- [ ] Error handling in place
|
||||
- [ ] Documentation updated
|
||||
- [ ] Commit messages are clear
|
||||
|
||||
```bash
|
||||
# Run full test suite
|
||||
pytest
|
||||
pytest tests/ -q
|
||||
|
||||
# Check for debug code
|
||||
grep -r "print(" src/ --include="*.py"
|
||||
grep -r "debugger" src/ --include="*.js"
|
||||
search_files("print(", path="src/", file_glob="*.py")
|
||||
search_files("console.log", path="src/", file_glob="*.js")
|
||||
|
||||
# Check for TODOs
|
||||
grep -r "TODO" src/ --include="*.py"
|
||||
search_files("TODO|FIXME|HACK", path="src/")
|
||||
```
|
||||
|
||||
### Step 3: Request Review
|
||||
### Step 2: Gather Context
|
||||
|
||||
Use `delegate_task` to dispatch a reviewer subagent:
|
||||
```bash
|
||||
# Changed files
|
||||
git diff --name-only HEAD~1
|
||||
|
||||
# Diff summary
|
||||
git diff --stat HEAD~1
|
||||
|
||||
# Recent commits
|
||||
git log --oneline -5
|
||||
```
|
||||
|
||||
### Step 3: Dispatch Reviewer Subagent
|
||||
|
||||
Use `delegate_task` to dispatch a focused reviewer:
|
||||
|
||||
```python
|
||||
delegate_task(
|
||||
goal="Review implementation for quality and correctness",
|
||||
goal="Review implementation for correctness and quality",
|
||||
context="""
|
||||
WHAT WAS IMPLEMENTED: [Brief description]
|
||||
|
||||
ORIGINAL REQUIREMENTS: [From plan or issue]
|
||||
|
||||
WHAT WAS IMPLEMENTED:
|
||||
[Brief description of the feature/fix]
|
||||
|
||||
ORIGINAL REQUIREMENTS:
|
||||
[From plan, issue, or user request]
|
||||
|
||||
FILES CHANGED:
|
||||
- src/feature.py (added X function)
|
||||
- tests/test_feature.py (added tests)
|
||||
|
||||
COMMIT RANGE: [SHA range or branch]
|
||||
|
||||
CHECK FOR:
|
||||
- Correctness (does it do what it should?)
|
||||
- Edge cases handled?
|
||||
- Error handling adequate?
|
||||
- Code quality and style
|
||||
- Test coverage
|
||||
- Security issues
|
||||
- Performance concerns
|
||||
|
||||
- src/models/user.py (added User class)
|
||||
- src/auth/login.py (added login endpoint)
|
||||
- tests/test_auth.py (added 8 tests)
|
||||
|
||||
REVIEW CHECKLIST:
|
||||
- [ ] Correctness: Does it do what it should?
|
||||
- [ ] Edge cases: Are they handled?
|
||||
- [ ] Error handling: Is it adequate?
|
||||
- [ ] Code quality: Clear names, good structure?
|
||||
- [ ] Test coverage: Are tests meaningful?
|
||||
- [ ] Security: Any vulnerabilities?
|
||||
- [ ] Performance: Any obvious issues?
|
||||
|
||||
OUTPUT FORMAT:
|
||||
- Summary: [brief assessment]
|
||||
- Critical Issues: [must fix]
|
||||
- Important Issues: [should fix]
|
||||
- Critical Issues: [must fix — blocks merge]
|
||||
- Important Issues: [should fix before merge]
|
||||
- Minor Issues: [nice to have]
|
||||
- Verdict: [APPROVE / REQUEST_CHANGES / NEEDS_WORK]
|
||||
- Strengths: [what was done well]
|
||||
- Verdict: APPROVE / REQUEST_CHANGES
|
||||
""",
|
||||
toolsets=['file']
|
||||
)
|
||||
|
|
@ -123,110 +117,72 @@ delegate_task(
|
|||
|
||||
### Step 4: Act on Feedback
|
||||
|
||||
**Critical Issues (Block merge):**
|
||||
**Critical Issues (block merge):**
|
||||
- Security vulnerabilities
|
||||
- Broken functionality
|
||||
- Data loss risk
|
||||
- Test failures
|
||||
- **Action:** Fix immediately before proceeding
|
||||
|
||||
**Action:** Fix immediately before proceeding
|
||||
|
||||
**Important Issues (Should fix):**
|
||||
**Important Issues (should fix):**
|
||||
- Missing edge case handling
|
||||
- Poor error messages
|
||||
- Unclear code
|
||||
- Missing tests
|
||||
- **Action:** Fix before merge if possible
|
||||
|
||||
**Action:** Fix before merge if possible
|
||||
|
||||
**Minor Issues (Nice to have):**
|
||||
**Minor Issues (nice to have):**
|
||||
- Style preferences
|
||||
- Refactoring suggestions
|
||||
- Documentation improvements
|
||||
- **Action:** Note for later or quick fix
|
||||
|
||||
**Action:** Note for later or quick fix
|
||||
**If reviewer is wrong:**
|
||||
- Push back with technical reasoning
|
||||
- Show code/tests that prove it works
|
||||
- Request clarification
|
||||
|
||||
## Review Dimensions
|
||||
|
||||
### Correctness
|
||||
|
||||
**Questions:**
|
||||
- Does it implement the requirements?
|
||||
- Are there logic errors?
|
||||
- Do edge cases work?
|
||||
- Are there race conditions?
|
||||
|
||||
**Check:**
|
||||
- Read implementation against requirements
|
||||
- Trace through edge cases
|
||||
- Check boundary conditions
|
||||
|
||||
### Code Quality
|
||||
|
||||
**Questions:**
|
||||
- Is code readable?
|
||||
- Are names clear?
|
||||
- Is it too complex?
|
||||
- Are names clear and descriptive?
|
||||
- Is it too complex? (Functions >20 lines = smell)
|
||||
- Is there duplication?
|
||||
|
||||
**Check:**
|
||||
- Function length (aim <20 lines)
|
||||
- Cyclomatic complexity
|
||||
- DRY violations
|
||||
- Naming clarity
|
||||
|
||||
### Testing
|
||||
|
||||
**Questions:**
|
||||
- Are there tests?
|
||||
- Are there meaningful tests?
|
||||
- Do they cover edge cases?
|
||||
- Are they meaningful?
|
||||
- Do they pass?
|
||||
|
||||
**Check:**
|
||||
- Test coverage
|
||||
- Edge case coverage
|
||||
- Test clarity
|
||||
- Assertion quality
|
||||
- Do they test behavior, not implementation?
|
||||
- Do all tests pass?
|
||||
|
||||
### Security
|
||||
|
||||
**Questions:**
|
||||
- Any injection vulnerabilities?
|
||||
- Proper input validation?
|
||||
- Secrets handled correctly?
|
||||
- Access control in place?
|
||||
|
||||
**Check:**
|
||||
- Input sanitization
|
||||
- Authentication/authorization
|
||||
- Secret management
|
||||
- SQL/query safety
|
||||
|
||||
### Performance
|
||||
|
||||
**Questions:**
|
||||
- Any N+1 queries?
|
||||
- Unnecessary computation?
|
||||
- Unnecessary computation in loops?
|
||||
- Memory leaks?
|
||||
- Scalability concerns?
|
||||
|
||||
**Check:**
|
||||
- Database queries
|
||||
- Algorithm complexity
|
||||
- Resource usage
|
||||
- Caching opportunities
|
||||
- Missing caching opportunities?
|
||||
|
||||
## Review Output Format
|
||||
|
||||
Standard review format:
|
||||
Standard format for reviewer subagent output:
|
||||
|
||||
```markdown
|
||||
## Review Summary
|
||||
|
||||
**Assessment:** [Brief overall assessment]
|
||||
|
||||
**Verdict:** [APPROVE / REQUEST_CHANGES / NEEDS_WORK]
|
||||
**Verdict:** APPROVE / REQUEST_CHANGES
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -237,8 +193,6 @@ Standard review format:
|
|||
- Problem: [Description]
|
||||
- Suggestion: [How to fix]
|
||||
|
||||
---
|
||||
|
||||
## Important Issues (Should Fix)
|
||||
|
||||
1. **[Issue title]**
|
||||
|
|
@ -246,15 +200,11 @@ Standard review format:
|
|||
- Problem: [Description]
|
||||
- Suggestion: [How to fix]
|
||||
|
||||
---
|
||||
|
||||
## Minor Issues (Optional)
|
||||
|
||||
1. **[Issue title]**
|
||||
- Suggestion: [Improvement idea]
|
||||
|
||||
---
|
||||
|
||||
## Strengths
|
||||
|
||||
- [What was done well]
|
||||
|
|
@ -264,111 +214,41 @@ Standard review format:
|
|||
|
||||
### With subagent-driven-development
|
||||
|
||||
Review after EACH task:
|
||||
1. Subagent implements task
|
||||
2. Request code review
|
||||
3. Fix issues
|
||||
4. Proceed to next task
|
||||
Review after EACH task — this is the two-stage review:
|
||||
1. Spec compliance review (does it match the plan?)
|
||||
2. Code quality review (is it well-built?)
|
||||
3. Fix issues from either review
|
||||
4. Proceed to next task only when both approve
|
||||
|
||||
### With test-driven-development
|
||||
|
||||
Review checks:
|
||||
- Tests written first?
|
||||
- Tests are meaningful?
|
||||
Review verifies:
|
||||
- Tests were written first (RED-GREEN-REFACTOR followed?)
|
||||
- Tests are meaningful (not just asserting True)?
|
||||
- Edge cases covered?
|
||||
- All tests pass?
|
||||
|
||||
### With writing-plans
|
||||
|
||||
Review validates:
|
||||
- Implementation matches plan?
|
||||
- Implementation matches the plan?
|
||||
- All tasks completed?
|
||||
- Quality standards met?
|
||||
|
||||
## Common Review Patterns
|
||||
## Red Flags
|
||||
|
||||
### Pre-Merge Review
|
||||
|
||||
Before merging feature branch:
|
||||
|
||||
```bash
|
||||
# Create review checkpoint
|
||||
git log --oneline main..feature-branch
|
||||
|
||||
# Get summary of changes
|
||||
git diff --stat main..feature-branch
|
||||
|
||||
# Request review
|
||||
delegate_task(
|
||||
goal="Pre-merge review of feature branch",
|
||||
context="[changes, requirements, test results]"
|
||||
)
|
||||
|
||||
# Address feedback
|
||||
# Merge when approved
|
||||
```
|
||||
|
||||
### Continuous Review
|
||||
|
||||
During subagent-driven development:
|
||||
|
||||
```python
|
||||
# After each task
|
||||
if task_complete:
|
||||
review_result = request_review(task)
|
||||
if review_result.has_critical_issues():
|
||||
fix_issues(review_result.critical)
|
||||
re_review()
|
||||
proceed_to_next_task()
|
||||
```
|
||||
|
||||
### Emergency Review
|
||||
|
||||
When fixing production bugs:
|
||||
|
||||
1. Fix with tests
|
||||
2. Self-review
|
||||
3. Quick peer review
|
||||
4. Deploy
|
||||
5. Full review post-deploy
|
||||
|
||||
## Review Best Practices
|
||||
|
||||
### As Review Requester
|
||||
|
||||
**Do:**
|
||||
- Provide complete context
|
||||
- Highlight areas of concern
|
||||
- Ask specific questions
|
||||
- Be responsive to feedback
|
||||
- Fix issues promptly
|
||||
|
||||
**Don't:**
|
||||
- Rush the reviewer
|
||||
- Argue without evidence
|
||||
- Ignore feedback
|
||||
- Take criticism personally
|
||||
|
||||
### As Reviewer (via subagent)
|
||||
|
||||
**Do:**
|
||||
- Be specific about issues
|
||||
- Suggest improvements
|
||||
- Acknowledge what works
|
||||
- Prioritize issues
|
||||
|
||||
**Don't:**
|
||||
- Nitpick style (unless project requires)
|
||||
- Make vague comments
|
||||
- Block without explanation
|
||||
- Be overly critical
|
||||
**Never:**
|
||||
- Skip review because "it's simple"
|
||||
- Ignore Critical issues
|
||||
- Proceed with unfixed Important issues
|
||||
- Argue with valid technical feedback without evidence
|
||||
|
||||
## Quality Gates
|
||||
|
||||
**Must pass before merge:**
|
||||
- [ ] No critical issues
|
||||
- [ ] All tests pass
|
||||
- [ ] Review approved
|
||||
- [ ] Review verdict: APPROVE
|
||||
- [ ] Requirements met
|
||||
|
||||
**Should pass before merge:**
|
||||
|
|
@ -376,10 +256,6 @@ When fixing production bugs:
|
|||
- [ ] Documentation updated
|
||||
- [ ] Performance acceptable
|
||||
|
||||
**Nice to have:**
|
||||
- [ ] No minor issues
|
||||
- [ ] Extra polish
|
||||
|
||||
## Remember
|
||||
|
||||
```
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue