Skip to content

Latest commit

 

History

History
201 lines (132 loc) · 6.08 KB

PR.md

File metadata and controls

201 lines (132 loc) · 6.08 KB

Pull Request Description Guidelines

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.

When creating a PR, follow the GitHub PR template and use these guidelines to fill it out effectively.

Who's Reading Your PR?

Write for senior engineers familiar with the SDK. Assume your reader:

  • Understands the SDK's architecture and patterns
  • Has context about the broader system
  • Can read code diffs to understand implementation details
  • Values concise, focused communication

What to Include

Every PR description should have:

  1. Motivation — Why is this change needed?
  2. Public API Changes — What changes to the public API (with code snippets)?
  3. Use Cases (optional) — When would developers use this feature? Only include for non-obvious functionality; skip for trivial changes or obvious fixes.
  4. Breaking Changes (if applicable) — What breaks and how to migrate?

Writing Principles

Focus on WHY, not HOW:

  • ✅ "Hook providers need access to the agent's result to perform post-invocation actions like logging or analytics"
  • ❌ "Added result field to AfterInvocationEvent dataclass"

Document public API changes with example code snippets:

  • ✅ Show before/after code snippets for API changes
  • ❌ List every file or line changed

Be concise:

  • ✅ Use prose over bullet lists when possible
  • ❌ Create exhaustive implementation checklists

Emphasize user impact:

  • ✅ "Enables hooks to log conversation outcomes or trigger follow-up actions based on the result"
  • ❌ "Updated AfterInvocationEvent to include optional AgentResult field"

What to Skip

Leave these out of your PR description:

  • Implementation details — Code comments and commit messages cover this
  • Test coverage notes — CI will catch issues; assume tests are comprehensive
  • Line-by-line change lists — The diff provides this
  • Build/lint/coverage status — CI handles verification
  • Commit hashes — GitHub links commits automatically

Anti-patterns

❌ Over-detailed checklists:

### Type Definition Updates

- Added result field to AfterInvocationEvent dataclass
- Updated Agent._run_loop to capture and pass AgentResult

❌ Implementation notes reviewers don't need:

## Implementation Notes

- Result field defaults to None
- AgentResult is captured from EventLoopStopEvent before invoking hooks

❌ Test coverage bullets:

### Test Coverage

- Added test: AfterInvocationEvent includes AgentResult
- Added test: result is None when structured_output is used

Good Examples

✅ Motivation section:

## Motivation

Hook providers often need to perform actions based on the outcome of an agent's
invocation, such as logging results, updating metrics, or triggering follow-up
workflows. Currently, the `AfterInvocationEvent` doesn't provide access to the
`AgentResult`, forcing hook implementations to track state externally or miss
this information entirely.

✅ Public API Changes section:

## Public API Changes

`AfterInvocationEvent` now includes an optional `result` attribute containing
the `AgentResult`:

```python
# Before: no access to result
class MyHook(HookProvider):
    def on_after_invocation(self, event: AfterInvocationEvent) -> None:
        # Could only access event.agent, no result available
        logger.info("Invocation completed")

# After: result available for inspection
class MyHook(HookProvider):
    def on_after_invocation(self, event: AfterInvocationEvent) -> None:
        if event.result:
            logger.info(f"Completed with stop_reason: {event.result.stop_reason}")
```

The `result` field is `None` when invoked from `structured_output` methods.

✅ Use Cases section:

## Use Cases

- **Result logging**: Log conversation outcomes including stop reasons and token usage
- **Analytics**: Track agent performance metrics based on invocation results
- **Conditional workflows**: Trigger follow-up actions based on how the agent completed

Template

## Motivation

[Explain WHY this change is needed. What problem does it solve? What limitation
does it address? What user need does it fulfill?]

Resolves: #[issue-number]

## Public API Changes

[Document changes to public APIs with before/after code snippets. If no public
API changes, state "No public API changes."]

```python
# Before
[existing API usage]

# After
[new API usage]
```

[Explain behavior, parameters, return values, and backward compatibility.]

## Use Cases (optional)

[Only include for non-obvious functionality. Provide 1-3 concrete use cases
showing when developers would use this feature. Skip for trivial changes obvious fixes..]

## Breaking Changes (if applicable)

[If this is a breaking change, explain what breaks and provide migration guidance.]

### Migration

```python
# Before
[old code]

# After
[new code]
```

Why These Guidelines?

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.

Skip test/lint/coverage details because CI pipelines verify these automatically. Including them adds noise without value.

Write for senior engineers to enable concise, technical communication without redundant explanations.

References

Checklist Items

  • Does the PR description target a Senior Engineer familiar with the project?
  • Does the PR description give an overview of the feature being implemented, including any notes on key implementation decisions
  • Does the PR include a "Resolves #" in the body and is not bolded?
  • Does the PR contain the motivation or use-cases behind the change?
  • Does the PR omit irrelevant details not needed for historical reference?