fix: #781 replace assertion in handoff() with UserError#3339
fix: #781 replace assertion in handoff() with UserError#3339seratch merged 2 commits intoopenai:mainfrom
Conversation
There was a problem hiding this comment.
Thanks for the fix. The direction looks good to me, especially because it brings regular handoff() closer to the existing Realtime handoff validation.
Before merging, could you please tighten this up by making the user-facing validation consistent across the full path?
Specifically, please replace the adjacent assert callable(on_handoff) with a UserError as well, and apply the same treatment to realtime_handoff() so regular handoffs and Realtime handoffs behave consistently. Since these checks validate user-provided arguments, they should not rely on assert, which can be disabled with Python optimization flags.
It would also be good to use the same error wording in both places, for example:
raise UserError("You must provide on_handoff when input_type is provided")and a corresponding UserError message for the non-callable case.
199c2ea to
1b0fde0
Compare
The assertion `(X) or not (X)` is always True and enforces nothing. Replace with an explicit UserError when input_type is provided without on_handoff. Also replace assert callable(on_handoff) with UserError in both handoff() and realtime_handoff() for consistent user-facing validation that cannot be disabled by Python optimization flags.
1b0fde0 to
531a5f5
Compare
|
Thanks for updating the PR. I will merge other PRs before yours, but no need to rebase this (it triggers a new code review and CI builds for this relatively simple change). |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Replace tautological assertion in
handoff()with properUserErrorvalidation.Two problems with the original assertion:
Tautological logic —
(X) or not (X)is alwaysTrue, so the assertion never fires and enforces nothing.Incorrect error message — The message says "You must provide either both on_handoff and input_type, or neither", but
on_handoffwithoutinput_typeis a valid usage (simple callback that takes only context). The only invalid combination isinput_typewithouton_handoff, since there would be no callback to receive the parsed input.Before (tautology — never fires):
After (correct validation):
Valid combinations:
on_handoffinput_typeNote: downstream checks at lines 259–272 already catch most misuse (e.g.,
assert callable(on_handoff)and signature length validation), so the tautological assertion was effectively dead code. This fix replaces it with a clear, early validation that produces a helpful error message.Test plan
test_input_type_without_on_handoff_raises_errorto verify the fixtests/test_handoff_tool.py,tests/test_handoff_prompt.py,tests/test_handoff_history_duplication.py)make format,make lint,make typecheck— all passIssue number
Closes #781
Checks
make lintandmake format