feat(deposition): also log manifest if assembly creation fails

[anna-parker]

See pathoplexus/ena-submission#194 - it would be good to improve the logging further.

🚀 Preview: Add preview label to enable

[claude]

Code Review

Summary

This PR adds logging of the manifest.tsv file contents when ENA assembly creation fails. This is a useful debugging enhancement that will help diagnose submission failures.

Code Quality ✅

The implementation follows good practices:

• Uses proper exception handling with try/except blocks
• Logs at appropriate levels (info for success, warning for failures)
• Consistent with the existing pattern for logging webin-cli log files (lines 663-669)
• Path import is already present in the file (line 15)

Observations & Suggestions

1. Exception Handling
The use of a broad Exception catch is acceptable here since this is purely for debugging/logging purposes and should not interrupt the error flow. However, you could be more specific:

• FileNotFoundError - if the manifest file doesn't exist
• PermissionError - if there are permission issues
• UnicodeDecodeError - if there are encoding issues

But given this is non-critical logging code, the broad exception is fine.

2. Encoding Specification
Good choice to explicitly specify encoding="utf-8" - this matches the pattern used in the webin-cli log reading code below (line 666).

3. Log Message Clarity
The log message "manifest.tsv contents:" is clear and consistent with the existing "webin-cli log file {file_path} contents:" pattern.

Potential Issues ⚠️

File Size Consideration
The manifest file contents are logged in full without any size limits. If manifest files can be very large, this could:

• Generate extremely verbose logs
• Impact log storage/performance
• Make logs harder to read

Consider whether you need to:

• Truncate very large manifests
• Add a size check before logging
• Or document that manifest files are expected to be reasonably sized

Example enhancement (optional):

try:
    manifest_contents = Path(manifest_filename).read_text(encoding="utf-8")
    if len(manifest_contents) > 50000:  # ~50KB threshold
        logger.info(f"manifest.tsv is large ({len(manifest_contents)} chars), truncating...\n{manifest_contents[:50000]}...")
    else:
        logger.info(f"manifest.tsv contents:\n{manifest_contents}")
except Exception as e:
    logger.warning(f"Reading manifest from {manifest_filename} failed: {e}")

Test Coverage 📋

The PR checklist indicates:

• The implemented feature is covered by appropriate, automated tests.

This enhancement is purely for logging/debugging and would be difficult to test effectively. The existing behavior (error handling) remains unchanged. Consider updating the checklist to indicate this is a logging enhancement that doesn't require new tests, or if you want to be thorough, you could add a test that verifies the log output when assembly creation fails.

Security ✅

No security concerns. The code:

• Only reads files (no writes)
• Uses safe Path operations
• Handles exceptions properly
• Doesn't expose sensitive data beyond what's already in the manifest (which is submission data)

Performance ✅

Minimal performance impact:

• Only executes in error paths (not the happy path)
• Single file read operation
• Performance impact is negligible compared to the failed submission process

Recommendation

Approve with minor suggestion: The code is solid and ready to merge. The only consideration is whether to add size limits to the logged manifest contents to prevent potential log bloat. This is optional and depends on your expected manifest file sizes.

Great debugging enhancement! 🎯