feat(execution): attach media metadata to output entries in executed/history payloads - #15417
Conversation
Enrich file-type output entries with a nested metadata object
({kind, width, height} for images, plus {duration, fps, frame_count}
for videos) at output-processing time, reusing the extractors from the
asset system's media_metadata service. Unlike asset-id enrichment this
is not gated on --enable-assets: the properties flow into the executed
websocket message and /history for every consumer, including ones
without the assets system enabled.
The path containment guard moves to media_enrichment.py and is shared
with asset_enrichment.py.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @synap5e.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
| # added fields flow into ui_outputs and the cache alongside the | ||
| # raw entries, even when no client is connected. Media metadata is | ||
| # attached unconditionally; asset ids only under --enable-assets. | ||
| output_ui = enrich_output_with_media_metadata(output_ui) |
There was a problem hiding this comment.
🟡 Medium — enrich_output_with_media_metadata runs synchronously inside the async execute path on every produced output, unconditionally (not gated on --enable-assets, even with no client connected), invoking PIL/PyAV header parsing with no size or time bound. Large or numerous outputs — or a crafted/truncated media file that makes PyAV probe a large region — block the asyncio event loop and stall concurrent tasks and websocket traffic. Consider offloading extraction to a thread executor. Raised by 3 of 8 reviewers (gemini-3.1-pro adversarial, kimi-k2.7-code adversarial, claude-opus-4-8-thinking-max adversarial).
There was a problem hiding this comment.
Kept as-is — rationale in the PR description ("Review responses").
| if isinstance(entry, dict) and "metadata" not in entry: | ||
| abs_path = resolve_output_entry_path(entry) | ||
| if abs_path is not None: | ||
| mime_type = mimetypes.guess_type(entry["filename"], strict=False)[0] |
There was a problem hiding this comment.
🟢 Low — MIME type is guessed solely from the filename extension and extract_media_metadata returns nothing when it is falsy, so valid outputs silently get no metadata whenever mimetypes lacks the extension (e.g. .webp on minimal containers without /etc/mime.types) and any explicit format field already on the entry is ignored. Extensionless or custom-suffixed media are skipped and a mismatched extension can select the wrong extractor. Impact is limited to a missing best-effort field (no crash). Raised by 2 of 8 reviewers (gpt-5.6-sol-max edge-case, claude-opus-4-8-thinking-max edge-case).
There was a problem hiding this comment.
Kept as-is — rationale in the PR description ("Review responses").
- Resolve symlinks (realpath on base and candidate) before the containment check, so a symlink planted inside output/ can't smuggle an outside target past commonpath; also covers output dirs that are themselves symlinks. Applies to both enrichers via the shared helper. - Validate filename/subfolder/type are strings before path resolution so a malformed entry skips cleanly instead of raising TypeError. - Broaden the lazy-import guard beyond ImportError: the enrichment is called unguarded on the output path and must degrade to a no-op when a dependency fails to import for any reason. - Tests: real-filesystem symlink escape/containment, non-string fields, non-ImportError import failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
TL;DR
Reviewer attention: Worth a closer look at two decisions: the enrichment being ungated (runs without
--enable-assets) and the entry key namemetadata.PR Justification: ComfyUI already knows every output's media properties, but the
executedmessage and/historyonly carry{filename, subfolder, type}— so anything consuming those payloads must re-open and probe the files, which consumers that don't share the server's filesystem cannot do at all.Stakes: Medium. Runs once per produced output on the execution path; extraction is header-only (no frame decode) and per-entry errors are swallowed, so the worst credible failure is a missing
metadatakey, not a broken run.Changes: New
enrich_output_with_media_metadataattachesentry["metadata"] = {kind, width, height[, duration, fps, frame_count]}at output-processing time, reusing #15180's extractors; the path-containment guard is shared with asset enrichment.Description
Stacked on #15180 (base branch
feat/asset-video-media-metadata) — it reusesextract_media_metadatafrom that PR.Output entries in the
executedwebsocket message and in/historynow carry a nested media-properties object:{ "filename": "video_00001_.mp4", "subfolder": "video", "type": "output", "metadata": { "kind": "video", "width": 1280, "height": 720, "duration": 4.0, "fps": 24.0, "frame_count": 96 } }Design notes:
--enable-assets. feat(assets): extract video metadata into system_metadata on ingest and scan #15180 lands these properties in the asset DB; this PR is for every consumer that doesn't run the assets system — theexecutedpayload and/historyare the only interface many clients have to output files. The image half of this shape (metadata: {kind, width, height}) is already an established convention in the assets API, so consumers get one shape everywhere.ui_outputs/the cache/history even when no client is connected, and cached-output resends carry them for free.metadatakey are left untouched; non-media files, unreadable headers, and per-entry errors leave the entry unchanged. Old frontends ignore the unknown key.asset_enrichment.pyinto the new module and is shared by both enrichers, now with symlinks resolved (realpath) before the containment check and string-typed entry fields required.Review responses (kept as-is)
mimetypes.guess_type), so the same files get metadata in both paths; the failure mode is a missing optional field, never a crash.How tested
tests-unit/execution_test/test_media_enrichment.py(new, 17 cases): attach/shape, mime-type dispatch, no-mutation of originals, pre-existingmetadatakey preserved, traversal + absolute-path entries skipped, real-filesystem symlink escape/containment, non-string fields, extractor-missing / non-ImportError import failure / extractor-error degradation, multi-key outputs.tests-unit/execution_test/test_enrich_output.py(existing 15 cases) still passes against the shared path guard.ruff checkclean on changed files.