docs(github-contributor): add high-quality PR formula and investigation workflow
- Add 10-point high-quality PR formula based on real-world success cases - Add investigation phase workflow (post to issue before PR) - Add git history tracing techniques (git log, git blame) - Add evidence-loop pattern (reproduce → trace → link → post) - Add high-quality PR case study reference - Update PR checklist with investigation steps - Emphasize separation of concerns (detailed analysis in issue, fix summary in PR) Key principles: - Deep investigation before coding - Minimal, surgical fixes - Professional communication - No internal/irrelevant details in PR Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
daymade
328
github-contributor/references/high_quality_pr_case_study.md
Normal file
328
github-contributor/references/high_quality_pr_case_study.md
Normal file
@@ -0,0 +1,328 @@
|
||||
# High-Quality PR: Real-World Case Study
|
||||
|
||||
Based on OpenClaw PR #39763 - A successful bug fix contribution to a 278K star TypeScript project.
|
||||
|
||||
## What Made This PR High-Quality
|
||||
|
||||
### 1. Complete Evidence Chain
|
||||
|
||||
**Issue → Root Cause → Fix → Validation**
|
||||
|
||||
```
|
||||
✅ Original bug report with symptoms
|
||||
✅ Deep investigation with timeline analysis
|
||||
✅ Root cause identified in source code
|
||||
✅ Minimal, surgical fix
|
||||
✅ End-to-end testing with before/after comparison
|
||||
✅ Regression test added
|
||||
```
|
||||
|
||||
### 2. Thorough Investigation Before Coding
|
||||
|
||||
**Timeline Analysis** (Posted to issue, not PR):
|
||||
- Traced bug through 3 years of related changes
|
||||
- Identified when workaround was added (#29078)
|
||||
- Explained why removing workaround is now safe
|
||||
- Linked to all relevant historical PRs and issues
|
||||
|
||||
**Key insight**: Detailed investigation goes in the issue, not the PR. Keep PR focused on the fix.
|
||||
|
||||
### 3. Minimal, Focused Changes
|
||||
|
||||
**Files changed**: 2
|
||||
- `src/infra/process-respawn.ts` - 3 lines removed, 1 line added
|
||||
- `src/infra/process-respawn.test.ts` - Updated tests + regression test
|
||||
|
||||
**What we didn't do**:
|
||||
- ❌ Refactor surrounding code
|
||||
- ❌ Add "improvements" beyond the fix
|
||||
- ❌ Change unrelated files
|
||||
- ❌ Add extensive comments
|
||||
|
||||
### 4. Regression Test Added
|
||||
|
||||
```typescript
|
||||
test("launchd path never returns failed status", () => {
|
||||
const result = detectSupervisor("launchd");
|
||||
expect(result.mode).not.toBe("failed");
|
||||
expect(result.mode).toBe("supervised");
|
||||
});
|
||||
```
|
||||
|
||||
**Why this matters**: Prevents the bug from being reintroduced.
|
||||
|
||||
### 5. CHANGELOG Entry
|
||||
|
||||
Following project conventions:
|
||||
```markdown
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
- **darwin/launchd**: Remove `kickstart -k` self-restart to prevent race condition with launchd bootout (#39763)
|
||||
```
|
||||
|
||||
**Key**: Check if project maintains CHANGELOG and follow their format exactly.
|
||||
|
||||
### 6. Clear PR Structure
|
||||
|
||||
**Title**: `fix(darwin): remove launchd kickstart race condition`
|
||||
- Conventional commit format
|
||||
- Scope indicates platform
|
||||
- Clear what was fixed
|
||||
|
||||
**Body** (~50 lines, trimmed from original 136):
|
||||
```markdown
|
||||
## Summary
|
||||
[2 sentences: what + why]
|
||||
|
||||
## Root Cause
|
||||
[Technical explanation with code references]
|
||||
|
||||
## Changes
|
||||
- [Bullet list of actual changes]
|
||||
|
||||
## Why This Is Safe
|
||||
[Explain why the fix won't break anything]
|
||||
|
||||
## Testing
|
||||
[How it was validated]
|
||||
|
||||
## Related
|
||||
- Fixes #39760
|
||||
- Related: #27650, #29078
|
||||
```
|
||||
|
||||
### 7. Separation of Concerns
|
||||
|
||||
**Issue comment**: Detailed timeline, investigation, evidence
|
||||
**PR description**: Focused on the fix, testing, safety
|
||||
**Separate test comment**: End-to-end validation results
|
||||
|
||||
**Why**: Keeps PR reviewable. Detailed context available but not blocking review.
|
||||
|
||||
### 8. End-to-End Testing
|
||||
|
||||
**Test 1**: Reproduced bug with original version
|
||||
```
|
||||
Result: Bootstrap failed: 5, SIGKILL, exit code -9 ✅
|
||||
```
|
||||
|
||||
**Test 2**: Validated fix with patched version
|
||||
```
|
||||
Result: Clean restart, no errors, normal exit code ✅
|
||||
```
|
||||
|
||||
**Evidence**: Posted full logs with timestamps, PIDs, exit codes.
|
||||
|
||||
### 9. What We Avoided
|
||||
|
||||
**❌ Don't mention internal tooling**:
|
||||
- We had a custom monitor script that auto-remediated the bug
|
||||
- We initially mentioned it in PR comments
|
||||
- **Removed it** because it's not part of OpenClaw - would confuse maintainers
|
||||
|
||||
**❌ Don't over-explain in PR**:
|
||||
- Moved detailed timeline analysis to issue
|
||||
- Kept PR focused on fix validation
|
||||
|
||||
**❌ Don't add noise**:
|
||||
- No "I think this might work" comments
|
||||
- No "please review" pings
|
||||
- No unnecessary updates
|
||||
|
||||
### 10. Professional Communication
|
||||
|
||||
**In issue**:
|
||||
```markdown
|
||||
## Timeline Analysis
|
||||
|
||||
I traced this through the codebase history:
|
||||
|
||||
1. 2023-05: #27650 set ThrottleInterval to 60s
|
||||
2. 2023-08: #29078 added kickstart workaround
|
||||
3. Later: ThrottleInterval reduced to 1s
|
||||
4. Now: Safe to remove kickstart
|
||||
|
||||
[Detailed evidence with links]
|
||||
```
|
||||
|
||||
**In PR**:
|
||||
```markdown
|
||||
## Testing Complete ✅
|
||||
|
||||
End-to-end testing confirms:
|
||||
1. Bug reproduced with 2026.3.7
|
||||
2. Fix validated with PR branch
|
||||
3. Ready for review
|
||||
|
||||
Full logs: [link to issue comment]
|
||||
```
|
||||
|
||||
## The High-Quality PR Formula
|
||||
|
||||
```
|
||||
1. Deep investigation (post to issue)
|
||||
2. Minimal, surgical fix
|
||||
3. Regression test
|
||||
4. CHANGELOG entry (if project uses it)
|
||||
5. End-to-end validation
|
||||
6. Clear PR structure
|
||||
7. Professional communication
|
||||
8. Separate concerns (issue vs PR)
|
||||
9. No internal/irrelevant details
|
||||
10. Responsive to feedback
|
||||
```
|
||||
|
||||
## PR Lifecycle
|
||||
|
||||
```
|
||||
Day 1: Investigation
|
||||
├─ Reproduce bug locally
|
||||
├─ Trace through codebase history
|
||||
├─ Identify root cause
|
||||
└─ Post detailed analysis to issue
|
||||
|
||||
Day 2: Implementation
|
||||
├─ Create minimal fix
|
||||
├─ Add regression test
|
||||
├─ Update CHANGELOG
|
||||
└─ Test locally
|
||||
|
||||
Day 3: Validation
|
||||
├─ Test with original version (reproduce bug)
|
||||
├─ Test with fixed version (validate fix)
|
||||
├─ Document test results
|
||||
└─ Submit PR
|
||||
|
||||
Day 4: Refinement
|
||||
├─ Trim PR description (move details to issue)
|
||||
├─ Add context about historical changes
|
||||
├─ Post end-to-end test results
|
||||
└─ Mark ready for review
|
||||
|
||||
Day 5+: Review cycle
|
||||
├─ Respond to feedback promptly
|
||||
├─ Make requested changes
|
||||
└─ Wait for CI and approval
|
||||
```
|
||||
|
||||
## Key Metrics
|
||||
|
||||
**OpenClaw PR #39763**:
|
||||
- Files changed: 2
|
||||
- Lines added: ~20 (including tests)
|
||||
- Lines removed: 3
|
||||
- PR description: ~50 lines
|
||||
- Issue investigation: ~200 lines
|
||||
- Time to first draft: 3 days
|
||||
- Time to ready: 4 days
|
||||
|
||||
## What Maintainers Look For
|
||||
|
||||
Based on this experience:
|
||||
|
||||
1. **Does it fix the problem?** ✅ Bug reproduced and fixed
|
||||
2. **Is it minimal?** ✅ Only changed what's necessary
|
||||
3. **Will it break anything?** ✅ Explained why it's safe
|
||||
4. **Can it regress?** ✅ Added regression test
|
||||
5. **Is it documented?** ✅ CHANGELOG entry
|
||||
6. **Is it tested?** ✅ End-to-end validation
|
||||
7. **Is it reviewable?** ✅ Clear structure, focused scope
|
||||
|
||||
## Anti-Patterns We Avoided
|
||||
|
||||
1. ❌ **Drive-by PR**: "Here's a fix, hope it works"
|
||||
- ✅ We did: Deep investigation, thorough testing
|
||||
|
||||
2. ❌ **Kitchen sink PR**: "Fixed bug + refactored + added features"
|
||||
- ✅ We did: Minimal, focused fix only
|
||||
|
||||
3. ❌ **No evidence PR**: "Trust me, it works"
|
||||
- ✅ We did: Reproduced bug, validated fix, posted logs
|
||||
|
||||
4. ❌ **Wall of text PR**: 500-line description
|
||||
- ✅ We did: Trimmed to ~50 lines, moved details to issue
|
||||
|
||||
5. ❌ **Ghost PR**: Submit and disappear
|
||||
- ✅ We did: Responsive, iterative refinement
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
### Investigation Phase
|
||||
- Trace through git history to understand context
|
||||
- Link to all related issues and PRs
|
||||
- Post detailed analysis to issue, not PR
|
||||
|
||||
### Implementation Phase
|
||||
- Make the smallest possible change
|
||||
- Add regression test
|
||||
- Follow project conventions exactly
|
||||
|
||||
### Validation Phase
|
||||
- Test with original version (prove bug exists)
|
||||
- Test with fixed version (prove fix works)
|
||||
- Document both with timestamps and logs
|
||||
|
||||
### Communication Phase
|
||||
- Keep PR focused and reviewable
|
||||
- Move detailed context to issue
|
||||
- Remove internal/irrelevant details
|
||||
- Be professional and responsive
|
||||
|
||||
## Template for Future PRs
|
||||
|
||||
```markdown
|
||||
## Summary
|
||||
[1-2 sentences: what this fixes and why]
|
||||
|
||||
## Root Cause
|
||||
[Technical explanation with code references]
|
||||
|
||||
## Changes
|
||||
- [Actual code changes]
|
||||
- [Tests added]
|
||||
- [Docs updated]
|
||||
|
||||
## Why This Is Safe
|
||||
[Explain why it won't break anything]
|
||||
|
||||
## Testing
|
||||
[How you validated the fix]
|
||||
|
||||
### Test 1: Reproduce Bug
|
||||
Command: `[command]`
|
||||
Result: [failure output]
|
||||
|
||||
### Test 2: Validate Fix
|
||||
Command: `[same command]`
|
||||
Result: [success output]
|
||||
|
||||
## Related
|
||||
- Fixes #[issue]
|
||||
- Related: #[other issues]
|
||||
```
|
||||
|
||||
## Success Indicators
|
||||
|
||||
You know you have a high-quality PR when:
|
||||
|
||||
- ✅ Maintainers understand the problem immediately
|
||||
- ✅ Reviewers can verify the fix easily
|
||||
- ✅ CI passes on first try
|
||||
- ✅ No "can you explain..." questions
|
||||
- ✅ Minimal back-and-forth
|
||||
- ✅ Quick approval
|
||||
|
||||
## Final Checklist
|
||||
|
||||
Before submitting:
|
||||
- [ ] Bug reproduced with original version
|
||||
- [ ] Fix validated with patched version
|
||||
- [ ] Regression test added
|
||||
- [ ] CHANGELOG updated (if applicable)
|
||||
- [ ] PR description is focused (~50 lines)
|
||||
- [ ] Detailed investigation in issue, not PR
|
||||
- [ ] No internal tooling mentioned
|
||||
- [ ] No local paths/secrets in logs
|
||||
- [ ] All tests pass
|
||||
- [ ] Follows project conventions
|
||||
Reference in New Issue
Block a user