From 74e4eb497973e69773f1ddca557c3c0f8062fb36 Mon Sep 17 00:00:00 2001 From: yaythomas Date: Sun, 25 Oct 2026 19:28:04 +0000 Subject: [PATCH] docs: add rules for every change Copy the rules for every change from the JS SDK, so both SDKs ask the same of people and coding agents. - CONTRIBUTING.md gets the section "Rules for every change" with the same 9 rules. Two sentences are left out because they point to JS-only material: the ADR folder and the SDK invariants section. - A new .github/pull_request_template.md replaces the aws organization's default template for this repository. It keeps the default's prompts and license sentence, and adds the Design, Assumptions and Decisions sections and three checklist items from the JS template. - AGENTS.md links to the new section. Documentation only. --- .github/pull_request_template.md | 28 +++++++ AGENTS.md | 5 +- CONTRIBUTING.md | 135 +++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+), 2 deletions(-) create mode 100644 .github/pull_request_template.md 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.