Fix Bad Practices Skill

Systematically fix bad coding practices identified by audit, following fail-fast principles.

When to Use

Use this skill:

• After running /audit-code-quality
• When fixing specific bad patterns
• During refactoring for code quality
• Autonomously as part of quality workflow

Philosophy: Fail Fast Fixes

When fixing bad practices:

1. Make errors visible - Replace silent failures with loud ones
2. Be specific - Catch only exceptions you can handle
3. Always log - Enable debugging by humans and LLMs
4. Preserve stack traces - Use raise from or raise
5. Test after fixing - Ensure fixes don't break functionality

---

Fix Patterns

Fix 1: Bare except: pass → Specific Exception + Logging

Before (BAD):

try:
    do_something()
except:
    pass

After (GOOD):

try:
    do_something()
except SpecificError as e:
    logger.exception("Failed to do_something: %s", e)
    raise

Or if recovery is possible:

try:
    result = do_something()
except SpecificError as e:
    logger.warning("do_something failed, using fallback: %s", e)
    result = fallback_value  # Only if fallback is valid!

---

Fix 2: except Exception → Specific Exceptions

Before (BAD):

try:
    data = parse_json(text)
except Exception as e:
    logger.error(e)
    return None

After (GOOD):

try:
    data = parse_json(text)
except json.JSONDecodeError as e:
    logger.exception("Invalid JSON: %s", e)
    raise ValueError(f"Cannot parse JSON: {e}") from e
except FileNotFoundError as e:
    logger.exception("File not found: %s", e)
    raise

---

Fix 3: Silent continue → Explicit Error Handling

Before (BAD):

for item in items:
    if not item.is_valid():
        continue
    process(item)

After (GOOD) - Option A: Fail on first error:

for item in items:
    if not item.is_valid():
        raise ValueError(f"Invalid item: {item}")
    process(item)

After (GOOD) - Option B: Collect errors, report at end:

errors = []
for item in items:
    if not item.is_valid():
        errors.append(f"Invalid item: {item}")
        continue
    process(item)

if errors:
    raise ValueError(f"Processing failed with {len(errors)} invalid items:\n" +
                     "\n".join(errors))

After (GOOD) - Option C: Log warning if skip is intentional:

for item in items:
    if not item.is_valid():
        logger.warning("Skipping invalid item: %s (reason: %s)", item, item.validation_error)
        continue
    process(item)

---

Fix 4: return None on Error → Raise Exception

Before (BAD):

def load_config(path):
    if not os.path.exists(path):
        return None
    with open(path) as f:
        return json.load(f)

After (GOOD):

def load_config(path: Path) -> dict:
    """Load configuration from path.

    Raises:
        FileNotFoundError: If config file doesn't exist
        json.JSONDecodeError: If config is not valid JSON
    """
    if not path.exists():
        raise FileNotFoundError(f"Config file not found: {path}")

    with open(path) as f:
        return json.load(f)

---

Fix 5: Error Return Codes → Exceptions

Before (BAD):

def process_data(data):
    if not data:
        return -1, "No data provided"
    if not validate(data):
        return -2, "Invalid data"
    result = transform(data)
    return 0, result

After (GOOD):

def process_data(data):
    """Process the data.

    Raises:
        ValueError: If data is empty or invalid
    """
    if not data:
        raise ValueError("No data provided")
    if not validate(data):
        raise ValueError(f"Invalid data: {get_validation_errors(data)}")
    return transform(data)

---

Fix 6: Defensive None Checks → Fail Fast

Before (BAD):

def process(data):
    if data is None:
        return
    # ... rest of processing

After (GOOD) - If None is never valid:

def process(data):
    if data is None:
        raise ValueError("data cannot be None")
    # ... rest of processing

After (GOOD) - If function should handle None gracefully:

def process(data: Optional[Data]) -> Optional[Result]:
    """Process data if provided.

    Args:
        data: Data to process, or None to skip processing

    Returns:
        Result if data was processed, None if data was None
    """
    if data is None:
        logger.debug("process() called with None, returning None")
        return None
    # ... rest of processing

---

Fix 7: subprocess Without Error Checking → check=True

Before (BAD):

subprocess.run(["git", "commit", "-m", "message"])

After (GOOD):

try:
    subprocess.run(
        ["git", "commit", "-m", "message"],
        check=True,
        capture_output=True,
        text=True
    )
except subprocess.CalledProcessError as e:
    logger.exception("Git commit failed: %s\nstderr: %s", e, e.stderr)
    raise

---

Fix 8: Catch-Log-Reraise → Add Context or Remove

Before (BAD) - Adds noise:

try:
    do_something()
except SomeError as e:
    logger.error(f"Error: {e}")
    raise

After (GOOD) - Option A: Add context:

try:
    do_something()
except SomeError as e:
    logger.exception("Failed during do_something in context X")
    raise RuntimeError(f"Operation failed in context X") from e

After (GOOD) - Option B: Just let it propagate:

do_something()  # Let caller handle the exception

---

Adding Proper Logging

Logger Setup

Ensure each module has a logger:

import logging

logger = logging.getLogger(__name__)

Logging Levels Guide

| Level | Use For | |-------|---------| | logger.debug() | Detailed diagnostic info | | logger.info() | Normal operation milestones | | logger.warning() | Recoverable issues, degraded operation | | logger.error() | Errors that don't stop execution | | logger.exception() | Errors with stack trace (use in except blocks) | | logger.critical() | Fatal errors, application cannot continue |

Logging Best Practices

# GOOD - Use exception() in except blocks (includes stack trace)
except SomeError as e:
    logger.exception("Operation failed: %s", e)

# GOOD - Use lazy formatting
logger.debug("Processing item %s of %s", i, total)

# BAD - Eager string formatting
logger.debug(f"Processing item {i} of {total}")  # Formats even if debug disabled

# GOOD - Include relevant context
logger.error("Failed to load preset '%s' from %s: %s", preset_id, path, e)

# BAD - Vague messages
logger.error("Error occurred")

---

Fix Workflow

Step 1: Run Audit

# Run audit first
/audit-code-quality

# Or run grep patterns directly
grep -rn "except.*:$" --include="*.py" -A1 | grep -B1 "pass$"

Step 2: Categorize Issues

Priority order:

1. except: pass or except Exception: pass (CRITICAL)
2. Overly broad except Exception (HIGH)
3. Silent continuation patterns (MEDIUM)
4. Missing logging (MEDIUM)
5. Return None on error (LOW)

Step 3: Fix Each Issue

For each issue:

1. Understand the intent - Why was error suppressed?
2. Determine correct handling:
   • Should it fail fast? → Raise exception
   • Is recovery possible? → Handle specifically with logging
   • Is it truly optional? → Document and log at DEBUG level
3. Apply fix pattern from above
4. Add/update tests for error conditions
5. Run tests to verify fix

Step 4: Verify Fixes

# Run tests
uv run pytest MouseMaster/Testing/Python/ -v

# Run linting
uv run ruff check .

# Re-run audit to confirm
/audit-code-quality

---

Automated Fix Script

For simple patterns, use sed (review changes before committing!):

#!/bin/bash
# CAUTION: Review all changes manually!

# Find files with except:pass (for manual review)
echo "Files needing manual review:"
grep -rl "except.*:" --include="*.py" | while read f; do
    if grep -q "pass$" "$f"; then
        echo "  $f"
    fi
done

Note: Automated fixes are risky. Always review manually and run tests.

---

Exception Handling Decision Tree

Is this a programming error (bug)?
├─ Yes → Let it crash (don't catch)
└─ No → Can user/system recover?
         ├─ Yes → Catch, log, handle gracefully
         └─ No → Catch, log with context, re-raise or wrap

When catching:
├─ Can you name the specific exception?
│   ├─ Yes → Catch that specific type
│   └─ No → Research what exceptions can occur
└─ Is logging added?
    ├─ Yes → Good
    └─ No → Add logger.exception() call

---

Valid Exception Handling Cases

Not all exception handling is bad. Valid cases include:

1. User Input Validation

try:
    value = int(user_input)
except ValueError:
    logger.warning("Invalid input '%s', prompting user", user_input)
    show_error_to_user("Please enter a valid number")

2. Optional Feature Degradation

try:
    import optional_dependency
    FEATURE_AVAILABLE = True
except ImportError:
    logger.info("Optional feature unavailable: optional_dependency not installed")
    FEATURE_AVAILABLE = False

3. Resource Cleanup

try:
    resource = acquire_resource()
    use_resource(resource)
finally:
    resource.cleanup()  # Always runs

4. Retry Logic

for attempt in range(max_retries):
    try:
        return do_operation()
    except TransientError as e:
        logger.warning("Attempt %d failed: %s", attempt + 1, e)
        if attempt == max_retries - 1:
            raise
        time.sleep(backoff)

5. Boundary/API Error Translation

try:
    result = external_api.call()
except ExternalAPIError as e:
    logger.exception("External API failed")
    raise OurDomainError(f"Service unavailable: {e}") from e

---

Testing Error Paths

After fixing, add tests for error conditions:

def test_load_config_missing_file():
    """Test that missing config raises FileNotFoundError."""
    with pytest.raises(FileNotFoundError, match="Config file not found"):
        load_config(Path("/nonexistent/path"))

def test_process_invalid_data():
    """Test that invalid data raises ValueError with details."""
    with pytest.raises(ValueError, match="Invalid data"):
        process_data({"bad": "data"})

---

Commit Message Template

After fixes:

fix: replace exception swallowing with proper error handling

- Remove except:pass in module_name.py
- Add specific exception types with logging
- Add error path tests

Follows fail-fast principle: errors now surface immediately
with full context for debugging.