From 5587c43c2a6f887634272128495a6e4c57db3134 Mon Sep 17 00:00:00 2001 From: "bedrock-python-assets-write[bot]" <339090477+bedrock-python-assets-write[bot]@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:43:14 +0000 Subject: [PATCH] chore: sync engineering assets Touchmark-Hub: bedrock-python@github.com/1408350934 Touchmark-Stream: sync Touchmark-Content: sha256:b6d8bdc448a163f9ecb900d4b4acc8f94f7adfcfa1596b44a68b44b6b8c65d92 Touchmark-Hub-Commit: 264f7d16e9012a2a90d07c6929e0d77fc3b5345b --- .agents/guidelines/python.md | 19 ++++++++++++---- .agents/guidelines/testing.md | 7 +++++- .agents/prompts/pure-refactor.md | 30 +++++++++++++++++++++++++ .agents/prompts/refactor-check.md | 2 +- .agents/prompts/review.md | 32 +++++++++++++++++++++------ .claude/agents/reviewer.md | 11 ++++----- .claude/skills/pr/SKILL.md | 5 +++-- .claude/skills/review-change/SKILL.md | 15 ++++++++----- AGENTS.md | 8 ++++++- CLAUDE.md | 4 ++-- 10 files changed, 104 insertions(+), 29 deletions(-) create mode 100644 .agents/prompts/pure-refactor.md diff --git a/.agents/guidelines/python.md b/.agents/guidelines/python.md index f83948b..b7aea05 100644 --- a/.agents/guidelines/python.md +++ b/.agents/guidelines/python.md @@ -1,6 +1,6 @@ # Python guidelines for libraries -Our code runs inside other people's programs. Every choice we make (a dependency, a global, a log line, a warning) becomes theirs too. +Our code runs inside other people's programs. Every choice we make (a dependency, a global, a log line, a warning) becomes theirs too. Where `.agents/project.md` or the surrounding code settles a convention differently (naming, layout, libraries), follow them. ## Code @@ -8,15 +8,26 @@ Our code runs inside other people's programs. Every choice we make (a dependency - Type-annotate the public API completely and ship the `py.typed` marker. Keep the type checker clean in strict mode for the public modules. - Do not do work at import time: no network, no file access, no reading environment variables, no logging configuration. Importing the library must be cheap and free of side effects. - No global mutable state. Configuration goes into objects the caller creates and passes in. -- Raise exceptions from the library's own hierarchy, rooted in one base class, with messages that say what was wrong and what to do. Never `print`, and never call `sys.exit`. -- Log through `logging.getLogger(__name__)` and add no handlers: the application decides where logs go. +- Do not change the arguments the caller passes in; return new values. Builders and accumulators whose job is to be changed are the exception, and say so in their documentation. - Accept the most general type that works (`Iterable`, `Mapping`, `os.PathLike`) and return the most specific one. -- Async APIs, if any, never block the event loop; do not start event loops or threads behind the caller's back. +- Give closed sets of values an `Enum`. A tuple whose meaning depends on position becomes a named type. A boolean parameter that switches between two behaviours is usually two functions. +- Async APIs, if any, never block the event loop; do not start event loops or threads behind the caller's back. Catch `CancelledError` only to clean up and re-raise. +- Name things after what they mean, put units in names (`timeout_seconds`, `size_bytes`), and use one word for one concept across the public API. +- Give every number with a meaning a named constant and a reason. Leave no dead code: unused functions and parameters, commented-out code, TODOs without an issue. + +## Errors and logging + +- Raise exceptions from the library's own hierarchy, rooted in one base class, with messages that say what was wrong and what to do. Never `print`, and never call `sys.exit`. +- Raise on input you did not expect. End a chain of known cases with an `else` that raises (`typing.assert_never` lets the type checker prove every case is handled), so an unknown value never falls through to a default; never hide a failure behind a `None` return. +- Raise specific errors where the problem is found. Do not catch `KeyError`, `TypeError` or `AttributeError` far from where they happen as a stand-in for a real condition, and do not let them escape the public API where your own exception is documented. +- Chain the original exception (`raise LibraryError(...) from exc`) when you wrap one, so the caller keeps the cause. +- Log through `logging.getLogger(__name__)` and add no handlers: the application decides where logs go. Never log the caller's data or secrets. ## Dependencies - Every dependency is imposed on all our users: add one only when it is worth that, and keep version ranges as wide as the code supports. Do not pin exact versions. - Heavy or rarely needed dependencies go into optional extras, imported lazily with a clear error when missing. +- Prefer the standard library; check that it does not already solve the problem before you write a helper. ## Packaging diff --git a/.agents/guidelines/testing.md b/.agents/guidelines/testing.md index 26c2129..eb69bf5 100644 --- a/.agents/guidelines/testing.md +++ b/.agents/guidelines/testing.md @@ -1,6 +1,6 @@ # Testing guidelines for libraries -Users depend on the behaviour we promise. Tests pin that promise down. +Users depend on the behaviour we promise. Tests pin that promise down. Where `.agents/project.md` or the existing tests settle a convention differently (layout, naming, fixtures, tools), follow them. ## What to test @@ -9,6 +9,7 @@ Users depend on the behaviour we promise. Tests pin that promise down. - Cover edge cases of inputs: empty, very large, unicode, `None` where allowed, wrong types where the API promises an error. - Check that errors are the documented exception types with useful messages. - Run the test suite on every supported Python version and operating system the project claims, and against the lowest and the highest versions of dependencies that the version ranges allow. +- Do not test the standard library or the dependencies themselves. ## Kinds of tests @@ -20,6 +21,10 @@ Users depend on the behaviour we promise. Tests pin that promise down. ## How to write them - One behaviour per test, named after what it checks. +- No logic in a test: no `if`, loops over cases, `try`, or skipping depending on the environment beyond the documented platform markers. To cover several inputs, parametrise the test. +- Put the setup in fixtures or small factories, so the test body reads as "call, then check". +- Build expected values by hand. A test that computes them with the code under test proves nothing: do not check a decoder with its own encoder alone. +- Assert exact values and whole results (`assert result == expected`), not fragments (`> 0`, `in`, a substring). Floats compare with a tolerance (`pytest.approx`); an error's message is checked with `pytest.raises(..., match=...)`; property-based tests assert the invariant instead. - No network and no reliance on the machine's locale, time zone or current time; inject or freeze them. - Tests pass in any order and in parallel. - Deprecations: test that the old API still works and emits the documented `DeprecationWarning`, and turn warnings into errors in the test run so new ones are noticed. diff --git a/.agents/prompts/pure-refactor.md b/.agents/prompts/pure-refactor.md new file mode 100644 index 0000000..5f894fb --- /dev/null +++ b/.agents/prompts/pure-refactor.md @@ -0,0 +1,30 @@ +# Check that a change keeps behaviour + +Use this on a change that claims to change no behaviour: a refactoring, a move, a rename, a dependency swap. Assume it does change behaviour until you have traced every path. You are checking, not rewriting: do not edit files unless you are asked to. + +## What to check + +The change you were given; if none was named, the current branch against the default branch, plus uncommitted and untracked changes. Read every changed file in full, old and new, and find every caller of every changed function, including references by string: registries, entry points, configuration, templates, dynamic lookups. Behaviour most often changes at a call site the diff does not show. + +## How to work through it + +Split the change into slices small enough to trace completely: a function, a class, a group of related functions. If your tool can run sub-agents, give each slice to one and run them in parallel; otherwise work through them one at a time and write down each result before the next. Tell each sub-agent this prompt, its slice and how to get the whole change (a range, or the uncommitted and untracked changes); a sub-agent given one slice traces that slice only, and whoever split the change merges the results. + +For every slice, compare the old code with the new, path by path: + +- **Interface:** the same importable names and module paths, parameter names (callers may pass them by keyword), configuration keys and environment variables, command-line flags, routes, and names in stored data and events. +- **Inputs:** the same accepted arguments, types, defaults and validation; the same behaviour on `None`, empty and boundary values. +- **Outputs:** the same return values and types, the same order of collections, the same serialised form (field names, missing versus null, number and date formats). +- **Errors:** the same exceptions for the same conditions, the same error codes and messages reaching callers, the same log level and event. +- **Side effects:** the same writes to storage, the same outgoing requests (payload, headers, order, retries, timeouts), the same metrics and their labels. +- **Order and timing:** the same order of operations wherever it can be observed: transaction boundaries, when a lock is taken, when a stream yields, when a task starts. +- **Code that appeared or disappeared:** a check, a default, a conversion or a log line that runs now and did not before, or the other way round. +- **Tests:** a changed assertion or fixture in a change that claims to keep behaviour is a behaviour change until shown otherwise. + +## How to report + +- **Behaviour changes**: each as `file:line`, the old behaviour, the new one, and how a caller, a user or the stored data would notice. +- **Not fully traced**: what differs and what would settle it. +- **Unchanged**: each slice and what you traced in it. + +End with a one-line verdict: behaviour kept, behaviour changes found, or not settled (and how many paths are not fully traced). A change the author intended still counts if the description does not say so. diff --git a/.agents/prompts/refactor-check.md b/.agents/prompts/refactor-check.md index fa5701f..b03082d 100644 --- a/.agents/prompts/refactor-check.md +++ b/.agents/prompts/refactor-check.md @@ -9,4 +9,4 @@ Answer these questions before you change the structure of code. If an answer is 5. **Reach.** Does it stay inside the code the task is about? Shared modules, public interfaces and changes across many files need their owners' agreement. 6. **Timing.** Will it collide with open pull requests or a release in progress? -When you go ahead, keep refactoring commits apart from behaviour changes, run the tests after every step, and explain in the pull request what moved where and why behaviour is unchanged. +When you go ahead, keep refactoring commits apart from behaviour changes, run the tests after every step, and explain in the pull request what moved where and why behaviour is unchanged. Before you open it, check the refactoring commits with `.agents/prompts/pure-refactor.md`. diff --git a/.agents/prompts/review.md b/.agents/prompts/review.md index 6071456..a2c6d27 100644 --- a/.agents/prompts/review.md +++ b/.agents/prompts/review.md @@ -1,21 +1,35 @@ # Review a change -Review the change as a careful senior engineer of this project would. You are reviewing, not rewriting: do not edit files unless you are asked to. +Review the change as a careful senior engineer of this project would. You are not the author and you owe the change nothing: every addition has to earn its place. You are reviewing, not rewriting: do not edit files unless you are asked to. ## What to review -The change you were given: a pull request, a branch, a commit range or a set of files. If none was named, review the current branch against the default branch, plus uncommitted changes. Read enough of the surrounding code to judge each change in its context, and read `.agents/project.md` for the project's conventions. +The change you were given: a pull request, a branch, a commit range or a set of files. If none was named, review the current branch against the default branch, plus uncommitted and untracked changes. Read `.agents/project.md` for the project's conventions, and the guidelines in `.agents/guidelines/` that cover the code the change touches. + +Read every changed file in full, not only the changed lines, and look up the callers of every function whose behaviour changed. Most problems are in what the change does not fit with: a helper that already exists, a pattern the rest of the module follows, a caller that now breaks. Search for them and cite where they are instead of guessing. + +## How to work through it + +1. **Split the change into slices**, each small enough to read completely: a module or a package with its tests. A small change is one slice. +2. **Review each slice** against the list below. If your tool can run sub-agents, give each slice to one and run them in parallel; otherwise work through the slices one at a time and write down each slice's findings before you start the next. Tell each sub-agent this prompt, its slice and how to get the whole change (a range, or the uncommitted and untracked changes). A sub-agent given one slice does steps 2 and 4 for that slice only; steps 1 and 3 stay with whoever split the change. +3. **Review the change as a whole**, for what no slice can see: + - a second task mixed into the change, or files that do not belong in it (generated output the project does not commit, local notes, debugging leftovers); + - documentation, examples and the changelog that the change makes wrong; + - the same helper written twice, one idea under two names, an abstraction with a single implementation and no test double; + - behaviour that changes although the description says it does not. +4. **Check every finding** at the line you cite. Drop what you cannot confirm, merge duplicates, and list repeats once with their other locations. ## What to look for, most important first -1. **Correctness.** Does the change do what its description says? Check edge cases, error paths, empty and missing inputs, concurrency, time zones and encodings. +1. **Correctness.** Does the change do what its description says? Check edge cases, error paths, empty and missing inputs, concurrency, time zones and encodings. An unexpected input or state must fail loudly, not fall back to a default nobody will notice. 2. **Security.** Input validation, injection, authorisation, secrets in code or logs, unsafe deserialisation, new dependencies and what they can do. 3. **Compatibility and data.** Public APIs, configuration and file formats, database migrations, rollout and rollback. 4. **Tests.** Would a test fail without the change? Are the important cases covered? Do the tests check behaviour rather than implementation details? -5. **Operations.** Logging, metrics, error messages a person can act on, timeouts and retries. -6. **Readability.** Names, structure, duplication, dead code, comments that explain why rather than what. +5. **Necessity.** For every addition, meaning a new public name, parameter, option, configuration key or dependency: what problem does it solve now, and where is it used? "It might be useful later" is a reason to leave it out. +6. **Operations.** Logging, metrics, error messages a person can act on, timeouts and retries. +7. **Readability.** Names, structure, duplication, dead code, comments that explain why rather than what. -Skip style points that a formatter or linter already enforces. +Skip style points that a formatter or linter already enforces. Problems in lines the change adds or modifies are not excused because the code "already was like that"; problems elsewhere in a changed file are at most Consider. ## How to report @@ -25,4 +39,8 @@ Group findings by severity: - **Should fix**: likely problems, missing tests, confusing code. - **Consider**: suggestions and questions. -For each finding give the file and line, what is wrong, why it matters, and a concrete fix. If you are not sure, say so and say what would settle it. End with a one-line verdict. If you found nothing serious, say so plainly: do not invent findings to fill the list. +For each finding give the file and line, what is wrong, why it matters, and a concrete fix. Where the author has to decide, ask the question they must answer ("Where is this option used?", "What happens when the list is empty?"). If you are not sure, say so and say what would settle it. + +Then add **Justify or remove**: every addition (see Necessity), each with the reason you found in the change or its description, or "no reason found". Additions are judged here only, not again among the findings. + +End with a one-line verdict: ready, ready after the "should fix" items, or needs rework. If you found nothing serious, say so plainly: do not invent findings to fill the list. diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index 51f4c2e..2bc8bf1 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -1,12 +1,13 @@ --- name: reviewer -description: Reviews a change in its own context and reports findings by severity, without editing anything. Use for large changes or when an independent review is wanted. +description: Reviews a change, or one slice of it, in its own context and reports findings by severity, without editing anything. Use for large changes, for each slice of a split review, or when an independent review is wanted. tools: Read, Grep, Glob, Bash --- You review code changes in this repository. You never edit files, commit or push. -1. Read `.agents/project.md` for the project's conventions and commands, and `.agents/prompts/review.md` for the checklist. Follow the checklist. Where the profile is blank, find the facts in the repository as `AGENTS.md` says ("Before you start"), and say which ones you found there. -2. Work out what to review from the request. If it names nothing, review the current branch against the default branch, plus uncommitted changes. -3. Use Bash only to read: `git diff`, `git log`, `git show`, and the project's lint or test commands when you were asked to run them. -4. Report findings grouped as Must fix, Should fix and Consider, each with the file and line, the problem, why it matters and a fix. End with a one-line verdict. +1. Read `.agents/project.md` for the project's conventions and commands. Where the profile is blank, find the facts in the repository as `AGENTS.md` says ("Before you start"), and say which ones you found there. +2. Follow the prompt the request names: `.agents/prompts/pure-refactor.md` for a check that behaviour is kept, `.agents/prompts/review.md` otherwise. If the request gives you one slice of a larger change, review that slice completely and read the rest of the change only for context; the whole-change part of the prompt is left to the caller. +3. Work out what to review from the request. If it names nothing, review the current branch against the default branch, plus uncommitted and untracked changes. +4. Use Bash only to read: `git diff`, `git log`, `git show`, and the project's lint or test commands when you were asked to run them. +5. Report as the prompt says, with the file and line of every finding. diff --git a/.claude/skills/pr/SKILL.md b/.claude/skills/pr/SKILL.md index 0c0c7ff..c84d24b 100644 --- a/.claude/skills/pr/SKILL.md +++ b/.claude/skills/pr/SKILL.md @@ -9,7 +9,8 @@ Open a pull request for the current branch. $ARGUMENTS 1. Read `.agents/project.md` for the default branch, the language of pull requests and how they refer to issues. 2. Make sure everything is committed and the project's checks pass. Report what fails and ask before going on. -3. Write the title in the project's format. Write the description from the commits and the diff: what changes and why, how it was tested, risks and how to roll back, and the issue it resolves. Fill in the repository's template if it has one (`.github/pull_request_template.md`, `.gitlab/merge_request_templates/`). -4. Show the title and description and wait for the user's approval. Then push the branch and open the pull request with the forge's command-line tool (`gh`, `glab` or `tea`), or tell the user how to open it. +3. Check that the documentation is still true: search the docs, the README, the examples and the changelog for what the branch renames, removes or changes, and for what it adds that users will need to know. If something is stale or missing, say what and offer to fix it on this branch; after a fix is committed, go back to step 2. +4. Write the title in the project's format. Write the description from the commits and the diff: what changes and why, how it was tested, risks and how to roll back, and the issue it resolves. Fill in the repository's template if it has one (`.github/pull_request_template.md`, `.gitlab/merge_request_templates/`). +5. Show the title and description and wait for the user's approval. Then push the branch and open the pull request with the forge's command-line tool (`gh`, `glab` or `tea`), or tell the user how to open it. Never merge, approve or assign reviewers unless asked. diff --git a/.claude/skills/review-change/SKILL.md b/.claude/skills/review-change/SKILL.md index dc80a56..4dbd946 100644 --- a/.claude/skills/review-change/SKILL.md +++ b/.claude/skills/review-change/SKILL.md @@ -1,11 +1,14 @@ --- name: review-change -description: Review a change for bugs, security problems, compatibility and missing tests, using the project's review checklist. Use when the user asks for a review or a second look at a diff, branch, commit range or pull request. -argument-hint: "[branch, commit range, files or pull request]" +description: Review a change for bugs, security problems, compatibility, unjustified additions and missing tests, using the project's review checklist; with pure-refactor, check that a change keeps behaviour. Use when the user asks for a review, a second look at a diff, branch, commit range or pull request, or whether a refactoring keeps behaviour. +argument-hint: "[branch, commit range, files or pull request] [pure-refactor]" --- -Review $ARGUMENTS by following `.agents/prompts/review.md`. With nothing named, review the current branch against the default branch, plus uncommitted changes. +Review the change that $ARGUMENTS names, leaving out the word `pure-refactor`. With nothing named, review the current branch against the default branch, plus uncommitted and untracked changes. -- Read `.agents/project.md` first for the project's conventions and default branch. -- Do not edit files. Report the findings as the checklist says, grouped by severity. -- For a large change, hand the review to the `reviewer` subagent and pass on its findings. +1. Pick the prompt: `.agents/prompts/pure-refactor.md` when the arguments say `pure-refactor` or the user asks whether behaviour is kept, `.agents/prompts/review.md` otherwise. Read it in full and follow it, including its report. +2. Read `.agents/project.md` for the project's conventions and default branch. +3. Split the change into slices as the prompt says. For a small change, one slice, review it yourself. For more, read only what splitting and the whole-change checks need (`git diff --stat`, the description, the docs it may affect) and give each slice to a `reviewer` subagent, all in parallel. Name in each request the prompt, the slice, and how to get the whole change: a range, or "uncommitted and untracked changes". +4. Merge what the subagents return. With `review.md`, do its whole-change step yourself. Check every finding at the line it cites, then report as the prompt says. + +Do not edit files. diff --git a/AGENTS.md b/AGENTS.md index 107e924..ab3ec47 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,11 +13,16 @@ Instructions for AI coding agents working in this repository. People may find th ## How to work - Make the smallest change that solves the task. Do not rename, reformat or restructure code you were not asked to touch; suggest it separately. +- Before you add a public name, parameter, option, configuration key, file or dependency, know what problem it solves now. Do not add things because they might be useful later. +- Prefer the simplest explicit version: a plain function over a class, a concrete class over an interface that nothing else implements. +- Fail loudly. Stop with an error on input or state you did not expect; do not swallow errors or fall back to a default that hides the problem. +- Keep each decision in one place. When the same condition shows up in several places, make the decision once and let the rest of the code follow its outcome. +- Fix the cause, not the symptom. If a fix has to work around bad data, find out what produced the data. - Follow the conventions of the surrounding code rather than your own preferences. - Keep changes reviewable: one purpose per commit and per pull request. - Change behaviour together with its tests. A bug fix starts with a test that fails without the fix. - Before you call a task done, run the format, lint and test commands from the profile, or those you found in the repository, and report what you ran and what it printed. Never say a check passed if you did not run it. -- Update the documentation and the changelog when something users can see changes. +- Keep the documentation true. Update the documentation and the changelog when something users can see changes; search the docs and examples for what you renamed, removed or changed, and fix them in the same change. - Use the standard library and the dependencies the project already has. Adding a dependency is a decision for people: ask first. - When the guidelines in `.agents/guidelines/` cover what you are doing, follow them. @@ -36,3 +41,4 @@ This file and some others come from your organisation's engineering-assets hub, - Reviewing a change: follow `.agents/prompts/review.md`. - Before refactoring: answer `.agents/prompts/refactor-check.md`. +- Checking that a change keeps behaviour, such as a refactoring: follow `.agents/prompts/pure-refactor.md`. diff --git a/CLAUDE.md b/CLAUDE.md index 088c060..b7fb17c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,6 +7,6 @@ The instructions for every coding agent are in AGENTS.md, and the facts about th ## Claude Code -- Skills: `/review-change` for a review; `/spec` before a larger change; `/new-branch`, `/commit` and `/pr`, which change the repository or the forge, run only when you invoke them. -- The `reviewer` subagent reviews a large change in a context of its own. +- Skills: `/review-change` for a review, `/review-change pure-refactor` to check that a change keeps behaviour; `/spec` before a larger change; `/new-branch`, `/commit` and `/pr`, which change the repository or the forge, run only when you invoke them. +- The `reviewer` subagent reviews a change, or one slice of a large one, in a context of its own; `/review-change` runs one per slice in parallel. - `.claude/settings.json` only denies access to secrets. Keep your own permissions in `.claude/settings.local.json`, which is not committed.