fix: isolate usage between RunState checkpoints - #4479
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41cb6a518a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Please keep the new copied.usage = copy.deepcopy(self.usage) assignment in src/agents/run_context.py; that correctly isolates top-level checkpoints.
The additional runtime change belongs in Agent.as_tool()'s _run_agent_impl() in src/agents/agent.py. After the pending_resume_state if / elif block selects resume_state, move the existing usage assignment out of the status in ("approved", "rejected") branch and apply it to both paths:
if resume_state is not None and resume_state._context is not None:
resume_state._context.usage = context.usagePlace this immediately after the pending_resume_state if / elif block and before if run_result is None:. This preserves top-level checkpoint isolation while ensuring cached nested Agent.as_tool() resumes continue accumulating usage on the current outer run.
|
Done. Moved the usage rebind out of the approved/rejected branch so it applies to the cached resume path too, and kept the |
Summary
RunContextWrapper._copy_for_run_statedetaches_approvals,_tool_invocationsand_restored_unbound_approval_call_ids, but thecopy.copy(self)it starts from leavesusageshared.Usage.addaccrues in place and extendsrequest_usage_entries, so every checkpoint taken from a result, and the result itself, accumulate into one instance.Two consequences, both reproducible on
main:A
RunResultthe caller already holds keeps changing. Afterresult.to_state()and a resume,result.context_wrapper.usage.requestsgoes from 1 to 2.Two checkpoints from one result bill each other. Each resumed once from the same one-request run, so both should report 2 requests. The second reports 3.
The copy is deep because
addextendsrequest_usage_entriesrather than replacing it.turn_inputis aliased by the samecopy.copy, but it is reassigned rather than mutated inturn_resolution.pyandrun_loop.py, so I left it as is rather than widen the change.Test plan
Two tests in
TestRunStateResumption, next to the existing resume-isolation coverage: one asserts a resume does not move the source result's usage, the other that two checkpoints from one result report the same total and do not sharerequest_usage_entries. Reverting only therun_context.pyline fails them withassert 2 == 1andassert 3 == 2.make format,make lint,make typecheckandmake testsall pass (8806 passed, plus the serial pass).Issue number
None, filing the fix directly since it is a narrow follow-up to #4413.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR