Skip to content

[0.64.1] Retire the Crof provider safely - #596

Open
Finesssee wants to merge 2 commits into
mainfrom
codex/port-0.64.1-retire-crof
Open

Finesssee wants to merge 2 commits into
mainfrom
codex/port-0.64.1-retire-crof

Conversation

@Finesssee

@Finesssee Finesssee commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • retire the discontinued Crof provider from Rust, CLI/dashboard, settings, and React registration surfaces
  • tolerate unknown provider keys while loading persisted provider_configs, so an existing Crof entry cannot invalidate the settings file
  • sanitize retired provider IDs from active selections and omit them on the next explicit save while preserving recognized provider settings

Upstream reference: 3404ae350ae5faf7d23b44dcac3c72390694c0c9 (fix(crof): remove provider after shutdown).

Validation

  • cargo fmt --all
  • retired-provider settings compatibility regression (1 passed)
  • provider catalog regression (1 passed)
  • cargo clippy --manifest-path rust/Cargo.toml --lib --tests -- -D warnings
  • focused frontend provider icon/catalog/settings tests (3 passed)
  • git diff --check

Stack

This PR targets the head branch of #590 because both lanes modify provider registration and frontend catalog surfaces. After #590 merges, this PR can be rebased or retargeted to main without carrying prerequisite code in its review diff.

Summary by CodeRabbit

  • Removed Providers

    • Removed support for the Crof provider from the desktop app, CLI, settings, provider selection, usage dashboard actions, icons, and API-key configuration.
    • Existing Crof configuration is no longer recognized as an active provider.
  • Bug Fixes

    • Settings now normalize provider names and aliases consistently.
    • Unknown or retired providers are ignored during settings loading.
    • Provider lists and metrics are deduplicated and restricted to valid providers, while recognized configuration is preserved.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 6256e4da-25da-480c-82db-3e1249d35322

📥 Commits

Reviewing files that changed from the base of the PR and between bca91ac and 1ccb4de.

⛔ Files ignored due to path filters (3)
  • apps/desktop-tauri/src/components/providers/icons/ProviderIcon-crof.svg is excluded by !**/*.svg
  • rust/assets/icons/ProviderIcon-crof.svg is excluded by !**/*.svg
  • rust/src/cli/serve/dashboard/icons/ProviderIcon-crof.svg is excluded by !**/*.svg
📒 Files selected for processing (14)
  • apps/desktop-tauri/src/components/providers/providerIcons.test.ts
  • apps/desktop-tauri/src/components/providers/providerIcons.ts
  • apps/desktop-tauri/src/surfaces/TrayPanel.tsx
  • apps/desktop-tauri/src/surfaces/settings/tabs/ProvidersTab.tsx
  • apps/desktop-tauri/src/test/providerCatalog.ts
  • rust/src/cli/serve/dashboard/icons.rs
  • rust/src/core/provider.rs
  • rust/src/core/provider_factory.rs
  • rust/src/core/token_accounts.rs
  • rust/src/providers/crof/mod.rs
  • rust/src/providers/mod.rs
  • rust/src/settings/api_keys.rs
  • rust/src/settings/raw.rs
  • rust/src/settings/tests.rs
💤 Files with no reviewable changes (8)
  • apps/desktop-tauri/src/components/providers/providerIcons.ts
  • rust/src/cli/serve/dashboard/icons.rs
  • apps/desktop-tauri/src/test/providerCatalog.ts
  • rust/src/providers/mod.rs
  • rust/src/core/token_accounts.rs
  • rust/src/settings/api_keys.rs
  • apps/desktop-tauri/src/surfaces/settings/tabs/ProvidersTab.tsx
  • rust/src/providers/crof/mod.rs

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Crof provider retirement

Layer / File(s) Summary
Provider core retirement
rust/src/core/provider.rs, rust/src/core/provider_factory.rs, rust/src/core/token_accounts.rs, rust/src/providers/..., rust/src/settings/api_keys.rs
The Crof provider ID, implementation, factory wiring, exports, token-account handling, and API-key metadata are removed.
Settings load sanitization
rust/src/settings/raw.rs, rust/src/settings/tests.rs
Settings loading canonicalizes recognized provider IDs, ignores invalid IDs, resolves aliases, deduplicates lists, and omits retired provider data.
Desktop Crof surfaces
apps/desktop-tauri/src/components/providers/..., apps/desktop-tauri/src/surfaces/..., apps/desktop-tauri/src/test/..., rust/src/cli/serve/dashboard/icons.rs
Crof icons, dashboard actions, source hints, test catalog data, and related registry entries are removed or updated.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~30 minutes

Change: Other

Suggested reviewers: darknight105

Merge Risk: ⚪ Minimal · up to 1ccb4

The PR safely removes Crof while preserving recognized settings and normalizing provider identifiers; no merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: safely retiring the Crof provider across the application.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Finesssee

Copy link
Copy Markdown
Collaborator Author

Thermo-nuclear code-quality review

Verdict: FINDINGS

P2 — canonicalize provider IDs instead of preserving aliases

rust/src/settings/raw.rs:338

The new is_known_provider predicate uses ProviderId::from_cli_name, which accepts aliases and mixed case, but the filters later in the conversion retain the original string. This leaves settings in a known-but-noncanonical state. For example, "openai" passes validation as Codex, while is_provider_enabled(ProviderId::Codex) looks for "codex" and therefore treats it as disabled; metric lookups have the same canonical-name assumption.

Please normalize accepted values to provider_id.cli_name() at this load boundary, with deterministic collision handling for maps. A shared canonicalization helper should cover enabled providers, provider metrics, and float-bar IDs, removing the split invariant. Add an alias or mixed-case regression alongside the Crof test.

No production file crosses from below 1,000 lines to above it. The compatibility logic remains in the settings deserialization boundary rather than leaking across callers.

@Finesssee
Finesssee deleted the branch main September 22, 2026 14:50
@Finesssee Finesssee closed this Sep 22, 2026
@Finesssee Finesssee reopened this Sep 22, 2026
@Finesssee
Finesssee changed the base branch from codex/port-0.63.0-pi to main September 22, 2026 14:52
@Finesssee
Finesssee force-pushed the codex/port-0.64.1-retire-crof branch from 4969ebb to 1ccb4de Compare September 22, 2026 14:56
@Finesssee

Copy link
Copy Markdown
Collaborator Author

Thermo-nuclear current-head review

PASS at 1ccb4de0c.

Provider IDs are normalized at the load boundary, metric collisions resolve deterministically, retired Crof configuration is ignored through the generic provider deserialization boundary, no Crof-specific production spaghetti remains, and no production file crosses the 1,000-line threshold. Current CircleCI and CodeRabbit checks are green.

@Finesssee

Copy link
Copy Markdown
Collaborator Author

Thermo integration follow-up: combining Crof retirement with #603 exposed two additional persisted provider references, stacked_tray_top_provider and stacked_tray_bottom_provider. #610 now canonicalizes both with the existing provider-ID helper, drops retired/unknown IDs, and preserves known provider choices even when temporarily disabled. Alias, retired-ID, enablement, and round-trip regressions cover this boundary. Independent source review found no remaining blocker in the correction.

The corrected combined backend suite passed 2,221 tests (one pre-existing ignored). Fresh Windows CUA proof on the integrated desktop build also confirms searching the provider catalog for Crof produces “No matching providers”; the new adapters from #604 remain registered. The final combined CircleCI gate is required before landing.

Local native evidence: C:\Users\mac\AppData\Local\Temp\wcb-ci-recovery-20260923-01a0ce34\integration-crof-removed.png and integration-crof-removed-state.json. Settings backup and restoration are tracked with the proof run; no credentials are included in this comment.

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