Document development and PR review workflows

docs/operations/git/development_workflow.md

@@ -0,0 +1,136 @@
+# Standard Development Workflow
+This document describes how to contribute to repositories under the Neon AI
+GitHub organization. These best practices help to ensure contributions can be
+efficiently reviewed and accepted by project maintainers while maintaining a
+clear commit history and high standard of quality.
+
+## Branch Naming
+When creating a new branch to work on changes, it is recommended to use the
+format `<type>_<description>`, where <type> is `feat`, `fix`, `doc`, or a 
+similar descriptor of the nature of changes being contributed and <description>
+is a brief description in kebab-case. For example, the branch that adds this
+page to documentation is labeled `doc_developer-workflow`.
+> This is similar to [Conventional Branch](https://conventionalbranch.org/), 
+  though there are cases where `/` in a branch name can cause issues with
+  git ref parsing that naively assumes it can segment the complete ref on `/`.
+
+This naming convention is not a hard requirement for submitting changes, but
+it is recommended to help quickly organize and manage branches.
+
+## Committing Changes
+Since PRs will squash the commit history into a single commit on the base branch,
+the feature branch commit history is unimportant. Developers are encouraged to
+commit changes in whatever manner is most efficient for their workflow.
+> [git-scm](https://git-scm.com/docs/git-commit) has a good description of how
+  the `git commit` command works and its application.
+
+## Pull Requests
+PRs should always be opened with the repository default branch as the base
+(`dev` in most cases). PR titles should be a short description of changes
+in plain English; for clarity, they should not contain any reference to other
+PR or Issue numbers.
+> More information about Pull Requests can be found in the
+  [GitHub Docs](https://docs.github.com/en/pull-requests).
+
+### PR Description
+PR descriptions should use the configured template if present so that changelogs
+and release notes are consistent. Following the template, the Description section should
+include a detailed description of changes; the organization is not specified, so
+this may be a list of changes or a free-form description of the changes. The
+description may include rationale for the changes and design decisions, though
+this is not strictly required; the description should remain focused on the
+changes contained in the PR.
+
+The Issues section should contain a bulleted list of related issues and PRs,
+both in the current repository and other repositories. Any issues that are
+closed by the PR should use the "Closes" keyword so that they are automatically
+closed when the PR is merged. There may be related issues that are not closed,
+but they should still be included here for reference; this helps when reviewing
+issues since related changes will be clearly visible in the issue history.
+> A bulleted list is specified here because GitHub will render more detailed
+  links in a bulleted list than it does in a regular text block.
+
+The Other Notes section is available to include any additional information or
+context that is relevant to the PR. This is a good place to include links to
+references, justification of design decisions, edge cases that are handled, or
+any other background information.
+
+This format is not a hard requirement, but it is recommended to help PR 
+reviewers quickly understand the changes and to maintain a consistent format
+when reviewing previous changes/releases.
+
+### PR Reviews
+All PRs must receive an approving review before they may be merged with very
+few exceptions. The review process is the most important control that ensures
+we maintain a high standard of quality and that every change properly implements
+its intent. The process for completing a review is described in [Reviewing a Pull Request](./review_pr.md).
+
+When a PR is complete, it should not be marked as a draft and one or more 
+reviewers should be requested. If the PR is not complete but a review is still
+desired for feedback on WIP changes, or to discuss design decisions, it is best
+to leave a comment with those specific questions, tagging the reviewer.
+
+### GitHub Actions
+Many repositories have configured automation that runs when a PR is opened or
+commits are added to an open PR. Some of these will not run until the PR is
+marked ready for review, while others will run even on draft PRs. In either 
+case, a PR may not be merged until all checks have passed with no exceptions.
+
+### Merging PRs
+A PR is ready to be merged when at least one approving review has been left and
+all checks have passed. If there are any reviews left requesting changes, those
+changes must be addressed and that reviewer must leave an approving review. For
+example, if there are two reviews requesting changes, then both reviewers must
+approve the PR before it may be merged.
+
+Once the PR is ready to be merged, it should be merged using the
+"Squash and Merge" option. This ensures that the commit history of the base
+branch remains clean and that the changes in the PR are represented as a
+single commit with the PT title and description in the commit history.
+> A PR should never be merged with a "Merge Commit" or "Rebase and Merge" as
+  this will create a complex commit history on the base branch. With more 
+  commits, it becomes difficult to parse the commit history and rebasing other
+  feature branches is much more difficult.
+
+## Rebasing Branches
+Since multiple PRs may be open at the same time, and merge order is generally
+unknown, it is likely that a PR will need to be rebased on the base branch at
+some point. If a single author has multiple open PRs, they may choose to 
+define the merge order by marking PRs as Drafts, or leaving comments in open
+PRs to indicate an intended merge order.
+> The [git-scm Cheat Sheet](https://git-scm.com/cheat-sheet#combine-diverged-branches)
+  has a good visual representation of what rebasing does.
+
+### Resolving Conflicts
+When rebasing a branch, it is possible that merge conflicts will arise due to
+changes in the base branch overlapping changes in the feature branch. When this
+happens, the developer performing the rebase must make decisions about how to
+resolve the conflicts.
+
+Since resolving conflicts is a potentially destructive operation, it is
+recommended to create a backup of the feature branch being rebased; this may
+be done locally or by creating a new branch on GitHub, based on the
+feature branch. With a safe backup in place, resolve the conflicts and complete
+the rebase.
+> Note that because rebasing is basically re-playing each commit on the feature
+  branch, there may be changes that need to be resolved where the result does
+  not match either the base branch or the feature branch.
+
+Once the rebase is complete, the local feature branch should be force pushed to
+the remote (overwriting the previous branch that lacked recent commits on the
+base branch). In nearly all cases, the PR diff should look identical to what it
+was before the rebase.
+
+### Rebase Tips
+- If rebasing a complex feature branch, it may be easier to squash commits on
+  the feature branch before rebasing. This reduces the number of commits that
+  need to be rebased and avoids tricky merge conflicts that come up when the
+  feature branch has multiple changes to the same chunk of code.
+- If during a rebase you find something has gone wrong, you can always abort
+  and try again using `git rebase --abort`. This will return the local branch
+  to the state it was before the rebase started.
+- If you find the PR diff has changed in a way you didn't expect after force
+  pushing changes, you can look at the backup branch to compare the changes.
+  To resolve, you can either start over by using `git reset --hard <backup-branch>`
+  to return to the pre-rebase state, or you can create an additional commit to
+  manually correct the rebase errors.

docs/operations/git/review_pr.md

@@ -0,0 +1,104 @@
+# Reviewing a Pull Request
+This document describes the process for reviewing a pull request (PR). Specific
+review criteria may apply to different projects, but this document aims to
+cover the general process that applies to Neon AI repositories.
+> See also [Project QA Standards](../qa_standards.md) for
+  more information about expectations for testing and
+  automation.
+
+## When to Review
+A PR should generally be reviewed by requested reviewers when it is marked 
+"Ready for Review". If a review is requested on a Draft PR, the reviewer should
+read the PR description and comments to understand if the author is looking for
+specific feedback or input.
+> Note that a PR may be marked "Ready for Review", but pending or failing 
+  checks may result in further changes before the PR is actually ready. In
+  these cases, it is best to notify the PR author of the issues and hold a
+  full review until they are resolved.
+
+## Review Process
+The purpose of PR reviews is to ensure that changes are complete, described
+accurately and completely in the PR description, adequately tested, and that
+any relevant issues are linked and resolved or otherwise partially addressed.
+
+### 1. Read the PR Title, Description, and Comments
+The PR title should be a succinct and understandable statement of the changes
+in the PR. If unclear, edit or ask the author to do so. The PR description
+should provide an understandable summary of the changes and highlight any
+specific trade-offs or design decisions. If any of those design decisions don't
+make sense or there is a better alternative, quote the relevant line in a 
+comment to discuss with the PR author. Be sure to read the comment history as
+well in case the decision has already been discussed.
+
+Once there is clear alignment on the goal of the PR and design discussions are
+resolved, the code is ready for review.
+
+### 2. Review the Code Changes
+Code changes should be reviewed for correctness, readability, consistency, and
+scope. Much of the review process is subjective, so this document will only aim
+to provide guidance for things to look for, rather than a rubric for evaluation.
+
+#### Correctness
+Changes should obviously be free of syntax errors and other bugs. Ensure that
+any changes come with test coverage or that they can be manually validated if
+adding tests is not feasible. Consider any edge cases or potential exceptions
+and confirm they are handled or at least acknowledged. If there are undefined
+behaviors that are not explicitly handled or discussed in the PR, flag them as
+issues to be resolved.
+
+#### Readability
+Review new methods, functions, and variables for clear names and type annotations
+as appropriate. New methods and functions should have docstrings that match the
+format of other docstrings in the repository. If there is complex logic that
+isn't clear, make sure it has a comment explaining its purpose. Ensure that
+any exception handling includes clear logs and that any raised exceptions use
+an appropriate exception type and message.
+
+#### Consistency
+Ensure that changes are consistent with the existing codebase. New functions and
+classes should integrate with existing modules where it makes sense and
+signatures should follow the same format as existing code. Suggest changes as
+appropriate to help the author address any issues.
+
+#### Scope
+All changes in the PR should be constrained in scope to one specific feature,
+fix, documentation task, or other discrete change. If a PR includes multiple
+unrelated changes, flag the changes that are out of scope to request they be
+separated into their own PR(s). This can be particularly helpful if some
+changes are complete and ready to merge, while others have extensive feedback
+to address.
+> Note that this may lead to PRs that are dependent on each other. Use your
+  judgement to determine if this is appropriate, or if separating changes
+  will create more work for limited benefit.
+
+### 3. Complete the Review
+Once changes have been reviewed and comments left, a review must be completed.
+A review can be completed to Approve, Request Changes, or Comment on the PR.
+
+#### Approving Review
+If there are no changes to be made and the PR is ready to be merged, leave an
+approving review. This may be done with or without comment, but it is best
+practice to leave a comment indicating what you did (i.e. tested changes,
+reviewed code, reviewed tests, etc.) in case something comes up in the future
+and the merged PR needs to be referenced.
+> Note that an approving review may include comments that suggest a future
+  enhancement or consideration to be moved to a new Issue. These should be
+  addressed and resolved before merge, even though the PR is already approved.
+
+#### Requesting Changes
+If there are issues noted in the review that must be addressed before the PR
+is ready to merge, leave a review requesting changes. It is helpful to highlight
+in the review comment what comments in particular need to be addressed and if
+any of the comments are optional suggestions or open for discussion.
+
+#### Comment Only
+If there is feedback that is not a specific request for changes and the PR is
+not yet ready to be approved, a Comment review may be left. This will most 
+often be done when a PR is still a Draft and comments don't necessarily relate
+to a specific change. This kind of review can also be helpful when changes are
+reviewed, but further testing will be done before approval.
+> If the PR author is reviewing changes, they are only allowed to leave a 
+  Comment, since it would not make sense to approve or request changes on their
+  own PR. This can be a useful practice for reviewing changes before requesting
+  reviews from others.
+

mkdocs.yml

@@ -138,6 +138,8 @@ nav:
         - Git:
             - Undo a Stable PR Workflow: operations/git/undo_stable_pr.md
             - Triage New GitHub Issues: operations/git/triage_issues.md
+            - Development Workflow: operations/git/development_workflow.md
+            - Pull Request Review: operations/git/review_pr.md
         - Skills:
             - Stable Release Workflow: operations/skills/workflow_stable_release.md
             - Alpha Release Workflow: operations/skills/workflow_alpha_release.md