Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 44 additions & 7 deletions .github/agent-sops/task-implementer.sop.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
- 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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
7 changes: 0 additions & 7 deletions .github/agent-sops/task-refiner.sop.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
203 changes: 203 additions & 0 deletions docs/PR.md
Original file line number Diff line number Diff line change
@@ -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 #<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?
Loading