docs: correct Agent.clone list attribute semantics - #4474
docs: correct Agent.clone list attribute semantics#4474thegoodengineer wants to merge 2 commits into
Conversation
seratch
left a comment
There was a problem hiding this comment.
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.
|
Thanks, that is a fair catch. One deliberate difference from the No Realtime test added, per your note. I also confirmed nothing else repeats the old wording: it now returns no matches anywhere in the repo, and the only other Locally: The workflow run for 03a2c7a is currently showing |
03a2c7a to
d18d7b7
Compare
|
Correction to the SHA in my note above: I rebased onto The rebase carried no content change. The diff against |
d18d7b7 to
2943fe1
Compare
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:Neither half is reliable.
clone()callsdataclasses.replace, which never copies a list at all, so what the clone gets is whatever the merged arguments hold:So
agent.clone(tools=agent.tools)is an override that creates no new list, andagent.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.toolsand 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_copypassestools=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.replacenever 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_giventest_agent_clone_uses_a_given_list_as_istest_agent_clone_still_shares_when_given_the_original_listtest_agent_clone_shared_list_mutation_affects_both_agentsThey 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:Then reverted that experiment. The pre-existing
test_agent_clone_shallow_copyis left untouched.Commands run locally on Windows.
makeis unavailable in this environment, so the Makefile targets were invoked directly throughuv:uv run pytest tests/test_agent_clone_shallow_copy.py: 5 passeduv run ruff format: 905 files left unchangeduv run ruff check: all checks passeduv run python .github/scripts/check_optional_truthiness.py src/agents: passeduv run pytest -n auto --maxprocesses=9 --dist worksteal -m "not serial": 17 failed, 7847 passed, 82 skippedmain: 17 failed, 7843 passed, 82 skipped. The failure set is identical; all 17 are pre-existing on this platform, failing inos.symlinkwithOSError: [WinError 1314] A required privilege is not held by the client, intests/sandbox/test_tar_utils.py,tests/sandbox/test_workspace_paths.py, andtests/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, insandbox/util/tar_utils.py. Confirmed pre-existing by stashing this change and re-running on cleanmain, 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
.agents/skills/code-change-verification/scripts/run.sh(makeis unavailable here; the equivalent commands are listed above)/reviewbefore submitting this PR