feat: Create CLIBase with shared CLI utilities

[andreasronge]

Summary

Implement issue #202 by creating demo/lib/ptc_demo/cli_base.ex with shared CLI utility functions extracted from LispCLI.

Changes

• 7 functions extracted to shared module:
  • load_dotenv/0 - Load environment variables from .env file
  • ensure_api_key!/0 - Validate API key environment variables
  • parse_common_args/1 - Parse common CLI arguments
  • resolve_model/2 - Resolve model names using preset maps
  • format_stats/1 - Format session statistics
  • format_number/1 - Format integers with thousand separators
  • format_cost/1 - Format costs with 6 decimal places (CLI precision)

Validation

• ✅ Code compiles without warnings
• ✅ All 1081 existing tests pass
• ✅ mix precommit passes (formatting, compilation, Credo)
• ✅ No workflow files modified
• ✅ Cost formatting uses 6 decimal places as specified

Fixes #202

🤖 Generated with Claude Code

[github-actions]

PR Review: Create CLIBase with shared CLI utilities

Summary

This PR creates a new PtcDemo.CLIBase module with 7 shared CLI utility functions extracted from LispCLI. The implementation follows the specification in demo/docs/test-runner-refactor-plan.md (Section 4: CLIBase) and correctly mirrors the existing implementations in lisp_cli.ex.

What's Good

• Correct structure: Module follows the exact specification from Section 4 of the refactor plan
• Accurate extraction: All 7 functions match the existing implementations in LispCLI (lines 424-513)
• Proper documentation: Good @moduledoc and @doc for all public functions with examples
• Correct precision: format_cost/1 uses 6 decimal places as specified in the issue (not the 4-decimal version in TestRunner.Base)
• Sensible design: resolve_model/2 takes presets as parameter for flexibility across different CLI modules

Issues (Must Fix)

1. Minor behavior difference in parse_common_args/1 - demo/lib/ptc_demo/cli_base.ex:95
   • Problem: When an unknown flag is encountered, CLIBase.parse_common_args/1 only prints the error message, but the original LispCLI.parse_args/1 (line 79) also prints the usage() text before exiting
   • Impact: When LispCLI is refactored to use CLIBase, users will lose the helpful usage information on invalid flags
   • Fix: Either (a) have parse_common_args/1 accept an optional :usage callback/string parameter, or (b) document that callers should catch/handle unknown flag errors themselves, or (c) accept this minor behavior change since this is Phase 1 (foundation only)
   • Complexity: Low - this is a minor UX detail that could be addressed in Phase 2 when LispCLI is actually refactored

Verdict on this issue: This is acceptable as-is for Phase 1. The current behavior is reasonable, and the usage printing can be added during Phase 2 (task 2.2) when LispCLI is refactored to actually use CLIBase. No blocking change needed.

Suggestions (Optional)

1. Doctest examples - Nice-to-have

   • Suggestion: The @doc examples for resolve_model/2, format_number/1, and format_cost/1 use iex> prefix but aren't runnable doctests (missing doctest PtcDemo.CLIBase in a test file)
   • Benefit: Would provide automatic documentation verification
   • Recommendation: Out of scope for this PR; can be added in Phase 1.5 (demo test infrastructure setup)

2. Consider guard clause for format_number/1 - Nice-to-have

   • Suggestion: The fallback clause def format_number(n), do: inspect(n) could explicitly handle nil and non-numeric values
   • Benefit: More defensive
   • Recommendation: Current behavior is fine and matches the original implementation

Security

No concerns. The functions handle environment variables and file paths appropriately. No user input is passed to dangerous operations.

Documentation

No updates needed. This is internal demo code, and the module has appropriate @moduledoc and @doc strings. The refactor plan document already describes this module.

Verdict

Approve ✅

The PR correctly implements task 1.4 from the refactor plan. The code is well-structured, properly documented, and exactly matches the specification. The minor difference in unknown flag handling is not a blocker since this is Phase 1 (foundation only) and the behavior will be refined when LispCLI is refactored in Phase 2.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Minor behavior difference in parse_common_args/1 - missing usage() on unknown flags	DEFER_ISSUE	Created issue #204 (to be handled in Phase 2, task 2.2)
2	Doctest examples suggestion	IGNORE	Reviewer explicitly deferred to Phase 1.5 (demo test infrastructure setup)
3	Consider guard clause for format_number/1	IGNORE	Reviewer stated "Current behavior is fine and matches the original implementation"

Rationale

Issue #1 - Usage callback (DEFER_ISSUE):

• Reviewer verdict: "This is acceptable as-is for Phase 1... No blocking change needed"
• Reviewer explicitly stated this can be addressed in Phase 2 (task 2.2) when LispCLI is refactored to use CLIBase
• Created issue [From PR #203] Add usage callback to parse_common_args for error messages #204 to track this for Phase 2

Issue #2 - Doctests (IGNORE):

• Reviewer recommendation: "Out of scope for this PR; can be added in Phase 1.5 (demo test infrastructure setup)"
• Will be handled as part of Phase 1.5 planning

Issue #3 - Guard clause (IGNORE):

• Reviewer assessment: "Current behavior is fine and matches the original implementation"
• This is an optional suggestion with no functional benefit

Status

• FIX_NOW items: 0
• Issues created: 1 ([From PR #203] Add usage callback to parse_common_args for error messages #204)
• Items ignored: 2

Next Steps

No blocking issues found. The PR is approved and ready to merge. Issue #204 will be addressed in Phase 2.