From 5a74e5104d5168e896cf7a55df65aa09793dd1c0 Mon Sep 17 00:00:00 2001 From: Henry Su Date: Fri, 24 Jul 2026 17:01:19 -0500 Subject: [PATCH 01/10] fix: await async callable-object dynamic instructions (#3942) --- src/agents/agent.py | 12 ++++---- src/agents/realtime/agent.py | 15 ++++++---- tests/realtime/test_agent.py | 24 +++++++++++++++ tests/test_agent_config.py | 7 +++++ tests/test_agent_instructions_signature.py | 34 ++++++++++++++++++++++ 5 files changed, 81 insertions(+), 11 deletions(-) diff --git a/src/agents/agent.py b/src/agents/agent.py index f5977d9e54..e5b61aaa71 100644 --- a/src/agents/agent.py +++ b/src/agents/agent.py @@ -1003,11 +1003,13 @@ async def get_system_prompt(self, run_context: RunContextWrapper[TContext]) -> s f"but got {len(params)}: {[p.name for p in params]}" ) - # Call the instructions function properly - if inspect.iscoroutinefunction(self.instructions): - return await cast(Awaitable[str], self.instructions(run_context, self)) - else: - return cast(str, self.instructions(run_context, self)) + # Call once, then await if needed. Callable instances with async + # ``__call__`` are not coroutine functions, so checking + # ``iscoroutinefunction(self.instructions)`` would skip the await. + result = self.instructions(run_context, self) + if inspect.isawaitable(result): + return await result + return result elif self.instructions is not None: logger.error( diff --git a/src/agents/realtime/agent.py b/src/agents/realtime/agent.py index fe2f9e7200..0fcead874b 100644 --- a/src/agents/realtime/agent.py +++ b/src/agents/realtime/agent.py @@ -2,9 +2,9 @@ import dataclasses import inspect -from collections.abc import Awaitable, Callable +from collections.abc import Callable from dataclasses import dataclass, field -from typing import Any, Generic, cast +from typing import Any, Generic from agents.prompts import Prompt @@ -121,10 +121,13 @@ async def get_system_prompt(self, run_context: RunContextWrapper[TContext]) -> s if isinstance(self.instructions, str): return self.instructions elif callable(self.instructions): - if inspect.iscoroutinefunction(self.instructions): - return await cast(Awaitable[str], self.instructions(run_context, self)) - else: - return cast(str, self.instructions(run_context, self)) + # Call once, then await if needed. Callable instances with async + # ``__call__`` are not coroutine functions, so checking + # ``iscoroutinefunction(self.instructions)`` would skip the await. + result = self.instructions(run_context, self) + if inspect.isawaitable(result): + return await result + return result elif self.instructions is not None: if _debug.DONT_LOG_MODEL_DATA: logger.error("Instructions must be a string or a function") diff --git a/tests/realtime/test_agent.py b/tests/realtime/test_agent.py index bc2a4c408c..8f58de19db 100644 --- a/tests/realtime/test_agent.py +++ b/tests/realtime/test_agent.py @@ -30,6 +30,30 @@ def _instructions(ctx, agt) -> str: assert instructions == "Dynamic" +@pytest.mark.asyncio +async def test_async_callable_object_instructions_are_awaited(): + """Callable instances whose ``__call__`` is async must be awaited. + + ``inspect.iscoroutinefunction`` returns ``False`` for the instance itself, so the + previous implementation returned the unawaited coroutine as the system prompt. + """ + + class AsyncInstructions: + def __init__(self) -> None: + self.calls = 0 + + async def __call__(self, ctx, agt) -> str: + self.calls += 1 + assert ctx.context is None + return "Dynamic async callable" + + instructions = AsyncInstructions() + agent = RealtimeAgent(name="test", instructions=instructions) + prompt = await agent.get_system_prompt(RunContextWrapper(context=None)) + assert prompt == "Dynamic async callable" + assert instructions.calls == 1 + + @pytest.mark.asyncio @pytest.mark.parametrize("redacted", [True, False]) async def test_mutated_invalid_instructions_respect_model_data_policy( diff --git a/tests/test_agent_config.py b/tests/test_agent_config.py index f935cfd7a7..580a08d1f4 100644 --- a/tests/test_agent_config.py +++ b/tests/test_agent_config.py @@ -33,6 +33,13 @@ async def async_instructions(agent: Agent[None], context: RunContextWrapper[None agent = agent.clone(instructions=async_instructions) assert await agent.get_system_prompt(context) == "async_123" + class AsyncCallableInstructions: + async def __call__(self, context: RunContextWrapper[None], agent: Agent[None]) -> str: + return "async_callable_123" + + agent = agent.clone(instructions=AsyncCallableInstructions()) + assert await agent.get_system_prompt(context) == "async_callable_123" + @pytest.mark.asyncio async def test_handoff_with_agents(): diff --git a/tests/test_agent_instructions_signature.py b/tests/test_agent_instructions_signature.py index 79c56018f9..c4239ecf62 100644 --- a/tests/test_agent_instructions_signature.py +++ b/tests/test_agent_instructions_signature.py @@ -35,6 +35,40 @@ def valid_instructions(context, agent): result = await agent.get_system_prompt(mock_run_context) assert result == "Valid sync instructions" + @pytest.mark.asyncio + async def test_async_callable_object_is_awaited(self, mock_run_context): + """Callable instances whose ``__call__`` is async must be awaited. + + ``inspect.iscoroutinefunction`` returns ``False`` for the instance itself, so the + previous implementation returned the unawaited coroutine as the system prompt. + """ + + class AsyncInstructions: + def __init__(self) -> None: + self.calls = 0 + + async def __call__(self, context, agent) -> str: + self.calls += 1 + return "Valid async callable instructions" + + instructions = AsyncInstructions() + agent = Agent(name="test_agent", instructions=instructions) + result = await agent.get_system_prompt(mock_run_context) + assert result == "Valid async callable instructions" + assert instructions.calls == 1 + + @pytest.mark.asyncio + async def test_sync_callable_object_still_works(self, mock_run_context): + """Sync callable instances should continue to work as dynamic instructions.""" + + class SyncInstructions: + def __call__(self, context, agent) -> str: + return "Valid sync callable instructions" + + agent = Agent(name="test_agent", instructions=SyncInstructions()) + result = await agent.get_system_prompt(mock_run_context) + assert result == "Valid sync callable instructions" + @pytest.mark.asyncio async def test_one_parameter_raises_error(self, mock_run_context): """Test that function with only one parameter raises TypeError""" From a6d577c99ddcb2fd0972bfad790e3e48d3483adf Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 07:26:53 +0900 Subject: [PATCH 02/10] fix: await async callable-object callbacks (#3944) --- src/agents/realtime/handoffs.py | 14 ++++---- src/agents/run_internal/turn_resolution.py | 12 +++---- tests/realtime/test_realtime_handoffs.py | 28 ++++++++++++++++ tests/test_tool_use_behavior.py | 37 ++++++++++++++++++++++ 4 files changed, 75 insertions(+), 16 deletions(-) diff --git a/src/agents/realtime/handoffs.py b/src/agents/realtime/handoffs.py index a26126df97..7cc150d631 100644 --- a/src/agents/realtime/handoffs.py +++ b/src/agents/realtime/handoffs.py @@ -166,16 +166,14 @@ async def _invoke_handoff( strict=True, ) input_func = cast(OnHandoffWithInput[THandoffInput], on_handoff) - if inspect.iscoroutinefunction(input_func): - await input_func(ctx, validated_input) - else: - input_func(ctx, validated_input) + result = input_func(ctx, validated_input) + if inspect.isawaitable(result): + await result elif on_handoff is not None: no_input_func = cast(OnHandoffWithoutInput, on_handoff) - if inspect.iscoroutinefunction(no_input_func): - await no_input_func(ctx) - else: - no_input_func(ctx) + result = no_input_func(ctx) + if inspect.isawaitable(result): + await result return agent diff --git a/src/agents/run_internal/turn_resolution.py b/src/agents/run_internal/turn_resolution.py index 2f54478e32..e55436c295 100644 --- a/src/agents/run_internal/turn_resolution.py +++ b/src/agents/run_internal/turn_resolution.py @@ -688,14 +688,10 @@ async def check_for_final_output_from_tools( ) return ToolsToFinalOutputResult(is_final_output=False, final_output=None) elif callable(agent.tool_use_behavior): - if inspect.iscoroutinefunction(agent.tool_use_behavior): - return await cast( - Awaitable[ToolsToFinalOutputResult], - agent.tool_use_behavior(context_wrapper, tool_results), - ) - return cast( - ToolsToFinalOutputResult, agent.tool_use_behavior(context_wrapper, tool_results) - ) + result = agent.tool_use_behavior(context_wrapper, tool_results) + if inspect.isawaitable(result): + return await result + return result logger.error("Invalid tool_use_behavior: %s", agent.tool_use_behavior) raise UserError(f"Invalid tool_use_behavior: {agent.tool_use_behavior}") diff --git a/tests/realtime/test_realtime_handoffs.py b/tests/realtime/test_realtime_handoffs.py index 952f79c7f2..4c5fc6e800 100644 --- a/tests/realtime/test_realtime_handoffs.py +++ b/tests/realtime/test_realtime_handoffs.py @@ -263,6 +263,34 @@ async def on_handoff(ctx: RunContextWrapper[Any]) -> None: assert called == [True] +@pytest.mark.asyncio +async def test_realtime_handoff_async_callable_objects_are_awaited() -> None: + class WithInput: + def __init__(self) -> None: + self.calls: list[int] = [] + + async def __call__(self, ctx: RunContextWrapper[Any], value: int) -> None: + self.calls.append(value) + + class NoInput: + def __init__(self) -> None: + self.calls = 0 + + async def __call__(self, ctx: RunContextWrapper[Any]) -> None: + self.calls += 1 + + rt = RealtimeAgent(name="async_callable") + with_input = WithInput() + with_input_handoff = realtime_handoff(rt, on_handoff=with_input, input_type=int) + assert await with_input_handoff.on_invoke_handoff(RunContextWrapper(None), "7") is rt + assert with_input.calls == [7] + + no_input = NoInput() + no_input_handoff = realtime_handoff(rt, on_handoff=no_input) + assert await no_input_handoff.on_invoke_handoff(RunContextWrapper(None), "") is rt + assert no_input.calls == 1 + + class StrictInput(BaseModel): name: str age: int diff --git a/tests/test_tool_use_behavior.py b/tests/test_tool_use_behavior.py index de7f98b40f..2162baafd4 100644 --- a/tests/test_tool_use_behavior.py +++ b/tests/test_tool_use_behavior.py @@ -138,6 +138,43 @@ async def behavior( assert result.final_output == "async_custom" +@pytest.mark.asyncio +async def test_custom_tool_use_behavior_async_callable_object() -> None: + """Async callable objects should be awaited and invoked exactly once.""" + + class Behavior: + def __init__(self) -> None: + self.calls = 0 + + async def __call__( + self, + context: RunContextWrapper, + results: list[FunctionToolResult], + ) -> ToolsToFinalOutputResult: + self.calls += 1 + assert len(results) == 2 + return ToolsToFinalOutputResult( + is_final_output=True, + final_output="async_callable", + ) + + behavior = Behavior() + agent = Agent(name="test", tool_use_behavior=behavior) + tool_results = [ + _make_function_tool_result(agent, "ignored1"), + _make_function_tool_result(agent, "ignored2"), + ] + result = await run_loop.check_for_final_output_from_tools( + agent=agent, + tool_results=tool_results, + context_wrapper=RunContextWrapper(context=None), + ) + + assert result.is_final_output is True + assert result.final_output == "async_callable" + assert behavior.calls == 1 + + @pytest.mark.asyncio async def test_invalid_tool_use_behavior_raises() -> None: """If tool_use_behavior is invalid, we should raise a UserError.""" From fc0c8d33f2951f473d775a01289d38389e792a94 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 08:05:35 +0900 Subject: [PATCH 03/10] fix: isolate unit tests from ambient proxy settings (#3945) --- tests/conftest.py | 30 +++++++++++++++++++++++++++-- tests/test_test_environment.py | 35 ++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 tests/test_test_environment.py diff --git a/tests/conftest.py b/tests/conftest.py index c279b6c9ef..de07690f7e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,8 @@ from __future__ import annotations +import os import sys +from collections.abc import MutableMapping import pytest @@ -13,6 +15,32 @@ from .testing_processor import SPAN_PROCESSOR_TESTING +_PROXY_ENVIRONMENT_VARIABLES = ( + "ALL_PROXY", + "HTTP_PROXY", + "HTTPS_PROXY", + "all_proxy", + "http_proxy", + "https_proxy", +) +_PROXY_OPT_IN_ENVIRONMENT_VARIABLE = "OPENAI_AGENTS_TEST_USE_PROXY" + + +def _remove_ambient_proxy_environment(environment: MutableMapping[str, str]) -> None: + """Keep unit tests independent from host proxy configuration.""" + if environment.get(_PROXY_OPT_IN_ENVIRONMENT_VARIABLE, "").lower() in { + "1", + "true", + "yes", + }: + return + + for variable in _PROXY_ENVIRONMENT_VARIABLES: + environment.pop(variable, None) + + +_remove_ambient_proxy_environment(os.environ) + collect_ignore: list[str] = [] if sys.platform == "win32": @@ -50,8 +78,6 @@ def setup_span_processor(): # monkeypatch.delenv("OPENAI_API_KEY", ...) to remove it locally. @pytest.fixture(scope="session", autouse=True) def ensure_openai_api_key(): - import os - if not os.environ.get("OPENAI_API_KEY"): os.environ["OPENAI_API_KEY"] = "test_key" diff --git a/tests/test_test_environment.py b/tests/test_test_environment.py new file mode 100644 index 0000000000..dac226337f --- /dev/null +++ b/tests/test_test_environment.py @@ -0,0 +1,35 @@ +from __future__ import annotations + +import pytest + +from .conftest import ( + _PROXY_ENVIRONMENT_VARIABLES, + _PROXY_OPT_IN_ENVIRONMENT_VARIABLE, + _remove_ambient_proxy_environment, +) + + +def test_remove_ambient_proxy_environment_clears_proxy_variables() -> None: + environment = { + variable: "socks5h://127.0.0.1:1234" for variable in _PROXY_ENVIRONMENT_VARIABLES + } + environment["UNRELATED"] = "preserved" + + _remove_ambient_proxy_environment(environment) + + assert all(variable not in environment for variable in _PROXY_ENVIRONMENT_VARIABLES) + assert environment["UNRELATED"] == "preserved" + + +@pytest.mark.parametrize("opt_in", ["1", "true", "TRUE", "yes", "YES"]) +def test_remove_ambient_proxy_environment_preserves_proxy_variables_when_opted_in( + opt_in: str, +) -> None: + environment = { + _PROXY_OPT_IN_ENVIRONMENT_VARIABLE: opt_in, + "ALL_PROXY": "socks5h://127.0.0.1:1234", + } + + _remove_ambient_proxy_environment(environment) + + assert environment["ALL_PROXY"] == "socks5h://127.0.0.1:1234" From e16ba7ea8a41c2db9a125de221295003c454ed75 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 09:31:28 +0900 Subject: [PATCH 04/10] chore: update AGENTS.md and code change/reiew skill details --- .../skills/implementation-strategy/SKILL.md | 37 ++++++++++++++----- AGENTS.md | 13 ++++++- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/.agents/skills/implementation-strategy/SKILL.md b/.agents/skills/implementation-strategy/SKILL.md index a18cf574f2..221c23d5ed 100644 --- a/.agents/skills/implementation-strategy/SKILL.md +++ b/.agents/skills/implementation-strategy/SKILL.md @@ -1,25 +1,34 @@ --- name: implementation-strategy -description: Decide how to implement runtime and API changes in openai-agents-python before editing code. Use when a task changes exported APIs, runtime behavior, serialized state, tests, or docs and you need to choose the compatibility boundary, whether shims or migrations are warranted, and when unreleased interfaces can be rewritten directly. +description: Decide how to implement or review runtime and API changes in openai-agents-python. Use when a task changes or reviews exported APIs, runtime behavior, serialized state, tests, or docs and you need to choose the compatibility boundary, the smallest coherent implementation, whether shims or migrations are warranted, and when unreleased interfaces can be rewritten directly. --- # Implementation Strategy ## Overview -Use this skill before editing code when the task changes runtime behavior or anything that might look like a compatibility concern. The goal is to keep implementations simple while protecting real released contracts. +Use this skill before editing or reviewing code when the task changes runtime behavior or anything that might look like a compatibility concern. The goal is to keep implementations and review requests focused while protecting real released contracts. ## Quick start -1. Identify the surface you are changing: released public API, unreleased branch-local API, internal helper, persisted schema, wire protocol, CLI/config/env surface, or docs/examples only. -2. Determine the latest release boundary from `origin` first, and only fall back to local tags when remote tags are unavailable: +1. Identify the surface you are changing or reviewing: released public API, unreleased branch-local API, internal helper, persisted schema, wire protocol, CLI/config/env surface, or docs/examples only. +2. Define the concrete required outcome, supported behavior that must remain, and work that is outside the current task. +3. Determine the latest release boundary from `origin` first, and only fall back to local tags when remote tags are unavailable: ```bash BASE_TAG="$(.agents/skills/final-release-review/scripts/find_latest_release_tag.sh origin 'v*' 2>/dev/null || git tag -l 'v*' --sort=-v:refname | head -n1)" echo "$BASE_TAG" ``` -3. Judge breaking-change risk against that latest release tag, not against unreleased branch churn or post-tag changes already on `main`. If the command fell back to local tags, treat the result as potentially stale and say so. -4. Prefer the simplest implementation that satisfies the current task. Update callers, tests, docs, and examples directly instead of preserving superseded unreleased interfaces. -5. Add a compatibility layer only when there is a concrete released consumer, an otherwise supported durable external state boundary that requires it, or when the user explicitly asks for a migration path. +4. Judge breaking-change risk against that latest release tag, not against unreleased branch churn or post-tag changes already on `main`. If the command fell back to local tags, treat the result as potentially stale and say so. +5. Apply the scope and simplicity rules below to choose the implementation or review recommendation. +6. Add a compatibility layer only when there is a concrete released consumer, an otherwise supported durable external state boundary that requires it, or when the user explicitly asks for a migration path. + +## Scope and simplicity rules + +- Make the smallest coherent change that fully satisfies the current task and preserves required supported behavior. +- Prefer existing patterns and direct implementations. Add a new abstraction, general-purpose helper, configuration knob, dependency, compatibility layer, feature flag, or parallel code path only when a concrete current requirement or supported contract needs it. +- Trace only the code paths being changed and the contracts they rely on. Expand the investigation or implementation only when concrete evidence or validation exposes another required path. +- Keep root-cause fixes within the requested boundary. Leave unrelated refactors, cleanup, feature work, and pre-existing failures out of the patch; report them separately when they materially affect the result. +- Add focused tests for the required behavior and realistic regression paths. Do not generalize production code or test infrastructure for hypothetical future cases without evidence. ## Compatibility boundary rules @@ -33,11 +42,18 @@ Use this skill before editing code when the task changes runtime behavior or any ## Default implementation stance -- Prefer deletion or replacement over aliases, overloads, shims, feature flags, and dual-write logic when the old shape is unreleased. -- Do not preserve a confusing abstraction just because it exists in the current branch diff. +- Prefer deletion or direct replacement over aliases, overloads, shims, feature flags, and dual-write logic when the old shape is unreleased. - If review feedback claims a change is breaking, verify it against the latest release tag and actual external impact before accepting the feedback. - If a change truly crosses the latest released contract boundary, call that out explicitly in the ExecPlan, release notes context, and user-facing summary. +## Applying this skill during review + +- Establish the requested outcome and compatibility boundary before judging whether the implementation is too narrow or too broad. +- Treat complexity as an actionable finding only when specific added machinery is not needed by the current task, a released contract, supported durable state, or a verified runtime or platform risk. Name that machinery and recommend the smallest safe removal or replacement. +- Do not request abstractions, configuration, dependencies, compatibility work, or extensibility for hypothetical future consumers. +- Keep unrelated cleanup and pre-existing problems out of blocking findings. Report them separately only when they are useful to the maintainer. +- Require a broader refactor only when concrete evidence shows that the focused change would otherwise be incorrect, unsafe, incompatible, or materially harder to maintain. + ## SDK-specific decision rules - When unsupported OpenAI API or provider-adapter behavior already has a released default path, avoid turning it into a default hard error unless the latest release boundary justifies that break. Prefer an opt-in strict mode such as `strict_feature_validation=True`, while keeping the default path compatible through warning, ignoring unsupported data, or a clearly non-empty placeholder. @@ -53,6 +69,7 @@ Use this skill before editing code when the task changes runtime behavior or any - The change would alter behavior shipped in the latest release tag. - The change would modify durable external data, protocol formats, or serialized state. +- The correct solution would materially expand beyond the requested outcome or require unrelated architectural work. - The user explicitly asked for backward compatibility, deprecation, or migration support. ## Output expectations @@ -61,3 +78,5 @@ When this skill materially affects the implementation approach, state the decisi - `Compatibility boundary: latest release tag v0.x.y; branch-local interface rewrite, no shim needed.` - `Compatibility boundary: released RunState schema; preserve compatibility and add migration coverage.` +- `Scope decision: direct change using existing patterns; no new abstraction or adjacent cleanup needed.` +- `Review decision: the added compatibility path has no released or supported consumer; replace it with the direct implementation.` diff --git a/AGENTS.md b/AGENTS.md index 07248efa9b..5da3d17f8b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,6 +9,7 @@ This guide helps new contributors get started with the OpenAI Agents Python repo 1. [Policies & Mandatory Rules](#policies--mandatory-rules) 2. [Project Structure Guide](#project-structure-guide) 3. [Operation Guide](#operation-guide) +4. [Code Review Rules](#code-review-rules) ## Policies & Mandatory Rules @@ -32,7 +33,7 @@ When working on OpenAI API or OpenAI platform integrations in this repo (Respons #### `$implementation-strategy` -Before changing runtime code, exported APIs, external configuration, persisted schemas, wire protocols, or other user-facing behavior, use `$implementation-strategy` to decide the compatibility boundary and implementation shape. Judge breaking changes against the latest release tag, not unreleased branch-local churn. Interfaces introduced or changed after the latest release tag may be rewritten without compatibility shims unless they define a released or explicitly supported durable external state boundary, or the user explicitly asks for a migration path. Unreleased persisted formats on `main` may be renumbered or squashed before release when intermediate snapshots are intentionally unsupported. +Before changing or reviewing runtime code, exported APIs, external configuration, persisted schemas, wire protocols, or other user-facing behavior, use `$implementation-strategy` to decide the compatibility boundary and implementation shape. During review, use it before requesting compatibility layers, migrations, new abstractions, or broader refactors. Judge breaking changes against the latest release tag, not unreleased branch-local churn. Interfaces introduced or changed after the latest release tag may be rewritten without compatibility shims unless they define a released or explicitly supported durable external state boundary, or the user explicitly asks for a migration path. Unreleased persisted formats on `main` may be renumbered or squashed before release when intermediate snapshots are intentionally unsupported. #### `$pr-draft-summary` @@ -234,7 +235,15 @@ make tests - Run `make format`, `make lint`, `make typecheck`, and `make tests` before marking work ready. - Commit messages should be concise and written in the imperative mood. Small, focused commits are preferred. -### Review Process & What Reviewers Look For +## Code Review Rules + +- Use `$implementation-strategy` to establish the requested outcome and latest released compatibility boundary before judging implementation scope or architecture. +- Treat added complexity as an actionable finding only when specific machinery is not required by the task, a released contract, supported durable state, or a verified runtime or platform risk. Identify the unnecessary machinery and recommend the smallest safe removal or direct replacement. +- Do not request speculative abstractions, general-purpose helpers, configuration knobs, dependencies, compatibility layers, feature flags, parallel code paths, or extensibility for hypothetical future consumers. +- Keep findings scoped to the patch. Do not block on unrelated cleanup, pre-existing bugs, or optional refactors; report them separately when useful. +- Require a broader refactor only when concrete evidence shows the focused change would otherwise be incorrect, unsafe, incompatible, or materially harder to maintain. + +### Baseline review expectations - ✅ Checks pass (`make format`, `make lint`, `make typecheck`, `make tests`). - ✅ Tests cover new behavior and edge cases. From 14ad7e1c62446cc809144e9d8b30b4b047aa9fdb Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 09:47:04 +0900 Subject: [PATCH 05/10] fix: redact Realtime and RunState diagnostics (#3948) --- src/agents/logger.py | 22 +++ src/agents/realtime/openai_realtime.py | 35 ++--- src/agents/run_state.py | 50 +++++-- tests/realtime/test_openai_realtime.py | 137 ++++++++++++++---- tests/test_error_logging_redaction.py | 190 +++++++++++++++++++++++++ 5 files changed, 374 insertions(+), 60 deletions(-) diff --git a/src/agents/logger.py b/src/agents/logger.py index 18b8670680..d534cec198 100644 --- a/src/agents/logger.py +++ b/src/agents/logger.py @@ -7,6 +7,7 @@ logger = logging.getLogger("openai.agents") _DiagnosticExtra = Callable[[], Mapping[str, object]] +_DiagnosticArgs = Callable[[], tuple[object, ...]] _DIAGNOSTIC_CONTEXT_FIELD = "openai_agents_diagnostic_context" @@ -242,3 +243,24 @@ def log_model_and_tool_action_warning( stacklevel=stacklevel, diagnostic_extra=diagnostic_extra, ) + + +def log_model_and_tool_data_warning( + target_logger: logging.Logger, + redacted_message: str, + *, + diagnostic_message: str, + diagnostic_args: _DiagnosticArgs | None = None, + stacklevel: int = 2, +) -> None: + """Log mixed model/tool data only when both data policies allow it.""" + if _debug.DONT_LOG_MODEL_DATA or _debug.DONT_LOG_TOOL_DATA: + target_logger.warning(redacted_message, stacklevel=stacklevel) + return + + try: + args = diagnostic_args() if diagnostic_args is not None else () + except Exception: + target_logger.warning(redacted_message, stacklevel=stacklevel) + return + target_logger.warning(diagnostic_message, *args, stacklevel=stacklevel) diff --git a/src/agents/realtime/openai_realtime.py b/src/agents/realtime/openai_realtime.py index f36aafffa4..141606246a 100644 --- a/src/agents/realtime/openai_realtime.py +++ b/src/agents/realtime/openai_realtime.py @@ -195,37 +195,19 @@ async def get_api_key(key: str | Callable[[], MaybeAwaitable[str]] | None) -> st ServerEventTypeAdapter: TypeAdapter[AllRealtimeServerEvents] | None = None -def _server_event_validation_summary(error: BaseException) -> str: - if isinstance(error, pydantic.ValidationError): - return f"{error.error_count()} validation error(s)" - - if not _debug.DONT_LOG_MODEL_DATA: - return type(error).__name__ - return "validation failed" - - -def _server_event_identity(event: Any) -> tuple[Any, Any]: +def _server_event_type(event: Any) -> Any: if not isinstance(event, dict): - return "unknown", None + return "unknown" - return event.get("type", "unknown"), event.get("event_id") + return event.get("type", "unknown") -def _log_server_event_validation_failure(event: Any, error: BaseException) -> str: - event_type, event_id = _server_event_identity(event) - +def _log_server_event_validation_failure(event: Any) -> None: if _debug.DONT_LOG_MODEL_DATA: - logger.error( - "Failed to validate server event type=%s event_id=%s: %s", - event_type, - event_id, - _server_event_validation_summary(error), - ) + logger.error("Failed to validate server event") else: logger.error("Failed to validate server event: %s", event, exc_info=True) - return str(event_type) - @dataclass(frozen=True) class _PendingResponseCreate: @@ -723,7 +705,7 @@ async def send_event(self, event: RealtimeModelSendEvent) -> None: else: await self._send_raw_message(converted) elif _debug.DONT_LOG_MODEL_DATA: - logger.error("Failed to convert raw message type=%s", event.message.get("type")) + logger.error("Failed to convert raw message") else: logger.error("Failed to convert raw message: %s", event) elif isinstance(event, RealtimeModelSendUserInput): @@ -1140,11 +1122,12 @@ async def _handle_ws_event(self, event: dict[str, Any]): validation_event ) except pydantic.ValidationError as e: - _log_server_event_validation_failure(event, e) + _log_server_event_validation_failure(event) await self._emit_event(RealtimeModelErrorEvent(error=e)) return except Exception as e: - event_type = _log_server_event_validation_failure(event, e) + _log_server_event_validation_failure(event) + event_type = str(_server_event_type(event)) exception_event = RealtimeModelExceptionEvent( exception=e, context=f"Failed to validate server event: {event_type}", diff --git a/src/agents/run_state.py b/src/agents/run_state.py index ef9cdc6c76..b5bc887297 100644 --- a/src/agents/run_state.py +++ b/src/agents/run_state.py @@ -81,7 +81,11 @@ coerce_tool_search_call_raw_item, coerce_tool_search_output_raw_item, ) -from .logger import log_model_and_tool_action_warning, logger +from .logger import ( + log_model_and_tool_action_warning, + log_model_and_tool_data_warning, + logger, +) from .run_context import RunContextWrapper from .run_internal.items import ( NestedHistoryOwnedItemRef, @@ -3556,6 +3560,18 @@ def _deserialize_items( result: list[RunItem] = [] + def _capture_diagnostic_args(*values: object) -> Callable[[], tuple[object, ...]]: + def diagnostic_args() -> tuple[object, ...]: + return values + + return diagnostic_args + + def _capture_diagnostic_extra(**values: object) -> Callable[[], Mapping[str, object]]: + def diagnostic_extra() -> Mapping[str, object]: + return values + + return diagnostic_extra + def _resolve_agent_info( item_data: Mapping[str, Any], item_type: str ) -> tuple[Agent[Any] | None, str | None]: @@ -3591,9 +3607,19 @@ def _resolve_agent_info( agent, agent_name = _resolve_agent_info(item_data, item_type) if not agent: if agent_name: - logger.warning("Agent %s not found, skipping item", agent_name) + log_model_and_tool_data_warning( + logger, + "Agent not found, skipping item", + diagnostic_message="Agent %s not found, skipping item", + diagnostic_args=_capture_diagnostic_args(agent_name), + ) else: - logger.warning("Item missing agent field, skipping: %s", item_type) + log_model_and_tool_data_warning( + logger, + "Item missing agent field, skipping", + diagnostic_message="Item missing agent field, skipping: %s", + diagnostic_args=_capture_diagnostic_args(item_type), + ) continue raw_item_data = item_data["raw_item"] @@ -3682,11 +3708,14 @@ def _resolve_agent_info( if not source_agent or not target_agent: source_name = item_data.get("source_agent") target_name = item_data.get("target_agent") - logger.warning( - "Skipping handoff_output_item: could not resolve agents " - "(source=%s, target=%s).", - source_name, - target_name, + log_model_and_tool_data_warning( + logger, + "Skipping handoff output item: could not resolve agents", + diagnostic_message=( + "Skipping handoff_output_item: could not resolve agents " + "(source=%s, target=%s)." + ), + diagnostic_args=_capture_diagnostic_args(source_name, target_name), ) continue @@ -3753,7 +3782,10 @@ def _resolve_agent_info( raise except Exception as e: log_model_and_tool_action_warning( - logger, f"Failed to deserialize item of type {item_type}", e + logger, + "Failed to deserialize item", + e, + diagnostic_extra=_capture_diagnostic_extra(item_type=item_type), ) continue diff --git a/tests/realtime/test_openai_realtime.py b/tests/realtime/test_openai_realtime.py index aeeb58081b..9e8618d8b9 100644 --- a/tests/realtime/test_openai_realtime.py +++ b/tests/realtime/test_openai_realtime.py @@ -1,5 +1,6 @@ import asyncio import json +import logging import time from types import SimpleNamespace from typing import Any, cast @@ -448,59 +449,145 @@ async def test_handle_invalid_event_schema_logs_error(self, model): assert error_event.type == "error" @pytest.mark.asyncio - async def test_handle_invalid_event_schema_redacts_payload_from_logs(self, model, monkeypatch): - """Test that invalid event logs omit payload data when model data logging is disabled.""" + async def test_handle_invalid_event_schema_redacts_event_from_logs( + self, model, monkeypatch, caplog + ): + """Invalid event logs omit all event data when model data logging is disabled.""" mock_listener = AsyncMock() model.add_listener(mock_listener) monkeypatch.setattr( "agents.realtime.openai_realtime._debug.DONT_LOG_MODEL_DATA", True, ) + caplog.set_level(logging.ERROR, logger="openai.agents") invalid_event = { - "type": "response.output_audio.delta", - "event_id": "evt_123", - "delta": "secret transcript", + "type": "SECRET_EVENT_TYPE", + "event_id": "SECRET_EVENT_ID", + "delta": "SECRET_EVENT_PAYLOAD", } - with patch("agents.realtime.openai_realtime.logger") as mock_logger: - await model._handle_ws_event(invalid_event) + await model._handle_ws_event(invalid_event) - mock_logger.error.assert_called_once() - logged_call = str(mock_logger.error.call_args) - assert "secret transcript" not in logged_call - assert "response.output_audio.delta" in logged_call - assert "evt_123" in logged_call - assert mock_logger.error.call_args.kwargs.get("exc_info") is not True + records = [ + record for record in caplog.records if record.msg == "Failed to validate server event" + ] + assert len(records) == 1 + record = records[0] + assert record.args == () + assert record.exc_info is None + assert record.exc_text is None + assert invalid_event not in record.__dict__.values() + rendered = logging.Formatter().format(record) + assert rendered == "Failed to validate server event" + assert "SECRET_EVENT_TYPE" not in rendered + assert "SECRET_EVENT_ID" not in rendered + assert "SECRET_EVENT_PAYLOAD" not in rendered assert mock_listener.on_event.call_count == 2 error_event = mock_listener.on_event.call_args_list[1][0][0] assert error_event.type == "error" @pytest.mark.asyncio - async def test_send_raw_message_conversion_failure_redacts_payload_from_logs( - self, model, monkeypatch + async def test_handle_invalid_event_schema_preserves_diagnostics_when_enabled( + self, model, monkeypatch, caplog + ): + """Invalid event logs retain event data when model data logging is enabled.""" + mock_listener = AsyncMock() + model.add_listener(mock_listener) + monkeypatch.setattr( + "agents.realtime.openai_realtime._debug.DONT_LOG_MODEL_DATA", + False, + ) + caplog.set_level(logging.ERROR, logger="openai.agents") + + invalid_event = { + "type": "diagnostic.event", + "event_id": "diagnostic_event_id", + "delta": "diagnostic payload", + } + + await model._handle_ws_event(invalid_event) + + records = [ + record + for record in caplog.records + if record.msg == "Failed to validate server event: %s" + ] + assert len(records) == 1 + record = records[0] + assert record.args == invalid_event + assert record.exc_info is not None + rendered = logging.Formatter().format(record) + assert "diagnostic.event" in rendered + assert "diagnostic_event_id" in rendered + assert "diagnostic payload" in rendered + + assert mock_listener.on_event.call_count == 2 + error_event = mock_listener.on_event.call_args_list[1][0][0] + assert error_event.type == "error" + + @pytest.mark.asyncio + async def test_send_raw_message_conversion_failure_redacts_event_from_logs( + self, model, monkeypatch, caplog ): - """A raw client message that fails to convert must not leak its payload to the logs - when model-data logging is disabled.""" + """A raw client message that fails to convert must not leak event data to logs.""" monkeypatch.setattr( "agents.realtime.openai_realtime._debug.DONT_LOG_MODEL_DATA", True, ) + caplog.set_level(logging.ERROR, logger="openai.agents") raw = RealtimeModelSendRawMessage( message={ - "type": "invalid.event.type", - "other_data": {"transcript": "secret transcript"}, + "type": "SECRET_RAW_EVENT_TYPE", + "other_data": {"transcript": "SECRET_RAW_EVENT_PAYLOAD"}, } ) - with patch("agents.realtime.openai_realtime.logger") as mock_logger: - await model.send_event(raw) + await model.send_event(raw) - mock_logger.error.assert_called_once() - logged_call = str(mock_logger.error.call_args) - assert "secret transcript" not in logged_call - assert "invalid.event.type" in logged_call + records = [ + record for record in caplog.records if record.msg == "Failed to convert raw message" + ] + assert len(records) == 1 + record = records[0] + assert record.args == () + assert record.exc_info is None + assert record.exc_text is None + assert raw not in record.__dict__.values() + rendered = logging.Formatter().format(record) + assert rendered == "Failed to convert raw message" + assert "SECRET_RAW_EVENT_TYPE" not in rendered + assert "SECRET_RAW_EVENT_PAYLOAD" not in rendered + + @pytest.mark.asyncio + async def test_send_raw_message_conversion_failure_preserves_diagnostics_when_enabled( + self, model, monkeypatch, caplog + ): + """A raw conversion failure retains event data when model data logging is enabled.""" + monkeypatch.setattr( + "agents.realtime.openai_realtime._debug.DONT_LOG_MODEL_DATA", + False, + ) + caplog.set_level(logging.ERROR, logger="openai.agents") + raw = RealtimeModelSendRawMessage( + message={ + "type": "diagnostic.raw.event", + "other_data": {"transcript": "diagnostic transcript"}, + } + ) + + await model.send_event(raw) + + records = [ + record for record in caplog.records if record.msg == "Failed to convert raw message: %s" + ] + assert len(records) == 1 + record = records[0] + assert record.args == (raw,) + rendered = logging.Formatter().format(record) + assert "diagnostic.raw.event" in rendered + assert "diagnostic transcript" in rendered @pytest.mark.asyncio async def test_custom_voice_response_events_update_response_sequencer(self, model, monkeypatch): diff --git a/tests/test_error_logging_redaction.py b/tests/test_error_logging_redaction.py index 4f08056915..85ebd221b3 100644 --- a/tests/test_error_logging_redaction.py +++ b/tests/test_error_logging_redaction.py @@ -23,6 +23,7 @@ import agents._debug as _debug from agents import ( + Agent, ModelSettings, ModelTracing, OpenAIResponsesModel, @@ -37,6 +38,7 @@ log_model_and_tool_action_debug, log_model_and_tool_action_error, log_model_and_tool_action_warning, + log_model_and_tool_data_warning, log_tool_action_debug, log_tool_action_error as log_shared_tool_action_error, log_tool_action_warning, @@ -45,6 +47,7 @@ log_tool_action_error, resolve_approval_rejection_message, ) +from agents.run_state import _deserialize_items from agents.tracing.processor_interface import TracingProcessor from agents.tracing.provider import SynchronousMultiTracingProcessor from agents.tracing.spans import Span @@ -88,6 +91,14 @@ def __bool__(self) -> bool: return False +class _HostileValue: + def __str__(self) -> str: + raise AssertionError("redacted logging inspected __str__") + + def __repr__(self) -> str: + raise AssertionError("redacted logging inspected __repr__") + + class _FailingTracingProcessor(TracingProcessor): def __init__(self) -> None: self.str_calls = 0 @@ -127,6 +138,15 @@ def _emit_tool_execution_error_for_location() -> None: log_tool_action_error("Fixed operational message", ValueError("failure")) +def _emit_data_warning_for_location(test_logger) -> None: + log_model_and_tool_data_warning( + test_logger, + "Fixed operational warning", + diagnostic_message="Warning for %s", + diagnostic_args=lambda: ("diagnostic-value",), + ) + + def _responses_model() -> OpenAIResponsesModel: return OpenAIResponsesModel( model="test-model", @@ -382,6 +402,176 @@ def diagnostic_extra() -> dict[str, object]: assert "original failure" in logging.Formatter().format(record) +def test_shared_data_warning_does_not_inspect_or_attach_redacted_arguments(monkeypatch) -> None: + monkeypatch.setattr(_debug, "DONT_LOG_MODEL_DATA", True) + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + test_logger = logging.Logger("sensitive-logging-data-warning") + handler = _RecordingHandler() + test_logger.addHandler(handler) + hostile = _HostileValue() + + log_model_and_tool_data_warning( + test_logger, + "Fixed operational warning", + diagnostic_message="Warning for %s", + diagnostic_args=lambda: (hostile,), + ) + + assert len(handler.records) == 1 + record = handler.records[0] + assert record.msg == "Fixed operational warning" + assert record.args == () + assert record.exc_info is None + assert record.exc_text is None + assert hostile not in record.__dict__.values() + assert logging.Formatter().format(record) == "Fixed operational warning" + + +def test_shared_data_warning_preserves_diagnostics_when_enabled(monkeypatch) -> None: + monkeypatch.setattr(_debug, "DONT_LOG_MODEL_DATA", False) + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + test_logger = logging.Logger("sensitive-logging-data-warning") + handler = _RecordingHandler() + test_logger.addHandler(handler) + diagnostic_value = "diagnostic-agent" + + log_model_and_tool_data_warning( + test_logger, + "Fixed operational warning", + diagnostic_message="Warning for %s", + diagnostic_args=lambda: (diagnostic_value,), + ) + + assert len(handler.records) == 1 + record = handler.records[0] + assert record.msg == "Warning for %s" + assert record.args == (diagnostic_value,) + assert logging.Formatter().format(record) == "Warning for diagnostic-agent" + + +def test_shared_data_warning_falls_back_when_diagnostic_arguments_fail(monkeypatch) -> None: + monkeypatch.setattr(_debug, "DONT_LOG_MODEL_DATA", False) + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", False) + test_logger = logging.Logger("sensitive-logging-data-warning") + handler = _RecordingHandler() + test_logger.addHandler(handler) + + def diagnostic_args() -> tuple[object, ...]: + raise RuntimeError("SECRET_DIAGNOSTIC_ARGUMENT_FAILURE") + + log_model_and_tool_data_warning( + test_logger, + "Fixed operational warning", + diagnostic_message="Warning for %s", + diagnostic_args=diagnostic_args, + ) + + assert len(handler.records) == 1 + record = handler.records[0] + assert record.msg == "Fixed operational warning" + assert record.args == () + assert record.exc_info is None + assert "SECRET_DIAGNOSTIC_ARGUMENT_FAILURE" not in logging.Formatter().format(record) + + +def test_shared_data_warning_preserves_direct_caller_location(monkeypatch) -> None: + monkeypatch.setattr(_debug, "DONT_LOG_MODEL_DATA", True) + test_logger = logging.Logger("sensitive-logging-data-warning-location") + handler = _RecordingHandler() + test_logger.addHandler(handler) + + _emit_data_warning_for_location(test_logger) + + record = handler.records[0] + assert Path(record.pathname).resolve() == Path(__file__).resolve() + assert record.funcName == "_emit_data_warning_for_location" + + +@pytest.mark.parametrize( + ("model_redacted", "tool_redacted"), + [ + (True, False), + (False, True), + (True, True), + (False, False), + ], +) +@pytest.mark.parametrize( + ("scenario", "secrets", "redacted_message"), + [ + ( + "missing_agent", + ("SECRET_AGENT_NAME",), + "Agent not found, skipping item", + ), + ( + "missing_agent_field", + ("SECRET_ITEM_TYPE",), + "Item missing agent field, skipping", + ), + ( + "missing_handoff_agents", + ("SECRET_SOURCE_AGENT", "SECRET_TARGET_AGENT"), + "Skipping handoff output item: could not resolve agents", + ), + ], +) +def test_run_state_deserialization_warnings_follow_both_data_policies( + monkeypatch, + model_redacted: bool, + tool_redacted: bool, + scenario: str, + secrets: tuple[str, ...], + redacted_message: str, +) -> None: + monkeypatch.setattr(_debug, "DONT_LOG_MODEL_DATA", model_redacted) + monkeypatch.setattr(_debug, "DONT_LOG_TOOL_DATA", tool_redacted) + known_agent = Agent(name="KnownAgent") + if scenario == "missing_agent": + item_data = { + "type": "message_output_item", + "agent": secrets[0], + "raw_item": {}, + } + elif scenario == "missing_agent_field": + item_data = { + "type": secrets[0], + "raw_item": {}, + } + else: + item_data = { + "type": "handoff_output_item", + "agent": "KnownAgent", + "source_agent": secrets[0], + "target_agent": secrets[1], + "raw_item": {}, + } + + test_logger = logging.Logger("sensitive-logging-run-state") + handler = _RecordingHandler() + test_logger.addHandler(handler) + with patch("agents.run_state.logger", test_logger): + result = _deserialize_items([item_data], {"KnownAgent": known_agent}) + + assert result == [] + assert len(handler.records) == 1 + record = handler.records[0] + rendered = logging.Formatter().format(record) + redacted = model_redacted or tool_redacted + if redacted: + assert record.msg == redacted_message + assert record.args == () + assert record.exc_info is None + assert record.exc_text is None + assert rendered == redacted_message + for secret in secrets: + assert secret not in rendered + assert secret not in record.__dict__.values() + else: + for secret in secrets: + assert secret in rendered + + @pytest.mark.parametrize( "operation", [ From d9d623098e879baf904e869416486f7cc2b93ac8 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 10:20:51 +0900 Subject: [PATCH 06/10] fix: reject unsupported streamed STT audio dtypes (#3950) --- src/agents/voice/models/openai_stt.py | 4 +++- tests/voice/test_openai_stt.py | 14 +++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/agents/voice/models/openai_stt.py b/src/agents/voice/models/openai_stt.py index 89ee27dc10..d3be57ca52 100644 --- a/src/agents/voice/models/openai_stt.py +++ b/src/agents/voice/models/openai_stt.py @@ -11,7 +11,7 @@ from openai import AsyncOpenAI from ... import _debug -from ...exceptions import AgentsException +from ...exceptions import AgentsException, UserError from ...logger import logger from ...tracing import Span, SpanError, TranscriptionSpanData, transcription_span from ...util._error_tracing import get_trace_error @@ -49,6 +49,8 @@ def _audio_buffer_to_base64(buffer: npt.NDArray[np.int16 | np.float32]) -> str: # Convert to int16. buffer = np.clip(buffer, -1.0, 1.0) buffer = (buffer * 32767).astype(np.int16) + elif buffer.dtype != np.int16: + raise UserError("Buffer must be a numpy array of int16 or float32") return base64.b64encode(buffer.tobytes()).decode("utf-8") diff --git a/tests/voice/test_openai_stt.py b/tests/voice/test_openai_stt.py index e13532a769..090afa5806 100644 --- a/tests/voice/test_openai_stt.py +++ b/tests/voice/test_openai_stt.py @@ -11,6 +11,7 @@ import pytest from agents import trace +from agents.exceptions import UserError from tests.testing_processor import fetch_span_errors try: @@ -22,7 +23,10 @@ STTModelSettings, ) from agents.voice.exceptions import STTWebsocketConnectionError - from agents.voice.models.openai_stt import EVENT_INACTIVITY_TIMEOUT + from agents.voice.models.openai_stt import ( + EVENT_INACTIVITY_TIMEOUT, + _audio_buffer_to_base64, + ) from .fake_models import FakeStreamedAudioInput except ImportError: @@ -221,6 +225,14 @@ async def test_stream_audio_sends_pcm16( await session.close() +@pytest.mark.parametrize("dtype", [np.int32, np.float64], ids=["int32", "float64"]) +def test_stream_audio_rejects_unsupported_dtype(dtype: npt.DTypeLike) -> None: + buffer = np.array([1, 2], dtype=dtype) + + with pytest.raises(UserError, match="Buffer must be a numpy array of int16 or float32"): + _audio_buffer_to_base64(buffer) + + @pytest.mark.asyncio @pytest.mark.parametrize( "created,updated,completed", From f45d6e2610105934ecb00dc38ee721a67258fa82 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 12:12:15 +0900 Subject: [PATCH 07/10] chore: update final-release-review skill details --- .agents/skills/final-release-review/SKILL.md | 37 +++++++++---- .../references/review-checklist.md | 53 +++++++++++++++++-- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/.agents/skills/final-release-review/SKILL.md b/.agents/skills/final-release-review/SKILL.md index d2c546f971..5836308bc0 100644 --- a/.agents/skills/final-release-review/SKILL.md +++ b/.agents/skills/final-release-review/SKILL.md @@ -26,8 +26,9 @@ The review must be stable and actionable: avoid variance between runs by using e git log --oneline --reverse "${BASE_TAG}".."${TARGET}" git diff --name-status "${BASE_TAG}"..."${TARGET}" ``` -5. Deep review using `references/review-checklist.md` to spot breaking changes, regressions, and improvement chances. -6. Capture findings and call the release gate: ship/block with conditions; propose focused tests for risky areas. +5. Use the broad signals in `references/review-checklist.md` to find breaking-change, regression, and release-polish candidates. +6. Prove or dismiss each candidate with a BASE-versus-TARGET contract comparison and the owning SDK invariant from `.agents/references/README.md`. +7. Report only actionable findings and call the release gate: ship/block with concrete conditions. ## Deterministic gate policy @@ -42,7 +43,7 @@ The review must be stable and actionable: avoid variance between runs by using e - Large diff size, broad refactor, or many touched files. - "Could regress" risk statements without concrete evidence. - Not running tests locally. -- If evidence is incomplete, issue **🟢 GREEN LIGHT TO SHIP** with targeted validation follow-ups instead of `BLOCKED`. +- If evidence is incomplete, do not block. Report a validation action only when the diff establishes a concrete unresolved risk; otherwise omit the candidate. ## Workflow @@ -52,28 +53,42 @@ The review must be stable and actionable: avoid variance between runs by using e - Keep the working tree clean to avoid diff noise. - **Assumptions** - Assume the target commit (default `origin/main` tip) has already passed `$code-change-verification` in CI unless the user says otherwise. - - Do not block a release solely because you did not run tests locally; focus on concrete behavioral or API risks. + - Treat repository unit tests, lint, formatting, type checking, and coverage as CI evidence, not as the release audit. Do not rerun them by default. + - Do not block a release solely because you did not rerun CI checks locally; focus on concrete behavioral, compatibility, packaging, or API risks. - Release policy: routine releases use patch versions; use minor only for breaking changes or major feature additions. Major versions are reserved until the 1.0 release. - **Map the diff** - Use `--stat`, `--dirstat`, and `--name-status` outputs to spot hot directories and file types. - For suspicious files, prefer `git diff --word-diff BASE...TARGET -- `. - Note any deleted or newly added tests, config, migrations, or scripts. -- **Analyze risk** - - Walk through the categories in `references/review-checklist.md` (breaking changes, regression clues, improvement opportunities). - - When you suspect a risk, cite the specific file/commit and explain the behavioral impact. +- **Discover candidates** + - Walk through all categories in `references/review-checklist.md` (breaking changes, regression clues, improvement opportunities). Keep this broad scan so refactors, error handling, concurrency, dependencies, docs drift, and missing coverage remain visible. + - Read changed tests to understand the intended behavior, exercised branches, and missing invariants. A changed or missing test is a clue, not a finding by itself. +- **Audit contract deltas** + - Compare BASE and TARGET rather than reviewing TARGET in isolation. + - For public APIs, compare exports, import identity, signatures, constructor and dataclass field order, defaults, enums, and documented behavior. + - For package metadata, compare supported Python versions, dependencies, optional extras, distribution contents, and import behavior from the built artifacts. + - For persisted state, schemas, protocols, config, and environment variables, identify the released durable boundary and verify backward-read or migration behavior where required. + - Route each changed runtime area through the owning reference in `.agents/references/README.md`. Trace the affected value, state, item, or side effect across all required downstream surfaces instead of stopping at the edited function. + - Check only the relevant symmetry and failure axes: streaming/non-streaming, sync/async, fresh/resumed, client/server-managed state, success/error/cancellation, sequential/concurrent, and normal/repeated cleanup. +- **Prove findings** + - Promote a candidate to a finding only when the diff shows a concrete contract violation, a reachable supported-path regression, or a release-polish gap with user impact. + - If static evidence cannot resolve a concrete semantic question, use the smallest public-path or installed-artifact probe that can. Prefer the same scenario against BASE and TARGET so environment failures and pre-existing behavior are separated from regressions. + - Do not run repository unit-test slices merely to accumulate passing evidence. Run a focused test only when reproducing a specific failure or when no more direct contract, artifact, or runtime probe is available. + - When you confirm a risk, cite the specific file/commit and explain the behavioral impact. - For every finding, include all of: `Evidence`, `Impact`, and `Action`. - Severity calibration: - **🟢 LOW**: low blast radius or clearly covered behavior; no release gate impact. - **🟡 MODERATE**: plausible user-facing regression signal; needs validation but not a confirmed blocker. - **🔴 HIGH**: confirmed or strongly evidenced release-blocking issue. - - Suggest minimal, high-signal validation commands (targeted tests or linters) instead of generic reruns when time is tight. + - Attach a validation action only to a concrete unresolved risk. Give the smallest command or task and a pass condition; do not add generic follow-up checks. - Breaking changes do not automatically require a BLOCKED release call when they are already covered by an appropriate version bump and migration/upgrade notes; only block when the bump is missing/mismatched (e.g., patch bump) or when the breaking change introduces unresolved risk. - **Form a recommendation** - State BASE_TAG and TARGET explicitly. - Provide a concise diff summary (key directories/files and counts). - - List: breaking-change candidates, probable regressions/bugs, improvement opportunities, missing release notes/migrations. + - List only substantiated breaking changes, regressions/bugs, improvement opportunities, and missing release notes/migrations. Do not turn every audit clue into a report item. - Recommend ship/block and the exact checks needed to unblock if blocking. If a breaking change is properly versioned (minor/major), you may still recommend a GREEN LIGHT TO SHIP while calling out the change. Use emoji and boldface in the release call to make the gate obvious. - If you cannot provide a concrete unblock checklist item, do not use `BLOCKED`. + - Do not include routine command results, pass counts, skips, deselections, or a validation-status inventory. Mention a validation limitation only when it materially changes a specific finding or the release call. ## Output format (required) @@ -114,10 +129,10 @@ https://github.com/openai/openai-agents-python/compare/... 2. ... ### Notes: -- +- ``` -If no risks are found, include a "No material risks identified" line under Risk assessment and still provide a ship call. If you did not run local verification, do not add a verification status section or use it as a release blocker; note any assumptions briefly in Notes. If the report is not blocked, omit the `Unblock checklist` section. +If no risks are found, include a "No material risks identified" line under Risk assessment and still provide a ship call. Do not add a verification-status section or report routine check results. If the report is not blocked, omit the `Unblock checklist` section. ### Resources diff --git a/.agents/skills/final-release-review/references/review-checklist.md b/.agents/skills/final-release-review/references/review-checklist.md index 3cd5d4d2a6..76b2e2bbf0 100644 --- a/.agents/skills/final-release-review/references/review-checklist.md +++ b/.agents/skills/final-release-review/references/review-checklist.md @@ -20,7 +20,7 @@ - Large refactor or high file count. - Speculative risk without evidence. - Not running tests locally. -- If uncertain, keep gate green and provide focused follow-up checks. +- If uncertain, keep the gate green. Add a focused follow-up only when it resolves a concrete risk already identified in the diff. ## Actionability contract @@ -28,8 +28,52 @@ - `Evidence`: specific file/commit/diff/test signal. - `Impact`: one-sentence user or runtime effect. - `Action`: concrete command/task with pass criteria. +- A candidate becomes a finding only when it has a concrete contract violation, a reachable supported path, or a release-polish gap with user impact. +- Changed tests, missing tests, diff size, and risky patterns are discovery signals; they are not findings without contract or runtime evidence. - A `BLOCKED` report must contain an `Unblock checklist` with at least one executable item. -- If no executable unblock item exists, do not block; downgrade to green with follow-up checks. +- If no executable unblock item exists, do not block. Keep the gate green and include an action only for a concrete unresolved risk. + +## Two-stage audit + +### Stage 1: broad discovery + +Use all of the existing breaking-change, regression, dependency, documentation, and improvement signals below. The goal is high recall: collect plausible candidates without prematurely reporting them. + +Read changed tests as behavioral documentation. Identify the intended outcome, covered branches, deleted assertions, new skips, and missing failure paths, but do not rerun repository unit tests merely to accumulate passing evidence. + +### Stage 2: contract and invariant proof + +For each candidate: + +1. Compare the released BASE behavior or contract with TARGET. Do not infer compatibility from TARGET alone. +2. Identify the owning SDK boundary using `.agents/references/README.md`. +3. Trace the changed value, state, item, identity, or side effect across every downstream consumer required by that boundary. +4. Check the relevant paired paths and failure modes. +5. Promote the candidate to a finding only when this trace establishes concrete impact. + +Use these contract comparisons when relevant: + +| Changed surface | BASE-versus-TARGET audit | +|---|---| +| Public API | Exports, import identity, signatures, positional parameter order, dataclass field order, defaults, enums, and documented behavior | +| Runner and run items | Provider output, result items, semantic stream events, session history, replay, handoffs, and `RunState` | +| Tool execution | Planning, approvals, guardrails, invocation, hooks, output conversion, persistence, cancellation, and cleanup | +| Conversation and sessions | First turn, follow-up, retry, filtering, handoff, compaction, interruption, and resume | +| Model and provider adapters | Model/settings resolution, request conversion, streaming terminals, provider data, errors, retries, and transport ownership | +| Persisted schemas and config | Serialized shape, version support, backward reads, migrations, defaults, environment variables, and wire compatibility | +| Package boundary | Supported Python versions, dependencies, extras, distribution contents, public imports, and built wheel/sdist behavior | + +Select only the axes implicated by the diff: + +- streaming versus non-streaming; +- sync versus async; +- fresh execution versus serialized resume; +- client-managed versus server-managed state; +- success, exception, and cancellation; +- sequential versus concurrent execution; +- normal, partial-failure, and repeated cleanup. + +If static inspection cannot resolve a concrete semantic question, run the smallest public-path or installed-artifact probe that can. Prefer an identical BASE and TARGET scenario. A focused unit test is a fallback for reproducing a specific failure, not the default release validation. ## Breaking change signals @@ -59,7 +103,8 @@ - BASE tag and TARGET ref used for the diff; confirm tags fetched. - High-level diff stats and key directories touched. -- Concrete files/commits that indicate breaking changes or risk, with brief rationale. -- Tests or commands suggested to validate suspected risks (include pass criteria). +- Only concrete, actionable findings with evidence, impact, affected files, and action. +- A validation command or task only when it resolves a specific finding; include its pass criteria. - Explicit release gate call (ship/block) with conditions to unblock. - `Unblock checklist` section when (and only when) gate is `BLOCKED`. +- Do not report routine command results, pass counts, skips, deselections, or a validation-status inventory. From c549a825ae1df3b0b3c7c87b396eda6c0ea713b4 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 25 Jul 2026 13:31:27 +0900 Subject: [PATCH 08/10] chore: update final-release-review skill details --- .agents/skills/final-release-review/SKILL.md | 18 ++++++++++++------ .../references/review-checklist.md | 12 +++++++++--- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/.agents/skills/final-release-review/SKILL.md b/.agents/skills/final-release-review/SKILL.md index 5836308bc0..ced1118919 100644 --- a/.agents/skills/final-release-review/SKILL.md +++ b/.agents/skills/final-release-review/SKILL.md @@ -44,6 +44,7 @@ The review must be stable and actionable: avoid variance between runs by using e - "Could regress" risk statements without concrete evidence. - Not running tests locally. - If evidence is incomplete, do not block. Report a validation action only when the diff establishes a concrete unresolved risk; otherwise omit the candidate. +- A green gate must still explain the important release surfaces that were audited. Do not collapse a behavior-impacting release into a bare "No material risks identified" result. ## Workflow @@ -72,6 +73,9 @@ The review must be stable and actionable: avoid variance between runs by using e - Check only the relevant symmetry and failure axes: streaming/non-streaming, sync/async, fresh/resumed, client/server-managed state, success/error/cancellation, sequential/concurrent, and normal/repeated cleanup. - **Prove findings** - Promote a candidate to a finding only when the diff shows a concrete contract violation, a reachable supported-path regression, or a release-polish gap with user impact. + - Also retain substantiated non-blocking release considerations when they explain an intentional default change, public API or package expansion, durable schema transition, trace/logging behavior change, or other user-visible contract that is safe but important for release consumers to understand. + - For a green gate, report at least one such consideration whenever the diff changes runtime behavior, public APIs, package support, persisted schemas, protocols, configuration defaults, observability, or documented user workflows. Normally report two to five, grouped by contract rather than by directory. + - Assign **🟢 LOW** to a verified, correctly versioned, non-blocking consideration. Use neutral titles that describe the contract change; do not imply that a safe intentional change is a defect. - If static evidence cannot resolve a concrete semantic question, use the smallest public-path or installed-artifact probe that can. Prefer the same scenario against BASE and TARGET so environment failures and pre-existing behavior are separated from regressions. - Do not run repository unit-test slices merely to accumulate passing evidence. Run a focused test only when reproducing a specific failure or when no more direct contract, artifact, or runtime probe is available. - When you confirm a risk, cite the specific file/commit and explain the behavioral impact. @@ -80,12 +84,12 @@ The review must be stable and actionable: avoid variance between runs by using e - **🟢 LOW**: low blast radius or clearly covered behavior; no release gate impact. - **🟡 MODERATE**: plausible user-facing regression signal; needs validation but not a confirmed blocker. - **🔴 HIGH**: confirmed or strongly evidenced release-blocking issue. - - Attach a validation action only to a concrete unresolved risk. Give the smallest command or task and a pass condition; do not add generic follow-up checks. + - Every reported item needs a concrete next step and pass condition. For an unresolved risk, give the smallest validation or fix. For a verified LOW consideration, use a release-handoff task such as preserving exact migration, opt-out, compatibility, or supported-version wording in generated release notes. Do not invent additional code or test work merely to populate the report. - Breaking changes do not automatically require a BLOCKED release call when they are already covered by an appropriate version bump and migration/upgrade notes; only block when the bump is missing/mismatched (e.g., patch bump) or when the breaking change introduces unresolved risk. - **Form a recommendation** - State BASE_TAG and TARGET explicitly. - Provide a concise diff summary (key directories/files and counts). - - List only substantiated breaking changes, regressions/bugs, improvement opportunities, and missing release notes/migrations. Do not turn every audit clue into a report item. + - List substantiated breaking changes, regressions/bugs, improvement opportunities, missing release notes/migrations, and the most important verified non-blocking contract changes. Do not turn every audit clue or touched directory into a report item. - Recommend ship/block and the exact checks needed to unblock if blocking. If a breaking change is properly versioned (minor/major), you may still recommend a GREEN LIGHT TO SHIP while calling out the change. Use emoji and boldface in the release call to make the gate obvious. - If you cannot provide a concrete unblock checklist item, do not use `BLOCKED`. - Do not include routine command results, pass counts, skips, deselections, or a validation-status inventory. Mention a validation limitation only when it materially changes a specific finding or the release call. @@ -98,7 +102,7 @@ Use the following report structure in every response produced by this skill. Be Always use the fixed repository URL in the Diff section (`https://github.com/openai/openai-agents-python/compare/...`). Do not use `${GITHUB_REPOSITORY}` or any other template variable. Format risk levels as bold emoji labels: **🟢 LOW**, **🟡 MODERATE**, **🔴 HIGH**. -Every risk finding must contain an actionable next step. If the report uses `**🔴 BLOCKED**`, include an `Unblock checklist` section with at least one concrete command/task and a pass condition. +Every Risk assessment item must contain an actionable next step. If the report uses `**🔴 BLOCKED**`, include an `Unblock checklist` section with at least one concrete command/task and a pass condition. ``` ### Release readiness review ( -> TARGET ) @@ -116,11 +120,11 @@ https://github.com/openai/openai-agents-python/compare/... - ### Risk assessment (ordered by impact): -1) **** +1) **** - Risk: **<🟢 LOW | 🟡 MODERATE | 🔴 HIGH>**. - Evidence: - Files: - - Action: + - Action: 2) ... ### Unblock checklist (required when Release call is BLOCKED): @@ -132,7 +136,9 @@ https://github.com/openai/openai-agents-python/compare/... - ``` -If no risks are found, include a "No material risks identified" line under Risk assessment and still provide a ship call. Do not add a verification-status section or report routine check results. If the report is not blocked, omit the `Unblock checklist` section. +For a green gate, the Risk assessment must still itemize the important verified release considerations as **🟢 LOW** when the diff has behavior, API, package, schema, protocol, configuration, observability, or user-workflow impact. Do not use "No material risks identified" as the sole Risk assessment for such a release. That fallback is allowed only when the diff has no reportable contract or user-facing surface, such as a metadata-only release. Do not add a verification-status section or report routine check results. If the report is not blocked, omit the `Unblock checklist` section. + +Typical green items include a correctly versioned default change with its exact opt-in or opt-out path, a durable schema bump with backward-read behavior, an optional-extra or supported-version expansion that retains compatibility, or a tracing change with an explicit opt-out. Keep each item tied to consumer impact and a release-handoff pass condition. ### Resources diff --git a/.agents/skills/final-release-review/references/review-checklist.md b/.agents/skills/final-release-review/references/review-checklist.md index 76b2e2bbf0..8368fd6e1c 100644 --- a/.agents/skills/final-release-review/references/review-checklist.md +++ b/.agents/skills/final-release-review/references/review-checklist.md @@ -21,17 +21,22 @@ - Speculative risk without evidence. - Not running tests locally. - If uncertain, keep the gate green. Add a focused follow-up only when it resolves a concrete risk already identified in the diff. +- A green gate is not an empty audit. Itemize the most important verified release considerations when the diff changes behavior, APIs, packages, schemas, defaults, observability, or user workflows. ## Actionability contract -- Every risk finding should include: +- Every risk finding or non-blocking release consideration should include: - `Evidence`: specific file/commit/diff/test signal. - `Impact`: one-sentence user or runtime effect. - `Action`: concrete command/task with pass criteria. - A candidate becomes a finding only when it has a concrete contract violation, a reachable supported path, or a release-polish gap with user impact. +- A verified intentional change may become a **🟢 LOW** release consideration when it defines a contract users must understand, such as a default flip, new trace behavior, public API expansion, supported-version widening, or durable schema transition. +- For a green gate with behavior or contract impact, include at least one consideration and normally two to five. Group related changes by consumer impact rather than listing files or commits individually. +- For a resolved LOW consideration, the action may be a release-handoff check: retain exact compatibility, migration, opt-out, or configuration wording in generated release notes and state the pass condition. - Changed tests, missing tests, diff size, and risky patterns are discovery signals; they are not findings without contract or runtime evidence. - A `BLOCKED` report must contain an `Unblock checklist` with at least one executable item. -- If no executable unblock item exists, do not block. Keep the gate green and include an action only for a concrete unresolved risk. +- If no executable unblock item exists, do not block. Keep the gate green; use validation or fix actions for unresolved risks and release-handoff checks for resolved LOW considerations. +- Do not use "No material risks identified" as the sole Risk assessment when the diff has reportable behavior or contract changes. Reserve it for metadata-only or otherwise non-reportable release diffs. ## Two-stage audit @@ -103,8 +108,9 @@ If static inspection cannot resolve a concrete semantic question, run the smalle - BASE tag and TARGET ref used for the diff; confirm tags fetched. - High-level diff stats and key directories touched. -- Only concrete, actionable findings with evidence, impact, affected files, and action. +- Concrete, actionable findings plus the most important verified non-blocking release considerations, each with evidence, impact, affected files, and action. - A validation command or task only when it resolves a specific finding; include its pass criteria. +- For a resolved LOW consideration, a precise generated-release-note or migration-wording check with a pass condition is sufficient; do not manufacture code changes or redundant tests. - Explicit release gate call (ship/block) with conditions to unblock. - `Unblock checklist` section when (and only when) gate is `BLOCKED`. - Do not report routine command results, pass counts, skips, deselections, or a validation-status inventory. From c1b423749e2bf8ca5f89cad13e2a144c9683a6ee Mon Sep 17 00:00:00 2001 From: TheSaiEaranti Date: Sat, 25 Jul 2026 00:20:22 -0500 Subject: [PATCH 09/10] fix: include device nodes when parsing ls output (#3951) --- src/agents/sandbox/util/parse_utils.py | 25 ++++++++++++--- tests/sandbox/test_parse_utils.py | 44 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/agents/sandbox/util/parse_utils.py b/src/agents/sandbox/util/parse_utils.py index e9c49e1cd4..eb216ad8ad 100644 --- a/src/agents/sandbox/util/parse_utils.py +++ b/src/agents/sandbox/util/parse_utils.py @@ -20,10 +20,26 @@ def parse_ls_la(output: str, *, base: str) -> list[FileEntry]: permissions_str = parts[0] owner = parts[2] group = parts[3] - try: - size = int(parts[4]) - except ValueError: - continue + # Character and block devices report a device identifier in place of the + # size column, in one of two formats. `stat` reports size 0 for them. + if permissions_str[:1] in {"c", "b"}: + size = 0 + if parts[4].endswith(","): + # GNU coreutils prints "major, minor", which occupies two + # fields and shifts every following field by one. + parts = line.split(maxsplit=9) + if len(parts) < 10: + continue + name = parts[9] + else: + # BSD ls prints a single hexadecimal identifier, e.g. 0x3000002. + name = parts[8] + else: + try: + size = int(parts[4]) + except ValueError: + continue + name = parts[8] kind_map: dict[str, EntryKind] = { "d": EntryKind.DIRECTORY, @@ -37,7 +53,6 @@ def parse_ls_la(output: str, *, base: str) -> list[FileEntry]: if permissions_str[:1] not in {"d", "-"} and len(permissions_str) >= 2: permissions_str = "-" + permissions_str[1:] - name = parts[8] if kind == EntryKind.SYMLINK and " -> " in name: name = name.split(" -> ", 1)[0] diff --git a/tests/sandbox/test_parse_utils.py b/tests/sandbox/test_parse_utils.py index 549f830d1f..2cc29a8037 100644 --- a/tests/sandbox/test_parse_utils.py +++ b/tests/sandbox/test_parse_utils.py @@ -85,6 +85,50 @@ def test_parse_ls_la_strips_trailing_alternate_access_markers() -> None: assert entries[2].permissions.owner & FileMode.READ +def test_parse_ls_la_includes_gnu_device_nodes() -> None: + # GNU coreutils prints "major, minor" in place of the single size column, + # which shifts every following field by one. + output = ( + "-rw-r--r-- 1 root root 123 Jan 1 00:00 regular.txt\n" + "crw-rw-rw- 1 root root 1, 3 Jan 1 00:00 null\n" + "brw-rw---- 1 root disk 8, 0 Jan 1 00:00 sda\n" + ) + + entries = parse_ls_la(output, base="/dev") + + assert [entry.path for entry in entries] == [ + "/dev/regular.txt", + "/dev/null", + "/dev/sda", + ] + assert entries[1].kind == EntryKind.OTHER + assert entries[1].size == 0 + assert entries[2].owner == "root" + assert entries[2].group == "disk" + + +def test_parse_ls_la_includes_bsd_device_nodes() -> None: + # BSD ls prints a single hexadecimal device identifier instead of the + # "major, minor" pair, so the fields are not shifted. + output = ( + "-rw-r--r-- 1 root wheel 123 Jan 1 00:00 regular.txt\n" + "crw-rw-rw- 1 root wheel 0x3000002 Jan 1 00:00 null\n" + "brw-r----- 1 root operator 0x1000000 Jan 1 00:00 disk0\n" + ) + + entries = parse_ls_la(output, base="/dev") + + assert [entry.path for entry in entries] == [ + "/dev/regular.txt", + "/dev/null", + "/dev/disk0", + ] + assert entries[1].kind == EntryKind.OTHER + assert entries[1].size == 0 + assert entries[2].owner == "root" + assert entries[2].group == "operator" + + @pytest.mark.parametrize( "permissions", [ From 7a466f933101a5f16ff5d6d36096dbfbd96c5634 Mon Sep 17 00:00:00 2001 From: ashsolei Date: Sat, 25 Jul 2026 14:39:49 +0200 Subject: [PATCH 10/10] chore: re-apply iAiFy overlay after upstream force-sync --- AGENTS.md | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a04b51dea4..8724609508 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,7 +9,6 @@ This guide helps new contributors get started with the OpenAI Agents Python repo 1. [Policies & Mandatory Rules](#policies--mandatory-rules) 2. [Project Structure Guide](#project-structure-guide) 3. [Operation Guide](#operation-guide) -4. [Code Review Rules](#code-review-rules) ## Policies & Mandatory Rules @@ -33,7 +32,7 @@ When working on OpenAI API or OpenAI platform integrations in this repo (Respons #### `$implementation-strategy` -Before changing or reviewing runtime code, exported APIs, external configuration, persisted schemas, wire protocols, or other user-facing behavior, use `$implementation-strategy` to decide the compatibility boundary and implementation shape. During review, use it before requesting compatibility layers, migrations, new abstractions, or broader refactors. Judge breaking changes against the latest release tag, not unreleased branch-local churn. Interfaces introduced or changed after the latest release tag may be rewritten without compatibility shims unless they define a released or explicitly supported durable external state boundary, or the user explicitly asks for a migration path. Unreleased persisted formats on `main` may be renumbered or squashed before release when intermediate snapshots are intentionally unsupported. +Before changing runtime code, exported APIs, external configuration, persisted schemas, wire protocols, or other user-facing behavior, use `$implementation-strategy` to decide the compatibility boundary and implementation shape. Judge breaking changes against the latest release tag, not unreleased branch-local churn. Interfaces introduced or changed after the latest release tag may be rewritten without compatibility shims unless they define a released or explicitly supported durable external state boundary, or the user explicitly asks for a migration path. Unreleased persisted formats on `main` may be renumbered or squashed before release when intermediate snapshots are intentionally unsupported. ### ExecPlans @@ -206,15 +205,7 @@ make tests - Run `make format`, `make lint`, `make typecheck`, and `make tests` before marking work ready. - Commit messages should be concise and written in the imperative mood. Small, focused commits are preferred. -## Code Review Rules - -- Use `$implementation-strategy` to establish the requested outcome and latest released compatibility boundary before judging implementation scope or architecture. -- Treat added complexity as an actionable finding only when specific machinery is not required by the task, a released contract, supported durable state, or a verified runtime or platform risk. Identify the unnecessary machinery and recommend the smallest safe removal or direct replacement. -- Do not request speculative abstractions, general-purpose helpers, configuration knobs, dependencies, compatibility layers, feature flags, parallel code paths, or extensibility for hypothetical future consumers. -- Keep findings scoped to the patch. Do not block on unrelated cleanup, pre-existing bugs, or optional refactors; report them separately when useful. -- Require a broader refactor only when concrete evidence shows the focused change would otherwise be incorrect, unsafe, incompatible, or materially harder to maintain. - -### Baseline review expectations +### Review Process & What Reviewers Look For - ✅ Checks pass (`make format`, `make lint`, `make typecheck`, `make tests`). - ✅ Tests cover new behavior and edge cases.