Skip to content

fix(lib): concat list entries broke on relative paths and quotes in paths - #673

Merged
jeertmans merged 6 commits into
jeertmans:mainfrom
Agi-Asi:fix/concat-paths
Aug 28, 2026
Merged

jeertmans merged 6 commits into
jeertmans:mainfrom
Agi-Asi:fix/concat-paths

Conversation

@Agi-Asi

@Agi-Asi Agi-Asi commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Description

concatenate_video_files writes an ffmpeg concat demuxer list file into the system temp directory, with entries written verbatim as file '<path>'. Two failure modes fall out of that:

  1. Relative input paths silently break. The concat demuxer resolves relative entries against the directory of the list file (the temp folder), not the caller's working directory — so every relative segment path became a dangling reference and av.open failed with FileNotFoundError on files that exist. Reproduced locally by calling concatenate_video_files([Path('tests/data/video.mp4'), ...]) from the repo root.

  2. Quotes in paths make entries unparseable. A single quote anywhere in a component (John's slides/…) terminates the quoted entry early. Any user whose project lives under such a directory gets a confusing parse/FileNotFoundError failure from deep inside PyAV.

Fix, in one place (_concat_list_entry): entries are written as absolute (resolved) paths, with single quotes escaped shell-style ('\'') — the quoting ffmpeg's av_get_token() understands.

Today's internal callers happen to always pass absolute paths, which is why the pipeline works — but concatenate_video_files is part of the importable API surface, and both failure modes produce errors that look exactly like the "file vanished mid-render" class of reports (e.g. the Errno 22 Invalid argument family in #540 is being debugged around this same function). Hardening the list-file writing removes two entire failure modes from the suspect list.

Testing

  • Two new regression tests: relative CWD-dependent input (via monkeypatch.chdir), and a path containing a single quote — both assert a decodable output. Both fail on main and pass with this change.
  • pytest tests/test_utils.py — 4 passed.
  • ruff check / ruff format --check clean (the helper moved to module level to stay under the complexity limit).

…aths

concatenate_video_files writes an ffmpeg concat demuxer list file into
the system temp directory, with entries written verbatim as
"file '<path>'". Two failure modes:

1. Relative input paths: the concat demuxer resolves relative entries
   against the directory of the LIST FILE (the temp folder), not the
   caller's working directory, so every relative path became a dangling
   reference and av.open failed with FileNotFoundError on a file that
   exists.

2. Quotes in paths: a single quote in any component ("John's slides")
   terminated the quoted entry early and made it unparseable.

Fix both in one place: entries are written as absolute (resolved)
paths, with single quotes escaped shell-style ('\''), which is the
quoting ffmpeg's av_get_token() understands.

Adds regression tests for both cases (relative CWD-dependent input,
and a path containing a single quote), asserting a decodable output.

The helper moved to module level to keep ruff's complexity limit happy.
@github-actions github-actions Bot added waiting review Waiting review (or response) by a maintainer waiting triage This issue or PR needs to be labeled accordingly. labels Aug 27, 2026
@codacy-production

codacy-production Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 2 high · 1 minor

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category Results
Security 2 high
CodeStyle 1 minor

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@codecov

codecov Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.10%. Comparing base (1549ef4) to head (8834128).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #673   +/-   ##
=======================================
  Coverage   79.10%   79.10%           
=======================================
  Files          23       23           
  Lines        2178     2178           
=======================================
  Hits         1723     1723           
  Misses        455      455           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeertmans jeertmans left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Agi-Asi, thanks for your contribution! Could you just check the two comments I left? Except this, it looks all good to me.

Comment thread CHANGELOG.md Outdated
Comment thread manim_slides/utils.py
@jeertmans jeertmans added bug Something isn't working lib Related to the library (a.k.a. module) and removed waiting triage This issue or PR needs to be labeled accordingly. waiting review Waiting review (or response) by a maintainer labels Aug 27, 2026
Addresses review feedback on jeertmans#673. I tested the suggested
shlex.quote form empirically and it unfortunately can't be used
here: shlex quotes for POSIX shells (' -> '"'"'), while ffmpeg's
concat demuxer parses entries with av_get_token, which only
understands backslash escapes and '\'' -style quoting — a
shlex-quoted path containing a single quote fails to parse (details
in the PR thread). Kept the av_get_token-compatible escaping, and
switched str(resolve()) to resolve().as_posix() from the suggestion.
@Agi-Asi

Agi-Asi commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I applied the CHANGELOG suggestion and switched to file.resolve().as_posix().

On shlex.quote: I tried it first since it would indeed be fewer lines, but it can't be used here — shlex quotes for POSIX shells (embedded ' becomes '"'"'), while ffmpeg's concat demuxer parses list entries with av_get_token(), which only understands backslash escapes and '\''-style quoting. Empirically (PyAV 15, ffmpeg 7):

# path: /tmp/x/John's slides/video.mp4
f"file '{quote(path)}'\n"   # -> parse fails, FileNotFoundError
f"file {quote(path)}\n"     # -> parse fails too
"file '" + path.replace("'", "'\\''") + "'\n"  # -> works

(The doubled quoting in the first form also breaks plain paths only when they contain quotes, so the failure is silent until a user has a ' in a directory name — the exact case the PR fixes.)

So I kept the av_get_token-compatible escaping with a comment pointing at the ffmpeg quoting rules, but simplified per your suggestion where possible. Happy to adjust further!

@jeertmans

Copy link
Copy Markdown
Owner

Thanks for checking, see my latest #673 (comment)

@jeertmans

Copy link
Copy Markdown
Owner

Thanks @Agi-Asi! A typo in one of the comments has been reported by the pre-commit codspell check, could you fix it?

@Agi-Asi

Agi-Asi commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor Author

Note on CI: the remaining pre-commit.ci - pr failure is the ty hook failing to download dependencies (dns error / Temporary failure in name resolution fetching wheels from PyPI) on pre-commit.ci's runner — the same failure currently shows on all other open PRs (#675, #671, #668), so it's infrastructure-side rather than related to this change. The codespell finding from the earlier run (unparseable) is fixed in 3056cf5, and all other hooks pass.

Comment thread tests/test_utils.py Outdated
@Agi-Asi

Agi-Asi commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

pre-commit.ci run

@jeertmans

Copy link
Copy Markdown
Owner

Thanks for your contribution @Agi-Asi!

@jeertmans
jeertmans merged commit 1d90682 into jeertmans:main Aug 28, 2026
52 of 54 checks passed
ajlaanayan-crypto added a commit to ajlaanayan-crypto/manim-slides that referenced this pull request Sep 29, 2026
…aths (jeertmans#673)

* fix(lib): concat list entries broke on relative paths and quotes in paths

concatenate_video_files writes an ffmpeg concat demuxer list file into
the system temp directory, with entries written verbatim as
"file '<path>'". Two failure modes:

1. Relative input paths: the concat demuxer resolves relative entries
   against the directory of the LIST FILE (the temp folder), not the
   caller's working directory, so every relative path became a dangling
   reference and av.open failed with FileNotFoundError on a file that
   exists.

2. Quotes in paths: a single quote in any component ("John's slides")
   terminated the quoted entry early and made it unparseable.

Fix both in one place: entries are written as absolute (resolved)
paths, with single quotes escaped shell-style ('\''), which is the
quoting ffmpeg's av_get_token() understands.

Adds regression tests for both cases (relative CWD-dependent input,
and a path containing a single quote), asserting a decodable output.

The helper moved to module level to keep ruff's complexity limit happy.

* chore(changelog): add entry for jeertmans#673

* review: use as_posix() and apply changelog suggestion

Addresses review feedback on jeertmans#673. I tested the suggested
shlex.quote form empirically and it unfortunately can't be used
here: shlex quotes for POSIX shells (' -> '"'"'), while ffmpeg's
concat demuxer parses entries with av_get_token, which only
understands backslash escapes and '\'' -style quoting — a
shlex-quoted path containing a single quote fails to parse (details
in the PR thread). Kept the av_get_token-compatible escaping, and
switched str(resolve()) to resolve().as_posix() from the suggestion.

* review: inline escaping into concatenate_video_files

* chore: fix codespell finding (unparseable -> unparsable)

* Apply suggestion from @jeertmans

---------

Co-authored-by: Agi-Asi <206806952+Agi-Asi@users.noreply.github.com>
Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lib Related to the library (a.k.a. module)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants