Skip to content

fix(core): avoid redundant final computer screenshots - #1827

Open
kyletser wants to merge 3 commits into
openai:mainfrom
kyletser:fix/single-final-computer-screenshot
Open

fix(core): avoid redundant final computer screenshots#1827
kyletser wants to merge 3 commits into
openai:mainfrom
kyletser:fix/single-final-computer-screenshot

Conversation

@kyletser

@kyletser kyletser commented Sep 5, 2026

Copy link
Copy Markdown

Summary

  • Treat screenshot actions as no-ops during computer-action dispatch and use the existing post-batch capture for the observation.
  • Remove the branch-local screenshot cache and invalidation logic, as requested in review.
  • Cover a screenshot request before a click, a trailing screenshot request, direct execution, HITL resume, and legacy replay. The screenshot-before-click regression checks that the returned image reflects the completed click.

Test plan

  • Direct execution, HITL and replay: 337 passed.
  • Runner tests: 692 passed.
  • Full Linux suite: 6453 passed, 2 skipped (209 test files passed).
  • Frozen dependency installation, build, recursive build-check, package dist:check, lint, Prettier for all changed files, and git diff --check passed.

Issue number

Fixes #1735

Checks

  • I've added new tests.
  • I've added a patch changeset for @openai/agents-core.
  • No public API or documentation changes are needed.

Copilot AI lite review requested due to automatic review settings September 5, 2026 08:32
@changeset-bot

changeset-bot Bot commented Sep 5, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1b75d67

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@openai/agents-core Patch
@openai/agents-extensions Patch
@openai/agents-openai Patch
@openai/agents-realtime Patch
@openai/agents Patch
realtime-react-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-08T13:15:20.811101Z 1b75d67 Manual request
🔒 Security Review Completed 2026-09-08T13:17:33.052522Z 1b75d67 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is narrowly scoped to removing a redundant screenshot call, and the updated/added tests cover the reported regression scenarios and verify the single-capture behavior.

Pull request overview

Fixes duplicate computer screenshot captures in the agents-core runner by deferring explicit screenshot actions to the existing single “final post-batch” capture, so each computer tool call produces exactly one screenshot observation.

Changes:

  • Treat computer_call screenshot actions as no-ops during action dispatch and rely on the single final capture.
  • Update/extend test coverage for direct execution, HITL resume, and legacy replay to assert a single screenshot capture.
  • Add a patch changeset for @openai/agents-core.
File summaries
File Description
packages/agents-core/src/runner/toolExecution.ts Stops invoking computer.screenshot() for action.type === 'screenshot', leaving only the final capture after all actions.
packages/agents-core/test/runner/toolExecution.test.ts Adds/updates assertions to ensure exactly one screenshot per computer call and validates ordering for non-screenshot actions.
packages/agents-core/test/runner/turnResolution.test.ts Adjusts resume-related expectations to match single-capture behavior.
packages/agents-core/test/toolInvocationReplay.test.ts Updates replay expectations to reflect removing the redundant screenshot call.
.changeset/single-final-computer-screenshot.md Declares a patch bump for the bug fix in @openai/agents-core.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Deferring every explicit screenshot changes the semantics of the action sequence, rather than only removing a duplicate capture. For [screenshot, click], the caller asked the backend to capture before the click, but this patch deliberately executes click first and captures only afterward. A screenshot can also be an observable or synchronizing operation for remote computer implementations. A safer shape would be to execute explicit screenshots in order, cache the most recent capture, and reuse it only when no later non-screenshot action invalidates it. That still gives one capture for [screenshot] and [click, screenshot], while preserving the requested ordering for [screenshot, click].

@kyletser kyletser changed the title fix(core): capture computer screenshots once per batch fix(core): avoid redundant final computer screenshots Sep 5, 2026
@kyletser

kyletser commented Sep 5, 2026

Copy link
Copy Markdown
Author

Addressed in 1ee899c. Explicit screenshot actions now execute at their original sequence position. The most recent capture is reused only when no later non-screenshot action invalidates it; otherwise the runner captures the final state after the remaining actions. Added regressions for both [screenshot, click]\ and [click, screenshot]. The focused execution, HITL resume, and replay suite passes (337 tests), along with build, type, dist, lint, and formatting checks. The Windows full-suite run reached 6,363 passes; its 92 failures are confined to existing Unix sandbox, POSIX path/permission, and Windows child-process fixtures, while all changed-path tests pass.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the current action loop. Explicit screenshots now run in sequence, a later non-screenshot action invalidates the cached capture, and only a final explicit screenshot is reused. The [screenshot, click] and [click, screenshot] regressions cover the ordering boundary I raised. My concern is resolved.

@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.

Thanks for the update. The current revision restores the caching and invalidation approach that we previously decided against in #1736. The official computer-use handlers treat screenshot actions as no-ops within the batch and capture the final screen afterward.

Please remove the screenshot cache and invalidation logic, make the screenshot switch case a no-op, and use the existing final capture. Update the [screenshot, click] regression to expect the click followed by one screenshot, with that image returned as the observation. This addresses #1735 without adding an intermediate-capture contract.

@kyletser

kyletser commented Sep 7, 2026

Copy link
Copy Markdown
Author

Thanks for clarifying. Addressed in 1b75d67: the screenshot case is now a no-op, and the cache/invalidation logic is removed. The existing final capture supplies the observation. The [screenshot, click] regression now expects click followed by exactly one screenshot and checks that the returned image reflects the completed click. Focused execution/HITL/replay: 337 passed; runner: 692 passed; full Linux suite: 6453 passed, 2 skipped. Build, type checks, lint, and formatting also passed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b75d67cc7

ℹ️ 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".

Comment thread packages/agents-core/src/runner/toolExecution.ts

@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.

Please fix all the codex feedback

@kyletser

kyletser commented Sep 8, 2026

Copy link
Copy Markdown
Author

@seratch Could you confirm the intended screenshot semantics before I change the implementation again?

Your earlier review requested removing the cache/invalidation logic and treating screenshot actions as no-ops, with one final capture. That is implemented in 1b75d67. The new automated finding requests the opposite: capture at each screenshot action's position, cache the image, and invalidate it after subsequent actions, citing the guide's statement that batched actions execute in order.

For [screenshot, click], should the expected calls remain click -> screenshot, or should they be screenshot -> click -> screenshot? If final-only capture is still intended, I can clarify the screenshot exception in the appropriate documentation, coordinated with release timing, rather than restore the approach rejected in #1736. I have left the finding open pending clarification.

@seratch

seratch commented Sep 8, 2026

Copy link
Copy Markdown
Member

Thanks for flagging the conflicting feedback. The intended behavior remains one final capture per completed batch: [screenshot, click] should execute click followed by screenshot.

My later request to address all Codex feedback was too broad. The intermediate-capture finding does not change the specific direction previously requested. The current implementation matches that direction; no cache or invalidation logic is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Computer screenshot action captures twice before returning observation

4 participants