Code Review: PR #287

Commit: 10b95a4da3edafe681892cf2a3fc6e6088e343f8

Summary

Adds the /pdf skill — converts framework-generated docs (markdown / HTML / BPMN) to PDF with a 4-option destination prompt that mirrors the existing "would it follow the code if the project spun out?" rule from docs/multi-project.md. Shells out to a converter-dispatch helper (convert.sh) that picks the best available backend from pandoc / md-to-pdf / wkhtmltopdf / bpmn-to-image and graceful-degrades (exit 3 + advisory) when no converter is installed. Same shape as /process (bpmnlint) and /c4 (Mermaid lint), so adopters who never need PDFs pay zero install cost.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass (no auth/secrets touched)
• Performance: Pass (synchronous shell, single conversion per invocation, no hot paths)
• PR Description & Glossary: Pass (6 well-explained terms)
• Technical Decisions (AgDR): Pass — AgDR-0034 covers all three load-bearing axes (standalone vs per-skill flag, single converter vs dispatch, prompt vs auto-pick)
• Adopter Handbooks: Pass — no architecture/migration/language-handbook triggers fire; commit-message-quality passes cleanly

Verification Done

• Exit-code taxonomy (0/1/2/3) matches sibling shapes in _lib-mermaid-lint.sh and process/lint.sh ✓
• set -uo pipefail (no -e) matches _lib-mermaid-lint.sh (intentional — explicit exit 1 after each converter invocation) ✓
• Portfolio path resolution: SKILL.md sources _lib-read-config.sh + _lib-portfolio-paths.sh and calls portfolio_projects_dir / portfolio_workspace_dir — no hardcoded literals ✓
• Test PATH-stripping technique correctly forces the "no converter" branch regardless of host environment; falls back to skip if a symlink leaks ✓
• BPMN multi-step pipeline (bpmn-to-image → SVG → pandoc → PDF) clearly described in SKILL.md § 6, AgDR-0034, and the convert.sh header comment ✓
• 4-option destination prompt clearly described (lines 117–130 of SKILL.md) with explicit hint text ✓
• AgDR slot 0034 doesn't collide with open PRs #285 (0031) or #289 (0033) ✓
• Commit message quality: subject + body + Closes #284 + Co-Authored-By ✓

Issues Found

None blocking.

Suggestions (non-blocking)

1. Smoke test §8 option 3 is a tautology. Lines 932–936:

   custom="/tmp/out.pdf"
   got="$custom"
   if [ "$got" = "/tmp/out.pdf" ]; then ok "option 3 → ..."; ...

   This asserts $custom == $custom. The intent — verify option 3 passes through the literal — is documented in the comments but not actually exercised. Consider replacing with a real call into a resolve_dest extension that handles option 3, or remove the assertion. Keeping it as-is is fine for v1; just flagging that it does not exercise real behavior.

2. convert.sh does not source the pdf.* config block. Config keys preferred_converter and pdf_engine are read by the model orchestrating the skill (SKILL.md instructs the agent to pass them as --converter / --pdf-engine flags), not by convert.sh itself. This is a reasonable separation-of-concerns choice — convert.sh stays config-free — but the SKILL.md "Path resolution" section could state it explicitly. Future readers may otherwise expect convert.sh to read the config block. Single-line addition under the Config heading would close the loop.

3. $eng_flag unquoted at lines 525/568/620 is the standard "optional flag" pattern (quoting an empty eng_flag would inject "" as a literal arg to pandoc). Intentional and correct, but a one-line comment # unquoted on purpose — empty PDF_ENGINE means omit the flag entirely would prevent a well-meaning future shellcheck-driven cleanup from breaking it.

4. npx … >&2 routing converter chatter to stderr (lines 537/579/599) is unusual — most callers route to /dev/null and trust the exit code. Current shape keeps the operator informed during the npx fetch (~5s cold start) which is helpful UX; just noting for any future refactor.

5. AgDR slot ordering. This PR uses AgDR-0034 while slots 0032 / one of {0033} are still unallocated on dev. Not blocking — apexyard's AgDR sequence has accepted gaps before — and PRs #285 (0031) and #289 (0033) keep the global sequence monotone-ish. Worth a one-liner in the AgDR's intro if there's a story behind the gap; otherwise let it be.

Verdict

APPROVED

The PR is consistent with the agent's plan, mirrors the existing graceful-degrade patterns (/process lint, _lib-mermaid-lint.sh), routes through portfolio_* helpers (so split-portfolio adopters get correct paths transparently), and ships an AgDR that genuinely earns its keep across three real design axes. The 4-option destination prompt is a thoughtful echo of the existing "would it follow the code?" rule and avoids the silent-auto-pick foot-gun. Tests cover the load-bearing behaviors. The five style nits above are suggestions, not requested changes.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 10b95a4da3edafe681892cf2a3fc6e6088e343f8