From 65fdeb371d5dea3d239735ae2744d6e8cfe25856 Mon Sep 17 00:00:00 2001 From: eavanvalkenburg Date: Tue, 3 Nov 2026 14:02:56 +0100 Subject: [PATCH 1/2] docs: strengthen Python API design and review guidance --- .github/instructions/python.instructions.md | 20 +++++++ .github/skills/pull-requests/SKILL.md | 16 +++++ CONTRIBUTING.md | 14 +++-- python/.github/skills/pull-requests/SKILL.md | 16 +++++ python/CODING_STANDARD.md | 62 ++++++++++++++++++++ 5 files changed, 122 insertions(+), 6 deletions(-) diff --git a/.github/instructions/python.instructions.md b/.github/instructions/python.instructions.md index 096015c4320..0089801240f 100644 --- a/.github/instructions/python.instructions.md +++ b/.github/instructions/python.instructions.md @@ -16,3 +16,23 @@ for task-specific architecture and workflow guidance. performance, or usability impact. - Do not report `MAY` choices as violations. - Do not duplicate purely mechanical findings that Ruff, Pyright, or another required CI check will report. + +## Semantic API Review + +For public API and integration changes, apply the standards in this order: + +1. Start from public imports and the smallest realistic initialization example. Check the common path and each + materially different execution mode before reading implementation details. +2. Trace parameters through construction, factories, request preparation, execution, persistence, and cleanup. + Look for accepted-but-unused settings and differences between execution paths. +3. Check whether names expose required inputs, data direction, and ownership without relying on constructor conditions. +4. Identify resource, history, continuation, cache, approval, and checkpoint ownership where applicable. Check + cross-request sharing and failure windows. +5. Review observable compatibility, including defaults, wire shapes, option precedence, retention, and failure + behavior, not only signature-checker results. +6. Match evidence to the claim. Offline tests do not establish live deployment behavior. An upstream fix needs + release evidence. An investigation result is not approval for its proposed implementation. + +Findings should identify the affected behavior, a concrete scenario, and its consequence. Do not prescribe a preferred +architecture as a correctness finding when multiple designs satisfy the contract. Treat unresolved alternatives as +design questions. Review author-supplied focus notes as context, not a limit on review scope. diff --git a/.github/skills/pull-requests/SKILL.md b/.github/skills/pull-requests/SKILL.md index 6b968a288dc..a727e075686 100644 --- a/.github/skills/pull-requests/SKILL.md +++ b/.github/skills/pull-requests/SKILL.md @@ -32,6 +32,18 @@ prompts: reviewers only**. Automated/AI reviewers must ignore it and review the entire change rather than narrowing scope to it. +For material Python public-API changes, include the following within this section: +- A short before/after usage example with public imports and initialization. +- The affected execution modes and their intentional differences. +- Changes to defaults, precedence, ownership, persistence, or compatibility. +- The agreed decision or investigation that supports the direction. An investigation + supplies evidence; its proposed implementation still needs maintainer agreement. + +Use an options matrix only when interacting settings make it useful. Do not add a +separate mandatory section or repeat the canonical +[Python coding standards](../../../python/CODING_STANDARD.md). For semantic review, +follow the [Python review guidance](../../instructions/python.instructions.md). + ### `### Related Issue` Link the issue the PR fixes using a GitHub closing keyword (`Fixes #123` / `Closes #123`) so it closes automatically on merge. For a non-trivial change, @@ -40,6 +52,10 @@ agreed with the direction in an issue comment. A PR with no agreed issue may be closed without detailed review. Before opening, confirm there is no other open PR for the same issue; if there is, explain how this PR differs. +A team-only issue, assignment, investigation, `/plan` comment, or suggested option +does not authorize implementation. Check the agreed scope and readiness; team-only +work remains unavailable for community pickup unless maintainers explicitly open it. + For a trivial change that does not require an issue, replace the `Fixes #` placeholder with `N/A - trivial change` and briefly explain why the change is an obvious correction whose desired result is not reasonably in dispute. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 74b7c71d926..dca225eca5f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -100,11 +100,12 @@ DO's: - **DO** follow the standard coding conventions - [.NET](https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) - - [Python](https://pypi.org/project/black/) + - [Python](./python/CODING_STANDARD.md) -- **DO** give priority to the current style of the project or file you're changing - if it diverges from the general guidelines. -- **DO** use the pre-commit hooks for python to ensure proper formatting. +- **DO** follow the repository's language-specific standards when local file style + diverges. Do not copy non-conforming legacy patterns. +- **DO** follow the [Python code-quality guidance](./python/.github/skills/python-code-quality/SKILL.md) + for formatting, linting, and repository hooks. - **DO** include tests when adding new features. When fixing bugs, start with adding a test that highlights how the current behavior is broken. - **DO** keep the discussions focused. When a new or related topic comes up @@ -128,8 +129,9 @@ DON'Ts: ### Breaking Changes Contributions must maintain API signature and behavioral compatibility. Contributions -that include breaking changes will be rejected. Please file an issue to discuss -your idea or change if you believe that a breaking change is warranted. +that include breaking changes require explicit maintainer approval and documentation. +Please file an issue and agree on the direction before implementing a breaking change. +A breaking-change label or title declares intent; it does not grant design approval. #### Python Public API Compatibility diff --git a/python/.github/skills/pull-requests/SKILL.md b/python/.github/skills/pull-requests/SKILL.md index 9d8395cdabe..d217d786a2e 100644 --- a/python/.github/skills/pull-requests/SKILL.md +++ b/python/.github/skills/pull-requests/SKILL.md @@ -32,6 +32,18 @@ prompts: reviewers only**. Automated/AI reviewers must ignore it and review the entire change rather than narrowing scope to it. +For material Python public-API changes, include the following within this section: +- A short before/after usage example with public imports and initialization. +- The affected execution modes and their intentional differences. +- Changes to defaults, precedence, ownership, persistence, or compatibility. +- The agreed decision or investigation that supports the direction. An investigation + supplies evidence; its proposed implementation still needs maintainer agreement. + +Use an options matrix only when interacting settings make it useful. Do not add a +separate mandatory section or repeat the canonical +[Python coding standards](../../../CODING_STANDARD.md). For semantic review, +follow the [Python review guidance](../../../../.github/instructions/python.instructions.md). + ### `### Related Issue` Link the issue the PR fixes using a GitHub closing keyword (`Fixes #123` / `Closes #123`) so it closes automatically on merge. For a non-trivial change, @@ -40,6 +52,10 @@ agreed with the direction in an issue comment. A PR with no agreed issue may be closed without detailed review. Before opening, confirm there is no other open PR for the same issue; if there is, explain how this PR differs. +A team-only issue, assignment, investigation, `/plan` comment, or suggested option +does not authorize implementation. Check the agreed scope and readiness; team-only +work remains unavailable for community pickup unless maintainers explicitly open it. + For a trivial change that does not require an issue, replace the `Fixes #` placeholder with `N/A - trivial change` and briefly explain why the change is an obvious correction whose desired result is not reasonably in dispute. diff --git a/python/CODING_STANDARD.md b/python/CODING_STANDARD.md index 9dfdea16cf5..1ba76dfebb6 100644 --- a/python/CODING_STANDARD.md +++ b/python/CODING_STANDARD.md @@ -179,6 +179,26 @@ def create_agent( - The API MUST document who owns the mapping, where it is forwarded, and whether values are copied or mutated. - Wrappers MUST NOT accept or forward generic keyword arguments they do not use. +### Execution Modes and Configuration + +- Public APIs SHOULD make supported execution modes and their required inputs apparent from signatures and types. +- When modes require substantially different parameters, lifecycle rules, or state contracts, authors SHOULD compare + separate entry points, typed alternatives, and a combined signature. +- A long list of conditional parameter requirements SHOULD trigger an API-design review. Additional documentation + alone does not resolve an unclear API. Configuration classes must still satisfy the Import-Light Common Paths rules. +- Invalid combinations MUST fail before execution or external side effects when the required information is already + available. Factory-dependent validation MUST occur before running the resolved object. + +### Names, Direction, and Ownership + +- Names SHOULD identify the operation's direction and the object or layer it affects. +- Related settings SHOULD use the same term for the same concept. A different term requires a meaningful semantic + distinction. +- Wrappers SHOULD distinguish framework behavior, hosting-platform behavior, and downstream-service behavior. +- Public renames MUST follow the compatibility policy; improved terminology alone does not authorize a breaking change. +- Related APIs across protocols or languages SHOULD use consistent concepts and terminology, without forcing identical + signatures where their contracts or language conventions differ. + ### Interfaces and Reuse - Behavioral contracts SHOULD use `Protocol`. @@ -187,6 +207,15 @@ def create_agent( - Composition and inheritance are both acceptable. Choose based on substitutability, lifecycle, and clarity rather than a blanket preference. +### Compatibility and Feature Lifecycle + +- Compatibility review MUST cover behavior as well as signatures, including defaults, accepted combinations, wire + formats, persistence, and resource ownership. +- Upstream dependency stability MUST NOT be treated as evidence that an integration feature is stable or that a + required fix is available. +- Dependency-specific capability claims MUST be supported by the required dependency version and relevant behavioral + evidence. + ## Asynchronous Code and Resource Ownership - Use asynchronous APIs for I/O and concurrency. Keep purely synchronous work synchronous. @@ -198,6 +227,27 @@ def create_agent( - Do not close caller-injected resources. - Use synchronous or asynchronous context managers when lifecycle management is part of correct use. +### Factories and Identity-Dependent Resources + +- Factory APIs MUST document invocation frequency, access to request context, and ownership of returned objects and + their resources. +- State required across requests MUST NOT depend on request-owned object identity unless that lifetime is explicitly + guaranteed. +- Identity-dependent caches and connections MUST have an explicit isolation scope and invalidation or rebinding + contract. +- Examples MUST follow the same ownership contract as package code; sample brevity does not justify unsafe sharing. + +## State and Continuation Contracts + +- APIs that manage state MUST identify the authority for each kind of state and the purpose of each continuation + identifier. +- Independently configurable state layers MUST document their interaction, including duplicate persistence, history + divergence, branching restrictions, and unsupported combinations where applicable. +- State transitions with externally visible consequences MUST define when ownership is claimed, when state becomes + durable, and what happens after failure or retry. +- Checkpointing or retry support MUST NOT be described as exactly-once execution unless that guarantee also covers + external side effects. + ## Exception Hierarchy The authoritative classes live in @@ -304,6 +354,18 @@ Package-governance decisions and detailed validation commands live in ## Documentation +### Adapter and Option Contracts + +- Public adapter parameters MUST document whether the adapter interprets, translates, forwards, overrides, or ignores + their values, including unsupported or ineffective settings. +- Documentation for upstream SDK types SHOULD link to the upstream contract and explain adapter-specific behavior + or limitations. +- APIs with multiple configuration sources MUST document precedence and distinguish construction defaults from + per-run overrides. +- Option hooks MUST document their inputs, return contract, invocation timing, supported execution paths, mutation + behavior, and validation applied after the hook. +- Examples SHOULD include public imports and show the smallest complete initialization for the scenario. + ### Module Docstrings - Every public module, including public `__init__.py` modules, MUST have a concise module docstring focused on its From 47ee09b654ef09340142a1f1b43a8b5b80a40faf Mon Sep 17 00:00:00 2001 From: eavanvalkenburg Date: Wed, 4 Nov 2026 16:09:34 +0100 Subject: [PATCH 2/2] docs: clarify parameter validation and deduplicate PR guidance --- .github/instructions/python.instructions.md | 4 +++- python/.github/skills/pull-requests/SKILL.md | 18 ++++-------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/.github/instructions/python.instructions.md b/.github/instructions/python.instructions.md index 0089801240f..0e3a45ce60b 100644 --- a/.github/instructions/python.instructions.md +++ b/.github/instructions/python.instructions.md @@ -24,7 +24,9 @@ For public API and integration changes, apply the standards in this order: 1. Start from public imports and the smallest realistic initialization example. Check the common path and each materially different execution mode before reading implementation details. 2. Trace parameters through construction, factories, request preparation, execution, persistence, and cleanup. - Look for accepted-but-unused settings and differences between execution paths. + Look for accepted-but-unused settings, mutually exclusive parameter combinations, and differences between execution + paths. Check that invalid combinations are rejected before execution or external side effects when the required + information is available. 3. Check whether names expose required inputs, data direction, and ownership without relying on constructor conditions. 4. Identify resource, history, continuation, cache, approval, and checkpoint ownership where applicable. Check cross-request sharing and failure windows. diff --git a/python/.github/skills/pull-requests/SKILL.md b/python/.github/skills/pull-requests/SKILL.md index d217d786a2e..b887bba384c 100644 --- a/python/.github/skills/pull-requests/SKILL.md +++ b/python/.github/skills/pull-requests/SKILL.md @@ -32,17 +32,8 @@ prompts: reviewers only**. Automated/AI reviewers must ignore it and review the entire change rather than narrowing scope to it. -For material Python public-API changes, include the following within this section: -- A short before/after usage example with public imports and initialization. -- The affected execution modes and their intentional differences. -- Changes to defaults, precedence, ownership, persistence, or compatibility. -- The agreed decision or investigation that supports the direction. An investigation - supplies evidence; its proposed implementation still needs maintainer agreement. - -Use an options matrix only when interacting settings make it useful. Do not add a -separate mandatory section or repeat the canonical -[Python coding standards](../../../CODING_STANDARD.md). For semantic review, -follow the [Python review guidance](../../../../.github/instructions/python.instructions.md). +For material Python public-API changes, follow the additional guidance in the +[root PR skill's Description & Review Guide section](../../../../.github/skills/pull-requests/SKILL.md#-description--review-guide). ### `### Related Issue` Link the issue the PR fixes using a GitHub closing keyword (`Fixes #123` / @@ -52,9 +43,8 @@ agreed with the direction in an issue comment. A PR with no agreed issue may be closed without detailed review. Before opening, confirm there is no other open PR for the same issue; if there is, explain how this PR differs. -A team-only issue, assignment, investigation, `/plan` comment, or suggested option -does not authorize implementation. Check the agreed scope and readiness; team-only -work remains unavailable for community pickup unless maintainers explicitly open it. +For team-only work and investigation or planning outcomes, follow the approval +boundaries in the [root PR skill's Related Issue section](../../../../.github/skills/pull-requests/SKILL.md#-related-issue). For a trivial change that does not require an issue, replace the `Fixes #` placeholder with `N/A - trivial change` and briefly explain why the change is an