refactor(api): move file processing polling into SDK-owned helpers - #3712
Conversation
apcha-oai
added
the
custom-code-burndown
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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 code✅ No new custom-code files detected. 36 mixed files remain; 1 existing customization changed. Compared
35 existing customizations unchanged
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 32498300373 --repo openai/openai-python \
--name castiron-custom-code-32498300373-1 --dir /tmp/castiron-custom-code-32498300373-1
git apply --stat /tmp/castiron-custom-code-32498300373-1/custom-code.patch
cat /tmp/castiron-custom-code-32498300373-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin a002ef3c7d2276fb63deeea58d920d87da61b1e5 c708fbd05d196493874aed3970c314792442b0c8
python3 scripts/castiron/custom_code_report.py report \
--base a002ef3c7d2276fb63deeea58d920d87da61b1e5 \
--head c708fbd05d196493874aed3970c314792442b0c8 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-c708fbd05d19
cat /tmp/castiron-custom-code-c708fbd05d19/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: 18d4b16498
ℹ️ 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".
| @pytest.mark.parametrize("status", ["processed", "error", "deleted"]) | ||
| @pytest.mark.parametrize("max_wait_seconds", [-1.0, 0.0, 1800.0]) |
There was a problem hiding this comment.
This 358-line suite adds roughly 80 cases for a 57-line extraction whose behavior is otherwise unchanged, including combinatorial checks for incidental implementation details such as negative and non-finite timeouts, backward wall clocks, and exact internal event ordering. That makes future polling fixes unnecessarily expensive and conflicts with the repository requirement that handwritten tests remain small; retain focused sync/async coverage for terminal results, polling, and timeout propagation instead.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Please address the existing discussion on the new handwritten test suite: repository AGENTS.md requires handwritten tests to remain small, but this behavior-preserving extraction adds a 358-line suite with broad combinations that lock in incidental clock, timeout, and event-order details. Keep focused sync/async coverage for terminal states, polling, timeout, cancellation, and error propagation. Treat this as a maintainability issue, not a runtime/security defect; no duplicate inline thread needed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Thanks for addressing the maintainability feedback. The handwritten polling suite is now a focused 87 lines covering terminal states, polling, timeout boundaries, transport errors, and cancellation across sync/async paths; the production extraction remains behavior- and API-preserving. Looks good.
2 participants
Changes being requested
Move the existing sync and async file-processing polling bodies into SDK-owned
lib/_files.py.Files.wait_for_processingandAsyncFiles.wait_for_processingkeep their public signatures, defaults, docstrings, and return types, and delegate to the helpers. No schema, compiler, dependency, or generation-metadata changes are needed.The polling bodies are unchanged apart from renaming the receiver from
selftofiles. This preserves theprocessed/error/deletedterminal states, wall-clock timing, dynamicretrieveand_sleephooks, return identity, and exact errors. In particular, the strict>timeout check still happens after a repeated retrieval, even if that retrieval returns a terminal state. This does not introduce a generic polling framework or change existing timeout behavior.Review pointers:
Following the review discussion, the new handwritten suite is limited to 87 lines rather than an exhaustive matrix of clock, timeout, and event-order details. Existing Files API tests are unchanged. The review revision changes only this new test file; runtime source remains byte-identical to the original PR head.
Additional context & links
Validation:
TEST_API_BASE_URL=http://127.0.0.1:4141 .venv/bin/python -m pytest tests/lib/test_file_processing.py tests/api_resources/test_files.py -q -n 0and the same command using.venv-pydantic-v1/bin/python: 130 passed, 3 skipped in each Pydantic mode. The skips are the existing aiohttp/respx2-incompatibletest_method_content,test_raw_response_content, andtest_streaming_response_contentcases..venv/bin/ruff format tests/lib/test_file_processing.pyand./scripts/lintpassed on the review revision, including Ruff, Pyright, mypy, and import checks. The initial full./scripts/formatalso passed; unrelated reporter-formatting changes were excluded../scripts/buildpassed. Both the wheel and source distribution containopenai/lib/_files.py; importing the helper and resource classes from the built wheel also passed.The verified custom-code report keeps 36 mixed files and changes only the Files resource customization, shrinking it from +47/-0 to +26/-0. The other 35 customizations are unchanged. Reproduction command: