-
Notifications
You must be signed in to change notification settings - Fork 1
docs: add PR self-review norms and code flow section #111
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 |
|---|---|---|
| @@ -1,5 +1,13 @@ | ||
| # Team Operator Code Review Guidelines | ||
|
|
||
| ## Reviewer Responsibilities | ||
|
|
||
| Your job is to review the **code itself**, not just the description or intent. In a world of agentic coding, the author may not have hand-written every line. That means: | ||
|
|
||
| - Read the actual diff, not just the PR summary | ||
| - Verify the code does what the description claims | ||
| - Look for correctness issues, not just style preferences | ||
|
|
||
| ## Core Principles | ||
|
|
||
| ### Simplicity | ||
|
|
@@ -14,6 +22,12 @@ | |
| - Dependencies should be minimal and justified | ||
|
Collaborator
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. Added code quality fundamentals (encapsulation, DRY, naming, complexity). The existing review-guidelines.md had simplicity and maintainability principles but not these explicit quality checks. |
||
| - Breaking changes need migration paths | ||
|
|
||
| ### Code Quality Fundamentals | ||
| - **Encapsulation**: Is internal state properly hidden? Are interfaces clean? | ||
| - **DRY**: Is there duplicated logic that should be extracted? But don't over-abstract — three similar lines can be better than a premature helper | ||
| - **Naming**: Do names reveal intent? Would a future reader understand this? | ||
| - **Complexity**: Is this more complicated than it needs to be? | ||
|
|
||
| ### Security (Elevated Scrutiny) | ||
|
|
||
| These changes require extra review attention: | ||
|
|
@@ -60,3 +74,12 @@ Use clear, actionable language: | |
| - **Critical**: "This will break X because Y. Consider Z." | ||
| - **Important**: "This pattern differs from existing code in A. Recommend B for consistency." | ||
| - **Suggestion**: "Consider X for improved Y." | ||
|
|
||
| ## Self-Review Norm | ||
|
|
||
| PR authors are expected to review their own diff before opening the PR and add inline comments to draw attention to: | ||
| - Lines they don't fully understand | ||
| - Areas of concern or uncertainty | ||
| - Key decision points reviewers should weigh in on | ||
|
|
||
| This balances effort between author and reviewer. If the author hasn't commented their PR, ask them to. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # Description | ||
|
Collaborator
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. Team-operator didn't have a PR template before — this is new. It includes a reminder about the conventional commit format for PR titles since that's enforced by CI and drives semantic versioning. |
||
|
|
||
| <!-- | ||
| Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies required for this change. | ||
|
|
||
| Remember: PR title must follow conventional commit format (feat:, fix:, docs:, etc.) — this is enforced by CI and used for semantic versioning. | ||
|
|
||
| ## Issue | ||
|
|
||
| Link to GitHub or JIRA issue tracking this work; if it exists. | ||
| --> | ||
|
|
||
| ## Code Flow | ||
|
|
||
| <!-- | ||
| Explain how the code works at a high level. Walk through the key paths, data flow, or control flow introduced or changed by this PR. If you discussed the implementation with an AI assistant, include the relevant architectural explanations here. | ||
|
|
||
| Delete this section if the change is trivial (typo fix, version bump, etc.). | ||
| --> | ||
|
|
||
| ## Category of change | ||
|
|
||
| - [ ] Bug fix (non-breaking change which fixes an issue) | ||
| - [ ] New feature (non-breaking change which adds functionality) | ||
| - [ ] Build: a code change that affects the build system or external dependencies | ||
| - [ ] Performance: a code change that improves performance | ||
| - [ ] Refactor: a code change that neither fixes a bug nor adds a feature | ||
| - [ ] Documentation: documentation changes | ||
| - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) | ||
|
|
||
| ## Checklist | ||
|
|
||
| - [ ] I have run `just test` and all tests pass | ||
| - [ ] I have reviewed my own diff and added inline comments on lines I want reviewers to focus on or that I am uncertain about | ||
|
Collaborator
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. Same self-review checklist item as ptd and ptd-config. Also added |
||
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.
New section. Same reviewer responsibilities framing as the other PTD repos — review the code itself, not just the description.