feat: add ScottyShellProvider for shell command execution in scotty containers

[stmh]

Summary

Add ScottyShellProvider to enable shell command execution in scotty containers via scottyctl app:shell. This allows all phabalicious commands (deploy, backup, shell, etc.) to execute inside scotty services.

Changes

• ScottyShellProvider: New shell provider extending LocalShellProvider
  • Wraps commands with scottyctl app:shell
  • Validates scotty.shellService configuration
  • File operations throw RuntimeException (not yet supported by scottyctl)
• ScottyMethod: Sets shellProvider: scotty as default and validates shellService
• ShellProviderFactory: Registers the new scotty shell provider
• CompletionCommand: Fixed return type for PHP 8+ compatibility
• Global Config Merging: Properly merges global scotty settings with host-specific settings

Configuration

Hosts with needs: [scotty] automatically get shellProvider: scotty

Global configuration is properly inherited by all hosts.

Testing

• All 163 tests pass
• New test host hostShellProvider demonstrates scotty shell provider usage
• Global configuration properly inherited by hosts

Documentation

Updated docs/scotty.md with shell provider integration section, shellService configuration, and usage examples.

Issue: phab-93f21d

[claude]

Pull Request Review: ScottyShellProvider Implementation

I've reviewed this PR and overall it's a solid implementation that follows the project's conventions well. Here are my findings:

✅ Strengths

1. Architecture & Design

   • Proper inheritance from LocalShellProvider - good reuse of existing functionality
   • Clean separation of concerns with dedicated ScottyCtlOptions and ScottyCtlCreateOptions classes
   • Follows the existing shell provider pattern consistently
   • Integration with ShellProviderFactory is straightforward

2. Configuration & Validation

   • Good use of the validation framework to ensure required fields are present
   • Proper merging of global and host-specific scotty settings in ScottyMethod.php:43-46
   • Access token is properly registered for obfuscation (ScottyCtlOptions.php:31-33)
   • Comprehensive documentation with examples

3. Test Coverage

   • Extensive test suite with 5 test cases covering different scenarios
   • Tests verify configuration validation, inheritance, scaffolding, and command building
   • Test assets properly organized

4. Documentation

   • Excellent, comprehensive documentation in docs/scotty.md
   • Clear examples for all features
   • Good troubleshooting section

🐛 Issues Found

1. Critical: Logic Error in CompletionCommand.php:96

Missing else keyword causes both branches to execute when both option and argument are set. Line 96 should be elseif not if.

Current code has two consecutive if statements when it should be if/elseif.

2. Potential Issue: Shell Escaping in ScottyShellProvider

File: src/ShellProvider/ScottyShellProvider.php:83

The program_to_call array is imploded with spaces without proper escaping. If any element contains spaces or special characters, this could lead to command injection or unexpected behavior.

Recommendation: Either properly escape each element before imploding, pass the array through a shell escaping function, or document that callers are responsible for escaping.

3. Error Handling: RuntimeException for File Operations

Files: src/ShellProvider/ScottyShellProvider.php:97,102

The method signature returns bool but always throws an exception. This could cause confusion.

Recommendations:

• Consider creating a more specific exception type
• Document this limitation in the class-level PHPDoc
• Consider if there's a better way to signal unsupported operations in the interface

⚠️ Minor Issues & Suggestions

4. Code Style: Remove auto-generated TODO comment at ScottyMethod.php:64

5. Missing Type Hints: ShellProviderFactory.php:9 could benefit from nullable return type since it can return null

6. Documentation: File operation limitations could be more prominently featured

🔒 Security Considerations

Good:

• Access tokens properly obfuscated in logs
• Password manager integration for secrets
• No obvious command injection vulnerabilities in main code paths

Watch:

• The command building in getShellCommand() should be verified with actual scottyctl to ensure proper escaping

🧪 Test Coverage Assessment

Coverage is good with tests for configuration validation, inheritance, scaffolding, and command building.

Missing tests:

• ScottyShellProvider unit tests (only ScottyMethod is tested)
• Error cases (missing required config, file operation exceptions)

📝 Recommendations

1. HIGH PRIORITY: Fix the CompletionCommand.php:96 logic error
2. MEDIUM PRIORITY: Review shell escaping in getShellCommand()
3. LOW PRIORITY: Clean up TODO comments and improve type hints
4. CONSIDER: Adding unit tests specifically for ScottyShellProvider
5. CONSIDER: Creating a custom exception type for unsupported operations

Summary

This is a well-implemented feature that integrates cleanly with the existing architecture. The main concern is the logic error in CompletionCommand.php which should be fixed before merging. The shell escaping issue should also be reviewed to ensure security. Otherwise, the code quality is high and follows project conventions well.

Recommendation: Request changes to address the critical logic error, then approve after fixes.

[claude]

Code Review: ScottyShellProvider Integration

I've reviewed PR #371 and overall this is a well-implemented feature that adds shell provider support for Scotty containers. Here's my detailed feedback:

Strengths

1. Good architectural fit: The ScottyShellProvider properly extends LocalShellProvider, following the established pattern used by DockerExecShellProvider and KubectlShellProvider.

2. Comprehensive test coverage: The test file includes 6 test cases covering validation, inheritance, scaffolding, and various option configurations.

3. Excellent documentation: The docs/scotty.md file is thorough with clear examples, configuration reference, troubleshooting guide, and security considerations.

4. Proper validation: Both ScottyShellProvider and ScottyMethod validate required configuration keys with helpful error messages.

5. Global config merging: The implementation properly merges global scotty settings with host-specific settings.

Issues Found

1. Critical: Logic error in CompletionCommand.php:96

Missing else - if both option and argument are set, return gets overwritten. Change line 96 from if to elseif.

2. Security: Potential command injection in ScottyShellProvider.php:83

The implode of program_to_call without escaping could lead to command injection. Compare with DockerExecShellProvider.php:64-66 which adds elements individually, or ensure proper escaping if scottyctl requires a single string.

3. Code Quality: TODO comment in ScottyMethod.php:65

IDE-generated TODO comment should be removed.

Suggestions

• Add null checks in getShellCommand before accessing hostConfig values
• Consider adding unit tests for shell command generation
• Add comment to invalid test host explaining it's intentionally invalid

Summary

Must Fix Before Merge:

1. Fix if/else logic error in CompletionCommand.php:96
2. Address command injection concern in ScottyShellProvider.php:83
3. Remove TODO comment from ScottyMethod.php:65

Overall, this is a solid implementation that follows project conventions well. The critical issues are fixable and once addressed, this will be a valuable addition to phabalicious.

[claude]

PR Review: ScottyShellProvider Integration

CRITICAL: Command Injection Vulnerability
File: src/ShellProvider/ScottyShellProvider.php:83
Issue: program_to_call array joined without escaping
Fix: Use array_map('escapeshellarg', $program_to_call) before implode

MEDIUM: Logic Bug in CompletionCommand
File: src/Command/CompletionCommand.php:96
Issue: Missing else before second if statement
Fix: Change to elseif

MEDIUM: Undefined array key in ScottyCtlOptions.php:30
Fix: Use !empty() instead of if($this->data['access-token'])

MINOR: Remove TODO comment in ScottyMethod.php:67

Overall: 7.5/10 - Solid PR with good architecture but must fix command injection before merge

[claude]

Pull Request Review - ScottyShellProvider

Thank you for this comprehensive PR! This is a well-structured feature addition with good test coverage.

Strengths

1. Solid Architecture - ScottyShellProvider extending LocalShellProvider is clean
2. Comprehensive Testing - 163 tests passing, covers validation, inheritance, scaffolding
3. Good Documentation - Updated docs/scotty.md
4. Proper Validation - Required keys enforced
5. Security Conscious - Access tokens properly obfuscated

Critical Issues

1. Missing elseif in CompletionCommand.php line 96

Both if blocks can execute, causing logic bug. Should be elseif.

2. Command Injection in ScottyShellProvider line 83

program_to_call joined without escaping. Use array_map with escapeshellarg.

3. File path injection in exists() line 91

File parameter not escaped in stat command.

Security Concerns

• Access token validation uses truthy check instead of isset
• Hardcoded test path in fabfile.yaml line 4
• Authentication skipped for configured tokens without validation

Recommendations

Must Fix: elseif bug, command injection, test config
Should Fix: Shell escaping, null checks
Nice to Have: More error tests, direct shell provider tests

Overall: Approve with required changes. Great implementation!

[claude]

Pull Request Review: ScottyShellProvider Integration

Thank you for this comprehensive feature addition! Overall, this is a well-structured implementation that follows phabalicious patterns.

Strengths

Architecture and Design

• Clean abstraction: Extending LocalShellProvider is the right approach
• Proper integration: Registration in ShellProviderFactory follows established patterns
• Configuration inheritance: Global scotty settings merging (ScottyMethod.php:46-49) is well-implemented
• Good separation of concerns: Shell provider handles command execution, Method handles lifecycle

Code Quality

• Type safety: Proper type hints throughout
• Error handling: Throws RuntimeException for unsupported operations - clear failure mode
• Validation: Comprehensive config validation in both Method and ShellProvider
• Documentation: Excellent scotty.md documentation with clear examples

Testing

• Good coverage: Tests cover config validation, inheritance, scaffolding, and secret resolution
• Test organization: Clean test assets structure

Security Concerns

1. Command Injection Risk (HIGH PRIORITY)

Location: src/ShellProvider/ScottyShellProvider.php:83

The program_to_call array is joined with spaces and passed as a single string. If any element contains shell metacharacters, this could lead to command injection.

Recommendation: Use escapeshellarg() around the implode result.

2. Token Exposure in Logs

Location: src/ShellProvider/ScottyShellProvider.php:55-65

Access tokens appear in command arrays that may be logged.

Recommendation: Consider using environment variables for tokens or redact in logs.

3. exists() Implementation

Location: src/ShellProvider/ScottyShellProvider.php:91

Missing shell escaping on file parameter - potential command injection if filename contains special characters.

Fix: Use escapeshellarg() on the file variable.

Potential Bugs

1. Missing elseif

Location: src/Command/CompletionCommand.php:96

Second condition should be elseif, not if - otherwise both branches execute.

2. TODO Comment

Location: src/Method/ScottyMethod.php:67

Remove the TODO comment.

3. Test Coverage Gap

Missing unit tests specifically for ScottyShellProvider. Consider adding tests/ScottyShellProviderTest.php

Recommendations Priority

Must Fix (Before Merge)

1. Security: Fix command injection in getShellCommand() line 83
2. Security: Fix command injection in exists() line 91
3. Bug: Fix missing elseif in CompletionCommand.php:96

Should Fix (Before Merge)

4. Security: Document/mitigate token exposure in logs
5. Cleanup: Remove TODO comment

Nice to Have (Future PR)

6. Testing: Add ScottyShellProvider unit tests
7. Docs: Add troubleshooting section

Summary

Overall Assessment: This is a solid, well-architected addition to phabalicious. The main concerns are security-related that should be addressed before merge.

Code Quality: 4/5
Security: Needs fixes
Test Coverage: 3/5
Documentation: 5/5

Recommendation: Request changes to address security issues, then approve once fixed.

Great work on this feature! The integration is clean and follows phabalicious patterns well.

[claude]

Pull Request Review - ScottyShellProvider

Thank you for this comprehensive implementation! This is a well-structured addition that follows existing patterns in the codebase.

Strengths

Architecture & Design:

• Excellent abstraction extending LocalShellProvider
• Follows established shell provider patterns (Docker, SSH, Kubectl)
• Smart defaults: auto-sets shellProvider when needs scotty
• Clean separation with ScottyCtlOptions class

Security:

• Proper secret management integration
• Tokens registered for obfuscation
• Smart auth verification using app:list
• Secret resolution works correctly

Code Quality:

• Comprehensive config validation
• Appropriate error handling
• Excellent inline documentation
• Good use of PHP 8+ features

Testing:

• 7 comprehensive test methods
• Multiple host configurations
• Integration tests with real configs

Documentation:

• Thorough scotty.md updates
• Clear examples and use cases

Issues Found

1. Logic Error in CompletionCommand.php:96 (MEDIUM)
Missing else between if statements - if both option and argument are truthy, argument overrides option results.
Fix: Change line 96 from "} if" to "} elseif"

2. Missing Null Check in ScottyCtlOptions.php:30 (LOW)
Line checks array key without isset. Should use: if (!empty($this->data['access-token']))

3. Inconsistent Null Handling (LOW)
Different null-checking patterns used throughout. Consider consistency.

4. Hardcoded Path in Test Config (LOW - Test Only)
fabfile.yaml has /Users/stephan/dev/rust/scotty path - won't work on other systems

5. Command Injection Note (INFO)
Line 68 in ScottyShellProvider - implode program_to_call. Likely sanitized upstream but add comment.

6. URL Validation Missing (LOW)
buildCommand doesn't validate server URL format

7. Documentation Gap
Global config merge not clearly documented with examples

Recommendation

APPROVE with minor fixes

Only blocking issue is the CompletionCommand logic error. Others are minor improvements.

Before Merging:

1. Fix else if bug in CompletionCommand.php:96
2. Fix null check in ScottyCtlOptions.php:30

Post-Merge:

• Add global config examples to docs
• Consider other enhancements

Great work! The core functionality is solid and well-tested.

[claude]

Code Review - PR #371: ScottyShellProvider

Overall, this is a well-implemented feature with good test coverage and documentation. The code follows existing architecture patterns and integrates cleanly with the shell provider system.

Strengths

1. Clean Architecture Integration - ScottyShellProvider properly extends LocalShellProvider and follows existing patterns
2. Good Security Practices - Proper secret resolution via PasswordManager with test coverage
3. Excellent Test Coverage - Comprehensive unit tests with 163 tests passing
4. Good Error Handling - Clear RuntimeException for unsupported file operations with helpful messages
5. Documentation - Updated docs/scotty.md with shell provider integration details

Issues Found

CRITICAL: Missing elseif in CompletionCommand.php:96

Line 96 has: } if ($argument) {
Should be: } elseif ($argument) {

Impact: When $option is set, the code will still check $argument and potentially overwrite the result, breaking shell completion logic.

Minor: Unused TODO Comment in ScottyMethod.php:67

Remove the TODO comment - the implementation is complete.

Design Considerations

1. File Operations - putFile() and getFile() throw RuntimeException. This is acceptable but may cause confusion during deployment workflows. Consider adding a warning during configuration validation if file-heavy methods are used.

2. Authentication Check Timing - The auth check runs once per config. Long-running processes might have tokens expire mid-execution. Consider documenting this behavior or adding a token refresh mechanism.

Test Coverage

Excellent coverage for configuration validation, inheritance, secret resolution, and ScottyCtlOptions building.

Missing: No unit tests for ScottyShellProvider itself. Recommend adding tests for putFile/getFile exceptions, getShellCommand() structure, and exists() method.

Security Review

No security concerns identified:

• Secret handling is correct
• Command injection prevention via array-based commands
• Input validation is comprehensive
• No hardcoded credentials

Performance

No performance issues:

• Authentication check cached per config
• Efficient command building
• No N+1 queries or loops

Conclusion

This is a high-quality PR that adds valuable functionality. After fixing the critical elseif bug in CompletionCommand.php:96, this should be ready to merge.

Recommendation: Approve after fixing the CompletionCommand bug

Great work on the implementation, documentation, and test coverage!