Repository navigation
fix(responses): resolve function arguments within their namespace - #4166
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom codeEvaluated main: Trusted main policy: ✅ No new custom-code files detected. 0 mixed files remain; 0 existing customizations changed. Compared A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 39878433648 --repo openai/openai-python \
--name castiron-custom-code-39878433648-1 --dir /tmp/castiron-custom-code-39878433648-1
git apply --stat /tmp/castiron-custom-code-39878433648-1/custom-code.patch
cat /tmp/castiron-custom-code-39878433648-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 56f33a12591516e9ce88ee72e3a59b15ac518641 d62e59b748af93e9525a06409050455fd12b9f1c
python3 scripts/castiron/custom_code_report.py report \
--base 56f33a12591516e9ce88ee72e3a59b15ac518641 \
--head d62e59b748af93e9525a06409050455fd12b9f1c --target-branch main --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-d62e59b748af
cat /tmp/castiron-custom-code-d62e59b748af/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4af63c1ce1
ℹ️ 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".
| # Deferred top-level functions can return their own name as the namespace. | ||
| for tool in input_tools: | ||
| if tool["type"] == "function" and tool.get("name") == name: |
There was a problem hiding this comment.
When a function call has a namespace that is not present in input_tools, this loop still selects any same-named top-level function. This can occur when tool search returns a dynamically loaded namespaced function alongside a submitted top-level function with the same name, causing arguments to be parsed with the wrong schema or raising a validation error. Limit the fallback to unnamespaced calls or the deferred top-level convention where the namespace equals the function's own name.
Useful? React with 👍 / 👎.
| for nested_tool in tool["tools"]: | ||
| if nested_tool["type"] == "function" and nested_tool["name"] == name: | ||
| return nested_tool |
There was a problem hiding this comment.
When NamespaceToolParam.tools is a generator or another one-shot iterable, the request transformation consumes it while serializing the request, so this later iteration is empty and parsed_arguments remains None. The public type explicitly accepts Iterable[Tool], so materialize the nested tools before request serialization or otherwise retain a reusable collection for parsing.
Useful? React with 👍 / 👎.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two P2 parsing gaps remain at 4af63c1. I independently traced and agree with the existing findings, so I am not duplicating inline comments:
- #4166 (comment) — An unknown namespace falls through to a same-named top-level function, which can parse with the wrong model or raise validation errors. Preserve the deferred top-level self-name convention, but return no parsed arguments for unrelated namespaces.
- #4166 (comment) — NamespaceToolParam.tools accepts Iterable, but request transformation consumes a nested generator before this lookup. Retain a reusable nested collection before serialization and cover one-shot iterables through the public parsing path.
Static review of the diff, request transformation and parsing paths, types, tests, and existing feedback; no candidate code or tests executed. Hosted CI was still running during review.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Both previous P2 findings are addressed at ac92c60. Unknown namespaces no longer fall through to unrelated top-level functions, while the deferred self-name convention remains supported. _make_tools materializes nested iterables before serialization and retains the Pydantic tool objects for subsequent parsing; the shared helper covers sync/async parse and stream paths. The added public-path regressions cover both cases. No remaining blocking findings from independent static reviews.
Hosted CI is still running (20 successful, 12 skipped, 4 in progress at the latest check). No candidate code or tests executed.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all three Python source/test changes at ac92c603c6d78f3eb26a288120369b5af8ff62a5. Both previous findings are fixed: namespace lookup preserves namespace authority, and nested tool iterables are materialized before serialization while retaining their parsing models.
The new regressions cover namespace selection and one-shot iterators in sync parse; I also traced the shared conversion helper through sync/async parse and streaming. Static source review only; I did not run candidate code or tests. All 36 hosted check contexts succeeded or were skipped at this read.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-approved at d62e59b after the rebase onto the relocated response helpers. The parser and namespace regression tests are unchanged from the previously approved head. The remaining helper change preserves reusable nested tool collections and Pydantic metadata before request serialization; sync/async parse and stream paths use that helper. Both prior namespace-fallback and one-shot-iterator fixes remain intact. No new actionable findings from independent static reviews. Current-head hosted CI is running; no candidate code or tests executed.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: d62e59b748
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| input_tool = get_input_tool_by_name( | ||
| input_tools=input_tools, name=function_call.name, namespace=function_call.namespace | ||
| ) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When an application uses ResponsesToolRunner with RegisteredFunction.from_tool and a side-effecting executor that tolerates None, a response can name that registered function but add an unrelated non-null namespace. The new lookup returns None before JSON/Pydantic validation; the runner stores that as parsed arguments and invokes the executor with it. This breaks the documented fail-closed guarantee, so an invalid prompt-influenced/API response can trigger callback effects without satisfying the registered model. Reject namespace-bearing calls in the top-level runner (except the deferred namespace == name convention), or treat a missing parser match as an error.
SECURITY.md reference: SECURITY.md:L9-L11
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Changing my earlier approval to request changes after independently validating the new P2: #4166 (comment). I missed the ResponsesToolRunner caller in the earlier review; no duplicate inline comment is needed.
RegisteredFunction.from_tool (and its async counterpart) requires a Pydantic model, but runner metadata validation accepts an unrelated namespace. The new parser lookup then returns None before JSON/Pydantic validation, and _Run.validate stores it as arguments. Both sync and async executors invoke the callback with that None. A callback that tolerates or ignores its argument can perform effects without satisfying the registered model; the base parser validated the model on this path.
Reject unmatched namespaces before invoking top-level registered tools, retaining the deferred namespace == name convention, or explicitly reject a missing typed parser result. Add runner regressions that assert the callback is not invoked for an unrelated namespace, including malformed/model-invalid arguments. The earlier parser namespace isolation and nested-iterator fixes should remain intact.
Static review only; no candidate code or tests executed. Hosted CI still has a browser provider-types check pending.
4 participants
Namespaced function calls currently leave
parsed_argumentsempty or parse against a same-named top-level tool's model. Resolve the function inside the matching namespace. If the namespace or nested function is unknown, leave arguments unparsed rather than selecting an unrelated top-level tool. Preserve top-level lookup for calls without a namespace and the deferred-tool convention where the namespace equals the function name.Materialize nested tool iterables before request serialization so generators remain available for parsing, preserving Pydantic model wrappers without modifying the caller's namespace dictionary. Both changes now live in handwritten helpers (
_parsing/_responses.pyand_response_resources.py); generated resource files are unchanged from main.Fixes #4064.
Validation after merging main
56f33a125:--python-version 3.10) pass.