feat: Refactor logging and enhance output conversion toolchain#46
feat: Refactor logging and enhance output conversion toolchain#46
Conversation
📝 WalkthroughWalkthroughThis PR adds a new internal OutputConverter that selects between ffmpeg (with librsvg), resvg, or a resvg→ffmpeg pipeline for SVG→raster/video conversion, moves distribution to archive-based bundles, introduces a release assets build script, updates installer/action logic, and adds tests and minor docs/help adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Program as "Program"
participant Output as "OutputConverter"
participant ffmpeg as "ffmpeg"
participant resvg as "resvg"
participant FS as "FileSystem"
User->>Program: Request convert SVG -> PNG/MP4
Program->>Output: ConvertSvgToRaster / ConvertSvgFramesToVideo
Output->>ffmpeg: Probe: `ffmpeg -h`
ffmpeg-->>Output: Help output
Output->>Output: Detect `--enable-librsvg` in help
alt librsvg enabled
Output->>ffmpeg: Convert SVG directly (-i input.svg ...)
ffmpeg->>FS: Write output file
else librsvg not available
Output->>resvg: Render SVG -> temp PNG(s)
resvg->>FS: Write temp PNG(s)
Output->>ffmpeg: Encode temp PNG(s) -> target format
ffmpeg->>FS: Write output file
Output->>FS: Remove temp files
end
Output-->>Program: Return result
Program-->>User: Report success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4927d5fe99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
.github/workflows/release.yaml
Outdated
| PACKAGE_VERSION: ${{ needs.build-nupkg.outputs.package-version }} | ||
| PACKAGE_ITERATION: ${{ needs.build-nupkg.outputs.package-iteration }} | ||
| GITHUB_REPOSITORY: ${{ github.repository }} | ||
| run: ./scripts/release/build-release-assets.sh |
There was a problem hiding this comment.
The release workflow now executes ./scripts/release/build-release-assets.sh, but this file is not present in this commit (repo-wide file search shows no scripts/release/build-release-assets.sh), so the release-github job will fail at runtime with a “No such file or directory” error before publishing artifacts.
Useful? React with 👍 / 👎.
| var env = new Dictionary<string, string>(); | ||
| foreach (System.Collections.DictionaryEntry entry in Environment.GetEnvironmentVariables()) | ||
| { | ||
| if (entry.Key is string key && entry.Value is string value) | ||
| { | ||
| env[key] = value; | ||
| logger.ZLogDebug($"Inherited environment variable: {key}={value}"); | ||
| } | ||
| } | ||
|
|
||
| logger.ZLogDebug($"Setting PTY size environment variables: COLUMNS={width} LINES={height}"); | ||
| env["COLUMNS"] = width.ToString(System.Globalization.CultureInfo.InvariantCulture); | ||
| env["LINES"] = height.ToString(System.Globalization.CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
This change initializes the PTY environment with only COLUMNS and LINES, so the spawned shell no longer inherits variables like PATH/HOME. In the Windows PTY path, that dictionary is passed directly to CreateProcessW as the full environment block, which causes common commands invoked by name (e.g. git, node, python) to fail to resolve in normal usage.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/release.yaml (1)
155-163: Add a release-bundle smoke check before publishing.This step is now the single place where release archives are assembled, while
action.ymlandwrapper/npm/cli/install.jsboth assume exact filenames and root-level contents. A quick assertion here would fail CI before a misnamed or mispacked bundle gets published.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yaml around lines 155 - 163, Add a pre-publish smoke check in the build-release-assets step to validate the assembled release bundle(s) created by build-release-assets.sh: modify that script (or the GitHub Actions run wrapper) to assert that RELEASE_UPLOAD_DIR contains expected archive filenames and that each archive has the required root-level entries (matching assumptions in action.yml and wrapper/npm/cli/install.js), and exit non‑zero with a clear error log if any filename or top-level content is missing or mispacked so CI fails fast before publishing.tests/ConsoleToSvg.Tests/Conversion/OutputConverterTests.cs (2)
117-122: Add a regression assertion for--disable-librsvg.Current checks cover positive + absent flag, but not the disable form. Adding that case protects against substring-based false positives in parser logic.
🧪 Suggested test addition
[Test] public void HelpOutputEnablesLibrsvg_DetectsBuildFlag() { OutputConverter.HelpOutputEnablesLibrsvg("configuration: --enable-gpl --enable-librsvg") .ShouldBeTrue(); OutputConverter.HelpOutputEnablesLibrsvg("configuration: --enable-gpl").ShouldBeFalse(); + OutputConverter.HelpOutputEnablesLibrsvg("configuration: --enable-gpl --disable-librsvg") + .ShouldBeFalse(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ConsoleToSvg.Tests/Conversion/OutputConverterTests.cs` around lines 117 - 122, Update the test HelpOutputEnablesLibrsvg_DetectsBuildFlag to include a regression assertion that the disable form does not trigger a positive result: call OutputConverter.HelpOutputEnablesLibrsvg with a string containing "--disable-librsvg" (for example "configuration: --enable-gpl --disable-librsvg") and assert it returns false; this ensures the method OutputConverter.HelpOutputEnablesLibrsvg correctly distinguishes the disable flag from the enable flag and prevents substring-based false positives.
19-45: Reduce duplicated temp-directory setup/cleanup in filesystem tests.Line 19 and Line 51 repeat the same arrange/cleanup shape. Extracting a small disposable helper (or a shared utility method) would keep future path-resolution tests easier to extend and less error-prone.
Also applies to: 51-77, 124-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ConsoleToSvg.Tests/Conversion/OutputConverterTests.cs` around lines 19 - 45, The test duplicates temp-directory setup/cleanup; refactor by extracting a reusable disposable temp-directory helper (e.g., a TempDirectory class implementing IDisposable or a CreateTempDirectoryScope method) and replace the manual CreateTempDirectory/try/finally/DeleteTempDirectory pattern in the OutputConverterTests with a using block or the scope helper; update tests that call CreateTempDirectory/DeleteTempDirectory (including the test around OutputConverter.TryResolveExecutable and the other ranges 51-77, 124-137) to obtain app/path dirs from the helper and rely on its Dispose to remove the folder, leaving the assertions (resolved.ShouldBe(bundled)) and the call to OutputConverter.TryResolveExecutable unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@action.yml`:
- Around line 85-112: Before extracting a new bundle into INSTALL_DIR, clear any
stale files to avoid leftover sidecar binaries (resvg, ffmpeg) being used by
console2svg: ensure the script removes or empties INSTALL_DIR (e.g., rm -rf
"${INSTALL_DIR:?}/"* or recreate the directory) immediately after mkdir -p and
before unpacking in the download/extract logic (the block that uses
ARCHIVE_URL/LEGACY_URL, ARCHIVE_PATH and the tar/curl steps). Use a safe
expansion (the ${INSTALL_DIR:?} pattern) to avoid accidental deletions, then
proceed with downloading, extracting, and applying chmod as currently done.
In `@README.md`:
- Around line 307-312: Add a language identifier to the fenced code block
containing the ASCII flow diagram so markdownlint stops flagging it; replace the
opening "```" before the lines starting with "# if ffmpeg has librsvg" with
"```text" (leaving the closing "```" unchanged) so the block is explicitly
marked as plain text.
In `@src/ConsoleToSvg/Conversion/OutputConverter.cs`:
- Around line 81-85: The code currently attempts to resolve executables from
Environment.CurrentDirectory via the call
TryResolveDirectoryExecutable(fileName, currentDirectory) and returns
fromCurrentDirectory if found; remove this lookup so tools like ffmpeg/resvg
cannot be shadowed by a caller's working directory. Update the logic in
OutputConverter/wherever TryResolveDirectoryExecutable is used so it only checks
the process-shipped directory and/or PATH (or an explicit override) — i.e.,
delete or bypass the TryResolveDirectoryExecutable(fileName, currentDirectory)
call and its fromCurrentDirectory return, ensuring resolution uses the process
directory resolution path and PATH-based resolution functions instead.
In `@wrapper/npm/cli/install.js`:
- Around line 54-57: Current resetDistDir aggressively wipes distDir before the
new bundle is validated; instead, change the flow to extract/build into a
temporary staging directory (e.g., tmpDistDir) and only replace the live distDir
after validation. Modify code that currently uses resetDistDir to: create
tmpDistDir, write/extract the new console2svg binary there, validate existence
and executability of console2svg, then atomically remove or move the old distDir
and rename tmpDistDir to distDir (fs.renameSync or equivalent); ensure any
failures clean up tmpDistDir and do not touch the existing distDir so installs
aren’t left broken.
---
Nitpick comments:
In @.github/workflows/release.yaml:
- Around line 155-163: Add a pre-publish smoke check in the build-release-assets
step to validate the assembled release bundle(s) created by
build-release-assets.sh: modify that script (or the GitHub Actions run wrapper)
to assert that RELEASE_UPLOAD_DIR contains expected archive filenames and that
each archive has the required root-level entries (matching assumptions in
action.yml and wrapper/npm/cli/install.js), and exit non‑zero with a clear error
log if any filename or top-level content is missing or mispacked so CI fails
fast before publishing.
In `@tests/ConsoleToSvg.Tests/Conversion/OutputConverterTests.cs`:
- Around line 117-122: Update the test HelpOutputEnablesLibrsvg_DetectsBuildFlag
to include a regression assertion that the disable form does not trigger a
positive result: call OutputConverter.HelpOutputEnablesLibrsvg with a string
containing "--disable-librsvg" (for example "configuration: --enable-gpl
--disable-librsvg") and assert it returns false; this ensures the method
OutputConverter.HelpOutputEnablesLibrsvg correctly distinguishes the disable
flag from the enable flag and prevents substring-based false positives.
- Around line 19-45: The test duplicates temp-directory setup/cleanup; refactor
by extracting a reusable disposable temp-directory helper (e.g., a TempDirectory
class implementing IDisposable or a CreateTempDirectoryScope method) and replace
the manual CreateTempDirectory/try/finally/DeleteTempDirectory pattern in the
OutputConverterTests with a using block or the scope helper; update tests that
call CreateTempDirectory/DeleteTempDirectory (including the test around
OutputConverter.TryResolveExecutable and the other ranges 51-77, 124-137) to
obtain app/path dirs from the helper and rely on its Dispose to remove the
folder, leaving the assertions (resolved.ShouldBe(bundled)) and the call to
OutputConverter.TryResolveExecutable unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaea3084-a805-42a4-9ca9-be8ff6a9c335
📒 Files selected for processing (10)
.github/workflows/release.yamlREADME.mdaction.ymlsrc/ConsoleToSvg/AssemblyInfo.cssrc/ConsoleToSvg/Cli/OptionParser.cssrc/ConsoleToSvg/Conversion/OutputConverter.cssrc/ConsoleToSvg/Program.cssrc/ConsoleToSvg/Recording/PtyRecorder.cstests/ConsoleToSvg.Tests/Conversion/OutputConverterTests.cswrapper/npm/cli/install.js
💤 Files with no reviewable changes (1)
- src/ConsoleToSvg/Recording/PtyRecorder.cs
| INSTALL_DIR="${{ runner.tool_cache }}/console2svg/${VERSION}/${{ runner.arch }}" | ||
| mkdir -p "$INSTALL_DIR" | ||
|
|
||
| echo "Downloading ${URL} ..." | ||
| curl -fsSL -L -o "${INSTALL_DIR}/console2svg${EXT}" "${URL}" | ||
| ARCHIVE_URL="${BASE_URL}/${ARCHIVE_FILE}" | ||
| LEGACY_URL="${BASE_URL}/${LEGACY_FILE}" | ||
| ARCHIVE_PATH="${INSTALL_DIR}/${ARCHIVE_FILE}" | ||
|
|
||
| echo "Downloading ${ARCHIVE_URL} ..." | ||
| if curl -fsSL -L -o "${ARCHIVE_PATH}" "${ARCHIVE_URL}"; then | ||
| if [ "${ARCHIVE_EXT}" = ".zip" ]; then | ||
| tar -xf "${ARCHIVE_PATH}" -C "${INSTALL_DIR}" | ||
| else | ||
| tar -xzf "${ARCHIVE_PATH}" -C "${INSTALL_DIR}" | ||
| fi | ||
| rm -f "${ARCHIVE_PATH}" | ||
| else | ||
| echo "Archive asset not found, falling back to legacy single-binary asset." | ||
| curl -fsSL -L -o "${INSTALL_DIR}/console2svg${EXT}" "${LEGACY_URL}" | ||
| fi | ||
|
|
||
| if [ ! -f "${INSTALL_DIR}/console2svg${EXT}" ]; then | ||
| echo "Installed console2svg binary not found in ${INSTALL_DIR}" | ||
| ls -la "${INSTALL_DIR}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| [ -z "${EXT}" ] && chmod +x "${INSTALL_DIR}/console2svg" | ||
| [ -f "${INSTALL_DIR}/resvg" ] && chmod +x "${INSTALL_DIR}/resvg" |
There was a problem hiding this comment.
Clear the cached install directory before unpacking a new bundle.
Reusing the same ${INSTALL_DIR} leaves files behind when a newer bundle removes or renames a sidecar tool. Because src/ConsoleToSvg/Conversion/OutputConverter.cs prefers bundled executables first, a stale resvg or ffmpeg can be executed with a newer console2svg.
🧹 Minimal fix
INSTALL_DIR="${{ runner.tool_cache }}/console2svg/${VERSION}/${{ runner.arch }}"
- mkdir -p "$INSTALL_DIR"
+ rm -rf "$INSTALL_DIR"
+ mkdir -p "$INSTALL_DIR"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSTALL_DIR="${{ runner.tool_cache }}/console2svg/${VERSION}/${{ runner.arch }}" | |
| mkdir -p "$INSTALL_DIR" | |
| echo "Downloading ${URL} ..." | |
| curl -fsSL -L -o "${INSTALL_DIR}/console2svg${EXT}" "${URL}" | |
| ARCHIVE_URL="${BASE_URL}/${ARCHIVE_FILE}" | |
| LEGACY_URL="${BASE_URL}/${LEGACY_FILE}" | |
| ARCHIVE_PATH="${INSTALL_DIR}/${ARCHIVE_FILE}" | |
| echo "Downloading ${ARCHIVE_URL} ..." | |
| if curl -fsSL -L -o "${ARCHIVE_PATH}" "${ARCHIVE_URL}"; then | |
| if [ "${ARCHIVE_EXT}" = ".zip" ]; then | |
| tar -xf "${ARCHIVE_PATH}" -C "${INSTALL_DIR}" | |
| else | |
| tar -xzf "${ARCHIVE_PATH}" -C "${INSTALL_DIR}" | |
| fi | |
| rm -f "${ARCHIVE_PATH}" | |
| else | |
| echo "Archive asset not found, falling back to legacy single-binary asset." | |
| curl -fsSL -L -o "${INSTALL_DIR}/console2svg${EXT}" "${LEGACY_URL}" | |
| fi | |
| if [ ! -f "${INSTALL_DIR}/console2svg${EXT}" ]; then | |
| echo "Installed console2svg binary not found in ${INSTALL_DIR}" | |
| ls -la "${INSTALL_DIR}" | |
| exit 1 | |
| fi | |
| [ -z "${EXT}" ] && chmod +x "${INSTALL_DIR}/console2svg" | |
| [ -f "${INSTALL_DIR}/resvg" ] && chmod +x "${INSTALL_DIR}/resvg" | |
| INSTALL_DIR="${{ runner.tool_cache }}/console2svg/${VERSION}/${{ runner.arch }}" | |
| rm -rf "$INSTALL_DIR" | |
| mkdir -p "$INSTALL_DIR" | |
| ARCHIVE_URL="${BASE_URL}/${ARCHIVE_FILE}" | |
| LEGACY_URL="${BASE_URL}/${LEGACY_FILE}" | |
| ARCHIVE_PATH="${INSTALL_DIR}/${ARCHIVE_FILE}" | |
| echo "Downloading ${ARCHIVE_URL} ..." | |
| if curl -fsSL -L -o "${ARCHIVE_PATH}" "${ARCHIVE_URL}"; then | |
| if [ "${ARCHIVE_EXT}" = ".zip" ]; then | |
| tar -xf "${ARCHIVE_PATH}" -C "${INSTALL_DIR}" | |
| else | |
| tar -xzf "${ARCHIVE_PATH}" -C "${INSTALL_DIR}" | |
| fi | |
| rm -f "${ARCHIVE_PATH}" | |
| else | |
| echo "Archive asset not found, falling back to legacy single-binary asset." | |
| curl -fsSL -L -o "${INSTALL_DIR}/console2svg${EXT}" "${LEGACY_URL}" | |
| fi | |
| if [ ! -f "${INSTALL_DIR}/console2svg${EXT}" ]; then | |
| echo "Installed console2svg binary not found in ${INSTALL_DIR}" | |
| ls -la "${INSTALL_DIR}" | |
| exit 1 | |
| fi | |
| [ -z "${EXT}" ] && chmod +x "${INSTALL_DIR}/console2svg" | |
| [ -f "${INSTALL_DIR}/resvg" ] && chmod +x "${INSTALL_DIR}/resvg" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@action.yml` around lines 85 - 112, Before extracting a new bundle into
INSTALL_DIR, clear any stale files to avoid leftover sidecar binaries (resvg,
ffmpeg) being used by console2svg: ensure the script removes or empties
INSTALL_DIR (e.g., rm -rf "${INSTALL_DIR:?}/"* or recreate the directory)
immediately after mkdir -p and before unpacking in the download/extract logic
(the block that uses ARCHIVE_URL/LEGACY_URL, ARCHIVE_PATH and the tar/curl
steps). Use a safe expansion (the ${INSTALL_DIR:?} pattern) to avoid accidental
deletions, then proceed with downloading, extracting, and applying chmod as
currently done.
| var fromCurrentDirectory = TryResolveDirectoryExecutable(fileName, currentDirectory); | ||
| if (fromCurrentDirectory is not null) | ||
| { | ||
| return fromCurrentDirectory; | ||
| } |
There was a problem hiding this comment.
Don't resolve ffmpeg/resvg from the caller's working directory.
Searching Environment.CurrentDirectory lets an untrusted checkout shadow the system or bundled tool with a local binary. The process directory already covers shipped executables; anything else should come from PATH or an explicit override.
🔒 Minimal fix
- var fromCurrentDirectory = TryResolveDirectoryExecutable(fileName, currentDirectory);
- if (fromCurrentDirectory is not null)
- {
- return fromCurrentDirectory;
- }
-
if (string.IsNullOrWhiteSpace(pathEnvironment))
{
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var fromCurrentDirectory = TryResolveDirectoryExecutable(fileName, currentDirectory); | |
| if (fromCurrentDirectory is not null) | |
| { | |
| return fromCurrentDirectory; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ConsoleToSvg/Conversion/OutputConverter.cs` around lines 81 - 85, The
code currently attempts to resolve executables from Environment.CurrentDirectory
via the call TryResolveDirectoryExecutable(fileName, currentDirectory) and
returns fromCurrentDirectory if found; remove this lookup so tools like
ffmpeg/resvg cannot be shadowed by a caller's working directory. Update the
logic in OutputConverter/wherever TryResolveDirectoryExecutable is used so it
only checks the process-shipped directory and/or PATH (or an explicit override)
— i.e., delete or bypass the TryResolveDirectoryExecutable(fileName,
currentDirectory) call and its fromCurrentDirectory return, ensuring resolution
uses the process directory resolution path and PATH-based resolution functions
instead.
| function resetDistDir() { | ||
| fs.rmSync(distDir, { recursive: true, force: true }); | ||
| fs.mkdirSync(distDir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
Don't delete the live dist/ tree before the replacement bundle is ready.
wrapper/npm/package.json runs this script in postinstall, so wiping dist up front turns a transient download/extract failure into a broken install and forces a re-download on every reinstall. Please stage into a temp directory and swap it into place only after the extracted console2svg binary has been validated.
Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wrapper/npm/cli/install.js` around lines 54 - 57, Current resetDistDir
aggressively wipes distDir before the new bundle is validated; instead, change
the flow to extract/build into a temporary staging directory (e.g., tmpDistDir)
and only replace the live distDir after validation. Modify code that currently
uses resetDistDir to: create tmpDistDir, write/extract the new console2svg
binary there, validate existence and executability of console2svg, then
atomically remove or move the old distDir and rename tmpDistDir to distDir
(fs.renameSync or equivalent); ensure any failures clean up tmpDistDir and do
not touch the existing distDir so installs aren’t left broken.
There was a problem hiding this comment.
Pull request overview
Refactors the output conversion pipeline to work when ffmpeg lacks librsvg support by introducing resvg as a fallback renderer, and updates distribution/install paths to ship archive-based bundles that can include resvg.
Changes:
- Introduces
OutputConverterto detect an available conversion toolchain (ffmpegwith/withoutlibrsvg,resvg) and route raster/video conversions accordingly. - Updates packaging/install flows (GitHub release assets, npm wrapper, and GitHub Action) to download/extract bundles instead of single binaries, bundling
resvgwhere available. - Adds unit tests for the new conversion/tool resolution logic and updates CLI help/docs to reflect the new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/ConsoleToSvg/Conversion/OutputConverter.cs |
New centralized conversion logic + toolchain probing (ffmpeg/resvg). |
src/ConsoleToSvg/Program.cs |
Switches raster/video conversion calls to OutputConverter. |
tests/ConsoleToSvg.Tests/Conversion/OutputConverterTests.cs |
Adds tests for executable resolution and strategy selection. |
src/ConsoleToSvg/AssemblyInfo.cs |
Enables test access to internal conversion APIs via InternalsVisibleTo. |
src/ConsoleToSvg/Cli/OptionParser.cs |
Updates help text to describe ffmpeg/resvg selection. |
wrapper/npm/cli/install.js |
Changes npm install to download and extract bundles (zip/tar.gz), and chmod extracted tools. |
scripts/release-action/build-release-assets.sh |
New script to build release bundles/packages and optionally bundle upstream resvg. |
.github/workflows/release.yaml |
Uses the new release-asset build script and installs required tooling. |
action.yml |
GitHub Action now prefers archive bundles and falls back to legacy binaries. |
README.md |
Updates installation + conversion documentation to match bundle/resvg behavior. |
src/ConsoleToSvg/Recording/PtyRecorder.cs |
Removes debug logging of inherited environment variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Prefer bundled tools first, then the current working directory, then PATH. | ||
| internal static string? TryResolveExecutable(string toolName) | ||
| => TryResolveExecutable( | ||
| toolName, | ||
| RuntimeInformation.IsOSPlatform(OSPlatform.Windows), | ||
| Environment.ProcessPath, | ||
| Environment.CurrentDirectory, | ||
| Environment.GetEnvironmentVariable("PATH") | ||
| ); | ||
|
|
||
| internal static string? TryResolveExecutable( | ||
| string toolName, | ||
| bool isWindows, | ||
| string? processPath, | ||
| string? currentDirectory, | ||
| string? pathEnvironment | ||
| ) | ||
| { | ||
| var fileName = GetExecutableFileName(toolName, isWindows); | ||
|
|
||
| var bundled = TryResolveDirectoryExecutable(fileName, Path.GetDirectoryName(processPath)); | ||
| if (bundled is not null) | ||
| { | ||
| return bundled; | ||
| } | ||
|
|
||
| var fromCurrentDirectory = TryResolveDirectoryExecutable(fileName, currentDirectory); | ||
| if (fromCurrentDirectory is not null) | ||
| { | ||
| return fromCurrentDirectory; | ||
| } | ||
|
|
There was a problem hiding this comment.
TryResolveExecutable currently searches the current working directory before PATH. That makes it easy to accidentally (or maliciously) execute a ffmpeg/resvg binary dropped into the working directory, which is a common security footgun on Unix where . is typically not in PATH. Consider removing the current-directory probe (or gating it behind an explicit option) and limiting resolution to the bundled directory + PATH.
| // Keep the user-facing failure terse; the search order is deterministic from the code path above. | ||
| private static string BuildUnavailableToolsMessage(string outputPath) => | ||
| $"Cannot generate {outputPath} because ffmpeg and resvg cannot be used for this conversion."; | ||
|
|
There was a problem hiding this comment.
BuildUnavailableToolsMessage always claims "ffmpeg and resvg cannot be used", but many failure cases here are actually "resvg is missing" (ffmpeg exists but lacks SVG) or "ffmpeg is missing" (resvg exists but output needs ffmpeg, e.g. JPG/MP4). This message is user-facing and becomes misleading; consider including which tool is missing/insufficient and the required pipeline for the selected output extension.
| // Keep the user-facing failure terse; the search order is deterministic from the code path above. | |
| private static string BuildUnavailableToolsMessage(string outputPath) => | |
| $"Cannot generate {outputPath} because ffmpeg and resvg cannot be used for this conversion."; | |
| // Keep the user-facing failure terse, but describe the required toolchain for the requested output | |
| // format rather than claiming both tools are unusable in every failure case. | |
| private static string BuildUnavailableToolsMessage(string outputPath) | |
| { | |
| var extension = Path.GetExtension(outputPath)?.ToLowerInvariant(); | |
| return extension switch | |
| { | |
| ".png" => | |
| $"Cannot generate {outputPath} because PNG output requires either resvg or ffmpeg with SVG input support, and the required toolchain is unavailable.", | |
| ".jpg" or ".jpeg" => | |
| $"Cannot generate {outputPath} because JPEG output requires ffmpeg. Supported pipelines are ffmpeg reading SVG directly, or resvg rendering SVG to PNG followed by ffmpeg converting PNG to JPEG.", | |
| ".bmp" or ".gif" or ".webp" => | |
| $"Cannot generate {outputPath} because {extension.TrimStart('.').ToUpperInvariant()} output requires ffmpeg. Supported pipelines are ffmpeg reading SVG directly, or resvg rendering SVG to PNG followed by ffmpeg converting PNG to the requested format.", | |
| ".mp4" or ".mov" or ".webm" or ".mkv" or ".avi" => | |
| $"Cannot generate {outputPath} because video output requires ffmpeg. Supported pipelines are ffmpeg reading SVG directly, or resvg rendering SVG frames for ffmpeg to encode.", | |
| _ => | |
| $"Cannot generate {outputPath} because the required conversion toolchain is unavailable for the requested output format." | |
| }; | |
| } |
| .png/.jpg/… – Raster image via ffmpeg (ffmpeg must be installed). | ||
| .mp4/.webm/… – Video via ffmpeg using frame sequences (ffmpeg must be installed). | ||
| .svg : SVG output (default, no external tools required). | ||
| .png : Raster image via ffmpeg(librsvg build) or resvg. |
There was a problem hiding this comment.
Help text has a small formatting/spelling issue: ffmpeg(librsvg build) is missing a space and reads awkwardly. Consider changing to something like ffmpeg (librsvg-enabled build) to match the wording used elsewhere and improve readability.
| .png : Raster image via ffmpeg(librsvg build) or resvg. | |
| .png : Raster image via ffmpeg (librsvg-enabled build) or resvg. |
| @@ -66,8 +65,6 @@ function download(downloadUrl, redirects, onFinish) { | |||
| const agent = proxy ? new HttpsProxyAgent(proxy) : undefined; | |||
| const urlObj = new URL(downloadUrl); | |||
|
|
|||
| const tempPath = `${destPath}.tmp`; | |||
|
|
|||
| const request = https.get( | |||
| { | |||
| hostname: urlObj.hostname, | |||
| @@ -83,7 +80,7 @@ function download(downloadUrl, redirects, onFinish) { | |||
| (res) => { | |||
| if (res.statusCode >= 300 && res.statusCode < 400 && res.headers.location) { | |||
| res.resume(); | |||
| download(res.headers.location, redirects + 1, onFinish); | |||
| download(res.headers.location, tempPath, redirects + 1, onFinish); | |||
| return; | |||
| } | |||
|
|
|||
| @@ -105,11 +102,11 @@ function download(downloadUrl, redirects, onFinish) { | |||
| }); | |||
| file.on('error', (err) => { | |||
| try { | |||
| fs.unlinkSync(tempPath); | |||
| } catch { | |||
| // ignore | |||
| } | |||
| fail('console2svg: write failed.', err); | |||
| fs.unlinkSync(tempPath); | |||
| } catch { | |||
| // ignore | |||
| } | |||
| fail('console2svg: write failed.', err); | |||
| }); | |||
| } | |||
| ); | |||
| @@ -119,12 +116,15 @@ function download(downloadUrl, redirects, onFinish) { | |||
| }); | |||
| } | |||
|
|
|||
| resetDistDir(); | |||
|
|
|||
| if (isWin) { | |||
There was a problem hiding this comment.
resetDistDir() unconditionally deletes and recreates dist/ on every install. This removes the previous early-exit behavior and forces a network download even when the expected binary is already present, which can slow installs and break offline/air-gapped installs. Consider keeping the existing file when it already exists (or only clearing dist/ after a failed extraction/download).
|
|
||
| if (isWin) { | ||
| // On Windows: download the ffmpeg bundle zip (console2svg.exe + ffmpeg.exe + DLLs) | ||
| // On Windows: download the ffmpeg bundle zip (console2svg.exe + resvg.exe + ffmpeg.exe + DLLs) |
There was a problem hiding this comment.
The Windows bundle comment mentions DLLs, but the updated release bundling script only stages ffmpeg.exe (and not additional ffmpeg DLLs). If the bundle no longer contains DLLs, this comment is misleading; if DLLs are still required, then the bundle creation should include them to avoid runtime failures.
| // On Windows: download the ffmpeg bundle zip (console2svg.exe + resvg.exe + ffmpeg.exe + DLLs) | |
| // On Windows: download the ffmpeg bundle zip (console2svg.exe + resvg.exe + ffmpeg.exe) |
| curl -fsSL \ | ||
| -o "$asset_path" \ | ||
| "https://github.com/linebender/resvg/releases/latest/download/${asset_name}" | ||
|
|
There was a problem hiding this comment.
The release asset build pulls resvg from the releases/latest URL. This makes releases non-reproducible (the bundled resvg can change over time) and adds supply-chain risk. Consider pinning a specific resvg version (e.g., via an env var) and/or verifying checksums before bundling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df5d0f65d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| var candidate = Path.Combine(directory, fileName); | ||
| return File.Exists(candidate) ? Path.GetFullPath(candidate) : null; |
There was a problem hiding this comment.
TryResolveDirectoryExecutable currently treats any existing file as a runnable tool (File.Exists), and TryResolveExecutable checks the current directory before PATH. In practice, if the working directory (or an earlier PATH entry) contains a non-executable file named ffmpeg/resvg, that path is selected and later Process.Start fails even though a valid executable exists later in PATH. This is a regression from relying on OS command resolution and can break raster/video conversion unexpectedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/release-action/build-release-assets.sh (3)
113-140: Minor: Temp directory may leak on early exit.The
temp_dircreated at line 113 is cleaned up at line 140, but ifcurl,unzip,tar, orfindfails (due toset -e), the script exits before cleanup. Since CI runners are ephemeral, this is low impact, but a subshell trap could ensure cleanup.♻️ Optional: Use subshell trap for guaranteed cleanup
temp_dir="$(mktemp -d)" + trap 'rm -rf "$temp_dir"' RETURN asset_path="${temp_dir}/${asset_name}" extract_dir="${temp_dir}/extract" mkdir -p "$cache_dir" "$extract_dir" ... - rm -rf "$temp_dir" printf '%s' "$cached_binary"Note:
trap ... RETURNrequires bash 4.0+. Alternatively, wrap the download/extract logic in a subshell with its ownEXITtrap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release-action/build-release-assets.sh` around lines 113 - 140, The temp_dir created by mktemp can leak if any command between its creation and the final rm -rf fails; ensure guaranteed cleanup by installing a trap that removes "$temp_dir" on EXIT (or wrap the download/extract block in a subshell that sets trap on EXIT/RETURN) before running curl/unzip/tar/find. Locate the block that defines temp_dir, asset_path, extract_dir and the subsequent curl/unzip/tar logic and add a trap 'rm -rf "$temp_dir"' (or create a subshell for that block and set an EXIT trap) so that temp_dir is removed on any early exit, while preserving the copying to "$cached_binary" and the chmod on "resvg".
128-133: Thefindpattern to locate extracted binaries is fragile.The pattern
find "$extract_dir" -type f -name "$binary_name" | sort | head -n 1works assuming upstream archives maintain a consistent structure. If upstream changes the archive layout (e.g., nested directories, multiple binaries), this could silently pick the wrong file.💡 Consider adding validation
extracted_binary="$(find "$extract_dir" -type f -name "$binary_name" | sort | head -n 1)" + # Validate exactly one match to detect unexpected archive structure changes + match_count="$(find "$extract_dir" -type f -name "$binary_name" | wc -l)" + if [[ "$match_count" -gt 1 ]]; then + echo "Warning: Found $match_count matches for $binary_name; using first one" >&2 + fi if [[ -z "$extracted_binary" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release-action/build-release-assets.sh` around lines 128 - 133, The current find invocation that sets extracted_binary (find "$extract_dir" -type f -name "$binary_name" | sort | head -n 1) is fragile and can pick the wrong file; change the lookup to (a) search for files whose basename exactly equals $binary_name and are executable (use find ... -type f -name "$binary_name" -perm /111 or -executable) within $extract_dir, (b) collect all matches into an array instead of piping to head, validate there is exactly one match and that its size is nonzero, and (c) if multiple matches exist, fail with a clear error listing candidates (and include $asset_name and $extract_dir in the message) so the caller can decide; update uses of extracted_binary accordingly to rely on this validated result.
268-279: Consider version pinning for ffmpeg downloads.Using
latestfor ffmpeg builds means the exact ffmpeg version can vary between releases. This is likely intentional to get recent builds, but could cause subtle behavioral differences if upstream changes. The ffmpeg download URLs are working correctly, but if strict reproducibility becomes important, consider pinning to a specific ffmpeg release tag instead oflatest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release-action/build-release-assets.sh` around lines 268 - 279, The ffmpeg download URLs in the case block set ffmpeg_zip_url using the literal "latest" which makes builds non-reproducible; modify the case in build-release-assets.sh to use a pinned tag variable (e.g., FFMPEG_TAG or FFMPEG_VERSION) instead of "latest" and construct ffmpeg_zip_url from that variable for each RID (win-x64 and win-arm64), or add a per-RID map of pinned tags; ensure the variable is documented/defined at the top of the script so maintainers can update the pinned ffmpeg release centrally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/release-action/build-release-assets.sh`:
- Around line 113-140: The temp_dir created by mktemp can leak if any command
between its creation and the final rm -rf fails; ensure guaranteed cleanup by
installing a trap that removes "$temp_dir" on EXIT (or wrap the download/extract
block in a subshell that sets trap on EXIT/RETURN) before running
curl/unzip/tar/find. Locate the block that defines temp_dir, asset_path,
extract_dir and the subsequent curl/unzip/tar logic and add a trap 'rm -rf
"$temp_dir"' (or create a subshell for that block and set an EXIT trap) so that
temp_dir is removed on any early exit, while preserving the copying to
"$cached_binary" and the chmod on "resvg".
- Around line 128-133: The current find invocation that sets extracted_binary
(find "$extract_dir" -type f -name "$binary_name" | sort | head -n 1) is fragile
and can pick the wrong file; change the lookup to (a) search for files whose
basename exactly equals $binary_name and are executable (use find ... -type f
-name "$binary_name" -perm /111 or -executable) within $extract_dir, (b) collect
all matches into an array instead of piping to head, validate there is exactly
one match and that its size is nonzero, and (c) if multiple matches exist, fail
with a clear error listing candidates (and include $asset_name and $extract_dir
in the message) so the caller can decide; update uses of extracted_binary
accordingly to rely on this validated result.
- Around line 268-279: The ffmpeg download URLs in the case block set
ffmpeg_zip_url using the literal "latest" which makes builds non-reproducible;
modify the case in build-release-assets.sh to use a pinned tag variable (e.g.,
FFMPEG_TAG or FFMPEG_VERSION) instead of "latest" and construct ffmpeg_zip_url
from that variable for each RID (win-x64 and win-arm64), or add a per-RID map of
pinned tags; ensure the variable is documented/defined at the top of the script
so maintainers can update the pinned ffmpeg release centrally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1657e543-83e2-4db0-9765-a24a0ba44e28
📒 Files selected for processing (3)
.github/workflows/release.yamlscripts/release-action/build-release-assets.shsrc/ConsoleToSvg/Recording/PtyRecorder.cs
💤 Files with no reviewable changes (1)
- src/ConsoleToSvg/Recording/PtyRecorder.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release.yaml
close #43
Summary by CodeRabbit
Documentation
Chores
Tests