-
Notifications
You must be signed in to change notification settings - Fork 33
Guide agents(s) to be more concise on issue & PR descriptions #338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,7 @@ Create a comprehensive list of test scenarios covering normal operation, edge ca | |
| - You MUST check for existing testing strategies documented in the repository documentation or your notes | ||
| - You MUST cover all acceptance criteria with at least one test scenario | ||
| - You MUST define explicit input/output pairs for each test case | ||
| - You MUST make note of these test scenarios | ||
| - You MUST make note of these test scenarios | ||
| - You MUST design tests that will initially fail when run against non-existent implementations | ||
| - You MUST NOT create mock implementations during the test design phase because tests should be written based solely on expected behavior, not influenced by implementation details | ||
| - You MUST focus on test scenarios and expected behaviors rather than detailed test code in documentation | ||
|
|
@@ -234,23 +234,40 @@ If the implementation meets all requirements and follows established patterns, p | |
|
|
||
| If all tests are passing, draft a conventional commit message, perform the git commit, and create/update the pull request. | ||
|
|
||
| **PR Checklist Verification (REQUIRED):** | ||
|
|
||
| Before creating or updating a PR, you MUST copy the checklist from [docs/PR.md](../../docs/PR.md) into your progress notes and explicitly verify each item. For each checklist item, you MUST: | ||
|
|
||
| 1. Copy the checklist item verbatim | ||
| 2. Mark it as `[x]` (pass) or `[ ]` (fail) | ||
| 3. If failed, revise the PR description until the item passes | ||
|
|
||
| Example format in your notes: | ||
|
|
||
| ```markdown | ||
| ## PR Description Checklist Verification | ||
|
|
||
| - [x] Does the PR description target a Senior Engineer familiar with the project? | ||
| - [ ] Does the PR include a "Resolves #<ISSUE NUMBER>" in the body? → FAILED: missing issue reference, adding now | ||
| ``` | ||
|
|
||
| You MUST NOT create or update the PR until ALL checklist items pass. | ||
|
|
||
| **Constraints:** | ||
|
|
||
| - You MUST read and follow the PR description guidelines in [docs/PR.md](../../docs/PR.md) when creating pull requests & commits | ||
| - You MUST check that all tasks are complete before proceeding | ||
| - You MUST reference your notes for the issue you are creating a pull request for | ||
| - You MUST NOT commit changes until builds AND tests have been verified because committing broken code can disrupt the development workflow and introduce bugs into the codebase | ||
| - You MUST NOT commit changes until builds AND tests have been verified because committing broken code can disrupt the development workflow and introduce bugs into the codebase | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't think this is necessary as we don't provide a justification for the other items here. We just command the agent to follow |
||
| - You MUST follow the Conventional Commits specification | ||
| - You MUST use `git status` to check which files have been modified | ||
| - You MUST use `git add` to stage all relevant files | ||
| - You MUST execute the `git commit -m <COMMIT_MESSAGE>` command with the prepared commit message | ||
| - You MAY use `git push origin <BRANCH_NAME>` to push the local branch to the remote if the `GITHUB_WRITE` environment variable is set to `true` | ||
| - If the push operation is deferred, continue with PR creation and note the deferred status | ||
| - If the push operation is deferred, continue with PR creation and note the deferred status | ||
| - You MUST attempt to create the pull request using the `create_pull_request` tool if it does not exist yet | ||
| - If the PR creation is deferred, continue with the workflow and note the deferred status | ||
| - You MUST use the task id recorded in your notes, not the issue id | ||
| - You MUST include "Resolves: #<ISSUE NUMBER>" in the body of the pull request | ||
| - You MUST NOT bold this line | ||
| - You MUST give an overview of the feature being implemented | ||
| - You MUST include any notes on key implementation decisions, ambiguity, or other information as part of the pull request description | ||
| - If the `create_pull_request` tool fails (excluding deferred responses): | ||
| - The tool automatically handles fallback by posting a properly URL-encoded manual PR creation link as a comment on the specified fallback issue | ||
| - You MUST verify the fallback comment was posted successfully by checking the tool's return message | ||
|
|
@@ -303,6 +320,7 @@ Based on the users feedback, you will review and update your implementation plan | |
| - You MUST NOT close the parent issue - only the user should close it after the pull request is merged | ||
| - You MUST not attempt to merge the pull request | ||
| - You MUST use the handoff_to_user tool to inform the user you are ready for clarifying information on the pull request | ||
| - You MUST include additional checklist items from [docs/PR.md](../../docs/PR.md) to validate the pull request description is correct after making additional changes | ||
|
|
||
| ## Desired Outcome | ||
|
|
||
|
|
@@ -396,8 +414,27 @@ If builds fail during implementation: | |
| - Use progress tracking with markdown checklists | ||
| - Document decisions, assumptions, and challenges | ||
|
|
||
| ### Checklist Verification Pattern | ||
|
|
||
| When documentation files contain checklists (e.g., `docs/PR.md`), you MUST: | ||
|
|
||
| 1. Copy the entire checklist into your progress notes | ||
| 2. Explicitly verify each item by marking `[x]` or `[ ]` | ||
| 3. For any failed items, document the issue and fix it before proceeding | ||
| 4. Re-verify failed items after fixes until all pass | ||
|
|
||
| This pattern ensures quality gates are not skipped and provides an audit trail of verification. | ||
|
|
||
| ### Pull Request Best Practices | ||
|
|
||
| - You MUST follow the PR description guidelines in [docs/PR.md](../../docs/PR.md) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: must or should? we have the same item twice with different wording
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch; switched to MUST above |
||
| - Focus on WHY the change is needed, not HOW it's implemented | ||
| - Document public API changes with before/after code examples | ||
| - Write for senior engineers familiar with the project | ||
| - Skip implementation details, test coverage notes, and line-by-line change lists | ||
|
|
||
| ### Git Best Practices | ||
| - Commit early and often with descriptive messages | ||
| - Follow Conventional Commits specification | ||
| - You must create a new commit for each feedback iteration | ||
| - You must only push to your feature branch, never main | ||
| - You must only push to your feature branch, never main | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| # 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](../.github/PULL_REQUEST_TEMPLATE.md) 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:** | ||
|
|
||
| - ✅ "The OpenAI SDK supports dynamic API keys, but we don't expose this capability" | ||
| - ❌ "Added ApiKeySetter type import from openai/client" | ||
|
|
||
| **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 secret manager integration for credential rotation" | ||
| - ❌ "Updated error message to mention 'string or function'" | ||
|
|
||
| ## 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:** | ||
|
|
||
| ```markdown | ||
| ### Type Definition Updates | ||
|
|
||
| - Added ApiKeySetter type import from 'openai/client' | ||
| - Updated OpenAIModelOptions interface apiKey type | ||
| ``` | ||
|
|
||
| ❌ **Implementation notes reviewers don't need:** | ||
|
|
||
| ```markdown | ||
| ## Implementation Notes | ||
|
|
||
| - No breaking changes - all existing string-based usage continues to work | ||
| - OpenAI SDK handles validation of function return values | ||
| ``` | ||
|
|
||
| ❌ **Test coverage bullets:** | ||
|
|
||
| ```markdown | ||
| ### Test Coverage | ||
|
|
||
| - Added test: accepts function-based API key | ||
| - Added test: accepts async function-based API key | ||
| ``` | ||
|
|
||
| ## Good Examples | ||
|
|
||
| ✅ **Motivation section:** | ||
|
|
||
| ```markdown | ||
| ## Motivation | ||
|
|
||
| The OpenAI SDK supports dynamic API key resolution through async functions, | ||
| enabling use cases like credential rotation and secret manager integration. | ||
| However, our SDK currently only accepts static strings for the apiKey parameter, | ||
| preventing users from leveraging these capabilities. | ||
| ``` | ||
|
|
||
| ✅ **Public API Changes section:** | ||
|
|
||
| ````markdown | ||
| ## Public API Changes | ||
|
|
||
| The `OpenAIModelOptions.apiKey` parameter now accepts either a string or an | ||
| async function: | ||
|
|
||
| ```typescript | ||
| // Before: only string supported | ||
| const model = new OpenAIModel({ | ||
| modelId: 'gpt-4o', | ||
| apiKey: 'sk-...', | ||
| }) | ||
|
|
||
| // After: function also supported | ||
| const model = new OpenAIModel({ | ||
| modelId: 'gpt-4o', | ||
| apiKey: async () => await secretManager.getApiKey(), | ||
| }) | ||
| ``` | ||
|
|
||
| The change is backward compatible—all existing string-based usage continues | ||
| to work without modification. | ||
|
|
||
| ```` | ||
|
|
||
| ✅ **Use Cases section:** | ||
| ```markdown | ||
| ## Use Cases | ||
|
|
||
| - **API key rotation**: Rotate keys without application restart | ||
| - **Secret manager integration**: Fetch credentials from AWS Secrets Manager, Vault, etc. | ||
| - **Multi-tenant systems**: Dynamically select API keys based on context | ||
| ```` | ||
|
|
||
| ## Template | ||
|
|
||
| ````markdown | ||
| ## 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."] | ||
|
|
||
| ```typescript | ||
| // 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 | ||
|
|
||
| ```typescript | ||
| // 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 | ||
|
|
||
| - [Conventional Commits](https://www.conventionalcommits.org/) | ||
| - [Google's Code Review Guidelines](https://google.github.io/eng-practices/review/) | ||
|
|
||
| ## 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 implemention decisions | ||
| - [ ] Does the PR include a "Resolves #<ISSUE NUMBER>" in the body and is not bolded? | ||
| - [ ] Does the PR contain the motivation or use-cases behind the change? | ||
| - [ ] Does the PR omit irrelevent details not needed for historical reference? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider
-for the failure case