diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..5770485c8 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,28 @@ +### Issue #, if available + +### Description of changes + +### Design + +See [Rules for every change](https://github.com/aws/aws-durable-execution-sdk-python/blob/main/CONTRIBUTING.md#rules-for-every-change). + +- Design note (rule 3), for a new feature, a new option or a public API change: link, or "not needed". +- Simpler options considered (rule 2), if the change adds logic: +- Root cause (rule 5), for a bug fix: +- Net change in source lines and in public API members: + +### Assumptions + +Each assumption about the service, Lambda, the runtime or another library, with its evidence, or marked "unverified" (rule 6): + +### Decisions not covered by the issue or the design note + +One line each, with the reason (rule 7): + +### Checklist + +- [ ] Every new branch has a test that fails without it (rule 4) +- [ ] For a bug fix: the first commit adds a test that fails on the current code (rule 5) +- [ ] Every edited test expectation is listed in the description, with the reason (rule 5) + +By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. diff --git a/AGENTS.md b/AGENTS.md index 8034ed0c2..859018632 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,8 +88,9 @@ Do NOT add or modify **conformance tests** without coordinating with the team. C extras package with custom operations such as `microvm` (`aws-durable-execution-sdk-python-extras`), and examples (`aws-durable-execution-sdk-python-examples`). -- Read [CONTRIBUTING.md](CONTRIBUTING.md) before making changes. Use - `hatch` for all tests, type checks, and formatting (for example +- Read [CONTRIBUTING.md](CONTRIBUTING.md) before making changes, and + follow its [rules for every change](CONTRIBUTING.md#rules-for-every-change). + Use `hatch` for all tests, type checks, and formatting (for example `hatch run dev-core:test`, `hatch run dev-core:typecheck`, and `hatch fmt --check` from a package directory). - When the developer guide and the installed SDK source disagree, trust diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ce5faaf30..3acdddf12 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -532,6 +532,141 @@ To send us a pull request, please: 6. Send us a pull request, answering any default questions in the pull request interface. 7. Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation. +### Rules for every change + +These rules apply to every package in this repository, to people and to coding agents. A +package can add its own rules in its `AGENTS.md` or README. Many pull requests are now +written with AI help, and AI-written changes tend to add logic. These rules keep each +change as small as the problem. + +#### 1. Keep it simple + +1. Do not add a mechanism only to make a rare case faster. Correctness is required. Speed + in a rare case is not. +2. Do not add configuration that nobody asked for. +3. For a new feature, prefer the simpler option, even if it loses a minor capability of a + more complex option. An existing feature has users, so removing a capability of it is + a breaking change and follows the release rules instead. + +#### 2. Simplify before you add + +1. Every new condition, branch or option adds cases that readers and tests must cover. +2. A bug or a missing use case often exists because the existing logic is more complex + than the problem. A special case hides another case, or two paths do the same job in + different ways. +3. So the first question for a bug fix or a feature is: can a simpler version of the + existing logic cover it? +4. A change that fixes the bug or covers the use case and removes or merges logic is + preferred to a change that adds logic. This holds even when the simpler change + touches more lines. +5. If a change must add logic, its description states which simpler options were + considered and why each one does not work. + +#### 3. Write a design note before the code + +1. A review finds a wrong approach only after the code exists. Rewriting code costs more + than rewriting a note. +2. So a new feature, a new option or a change to a public API starts with a design note. + A bug fix that changes a public API needs one too. +3. The note states, in this order: + 1. the problem, in one or two sentences, without a solution; + 2. the cause; + 3. the options considered, including a simplification of existing logic (rule 2); + 4. the chosen option and the reason; + 5. what the change removes, if anything. +4. A maintainer agrees to the note before the code is written. The note goes in the + issue or in the pull request description. + +#### 4. Every branch has a test that fails without it + +1. Line coverage does not prove that a branch matters. A test can run a branch whose + result changes no assertion. +2. So for each condition and each branch in a change, some test must fail when the branch + is removed or inverted. +3. The reviewer asks of each condition: which test fails if this is deleted? A branch + without an answer is removed, or it gets a test. +4. Defensive code for a case that cannot happen has no such test. So it is removed. + +#### 5. A bug fix starts with a failing test + +1. A test that reproduces the bug proves that the bug exists. The same test proves that + the fix removes it. +2. So a bug fix first adds a test that fails on the current code. The fix then makes it + pass. The pull request description shows the test failing before the fix, or the + commit order shows it. +3. The description names the root cause, not only the symptom. The fix changes the code + at the cause. Example: a value arrives in an unexpected unit. The fix converts it once, + where it enters the code. It does not change every place that reads it. +4. A change that edits the expectation of an existing test lists the edited test and the + reason in its description. + +#### 6. Mark unverified assumptions + +1. A change often depends on behavior outside the package: the service, Lambda, the + language runtime or another library. +2. An assumption about that behavior can be wrong, and the tests then pass anyway, + because a fake of the outside component encodes the same assumption. +3. So every assumption that a change depends on cites its evidence: a run against the + real component, a document or a test against the real component. +4. An assumption without evidence is marked "unverified" in the pull request description. + The reviewer decides whether a check against the real component is needed before + approval. + +#### 7. List the decisions that the task did not cover + +1. An issue or a design note never covers every detail. So the contributor makes some + decisions alone. +2. A decision that is hidden in the code cannot be reviewed. +3. So the pull request description lists every decision that the issue or the design + note did not cover, one line each, with the reason. Examples: a default value, an + error message, a changed edge case, a skipped test. +4. A maintainer accepts or rejects each listed decision. + +#### 8. Repeated review findings mean a redesign + +A review finding is a change request or a blocking issue from a reviewer, a person or an +agent. A round is one review of a revision, followed by the contributor's changes. + +1. A correct and simple change usually passes review after one round of fixes. +2. Sometimes a reviewer finds new problems in the code that fixed the previous findings. + Each fix then adds a case, a condition or a special path. +3. This pattern has a few common causes: + - the feature itself is wrong; + - the implementation is wrong or too complex; + - the change fixes a symptom instead of the cause. +4. More fixes on the same design add complexity and break rule 1. +5. So the trigger is this: the contributor addressed every finding of a round, and the + next review finds new blocking issues. At the trigger, the contributor stops fixing. +6. The contributor then returns to the design phase: + 1. State the problem again in one or two sentences, without the current solution. + 2. Find the cause. Ask why the problem happens until the answer is a fact about the + system, not about the code that the change added. + 3. List the review findings of all rounds. Check whether one cause explains several of + them. + 4. Consider simpler options, including not adding the feature. + 5. Write the chosen design and the reason in the pull request, and get the reviewer's + agreement before writing new code. +7. The new design may keep parts of the old code. It may also replace the change + completely. A smaller change that solves the cause is better than a larger change + that passes review. +8. A coding agent that hits the trigger stops. It reports the findings of all rounds and + its analysis of the cause, and it waits for a decision before it continues. + +#### 9. Reviewers enforce these rules + +1. A reviewer, a person or an agent, checks every change against these rules and the + package's own rules, not only for correctness. +2. A change can be correct and still break a rule. Examples: it adds logic where a + simplification works (rule 2), it has no design note (rule 3), or it has a branch that + no test needs (rule 4). +3. Change requests on such a change produce a better version of the wrong approach. So + for a change that breaks a rule, the reviewer does not ask for detailed code changes. +4. Instead, the reviewer pushes back on the approach. The review names the rule, states + how the change breaks it, and asks the contributor to return to the design phase + (rule 8, step 6). The reviewer can suggest a simpler direction. +5. The reviewer approves only a change that follows every rule. A reviewer who wants an + exception names the rule and the reason in the review, and a maintainer decides. + ### Pull Request Title and Commit Message Format We follow the [Conventional Commits](https://www.conventionalcommits.org/) specification for PR titles and commit messages. This helps us maintain a clear project history and enables automated tooling.