docs: note that Agent.clone shares non-list mutable attributes - #4505
docs: note that Agent.clone shares non-list mutable attributes#4505ayaangazali wants to merge 1 commit into
Conversation
openai#4474 corrected the clone docstring for list attributes, but its wording is list-only, so a reader reasonably concludes non-list attributes are copied. They are not: model_settings and mcp_config both arrive as the original agent's own object when omitted, so mutating them through the clone changes the original. Also records that overriding model alone does not detach model_settings. Fresh settings are substituted only when the current ones still match the implicit defaults for the current model, so an agent carrying any explicit setting keeps sharing that object across clone(model=...).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adaa7260e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - The same applies to the mutable attributes that are not lists, `model_settings` and | ||
| `mcp_config`. An omitted one arrives as the original agent's own object, so |
There was a problem hiding this comment.
Cover all shared non-list mutable fields
When an agent uses a static prompt dictionary or a StopAtTools dictionary for tool_use_behavior, dataclasses.replace() shares those objects too, so mutating either field through the clone also changes the original agent. Calling model_settings and mcp_config “the mutable attributes that are not lists” therefore preserves the same misleading implication this change is intended to remove; describe shallow reference sharing generally or also cover these fields and their replacement patterns.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
|
We'd like to avoid making the comment much longer. We may simplify the sentences there rather than listing all the possible properties. |
Summary
#4474 fixed the
Agent.clone()docstring for list attributes. Its wording is list-only, though, so a reader now reasonably concludes that anything which is not a list gets copied:Agenthas two mutable attributes that are not lists, anddataclasses.replaceshares both exactly the same way. On currentmain:So the surprise #4474 set out to document still bites through
model_settingsandmcp_config, and the advice it gives ("pass a new one, for exampleagent.clone(tools=[*agent.tools, extra_tool])") does not translate to either.The last line is the part I did not expect. Overriding
modelalone does not give the clone its own settings:clone()substitutes fresh ones only when_model_settings_match_implicit_model_defaults(self.model, self.model_settings)holds, so an agent carrying any explicit setting keeps sharing thatModelSettingsobject acrossagent.clone(model=...).This documents both, in the same shape #4474 used, and leaves the behavior alone.
RealtimeAgentneeds nothing: it has nomodel_settingsormcp_config, so its list-only wording is already complete.Test plan
tests/test_agent_clone_shallow_copy.py, next to the coverage docs: correct Agent.clone list attribute semantics #4474 added: one asserts both non-list attributes are shared and that mutating them through the clone changes the original, one assertsclone(model=...)alone keeps the shared settings. They pin the behavior the docstring describes, in the same way docs: correct Agent.clone list attribute semantics #4474's tests do, rather than pinning the prose.uv run pytest tests/test_agent_clone_shallow_copy.py: 7 passed..agents/skills/code-change-verification/scripts/run.sh: format, lint, typecheck and the full test run all pass.Issue number
None. Follow-up to #4474, which corrected the list half of the same docstring.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRcontext so this does not look like second-guessing a PR that just landed: I had measured the same sharing and left a note about it on #4474 rather than opening a competing change, and it merged without that half, so this is the leftover rather than a disagreement. #4474's version of the list wording is better than what I would have written. if you would rather the docstring stay list-only and treat
model_settingsas obvious, close it and I will take that. freshman in college, still calibrating what belongs in a docstring :)