Skip to content

Fix URL resolution and recipient guard in SimpleBrowserTool - #307

Open
Ultron09 wants to merge 2 commits into
openai:mainfrom
Ultron09:fix/browser-process-arguments-url
Open

Fix URL resolution and recipient guard in SimpleBrowserTool#307
Ultron09 wants to merge 2 commits into
openai:mainfrom
Ultron09:fix/browser-process-arguments-url

Conversation

@Ultron09

Copy link
Copy Markdown

Summary

Fixes two related bugs in SimpleBrowserTool argument parsing that break web search action emission and server stability in the Responses API:

  1. Missing url in process_arguments for open and find actions:

    • process_arguments() only resolved url when cursor >= 0 was explicitly present in function_args, or when id was a string.
    • When a model emits browser.open(id=0) (opening a link from the current search results with default cursor=-1) or browser.find(pattern="..."), process_arguments() returned the arguments dictionary without a url field.
    • In responses_api/api_server.py:
      • Non-streaming (generate_response): parsed_args["url"] raised KeyError: 'url', which was caught by except Exception and caused action to be set to None. As a result, the WebSearchActionOpenPage / WebSearchActionFind item was completely omitted from the API response.
      • Streaming (stream_response): url fell back to None, emitting invalid null URLs in web_search_call SSE events.
  2. AttributeError on None recipient in maybe_get_function_args:

    • maybe_get_function_args() directly called message.recipient.startswith(...). If message.recipient is None (standard for normal conversational messages), it crashed with AttributeError: 'NoneType' object has no attribute 'startswith'.

Changes

  • Guard maybe_get_function_args() to safely return None when message.recipient is None.
  • In process_arguments(), retrieve the active page using function_args.get("cursor", -1) when current_cursor >= 0.
  • Correctly map integer/string link IDs from page.urls, handle direct string URLs, and populate page.url for find or open operations on the current page.
  • Add comprehensive test coverage in tests/gpt_oss/tools/simple_browser/test_simple_browser_tool.py testing maybe_get_function_args guards and process_arguments across default cursor, explicit cursor, direct URL, and in-page find scenarios.

Verification

  • Added 10 regression and unit tests in tests/gpt_oss/tools/simple_browser/test_simple_browser_tool.py.
  • Ran full test suite (pytest): all 39 tests pass.

@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: f36e2dd4af

ℹ️ 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 on lines +589 to +590
if arg_id in page.urls:
function_args["url"] = page.urls[arg_id]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve direct-URL semantics for string IDs

When the current page has its normal numeric-string link keys (for example, {"0": "https://…"}), a call with id: "0" now reports the mapped link URL in the Responses API action. However, open() treats every string ID as a direct URL and attempts to fetch the literal "0" (lines 493-496), so the emitted open_page URL no longer describes the operation that actually ran. Only resolve link IDs here when they have the same type/semantics as open(), or make open() resolve string link IDs too.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 456504e. \process_arguments()\ now checks \isinstance(arg_id, str)\ first to preserve direct-URL semantics matching \open(), and only resolves numeric link IDs when \�rg_id\ is an \int >= 0. Added unit tests in \ est_simple_browser_tool.py\ covering string ID direct URL preservation and negative ID navigation.

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

Rechecked the current head after the string-ID fix. None recipients now exit safely, integer link IDs resolve against the page, string IDs keep direct-URL semantics, and find/open without an ID gets the current page URL.

That lines up with the actual browser operations now. I don't see a remaining issue in this patch.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants