Skip to content

docs: correct Agent.clone list attribute semantics - #4474

Open
thegoodengineer wants to merge 2 commits into
openai:mainfrom
thegoodengineer:docs/agent-clone-list-semantics
Open

docs: correct Agent.clone list attribute semantics#4474
thegoodengineer wants to merge 2 commits into
openai:mainfrom
thegoodengineer:docs/agent-clone-list-semantics

Conversation

@thegoodengineer

Copy link
Copy Markdown

Summary

The Agent.clone() docstring added in #1296 makes two claims that the implementation does not hold, both about how list attributes are copied. It currently says:

Mutable attributes like tools and handoffs are shallow-copied: new list objects are created only if overridden, but their contents (tool functions and handoff objects) are shared with the original.

Neither half is reliable. clone() calls dataclasses.replace, which never copies a list at all, so what the clone gets is whatever the merged arguments hold:

CLAIM 1: "new list objects are created only if overridden"
  omitted                      -> cloned.tools is original.tools     True
  overridden with the SAME list -> cloned.tools is original.tools     True
  overridden with a NEW list    -> cloned.tools is the supplied list  True

CLAIM 2: "their contents are shared with the original"
  overridden with [other_tool] -> any entry shared with original      False

So agent.clone(tools=agent.tools) is an override that creates no new list, and agent.clone(tools=[other_tool]) shares no contents. Overriding is not what decides either outcome; what the caller passes is.

The distinction that does hold, and that the docstring is right to warn about, is between passing an attribute and not passing it. When you do not pass one, the clone receives the original agent's own list, so appending through either agent changes the other.

This is easy to get backwards. The reporter in #1293 asserted clone.tools is not original.tools and the test failed in review for exactly this reason. The regression test added alongside that docstring works around the behavior rather than pinning it: test_agent_clone_shallow_copy passes tools=original.tools.copy(), and its own docstring calls that "the tools.copy() workaround". Nothing in the suite covered the un-passed case, which is why the inaccuracy survived.

The equivalent JSDoc in the JS SDK was corrected in openai/openai-agents-js#1705, where automated review flagged these same two claims. This change brings the Python docstring to the same description so both SDKs document one contract.

Fix

Docstring and tests only. No behavior change and no API change.

The note now leads with the mechanism, that dataclasses.replace never copies a list attribute, and lets the cases follow from it rather than asserting an outcome per case. It names the affected attributes (tools, handoffs, mcp_servers, input_guardrails, output_guardrails), separates passing an attribute from omitting it, gives both of the counterexamples above, and keeps the existing guidance on how to get an independently owned list.

Test plan

Four tests added to tests/test_agent_clone_shallow_copy.py, each pinning one behavior the docstring now describes:

  • test_agent_clone_keeps_list_attributes_it_is_not_given
  • test_agent_clone_uses_a_given_list_as_is
  • test_agent_clone_still_shares_when_given_the_original_list
  • test_agent_clone_shared_list_mutation_affects_both_agents

They pin current behavior, so they pass before and after. To confirm they are load bearing rather than tautological, I temporarily made clone() copy its list attributes when they are not passed, which is the behavior the old docstring described, and two of the four failed:

FAILED tests/test_agent_clone_shallow_copy.py::test_agent_clone_keeps_list_attributes_it_is_not_given
FAILED tests/test_agent_clone_shallow_copy.py::test_agent_clone_shared_list_mutation_affects_both_agents
2 failed, 3 passed

Then reverted that experiment. The pre-existing test_agent_clone_shallow_copy is left untouched.

Commands run locally on Windows. make is unavailable in this environment, so the Makefile targets were invoked directly through uv:

  • uv run pytest tests/test_agent_clone_shallow_copy.py: 5 passed
  • uv run ruff format: 905 files left unchanged
  • uv run ruff check: all checks passed
  • uv run python .github/scripts/check_optional_truthiness.py src/agents: passed
  • uv run pytest -n auto --maxprocesses=9 --dist worksteal -m "not serial": 17 failed, 7847 passed, 82 skipped
  • The same command on unmodified main: 17 failed, 7843 passed, 82 skipped. The failure set is identical; all 17 are pre-existing on this platform, failing in os.symlink with OSError: [WinError 1314] A required privilege is not held by the client, in tests/sandbox/test_tar_utils.py, tests/sandbox/test_workspace_paths.py, and tests/sandbox/test_docker.py. The only difference is the four tests this PR adds, all passing.
  • uv run mypy src: 3 errors, all pre-existing and all in files this PR does not touch (sandbox/util/tar_utils.py, sandbox/sandboxes/unix_local.py, sandbox/sandboxes/docker.py)
  • uv run pyright --project pyrightconfig.json: 1 error, in sandbox/util/tar_utils.py. Confirmed pre-existing by stashing this change and re-running on clean main, which reports the identical error.

One note on methodology, in case it saves anyone a repeat: an earlier full-suite run of mine reported 6 extra failures in tests/extensions/memory/test_advanced_sqlite_session.py, all of them multiprocess contention tests. That was host contention from running the suite and the typecheckers concurrently, not this change. Run on its own, that file reports 128 passed.

Issue number

N/A. Follows up #1293 and #1296, and aligns with openai/openai-agents-js#1705.

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh (make is unavailable here; the equivalent commands are listed above)
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@seratch seratch added the documentation Improvements or additions to documentation label Aug 17, 2026

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merge, please apply the same correction to RealtimeAgent.clone() in src/agents/realtime/agent.py, tailored to its list fields. That method uses the same implementation, and its generated API reference currently repeats the same incorrect claims. Leaving it unchanged would document two equivalent public clone paths inconsistently.

The existing tests/realtime/test_agent.py::test_clone_does_not_mutate_original_lists already covers both relevant cases: a supplied list is used as-is, and an omitted list remains shared. No additional Realtime test is required. Once the Realtime docstring is aligned and CI is green, this should be ready for another review.

@thegoodengineer

Copy link
Copy Markdown
Author

Thanks, that is a fair catch. RealtimeAgent.clone() is updated in 03a2c7a.

One deliberate difference from the Agent version: the Realtime note names tools, handoffs, mcp_servers, and output_guardrails, and omits input_guardrails, since RealtimeAgent does not define that field. I checked each of the four against RealtimeAgent directly rather than assuming it follows Agent, covering the omitted case, the passed-as-given case, clone(attr=agent.attr) still sharing that list, clone(tools=[other_tool]) sharing nothing, and an append through either agent being visible from the other.

No Realtime test added, per your note. test_clone_does_not_mutate_original_lists does already cover the supplied-list and omitted-list cases.

I also confirmed nothing else repeats the old wording: it now returns no matches anywhere in the repo, and the only other clone(**kwargs), HandoffInputData.clone(), makes no shallow-copy claims and holds tuples. The generated reference picks the change up on its own, since docs/ref/realtime/agent.md is an mkdocstrings stub.

Locally: ruff format and ruff check clean, tests/realtime/test_agent.py and tests/test_agent_clone_shallow_copy.py passing, and the full suite matches my clean-main baseline name for name (the only failures on this machine are pre-existing Windows symlink privilege ones under tests/sandbox/).

The workflow run for 03a2c7a is currently showing action_required, so it needs approval before CI can report.

@thegoodengineer
thegoodengineer force-pushed the docs/agent-clone-list-semantics branch from 03a2c7a to d18d7b7 Compare August 17, 2026 08:46
@thegoodengineer

Copy link
Copy Markdown
Author

Correction to the SHA in my note above: I rebased onto main afterwards to clear the out-of-date branch state, so 03a2c7a0 no longer exists. The current head is d18d7b7, and that is the run awaiting approval.

The rebase carried no content change. The diff against main is identical before and after it, and ruff check plus tests/realtime/test_agent.py and tests/test_agent_clone_shallow_copy.py still pass on the new base.

@thegoodengineer
thegoodengineer force-pushed the docs/agent-clone-list-semantics branch from d18d7b7 to 2943fe1 Compare August 17, 2026 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants