feat(core): Complete wizard type system foundation (CLI-4, CLI-5, CLI-6)

[dugshub]

Summary

This PR implements the complete foundational type system for CLI Patterns across three issues (CLI-4, CLI-5, CLI-6), establishing the core infrastructure for the wizard-based terminal framework.

What's Included

🎯 CLI-4: Semantic Type System

• Semantic types to prevent type confusion: BranchId, ActionId, OptionKey, MenuId
• Type guards and factory functions for runtime safety
• Collection types for type-safe data structures
• 520+ lines of comprehensive tests

🏗️ CLI-5: Pydantic Configuration Models

• WizardConfig: Top-level wizard definition
• BranchConfig: Individual wizard branches/screens
• Action configs: BashActionConfig, PythonActionConfig (discriminated union)
• Option configs: String, Select, Path, Number, Boolean (discriminated union)
• Unified SessionState: Shared state model for wizard and parser systems
• Result types: ActionResult, CollectionResult, NavigationResult
• 919+ lines of model tests

🔒 CLI-6: Security Hardening

• Command injection prevention in subprocess executor
• Input validation: String lengths, path traversal, numeric bounds
• DoS protection: Collection sizes, recursion depth, file sizes
• Production vs development validation modes
• Centralized security configuration
• 933+ lines of security tests

🔄 Parser Integration

• Migrated parser from custom Context to unified SessionState
• Updated 72 test files with correct attribute access patterns
• Resolved SemanticContext/SessionState compatibility issues

Architecture Decisions (ADR-008)

1. Framework Design: CLI Patterns is a framework users install, not an application
2. Discriminated Unions: Type-safe extensibility for actions and options
3. Protocol-Based: Runtime-checkable protocols for dependency injection
4. Unified State: Single SessionState model shared across all systems
5. Security-First: Built-in validation and attack prevention
6. Tree Navigation: Simplified tree structure (graph support deferred)
7. Global State: Options flow between branches with optional namespacing

Statistics

• 33 files changed: 6,473 insertions, 659 deletions
• 10 commits with semantic commit messages
• 782/782 tests passing (100% pass rate)
• Full MyPy strict mode compliance
• ~4,200 lines of production code + tests

Testing

make all  # Format, lint, type-check, test

All quality gates passing:

• ✅ Code formatting (black)
• ✅ Linting (ruff) - 0 errors
• ✅ Type checking (mypy strict) - 0 errors
• ✅ Tests (pytest) - 782/782 passed

Documentation

• ADR-008: Wizard Type System Architecture
• Follow-up Issues: Roadmap for next implementation phases
• Security Guide: Implementation guidance for security features

Next Steps

Per the follow-up issues document, the next priorities are:

1. YAML Definition Loader (~300-400 lines)
2. Python Decorator System (~500-600 lines)
3. Action Executors (Bash & Python)
4. Option Collectors Suite
5. Navigation Controller
6. CLI Entry Point

Breaking Changes

None - this is all new functionality building the foundation.

Security Considerations

All code includes comprehensive security hardening:

• Input validation with configurable limits
• Command injection prevention
• Path traversal protection
• DoS attack mitigation
• Separate dev/prod validation modes

---

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[dugshub]

@claude - please review this PR

[claude]

Claude finished @dugshub's task —— View job

---

PR Review: feat(core): Complete wizard type system foundation

Comprehensive review completed for this foundational PR implementing CLI-4, CLI-5, and CLI-6.

---

📊 Overview Assessment

Scope: 33 files changed, 6,473 additions, 659 deletions across 3 issues
Quality Gates: ✅ All passing (782/782 tests, MyPy strict, linting clean)
CI Status: ✅ Successful
Architecture: Well-documented with ADR-008

---

✅ Strengths

1. Excellent Type Safety (CLI-4)

• Semantic types properly implemented using NewType for BranchId, ActionId, OptionKey, MenuId
• Factory functions with optional validation (configurable via env vars)
• Type guards for runtime checks (is_branch_id, etc.)
• Collection types for type-safe data structures
• Clean separation prevents type confusion at compile time

Code Quality: src/cli_patterns/core/types.py:66-169 - Factory functions handle validation gracefully with clear error messages

2. Robust Pydantic Models (CLI-5)

• Discriminated unions properly implemented for actions and options using Literal["type"] discriminator
• BaseConfig pattern provides extensibility via metadata/tags
• Comprehensive validation with field validators and model validators
• SessionState unification successfully consolidates wizard and parser state
• Result types provide structured success/failure patterns

Code Quality: src/cli_patterns/core/models.py:96-127 - BashActionConfig validator properly checks for shell injection patterns

3. Strong Security Hardening (CLI-6)

Command Injection Prevention ⭐

• Regex-based validation blocks dangerous shell metacharacters when allow_shell_features=False
• SubprocessExecutor uses asyncio.create_subprocess_exec() (safe) by default instead of create_subprocess_shell()
• Clear security warnings and opt-in mechanism for shell features

Excellent implementation: src/cli_patterns/execution/subprocess_executor.py:148-180

DoS Protection

• JSON depth validation (max 50 levels) prevents stack overflow
• Collection size limits (max 1000 items) prevent memory exhaustion
• Validators module (validators.py) is well-structured and reusable
• Security config via environment variables with sensible defaults

Code Quality: src/cli_patterns/core/validators.py:41-158 - Recursive validators properly track depth/size

4. Comprehensive Testing

• 2,372 lines of test code across CLI-4, CLI-5, CLI-6 (3,152 total in core tests)
• Security tests thoroughly cover injection vectors
• Model validation tests check all constraint combinations
• Protocol compliance tests ensure implementations match contracts
• 100% test pass rate (782/782)

5. Excellent Documentation

• ADR-008 clearly documents architectural decisions and rationale
• Follow-up issues document provides clear roadmap
• Security guide (1,041 lines) offers implementation guidance
• Inline documentation is thorough and helpful

---

🔍 Issues & Concerns

1. Parser Migration Incomplete ⚠️

The parser migration to SessionState appears to have compatibility issues:

Issue: src/cli_patterns/ui/parser/semantic_context.py still exists and provides conversion between SemanticContext and SessionState, suggesting the migration may not be complete.

Evidence:

• Lines 45-77 show bidirectional conversion methods
• SemanticContext uses parser-specific semantic types (CommandId, ParseMode, ContextKey) not unified with core types
• Tests updated to use SessionState attributes, but SemanticContext wrapper still exists

Recommendation: Complete the parser migration as tracked in the follow-up issues document, or clarify if SemanticContext is intentionally kept as an adapter layer.

2. Semantic Type Duplication ⚠️

There appear to be two separate semantic type systems:

Core types (core/types.py): BranchId, ActionId, OptionKey, MenuId
Parser types (referenced in semantic_context.py): CommandId, ParseMode, ContextKey

Issue: This duplication could cause confusion. Are CommandId and ActionId meant to be the same concept?

Recommendation: Unify semantic types or clearly document why separate type hierarchies are needed.

3. StateValue Type Inconsistency 🔴

Critical Issue: Type definition inconsistency between modules:

• core/types.py:42 defines StateValue = JsonValue (union type)
• core/models.py:28 redefines StateValue = Any for Pydantic compatibility

Problem: This breaks type safety. MyPy sees two different definitions.

Evidence: Comment at models.py:26-27 acknowledges this is "for Pydantic compatibility"

Recommendation:

# In models.py, use the actual JsonValue type
from cli_patterns.core.types import StateValue as JsonStateValue
# Then use JsonStateValue in model fields instead of Any

4. Command Injection Validation Gaps ⚠️

While the regex validation in BashActionConfig is good, there are edge cases:

Missing patterns:

• Globbing (*, ?, [...]) - could expose files
• Brace expansion ({a,b,c})
• Tilde expansion (~/)
• Process substitution (<(), >())

Example: command="rm *.pyc" would pass validation but executes glob expansion

Recommendation: Either:

1. Add these patterns to the blocklist, OR
2. Document that glob patterns are allowed in "safe" mode (which may be reasonable)

5. Security Config Caching Issue 🟡

core/config.py:106-109 - Global config is cached permanently:

_security_config: SecurityConfig | None = None

def get_config() -> SecurityConfig:
    global _security_config
    if _security_config is None:
        _security_config = get_security_config()
    return _security_config

Issue: Config is loaded once and never refreshed, even if environment variables change. This could be confusing in long-running processes.

Recommendation: Document this behavior or add config refresh capability for testing/dynamic environments.

6. Validation Mode Confusion 🟡

Factory functions have a validate parameter with three states:

• True: Validate
• False: Skip validation
• None: Use global config

Issue: The None default creates a dependency on global state, and the tri-state boolean is non-obvious.

Example: make_branch_id("", validate=None) behavior depends on environment variable

Recommendation: Consider explicit modes or document this pattern prominently.

---

📝 Minor Improvements

Code Quality

1. Inconsistent error messages: Some validators raise ValueError, others raise ValidationError. Consider standardizing.

2. Magic numbers: Limits are hardcoded (100 branches, 50 options, etc.). Consider making these configurable via SecurityConfig.

3. Protocol documentation: protocols.py:117-121 has TODO comments for future protocols - these are helpful but could be tracked as issues.

Testing

1. Edge case coverage: Tests are comprehensive but could add:

   • Unicode handling in semantic types
   • Very long strings (near limit boundaries)
   • Nested discriminated unions

2. Integration tests: Only 2 integration test files for parser. Consider adding wizard-level integration tests.

Documentation

1. Type system guide: Users will need guidance on when to use semantic types vs strings. Consider adding a developer guide.

2. Security best practices: The security guide is excellent but could include threat modeling examples.

---

🎯 Recommendations

Required Before Merge

1. Fix StateValue type inconsistency (Issue Component Library Foundation #3 above) - This is a type safety violation
2. Clarify parser migration status (Issue Token System Definition #1) - Document if SemanticContext is intentional or temporary

Strongly Recommended

3. Document or fix command injection gaps (Issue Minimal Core Type Definitions #4) - Glob patterns may be a security concern
4. Unify semantic type systems (Issue Theme Architecture #2) - Reduce confusion between core and parser types

Nice to Have

5. Address validation mode confusion (Issue Bash Action Model Implementation #6)
6. Document config caching behavior (Issue Core Pydantic Models for Wizard Config #5)
7. Add developer guide for type system usage

---

🏆 Final Assessment

Overall Quality: ⭐⭐⭐⭐½ (4.5/5)

This is excellent foundational work with strong type safety, comprehensive security hardening, and thorough testing. The architecture is well-designed and the implementation quality is high.

Key Achievements:

• ✅ Complete semantic type system with runtime safety
• ✅ Discriminated unions for extensibility
• ✅ Security-first design with command injection prevention
• ✅ DoS protection with depth/size validation
• ✅ Unified SessionState across wizard and parser
• ✅ 100% test pass rate with comprehensive coverage
• ✅ Excellent documentation (ADR-008, security guide)

Remaining Concerns:

• 🔴 StateValue type inconsistency (must fix)
• ⚠️ Parser migration incomplete/unclear
• ⚠️ Semantic type duplication
• 🟡 Minor security gaps and config caching issues

Recommendation: Approve with required changes - Fix the StateValue type issue and clarify the parser migration status, then this is ready to merge. The foundation is solid and the follow-up roadmap is well-defined.

---

Job: https://github.com/pattern-stack/cli-patterns/actions/runs/18254549326
Branch: CLI-4-5-6-core-type-system