From 828f44d3d6ff403c14b28115d8cfb4a2f3e19bc1 Mon Sep 17 00:00:00 2001 From: Mackenzie Zastrow Date: Mon, 8 Dec 2025 12:08:38 -0500 Subject: [PATCH 1/3] Guide agents(s) to be more concise on issue & PR descriptions --- .github/agent-sops/task-implementer.sop.md | 51 +++++- .github/agent-sops/task-refiner.sop.md | 7 - docs/PR.md | 203 +++++++++++++++++++++ 3 files changed, 247 insertions(+), 14 deletions(-) create mode 100644 docs/PR.md diff --git a/.github/agent-sops/task-implementer.sop.md b/.github/agent-sops/task-implementer.sop.md index f74b198f..0e49e544 100644 --- a/.github/agent-sops/task-implementer.sop.md +++ b/.github/agent-sops/task-implementer.sop.md @@ -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 #" 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 - 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 ` command with the prepared commit message - You MAY use `git push origin ` 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: #" 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 SHOULD 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,6 +414,25 @@ 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) +- 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 diff --git a/.github/agent-sops/task-refiner.sop.md b/.github/agent-sops/task-refiner.sop.md index fd1f1a86..25cd1b38 100644 --- a/.github/agent-sops/task-refiner.sop.md +++ b/.github/agent-sops/task-refiner.sop.md @@ -185,7 +185,6 @@ Record that the task review is complete and ready as a comment on the issue. - If comment posting is deferred, continue with the workflow and note the deferred status - You MUST summarize what was accomplished in your comment - You MUST confirm in your comment that the issue is ready for implementation, or explain why it is not -- You MUST record the estimated scope of work based on repository analysis - You SHOULD mention any final recommendations or considerations ## Examples @@ -257,12 +256,6 @@ Based on clarification discussion and repository analysis: - [ ] 2FA can be enabled/disabled by user - [ ] Integration tests pass - [ ] Existing auth functionality remains intact - -### Estimated Scope -- **Complexity**: Medium -- **Files Modified**: ~8-10 files -- **New Components**: 2-3 React components -- **Database Migrations**: 1-2 migrations required ``` ## Troubleshooting diff --git a/docs/PR.md b/docs/PR.md new file mode 100644 index 00000000..30d89529 --- /dev/null +++ b/docs/PR.md @@ -0,0 +1,203 @@ +# 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 examples)? +3. **Use Cases** (optional) — When would developers use this feature? Only include for non-obvious functionality; skip for trivial changes. +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 examples:** + +- ✅ Show before/after code examples 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 examples. 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 2-4 concrete use cases +showing when developers would use this feature. Skip for trivial changes.] + +## 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 #" 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? From a545d311f5e2dbde876fb8b4c0e9a1c05158d1a1 Mon Sep 17 00:00:00 2001 From: Mackenzie Zastrow <3211021+zastrowm@users.noreply.github.com> Date: Mon, 8 Dec 2025 13:40:42 -0500 Subject: [PATCH 2/3] Change 'SHOULD' to 'MUST' in SOP guidelines --- .github/agent-sops/task-implementer.sop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/agent-sops/task-implementer.sop.md b/.github/agent-sops/task-implementer.sop.md index 0e49e544..34aee7e5 100644 --- a/.github/agent-sops/task-implementer.sop.md +++ b/.github/agent-sops/task-implementer.sop.md @@ -320,7 +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 SHOULD include additional checklist items from [docs/PR.md](../../docs/PR.md) to validate the pull request description is correct after making additional changes +- 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 @@ -437,4 +437,4 @@ This pattern ensures quality gates are not skipped and provides an audit trail o - 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 \ No newline at end of file +- You must only push to your feature branch, never main From 985eb505694875a72a85c1a46a97b60e30a46e32 Mon Sep 17 00:00:00 2001 From: Mackenzie Zastrow <3211021+zastrowm@users.noreply.github.com> Date: Mon, 8 Dec 2025 13:46:17 -0500 Subject: [PATCH 3/3] Revise PR guidelines for public API changes and examples Updated language for clarity and consistency in PR guidelines. --- docs/PR.md | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/docs/PR.md b/docs/PR.md index 30d89529..d92a2a11 100644 --- a/docs/PR.md +++ b/docs/PR.md @@ -18,8 +18,8 @@ Write for senior engineers familiar with the SDK. Assume your reader: Every PR description should have: 1. **Motivation** — Why is this change needed? -2. **Public API Changes** — What changes to the public API (with code examples)? -3. **Use Cases** (optional) — When would developers use this feature? Only include for non-obvious functionality; skip for trivial changes. +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 @@ -29,9 +29,9 @@ Every PR description should have: - ✅ "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 examples:** +**Document public API changes with example code snippets:** -- ✅ Show before/after code examples for API changes +- ✅ Show before/after code snippets for API changes - ❌ List every file or line changed **Be concise:** @@ -117,7 +117,6 @@ const model = new OpenAIModel({ apiKey: async () => await secretManager.getApiKey(), }) ``` -```` The change is backward compatible—all existing string-based usage continues to work without modification. @@ -145,7 +144,7 @@ Resolves: #[issue-number] ## Public API Changes -[Document changes to public APIs with before/after code examples. If no public +[Document changes to public APIs with before/after code snippets. If no public API changes, state "No public API changes."] ```typescript @@ -155,14 +154,13 @@ API changes, state "No public API changes."] // After [new API usage] ``` -```` [Explain behavior, parameters, return values, and backward compatibility.] ## Use Cases (optional) -[Only include for non-obvious functionality. Provide 2-4 concrete use cases -showing when developers would use this feature. Skip for trivial changes.] +[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) @@ -178,7 +176,7 @@ showing when developers would use this feature. Skip for trivial changes.] [new code] ``` -``` +```` ## Why These Guidelines? @@ -192,7 +190,6 @@ showing when developers would use this feature. Skip for trivial changes.] - [Conventional Commits](https://www.conventionalcommits.org/) - [Google's Code Review Guidelines](https://google.github.io/eng-practices/review/) -``` ## Checklist Items