fix(#404): remove stale --pdf-output-folder flag from md-to-pdf dispatch by atlas-apex · Pull Request #405 · me2resh/apexyard

atlas-apex · 2026-05-24T17:28:36Z

Summary

Removed stale --pdf-output-folder and --dest-name flags from convert.sh — md-to-pdf removed both flags in a breaking API change; every invocation of the md-to-pdf fallback path was aborting with ArgError: unknown or unexpected option: --pdf-output-folder, meaning /pdf was entirely broken for the majority of adopters who don't have pandoc installed (common on macOS where pandoc pulls in LaTeX dependencies)
New dispatch strategy: stage-in-temp-dir — source is copied into a private $WORK dir under the desired output stem, npx -y md-to-pdf <tmp>.md is invoked there (md-to-pdf writes <stem>.pdf next to the source by default), then the result is mv'd to the requested TO_ABS; this is compatible with the current md-to-pdf API and avoids any cwd or output-name collisions
Same fix applied to md-to-pdf-html branch — the HTML → PDF fallback path had identical stale flags and is fixed with the same staging pattern
Regression test added at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh — covers 4 cases: successful conversion, absence of --pdf-output-folder, absence of --dest-name, and custom output path landing at exactly the requested --to path; all 4 pass locally against npm latest

Testing

# Run the new regression test
bash .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh

# Run the existing smoke test (should still pass)
bash .claude/skills/pdf/tests/smoke.sh

# Manual end-to-end repro from the issue (requires Node, no pandoc needed)
echo '# Test' > /tmp/test.md
.claude/skills/pdf/convert.sh --from=/tmp/test.md --to=/tmp/test.pdf --converter=md-to-pdf
# Expected: exit 0, /tmp/test.pdf written with non-zero size

Closes #404

Glossary

Term	Definition
`md-to-pdf`	npm package that converts markdown/HTML to PDF using headless Chromium (Puppeteer). Used by `/pdf` as the no-pandoc fallback.
`--pdf-output-folder`	Flag removed from md-to-pdf in a breaking API change; `convert.sh` was still passing it, causing an `ArgError` on every invocation of the fallback path.
`--dest-name`	Companion stale flag — also removed from md-to-pdf; used alongside `--pdf-output-folder` to name the output file. Removed in the same fix.
`stage-in-temp-dir strategy`	The replacement approach: copy source into `$WORK/<desired-stem>.md`, run `npx md-to-pdf` there, then `mv` `$WORK/<desired-stem>.pdf` to the requested output path. Relies on md-to-pdf's default behaviour of writing `<source-basename>.pdf` next to the source.
`graceful degrade`	The pattern `/pdf` uses: try each converter in order, fall through on missing-dep; only exit non-zero if NO converter is installed. The stale flags were causing the md-to-pdf branch to error (exit 1) instead of degrade (exit 3).

md-to-pdf removed --pdf-output-folder and --dest-name in a breaking API change; convert.sh still passed both flags, causing every npx md-to-pdf invocation to abort with: ArgError: unknown or unexpected option: --pdf-output-folder Fix: stage the source into a private temp dir under the desired output stem (e.g. WORK/out.pdf → WORK/out.md), invoke `npx -y md-to-pdf <tmp>.md`, then mv the resulting <tmp>.pdf to the requested TO_ABS. md-to-pdf writes <basename>.pdf next to the source by default, so this is compatible with every currently-released version. The same fix is applied to the md-to-pdf-html branch (HTML → PDF fallback), which had identical stale flags. Adds regression test at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh covering four cases: successful conversion, absence of the stale --pdf-output-folder flag, absence of --dest-name, and custom output path resolution. All four pass locally against the current npm latest. Refs #404 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

atlas-apex

Code Review: PR #405

Commit: 6e9e42eca66ae0b62c69ba91652065bcb9fc22ca

Summary

Removes stale --pdf-output-folder and --dest-name flags from both md-to-pdf dispatch branches in convert.sh (markdown and HTML inputs). Replaces with a stage-in-temp-dir strategy: copy source to $WORK/<out_stem>.{md,html}, invoke npx -y md-to-pdf <tmp_src>, then mv the resulting $WORK/<out_stem>.pdf to TO_ABS. Adds 4-case regression test at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh.

Checklist Results

Architecture & Design: Pass
Code Quality: Pass
Testing: Pass
Security: Pass (no new attack surface — same external-process invocation pattern, no new args derived from untrusted input)
Performance: Pass (one extra cp to temp dir, negligible)
PR Description & Glossary: Pass (Summary, Testing, Closes #404, Glossary all present)
Summary Bullet Narrative: Pass (all 4 bullets are verb + >6 words + rationale)
Technical Decisions (AgDR): N/A (no new dependency, no new pattern — bug fix that restores the documented contract)
Adopter Handbooks: N/A (no handbooks in this repo affect .claude/skills/pdf/)

Verification

Diff scope is correct — only .claude/skills/pdf/convert.sh (+25 / -21) and the new .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh (+168). No incidental edits to other skills, hooks, rules, agents, or site/.
The fix works — the new md-to-pdf branch (lines ~250-270 in head) computes out_stem from TO_ABS, copies source to $WORK/<out_stem>.md, invokes npx -y md-to-pdf "$tmp_src" with no removed flags. md-to-pdf's default behaviour (write <source-basename>.pdf next to source) lands the result deterministically at $WORK/<out_stem>.pdf = the variable tmp_out, which is then mv'd to TO_ABS. The old "some versions of md-to-pdf output as <basename>.pdf" double-check fallback is removed, which is correct now that the source basename and the expected output are aligned by construction (no longer two competing locations). Same fix applied symmetrically to md-to-pdf-html branch.
Exit codes preserved — all exit points still use 0/1/2/3 per the SKILL.md contract. The temp-dir cleanup is handled by the existing trap 'rm -rf "$WORK"' EXIT at line ~218, so both success and failure paths clean up automatically.
Graceful degrade unchanged — choose_for_markdown still prefers pandoc, falls through to md-to-pdf, exits 3 with print_install_advisory if neither is available. The fix only changes how the md-to-pdf branch invokes the binary, not the dispatch logic.
Regression test is high quality — forces the md-to-pdf path via --converter=md-to-pdf; gates on command -v npx at top (skips cleanly on Node-less hosts); per-test handling of exit 3 (md-to-pdf network/install failure) as a skip rather than a hard fail; trap cleans up $FIXTURE. Tests B and C explicitly grep stderr for the removed flag names — this is a strong assertion that catches regression even if md-to-pdf's CLI changes again in a way that happens to ignore unknown flags silently. Test D verifies output lands at exactly the requested --to path including a nested subdir.

Issues Found

None.

Suggestions

None blocking. One edge-case observation for the next person who touches this code: out_stem="$(basename "${TO_ABS%.*}")" derives the stem from the destination path; if a caller passes --to=/tmp/foo with no extension, ${TO_ABS%.*} returns the path unchanged and the temp file becomes $WORK/foo.md → $WORK/foo.pdf → mv to /tmp/foo. Works correctly, just worth knowing.

Verdict

APPROVED

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 6e9e42eca66ae0b62c69ba91652065bcb9fc22ca

* fix(#404): remove stale --pdf-output-folder flag from md-to-pdf dispatch (#405) md-to-pdf removed --pdf-output-folder and --dest-name in a breaking API change; convert.sh still passed both flags, causing every npx md-to-pdf invocation to abort with: ArgError: unknown or unexpected option: --pdf-output-folder Fix: stage the source into a private temp dir under the desired output stem (e.g. WORK/out.pdf → WORK/out.md), invoke `npx -y md-to-pdf <tmp>.md`, then mv the resulting <tmp>.pdf to the requested TO_ABS. md-to-pdf writes <basename>.pdf next to the source by default, so this is compatible with every currently-released version. The same fix is applied to the md-to-pdf-html branch (HTML → PDF fallback), which had identical stale flags. Adds regression test at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh covering four cases: successful conversion, absence of the stale --pdf-output-folder flag, absence of --dest-name, and custom output path resolution. All four pass locally against the current npm latest. Refs #404 Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(#403): /release-sync — main→dev sync after each release (#406) * feat(#403): /release-sync skill — main→dev sync after each release Introduces the /release-sync skill to address squash-merge divergence that accumulated across releases and produced 99 conflicts on the v2.0.0 release PR. After each release, /release-sync vX.Y.Z opens a sync PR that merges upstream/main into upstream/dev with -X ours (dev wins on conflicts), making the squash commit an ancestor of dev so future release PRs only show genuinely-new commits. - .claude/skills/release-sync/SKILL.md — new skill spec with pre-flight checks, idempotent no-op, edge-case table, and PR body template - .claude/skills/release/SKILL.md — adds Step 9 (mandatory invoke of /release-sync) and updates Related section - docs/agdr/AgDR-0052-release-sync-skill.md — records the design choice (explicit skill vs auto-invoke vs switch-to-merge-commit) and the merge-strategy direction reasoning (-X ours on a dev-based branch) - .claude/hooks/tests/test_release_sync.sh — 11 sandbox-based smoke tests covering already-in-sync no-op, diverged detection, branch name shape, -X ours direction, idempotence, post-sync state, and version argument validation; all 11 pass Refs #403 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#403): refresh site skill-count (54→55) + 2 MD032 lint fixes Address red CI on Rex-approved release-sync PR: - site-counts drift: new /release-sync skill bumps total to 55. Update all 12 marketing-copy references across architecture.html, *.md.gen, llms.txt, llms-full.txt, skill.md (incl. 2 multiline / phrasing-variant references caught by the post-#377 cross-newline scanner). - markdownlint MD032: add blank lines around the bullet lists in release-sync SKILL.md (line 80) and AgDR-0052 (line 36). Refs #403 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(#408): CHANGELOG v2.1.0 + site version pill bump - Append [2.1.0] section above [2.0.2] - Bump site/index.html version pill v2.0.2 → v2.1.0 - Last cherry-pick release; /release-sync now live for v2.2.x+ Refs #408 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(me2resh#404): remove stale --pdf-output-folder flag from md-to-pdf dispatch (me2resh#405) md-to-pdf removed --pdf-output-folder and --dest-name in a breaking API change; convert.sh still passed both flags, causing every npx md-to-pdf invocation to abort with: ArgError: unknown or unexpected option: --pdf-output-folder Fix: stage the source into a private temp dir under the desired output stem (e.g. WORK/out.pdf → WORK/out.md), invoke `npx -y md-to-pdf <tmp>.md`, then mv the resulting <tmp>.pdf to the requested TO_ABS. md-to-pdf writes <basename>.pdf next to the source by default, so this is compatible with every currently-released version. The same fix is applied to the md-to-pdf-html branch (HTML → PDF fallback), which had identical stale flags. Adds regression test at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh covering four cases: successful conversion, absence of the stale --pdf-output-folder flag, absence of --dest-name, and custom output path resolution. All four pass locally against the current npm latest. Refs me2resh#404 Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(me2resh#403): /release-sync — main→dev sync after each release (me2resh#406) * feat(me2resh#403): /release-sync skill — main→dev sync after each release Introduces the /release-sync skill to address squash-merge divergence that accumulated across releases and produced 99 conflicts on the v2.0.0 release PR. After each release, /release-sync vX.Y.Z opens a sync PR that merges upstream/main into upstream/dev with -X ours (dev wins on conflicts), making the squash commit an ancestor of dev so future release PRs only show genuinely-new commits. - .claude/skills/release-sync/SKILL.md — new skill spec with pre-flight checks, idempotent no-op, edge-case table, and PR body template - .claude/skills/release/SKILL.md — adds Step 9 (mandatory invoke of /release-sync) and updates Related section - docs/agdr/AgDR-0052-release-sync-skill.md — records the design choice (explicit skill vs auto-invoke vs switch-to-merge-commit) and the merge-strategy direction reasoning (-X ours on a dev-based branch) - .claude/hooks/tests/test_release_sync.sh — 11 sandbox-based smoke tests covering already-in-sync no-op, diverged detection, branch name shape, -X ours direction, idempotence, post-sync state, and version argument validation; all 11 pass Refs me2resh#403 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(me2resh#403): refresh site skill-count (54→55) + 2 MD032 lint fixes Address red CI on Rex-approved release-sync PR: - site-counts drift: new /release-sync skill bumps total to 55. Update all 12 marketing-copy references across architecture.html, *.md.gen, llms.txt, llms-full.txt, skill.md (incl. 2 multiline / phrasing-variant references caught by the post-me2resh#377 cross-newline scanner). - markdownlint MD032: add blank lines around the bullet lists in release-sync SKILL.md (line 80) and AgDR-0052 (line 36). Refs me2resh#403 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(me2resh#408): CHANGELOG v2.1.0 + site version pill bump - Append [2.1.0] section above [2.0.2] - Bump site/index.html version pill v2.0.2 → v2.1.0 - Last cherry-pick release; /release-sync now live for v2.2.x+ Refs me2resh#408 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

…tch (#405) md-to-pdf removed --pdf-output-folder and --dest-name in a breaking API change; convert.sh still passed both flags, causing every npx md-to-pdf invocation to abort with: ArgError: unknown or unexpected option: --pdf-output-folder Fix: stage the source into a private temp dir under the desired output stem (e.g. WORK/out.pdf → WORK/out.md), invoke `npx -y md-to-pdf <tmp>.md`, then mv the resulting <tmp>.pdf to the requested TO_ABS. md-to-pdf writes <basename>.pdf next to the source by default, so this is compatible with every currently-released version. The same fix is applied to the md-to-pdf-html branch (HTML → PDF fallback), which had identical stale flags. Adds regression test at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh covering four cases: successful conversion, absence of the stale --pdf-output-folder flag, absence of --dest-name, and custom output path resolution. All four pass locally against the current npm latest. Refs #404 Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(me2resh#404): remove stale --pdf-output-folder flag from md-to-pdf dispatch (me2resh#405) md-to-pdf removed --pdf-output-folder and --dest-name in a breaking API change; convert.sh still passed both flags, causing every npx md-to-pdf invocation to abort with: ArgError: unknown or unexpected option: --pdf-output-folder Fix: stage the source into a private temp dir under the desired output stem (e.g. WORK/out.pdf → WORK/out.md), invoke `npx -y md-to-pdf <tmp>.md`, then mv the resulting <tmp>.pdf to the requested TO_ABS. md-to-pdf writes <basename>.pdf next to the source by default, so this is compatible with every currently-released version. The same fix is applied to the md-to-pdf-html branch (HTML → PDF fallback), which had identical stale flags. Adds regression test at .claude/skills/pdf/tests/test_md_to_pdf_fallback.sh covering four cases: successful conversion, absence of the stale --pdf-output-folder flag, absence of --dest-name, and custom output path resolution. All four pass locally against the current npm latest. Refs me2resh#404 Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(me2resh#403): /release-sync — main→dev sync after each release (me2resh#406) * feat(me2resh#403): /release-sync skill — main→dev sync after each release Introduces the /release-sync skill to address squash-merge divergence that accumulated across releases and produced 99 conflicts on the v2.0.0 release PR. After each release, /release-sync vX.Y.Z opens a sync PR that merges upstream/main into upstream/dev with -X ours (dev wins on conflicts), making the squash commit an ancestor of dev so future release PRs only show genuinely-new commits. - .claude/skills/release-sync/SKILL.md — new skill spec with pre-flight checks, idempotent no-op, edge-case table, and PR body template - .claude/skills/release/SKILL.md — adds Step 9 (mandatory invoke of /release-sync) and updates Related section - docs/agdr/AgDR-0052-release-sync-skill.md — records the design choice (explicit skill vs auto-invoke vs switch-to-merge-commit) and the merge-strategy direction reasoning (-X ours on a dev-based branch) - .claude/hooks/tests/test_release_sync.sh — 11 sandbox-based smoke tests covering already-in-sync no-op, diverged detection, branch name shape, -X ours direction, idempotence, post-sync state, and version argument validation; all 11 pass Refs me2resh#403 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(me2resh#403): refresh site skill-count (54→55) + 2 MD032 lint fixes Address red CI on Rex-approved release-sync PR: - site-counts drift: new /release-sync skill bumps total to 55. Update all 12 marketing-copy references across architecture.html, *.md.gen, llms.txt, llms-full.txt, skill.md (incl. 2 multiline / phrasing-variant references caught by the post-me2resh#377 cross-newline scanner). - markdownlint MD032: add blank lines around the bullet lists in release-sync SKILL.md (line 80) and AgDR-0052 (line 36). Refs me2resh#403 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(me2resh#408): CHANGELOG v2.1.0 + site version pill bump - Append [2.1.0] section above [2.0.2] - Bump site/index.html version pill v2.0.2 → v2.1.0 - Last cherry-pick release; /release-sync now live for v2.2.x+ Refs me2resh#408 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions Bot added github-issue has-ticket labels May 24, 2026

atlas-apex commented May 24, 2026

View reviewed changes

atlas-apex merged commit 6a6f3b0 into dev May 24, 2026
2 checks passed

atlas-apex deleted the fix/GH-404-pdf-convert branch May 24, 2026 18:27

atlas-apex mentioned this pull request May 24, 2026

release(#408): v2.1.0 #409

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#404): remove stale --pdf-output-folder flag from md-to-pdf dispatch#405

fix(#404): remove stale --pdf-output-folder flag from md-to-pdf dispatch#405
atlas-apex merged 1 commit into
devfrom
fix/GH-404-pdf-convert

atlas-apex commented May 24, 2026

Uh oh!

atlas-apex left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

atlas-apex commented May 24, 2026

Summary

Testing

Glossary

Uh oh!

atlas-apex left a comment

Choose a reason for hiding this comment

Code Review: PR #405

Summary

Checklist Results

Verification

Issues Found

Suggestions

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants