Skip to content

refactor Config to add Version and migratable#1352

Merged
yinwm merged 14 commits intosipeed:mainfrom
cytown:version
Mar 23, 2026
Merged

refactor Config to add Version and migratable#1352
yinwm merged 14 commits intosipeed:mainfrom
cytown:version

Conversation

@cytown
Copy link
Copy Markdown
Contributor

@cytown cytown commented Mar 11, 2026

📝 Description

Removed usage of the Providers.AuthMethod field

cmd/picoclaw/internal/auth/helpers.go - Removed assignment to Providers..AuthMethod
web/backend/api/oauth.go - Removed synchronization of Providers.
.AuthMethod
pkg/voice/transcriber_test.go - Remove test cases for Providers.Groq
Configuration versioning migration feature

pkg/config/migration.go - Add loadConfigV0 and loadConfig functions to support automatic migration from legacy configurations
pkg/config/config.go - Modified to support versioned configurations
pkg/config/config_old.go - Added legacy configuration struct
Provider factory simplification

pkg/providers/factory.go - Significantly simplified, removed old provider logic
pkg/providers/factory_test.go - Removed related tests

Translated with DeepL.com (free version)

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

📚 Technical Context (Skip for Docs)

  • Reference URL:
  • Reasoning:

🧪 Test Environment

  • Hardware: Apple MacBook Pro (x86_64)
  • OS: macOS 12.6 (Darwin Kernel 22.6.0
  • Model/Provider:
  • Channels:

📸 Evidence (Optional)

Click to view Logs/Screenshots

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly.

@sipeed-bot sipeed-bot bot added type: enhancement New feature or request domain: config domain: provider go Pull requests that update go code labels Mar 11, 2026
Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Found 1 issue that should be fixed before merging:

Agents config migration may lose user settings

In pkg/config/config_old.go:612-615:

// Merge agents config if workspace is set
if c.Agents.Defaults.Workspace != "" {
    cfg.Agents = c.Agents
}

Problem: User's Agents config (e.g., Provider, Model, MaxTokens) is only copied when Workspace is non-empty. If a user configured Model or Provider but didn't set Workspace, their settings will be lost after migration.

Example scenario:

  • User has: {"agents": {"defaults": {"model": "gpt-4o", "provider": "openai"}}}
  • After migration: Uses default config (user's model/provider settings lost)

Suggested fix:

// Always copy user's Agents config
cfg.Agents = c.Agents

// Ensure Workspace has a default if not set
if cfg.Agents.Defaults.Workspace == "" {
    homePath := getHomePath() // or reuse logic from DefaultConfig
    cfg.Agents.Defaults.Workspace = filepath.Join(homePath, "workspace")
}

Other than this, the PR looks good:

  • Config versioning design is correct
  • Provider factory simplification is appropriate (old factory.go logic replaced by factory_provider.go)
  • Migration of ProvidersModelList works correctly

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@sky5454
Copy link
Copy Markdown
Contributor

sky5454 commented Mar 11, 2026

And, Singleton testing is best done by introducing old and new versions of the configuration file, and doing a real-world scenario test that can also be verified by CI

@cytown cytown requested review from sky5454 and yinwm March 12, 2026 05:58
@cytown
Copy link
Copy Markdown
Contributor Author

cytown commented Mar 12, 2026

And, Singleton testing is best done by introducing old and new versions of the configuration file, and doing a real-world scenario test that can also be verified by CI

add test for this.

Copy link
Copy Markdown
Contributor

@sky5454 sky5454 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed OK.

Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yinwm yinwm merged commit cff9065 into sipeed:main Mar 23, 2026
4 checks passed
@cytown cytown deleted the version branch March 23, 2026 07:41
andressg79 pushed a commit to andressg79/picoclaw that referenced this pull request Mar 30, 2026
refactor Config to add Version and migratable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: config domain: provider go Pull requests that update go code type: enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants