add model command to set default model

[cytown]

📝 Description

Now we can use picoclaw model <new_model_name> to change the default model in config.json.

🗣️ 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: Hackintosh
• OS: macOS 13.6.3
• 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.

[cytown]

original is #1072, wrongly closed...

[cytown]

@yofabr
sorry for review again...

[yinwm]

PR Review: Add model command to set default model

Thanks for the contribution! Overall this is a well-structured implementation with good test coverage. Here are my findings:

---

✅ What's Good

1. Clean code structure - Well organized using cobra framework
2. Comprehensive tests - 446 lines of tests covering various scenarios
3. Good backward compatibility - Handles both ModelName and legacy Model field
4. Nice UX - Clear output format with > marker for current model
5. Reasonable config.go changes - Making model_name non-omitempty and model omitempty correctly reflects the field migration direction

---

⚠️ Issues & Suggestions

1. [Bug] Incomplete marker logic in listAvailableModels

// command.go:70-72
if model.ModelName == cfg.Agents.Defaults.ModelName {
    marker = "> "
}

This only compares ModelName, but if a user's config uses the legacy Model field (without ModelName), the marker won't work correctly. Should handle it the same way as showCurrentModel:

defaultModel := cfg.Agents.Defaults.ModelName
if defaultModel == "" {
    defaultModel = cfg.Agents.Defaults.Model
}
// Then compare with defaultModel

2. [Minor] local-model lacks documentation

if !modelFound && modelName != LocalModel {
    return fmt.Errorf("Model '%s' not found in config.", modelName)
}

local-model is a special value that doesn't need to exist in model_list, but users may not understand what it means. Consider adding an explanation in the command's Long description.

3. [Minor] Inconsistent error message formatting

return fmt.Errorf("Model '%s' not found in config.", modelName)  // Has period
return fmt.Errorf("failed to load config: %w", err)              // No period
return fmt.Errorf("failed to save config: %w", err)              // No period

Suggest unifying the style.

4. [Nit] Test helper could reduce boilerplate

The stdout capture code is repeated many times in tests. Could be extracted into a helper function.

---

📊 Summary

Category	Status
Functionality	✅ Complete
Code Quality	⚠️ Minor issues
Test Coverage	✅ Good
Backward Compat	✅ Handled well
Documentation	⚠️ Missing local-model explanation

Recommendation: Fix issue #1 (the bug) before merging. Others are optional improvements.

[cytown]

PR Review: Add model command to set default model

Thanks for the contribution! Overall this is a well-structured implementation with good test coverage. Here are my findings:

✅ What's Good

1. Clean code structure - Well organized using cobra framework
2. Comprehensive tests - 446 lines of tests covering various scenarios
3. Good backward compatibility - Handles both ModelName and legacy Model field
4. Nice UX - Clear output format with > marker for current model
5. Reasonable config.go changes - Making model_name non-omitempty and model omitempty correctly reflects the field migration direction

⚠️ Issues & Suggestions

1. [Bug] Incomplete marker logic in listAvailableModels

// command.go:70-72
if model.ModelName == cfg.Agents.Defaults.ModelName {
    marker = "> "
}

This only compares ModelName, but if a user's config uses the legacy Model field (without ModelName), the marker won't work correctly. Should handle it the same way as showCurrentModel:

defaultModel := cfg.Agents.Defaults.ModelName
if defaultModel == "" {
    defaultModel = cfg.Agents.Defaults.Model
}
// Then compare with defaultModel

2. [Minor] local-model lacks documentation

if !modelFound && modelName != LocalModel {
    return fmt.Errorf("Model '%s' not found in config.", modelName)
}

local-model is a special value that doesn't need to exist in model_list, but users may not understand what it means. Consider adding an explanation in the command's Long description.

3. [Minor] Inconsistent error message formatting

return fmt.Errorf("Model '%s' not found in config.", modelName)  // Has period
return fmt.Errorf("failed to load config: %w", err)              // No period
return fmt.Errorf("failed to save config: %w", err)              // No period

Suggest unifying the style.

4. [Nit] Test helper could reduce boilerplate

The stdout capture code is repeated many times in tests. Could be extracted into a helper function.

📊 Summary

Category Status
Functionality ✅ Complete
Code Quality ⚠️ Minor issues
Test Coverage ✅ Good
Backward Compat ✅ Handled well
Documentation ⚠️ Missing local-model explanation
Recommendation: Fix issue #1 (the bug) before merging. Others are optional improvements.

done

[yinwm]

Follow-up Review

Thanks for addressing the previous feedback! The marker logic bug in listAvailableModels has been fixed correctly.

---

⚠️ Still Needs Fix

Error message grammar issue

File: command.go:95

return fmt.Errorf("can not found model '%s' in config", modelName)

"can not found" is grammatically incorrect. Should be:

return fmt.Errorf("cannot find model '%s' in config", modelName)
// or
return fmt.Errorf("model '%s' not found in config", modelName)

---

💡 Suggested Improvement

local-model lacks documentation

local-model is a special value that bypasses the APIKey check, designed for local VLLM servers. However, users won't discover this feature from --help.

Consider adding to the Long description:

Long: `Show or change the default model configuration.

If no argument is provided, shows the current default model.
If a model name is provided, sets it as the default model.

Examples:
  picoclaw model                    # Show current default model
  picoclaw model gpt-5.2           # Set gpt-5.2 as default
  picoclaw model claude-sonnet-4.6 # Set claude-sonnet-4.6 as default
  picoclaw model local-model       # Set local VLLM server as default

Note: 'local-model' is a special value for using a local VLLM server
(running at localhost:8000 by default) which does not require an API key.`,

---

📊 Summary

Issue	Severity	Status
Marker logic bug	🔴 Required	✅ Fixed
Error message grammar	🟡 Suggested	❌ Pending
local-model docs	🟢 Optional	❌ Pending

Recommendation: Fix the error message grammar before merging. Documentation improvement is optional.

[cytown]

Long: `Show or change the default model configuration.

If no argument is provided, shows the current default model.
If a model name is provided, sets it as the default model.

Examples:
  picoclaw model                    # Show current default model
  picoclaw model gpt-5.2           # Set gpt-5.2 as default
  picoclaw model claude-sonnet-4.6 # Set claude-sonnet-4.6 as default
  picoclaw model local-model       # Set local VLLM server as default

Note: 'local-model' is a special value for using a local VLLM server
(running at localhost:8000 by default) which does not require an API key.`,

all done

[yinwm]

Review Summary

✅ Approved

The feature is complete with good test coverage. Ready to merge.

Pros

• Clean command design for viewing and setting default model
• Proper backward compatibility handling for model_name and legacy model field
• Comprehensive test coverage (369 lines of tests)

Minor issues (non-blocking, can be fixed in follow-up)

1. command.go:107 - "cannot found" is grammatically incorrect, should be "cannot find" or "not found"
2. getDefaultModel logic is duplicated in multiple places, could be extracted to a helper function

Good job! 🎉