Fix ChatGPT subscription generation routing - #2316
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
gltanaka
left a comment
There was a problem hiding this comment.
Changes requested
GitHub does not permit the PR author to submit a formal REQUEST_CHANGES event, but these are blocking findings. The single-call Responses routing is needed and the controlled focused tests support it; the current head is not merge-ready.
P1 — preserve the fix in PDD’s source of truth. The new behavior in pdd/llm_invoke.py:5685-6057 and pdd/model_tester.py:191-272 is not reflected in either checked-in prompt copy. pdd/prompts/llm_invoke_python.prompt:180-183 still says only OpenAI GPT-5 uses responses() and every other model uses completion/batch completion; pdd/prompts/model_tester_python.prompt:17-21 still requires exactly one litellm.completion() call. model_tester also has no correct architecture registration/baseline for pdd/model_tester.py. The repository’s own check confirms this: python -m pdd.ci_drift_heal --dry-run --diff-base origin/main...HEAD --modules llm_invoke model_tester --json exits 1 with llm_invoke = CONFLICT and model_tester = UNBASELINED. A future sync can therefore regenerate the unsupported endpoint and undo this fix. Update the canonical/package prompt copies plus architecture/metadata required by the sync workflow, and make the focused drift check clean.
P1 — the supported batch boundary still uses the broken endpoint. use_responses_api is gated by not use_batch_mode at pdd/llm_invoke.py:5692-5700; a chatgpt/* batch consequently reaches litellm.batch_completion() at pdd/llm_invoke.py:6234-6237. A focused reproduction with PDD_MODEL_DEFAULT=chatgpt/gpt-5.3-codex recorded responses_calls=0, batch_completion_calls=1. That retains the same unsupported chat-completions/Cloudflare failure for a public llm_invoke mode and conflicts with the original first-class-provider contract that every caller can use the subscription. Route batch items through the Responses path, or fail closed with an actionable unsupported-mode error; add a regression proving no ChatGPT request reaches completion or batch completion.
P2 — make the changed subscription tests hermetic. In this dedicated worktree, pytest -q tests/test_model_tester.py tests/test_codex_subscription.py gives 69 passed / 2 failed because test_fallback_reaches_chatgpt_when_anthropic_key_missing and the rewritten structured-output test inherit the project’s legitimate PDD_MODEL_DEFAULT from .env and never reach their mocked ChatGPT row. Pin/delete PDD_MODEL_DEFAULT explicitly in each test as appropriate. With PDD_MODEL_DEFAULT=claude-sonnet-4-6, the same suite passes 71/71.
The contributor’s live pdd generate smoke is useful but only covers an unstructured single request. After the fixes, please also record a real structured subscription invocation and the setup/model-test boundary changed here; batch can remain mocked if a real batch would consume unnecessary allowance.
|
Follow-up review on Resolved the original routing/prompt drift and batch safety issues, then addressed adversarial findings: the ChatGPT setup smoke now rejects empty Responses output, and code-generator text/image inputs retain Validation: focused model tester/subscription suite (75 passed); Responses/batch invocation selection (14 passed); syntax and diff checks; Residual risk: provider calls are mocked; no live ChatGPT subscription smoke was run. I cannot submit a formal GitHub approval because this account is the PR author. |
723e606 to
8f0cfc6
Compare
|
Final review/fix loop is complete. The PR head has been rebuilt on current Validation performed on the equivalent final tree:
Prerequisite governance/test-state changes were isolated and merged through #2355, #2356, #2357, #2359, #2360, and #2361. Awaiting this head’s full hosted CI before merge. |
Summary
chatgpt/*model smoke tests and normal generation through LiteLLM's Responses APIRoot cause
pdd setupandpdd generateexercised different LiteLLM paths. The setup/model test could validate the Codex subscription, while generation calledlitellm.completion()forchatgpt/gpt-5.6-sol. That targeted/backend-api/codex/chat/completions, which returned a browser-only Cloudflare challenge. PDD then fell back to Gemini despite the ChatGPT subscription being valid.The Codex subscription backend works through
litellm.responses()and requires list-shapedinput.Tests
pytest -q tests/test_model_tester.py tests/test_codex_subscription.py— 71 passedpython -m py_compile pdd/llm_invoke.py pdd/model_tester.pygit diff --checkLive verification
Ran an editable install against an authenticated Codex subscription:
The command completed without the Cloudflare response or Gemini fallback: