Skip to content

fix(key-vault): make Codex model discovery reliable - #677

Open
beruro wants to merge 1 commit into
org2AI:developfrom
beruro:junyu/fix-codex-model-detection
Open

fix(key-vault): make Codex model discovery reliable#677
beruro wants to merge 1 commit into
org2AI:developfrom
beruro:junyu/fix-codex-model-detection

Conversation

@beruro

@beruro beruro commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Problem

Codex subscription credential detection could stay loading indefinitely. The npm Codex wrapper spawns a native app-server descendant, but the previous cleanup only killed the wrapper and then waited forever for inherited stderr pipes to close.

The same flow also allowed persisted enabled_models to drift from the provider catalog in available_models. Different selectors then interpreted the same account differently, so refreshed GPT-5.6 models could appear in one surface but stale GPT-5.5 or GPT-5.4 rows remained in another. Auto-detection also lacked deterministic progress, success, timeout, and failure feedback.

Solution

  • Await the authoritative OAuth model catalog during credential detection and model refresh.
  • Run the Codex app-server wrapper in a dedicated process group, terminate the full process tree on success or timeout, bound child and stderr shutdown, and clean temporary Codex homes.
  • Add a single credential-detection state machine for idle, credential scan, catalog load, credential selection, success, and error, with duplicate-click protection and a 30-second UI deadline.
  • Enforce the invariant enabled_models ⊆ available_models on save, hydration, and model refresh while preserving user order and removing duplicates.
  • Apply the same available-and-enabled rule in the model picker, workflow options, CLI-managed config, collaboration fallback, and housekeeper consumers.
  • Add regression coverage for process-tree cleanup, persistence normalization, state transitions, timeout behavior, and stale model filtering.

Potential risks

  • The Windows process-tree path uses taskkill /T /F and compiles, but runtime verification was performed on macOS only.
  • The 30-second frontend deadline stops waiting and ignores a late result; it does not cancel the underlying IPC call. The backend has its own 10-second request deadline plus bounded shutdown.
  • Hydration and refresh intentionally remove enabled model ids no longer present in the authoritative catalog. This is a data cleanup behavior change, not a schema migration; rollback is a code revert, and the provider catalog can repopulate current defaults on the next refresh.
  • No public RPC shape or persisted schema changed. libc is added as a direct dependency for Unix process-group termination.

Verification

  • npx vitest run src/scaffold/WizardSystem/variants/KeyVault src/hooks/models/useModelAccountLookup.test.ts src/features/TeamCollaboration/forkModelFallback.test.ts src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.test.ts src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.test.ts — 9 files, 46 tests passed.
  • npx tsc --noEmit --pretty false — passed.
  • ESLint over every changed TypeScript and TSX file — passed.
  • cargo test --manifest-path src-tauri/crates/key-vault/Cargo.toml — 349 tests passed.
  • cargo clippy --manifest-path src-tauri/crates/key-vault/Cargo.toml --all-targets -- -D warnings — passed.
  • rustfmt --edition 2021 --check over all changed Rust files — passed.
  • Repository pre-commit hook — lint-staged, TypeScript, and scoped Key Vault Clippy passed.
  • Real local Codex catalog probe — live source returned 7 models in 2901 ms; temporary directory count was unchanged and no probe-owned app-server descendant remained.
  • git diff --check and changed-diff secret, personal-path, debug-log, and conflict-marker scan — passed.
  • Workspace-wide cargo fmt --all --check was not used as the gate because unrelated files on current develop are not rustfmt-clean; every Rust file changed by this PR passed direct rustfmt checks.
  • Windows process-tree runtime behavior was not executed on macOS.

Architecture audit

All 10 architecture-audit layers were covered; none were skipped. The call chain was traced from wizard auto-detect and login capture through OAuth catalog resolution, Key Vault save or refresh, hydration, and every changed model consumer. available_models remains the authoritative provider catalog and enabled_models remains the user-selected subset. Save, load, and refresh now share the same normalization invariant, and all changed resolvers check the same available-and-enabled dimensions. Wire fields and serialization remain unchanged.

UI audit

Frontend UI audit result: 0 fixes recommended, 2 keep-with-reason findings, and 0 abstraction candidates. The existing design-system Button and InlineAlert remain in use; progress uses a polite status region and failures use an assertive alert region.

No new screenshot or recording was captured because the user explicitly requested that this task not control or inspect the UI image. UI state behavior is covered by the reducer tests, full typecheck, ESLint, and the real backend catalog probe.

Performance guard

Area Verdict Evidence Change or reason kept Verification
Background work Pass One user-triggered catalog request; duplicate clicks are rejected; backend and UI waits are bounded Dedicated process group and bounded shutdown replace an unbounded pipe wait Live probe plus process-tree regression test
Memory Pass No retained cache or subscription added Timeout helper clears its timer; stderr reader is joined or aborted Unit tests and no residual temp homes
Scope and isolation Pass Detection state is local to one wizard instance and account No module-global mutable state added Reducer tests and call-chain audit
Rendering and hot path Pass State changes only across detection phases Memoized alert projection; no polling or high-frequency listener Typecheck, ESLint, and UI audit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant