Skip to content

fix: Update demo CLI to use ModelRegistry.resolve pattern (#228)#229

Merged
andreasronge merged 1 commit intomainfrom
claude/issue-228-20251211-113729
Dec 11, 2025
Merged

fix: Update demo CLI to use ModelRegistry.resolve pattern (#228)#229
andreasronge merged 1 commit intomainfrom
claude/issue-228-20251211-113729

Conversation

@andreasronge
Copy link
Copy Markdown
Owner

Summary

Replace legacy Map.get(presets, name) pattern with ModelRegistry.resolve/1 in the /model command handler of demo/lib/ptc_demo/cli.ex. This aligns with the pattern already used in JsonCLI and LispCLI.

Changes

  • Updated handle_input("/model " <> name) function to use PtcDemo.ModelRegistry.resolve/1
  • Replaced silent fallback behavior with explicit error handling
  • Model resolution now validates against known presets and custom model aliases
  • Consistent error messages when resolution fails

Closes #228

Replace legacy Map.get(presets, name) with ModelRegistry.resolve(1)
in the /model command handler. This aligns with the pattern used in
JsonCLI and LispCLI, ensuring consistent model resolution across all
CLI tools with explicit error handling.

Closes #228

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

PR Review: fix: Update demo CLI to use ModelRegistry.resolve pattern

Summary

Small tech-debt cleanup that aligns demo/lib/ptc_demo/cli.ex with the ModelRegistry.resolve/1 pattern established in PR #227. The implementation correctly mirrors the existing JsonCLI and LispCLI handlers.

What's Good

  • Exact pattern match: The updated /model command handler at cli.ex:103-115 is identical in structure to json_cli.ex:126-138 and lisp_cli.ex:126-138, ensuring consistency across all CLI implementations
  • Correct function choice: Uses ModelRegistry.resolve/1 directly rather than CLIBase.resolve_model/1 which would incorrectly exit the program on error (per issue discussion)
  • Explicit error handling: Invalid model names now show clear error messages instead of silently passing through as raw strings
  • Clean diff: Net zero lines changed, pure refactor with no scope creep

Issues (Must Fix)

None identified.

Suggestions (Optional)

None - the PR does exactly what's needed, no more, no less.

Security

No concerns. The change is internal to demo code and doesn't affect any validation logic.

Documentation

No updates needed. This is an internal refactor to demo CLI code; the public-facing behavior (model switching) is unchanged.

Verdict

Approve - Clean, mechanical change that brings cli.ex into alignment with the established pattern. The implementation matches the issue requirements exactly.

@github-actions github-actions bot added the auto-triage-pending Auto-triage in progress label Dec 11, 2025
@andreasronge
Copy link
Copy Markdown
Owner Author

Auto-Triage Summary

Review Analysis

The automated PR review found zero issues requiring action:

  • Must Fix: None
  • Suggestions: None
  • Security: No concerns
  • Documentation: No updates needed

Verdict

Approve - Clean, mechanical change that brings cli.ex into alignment with the established ModelRegistry.resolve pattern from PR #227.

Status

  • FIX_NOW items: 0
  • Issues created: 0
  • Items ignored: 0

This PR is ready to merge pending CI checks.

@andreasronge andreasronge added the ready-to-merge PR is ready to be merged label Dec 11, 2025
@github-actions github-actions bot added auto-triage-cycle-1 Auto-triage cycle 1 auto-triage-complete Auto-triage completed and removed auto-triage-pending Auto-triage in progress labels Dec 11, 2025
@andreasronge andreasronge enabled auto-merge (squash) December 11, 2025 11:40
@andreasronge andreasronge merged commit bc38d7a into main Dec 11, 2025
4 checks passed
@andreasronge andreasronge deleted the claude/issue-228-20251211-113729 branch December 11, 2025 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-triage-complete Auto-triage completed auto-triage-cycle-1 Auto-triage cycle 1 ready-to-merge PR is ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[From PR #227] Update legacy cli.ex to use ModelRegistry

1 participant