fix(onboard): shorten codex detect timeout to 30s and show progress spinner#81233
fix(onboard): shorten codex detect timeout to 30s and show progress spinner#81233sjf wants to merge 1 commit into
Conversation
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Source inspection shows current main can wait up to 60 seconds in the Codex source app-server request before the migration prompt, and the PR body includes an after-fix interactive onboard run showing spinner feedback. Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Keep the 30-second ceiling and interactive spinner, but make the spinner distinguish app-server inventory failures/timeouts from true no-state detection and cover that path with a focused regression test. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main can wait up to 60 seconds in the Codex source app-server request before the migration prompt, and the PR body includes an after-fix interactive onboard run showing spinner feedback. Is this the best way to solve the issue? No, not quite. The timeout/spinner approach is the right narrow fix, but the failure status should be wired to the Codex app-server error path instead of only to thrown Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against bd8e986bb2eb. Re-review progress:
|
Summary
The post-install migration prompt (#81192) can take up to 60 seconds before it appears, with no visible activity on the terminal. Two narrow fixes that together turn "the wizard looks frozen" into "the wizard is clearly checking, with a known time bound."
Shorten the source-side
codex app-servertimeout to 30 seconds inrequestSourceCodexAppServerJson(extensions/codex/src/migration/source.ts:268). This is the call thatprovider.detect()andprovider.plan()make to enumerate curated-marketplace plugins. 60s is too long for an interactive prompt path; on an unauth'd or stalled~/.codex, the full minute was eaten before the migration prompt could fire.Wrap each provider's
detect()call in a wizard progress spinner insrc/wizard/setup.post-install-migration.ts. The spinner showsChecking Codex state…while the probe runs, and ends withFound Codex state./No Codex state to migrate./Codex detection failed.so the operator sees outcome, not just absence. Only fires when the helper is actually going to drive an interactive prompt (TTY + prompter passed); non-interactive callers still rely onruntime.logand are unchanged.Behavior
Before (interactive wizard, post
ensureCodexRuntimePluginForModelSelection):After:
If
codex app-serverdoes not respond within 30s the spinner stops withCodex detection failed., the wizard moves on, and onboarding continues — no abort.Files
extensions/codex/src/migration/source.ts—timeoutMs: 60_000→30_000onrequestSourceCodexAppServerJson, with a comment recording why.src/wizard/setup.post-install-migration.ts—resolveCandidatesgains an optionalprompterparameter and runs eachdetect()insideprompter.progress(...)with success / no-state / failure end labels.offerPostInstallMigrationsonly forwards the prompter when the helper is in interactive mode (!nonInteractive && stdin.isTTY && prompter).Verification
Real behavior proof — interactive
bash run.sh onboard-fastAgainst the seeded fixture at
~/onboarding/codex_home:openai/gpt-5.5so the Codex harness install ran.◐ Checking Codex state…— running whilecodex app-server plugin/listwas in flight.Found Codex state.; migration confirm prompt followed immediately.Notes
plugin/listcomplete; it just caps the worst case to half what it used to be. Longer-term, a filesystem-only "light" detect mode (flagged in the original PR's follow-ups) would let this drop to milliseconds — that's a separate change.WizardPrompter.progressseam used bysrc/commands/onboard-skills.ts:168,src/commands/onboarding-plugin-install.ts:604, etc. No new prompter surface.prompter.progress(...)is only invoked whenprompteris forwarded, so non-interactive / non-TTY paths see no UI change.