diff --git a/.agents/references/README.md b/.agents/references/README.md new file mode 100644 index 0000000000..e57d5cab6c --- /dev/null +++ b/.agents/references/README.md @@ -0,0 +1,40 @@ +# SDK Maintainer References + +This directory captures long-lived implementation contracts of the OpenAI Agents Python SDK that are not replaceable by OpenAI API or platform facts from the Developer Docs MCP. The repo's `docs/` remain an SDK-specific behavioral contract; these references distill the ownership, compatibility, ordering, and failure semantics that maintainers need to preserve that contract. + +## Usage + +Read the reference map before changing or reviewing an affected runtime boundary, then open only the files relevant to that boundary. During issue and PR review, treat this directory as read-only background: use it to identify expected invariants, adjacent surfaces, and regression risks, but verify the current claim against the remote issue or PR, current code, tests, docs, release boundary, and focused runtime evidence. Do not edit references as a side effect of a review or treat them as proof of current issue status, PR behavior, or repository readiness. + +When implementation or dedicated repository-maintenance work establishes a reusable invariant that remains valid beyond one issue or PR, update the narrowest owning reference separately. Preserve the generalized contract, not the case history or decision outcome that revealed it. + +## Inclusion Criteria + +Add or retain a reference when the knowledge is SDK-specific, stable across multiple releases, easy to violate from one local code path, and expensive to reconstruct from source, tests, and repo docs during every review. Treat `docs/` as the SDK's user-facing behavioral contract; use these references to preserve the implementation constraints behind that contract. Prefer invariants and ownership rules over summaries of individual issues, PRs, or recent fixes. + +Do not store current issue or PR status, generic maintainer-review workflow, release notes, OpenAI API or platform behavior available through `$openai-knowledge`, or one-off implementation details in this directory. Put review methodology under `.agents/skills/`, released migration notes in `docs/release.md`, and API or platform facts behind `$openai-knowledge`. + +## Reference Map + +| Reference | Read before changing or reviewing | +|---|---| +| [Agent definition and run context](agent-definition-and-run-context.md) | Agent fields, cloning, dynamic instructions, enabled tools or handoffs, context wrappers, usage, or public agent identity | +| [Runner lifecycle](runner-lifecycle.md) | Turn accounting, guardrails, handoffs, interruptions, cancellation, or streaming parity | +| [Run item lifecycle](run-item-lifecycle.md) | Model output processing, new item types, stream events, replay conversion, session persistence, or RunState serialization | +| [Function and output schema](function-and-output-schema.md) | Function-tool signatures and metadata, strict JSON schema conversion, or structured output types | +| [Conversation state ownership](conversation-state-ownership.md) | Sessions versus server-managed continuation, input deltas, retries, compaction, or conversation resume | +| [Session persistence](session-persistence.md) | Session input callbacks, per-turn saves, retry rewind, atomicity, or compaction replacement | +| [RunState schema and resume boundary](runstate-schema.md) | Serialized state, schema versions, approvals, agent identity, or durable resume data | +| [Tool identity and routing](tool-identity.md) | Tool names, namespaces, lookup, approvals, MCP naming, handoffs, or call IDs | +| [Tool execution lifecycle](tool-execution-lifecycle.md) | Function-tool planning, approvals, guardrails, concurrency, cancellation, timeouts, or failure conversion | +| [Local MCP server lifecycle](local-mcp-server-lifecycle.md) | Local MCP connection ownership, manager state, request serialization, caching, filtering, retries, or cleanup | +| [Model and provider boundaries](model-provider-boundaries.md) | Model resolution, provider adapters, feature capability, request conversion, terminal events, or retries | +| [Tracing lifecycle](tracing-lifecycle.md) | Trace and span context, processors, export, flush, shutdown, resume, or sensitive data | +| [Realtime session lifecycle](realtime-session-lifecycle.md) | Realtime listeners, connections, background tasks, handoffs, event iteration, or cleanup | +| [Realtime tracing architecture](realtime-tracing.md) | Realtime API server traces versus Agents SDK client traces | +| [Voice pipeline lifecycle](voice-pipeline-lifecycle.md) | VoicePipeline STT/workflow/TTS ownership, event and audio ordering, stream cleanup, PCM framing, or tracing | +| [Sandbox runtime boundary](sandbox-runtime-boundary.md) | Sandbox session ownership, preparation, resume state, manifests, materialization, or cleanup | + +## Maintenance Rules + +Keep each rule in the narrowest reference that owns it. Cross-link instead of copying detailed rules between files. Describe current architecture and compatibility boundaries, not the chronology of how a bug was found. Use source paths and durable public contracts as anchors, and remove or rewrite guidance when ownership moves. diff --git a/.agents/references/agent-definition-and-run-context.md b/.agents/references/agent-definition-and-run-context.md new file mode 100644 index 0000000000..0eb8ebca8b --- /dev/null +++ b/.agents/references/agent-definition-and-run-context.md @@ -0,0 +1,57 @@ +# Agent Definition and Run Context + +Use this reference for changes to public `Agent` fields, cloning, dynamic instructions, enabled tools or handoffs, output schemas, `RunContextWrapper`, `ToolContext`, usage aggregation, or the distinction between a public agent and an internal prepared clone. + +## Public Definition and Cloning + +- Exported `Agent` and `AgentBase` dataclass field order is a positional compatibility boundary. Append optional fields where possible and test old positional construction when the order changes. +- `Agent.__post_init__()` is the eager boundary for invalid field categories such as names, tools, handoffs, hooks, model settings, output types, and tool-use behavior. Dynamic callbacks are validated when invoked because their result depends on the current run. +- `Agent.clone()` uses `dataclasses.replace()` and is shallow. Mutable fields and contained tool, handoff, hook, and provider objects remain shared unless the caller explicitly supplies replacements. +- When `clone(model=...)` replaces a model whose settings still equal the old model's implicit defaults, recompute the new model's implicit defaults. Preserve explicitly customized `model_settings` instead of silently resetting them. + +## Per-Turn Resolution + +- Resolve dynamic instructions with the current context and public agent for each model turn. Enforce the documented two-argument callable shape and await async results. +- Evaluate callable `FunctionTool.is_enabled` and `Handoff.is_enabled` against the current run context. Do not cache a prior run's enabled set on the reusable agent. +- Use one resolved tool and handoff view for model exposure, reserved-name and collision checks, local dispatch, tracing, and Realtime session updates. Re-resolving independently at those surfaces can expose one set and execute another. +- An internal prepared agent may add bound tools, instructions, or sampling settings, but hooks, `ToolContext.agent`, handoff callbacks, and public results should identify the public agent unless an internal identity is explicitly part of the contract. +- The effective output schema belongs to the agent and model call that produced the candidate output. A handoff can change the final output type, so do not assume the starting agent's schema when parsing or typing the final result. + +## Context Ownership + +- Every agent, tool, handoff, guardrail, and lifecycle hook in one run must agree on the same application context type. The context object is local runtime state and is never added to model input automatically. +- A normal `ToolContext.from_agent_context()` shares the underlying application object, usage accumulator, and approval mapping with its parent while adding call-scoped fields such as call ID, namespace, arguments, and conversation history. +- Nested `Agent.as_tool()` execution has a separate run loop, approval scope, and resumable tool state. On the normal function-tool path it still shares the application object and usage accumulator, while `tool_input` belongs to the nested wrapper and must not overwrite the parent's scoped value. +- Sharing the application object is not the same as sharing every wrapper field. Add explicit application-level isolation when nested mutation is unsafe, and do not reuse parent approval decisions for nested calls merely because the tool name or call ID looks similar. +- Context serialization is a separate durability decision. Read [RunState schema and resume boundary](runstate-schema.md) before persisting custom context objects, approvals, usage, or nested tool input. + +## Usage Accounting + +- `RunContextWrapper.usage` is the run-wide mutable accumulator. Add each model response exactly once across streaming, non-streaming, retries, nested runs, handoffs, and resume paths. +- Preserve authoritative `request_usage_entries` when combining usage. Do not synthesize a second per-request entry from aggregate totals when the provider or retry layer already supplied request-level records. +- Retry accounting may include failed attempts with no token totals. Keep request count, aggregate tokens, request-level entries, and trace span usage internally consistent without inventing provider token data. +- Streamed usage remains incomplete until terminal chunks and the stream driver finish. Do not finalize billing, result summaries, or usage-bearing spans from the last visible text delta alone. + +## Review Checklist + +1. Test direct construction and clone behavior without mutating shared caller-owned objects. +2. Resolve dynamic instructions, tools, and handoffs through the same public agent and current context used for dispatch. +3. Verify handoff and internal prepared-agent paths expose the intended public identity and effective output schema. +4. Test nested agent tools for shared application state and isolated scoped metadata. +5. Compare aggregate and per-request usage after streaming, retries, handoffs, interruption resume, and nested runs. + +## Sources + +- `docs/agents.md` +- `docs/context.md` +- `docs/results.md` +- `src/agents/agent.py` +- `src/agents/run_context.py` +- `src/agents/tool_context.py` +- `src/agents/usage.py` +- `src/agents/run_internal/turn_preparation.py` +- `src/agents/run_internal/run_loop.py` +- `tests/test_agent_config.py` +- `tests/test_agent_clone_shallow_copy.py` +- `tests/test_agent_as_tool.py` +- `tests/test_usage.py` diff --git a/.agents/references/conversation-state-ownership.md b/.agents/references/conversation-state-ownership.md new file mode 100644 index 0000000000..26783fa28a --- /dev/null +++ b/.agents/references/conversation-state-ownership.md @@ -0,0 +1,67 @@ +# Conversation State Ownership + +Use this reference for changes involving multi-turn input, sessions, `conversation_id`, `previous_response_id`, `auto_previous_response_id`, compaction, retries, `call_model_input_filter`, or `RunState` resume. + +## Choose One Conversation Strategy + +The state owner determines what the next model request should contain. + +| Strategy | State owner | Next-turn input | +|---|---|---| +| Explicit replay with `result.to_input_list()` | Application | Replay-ready history plus the new turn | +| SDK session | Application storage plus the SDK | The same session plus the new turn | +| `conversation_id` | OpenAI Conversations API | The same conversation ID plus only the new turn | +| `previous_response_id` or `auto_previous_response_id` | OpenAI Responses API | The previous response ID plus only the new turn | +| `RunState` resume | Serialized Agents SDK run | Resume the same interrupted run; this is not a new conversation strategy | + +In normal use, select one conversation strategy. Mixing client-managed replay or sessions with server-managed continuation can duplicate context unless the implementation explicitly reconciles both owners. Read [Session persistence](session-persistence.md) for the client-managed storage contract. + +## Server-Managed Continuation + +- `OpenAIServerConversationTracker` in `src/agents/run_internal/oai_conversation.py` owns delta calculation for `conversation_id`, `previous_response_id`, and `auto_previous_response_id`. +- Send only items that the server has not already acknowledged. Object identity is useful only within one process; resume and retry paths also require stable item IDs, tool call IDs, and content fingerprints. +- Update `previous_response_id` from the most recent response that actually has an ID. Do not erase a valid chain because an adjacent provider response lacks one. +- Session persistence cannot be combined with server-managed continuation. `validate_session_conversation_settings()` rejects a session with `conversation_id`, `previous_response_id`, or `auto_previous_response_id`; do not add a second history writer without defining reconciliation and dedupe semantics. +- Treat `conversation_id` and `previous_response_id` / `auto_previous_response_id` chaining as mutually exclusive state owners. + +## Filters, Retries, and Resume + +- `call_model_input_filter` runs on the prepared model payload. With server-managed continuation, that payload may already be a new-turn delta rather than full history. +- The filter must return `ModelInputData` with list input. Mark exactly the returned list as sent immediately before the request so nested preparation cannot add unsent items, rewind that tracking before retrying a failed request, and preserve it after success. +- Keep streaming and non-streaming tracker updates aligned. Both paths must preserve the same delta, retry, and response-ID semantics. +- Stateful retries require replay-safety evidence. Do not blindly resend a request that may already have advanced server state. +- `RunState` persists conversation identifiers and reconstructs tracker knowledge for resumed runs. Resume must not replay acknowledged input, lose unsent tool outputs, or increment the turn count without a model call. +- Conversation continuation carries context into a new turn. `RunState` resume continues a paused run. Do not substitute one mechanism for the other. + +## Compaction + +- `compaction_mode="previous_response_id"` depends on a usable stored response chain. +- `compaction_mode="input"` rebuilds from client-held items and is the fallback when the server chain is unavailable or `store=False` prevents later response lookup. +- Compaction must preserve the chosen state owner. Do not compact from local history and then also replay that history through server-managed continuation. + +## Handoffs + +- Server-managed conversations send deltas, so handoff input filters are not supported. `Handoff.input_filter` and `RunConfig.handoff_input_filter` should raise instead of rewriting a history the server already owns. +- `nest_handoff_history` is a client-history transformation. When server-managed continuation is active, disable it with a warning and continue with delta-only input. +- Keep generated items and session items distinct during handoff processing. The next model input may be filtered, but session history needs the full unfiltered item sequence when client-managed sessions are active. + +## Review Checklist + +1. Name the state owner before changing request construction. +2. Specify whether the model receives full history or a delta on every affected path. +3. Verify first turn, follow-up turn, retry, interruption, serialized resume, and streaming behavior. +4. Test tool calls and outputs separately; call IDs and output fingerprints have different dedupe roles. +5. Confirm that filtering, compaction, and session persistence do not introduce a second source of truth. + +## Sources + +- [OpenAI conversation state guide](https://developers.openai.com/api/docs/guides/conversation-state) +- [OpenAI running agents guide](https://developers.openai.com/api/docs/guides/agents/running-agents#choose-one-conversation-strategy) +- `src/agents/run_internal/oai_conversation.py` +- `src/agents/run_internal/run_loop.py` +- `src/agents/run_internal/session_persistence.py` +- `src/agents/run_state.py` +- `docs/running_agents.md` +- `docs/sessions/index.md` + +Recheck the official API reference with `$openai-knowledge` before changing server-managed continuation behavior. diff --git a/.agents/references/function-and-output-schema.md b/.agents/references/function-and-output-schema.md new file mode 100644 index 0000000000..fb313d6e28 --- /dev/null +++ b/.agents/references/function-and-output-schema.md @@ -0,0 +1,50 @@ +# Function and Output Schema + +Use this reference for changes to function-tool signature inspection, parameter metadata, strict JSON schema conversion, tool argument reconstruction, or structured agent output schemas. + +Schema behavior is a compatibility boundary shared by Python callables, Pydantic, model providers, and runtime validation. Keep schema generation and invocation aligned rather than fixing one representation in isolation. + +## Function Schema Ownership + +- Explicit decorator arguments for a function name or description override values inferred from the callable and its docstring. +- Parameter descriptions from parsed docstrings take precedence over description strings carried by `Annotated`. Preserve `Field` constraints, aliases, and defaults when merging `Annotated` metadata. +- A run context parameter is special only in the first parameter position. Exclude it from the model-visible schema while still supplying it during invocation; do not silently treat later context-typed parameters as injected context. +- Keep the inspected signature, generated Pydantic model, JSON schema, and `to_call_args()` reconstruction consistent. Cover positional-only parameters, keyword-only parameters, `*args`, and `**kwargs` when changing this path. +- Reject unsupported callable shapes or invalid schemas when the tool is constructed so failures do not depend on whether a particular model later selects the tool. + +## Strict JSON Schema Conversion + +- Strict conversion closes object schemas with `additionalProperties: false` and marks their declared properties required. Reject an explicit `additionalProperties: true` instead of silently changing its meaning. +- Preserve the meaning of unions, intersections, definitions, and references. Normalize `oneOf` where required, process `allOf`, retain chained references, and merge a referenced schema with sibling keys without discarding the siblings. +- Remove defaults that only encode Python `None`; a nullable type must remain represented by its type schema rather than by an unsupported default. +- `ensure_strict_json_schema()` may mutate a non-empty input dictionary. Copy caller-owned schemas at public boundaries before conversion. Empty-schema conversion must return a fresh object rather than shared mutable state. +- Keep strictness explicit. If a tool or output schema opts out of strict mode, preserve that choice through provider conversion instead of partially applying strict normalization. + +## Structured Output Schemas + +- Plain `str` output and no declared output type use the plain-text path. Pydantic models and dictionary-shaped outputs expose their object schema directly; other Python types use the SDK's wrapper object with the `response` key. +- Keep generated output names stable and descriptive for nested generics, unions, and `Literal` types. These names are observable in provider requests and diagnostics. +- Parse model output as JSON and validate it through the output type adapter. Convert JSON or validation failures to the SDK's model-behavior error boundary rather than leaking provider- or Pydantic-specific exceptions. +- Streaming and non-streaming adapters must carry the same schema, strictness flag, wrapper behavior, and validation result. + +## Review Checklist + +1. Test precedence among explicit metadata, docstrings, `Annotated`, and `Field` values. +2. Test invocation reconstruction for positional-only, keyword-only, variadic, and context-bearing callables. +3. Test nested objects, unions, intersections, sibling and chained references, nullable fields, and caller-owned schema mutation. +4. Test plain text, direct object output, wrapped scalar or generic output, invalid JSON, and validation failure. +5. Verify every provider adapter receives the same normalized schema and strictness decision. + +## Sources + +- `src/agents/function_schema.py` +- `src/agents/strict_schema.py` +- `src/agents/tool.py` +- `src/agents/agent_output.py` +- `src/agents/models/` +- `tests/test_function_schema.py` +- `tests/test_function_tool_decorator.py` +- `tests/test_strict_schema.py` +- `tests/test_strict_schema_oneof.py` +- `tests/test_output_tool.py` +- `tests/models/` diff --git a/.agents/references/local-mcp-server-lifecycle.md b/.agents/references/local-mcp-server-lifecycle.md new file mode 100644 index 0000000000..71903e6979 --- /dev/null +++ b/.agents/references/local-mcp-server-lifecycle.md @@ -0,0 +1,56 @@ +# Local MCP Server Lifecycle + +Use this reference for changes to Python-managed MCP servers, `MCPServerManager`, client-session request ordering, tool caching or filtering, local MCP retries, cancellation, or cleanup. Hosted MCP is a provider tool and follows the OpenAI API contract; use `$openai-knowledge` for that protocol surface. Read [Tool identity and routing](tool-identity.md) for server-prefixed names and [Tool execution lifecycle](tool-execution-lifecycle.md) for approval and invocation behavior after an MCP tool is converted to a `FunctionTool`. + +## Connection Ownership and Task Affinity + +- A local `MCPServer` owns its transport, `ClientSession`, and `AsyncExitStack` from `connect()` through `cleanup()`. Partial connection failure still requires closing every context already entered. +- Some MCP transports use AnyIO cancel scopes that require connection and cleanup in the same task. Do not wrap either operation in a helper that silently creates another task. +- `MCPServerManager` preserves task affinity in sequential mode and uses one long-lived worker task per server in parallel mode. Timeouts must run inside that owning task; on Python versions without `asyncio.timeout()`, cancel the current worker task and translate only timer-originated cancellation to `TimeoutError`. +- Cleanup runs servers in reverse order and continues across ordinary cleanup failures. Cancellation suppression is an explicit manager policy; do not accidentally convert unrelated `BaseException` failures into recoverable connection errors. +- Server cleanup must clear session and transport-visible state even when exit-stack cleanup raises, so the same server object can reconnect without exposing stale session handles or workers. + +## Manager State + +- Keep configured servers, connected servers, failed servers, active servers, and per-server errors as distinct views. `active_servers` is the agent-facing list; with `drop_failed_servers=True` it excludes failed connections while preserving configured order. +- Non-strict connection records failures and continues with the connected subset. Strict connection cleans up work started by the failed attempt and restores the previous coherent active state before raising. +- `reconnect(failed_only=True)` retries the deduplicated failed set without disturbing healthy connections. A full reconnect cleans up all servers first and rebuilds manager state. +- Parallel connection still needs deterministic per-server state and complete cleanup after cancellation or one hard failure. Do not let completion order decide `active_servers`, `failed_servers`, or which workers remain registered. + +## Shared Session Requests and Retries + +- Streamable HTTP can require requests on one shared MCP session to be serialized. The same lock must cover tool calls, tool listing, prompts, and resource operations that share that session; serializing only `call_tool()` still permits sibling cancellation and protocol races. +- Preserve outer cancellation. A cancelled shared request may qualify for an isolated-session retry only when the transport identifies it as an inner or transient session failure and retry budget remains. +- Isolated-session retries are transport-specific recovery. Count isolated session setup and execution against the same retry budget, retry only the supported transient failure shapes, and never replay mixed exception groups or ordinary 4xx failures as if they were safe. +- Generic `list_tools()` and `call_tool()` retries use the configured attempt count and backoff. Validate required arguments locally before starting retries so deterministic input errors never reach the server or consume retry budget. +- MCP tool failure conversion follows the effective server or agent `failure_error_function`. Explicit `None` means propagate; cancellation of the parent run must not become model-visible tool failure output. + +## Tool Discovery, Cache, and Filtering + +- The unfiltered server tool list is the cacheable value. Apply static or dynamic filters to a copy for each requesting agent and run context; never let one request's filtered or merged metadata mutate the shared cache. +- `cache_tools_list=True` assumes server schemas are stable until `invalidate_tools_cache()` marks them dirty. Connection or filter changes must not accidentally make a stale filtered list authoritative. +- Dynamic filters require both `run_context` and agent. A filter exception excludes that tool and logs the failure rather than exposing it by default. +- Schema conversion to strict form is best effort and must not mutate the MCP server's original input schema. If strict conversion fails, preserve the original schema and keep metadata isolated per converted `FunctionTool`. +- Tool list collision errors, prefixed-name generation, and approval policy validation must be deterministic regardless of server response or connection completion order. + +## Review Checklist + +1. Identify the task that owns connect, every request, timeout cancellation, and cleanup for each transport. +2. Test partial connect failure, strict and non-strict manager modes, reconnect, repeated cleanup, and manager cancellation. +3. Test overlapping tool, prompt, and resource requests when shared-session serialization is enabled. +4. Prove retries preserve outer cancellation, consume one budget, and do not replay deterministic or unsupported failures. +5. Test cache invalidation, context-dependent filters, schema immutability, duplicate names, and reconnect with the public runner path. + +## Sources + +- `docs/mcp.md` +- `src/agents/mcp/server.py` +- `src/agents/mcp/manager.py` +- `src/agents/mcp/util.py` +- `tests/mcp/test_mcp_server_manager.py` +- `tests/mcp/test_connect_disconnect.py` +- `tests/mcp/test_client_session_retries.py` +- `tests/mcp/test_caching.py` +- `tests/mcp/test_tool_filtering.py` +- `tests/mcp/test_server_errors.py` +- `tests/mcp/test_runner_calls_mcp.py` diff --git a/.agents/references/model-provider-boundaries.md b/.agents/references/model-provider-boundaries.md new file mode 100644 index 0000000000..d9f95a5d76 --- /dev/null +++ b/.agents/references/model-provider-boundaries.md @@ -0,0 +1,75 @@ +# Model and Provider Boundaries + +Use this reference for changes to model resolution, `ModelSettings`, provider adapters, Responses versus Chat Completions behavior, request conversion, streaming terminal events, transport reuse, or model retries. + +## Core Boundary + +The run loop depends on the `Model` interface, not on one provider's request or response schema. + +- `Model.get_response()` returns a normalized `ModelResponse`. +- `Model.stream_response()` yields normalized response stream events while preserving provider payloads needed by public raw-event consumers. +- `ModelProvider.get_model()` resolves names to model implementations and owns provider-level caches or connections. +- `Model.close()` and `ModelProvider.aclose()` release persistent transport resources when an implementation owns them. + +Provider adapters own request construction, provider feature validation, terminal event interpretation, usage conversion, and translation into SDK item shapes. Keep provider-specific branching out of the core run loop unless it represents a shared SDK contract. + +## Model and Settings Resolution + +- An explicit `RunConfig.model` overrides the agent model. A model instance is used directly; a model name is resolved through the configured `ModelProvider`. +- Implicit default settings must follow the resolved model name, including when a run-level model name replaces the agent default. +- Resolve agent settings with run-level settings by overlaying non-`None` values. Preserve the documented merge behavior for structured fields such as `extra_args` and retry settings. +- Do not pass provider request extras into tracing by default. `ModelSettings.to_traceable_dict()` is the boundary for settings considered safe and meaningful in traces. + +## Capability Ownership + +Do not infer that a feature available in one adapter is supported by every `Model` implementation. + +- Responses-specific features include server-managed response chaining, conversation-aware request fields, tool namespaces, deferred tool loading, tool search, response includes, compaction, and Responses websocket transport. +- Chat Completions generally requires client-managed replay and adapter conversion of Responses-compatible SDK items. Unsupported server-state or tool features should be rejected or explicitly ignored according to the adapter's documented validation mode. +- Realtime has its own session protocol, event model, and server tracing. Do not route Realtime behavior through the standard Responses or Chat Completions assumptions. +- Third-party model adapters may preserve only the shared `Model` contract. New provider-specific fields need an explicit conversion and fallback policy. + +Validate capabilities at the adapter boundary where the resolved model and complete request are known. Avoid public flags that appear accepted by the SDK but are silently dropped before the provider request. + +## Provider Data and Terminal Semantics + +- Preserve provider-supplied string IDs, request IDs, usage, and opaque provider data when the public SDK contract exposes them. +- Normalize provider objects and mapping payloads without relying on truthiness for valid empty or zero values. +- A transport stream ending is not automatically a successful model response. Responses `failed` and `incomplete` terminals, explicit error events, and a missing terminal payload must produce the documented failure behavior in both HTTP and websocket paths. +- Keep semantically equivalent HTTP, websocket, streaming, and non-streaming paths aligned on final `ModelResponse`, errors, request IDs, and usage. + +## Transport Resource Ownership + +- Persistent Responses websocket models are loop-bound resources. Cache reusable websocket model instances by running event loop and model name; do not share one connection or `asyncio.Lock` across loops. +- Use weak loop ownership so an unused cache does not keep a closed event loop alive. When a live connection itself pins a closed loop, prune it with synchronous abort and state clearing rather than awaiting work on that closed loop. +- A provider that caches persistent models must make `aclose()` close every unique cached model and clear its caches. Close on a still-running owner loop when possible; do not drive an inactive foreign loop inside `asyncio.to_thread()`. +- A model used without a running loop cannot safely join the loop-scoped websocket cache. Preserve the non-reuse fallback rather than attaching it to an arbitrary global loop. +- Connection reuse ends after protocol errors, pre-terminal disconnects, cancellation that invalidates framing, or explicit close. Clear connection and loop-bound lock state together so a later request cannot reuse half-closed transport state. + +## Retry and Replay Safety + +- Provider retry advice can describe retryability, delay, and replay safety; the runner must not replace provider-specific evidence with a generic status-code assumption. +- Requests that use server-managed conversation state or may have produced side effects are not automatically replay-safe. A retry policy must account for whether the provider could have accepted the previous attempt. +- Retry conversion and error handlers must preserve the original exception semantics and avoid leaking sensitive request payloads through chaining, logs, traces, or provider error objects. + +## Review Checklist + +1. Identify which adapter owns the feature and how unsupported adapters behave. +2. Verify model and implicit-settings resolution when run config overrides the agent. +3. Compare HTTP/websocket and streaming/non-streaming terminal behavior when applicable. +4. Preserve request IDs, usage, provider data, and error semantics through normalization. +5. Prove retries are safe for the request's state ownership and side effects. +6. Test transport reuse, cross-loop access, closed-loop pruning, and provider shutdown when persistent connections are involved. + +## Sources + +- `src/agents/models/interface.py` +- `src/agents/model_settings.py` +- `src/agents/run_internal/turn_preparation.py` +- `src/agents/models/openai_responses.py` +- `src/agents/models/openai_chatcompletions.py` +- `src/agents/models/multi_provider.py` +- `src/agents/models/_response_terminal.py` +- `src/agents/run_internal/model_retry.py` +- `tests/models/` +- `tests/test_config.py` diff --git a/.agents/references/realtime-session-lifecycle.md b/.agents/references/realtime-session-lifecycle.md new file mode 100644 index 0000000000..4a1f9c68a4 --- /dev/null +++ b/.agents/references/realtime-session-lifecycle.md @@ -0,0 +1,73 @@ +# Realtime Session Lifecycle + +Use this reference for `RealtimeSession` changes involving entry, exit, listeners, connections, background tasks, approvals, handoffs, event iteration, tracing context, or cleanup. + +## Resource Ownership + +Treat the session as the owner of these resources once they are acquired: + +| Resource | Acquisition | Required release or terminal state | +|---|---|---| +| Model listener | `add_listener()` during entry | `remove_listener()` | +| Model connection | `model.connect()` | `model.close()` | +| Event iterators | Waiting on the event queue | Wake or terminate every waiter on close | +| Guardrail tasks | Created during output processing | Complete, or cancel and account for completion | +| Tool-call tasks | Created when `async_tool_calls=True` | Complete, or cancel and account for completion | +| Pending approvals and outputs | Added during tool execution | Resolve, retain for retry, or clear during terminal cleanup | +| Agent and model settings | Updated on handoff or `update_agent()` | Keep runtime state and model configuration aligned | + +Do not add a new side effect before a failure point without defining who releases it. + +## Entry and Exit + +- Python does not call `__aexit__` when `__aenter__` raises. Any listener, connection, task, tracing scope, or other resource acquired before the exception needs explicit failure cleanup. +- Keep construction free of external side effects. Acquire listeners and connections during entry where failures can be handled coherently. +- `close()` and internal cleanup must be idempotent. Repeated close paths should still wake event iterators without closing the model twice. +- Mark the session closed only after the cleanup state is coherent. If model close fails, decide deliberately whether retry is possible and which resources remain owned. + +## Async Task and Context Rules + +- `asyncio` tasks inherit a snapshot of the creator's context. A background task cannot update the caller task's `ContextVar` state. +- A `ContextVar` token must be reset in the same context that created it. Never pass a token to a different task and assume cleanup can reset it safely. +- Shared session fields can be mutated by the listener path, tool-call tasks, `close()`, `update_agent()`, and handoff handling. Review ordering and races whenever one of those paths changes. +- Calling `task.cancel()` requests cancellation; it does not prove the task has finished its `finally` blocks or released resources. Await cancelled tasks when completion matters, or document and test why dropping them is safe. +- Background-task exceptions must reach a deterministic owner. They must not silently disappear or leave event consumers blocked. + +## Agent Transitions + +- Handoffs and the public `update_agent()` API are equivalent agent-transition surfaces. Keep their model settings, tool and handoff resolution, emitted events, and tracing metadata aligned unless a difference is intentional and documented. +- Resolve dynamic tools and enabled handoffs once per transition when possible, then reuse the exact resolved values for model settings and metadata. +- With concurrent tool calls, capture the agent snapshot associated with each call. Do not route a call through whichever agent happens to be current when the task eventually runs. + +## Guardrails and Response Ordering + +- Realtime output guardrails inspect accumulated transcript text at configured debounce thresholds, not each token and not a final `Runner` output object. They emit `guardrail_tripped` instead of raising a normal Runner tripwire exception. +- A tripped output guardrail marks the response interrupted before awaiting transport work, emits one trip event per response, forces response cancellation, and sends safe follow-up input naming the guardrail. Concurrent guardrail tasks must not interrupt or message the same response twice. +- Guardrail callbacks can run after audio has already been buffered or played. Consumers must treat `audio_interrupted` as the signal to stop local playback; text rejection alone cannot retract audio already delivered. +- An exception from one output guardrail is logged and skipped so it does not silently terminate the live session. Exceptions that escape the background guardrail task must become a `RealtimeError` event rather than disappearing. +- Realtime function-tool input guardrails follow the same optional pre-approval and mandatory post-approval ordering as standard function tools, but their rejection is returned through Realtime tool output and events. +- Follow-up `response.create` work triggered by tools, handoffs, or guardrails must respect the active response lifecycle. Wait for `response.done` or the model layer's equivalent gate before starting a conflicting response. + +## Failure-Path Tests + +Add focused tests for affected phases: + +1. Instruction, tool, or handoff resolution fails during entry. +2. Model connection fails after listener registration. +3. A background tool or guardrail task raises or is cancelled. +4. Cleanup runs while event iterators are waiting. +5. `close()` is called repeatedly or from another task. +6. A handoff or `update_agent()` fails partway through model-settings application. +7. Tool output sending fails after local execution and must be retried without running the tool twice. +8. Concurrent guardrail tasks trip once, cancel playback, and do not overlap follow-up responses. + +Verify lifecycle changes with the real public path where feasible; helper-only tests are insufficient when task ownership or context propagation determines the result. + +## Sources + +- `src/agents/realtime/session.py` +- `src/agents/realtime/model.py` +- `src/agents/realtime/openai_realtime.py` +- `tests/realtime/test_session.py` +- `tests/realtime/test_session_exceptions.py` +- `docs/realtime/guide.md` diff --git a/.agents/references/realtime-tracing.md b/.agents/references/realtime-tracing.md new file mode 100644 index 0000000000..4ee280c46b --- /dev/null +++ b/.agents/references/realtime-tracing.md @@ -0,0 +1,42 @@ +# Realtime Tracing Architecture + +Use this reference when reviewing or implementing Realtime tracing behavior, especially claims that `RealtimeSession` should emit the same trace hierarchy as `Runner`. + +## Two Separate Tracing Systems + +Realtime integrations involve two independent tracing paths: + +| Path | Owner | Configuration | Result | +|---|---|---|---| +| Realtime API server tracing | Realtime API | `"auto"`, `workflow_name`, `group_id`, and `metadata` | The server creates a Realtime session trace in the Traces Dashboard. | +| Agents SDK client tracing | Agents SDK tracing provider | `trace()`, `agent_span()`, and other SDK span factories | The SDK exports locally created traces and spans through its tracing processor. | + +The current Python SDK has no mapping from an Agents SDK client `trace_id`, `span_id`, or parent context into `RealtimeModelTracingConfig` or the model's `session.update`. A server-created Realtime trace is therefore not attached as a child of an SDK-created trace or span by this implementation. Likewise, adding an SDK `agent_span()` around `RealtimeSession` does not make server-side trace contents children of that span. + +If both paths are enabled, the dashboard can contain two separate traces. A shared `group_id` can make them easier to filter and correlate, but it does not merge them or create a parent-child relationship. + +## Current Python SDK Behavior + +- `RealtimeModelTracingConfig` exposes only `workflow_name`, `group_id`, and `metadata` in `src/agents/realtime/config.py`. +- `OpenAIRealtimeWebSocketModel` defaults the Realtime tracing configuration to `"auto"` when the caller does not provide one. +- After receiving `session.created`, the model sends the tracing configuration through a `session.update` event. +- `RealtimeRunConfig.tracing_disabled` prevents the SDK from enabling Realtime tracing for that session. + +Verify these paths in `src/agents/realtime/openai_realtime.py` and `src/agents/realtime/session.py`; do not rely on old issue descriptions because Realtime tracing support has changed over time. + +## Maintainer Constraints + +1. Identify whether the behavior belongs to the Realtime API's server trace or an Agents SDK client trace created with `trace()`. +2. A client-side agent span does not repair missing server tracing and does not create the unified hierarchy produced by `Runner`. +3. The current Python SDK cannot place server-created Realtime spans under an SDK-created trace or span because it does not carry client trace parentage through the Realtime tracing configuration. Recheck the live protocol with `$openai-knowledge` before treating that implementation gap as permanent. +4. Use the server trace for Realtime model activity. Use a shared `group_id` or metadata when correlation with a client trace is required. +5. Parallel SDK spans need an explicit product and maintenance contract covering the dual-trace user experience, async task context, handoff parenting, failure cleanup, and the client-only operations represented by those spans. +6. A client trace becoming non-empty is not evidence that Realtime server activity has been captured or parented correctly. + +## Sources + +- `src/agents/realtime/config.py` +- `src/agents/realtime/openai_realtime.py` +- `src/agents/realtime/session.py` + +Recheck the official API reference with `$openai-knowledge` before changing this guidance or implementing new protocol behavior. diff --git a/.agents/references/run-item-lifecycle.md b/.agents/references/run-item-lifecycle.md new file mode 100644 index 0000000000..79abe7ddc3 --- /dev/null +++ b/.agents/references/run-item-lifecycle.md @@ -0,0 +1,79 @@ +# Run Item Lifecycle + +Use this reference for changes to model output processing, `RunItem` types, tool call and output items, stream events, replay conversion, session history, or serialized run state. + +## Item Flow + +The runtime carries one semantic item through several representations: + +1. A model adapter returns provider output in `ModelResponse.output`. +2. `process_model_response()` converts recognized output into public `RunItem` objects and internal executable tool-run records in `ProcessedResponse`. +3. Tool execution and handoffs add output items and choose a `SingleStepResult.next_step`. +4. The resulting items feed `RunResult`, semantic stream events, session persistence, tracing, and `RunState` serialization. +5. Replayable items convert back to model input through `RunItem.to_input_item()` or `run_item_to_input_item()` after SDK-only metadata is handled. + +Keep provider payloads, public run items, and internal execution records distinct. A provider item may be observable without requiring local execution, while a local tool-run record may need to preserve the selected SDK tool object and routing identity. + +## Generated, Session, and Model Input Views + +- `new_step_items` describes items generated by the current step. +- `session_step_items` preserves the full unfiltered sequence when session history must retain items that a handoff or input filter omitted from the next model request. +- `generated_items` is the public observability view and prefers `session_step_items` when present. +- Model input is a replay view, not the canonical storage view. Approval placeholders, SDK-only metadata, unsupported IDs, and orphaned calls may need filtering or normalization before an API request. + +Do not force these views into one list. History persistence, user-visible results, and the next provider request have different correctness requirements. + +## Adding or Changing an Item Type + +Update every applicable surface together: + +- `src/agents/items.py` for the public `RunItem` type, accessors, and replay conversion. +- `src/agents/run_internal/run_steps.py` for processed response and executable tool-run records. +- `src/agents/run_internal/turn_resolution.py` for provider output recognition, item creation, side effects, and next-step selection. +- `src/agents/run_internal/tool_execution.py`, `tool_actions.py`, or `tool_planning.py` for execution, dedupe, approvals, and outputs. +- `src/agents/run_internal/items.py` for normalization, replay conversion, fingerprints, dedupe, and provider-boundary metadata stripping. +- `src/agents/stream_events.py` and streaming queue helpers for public semantic events. +- `src/agents/run_state.py` for serialization and deserialization when the item can survive interruption. +- `src/agents/run_internal/session_persistence.py` for session conversion, sanitization, and retry accounting. +- Tracing and usage conversion when the item contributes observable tool or model work. + +## Compatibility Rules + +- Public stream event names are compatibility-sensitive. Do not rename an existing event, even to fix spelling, without an explicit breaking-change plan. +- Preserve provider-supplied IDs and opaque provider data until the owning boundary deliberately removes them. Do not invent IDs or coerce malformed values to make replay appear valid. +- Preserve SDK-only metadata needed for display, routing, approvals, tool origin, or resume, but strip it before sending payloads to a provider that does not accept it. +- Tool call and output pairs must retain the same string call ID across execution, replay, session persistence, and resume. +- Empty, falsey, structured, image, file, and custom tool outputs are valid values unless the public tool contract explicitly rejects them; do not use broad truthiness checks to decide whether output exists. + +## Replay Integrity + +- Prune orphan calls only from runner-generated or resumed history where the SDK owns call/output pairing. Preserve caller-supplied initial input unless an explicit public normalization contract says otherwise. +- When dropping an orphan tool call, also drop reasoning items tied to that removed call so the provider does not receive a reasoning item without its required following item. Do not drop a lone reasoning item merely because its following item is absent locally; server-managed conversation state may own that item. +- `reasoning_item_id_policy="omit"` strips IDs only from SDK-generated follow-up reasoning items. It does not rewrite initial caller input, must survive `RunState` resume, and can be superseded by a later `call_model_input_filter` that deliberately returns IDs. +- Pair anonymous tool-search outputs with the latest compatible anonymous call and never pair a named call with an anonymous output. A missing call ID does not justify inventing a persistent provider identity. +- `provider_data` and provider IDs have boundary-specific ownership. Preserve them for raw results and provider requests that accept them, but strip private or replay-unsafe metadata from session and server-conversation history where the SDK contract requires sanitized items. + +## Review Checklist + +1. Follow the item from provider response through result, stream, session, replay, and `RunState`. +2. Test both typed provider objects and mapping payloads when adapters support both. +3. Verify IDs, metadata, and output values survive every required round-trip. +4. Test filtering and dedupe without losing the latest valid call/output pair. +5. Compare streaming event order with the non-streaming item sequence. +6. Test orphan pruning and reasoning pairing with client-managed replay and server-managed continuation separately. + +## Sources + +- `src/agents/items.py` +- `src/agents/stream_events.py` +- `src/agents/run_internal/items.py` +- `src/agents/run_internal/run_steps.py` +- `src/agents/run_internal/turn_resolution.py` +- `src/agents/run_internal/session_persistence.py` +- `src/agents/run_state.py` +- `tests/test_items_helpers.py` +- `tests/test_run_internal_items.py` +- `tests/test_stream_events.py` +- `tests/test_run_state.py` +- `docs/running_agents.md` +- `docs/results.md` diff --git a/.agents/references/runner-lifecycle.md b/.agents/references/runner-lifecycle.md new file mode 100644 index 0000000000..4c2a5fdad9 --- /dev/null +++ b/.agents/references/runner-lifecycle.md @@ -0,0 +1,67 @@ +# Runner Lifecycle + +Use this reference for changes to `Runner`, turn accounting, guardrails, hooks, handoffs, interruptions, cancellation, or streaming and non-streaming behavior. + +## Turn Boundary + +A turn is one logical model invocation plus processing of that response. Tool execution, handoff resolution, session persistence, interruption resume, and retries inside that logical invocation do not independently consume turns. + +- Increment the turn counter exactly once when the run loop starts a logical model turn. Transport or provider retries inside `get_new_response()` remain part of that turn. +- A handoff changes the current agent, but the next turn begins only when the new agent invokes a model. +- Resuming `NextStepInterruption` continues the paused turn. Resolve stored approvals and tool work before deciding whether another model call is needed. +- Preserve `max_turns` and the current turn in `RunState`; resume must not reset the budget or charge a turn twice. + +## Guardrail Ordering + +- Input guardrails belong to the starting agent and run only for the initial user input. Do not rerun them after handoffs or when resuming an interruption. +- Sequential input guardrails must finish before model-side effects begin. Parallel input guardrails may overlap the model call, so a tripwire or exception must cancel and await the in-flight model task and sibling guardrail tasks. +- Tool input guardrails run before the approved tool side effect. Tool output guardrails run after local execution and before the output is accepted into the next step. +- Output guardrails run only after a candidate final output exists. Streaming must await them and preserve the same tripwire and exception behavior as non-streaming execution before declaring completion. +- Guardrail results are observable run state. Preserve them across handoffs, error handlers, streamed completion, and `RunState` round-trips. + +## Step State Machine + +`SingleStepResult.next_step` is the control boundary after one model response and its local side effects: + +| Step | Meaning | +|---|---| +| `NextStepRunAgain` | Continue with the current agent and make another model call | +| `NextStepHandoff` | Switch the current agent, emit the transition, then continue | +| `NextStepFinalOutput` | A final candidate exists; finish terminal hooks, output guardrails, persistence, and result construction | +| `NextStepInterruption` | Persist enough processed state to resume pending approvals without rerunning completed work | + +Do not bypass this state machine with path-local completion logic. New terminal or pausable behavior must define non-streaming, streaming, session, tracing, and serialized-resume semantics. + +## Streaming Parity and Cancellation + +- Streaming and non-streaming paths must produce equivalent final output, generated items, current agent, usage, guardrail results, session history, and interruption state for the same model behavior. +- Raw transport events may differ, but semantic `RunItemStreamEvent` and `AgentUpdatedStreamEvent` emission must follow the same processed items and agent transitions used by the non-streaming result. +- `stream_events()` is the stream driver's cleanup boundary. Keep consuming it until exhaustion after normal completion or `cancel()`, or explicitly close the async iterator; merely breaking after the last visible token does not prove session writes, guardrails, compaction, sandbox cleanup, usage, or terminal errors have settled. +- Immediate cancellation marks the result complete and requests task cancellation. `after_turn` cancellation leaves the current model/tool turn running so it can persist state and usage before the next turn. Preserve this distinction instead of treating both modes as queue shutdown. +- Terminal run-loop, guardrail, and max-turn errors must be surfaced from `stream_events()` after the required queued events are handled. Preserve `run_loop_exception` as a diagnostic view of the background task, not as a replacement completion primitive. +- `task.cancel()` is a request, not cleanup completion. Await cancelled tasks when their `finally` blocks, exceptions, or owned resources affect run correctness. +- Keep lifecycle hooks aligned across both paths, especially model start/end, handoff, tool start/end, and final-output hooks. + +## Review Checklist + +1. Identify which turn and which agent own the behavior. +2. Trace every `NextStep` outcome, including interruption resume. +3. Compare streaming and non-streaming side effects and terminal ordering. +4. Test guardrail tripwires and exceptions in sequential and parallel modes when relevant. +5. Verify normal exhaustion, explicit iterator close, immediate cancellation, and after-turn cancellation leave the documented result and owned resources in a coherent state. + +## Sources + +- `src/agents/run.py` +- `src/agents/run_internal/run_loop.py` +- `src/agents/run_internal/run_steps.py` +- `src/agents/run_internal/turn_preparation.py` +- `src/agents/run_internal/turn_resolution.py` +- `src/agents/run_internal/guardrails.py` +- `tests/test_agent_runner.py` +- `tests/test_agent_runner_streamed.py` +- `tests/test_cancel_streaming.py` +- `tests/test_guardrails.py` +- `tests/test_run_state.py` +- `docs/streaming.md` +- `docs/results.md` diff --git a/.agents/references/runstate-schema.md b/.agents/references/runstate-schema.md new file mode 100644 index 0000000000..da1cad9b1d --- /dev/null +++ b/.agents/references/runstate-schema.md @@ -0,0 +1,64 @@ +# RunState Schema and Resume Boundary + +Use this reference for changes involving `RunState` serialization, deserialization, approvals, trace state, sandbox state, agent identity, tool output payloads, or any persisted resume data. + +## Compatibility Boundary + +`RunState` is the durable SDK pause/resume boundary. Treat the serialized JSON shape as compatibility-sensitive once a schema version has shipped in a release. + +- `to_json()` always emits `CURRENT_SCHEMA_VERSION`. +- `from_json()` must continue reading every version in `SUPPORTED_SCHEMA_VERSIONS`. +- Older SDKs intentionally reject newer or unsupported versions rather than attempting forward compatibility. +- Unreleased schema versions may be renumbered or squashed before release when intermediate snapshots are intentionally unsupported. +- Every supported version must have a non-empty one-line entry in `SCHEMA_VERSION_SUMMARIES`. + +## When to Bump the Schema + +Bump `CURRENT_SCHEMA_VERSION` when a serialized `RunState` snapshot changes in a way that affects resume correctness or would silently lose data when read under an older schema label. + +Examples include: + +- New persisted fields on `RunState`, `ModelResponse`, `ProcessedResponse`, interruptions, approvals, tool outputs, sandbox state, trace state, or agent-owned state. +- New run item, tool call, approval, or output item variants that can appear in serialized state. +- New SDK-only metadata needed to route, dedupe, approve, retry, or resume a tool call. +- A changed meaning for an existing serialized field. + +Do not rely on current-reader tests alone. Add a regression that rewrites `$schemaVersion` to an older supported label when appropriate and proves the old label is accepted, rejected, or migrated deliberately. + +## Identity and Routing State + +Serialized state must preserve enough identity to resume without changing behavior: + +- Agent identity must distinguish duplicate agent names in the same graph. +- Function tools should persist canonical lookup keys, including `bare`, `namespaced`, and `deferred_top_level`. +- Tool call IDs must remain provider-supplied strings; do not coerce arbitrary values into IDs. +- Approval decisions and rejection messages must restore against the same tool identity and call ID they originally targeted. +- The per-agent tool-use tracker must preserve stable duplicate-agent identity so tool-choice reset behaves the same after resume. +- Server-managed conversation identifiers must restore into `OpenAIServerConversationTracker` without replaying acknowledged input. + +## Context and Secrets + +Context serialization is intentionally conservative. + +- Mapping contexts can round-trip directly. +- Custom contexts need explicit serializers and deserializers when exact restoration matters. +- Without a safe serializer, snapshots may record metadata and warnings rather than the raw object. +- Do not persist secrets in `RunContextWrapper.context`, trace data, tool outputs, or custom data unless the caller explicitly chose that durability boundary. + +## Review Checklist + +1. Identify every serialized field whose shape or meaning changes. +2. Decide whether the affected schema version is released or unreleased. +3. Update `CURRENT_SCHEMA_VERSION` and `SCHEMA_VERSION_SUMMARIES` when resume compatibility requires it. +4. Keep released schema versions readable, or fail with an explicit compatibility error if the old label cannot safely represent the new data. +5. Test `to_json()` output, `from_json()` restoration, string round-trips, and resumed execution through the public `Runner.run(...)` or `Runner.run_streamed(...)` path. + +## Sources + +- `src/agents/run_state.py` +- `src/agents/result.py` +- `src/agents/run_internal/agent_runner_helpers.py` +- `src/agents/run_internal/oai_conversation.py` +- `src/agents/run_internal/run_steps.py` +- `src/agents/run_internal/tool_execution.py` +- `tests/test_run_state.py` diff --git a/.agents/references/sandbox-runtime-boundary.md b/.agents/references/sandbox-runtime-boundary.md new file mode 100644 index 0000000000..9afae44e84 --- /dev/null +++ b/.agents/references/sandbox-runtime-boundary.md @@ -0,0 +1,70 @@ +# Sandbox Runtime Boundary + +Use this reference for changes to sandbox session ownership, `SandboxAgent` preparation, manifests, capabilities, host-path materialization, snapshots, resume state, agent transitions, or cleanup. + +## Runtime Ownership + +The outer `Runner` owns agent turns, approvals, handoffs, tracing, session history, and `RunState`. A sandbox session owns the execution environment, workspace, processes, mounts, and provider-specific connection state. Do not move one layer's lifecycle into the other without defining resume and cleanup behavior for both. + +- A live `SandboxRunConfig.session` is caller-owned. The runner may configure and use it but must not delete or fully tear it down. +- A session created or resumed through `SandboxRunConfig.client` is runner-owned. Cleanup runs pre-stop hooks, persists snapshot-backed workspace state, stops and shuts down the session, deletes provider resources when required, and closes dependencies. +- Session cleanup must be idempotent and release acquired `SandboxAgent` concurrency guards even when persistence or provider cleanup fails. +- A `SandboxAgent` instance cannot be reused concurrently across runs because prepared capability tools and session state are bound to one live run. Clone or construct separate agents for concurrent work. + +## Session Source and Saved State + +Resolve the session source in this order: injected live session, resumable sandbox state carried by `RunState`, explicit `SandboxRunConfig.session_state`, then a newly created session. Manifest and snapshot inputs seed only a fresh session; they do not overwrite an injected or resumed workspace. + +- `RunState` sandbox data and explicit `session_state` represent provider connection or session state used to reconnect to existing work. +- A snapshot represents saved workspace contents used to seed a new session. It is not interchangeable with provider session state. +- Preserve stable per-agent resume identity across handoffs, including graphs with duplicate agent names. Object identity is process-local, so serialized state needs stable keys and explicit current-agent selection. +- Serialize runner-owned sessions after stop-time persistence has completed so a later resume can reattach when the backend survives or reconstruct the workspace from the saved snapshot when it does not. + +## Agent Preparation + +- Clone capability instances per run before binding them to a live session. Reusing mutable capability objects can leak tools, sampling settings, or session references across runs. +- Validate capability dependencies before exposing tools. Capability tool construction, instruction fragments, input processing, and sampling adjustments must use the same effective capability set. +- Build instructions in the documented order: SDK sandbox base prompt or explicit replacement, agent instructions, capability instructions, remote-mount policy, then the rendered filesystem description. +- Bind capability tools to the live session and preserve a link from the prepared clone to the public `SandboxAgent`. Dynamic instructions and hooks should observe the public agent rather than an internal clone with implementation-only state. +- Handoffs stay in the outer run loop and select another agent-bound sandbox session. A nested `Agent.as_tool()` run owns its own nested runner and sandbox lifecycle. + +## Filesystem Trust Boundary + +- Manifest entry destinations are workspace-relative and must not escape the workspace. The workspace root itself must be absolute where the backend requires an absolute runtime root. +- `LocalFile` and `LocalDir` sources are host-side inputs. Resolve them against a trusted base directory, require explicit application-controlled `extra_path_grants` outside that base, and reject untrusted manifests that try to authorize their own host access. +- Validate local sources at use time, not only when parsing the manifest. Defend against symlinked sources, parent-directory swaps, platform path aliases, and archive members that change meaning between validation and extraction. +- Archive extraction must reject traversal, unsafe links, and unsupported member types before writing, and enforce entry, byte, and expansion limits without materializing an unbounded member list. +- Extra path grants are runtime access, not durable workspace content. Snapshots and `persist_workspace()` include the workspace root, not arbitrary granted paths. +- Credentials for mounts or providers must remain in the owning adapter and must not appear in generated shell commands, model-visible errors, logs, or serialized sandbox state. + +## Provider and Error Boundary + +- Normalize backend failures to sandbox errors without discarding provider details needed for diagnosis. Preserve explicit retryability instead of inferring it later from a message string. +- Keep portable sandbox paths separate from host filesystem paths and provider identifiers. Conversion belongs in the backend or materialization boundary, not in agent-facing tools. +- Temporary clones, mounts, sinks, and dependency resources need failure cleanup during partial startup as well as normal shutdown. +- Capability tools should report bounded output and preserve provider exit status or structured error data without exposing private runtime metadata to the model. + +## Review Checklist + +1. Name the owner of every live session, provider client, mount, process, capability, and temporary resource. +2. Test injected, resumed, explicit-state, snapshot-seeded, and fresh-session paths separately. +3. Verify handoffs, duplicate agent names, interruption resume, and cleanup failure preserve the intended session mapping. +4. Test host-path, symlink, traversal, archive-limit, and credential-redaction boundaries on applicable platforms. +5. Exercise the public `Runner` path so agent preparation, capability binding, persistence, and cleanup run together. + +## Sources + +- `docs/sandbox/guide.md` +- `docs/sandbox/clients.md` +- `src/agents/sandbox/runtime.py` +- `src/agents/sandbox/runtime_session_manager.py` +- `src/agents/sandbox/runtime_agent_preparation.py` +- `src/agents/sandbox/manifest.py` +- `src/agents/sandbox/materialization.py` +- `src/agents/sandbox/workspace_paths.py` +- `src/agents/sandbox/session/archive_extraction.py` +- `tests/sandbox/test_runtime.py` +- `tests/sandbox/test_runtime_agent_preparation.py` +- `tests/sandbox/test_session_state_roundtrip.py` +- `tests/sandbox/test_materialization.py` +- `tests/sandbox/test_extract.py` diff --git a/.agents/references/session-persistence.md b/.agents/references/session-persistence.md new file mode 100644 index 0000000000..a81b3d5b19 --- /dev/null +++ b/.agents/references/session-persistence.md @@ -0,0 +1,80 @@ +# Session Persistence + +Use this reference for changes to client-managed sessions, session input callbacks, per-turn persistence, retries, rewind, compaction replacement, or session backend implementations. + +Read [Conversation state ownership](conversation-state-ownership.md) first when server-managed continuation is also involved. A client-managed session is a history store; it is not a second owner for a server-managed conversation. + +## Session Contract + +- `get_items(limit=N)` returns the latest `N` items in chronological order. +- `add_items()` appends one logical batch. Backends should make the batch atomic so partial turns are not visible after failure. +- `pop_item()` removes the current tail item and is used only for guarded rollback of items the current run can prove it owns. +- `clear_session()` clears the session boundary; compaction decorators that replace history must provide stronger restore behavior around destructive replacement. + +Third-party implementations target the `Session` protocol. Internal base classes and backend-specific metadata are not the compatibility contract unless explicitly documented. + +## Backend Consistency + +- An explicit `get_items(limit=N)` argument overrides the backend's default session limit. Return the latest `N` items in chronological order, with a deterministic tie-breaker when timestamps can collide. +- Preserve caller batch order. Persist the items and any indexes or structural metadata required to read them as one atomic operation. A failed batch must leave earlier history unchanged, and any backend-internal retry must not create duplicates. +- Serialize initialization and conflicting writes at the backend's actual consistency boundary. Concurrent first writers must not race, and cancellation or failure must not strand locks or transactions. +- Apply configured table or collection names and session settings consistently across reads, writes, deletes, metadata updates, and wrapper operations. +- For backends that deserialize stored records, a corrupt record must not hide valid history or cause unrelated records to be deleted. Define consistent `get_items()` and `pop_item()` behavior that isolates the bad record and continues safely. +- Preserve creation timestamps and advance update timestamps deliberately. Backend-only identifiers and metadata must not leak into model-facing session items. +- Close only resources the backend owns. An injected engine, client, or connection remains caller-owned unless the public contract explicitly transfers ownership. + +## Preparing Input Versus Persisting Input + +`prepare_input_with_session()` returns two different values: the normalized input for the next model request and the subset of new-turn items that should be appended to the session. + +- Existing history must not be re-appended as new input, even when `session_input_callback` deep-copies, reorders, filters, duplicates, or reconstructs items. +- A callback may change the model view without rewriting already stored history. +- Handoff and model-input filters may omit items from the next request while `session_step_items` retains the complete unfiltered sequence for history and observability. +- Normalize and deduplicate the model request and persistence candidates through the same canonical item helpers, then apply boundary-specific sanitization. + +## Per-Turn Save and Resume + +- Persist each completed turn, not only the final run result. Tool outputs and handoff items must survive a later error or interruption. +- `_current_turn_persisted_item_count` tracks which generated items have already been saved during streaming, retry, or resume. Count items after conversion and persistence filtering, not from the unsanitized source list. +- Resuming an interruption must save newly produced approval and tool output items without duplicating inputs or previously persisted outputs. +- Preserve full session items separately from filtered model input when updating `RunState` after resume. +- A guardrail trip must preserve the accepted user input while excluding speculative assistant or tool work that the tripwire invalidated. Test sequential and parallel guardrails in streaming and non-streaming modes because their persistence timing differs even though the resulting history must remain coherent. + +## Retry Rewind + +Retry cleanup is ownership-sensitive and best effort. + +- Rewind only an exact serialized suffix that belongs to the failed attempt. Never scan backward and delete merely similar historical items. +- Verify the complete suffix before popping. If a pop fails or returns an unexpected item, restore already popped items in chronological order. +- Wait for backends with asynchronous cleanup semantics before starting the next retry when stale tail items could be observed. +- Do not forward live `RunContextWrapper` objects through retry rewind or compaction storage paths unless the session API explicitly owns that runtime context. + +## Compaction Replacement + +- Treat history replacement as a transaction: capture the prior state, apply the compacted state, and restore the prior state if clear or replacement fails. +- Defer response-based compaction while local tool outputs still need to be associated with the response chain. +- Choose input-based or previous-response-based compaction according to the actual state owner and `store` behavior; do not combine a local replay with a server-owned history chain. +- Compaction output is a run item and must follow the item lifecycle, session sanitization, and `RunState` rules rather than bypassing them as backend-only data. + +## Review Checklist + +1. Distinguish model input, new-turn persistence candidates, and full session history. +2. Test atomic failure, duplicate content, reordered callbacks, and filtered handoff input. +3. Test save behavior after tool execution, handoff, guardrail trip, interruption, and resume. +4. Prove retry rewind removes only the attempt-owned suffix and restores on partial failure. +5. Test compaction replacement failures without losing the previous history. +6. Test backend ordering, atomic batches, concurrent first writes, configured names and limits, corrupt records, and resource ownership. + +## Sources + +- `src/agents/memory/session.py` +- `src/agents/memory/session_settings.py` +- `src/agents/memory/sqlite_session.py` +- `src/agents/extensions/memory/` +- `src/agents/run_internal/session_persistence.py` +- `src/agents/run_internal/items.py` +- `src/agents/run_internal/run_steps.py` +- `tests/memory/` +- `tests/extensions/memory/` +- `tests/test_agent_runner.py` +- `tests/test_agent_runner_streamed.py` diff --git a/.agents/references/tool-execution-lifecycle.md b/.agents/references/tool-execution-lifecycle.md new file mode 100644 index 0000000000..ad8f793a0e --- /dev/null +++ b/.agents/references/tool-execution-lifecycle.md @@ -0,0 +1,64 @@ +# Tool Execution Lifecycle + +Use this reference for changes to function-tool planning, approvals, tool guardrails, concurrency, cancellation, timeouts, hooks, error conversion, or resumed execution. Read [Tool identity and routing](tool-identity.md) when names, namespaces, lookup keys, or call IDs also change. + +## Plan Before Side Effects + +`process_model_response()` discovers executable work, but `tool_planning.py` decides which work may run now. Keep discovery, approval partitioning, and invocation as separate phases. + +- Fresh and resumed turns need different plans. A resumed interruption must execute unresolved or newly approved work without rediscovering or rerunning completed calls. +- Approval state is authoritative once resolved. Do not call a dynamic `needs_approval` checker again for a call whose status is already approved or rejected. +- Deduplicate by invocation identity before execution while preserving model order for public call and output items. A repeated tool definition is not a repeated call, and a repeated call ID must not execute twice. +- Validate enabled tools and canonical lookup before side effects. A tool disabled after model output or absent from the resolved tool set must follow the configured missing-tool behavior rather than reaching a stale callable. + +## Approval and Guardrail Ordering + +- Pre-approval input guardrails are an early rejection optimization. They may run before an approval interruption, but input guardrails must run again immediately before invocation because state, policy, or arguments may have changed while approval was pending. +- Rechecking guardrails does not mean rechecking approval. Persisted approval decisions and rejection messages must remain attached to the same tool identity and call ID across `RunState` resume. +- Tool input guardrails finish before the local side effect. Tool output guardrails finish before output becomes accepted run state, model input, or persisted session history. +- The tool guardrail pipeline applies to `FunctionTool` invocation. Handoffs, hosted tools, built-in provider tools, and nested `Agent.as_tool()` runs have separate execution boundaries unless they explicitly opt into equivalent checks. + +## Concurrency and Failure Semantics + +SDK-side function-tool concurrency is independent of provider-side parallel tool-call generation. The provider controls how many calls appear in one response; `RunConfig.tool_execution.max_function_tool_concurrency` controls how many local function handlers run at once. + +- Preserve model order in emitted outputs even when handlers complete out of order. +- Isolate sibling results. A cancelled or failed call must not discard outputs already produced by successful siblings. +- Distinguish cancellation of one tool handler from cancellation of the parent run. Tool-local cancellation can follow the configured tool failure policy; parent cancellation must propagate promptly instead of becoming model-visible tool output. +- `task.cancel()` is not terminal cleanup. On sibling failure, drain cancelled handlers and wait for post-invocation work within the bounded cleanup policy. On parent cancellation, cancel remaining tasks and attach result callbacks so late exceptions are observed without delaying cancellation indefinitely. +- Select and raise failures deterministically when several tasks fail, while still observing secondary failures. Do not let task-set iteration order or eager task execution change the public result. + +## Invocation Boundary + +- Decorated synchronous Python functions run through `asyncio.to_thread()` so they do not block the event loop. Async function tools run in the event loop and are the only decorated handlers that support SDK timeouts. +- Timeout handling and ordinary exception handling are distinct policies. `timeout_behavior` and `timeout_error_function` own timeout conversion; `failure_error_function=None` means ordinary exceptions propagate instead of becoming model-visible output. +- Tool start/end hooks and function spans surround the actual invocation once per call, including failure and cancellation paths. Do not emit a successful end state before output guardrails complete. +- Per-run resources such as resolved `Computer` implementations must be initialized and disposed by the run that acquired them. +- Nested `Agent.as_tool()` execution owns a nested run loop and nested resumable state. Scope cached nested state by the parent `RunState` and call identity, not only by the reusable agent or tool object. +- `AgentToolUseTracker` records tool use per agent identity. When `reset_tool_choice=True`, reset the effective next-turn tool choice after that agent uses a tool so `required` or a named choice cannot force an accidental loop; do not mutate the agent's declared settings across independent runs. +- Persist and restore the tool-use tracker across interruption and sandbox resume, including graphs with duplicate agent names, so resumed tool-choice behavior matches uninterrupted execution. + +## Review Checklist + +1. Trace fresh execution, approval interruption, approval rejection, and serialized resume separately. +2. Verify guardrail, approval, hook, trace, invocation, output, and persistence order. +3. Test sequential, bounded-concurrency, sibling failure, tool-local cancellation, and parent cancellation paths. +4. Test default, custom, and disabled failure conversion plus timeout behavior where applicable. +5. Confirm every started task and per-run resource reaches a deterministic terminal state. + +## Sources + +- `docs/running_agents.md` +- `docs/tools.md` +- `docs/guardrails.md` +- `docs/human_in_the_loop.md` +- `src/agents/run_internal/tool_planning.py` +- `src/agents/run_internal/tool_execution.py` +- `src/agents/tool.py` +- `tests/test_agent_runner.py` +- `tests/test_agent_runner_streamed.py` +- `tests/test_function_tool.py` +- `tests/test_tool_guardrails.py` +- `tests/test_tool_choice_reset.py` +- `tests/test_tool_use_tracker.py` +- `tests/test_run_state.py` diff --git a/.agents/references/tool-identity.md b/.agents/references/tool-identity.md new file mode 100644 index 0000000000..3465972c48 --- /dev/null +++ b/.agents/references/tool-identity.md @@ -0,0 +1,71 @@ +# Tool Identity and Routing + +Use this reference for changes involving function-tool names, namespaces, provider wire names, lookup, approvals, tracing, MCP exposure, handoffs, or tool call IDs. + +## Identity Layers + +One tool can have several related identifiers. They are not interchangeable. + +| Layer | Purpose | Canonical source | +|---|---|---| +| Public name | User- and model-facing tool name | `tool.name` | +| Explicit namespace | Distinguishes tools with the same public name | Tool namespace metadata | +| Qualified or dispatch name | Routes a model call to the intended tool | `namespace.name` when a namespace exists | +| Lookup key | Collision-free internal identity | `bare`, `namespaced`, or `deferred_top_level` tuple | +| Approval keys | Matches approval decisions to the intended tool | Canonical qualified and permitted alias keys | +| Trace name | Human-readable tracing label | Explicit trace name or public name | +| Call ID | Identifies one invocation, not the tool definition | Provider-supplied string | + +Do not collapse these layers into one string or introduce local rules that only one caller uses. + +## Canonical Helpers + +Use `src/agents/_tool_identity.py` as the single implementation layer. Important helpers include: + +- `get_function_tool_lookup_key_for_tool()` and `get_function_tool_lookup_key_for_call()` for canonical lookup identity. +- `get_function_tool_dispatch_name()` and `get_function_tool_qualified_name()` for routing and display surfaces that require qualification. +- `get_function_tool_approval_keys()` for approval matching. +- `get_function_tool_trace_name()` and `get_tool_call_trace_name()` for trace labels. +- `validate_function_tool_lookup_configuration()` and `build_function_tool_lookup_map()` for collision detection and dispatch maps. +- `normalize_tool_call_for_function_tool()` when provider payloads must be normalized for a selected tool. + +If a proposed change bypasses these helpers, first prove that the target surface has intentionally different semantics. + +## MCP and Handoff Rules + +- `include_server_in_tool_names` is opt-in. Server-prefixed MCP names affect the model-exposed collision-safe name; they do not rename the original tool on the MCP server. +- Reserved names and enabled handoff names participate in collision avoidance only on the paths that expose generated model-facing names. +- `Handoff.default_tool_name()` is the source of default handoff tool names. Keep Realtime and non-Realtime handoff conversion aligned with it. +- Do not forward a naming option through a path where the downstream helper does not consult it and then describe the change as runtime behavior. Trace the complete caller-to-dispatch path first. + +## Deferred Tool Search Rules + +- A top-level `FunctionTool` with `defer_loading=True` and no explicit namespace uses the synthetic lookup key `("deferred_top_level", tool.name)`. +- The Responses wire shape for a loaded deferred top-level tool can look like `namespace == name`. Treat that namespace as reserved for the synthetic deferred tool-search path, not as a normal explicit namespace. +- `tool_namespace()` must reject an explicit namespace that equals the inner tool name. Otherwise a normal namespaced tool and a deferred top-level tool would have the same wire shape. +- Preserve the synthetic namespace on approval, interruption, tracing, and `ToolContext` surfaces when it identifies the model call, but dispatch the actual local tool through the deferred lookup key and strip the synthetic namespace before invoking the tool. +- Permanent approvals for deferred top-level tools should key by `deferred_top_level:`. A bare-name approval alias is allowed only when no visible bare sibling can make that alias ambiguous. + +## Tool Call ID Rules + +- Preserve provider-supplied string call IDs across call items, approvals, outputs, retries, and serialized state. +- Do not coerce arbitrary values with `str(...)`. Canonical extractors return a call ID only when the source value is already a string. +- Do not use a call ID as a tool-definition identity or a tool name as an invocation identity. +- When a provider omits a stable identifier, use an existing fingerprint or dedupe policy for that item type instead of inventing a cross-provider ID contract. + +## Review Checklist + +1. Identify every identifier layer affected by the change. +2. Trace the actual runtime path from model-visible name to lookup, approval, invocation, output, and trace metadata. +3. Compare adjacent canonical helpers before adding conversion or fallback behavior. +4. Test collisions between bare, namespaced, deferred, MCP, local function, and handoff tools when applicable. +5. Require a regression test that fails on the base and proves the model-visible or dispatch behavior, not only an intermediate argument value. + +## Sources + +- `src/agents/_tool_identity.py` +- `src/agents/agent.py` +- `src/agents/mcp/` +- `src/agents/handoffs/__init__.py` +- `src/agents/run_internal/tool_execution.py` +- `src/agents/run_state.py` diff --git a/.agents/references/tracing-lifecycle.md b/.agents/references/tracing-lifecycle.md new file mode 100644 index 0000000000..30fcbf01bb --- /dev/null +++ b/.agents/references/tracing-lifecycle.md @@ -0,0 +1,58 @@ +# Tracing Lifecycle + +Use this reference for changes to SDK trace or span context, processors, export, flush, shutdown, resumed trace state, or sensitive-data handling. Read [Realtime tracing architecture](realtime-tracing.md) before applying these client-side rules to Realtime server traces. + +## Context and Parenting + +- The current trace and span are held in `ContextVar` state. Async tasks inherit a snapshot when created; later changes in a child task do not rewrite the parent task's context. +- A context token must be reset in the context that created it. Start and finish ownership cannot be transferred between tasks without an explicit context boundary. +- A no-op trace or span cannot be a real parent. Propagate no-op behavior instead of exporting children with the sentinel `no-op` trace or span ID. +- Span factories should inherit trace metadata needed by processors, but they must not mutate the trace's caller-owned metadata mapping. + +## Run and Resume Ownership + +- A runner-created trace encloses run-loop-owned guardrails, model calls, tool execution, handoffs, session persistence, and error handling. Do not assume every completion callback or resource cleanup runs before trace finish; place newly traced cleanup explicitly inside the trace lifetime or create a deliberate separate trace/span context. +- An existing caller trace remains caller-owned. `Runner` may create child spans but must not finish or flush the caller's trace. +- `RunState` stores enough trace metadata to continue an interrupted run. Resume may reattach only when the trace ID was previously started in the process and the effective workflow name, group ID, metadata, and tracing key identity still match. +- Reattachment must not emit a duplicate trace-start event. If the saved state cannot prove a compatible live trace, create a normal trace according to the current run configuration instead of pretending to resume the old context. +- Tracing API keys are omitted from serialized `RunState` by default. A hash can verify that the caller supplied the same explicit key without persisting the secret; raw key persistence is opt-in. + +## Processor and Export Isolation + +- Trace processors are observability extensions and must not change application success. Catch processor callback, exporter, flush, and shutdown failures and report them as non-fatal. +- The default batch worker starts lazily on first queued item to avoid import-time thread and fork hazards. Keep top-level imports free of worker creation and shutdown-handler duplication. +- An exporter exception must not kill the batch worker and strand future traces. Drop or report the failed batch according to policy, then keep the worker usable. +- `flush_traces()` waits for queued and in-flight export work, so callers should invoke it after the trace closes when they require immediate delivery. It is not a substitute for finishing a partially built trace. +- Shutdown is best effort and deadline-aware. It should request exporter shutdown, interrupt retry backoff, drain within the remaining deadline, and return without changing the process exit code when an exporter blocks or a backend remains unavailable. +- Keep `TraceProvider.force_flush()` and `shutdown()` defaulting to no-ops for compatibility with custom providers that predate these lifecycle methods. + +## Data Boundaries + +- `trace_include_sensitive_data=False` controls captured span payload fields; it does not automatically sanitize exception objects, chaining, tracebacks, logs, or telemetry created elsewhere. +- Redaction must cover `__cause__`, `__context__`, formatter failures, and model-visible error conversion when an original exception carries tool arguments or provider payloads. `raise ... from None` changes display, not object retention. +- The OpenAI trace exporter owns ingest-specific payload sanitization such as field-size limits and supported usage keys. Custom processors should continue receiving the SDK's normal trace data unless their contract says otherwise. +- Per-run tracing keys, organization, and project routing must stay attached to the trace or exported item that selected them; do not let mutable global exporter state reroute an already-created trace. + +## Review Checklist + +1. Identify which task and context own each trace and span start, finish, and token reset. +2. Test success, exception, cancellation, interruption, serialized resume, full stream exhaustion, and explicit stream close. +3. Verify processor and exporter failures remain non-fatal and do not kill later export work. +4. Test flush and shutdown with queued work, in-flight export, retry backoff, and a blocking exporter. +5. Audit sensitive data through span payloads, exception chains, logs, and serialized state. + +## Sources + +- `docs/tracing.md` +- `src/agents/tracing/context.py` +- `src/agents/tracing/scope.py` +- `src/agents/tracing/traces.py` +- `src/agents/tracing/spans.py` +- `src/agents/tracing/provider.py` +- `src/agents/tracing/processors.py` +- `src/agents/tracing/setup.py` +- `src/agents/run_state.py` +- `tests/test_trace_processor.py` +- `tests/test_tracing.py` +- `tests/test_run_state.py` +- `tests/tracing/test_import_side_effects.py` diff --git a/.agents/references/voice-pipeline-lifecycle.md b/.agents/references/voice-pipeline-lifecycle.md new file mode 100644 index 0000000000..a0934a102e --- /dev/null +++ b/.agents/references/voice-pipeline-lifecycle.md @@ -0,0 +1,56 @@ +# Voice Pipeline Lifecycle + +Use this reference for changes to `VoicePipeline`, `AudioInput`, `StreamedAudioInput`, STT sessions, TTS task ordering, voice lifecycle events, PCM framing, result streaming, or voice tracing. Realtime agents use a different live-session architecture; read [Realtime session lifecycle](realtime-session-lifecycle.md) for that path. + +## Pipeline Ownership + +`VoicePipeline` owns an STT-to-workflow-to-TTS producer task and returns a `StreamedAudioResult` that drives its observable completion. + +- Static `AudioInput` produces one transcription and one workflow turn. `StreamedAudioInput` creates a long-lived transcription session and runs one workflow turn for each emitted transcript until the input or session ends. +- The multi-turn pipeline owns the transcription session and closes it in `finally` before marking output complete. Partial setup and workflow failure must not strand the STT connection or producer task. +- `workflow.on_start()` applies only to the streamed multi-turn path. Its failure is logged and skipped so the transcription session can still start; normal per-turn workflow failures are terminal and surface through the result stream. +- The SDK does not provide application-level interruption handling for `StreamedAudioInput`. Lifecycle events expose turn boundaries, but microphone muting, playback interruption, and barge-in policy remain application-owned. + +## Text, Audio, and Event Ordering + +- A workflow can yield multiple text fragments. The text splitter returns ready-to-synthesize text plus a remainder; synthesize non-empty ready text even when it is shorter than a default sentence threshold, and retain the remainder for the turn's final flush. +- TTS segment tasks may run concurrently, but `_ordered_tasks` and the dispatcher must emit their audio and lifecycle events in workflow text order rather than completion order. +- `turn_started` precedes audio for that turn. `turn_ended` is emitted only after the turn's final text remainder has been synthesized and its audio dispatched. `session_ended` follows all ordered segment queues and all turns. +- A `VoiceStreamEventError` terminates result streaming and the stored exception is raised after task cleanup. `session_ended` is a lifecycle marker, not proof of success; consumers must still observe the terminal exception from `stream()`. +- Consuming `StreamedAudioResult.stream()` is the public completion and error boundary. On normal `session_ended`, let the producer finish before cleanup so session close and trace end are not cancelled by result teardown. + +## PCM and Caller Data + +- PCM16 samples span two bytes. Preserve a trailing half-sample across TTS chunks, combine it with the next chunk, and pad only the final unmatched byte at end of segment. +- Apply `buffer_size` to TTS source chunks without changing sample order. Convert to float32 only after PCM16 framing is complete, then apply caller-provided `transform_data` to each emitted array. +- `AudioInput.to_base64()` and audio-file conversion must not mutate the caller's NumPy buffer when converting float input to PCM16. +- Empty input and empty text-splitter output are valid boundaries. They must not cause NumPy reduction errors, phantom TTS calls, or missing turn/session lifecycle events. + +## Trace Lifetime and Data + +- The pipeline trace stays active for the full asynchronous producer lifecycle, not only until `VoicePipeline.run()` returns its result object. +- Each output turn owns a speech-group span and each synthesized segment owns a child speech span. Finish the turn span after ordered audio dispatch and finish the pipeline trace after STT session close and output completion. +- Text and audio sensitivity are independent controls. `trace_include_sensitive_data` governs transcript and TTS text, while `trace_include_sensitive_audio_data` governs encoded audio payloads. +- Error paths must finish active speech spans and the enclosing trace without replacing the original pipeline exception. + +## Review Checklist + +1. Test static and streamed input, including STT setup failure, workflow failure, TTS failure, and transcription-session close. +2. Verify fragment concurrency never changes audio, turn, or session event order. +3. Test short splitter output, empty output, odd-byte chunks, cross-chunk sample boundaries, int16, and float32 conversion. +4. Consume the public result stream and verify terminal errors, task cleanup, session close, and trace-end order. +5. Confirm sensitive text and audio are independently omitted from trace payloads. + +## Sources + +- `docs/voice/pipeline.md` +- `docs/voice/tracing.md` +- `src/agents/voice/pipeline.py` +- `src/agents/voice/result.py` +- `src/agents/voice/input.py` +- `src/agents/voice/model.py` +- `src/agents/voice/models/openai_stt.py` +- `tests/voice/test_pipeline.py` +- `tests/voice/test_input.py` +- `tests/voice/test_openai_stt.py` +- `tests/voice/test_openai_tts.py` diff --git a/.agents/skills/maintainer-review/SKILL.md b/.agents/skills/maintainer-review/SKILL.md new file mode 100644 index 0000000000..b81cdbff99 --- /dev/null +++ b/.agents/skills/maintainer-review/SKILL.md @@ -0,0 +1,211 @@ +--- +name: maintainer-review +description: Review a GitHub issue or pull request URL as an openai-agents-python maintainer, with a staged assessment of whether the claim is real, practically important, already solvable with supported functionality, correctly scoped, better served by another design, and worth maintainer and contributor effort. Use when assessing issue validity or severity, deciding whether an issue should be prioritized or closed, determining whether a requested feature represents an unmet need rather than a discoverability or usage gap, judging whether a PR is worth bringing to mergeable quality, comparing open PRs or alternative designs, separating code quality from repository readiness, or drafting a concise maintainer assessment. When closure, additional evidence, or code changes should be requested, also produce a polite, concise, complete, copy-paste-ready maintainer comment. +--- + +# Maintainer Review + +## Objective + +Make a maintainer decision, not a generic code-review summary. Separate these questions: + +1. Is the claimed behavior real? +2. What user outcome or constraint exists independently of the reporter's proposed API or fix? +3. Can supported functionality already achieve that outcome with reasonable composition or configuration? +4. If a gap remains, is the proposed solution the best design and implementation layer? +5. Can normal users plausibly reach the gap, and what happens when they do? +6. Is it important enough to act on now? +7. If this PR did not already exist, would maintainers choose to open and implement the same work? +8. For a PR, is this solution worth merging and maintaining? +9. Can overlapping or stale operations corrupt shared state or clean up resources owned by surviving work? +10. If competing PRs exist, which single implementation path should maintainers pursue? +11. Which ambiguous scope or semantic choices are maintainer-owned product/API decisions, and what concrete direction should the contributor implement? +12. What concise maintainer message should communicate a closure or change request clearly and politely? + +Treat an issue's requested field, callback, flag, class, or implementation strategy as a proposed mechanism, not as the accepted requirement. Do not begin by asking how to implement it. First prove that a concrete user outcome is not already supported and that the proposed mechanism is better than the available alternatives. + +Lead with the current review state. Use `Preliminary assessment` while runtime approval or evidence is pending, and `Maintainer decision` only when the review can be concluded. Use the diff, issue narrative, or contributor effort as evidence, not as a proxy for impact. + +## Workflow + +### 1. Establish the exact target + +- Accept a GitHub issue or PR URL as the primary input. Resolve its owner, repository, item type, and number before reviewing it. +- For an issue, read the full report, comments, reproduction, environment, linked material, and maintainer responses. +- For a PR, inspect the current remote base and head, full patch, commit history when relevant, tests, linked issue, and review discussion. Do not substitute the current local checkout for the remote change under review. +- State the claim in one falsifiable sentence. Distinguish the reported symptom from the reporter's proposed cause or fix. +- Identify the released behavior boundary when compatibility or regression claims matter. +- Verify whether linked evidence matches the PR's exact runtime variant, provider or tool type, triggering condition, and user outcome. A generic issue title, conceptual similarity, or wording such as `Related to` does not transfer evidence of need to an adjacent extension. If the reported scenario has already been fixed, treat additional variants as new needs requiring their own evidence. + +Respect repository instructions for remote access and mutation. A review does not authorize comments, labels, branch changes, pushes, or other remote writes. + +### 2. Establish the unmet need and challenge the proposed solution + +Complete this pass before deeply evaluating a proposed implementation and before any positive issue or PR assessment. + +First assign one `Need evidence` status: + +- **Demonstrated**: The exact scope has a concrete supported scenario, a real-path reproduction, a released compatibility requirement, repeated demand, or a broad invariant with a meaningful consequence. +- **Plausible but unproven**: The path can exist, but realistic provider behavior, user reach, frequency, consequence, or demand is not established. +- **Already covered**: A reasonable supported workflow already satisfies the outcome. +- **Unsupported**: The outcome belongs outside the SDK contract or at a provider, adapter, or caller-owned layer. + +Only `Demonstrated` need may receive `Merge-worthy as-is` or `Merge-worthy after focused changes`. For `Plausible but unproven`, prefer `Needs evidence` or `Not worth completing`; for `Already covered` or `Unsupported`, prefer closure or the relevant simpler alternative. + +1. Restate the desired user outcome without naming the requested API, class, file, option, or implementation. Separate the actual constraint from the reporter's preferred mechanism. +2. Trace the closest supported ways to achieve that outcome in the current release and current target. Inspect the owning code path, public API, tests, and relevant docs rather than assuming that an unfamiliar capability is missing. Consider configuration, composition, cloning, callbacks, extension points, provider adapters, and doing the work at a caller-owned layer. +3. Determine whether the report shows a capability gap, an ergonomics or discoverability problem, an unsupported use case, or no demonstrated problem. A more convenient spelling is not automatically a missing capability. +4. Compare the proposed solution against the strongest existing approach and at least one better-design candidate: no code change, clearer documentation or validation, a narrower fix, reuse of an existing abstraction, or enforcement at a more coherent shared boundary. +5. For each viable approach, compare whether it satisfies the concrete scenario, what new public or internal contract it creates, cross-path consistency, compatibility, and permanent maintenance cost. + +Do not treat a test proving that new code can work as evidence that the feature is needed. A `FakeModel` response, manually constructed provider item, mock, or new regression test can establish code-path reachability and implementation correctness; it does not by itself establish realistic provider behavior, user reach, frequency, practical consequence, or demand. + +API symmetry, naming consistency, and parity with an adjacent tool, provider, or output type are design arguments, not evidence of need. Parity may justify work when it removes existing complexity or enforces a broad demonstrated invariant, but adding branches, tests, documentation, or public behavior requires independent practical justification. + +If the need is not `Demonstrated`, inspect the patch only far enough to understand its contract, risk, and maintenance cost. Do not turn implementation defects, missing tests, or documentation gaps into a request-changes recommendation, because those questions become merge-blocking only after the need gate passes. If the report provides no concrete scenario, the existing functionality appears sufficient, or the requested mechanism solves only a hypothetical convenience problem, prefer `Needs evidence`, `Close`, `Supersede with a simpler alternative`, or `Not worth completing` over designing the requested feature on the reporter's behalf. + +### 3. Discover competing open PRs proportionally + +Do this before deeply evaluating a specified PR. A PR URL selects the starting point, not necessarily the entire comparison set. + +- Determine the primary issue from explicit closing keywords, linked issues, issue timeline or development links, PR body and comments, and the reproduced symptom. If the association is inferred rather than explicit, state the evidence. +- When an issue is explicitly linked, enumerate all open PRs that address it through the issue timeline, development links, cross-references, closing keywords, and ordinary references. Include draft PRs but label them as drafts. +- When no issue is linked, run a bounded duplicate search using the strongest two or three signals from the title, reproduction, violated invariant, and runtime path. Stop when additional queries are unlikely to produce a credible competing implementation. +- Exclude closed or merged PRs from the active comparison set, while using them as history when relevant. +- Do not group PRs merely because they mention the same subsystem. Require a shared issue, symptom, violated invariant, or materially overlapping fix. +- Record the search methods and candidate set internally. If repository access cannot establish completeness, say so instead of claiming that every open PR was found. Do not list unrelated search hits in the final report. + +When multiple candidates exist, compare them on need coverage, runtime correctness, scope, implementation layer, tests, compatibility, complexity, readiness, remaining maintainer work, and whether useful parts can be combined. Prefer the best maintainable solution, not the first submission or the smallest diff by default. + +### 4. Use a two-stage evidence flow + +Always begin with a desk review. Inspect the concrete runtime path before judging a small change as either trivial or meaningful. Check callers, adjacent helpers, validation layers, fallback paths, and existing tests. Search history or documentation only when it changes the decision. Inspecting test code is part of the desk review; executing tests, imports, examples, reproductions, benchmarks, or service calls is a runtime probe. + +For repository-specific runtime invariants, start with `.agents/references/README.md` and open only the references that match the affected boundary. Treat `.agents/references/` as read-only during issue and PR review: use it to identify expected invariants, adjacent surfaces, and regression risks, then verify the current claim against the remote change, current code, tests, docs, release boundary, and focused runtime evidence. Do not edit references as a side effect of the review, infer current issue or PR status from them, or treat old issue or PR outcomes as current evidence. If the review reveals a reusable invariant that should be captured, recommend a separate repository-maintenance update unless the user explicitly asks to update references in the same task. + +Use this evidence order across the two stages: + +1. Trace the closest existing supported capabilities and determine whether they already satisfy the underlying user outcome. +2. Inspect existing tests and complete the code-path trace, including the mandatory interleaving and ownership pass when triggered, without executing code. +3. With explicit user approval, run a focused local reproduction of the exact claim when the desk-review rules below require it. +4. A comparison with the released version, base branch, or known-good control. +5. A broader runtime matrix only when the maintainer decision remains uncertain and the user approves it. + +#### Stage 1: desk review + +Produce an initial result from static evidence before running code: + +##### Mandatory unmet-need and design pass + +Before a positive assessment, complete the pass in step 2 and be able to state all of the following from concrete evidence: + +1. The user outcome that current supported behavior cannot achieve. +2. The closest existing API or composition path and the exact reason it is insufficient. +3. Why the proposed behavior belongs at the chosen abstraction layer instead of a caller, adapter, validation, documentation, or existing extension point. +4. Why the proposed permanent contract is better than no code change and the strongest narrower alternative. +5. What real scenario, compatibility requirement, or repeated demand justifies the new maintenance surface. +6. Whether maintainers would choose to pursue the same work if no contributor had already supplied a patch. + +If any answer is missing and could change whether code should exist at all, do not call the issue actionable or the PR merge-worthy. Request only the evidence needed to distinguish a genuine capability gap from a usage, discoverability, or solution-design problem. This is a product and architecture evidence gap, not a runtime-probe trigger by itself. + +##### Mandatory interleaving and ownership pass + +Run this pass before any positive PR assessment when a patch adds, removes, or reorders cleanup, retry, reconnect, cancellation, listeners, shared futures or tasks, connections or streams, state flags, or mutable state across an `await`, callback, event, or deferred completion. + +1. Name each shared resource or state value and the operation that owns it. Include listeners, futures, tasks, connections, streams, locks, caches, state flags, persistence, and telemetry. +2. Trace at least two overlapping operations, `A` and `B`, across every suspension or re-entry point. Check `A pending -> B starts -> A fails -> B succeeds`, `A pending -> B starts -> B fails -> A succeeds`, close or cancellation between setup and completion, and a stale completion arriving after newer work. +3. For every cleanup or rollback, identify the exact attempt and resource generation it is allowed to dispose. Treat unconditional cleanup after a suspension point as a regression candidate until the code proves it cannot tear down newer or surviving work. +4. Compare base and head for the survivor invariant. Replacing duplicated work with missing handlers, a closed shared resource, reverted state, or a failed surviving task is a regression, not successful cleanup. +5. Inspect tests for controlled interleavings using deferred futures, callbacks, or events. Require assertions about the surviving operation's observable behavior and final resource state, not only listener counts or individual exception results. + +Do not mark a concurrency-sensitive patch `Merge-worthy as-is` merely because sequential reconnect, retry, failure, and close tests pass. If the code trace proves an unsafe interleaving, conclude from static evidence and request a focused fix and regression test. If ownership remains ambiguous, keep the result preliminary and request approval for the smallest decisive runtime probe. + +- If the claim or PR is decisively negative from a complete reachable code-path trace, conclude the review without a runtime probe. Examples include an impossible or unsupported path, duplicated existing handling, a demonstrated no-op, a direct compatibility break, or a clearly wrong abstraction. Do not call an ambiguous result negative merely to avoid a probe. +- If the initial result is positive and there is no unresolved runtime concern, and any triggered interleaving and ownership pass is complete, the desk review may be sufficient for a final maintainer decision. Do not run a probe only to restate evidence that cannot plausibly change the decision. +- If the initial result is positive but there is any unresolved runtime concern that could plausibly change claim validity, severity, merge-worthiness, required changes, or the preferred competing PR, stop before executing code. Report a `Preliminary assessment`, name the concern, propose the smallest decisive probe and control, and ask the user for approval to run it. +- A purely stylistic, documentation, CI-status, or repository-readiness concern does not trigger a runtime probe unless it masks a runtime question. + +Do not issue a definitive positive maintainer decision while a decision-relevant runtime concern remains unresolved. If the user declines the probe, keep the result preliminary and state the exact confidence limitation. + +#### Stage 2: approved runtime probe + +After explicit approval, run only the smallest probe needed to resolve the stated concern. Exercise the real public or internal path and include a base, release, or known-good control when relevant. Do not stop at a happy-path smoke check when failure behavior determines the decision. Return to the user for separate approval before expanding materially beyond the approved probe. + +For latency, timeout, buffering, backpressure, or cleanup claims, measure at least one observable elapsed-time or state-transition path when feasible. Do not assume that a mocked unit test exercises real scheduling or provider behavior. Prefer a local probe first; use an approval-gated live-service probe only when local evidence cannot settle the decision. + +Use `$runtime-behavior-probe` only when the user explicitly invokes it and the skill is available, or when the user explicitly approves using it for the proposed runtime work. Preserve its environment-variable approval, live-service, cost, cleanup, and reporting gates. Do not make ordinary maintainer review depend on that skill being available. + +For changes involving validation, fail-fast behavior, cleanup, retries, interruption, or concurrency, trace lifecycle ordering in addition to the main behavior: + +- Identify listeners, tasks, connections, files, locks, state mutations, and other resources acquired before the new check or failure point. +- Verify cleanup when construction, context-manager entry, validation, connection, or execution raises before normal teardown runs. +- Require a negative-path test when a failure can leave observable state or resources behind. + +Do not over-investigate. Stop when additional evidence is unlikely to change validity, severity, or the maintainer recommendation. + +### 5. Calibrate validity and impact + +Use `references/evaluation-framework.md` to assess claim validity, realistic reach, consequence, breadth, frequency, recoverability, compatibility, and severity. Keep observed facts separate from inference and state any missing evidence that could change the decision. + +Report the `Need evidence` status before classifying the need as a capability gap, ergonomics or discoverability gap, unsupported use case, or no demonstrated gap. Do not assign practical impact to the absence of the requested mechanism when an existing supported workflow already produces the requested outcome. Do not infer practical importance merely from reachability, API asymmetry, or a technically successful patch. + +For a PR, make `Severity` describe the underlying issue or user need only. Do not combine it with the risk created by the proposed patch. Report a meaningful patch-induced regression, compatibility, lifecycle, or maintenance risk separately as `Patch risk`. + +Do not infer that a report is low-value merely because an AI may have found or written it. Do not speculate about authorship or motive. Identify contribution-shaped reports through objective signals: no reproducible behavior, unrealistic inputs, an impossible call path, duplicated existing handling, tests that do not exercise the claim, or a fix whose runtime result is a no-op. + +### 6. Apply the maintainer-effort test + +Use the framework's issue dispositions and PR checks to decide whether the outcome justifies permanent code, tests, documentation, and maintainer attention. Classify code quality separately from repository readiness. + +Use one code recommendation: + +- **Merge-worthy as-is**: real need, sound implementation, proportionate scope, adequate tests. +- **Merge-worthy after focused changes**: real need and viable direction, with bounded corrections. +- **Supersede with a simpler alternative**: real need, but a smaller or more coherent fix is preferable. +- **Not worth completing**: negligible or unsupported impact, no-op behavior, wrong abstraction, or excessive completion cost. + +`Merge-worthy as-is` and `Merge-worthy after focused changes` are invalid unless `Need evidence` is `Demonstrated`. A bounded set of implementation fixes cannot promote a `Plausible but unproven` need into a merge-worthy recommendation. + +For `Merge-worthy as-is` and `Merge-worthy after focused changes`, use one repository-readiness status when it helps communicate the integration state: + +- **Ready**: current head is reviewable and required checks are green. +- **CI or review pending**: code recommendation is stable, but required external gates are incomplete. +- **Rebase or conflict resolution required**: the head cannot merge cleanly or is materially stale. +- **Blocked**: a concrete external or repository condition prevents a reliable merge decision. + +Omit repository readiness for `Supersede with a simpler alternative` and `Not worth completing`; CI, review, mergeability, or branch freshness does not change those dispositions. Put any validation limitation that materially affects confidence in the evidence instead. When readiness is included, use exactly one of the four statuses above and do not invent variants such as `ready mechanically` or use rebase status for semantic staleness. + +Do not downgrade an otherwise sound code recommendation solely because CI is pending. Do not call a PR ready when semantic conflict resolution or material code changes remain. + +When multiple open PRs address the same issue, make one portfolio-level recommendation: select the strongest PR, request focused changes in one candidate, combine specific ideas into one PR, supersede all candidates with a simpler approach, or close duplicates. Explain why the recommended path is better than each alternative without turning the report into line-by-line review. + +Always compare the proposed patch with the strongest existing supported approach and at least one alternative: no code change, validation or documentation, a narrower fix, reuse of an existing helper, or a different layer that enforces the invariant consistently. A review is incomplete if it establishes only that the patch works without establishing why the current product cannot meet the underlying need and why this design is preferable. + +When multiple plausible semantic scopes, compatibility boundaries, or public API contracts remain, do not ask the contributor to choose among maintainer-owned options. Decide the preferred scope from the evidence, compatibility contract, and product/API design principles, then request that specific change. If the evidence is insufficient to choose, mark the review preliminary or request maintainer input; do not present an open-ended implementation fork as the contributor's decision. + +### 7. Report findings and maintainer action + +Choose the assessment language using this precedence: + +1. Follow an explicit language request in the current conversation. +2. Follow an applicable language instruction from `~/.codex/AGENTS.md`, the repository's `AGENTS.md`, or another governing instruction file. +3. If recent conversation turns are consistently in one language, use that language. +4. Otherwise, default to English. + +Do not infer the assessment language from the GitHub URL, contributor, code, or browser locale. Maintainer comment drafts remain English regardless of the assessment language. Keep the report decision-oriented and compact. Use no more than five evidence bullets by default; add more only when the decision genuinely depends on them. + +Use the matching compact report variant in `references/evaluation-framework.md`. While runtime approval is pending, use its preliminary-assessment variant and end with the approval request instead of presenting a final recommendation. Collapse sections for simple cases rather than padding the answer. Put unexpected or negative runtime findings first, and name the preferred PR or approach explicitly when candidates compete. + +For PRs, put `Need evidence` before code recommendation. When the need is not `Demonstrated`, lead with that result, omit repository readiness, and avoid presenting patch fixes as the primary maintainer action. + +When existing functionality or a better alternative materially affects the decision, state it explicitly in the evidence and recommendation. Name the exact supported path, what it does and does not cover, and why it is preferable. Do not bury a `Not worth completing` or `Supersede with a simpler alternative` conclusion beneath praise for implementation quality. + +When recommending closure, requesting more evidence, requesting code changes, or superseding a PR, append the English, copy-paste-ready maintainer comment defined by the framework. If multiple PRs need different actions, label one draft for each affected PR. Include only merge-blocking requests in the main action paragraph; keep optional documentation or polish clearly non-blocking or omit it. + +For request-changes comments, phrase maintainer-owned semantic decisions as a directive, not as a menu. It is fine to mention the rejected alternative briefly in the rationale, but the requested action must identify the chosen behavior, scope, or compatibility boundary. Use "please do X because..." instead of "either do X or Y" when X versus Y changes the SDK contract or user-visible semantics. + +Do not produce a line-by-line review unless requested. Do not equate passing tests with merge-worthiness, or a logically correct patch with practical value. + +## Resource + +- `references/evaluation-framework.md` contains the severity rubric, evidence checks, lifecycle review, issue dispositions, PR quality checks, maintainer-comment guidance, and report variants. diff --git a/.agents/skills/maintainer-review/agents/openai.yaml b/.agents/skills/maintainer-review/agents/openai.yaml new file mode 100644 index 0000000000..549b0a989e --- /dev/null +++ b/.agents/skills/maintainer-review/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Maintainer Review" + short_description: "Gate PR value on demonstrated user need" + default_prompt: "Use $maintainer-review with this GitHub issue or PR URL. Before evaluating implementation quality, verify that linked evidence matches the exact runtime variant and assign Need evidence as Demonstrated, Plausible but unproven, Already covered, or Unsupported. Only a Demonstrated need may receive a merge-worthy recommendation; synthetic tests, API parity, and contributor effort do not establish need. Then compare existing and alternative approaches, complete the desk review and required lifecycle ownership checks, request approval before any decision-relevant runtime probe, compare credible competing PRs, recommend the best maintainer action, and include an English comment draft when closure or changes are needed." diff --git a/.agents/skills/maintainer-review/references/evaluation-framework.md b/.agents/skills/maintainer-review/references/evaluation-framework.md new file mode 100644 index 0000000000..9738a5aaad --- /dev/null +++ b/.agents/skills/maintainer-review/references/evaluation-framework.md @@ -0,0 +1,366 @@ +# Maintainer Evaluation Framework + +Use this reference when a claim is ambiguous, severity is disputed, or a PR is technically correct but may not justify merge effort. + +## Contents + +- [Decision model](#decision-model) +- [Severity rubric](#severity-rubric) +- [Evidence-strength checks](#evidence-strength-checks) +- [Unmet need and alternative design gate](#unmet-need-and-alternative-design-gate) +- [Issue disposition](#issue-disposition) +- [PR quality and value](#pr-quality-and-value) +- [Documentation threshold](#documentation-threshold) +- [Lifecycle and failure-path review](#lifecycle-and-failure-path-review) +- [Concurrency and cleanup ownership](#concurrency-and-cleanup-ownership) +- [Better-alternative prompts](#better-alternative-prompts) +- [Competing PR comparison](#competing-pr-comparison) +- [Maintainer comment drafts](#maintainer-comment-drafts) +- [Compact report variants](#compact-report-variants) + +## Decision model + +Treat validity, severity, and merge-worthiness as separate results. Also distinguish a `Preliminary assessment`, which may still require approved runtime evidence, from a final `Maintainer decision`. Do not label a provisional positive result as a verdict or final decision. + +| Dimension | Questions | Strong evidence | +|---|---|---| +| Claim validity | Does the exact reported behavior occur? Is the proposed cause correct? | Reproduction, failing focused test, or complete reachable code path | +| Reachability | Can supported, realistic inputs reach it? | Public API trace, real configuration, linked user report, or release comparison | +| Consequence | What fails, and is the result silent or recoverable? | Observed output/error/state plus downstream effect | +| Breadth | Who is affected? | Supported providers, platforms, versions, and configurations identified precisely | +| Frequency | Is this normal, intermittent, or pathological? | Repeat runs, telemetry or reports when available, deterministic preconditions | +| Need evidence | Is the exact scope demonstrated, merely plausible, already covered, or unsupported? | Same-scope user scenario, real-path reproduction, released compatibility requirement, repeated demand, or broad consequential invariant | +| Unmet need | What user outcome cannot be achieved through supported behavior today? | Concrete scenario plus a trace showing why the closest existing path is insufficient | +| Existing capability | Can configuration, composition, cloning, callbacks, extension points, or a caller-owned layer already satisfy the outcome? | Current release code, tests, docs, and an exact supported workflow | +| Compatibility | Is released behavior or durable state changed? | Latest release comparison and explicit contract inspection | +| Solution fit | Is the requested mechanism the best design and implementation layer? | Proposed solution compared with the strongest existing path and at least one narrower or more coherent alternative | +| Maintainer-owned scope | When several plausible semantics remain, which behavior should the SDK own? | A concrete maintainer decision grounded in compatibility, user outcome, and API design, not an open-ended contributor choice | +| Resource ownership | Can stale, failed, cancelled, or overlapping work mutate or clean up resources owned by surviving work? | Interleaving trace, attempt or generation ownership, and survivor assertions | +| Maintenance cost | What permanent complexity and review burden does it add? | Changed surface, new branches/configuration, test burden, remaining work | + +## Severity rubric + +- **Negligible**: No runtime difference, unreachable or unsupported input, cosmetic inconsistency, or a fully harmless edge case. Usually close, document, or decline code complexity. +- **Low**: Real but narrow and recoverable behavior with a simple workaround and no data, security, or compatibility risk. Merge only when the fix is small and clearly improves an invariant. +- **Moderate**: Plausible supported use fails or produces incorrect behavior for a meaningful subset of users. Prioritize a bounded fix and regression test. +- **High**: Common or important supported use is broken, causes serious compatibility problems, leaks sensitive data, or risks persistent corruption. Treat as urgent and require strong validation. +- **Critical**: Broadly exploitable security impact, severe data loss, or systemic failure requiring immediate coordinated action. Use only with concrete evidence. + +Severity is approximately consequence multiplied by realistic reach and frequency, reduced by recoverability. Do not raise severity because a report sounds alarming or lower it because a patch is small. + +## Evidence-strength checks + +Before calling a claim confirmed, answer: + +- Does the reproduction exercise the same public or internal path named in the report? +- Does the failure still occur on the relevant base, release, or current target? +- Does the test fail without the patch and pass with it? +- Are setup failures, stale builds, environment leakage, proxies, caches, or unsupported options excluded? +- Does an adjacent helper or equivalent path follow different semantics? +- Is the observed behavior prohibited by an actual contract, or merely surprising? +- For latency, timeout, buffering, backpressure, or cleanup claims, was observable elapsed time or a real state transition measured when feasible rather than inferred only from mocks? +- For shared asynchronous state, do tests control completion order and prove that stale failure or cleanup cannot affect the surviving operation? + +Use `partially confirmed` when the symptom is real but the cause, reach, or claimed scope is wrong. Use `unproven` when decisive evidence is missing. Use `contradicted` only when evidence directly disproves the claim. + +## Unmet need and alternative design gate + +Issue reports often combine a desired outcome with a proposed API or implementation. Treat the proposed mechanism as a hypothesis. Confirm the unmet outcome before evaluating how well the patch implements that mechanism. + +### Linked-evidence scope + +Evidence from a linked issue applies only when the issue and PR share the same runtime variant, provider or tool type, trigger, supported configuration, and user outcome. A broad title, ordinary reference, `Related to` statement, or conceptual similarity is not enough. If an earlier change already resolved the concrete reported scenario, an adjacent extension starts with no inherited evidence of need. + +### Need evidence status + +Assign one status before deep implementation review: + +- **Demonstrated**: The exact scope has a concrete supported scenario, real-path reproduction, released compatibility requirement, repeated demand, or broad invariant with meaningful consequence. +- **Plausible but unproven**: The code path is possible, but realistic reach, frequency, consequence, provider behavior, or demand is missing. +- **Already covered**: A reasonable supported workflow already satisfies the outcome. +- **Unsupported**: The outcome is outside the SDK contract or belongs at a provider, adapter, or caller-owned layer. + +Only `Demonstrated` need can support a merge-worthy code recommendation. `Plausible but unproven` maps to `Needs evidence` or `Not worth completing`, even when the patch is technically correct and its remaining fixes are bounded. `Already covered` and `Unsupported` normally map to closure or a simpler non-core alternative. + +Before accepting an issue or recommending a PR, record: + +| Question | Required evidence | +|---|---| +| What outcome is needed? | A concrete supported scenario stated without the proposed API or fix | +| What exists today? | The closest current-release API, configuration, composition, extension point, or caller-owned solution | +| Why is it insufficient? | An exact behavioral, compatibility, lifecycle, or operational constraint, not preference alone | +| What are the alternatives? | The proposed patch, the strongest existing path, and at least one no-code, narrower, or better-layer design | +| Why add a contract? | Practical benefit sufficient to justify public surface, runtime branches, cross-path tests, documentation, and long-term maintenance | + +Classify the result: + +- **Capability gap**: a supported, realistic outcome cannot be achieved with current functionality. Code may be warranted. +- **Ergonomics or discoverability gap**: the outcome is already possible, but the supported route is confusing or unnecessarily difficult. Prefer documentation, validation, or a narrowly justified convenience improvement. +- **Unsupported use case**: the desired outcome lies outside the SDK contract or belongs at a provider, adapter, application, or other caller-owned layer. Do not expand the core API merely to make it possible. +- **No demonstrated gap**: no concrete scenario proves that existing functionality is insufficient. Request evidence or close rather than designing from the proposed mechanism. + +Passing tests for a new implementation establish feasibility and correctness, not need. A `FakeModel` response, manually constructed provider item, mock, or synthetic fixture does not establish realistic provider behavior, user reach, frequency, consequence, or demand. API symmetry and parity with an adjacent runtime are design arguments, not need evidence. A technically coherent patch can still be `Not worth completing` when the motivating scenario is hypothetical, already supported, or better solved elsewhere. + +Use the counterfactual maintainer test: if the PR did not already exist, would maintainers choose to file and implement the same work from the available evidence? Contributor effort lowers implementation cost but does not create product need or remove permanent maintenance cost. + +When the need is not `Demonstrated`, inspect implementation only far enough to estimate contract, risk, and maintenance cost. Do not convert patch defects, missing tests, or documentation gaps into a request-changes disposition; those become merge blockers only after the need gate passes. + +## Issue disposition + +Choose one primary action: + +- **Prioritize**: confirmed moderate-or-higher impact or an important invariant with no safe workaround. +- **Accept, low priority**: confirmed low impact, existing supported functionality is insufficient for the demonstrated scenario, and a proportionate fix appears possible. +- **Narrow scope**: a valid core exists, but the report overstates affected paths or expected behavior. +- **Needs evidence**: plausible claim, but no minimal reproduction, supported setup, contract basis, or concrete scenario showing why existing functionality is insufficient. +- **Close**: duplicate, unsupported, unreachable, contradicted, no-op, already addressed by a reasonable supported path, or not worth permanent complexity. + +When requesting evidence, ask only for information that could change the disposition. + +## PR quality and value + +Assess these independently: + +1. **Need**: Same-scope issue or runtime evidence demonstrates a concrete unmet user outcome that the closest supported capability cannot reasonably satisfy. Do not inherit evidence from an adjacent variant or already-fixed scenario. +2. **Correctness**: The fix works for the reported case and meaningful boundaries. +3. **Placement**: The invariant is enforced once at the right layer instead of duplicating existing functionality, patching locally, or moving caller- or provider-owned policy into the core SDK. +4. **Consistency**: Equivalent sync/async, streaming/non-streaming, provider, serialization, and resume paths remain aligned where applicable. +5. **Tests**: A regression test fails on the base, passes on the head, and tests the exact non-happy-path value or state. When shared state crosses an asynchronous boundary, tests control relevant completion orders and assert the surviving operation's behavior and final resource state. +6. **Compatibility**: Released positional APIs, wire formats, persisted schemas, and established error behavior are preserved or intentionally migrated. +7. **Proportionality**: Complexity and public surface are justified by impact. +8. **Completion cost**: Remaining fixes, docs, tests, and design work are bounded enough to justify maintainer attention. + +A PR can be correct but not merge-worthy. Typical reasons include a nonexistent or negligible need, an outcome already supported through a reasonable existing mechanism, a no-op on the actual runtime path, incomplete cross-path semantics, an abstraction cost larger than the benefit, or a simpler design at another layer. + +Do not use implementation correctness, bounded remaining work, CI status, or contributor effort to upgrade a need that is only `Plausible but unproven`. Merge-worthiness is gated by demonstrated need, not by how close the patch is to completion. + +Keep issue impact and patch risk separate. `Severity` describes the underlying issue or user need. A regression, compatibility break, lifecycle leak, or maintenance hazard introduced by the proposed patch belongs under `Patch risk` and must not inflate or obscure the issue severity. + +When a PR exposes an ambiguous semantic boundary, decide whether that boundary belongs to maintainers before drafting requests. If the choice affects SDK contract, compatibility, persistence, error semantics, public API meaning, or cross-path behavior, the review should pick one direction or explicitly block on maintainer input. Do not delegate that choice to the contributor as "either X or Y"; ask for the chosen behavior and the tests or docs needed to lock it down. + +## Documentation threshold + +Do not treat documentation as automatically required for every public option, constructor parameter, provider setting, or behavior change. Make docs merge-blocking only when at least one of these is true: + +- Existing user-facing docs become materially false, unsafe, or misleading. +- Correct or safe use depends on a non-obvious constraint, migration step, compatibility boundary, or operational warning. +- Repository policy, the accepted issue scope, or an explicit maintainer decision requires documentation in the same PR. +- The intended feature would be practically unusable or undiscoverable by its target users without a documented entry point, and generated API reference or clear code-level discovery is insufficient. + +If docs would merely improve discoverability or completeness, keep them non-blocking. Do not change `Merge-worthy as-is` to `Merge-worthy after focused changes` solely for optional docs, and do not include optional docs in the maintainer comment's required-action paragraph. Respect an explicit maintainer choice to omit docs or defer them to a separate follow-up. + +## Lifecycle and failure-path review + +Apply this section when a change adds validation, fail-fast behavior, cleanup, retries, interruption, background work, or concurrency. + +- Identify the earliest point where all dynamic inputs needed for a correct decision are available. +- List side effects before and after that point: listeners, tasks, connections, files, locks, caches, state mutations, and telemetry. +- Exercise failure during construction, context-manager entry, validation, connection, and execution when those phases exist. +- Confirm that normal teardown is actually entered. If an enter or constructor fails, verify cleanup explicitly rather than assuming an exit hook runs. +- Prefer validation after dynamic configuration is resolved but before avoidable side effects begin. +- Require a regression test for any listener, task, connection, or state that could remain after failure. + +## Concurrency and cleanup ownership + +Apply this section before a positive assessment whenever lifecycle work crosses an `await`, callback, event, deferred completion, retry, reconnect, cancellation, or shared resource boundary. Sequential correctness is insufficient because a patch can improve isolated cleanup while introducing cross-attempt teardown. + +Use a two-operation interleaving matrix during desk review: + +| Ordering | Required question | +|---|---| +| `A pending -> B starts -> A fails -> B succeeds` | Can A's cleanup remove or revert anything B needs? | +| `A pending -> B starts -> B fails -> A succeeds` | Can B's cleanup leave A successful but non-functional? | +| `A succeeds -> B starts -> stale A completion` | Can stale A overwrite B's newer state or generation? | +| setup -> close/cancel -> late completion | Can late work resurrect listeners, state, tasks, or connections after teardown? | + +For each ordering: + +- Identify the resource owner before and after every suspension point. +- Distinguish per-attempt resources from shared runner, session, transport, cache, or listener state. +- Require cleanup to carry an ownership token, generation, identity check, serialization guarantee, or another invariant that prevents cross-attempt disposal. +- Compare base and head on the survivor invariant. Fewer duplicates do not justify losing the only active handler, connection, task, or state update. +- Require a controlled interleaving test when the ordering is reachable. The test must assert both the failing operation and the surviving operation's observable behavior after all completions settle. + +An unscoped `finally`, `except`, close handler, cancellation callback, or rollback that mutates shared state after a suspension point is merge-blocking when another operation can still own or use that state. + +## Better-alternative prompts + +Start with the strongest existing supported path, then test at least one additional alternative against the proposed patch. Do not complete a positive review without this comparison. + +- Can the requested outcome already be achieved through configuration, composition, cloning, callbacks, extension points, a custom provider or adapter, or caller-owned code? +- If the existing route is awkward, is the problem discoverability or ergonomics rather than missing capability? +- What happens if maintainers make no code change? +- Can input validation or an existing helper enforce the invariant earlier? +- Can the fix be limited to the one supported path that fails? +- Would documentation or a clearer error prevent misuse without runtime complexity? +- Can the test be added first to reveal the smallest correct change? +- Is the proposed public option compensating for an internal design issue? +- Is the proposed core behavior actually provider- or application-specific policy that belongs at another layer? + +## Competing PR comparison + +When two or more open PRs address the same issue, first verify that they belong in one comparison set. Accept an explicit issue link, the same minimal reproduction, the same violated invariant, or materially overlapping runtime paths as association evidence. Do not treat a shared label or subsystem as sufficient. + +Compare each candidate on the same evidence basis: + +| Criterion | Question | +|---|---| +| Need | Does a concrete user outcome remain unmet after tracing existing supported functionality? | +| Existing capability | Could every candidate be avoided by configuration, composition, an extension point, or a better caller- or provider-owned solution? | +| Coverage | Does it solve the whole confirmed issue, a useful subset, or an adjacent problem? | +| Correctness | Does the fix work on the real path and meaningful boundaries? | +| Placement | Does it enforce the invariant at the correct shared layer? | +| Tests | Does it reproduce the base failure and distinguish the candidate approaches? | +| Compatibility | Does it preserve released APIs, state, protocol, providers, and established behavior? | +| Complexity | What permanent branches, abstractions, configuration, or coupling does it add? | +| Readiness | Is it mergeable now, or how much focused work remains? | +| Reuse | Are there valuable tests or implementation pieces that should be combined into another candidate? | + +Choose one portfolio-level disposition: + +- **Prefer one PR**: identify the strongest candidate and close or supersede duplicates. +- **Prefer one after focused changes**: keep one candidate active and state bounded changes required before merge. +- **Combine selectively**: identify the destination PR and the exact ideas or tests worth transferring; avoid asking maintainers to reconcile entire competing implementations. +- **Replace all**: explain the simpler or more coherent implementation that should supersede every candidate. +- **Merge none**: the issue is invalid, negligible, unsupported, or none of the approaches justify completion cost. + +Do not split the decision into independent approvals. Competing PRs consume overlapping review and maintenance budgets, so recommend one path for the issue as a whole. + +## Maintainer comment drafts + +Always write maintainer comments in English, regardless of the assessment language. Produce a draft when the recommendation is to close, request evidence, request focused code changes, supersede a PR, or choose one competing PR over another. + +Keep each draft polite, direct, and copy-paste-ready. Usually use 60-160 words in one to three short paragraphs: + +1. Acknowledge the contribution or report. +2. Explain the decision with the smallest amount of decisive technical evidence. +3. Give the exact next action or the condition for reconsideration. + +Do not include internal labels such as `severity: low`, speculate about AI authorship or contributor intent, repeat the full review, or soften the message until the requested action becomes unclear. + +Do not ask contributors to choose maintainer-owned semantics. If two implementations are technically possible but one changes the SDK contract, decide the contract in the review and make the comment actionable. Use a short rationale such as "This keeps the new handler scoped to the existing raise site" or "This makes the handler name match all invalid final messages", then request the exact code and tests for that decision. + +### Close + +```text +Thanks for taking the time to investigate this. I traced the reported case through , and . In the supported path, , so the added complexity is not justified by the demonstrated impact. + +I am going to close this . If you can provide , we can revisit the underlying problem with that narrower scope. +``` + +### Request changes + +```text +Thanks for the contribution. The underlying issue is valid, and this approach is directionally reasonable. Before we can merge it, please address the following points: . + +These changes are needed because . Once they are covered with a regression test that fails on the base and passes on the updated branch, the PR should be ready for another review. +``` + +Adapt the wording to the actual evidence. Do not use these templates as generic filler. + +### Existing capability or better alternative + +```text +Thanks for the contribution. I traced the underlying use case through , which already supports . The proposed change adds , but the issue does not demonstrate a concrete supported case that the existing approach cannot handle. + +I am going to close this for now. If you can provide , we can revisit the unmet need and choose the narrowest appropriate design from that evidence. +``` + +## Compact report variants + +Use `Maintainer decision` for a concluded review. Use `Preliminary assessment` when a desk review is tentatively positive but a decision-relevant runtime concern remains. `Verdict` is intentionally avoided in the report headings because it does not communicate whether the result is provisional or final. + +### Runtime approval gate + +```markdown +## Preliminary assessment + + +## Static evidence +- +- + +## Proposed runtime probe +- Concern: +- Probe: +- Control: +- Scope: + +## Approval request + +``` + +### Issue + +```markdown +## Maintainer decision + + +## Evidence +- +- + +## Existing capability and alternatives + + +## Recommendation + + +## Maintainer comment draft + +``` + +### Pull request + +```markdown +## Maintainer decision + +- Need evidence: +- Code recommendation: +- Repository readiness: + +## Evidence +- +- + +## Existing capability and alternatives + + +## Issue impact +- Validity: +- Severity: +- Reach: + +## Patch risk + + +## PR quality +- Solution fit: +- Tests: +- Remaining effort: + +## Recommendation + + +## Maintainer comment draft + +``` + +### Competing pull requests + +```markdown +## Maintainer decision + + +## Open PR comparison +| PR | Approach | Correctness | Tests | Compatibility/complexity | Readiness | +|---|---|---|---|---|---| +| #... | ... | ... | ... | ... | ... | + +## Recommendation +