|
| 1 | +# Pull Request Description Guidelines |
| 2 | + |
| 3 | +Good PR descriptions help reviewers understand the context and impact of your changes. They enable faster reviews, better decision-making, and serve as valuable historical documentation. |
| 4 | + |
| 5 | +When creating a PR, follow the [GitHub PR template](../.github/PULL_REQUEST_TEMPLATE.md) and use these guidelines to fill it out effectively. |
| 6 | + |
| 7 | +## Who's Reading Your PR? |
| 8 | + |
| 9 | +Write for senior engineers familiar with the SDK. Assume your reader: |
| 10 | + |
| 11 | +- Understands the SDK's architecture and patterns |
| 12 | +- Has context about the broader system |
| 13 | +- Can read code diffs to understand implementation details |
| 14 | +- Values concise, focused communication |
| 15 | + |
| 16 | +## What to Include |
| 17 | + |
| 18 | +Every PR description should have: |
| 19 | + |
| 20 | +1. **Motivation** — Why is this change needed? |
| 21 | +2. **Public API Changes** — What changes to the public API (with code snippets)? |
| 22 | +3. **Use Cases** (optional) — When would developers use this feature? Only include for non-obvious functionality; skip for trivial changes or obvious fixes. |
| 23 | +4. **Breaking Changes** (if applicable) — What breaks and how to migrate? |
| 24 | + |
| 25 | +## Writing Principles |
| 26 | + |
| 27 | +**Focus on WHY, not HOW:** |
| 28 | + |
| 29 | +- ✅ "Hook providers need access to the agent's result to perform post-invocation actions like logging or analytics" |
| 30 | +- ❌ "Added result field to AfterInvocationEvent dataclass" |
| 31 | + |
| 32 | +**Document public API changes with example code snippets:** |
| 33 | + |
| 34 | +- ✅ Show before/after code snippets for API changes |
| 35 | +- ❌ List every file or line changed |
| 36 | + |
| 37 | +**Be concise:** |
| 38 | + |
| 39 | +- ✅ Use prose over bullet lists when possible |
| 40 | +- ❌ Create exhaustive implementation checklists |
| 41 | + |
| 42 | +**Emphasize user impact:** |
| 43 | + |
| 44 | +- ✅ "Enables hooks to log conversation outcomes or trigger follow-up actions based on the result" |
| 45 | +- ❌ "Updated AfterInvocationEvent to include optional AgentResult field" |
| 46 | + |
| 47 | +## What to Skip |
| 48 | + |
| 49 | +Leave these out of your PR description: |
| 50 | + |
| 51 | +- **Implementation details** — Code comments and commit messages cover this |
| 52 | +- **Test coverage notes** — CI will catch issues; assume tests are comprehensive |
| 53 | +- **Line-by-line change lists** — The diff provides this |
| 54 | +- **Build/lint/coverage status** — CI handles verification |
| 55 | +- **Commit hashes** — GitHub links commits automatically |
| 56 | + |
| 57 | +## Anti-patterns |
| 58 | + |
| 59 | +❌ **Over-detailed checklists:** |
| 60 | + |
| 61 | +```markdown |
| 62 | +### Type Definition Updates |
| 63 | + |
| 64 | +- Added result field to AfterInvocationEvent dataclass |
| 65 | +- Updated Agent._run_loop to capture and pass AgentResult |
| 66 | +``` |
| 67 | + |
| 68 | +❌ **Implementation notes reviewers don't need:** |
| 69 | + |
| 70 | +```markdown |
| 71 | +## Implementation Notes |
| 72 | + |
| 73 | +- Result field defaults to None |
| 74 | +- AgentResult is captured from EventLoopStopEvent before invoking hooks |
| 75 | +``` |
| 76 | + |
| 77 | +❌ **Test coverage bullets:** |
| 78 | + |
| 79 | +```markdown |
| 80 | +### Test Coverage |
| 81 | + |
| 82 | +- Added test: AfterInvocationEvent includes AgentResult |
| 83 | +- Added test: result is None when structured_output is used |
| 84 | +``` |
| 85 | + |
| 86 | +## Good Examples |
| 87 | + |
| 88 | +✅ **Motivation section:** |
| 89 | + |
| 90 | +```markdown |
| 91 | +## Motivation |
| 92 | + |
| 93 | +Hook providers often need to perform actions based on the outcome of an agent's |
| 94 | +invocation, such as logging results, updating metrics, or triggering follow-up |
| 95 | +workflows. Currently, the `AfterInvocationEvent` doesn't provide access to the |
| 96 | +`AgentResult`, forcing hook implementations to track state externally or miss |
| 97 | +this information entirely. |
| 98 | +``` |
| 99 | + |
| 100 | +✅ **Public API Changes section:** |
| 101 | + |
| 102 | +````markdown |
| 103 | +## Public API Changes |
| 104 | + |
| 105 | +`AfterInvocationEvent` now includes an optional `result` attribute containing |
| 106 | +the `AgentResult`: |
| 107 | + |
| 108 | +```python |
| 109 | +# Before: no access to result |
| 110 | +class MyHook(HookProvider): |
| 111 | + def on_after_invocation(self, event: AfterInvocationEvent) -> None: |
| 112 | + # Could only access event.agent, no result available |
| 113 | + logger.info("Invocation completed") |
| 114 | + |
| 115 | +# After: result available for inspection |
| 116 | +class MyHook(HookProvider): |
| 117 | + def on_after_invocation(self, event: AfterInvocationEvent) -> None: |
| 118 | + if event.result: |
| 119 | + logger.info(f"Completed with stop_reason: {event.result.stop_reason}") |
| 120 | +``` |
| 121 | + |
| 122 | +The `result` field is `None` when invoked from `structured_output` methods. |
| 123 | + |
| 124 | +```` |
| 125 | + |
| 126 | +✅ **Use Cases section:** |
| 127 | + |
| 128 | +```markdown |
| 129 | +## Use Cases |
| 130 | + |
| 131 | +- **Result logging**: Log conversation outcomes including stop reasons and token usage |
| 132 | +- **Analytics**: Track agent performance metrics based on invocation results |
| 133 | +- **Conditional workflows**: Trigger follow-up actions based on how the agent completed |
| 134 | +```` |
| 135 | + |
| 136 | +## Template |
| 137 | + |
| 138 | +````markdown |
| 139 | +## Motivation |
| 140 | + |
| 141 | +[Explain WHY this change is needed. What problem does it solve? What limitation |
| 142 | +does it address? What user need does it fulfill?] |
| 143 | + |
| 144 | +Resolves: #[issue-number] |
| 145 | + |
| 146 | +## Public API Changes |
| 147 | + |
| 148 | +[Document changes to public APIs with before/after code snippets. If no public |
| 149 | +API changes, state "No public API changes."] |
| 150 | + |
| 151 | +```python |
| 152 | +# Before |
| 153 | +[existing API usage] |
| 154 | + |
| 155 | +# After |
| 156 | +[new API usage] |
| 157 | +``` |
| 158 | + |
| 159 | +[Explain behavior, parameters, return values, and backward compatibility.] |
| 160 | + |
| 161 | +## Use Cases (optional) |
| 162 | + |
| 163 | +[Only include for non-obvious functionality. Provide 1-3 concrete use cases |
| 164 | +showing when developers would use this feature. Skip for trivial changes obvious fixes..] |
| 165 | + |
| 166 | +## Breaking Changes (if applicable) |
| 167 | + |
| 168 | +[If this is a breaking change, explain what breaks and provide migration guidance.] |
| 169 | + |
| 170 | +### Migration |
| 171 | + |
| 172 | +```python |
| 173 | +# Before |
| 174 | +[old code] |
| 175 | + |
| 176 | +# After |
| 177 | +[new code] |
| 178 | +``` |
| 179 | + |
| 180 | +```` |
| 181 | +
|
| 182 | +## Why These Guidelines? |
| 183 | +
|
| 184 | +**Focus on WHY over HOW** because code diffs show implementation details, commit messages document granular changes, and PR descriptions provide the broader context reviewers need. |
| 185 | +
|
| 186 | +**Skip test/lint/coverage details** because CI pipelines verify these automatically. Including them adds noise without value. |
| 187 | +
|
| 188 | +**Write for senior engineers** to enable concise, technical communication without redundant explanations. |
| 189 | +
|
| 190 | +## References |
| 191 | +
|
| 192 | +- [Conventional Commits](https://www.conventionalcommits.org/) |
| 193 | +- [Google's Code Review Guidelines](https://google.github.io/eng-practices/review/) |
| 194 | +
|
| 195 | +## Checklist Items |
| 196 | +
|
| 197 | + - [ ] Does the PR description target a Senior Engineer familiar with the project? |
| 198 | + - [ ] Does the PR description give an overview of the feature being implemented, including any notes on key implementation decisions |
| 199 | + - [ ] Does the PR include a "Resolves #<ISSUE NUMBER>" in the body and is not bolded? |
| 200 | + - [ ] Does the PR contain the motivation or use-cases behind the change? |
| 201 | + - [ ] Does the PR omit irrelevant details not needed for historical reference? |
0 commit comments