StarPunk/docs/design/v1.4.1/architect-responses.md

Architect Responses - v1.4.1 Media Upload Logging

Architect: Architect Agent Date: 2025-12-16 Status: Complete

---

Responses to Developer Questions

Question 1: Variant Generation Error Handling and Re-raising

Answer: Option A - Catch all exceptions, log at WARNING, and continue.

Rationale: The design explicitly states variant generation failure should NOT fail the overall upload. The original image is the primary deliverable; variants are optimizations. Users can still use their uploaded image even if variant generation fails.

Return value when generation fails: Empty list (variants = []).

The success log will then show variants=0 (or variants=1 if we count the original, but per the current code the generate_all_variants return value only counts generated variants, not the original). This accurately reflects what was created.

Implementation guidance: Wrap the generate_all_variants() call in a try/except that catches Exception, logs at WARNING with the specified format, and allows the function to continue to the success log and return.

---

Question 2: Unexpected Error Logging and Exception Re-raising

Answer: Add a top-level try/except in save_media() that catches non-ValueError exceptions, logs at ERROR, and re-raises.

Rationale: The ERROR log format (line 111-122) is meant for truly unexpected errors like disk full, database errors, or other system failures. These should be logged before propagating to the caller.

Implementation pattern:

def save_media(file_data: bytes, filename: str) -> Dict:
    input_size = len(file_data)

    try:
        # All existing logic (validation, optimization, save, variants)
        ...
        # Success log
        ...
        return result

    except ValueError:
        # Already logged at WARNING in validation/optimization blocks
        raise

    except Exception as e:
        current_app.logger.error(
            f'Media upload failed unexpectedly: filename="{filename}", '
            f'error_type="{type(e).__name__}", error="{e}"'
        )
        raise

This ensures:

1. Validation/optimization ValueErrors are logged at WARNING and re-raised (per existing design)
2. Unexpected errors (OSError, database errors, etc.) are logged at ERROR before re-raising
3. Callers still receive exceptions and can handle them appropriately

---

Question 3: Logging Import

Answer: No additional imports needed.

Confirmation: current_app.logger from Flask is sufficient. No import logging is required in starpunk/media.py.

---

Question 4: Test File Naming and Location

Answer: Option A - Add tests to the existing tests/test_media_upload.py file.

Rationale: The design document incorrectly referenced tests/test_media.py. The existing file tests/test_media_upload.py already tests save_media() and related functions, making it the logical home for logging tests.

Design document update: I will update the design document to reference the correct filename.

---

Question 5: Integration Test Scope

Answer: Option C - Skip integration tests for logging; unit tests are sufficient.

Rationale:

1. The logging occurs entirely within save_media(), which is thoroughly unit-tested
2. Integration tests for routes should focus on HTTP behavior (status codes, response bodies, flash messages)
3. Adding caplog assertions to route tests couples them to implementation details
4. The routes are not adding or modifying logging - they just call save_media()

Keep integration tests focused on:

• Flash messages work correctly (admin route)
• Error responses are correct (micropub route)
• No duplicate logging assertion needed since we're removing the duplicate

Design document update: I will clarify that integration tests verify behavior, not logging.

---

Question 6: Optimized Image Bytes Variable Scope

Answer: Use existing file_size variable; no rename needed.

Rationale: The existing code has file_size = len(file_data) at line 398. The design pseudocode used input_size for clarity, but the existing variable name is fine. Consistency within the existing codebase takes precedence over matching pseudocode exactly.

Implementation: Use file_size wherever the design shows input_size. The logs will still be accurate and meaningful.

---

Question 7: Media ID Availability for Variant Failure Logging

Confirmation: Correct. The media_id is available at line 442 after the database insert, and variant generation happens at line 447. No changes needed.

---

Question 8: Empty File Handling

Confirmation: Correct. Empty files will show size=0b in the validation failure log. This is accurate and helpful for debugging.

---

Question 9: Animated GIF Rejection Logging

Confirmation: Correct. Animated GIF >10MB rejection is a validation failure and will be logged at WARNING with the detailed error message. This is intended behavior.

---

Question 10: GIF Optimization Path

Confirmation: Correct. For animated GIFs:

• optimized=False (original bytes unchanged)
• variants=1 (only 'original' variant in database)

This is expected and documented behavior.

---

Question 11: Mocking and Application Context

Answer: Option B - Create a new test class TestMediaLogging.

Rationale: A dedicated test class improves clarity and groups related tests. However, this is a minor organizational preference; either approach is acceptable.

---

Question 12: Caplog Level Setting

Answer: Use caplog.at_level(logging.INFO) as shown in the design.

Rationale: The context manager pattern is cleaner and ensures the level is reset after the test. This matches the design document.

---

Implementation Approach Validation

The developer's planned implementation approach (Question section, lines 149-161) is approved with one clarification:

• Step 2 and 3 should only catch ValueError (as shown in the design)
• Step 4 should catch Exception (variant failures)
• Add top-level Exception handler for unexpected errors (per Question 2 response)

---

Design Document Updates

Based on these questions, the following updates should be made to docs/design/v1.4.1/media-logging-design.md:

1. Line 225: Change tests/test_media.py to tests/test_media_upload.py
2. Line 245-250: Clarify that integration tests verify HTTP behavior, not logging assertions
3. Line 270: Change tests/test_media.py to tests/test_media_upload.py
4. Add clarification: Top-level exception handler for unexpected errors

I will make these updates now.

---

Summary

All questions answered. The design is sound and ready for implementation. Key clarifications:

1. Variant failures: Catch all, log WARNING, continue with empty list
2. Unexpected errors: Top-level handler logs ERROR, then re-raises
3. Test file: Use existing tests/test_media_upload.py
4. Integration tests: Focus on HTTP behavior, not logging assertions
5. Variable naming: Use existing file_size, no rename needed

The developer may proceed with implementation.