Add VHS integration tests with Require assertions

[yimsk]

Summary

• Add CI workflow for VHS integration tests (LocalStack + VHS)
• Add Require assertions to all VHS tapes for early failure detection
• New command-mode.tape for testing command suggestions/tab completion
• New Taskfile tasks: demo:record:theme-light, demo:record:command-mode, test:vhs

Test plan

• CI passes on push to develop/main
• task test:vhs runs all tapes locally (Linux only)

[claude]

Pull Request Review

This PR adds VHS integration tests with Require assertions and CI automation. Overall, this is a well-structured and valuable addition that improves test coverage and CI validation.

✅ Strengths

1. Excellent Test Coverage Enhancement

   • Adding Require assertions to all tapes provides early failure detection
   • New command-mode.tape thoroughly tests command suggestions and tab completion
   • Comprehensive coverage of different navigation patterns

2. Well-Structured CI Workflow

   • Clean separation of concerns with dedicated integration test workflow
   • Proper LocalStack health check with timeout and retry logic (lines 37-45)
   • Good use of timeout-minutes: 5 on setup step to prevent hanging
   • Artifact upload on failure for debugging (lines 72-77)
   • Proper cleanup with if: always() condition (lines 80-82)

3. Consistent Taskfile Updates

   • New tasks follow existing patterns perfectly
   • Proper dependency declarations
   • Good error handling with set -e in test:vhs task

4. Documentation

   • docs/tapes/README.md updated with clear usage instructions
   • Comments in tapes explain what each section tests

[claude]

🔍 Code Quality Observations

Minor Issues

1. CI Workflow - LocalStack Health Check (.github/workflows/integration-test.yml:38)

   • The health check only validates S3 service
   • Since the tapes test EC2 and VPC resources, consider checking those services too
   • Suggested improvement: Check for "(s3|ec2|vpc)" instead of just "s3"

2. Potential Race Condition (.github/workflows/integration-test.yml:49)

   • The localstack-demo-setup.sh script runs immediately after LocalStack starts
   • While there's a health check, there might be a race if services are still initializing
   • Consider adding a small delay (e.g., sleep 2) after the health check succeeds

3. Error Handling

   • Both Taskfile and CI workflow use set -e which is good
   • Consider adding set -euo pipefail for stricter error handling

4. command-mode.tape - Missing Final Verification (docs/tapes/command-mode.tape:83-84)

   • The tape ends with Type "q" but no Require to verify clean exit
   • Consider adding: Require "Services" or similar after quit if it returns to main screen

[claude]

🚀 Performance Considerations

1. CI Job Timeout

   • The VHS job has no global timeout - consider adding timeout-minutes: 15 at the job level
   • Individual tape runs could hang; the loop should have a per-tape timeout

2. LocalStack Image Version Pinning

   • Good practice to pin localstack/localstack:4.12.0 (line 35)
   • ✅ This prevents unexpected behavior from version changes

3. Docker Network Mode

   • Using --network host is necessary for LocalStack access on Linux
   • ✅ The precondition check for Linux is correctly implemented in Taskfile

🔒 Security Considerations

1. Hardcoded AWS Credentials

   • AWS_ACCESS_KEY_ID: test and AWS_SECRET_ACCESS_KEY: test are used (lines 18-19)
   • ✅ This is safe for LocalStack testing (no real AWS access)
   • ✅ Good use of AWS_EC2_METADATA_DISABLED: "true" to prevent credential leakage

2. Volume Mounts

   • -v ~/.aws:/root/.aws:ro mounts AWS config as read-only
   • ✅ Read-only mount is a good security practice

3. Docker Permissions

   • The workflow runs as default GitHub Actions user (not root)
   • ✅ No elevated privileges needed

[claude]

🧪 Test Coverage

What's Covered:

• ✅ Command mode navigation (:ec, :ec2/, etc.)
• ✅ Tab completion
• ✅ Enter key behavior
• ✅ Suggestion UI rendering
• ✅ All existing features (demo, themes, resource browser)

Potential Gaps:

• ❓ No explicit test for invalid commands or error states
• ❓ No test for edge cases (e.g., very long resource names, special characters)
• ℹ️ These aren't critical for this PR but could be future improvements

📝 Recommendations

High Priority

1. Update LocalStack health check to verify EC2/VPC services, not just S3

Medium Priority

2. Add job-level timeout to CI workflow (timeout-minutes: 15)
3. Add per-tape timeout in the VHS loop (wrap docker run with timeout 3m)

Low Priority (Optional)

4. Consider adding error state tests in a future PR
5. Add comment in CI workflow explaining why --network host is needed

[claude]

🎯 Suggested Improvements for CI Workflow

# .github/workflows/integration-test.yml
jobs:
  vhs-test:
    runs-on: ubuntu-latest
    timeout-minutes: 15  # Add global timeout
    env:
      # ... existing env vars ...
    steps:
      # ... existing steps ...

      - name: Start LocalStack
        run: |
          docker run -d --name localstack -p 4566:4566 localstack/localstack:4.12.0
          echo "Waiting for LocalStack..."
          for i in $(seq 1 30); do
            # Check EC2 and VPC services too
            if curl -s http://localhost:4566/_localstack/health | grep -qE '"(s3|ec2|vpc)": "(available|running)"'; then
              echo "LocalStack is ready"
              sleep 2  # Brief delay for full initialization
              exit 0
            fi
            sleep 1
          done
          echo "LocalStack failed to start"
          exit 1

      - name: Run VHS tapes
        run: |
          set -euo pipefail  # Stricter error handling
          for tape in docs/tapes/*.tape; do
            echo "=========================================="
            echo "Running: $tape"
            echo "=========================================="
            # Add timeout per tape
            timeout 3m docker run --rm --network host \
              -v "$(pwd)":/vhs \
              -v ~/.aws:/root/.aws:ro \
              -e AWS_ENDPOINT_URL=http://localhost:4566 \
              -e AWS_EC2_METADATA_DISABLED=true \
              ghcr.io/charmbracelet/vhs "$tape"
          done

[claude]

✅ Conclusion

This is a high-quality PR that adds significant value:

• ✅ Clean, well-structured code
• ✅ Follows existing patterns
• ✅ Good documentation
• ✅ Minimal security concerns (LocalStack testing is safe)
• ✅ Comprehensive test coverage for the command mode feature

Recommendation: ✅ Approve with minor suggestions

The suggested improvements are not blockers - they're optimizations that can be addressed in this PR or a follow-up. The core implementation is solid and ready to merge.

Great work on improving the test infrastructure! 🎉

---

Review completed by Claude Code - Analyzed 31 files (249 additions, 56 deletions) including:

• New CI workflow for VHS integration tests
• 84-line command-mode.tape with comprehensive command mode testing
• Updated Taskfile with new test tasks
• Documentation updates in docs/tapes/README.md
• Require assertions added to all existing tapes