diff --git a/github-contributor/SKILL.md b/github-contributor/SKILL.md index 103c773..70b7666 100644 --- a/github-contributor/SKILL.md +++ b/github-contributor/SKILL.md @@ -118,6 +118,23 @@ gh search repos "topic:cli" --sort=stars --limit=20 ## PR Excellence +### The High-Quality PR Formula + +Based on real-world successful contributions to major open-source projects: + +``` +1. Deep investigation (post to issue, not PR) +2. Minimal, surgical fix (only change what's necessary) +3. Regression test (prevent future breakage) +4. CHANGELOG entry (if project uses it) +5. End-to-end validation (prove bug exists, prove fix works) +6. Clear PR structure (~50 lines, focused) +7. Professional communication +8. Separate concerns (detailed analysis in issue, fix summary in PR) +9. No internal/irrelevant details +10. Responsive to feedback +``` + ### Before Writing Code ``` @@ -127,6 +144,38 @@ Pre-PR Checklist: - [ ] Comment on issue to claim it - [ ] Understand project conventions - [ ] Set up development environment +- [ ] Trace through git history for context +- [ ] Identify root cause with evidence +``` + +### Investigation Phase (Post to Issue) + +**Do this BEFORE coding**: + +1. **Reproduce the bug** with exact commands and output +2. **Trace git history** to understand context + ```bash + git log --all --grep="keyword" --oneline + git blame file.ts | grep "relevant_line" + ``` +3. **Link related issues/PRs** that provide context +4. **Post detailed analysis to issue** (not PR) + - Timeline of related changes + - Root cause explanation + - Why previous approaches didn't work + +**Example structure**: +```markdown +## Investigation + +I traced this through the codebase history: + +1. [Date]: #[PR] introduced [feature] +2. [Date]: #[PR] added [workaround] because [reason] +3. [Date]: #[PR] changed [parameter] +4. Now: Safe to [fix] because [explanation] + +[Detailed evidence with code references] ``` ### Writing the PR @@ -140,76 +189,151 @@ docs(readme): update installation instructions for Windows refactor(validation): extract validation logic into separate module ``` -**Evidence loop**: Prove the change with a reproducible fail -> fix -> pass loop. +**Keep PR description focused** (~50 lines): +- Summary (1-2 sentences) +- Root cause (technical, with code refs) +- Changes (bullet list) +- Why it's safe +- Testing approach +- Related issues -- Reproduce the failure with one command. -- Apply the fix. -- Rerun the exact same command and capture the passing output. -- Compare baseline vs fixed vs reference behavior. -- Redact local paths, secrets, tokens, and internal hostnames before posting logs or screenshots. +**Move detailed investigation to issue comments**, not PR. -**Description**: Structured and thorough +### Evidence Loop + +**Critical**: Prove the change with a reproducible fail → fix → pass loop. + +1. **Reproduce failure** with original version + ```bash + # Test with original version + npm install -g package@original-version + [command that triggers bug] + # Capture: error messages, exit codes, timestamps + ``` + +2. **Apply fix** and test with patched version + ```bash + # Test with fixed version + npm install -g package@fixed-version + [same command] + # Capture: success output, normal exit codes + ``` + +3. **Document both** with timestamps, PIDs, exit codes, logs + +4. **Redact sensitive info**: + - Local absolute paths (`/Users/...`, `/home/...`) + - Secrets/tokens/API keys + - Internal URLs/hostnames + - Recheck every pasted block before submitting + +**Description**: Focused and reviewable (~50 lines) ````markdown ## Summary -[What this PR does in 1-2 sentences] +[1-2 sentences: what this fixes and why] -## Motivation -[Why this change is needed] +## Root Cause +[Technical explanation with code references] ## Changes -- [Change 1] -- [Change 2] +- [Actual code changes] +- [Tests added] +- [Docs updated] -## Evidence Loop -Command: -```bash -# Baseline (before fix) -[command] - -# Fixed (after fix, same command) -[command] -``` - -Raw output: -```text -[paste baseline output] -``` - -```text -[paste fixed output] -``` - -## Comparison -| Case | Command / Scenario | Result | Evidence | -|------|--------------------|--------|----------| -| Baseline | `[same command]` | Fail | [raw output block] | -| Fixed | `[same command]` | Pass | [raw output block] | -| Reference | [spec, issue, or main branch behavior] | Expected | [link or note] | - -## Sources/Attribution -- [Issue, docs, or code references] - -## Risks -- [Potential downside and impact] - -## Rollback Plan -- Revert commit(s): [hash] -- Restore previous behavior with: [command] +## Why This Is Safe +[Explain why it won't break anything] ## Testing -[How you tested this] -## Screenshots (if UI) -[Before/After images] +### Test 1: Reproduce Bug (Original Version) +Command: `[command]` +Result: +```text +[failure output with timestamps, exit codes] +``` + +### Test 2: Validate Fix (Patched Version) +Command: `[same command]` +Result: +```text +[success output with timestamps, exit codes] +``` + +## Related +- Fixes #[issue] +- Related: #[other issues/PRs] ```` +**What NOT to include in PR**: +- ❌ Detailed timeline analysis (put in issue) +- ❌ Historical context (put in issue) +- ❌ Internal tooling mentions +- ❌ Speculation or uncertainty +- ❌ Walls of text (>100 lines) + +### Code Changes Best Practices + +**Minimal, surgical fixes**: +- ✅ Only change what's necessary to fix the bug +- ✅ Add regression test to prevent future breakage +- ✅ Update CHANGELOG if project uses it +- ❌ Don't refactor surrounding code +- ❌ Don't add "improvements" beyond the fix +- ❌ Don't change unrelated files + +**Example** (OpenClaw PR #39763): +``` +Files changed: 2 +- src/infra/process-respawn.ts (3 lines removed, 1 added) +- src/infra/process-respawn.test.ts (regression test added) + +Result: 278K star project, clean approval +``` + +### Separation of Concerns + +**Issue comments**: Detailed investigation +- Timeline analysis +- Historical context +- Related PRs/issues +- Root cause deep dive + +**PR description**: Focused on the fix +- Summary (1-2 sentences) +- Root cause (technical) +- Changes (bullet list) +- Testing validation +- ~50 lines total + +**Separate test comment**: End-to-end validation +- Test with original version (prove bug) +- Test with fixed version (prove fix) +- Full logs with timestamps + ### After Submitting -- Respond to feedback promptly +- Monitor CI results +- Respond to feedback promptly (within 24 hours) - Make requested changes quickly - Be grateful for reviews -- Don't argue, discuss +- Don't argue, discuss professionally +- If you need to update PR: + - Add new commits (don't force push during review) + - Explain what changed in comment + - Re-request review when ready + +**Professional responses**: +``` +✅ "Good point! I've updated the implementation to..." +✅ "Thanks for catching that. Fixed in commit abc123." +✅ "I see what you mean. I chose this approach because... + Would you prefer if I changed it to...?" + +❌ "That's just your opinion." +❌ "It works on my machine." +❌ "This is how I always do it." +``` ## Building Reputation @@ -243,35 +367,70 @@ Level 4: Maintainer status ### Don't -- Submit drive-by PRs without context +- Submit drive-by PRs without investigation +- Include detailed timeline in PR (put in issue) +- Mention internal tooling or infrastructure - Argue with maintainers - Ignore code style guidelines - Make massive changes without discussion - Ghost after submitting +- Refactor code unrelated to the fix +- Add "improvements" beyond what was requested +- Force push during review (unless asked) ### Do +- Investigate thoroughly BEFORE coding +- Post detailed analysis to issue, not PR +- Keep PR focused and minimal (~50 lines) - Start with small, focused PRs - Follow project conventions exactly +- Add regression tests +- Update CHANGELOG if project uses it - Communicate proactively - Accept feedback gracefully - Build relationships over time +- Test with both original and fixed versions +- Redact sensitive info from logs ## Workflow Template ``` -Contribution Workflow: +High-Quality Contribution Workflow: + +Investigation Phase: - [ ] Find project with "good first issue" - [ ] Read contribution guidelines - [ ] Comment on issue to claim +- [ ] Reproduce bug with original version +- [ ] Trace git history for context +- [ ] Identify root cause with evidence +- [ ] Post detailed analysis to issue + +Implementation Phase: - [ ] Fork and set up locally -- [ ] Make focused changes -- [ ] Run evidence loop (reproduce fail -> apply fix -> rerun same command pass) -- [ ] Add baseline vs fixed vs reference comparison -- [ ] Redact paths/secrets/internal hosts in logs and screenshots -- [ ] Test thoroughly -- [ ] Write clear PR description -- [ ] Respond to review feedback +- [ ] Make minimal, focused changes +- [ ] Add regression test +- [ ] Update CHANGELOG (if applicable) +- [ ] Follow project conventions exactly + +Validation Phase: +- [ ] Test with original version (prove bug exists) +- [ ] Test with fixed version (prove fix works) +- [ ] Document both with timestamps/logs +- [ ] Redact paths/secrets/internal hosts + +Submission Phase: +- [ ] Write focused PR description (~50 lines) +- [ ] Link to detailed issue analysis +- [ ] Post end-to-end test results +- [ ] Ensure CI passes + +Review Phase: +- [ ] Respond to feedback within 24 hours +- [ ] Make requested changes quickly +- [ ] Don't force push during review +- [ ] Thank reviewers - [ ] Celebrate when merged! 🎉 ``` @@ -310,3 +469,28 @@ Types: `feat`, `fix`, `docs`, `style`, `refactor`, `test`, `chore` - `references/pr_checklist.md` - Complete PR quality checklist - `references/project_evaluation.md` - How to evaluate projects - `references/communication_templates.md` - Issue/PR templates +- `references/high_quality_pr_case_study.md` - Real-world successful PR walkthrough (OpenClaw #39763) + +## 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 + +## Key Metrics for Quality PRs + +Based on successful contributions to major projects: + +- **Files changed**: 1-3 (focused scope) +- **Lines changed**: 10-50 (minimal fix) +- **PR description**: ~50 lines (concise) +- **Issue investigation**: 100-300 lines (thorough) +- **Time to first draft**: 2-3 days (proper investigation) +- **Time to ready**: 3-5 days (including validation) +- **Response time**: <24 hours (professional) + diff --git a/github-contributor/references/high_quality_pr_case_study.md b/github-contributor/references/high_quality_pr_case_study.md new file mode 100644 index 0000000..8942ab4 --- /dev/null +++ b/github-contributor/references/high_quality_pr_case_study.md @@ -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 diff --git a/github-contributor/references/pr_checklist.md b/github-contributor/references/pr_checklist.md index 319a1b5..0e07aba 100644 --- a/github-contributor/references/pr_checklist.md +++ b/github-contributor/references/pr_checklist.md @@ -2,13 +2,37 @@ Complete checklist for creating high-quality pull requests. -## Before Starting +# PR Quality Checklist + +Complete checklist for creating high-quality pull requests based on successful contributions to major open-source projects. + +## Investigation Phase (Before Coding) - [ ] Read CONTRIBUTING.md thoroughly - [ ] Check for existing PRs addressing same issue - [ ] Comment on issue to express interest -- [ ] Wait for maintainer acknowledgment (if required) -- [ ] Understand project's code style +- [ ] Reproduce bug with original version +- [ ] Trace git history for context (`git log --all --grep="keyword"`) +- [ ] Identify root cause with code references +- [ ] Post detailed investigation to issue (not PR) +- [ ] Link all related issues/PRs + +### Investigation Template (Post to Issue) + +```markdown +## Investigation + +I traced this through the codebase history: + +1. [Date]: #[PR] introduced [feature] +2. [Date]: #[PR] added [workaround] because [reason] +3. [Date]: #[PR] changed [parameter] +4. Now: Safe to [fix] because [explanation] + +[Detailed evidence with code references] +``` + +## Before Starting ## Environment Setup @@ -28,11 +52,27 @@ refactor/extract-validation-logic ## During Development -- [ ] Make small, focused commits +- [ ] Make minimal, focused changes (only what's necessary) +- [ ] Add regression test to prevent future breakage +- [ ] Update CHANGELOG if project uses it - [ ] Follow project's commit message format -- [ ] Add tests for new functionality -- [ ] Update documentation if needed - [ ] Run linter/formatter before committing +- [ ] Don't refactor unrelated code +- [ ] Don't add "improvements" beyond the fix + +### What to Change + +✅ **Do change**: +- Code directly related to the fix +- Tests for the fix +- CHANGELOG entry +- Relevant documentation + +❌ **Don't change**: +- Surrounding code (refactoring) +- Unrelated files +- Code style of existing code +- Add features beyond the fix ## Before Submitting @@ -46,10 +86,25 @@ refactor/extract-validation-logic ### Evidence Loop -- [ ] Reproduce the failure with one command and capture baseline output +- [ ] Test with original version and capture failure output - [ ] Apply the fix -- [ ] Rerun the exact same command and capture passing output -- [ ] Add baseline vs fixed vs reference comparison +- [ ] Test with fixed version and capture success output +- [ ] Document both tests with timestamps, exit codes, PIDs +- [ ] Compare baseline vs fixed behavior + +### Testing Commands + +```bash +# Test 1: Reproduce bug with original version +npm install -g package@original-version +[command that triggers bug] +# Capture: error messages, exit codes, timestamps + +# Test 2: Validate fix with patched version +npm install -g package@fixed-version +[same command] +# Capture: success output, normal exit codes +``` ### Redaction Gate @@ -67,15 +122,32 @@ refactor/extract-validation-logic ### PR Description -- [ ] Clear, descriptive title -- [ ] Summary of changes -- [ ] Motivation/context -- [ ] Testing approach -- [ ] Screenshots (for UI changes) +- [ ] Clear, descriptive title (conventional commit format) +- [ ] Focused description (~50 lines, not >100) +- [ ] Summary (1-2 sentences) +- [ ] Root cause (technical, with code refs) +- [ ] Changes (bullet list) +- [ ] Why it's safe +- [ ] Testing validation - [ ] Related issues linked -- [ ] Sources/Attribution section added -- [ ] Risks section added -- [ ] Rollback Plan section added +- [ ] No detailed timeline (move to issue) +- [ ] No internal tooling mentions +- [ ] No speculation or uncertainty + +### PR Description Length Guide + +✅ **Good**: ~50 lines +- Summary: 2 lines +- Root cause: 5 lines +- Changes: 5 lines +- Why safe: 5 lines +- Testing: 20 lines +- Related: 3 lines + +❌ **Too long**: >100 lines +- Move detailed investigation to issue +- Move timeline analysis to issue +- Keep PR focused on the fix ## PR Title Format @@ -96,82 +168,51 @@ chore(deps): update lodash to 4.17.21 ````markdown ## Summary -Brief description of what this PR does. +[1-2 sentences: what this fixes and why] -## Motivation +## Root Cause -Why is this change needed? Link to issue if applicable. - -Closes #123 +[Technical explanation with code references] ## Changes -- Change 1 -- Change 2 -- Change 3 +- [Actual code changes] +- [Tests added] +- [Docs updated] -## Evidence Loop +## Why This Is Safe -Command: -```bash -# Baseline (before fix) -[command] - -# Fixed (after fix, same command) -[command] -``` - -Raw output: -```text -[baseline output] -``` - -```text -[fixed output] -``` - -## Comparison (Baseline vs Fixed vs Reference) - -| Case | Command / Scenario | Result | Evidence | -|------|--------------------|--------|----------| -| Baseline | `[same command]` | Fail | [raw output block] | -| Fixed | `[same command]` | Pass | [raw output block] | -| Reference | [spec, issue, or main behavior] | Expected | [link or note] | - -## Sources/Attribution - -- [Issue, docs, benchmark source, or code reference] - -## Risks - -- [Risk and impact] - -## Rollback Plan - -- Revert commit(s): [hash] -- Restore previous behavior with: [command] +[Explain why it won't break anything] ## Testing -How did you test this change? +### Test 1: Reproduce Bug (Original Version) +Command: `[command]` +Result: +```text +[failure output with timestamps, exit codes] +``` -- [ ] Unit tests added/updated -- [ ] Manual testing performed -- [ ] Integration tests pass +### Test 2: Validate Fix (Patched Version) +Command: `[same command]` +Result: +```text +[success output with timestamps, exit codes] +``` -## Screenshots (if applicable) +## Related -| Before | After | -|--------|-------| -| image | image | - -## Checklist - -- [ ] Tests pass -- [ ] Documentation updated -- [ ] Code follows project style +- Fixes #[issue] +- Related: #[other issues/PRs] ```` +**What NOT to include**: +- ❌ Detailed timeline analysis (put in issue) +- ❌ Historical context (put in issue) +- ❌ Internal tooling mentions +- ❌ Speculation or uncertainty +- ❌ Walls of text (>100 lines) + ## Comparison Table Template ```markdown @@ -185,10 +226,34 @@ How did you test this change? ## After Submitting - [ ] Monitor for CI results -- [ ] Respond to review comments promptly +- [ ] Respond to review comments within 24 hours - [ ] Make requested changes quickly - [ ] Thank reviewers for their time - [ ] Don't force push after review starts (unless asked) +- [ ] Add new commits during review (don't amend) +- [ ] Explain what changed in follow-up comments +- [ ] Re-request review when ready + +## Separation of Concerns + +### Issue Comments (Detailed Investigation) +- Timeline analysis +- Historical context +- Related PRs/issues +- Root cause deep dive +- 100-300 lines OK + +### PR Description (Focused on Fix) +- Summary (1-2 sentences) +- Root cause (technical) +- Changes (bullet list) +- Testing validation +- ~50 lines total + +### Separate Test Comment (End-to-End Validation) +- Test with original version +- Test with fixed version +- Full logs with timestamps ## Review Response Etiquette