refactor: remove migrate_env_credentials and MigrationResult#2774
Open
tusharmath wants to merge 2 commits intomainfrom
Open
refactor: remove migrate_env_credentials and MigrationResult#2774tusharmath wants to merge 2 commits intomainfrom
tusharmath wants to merge 2 commits intomainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove the
migrate_env_credentialsone-time migration path that existed to move API keys from environment variables to the file-based credentials store, along with theMigrationResultdomain type and all associated plumbing.Context
When file-based credential storage was first introduced, a migration helper was added so that users with existing
OPENAI_API_KEY/ANTHROPIC_API_KEYenvironment variables would have those credentials automatically written to the credentials file on the first run. That migration has now served its purpose — any user who was affected has already been migrated — making this code dead weight. Keeping it in the API trait, service layer, repository layer, and UI creates unnecessary surface area and maintenance burden.Changes
forge_domain/src/migration.rs— removes theMigrationResultstruct and its module entirely.migrate_env_credentialsfromProviderRepositorytrait (forge_domain/src/repo.rs) and all implementations/mocks acrossforge_repo,forge_app,forge_services, andforge_api.migrate_env_to_filefromForgeProviderRepository(forge_repo/src/provider/provider_repo.rs) along with the now-deadUrlParamVarConfig::param_namehelper and env-scanning logic (~100 lines).handle_migrate_credentialsfromUI(forge_main/src/ui.rs) and its call-site ininit_state; theInfocommand now correctly callson_new()to initialise the model instead of the lighterinit_state(false).generate_commit_message(forge_app/src/git_app.rs) — dropped the retry wrapper and theClonederive onDiffContext; also removed the now-redundant empty-message error guard.total_linesreporting inoperation.rs— the attribute now reflects the number of lines actually returned to the caller rather than the total line count of the full file, correcting snapshot tests accordingly.Key Implementation Details
The migration function iterated environment variables, matched known provider patterns, and wrote them to a JSON credentials file if the file did not yet exist. All of this logic lived inside
ForgeProviderRepository::migrate_env_to_file. Removing it required stripping the method from every layer: domain trait → repository implementation → service wrapper → API trait → UI call-site. Mock implementations in tests were updated to drop the stub.Testing
Links