chore(deps): bump termimad to 0.35.1, drop ragged-table panic workaround - #3422
Conversation
termimad 0.35.1 fixes the ragged-table out-of-bounds panic in Table::fix_columns (Canop/termimad#77) that #3408 contained with a catch_unwind guard around the PR-comments render path. With the upstream fix, remove the workaround and render directly again. Verified 0.35.1 renders the reproducer across the full narrow-width band (16/20/24, previously panicked) without incident. The bump also pulls minimad 0.16, which no longer treats a single-dash (`| - |`) row as a table separator; worktrunk only ever emits multi-dash (`---`) separators, so real help/list output is unaffected (help/docs snapshot tests unchanged). Updated the one artificial single-dash test input to the canonical `---` form. The regression test now guards against reintroducing the panic via a downgrade rather than asserting the plain-text fallback.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewed independently (self-authored PR, so commenting rather than approving). The mechanics are clean: verified that render_data_table emits | --- | and no single-dash separator survives anywhere in src/, docs/, or skills/, so the minimad 0.16 separator change can't touch real help/list output — consistent with the snapshot tests not moving. The test repurposing is reasonable.
One point worth an explicit maintainer decision rather than defaulting to removal. Dropping catch_unwind removes the only panic containment left in the tree (git grep catch_unwind is now empty), and it sat on the untrusted-input path that matters most: the picker renders PR-comment markdown through render_markdown_flush → render_table_with_termimad → MadSkin::text inside a rayon::spawn_fifo worker (preview_orchestrator.rs), where an escaped panic isn't contained — it takes down the whole wt switch process, not just the preview pane.
My independent lean differs from the PR's default: I'd keep the guard. The argument for removal ("the only reachable panic class is fixed") is a negative that can't really be proven for a markdown renderer fed arbitrary comment bodies, and the failure mode if a future termimad regression proves it wrong is a full-process abort while a user is browsing PRs — exactly the kind of blast radius defense-in-depth exists for. The stated cost of keeping it (an uncovered fallback branch tripping the coverage gate) is the weaker consideration and is solvable via the #[cfg]-gated / coverage-exempt shape you already offered in the description.
Not blocking — just registering the uncorrelated signal that the tradeoff leans the other way for me. Your call.
Follow-up to #3408, as promised in that thread: @Canop published termimad 0.35.1 with the fix for the ragged-table panic (Canop/termimad#77). This bumps to it and drops the
catch_unwindworkaround.What changed
termimad0.34.1 → 0.35.1 (pulls transitiveminimad0.14 → 0.16).catch_unwindguard inrender_table_with_termimad— the only panic reachable through this path was theTable::fix_columnsout-of-bounds on a ragged narrow table, now fixed upstream. (The broken-pipe panics termimad has had historically live inprint_text, an I/O path we don't use — we callMadSkin::text, which returns aString.)test_render_table_ragged_narrow_does_not_panicnow guards against reintroducing the panic via a downgrade rather than asserting the plain-text fallback.Verification
Ran the original reproducer against a clean
termimad = "=0.35.1"scratch crate across the full width band — all render, none panic (16/20/24 previously panicked):md_helpsuite (26), picker unit tests (226), and the integrationtest_help/test_docs_are_in_syncsnapshots (48) all pass.cargo clippyclean.One rendering note
minimad0.16 no longer treats a single-dash delimiter row (| - | - |) as a table separator — it renders as cell content. Worktrunk only ever emits multi-dash (---) separators (render_data_tablebuilds| --- | ... |, and every hand-authored help table uses-----), so real help/list output is byte-for-byte unchanged — the help/docs snapshot tests didn't move. The one test that used a single-dash input was artificial; I switched it to the canonical---form.Defense-in-depth tradeoff — your call
Dropping
catch_unwindremoves a general safety net around rendering untrusted PR-comment markdown on a rayon worker (where an escaped panic aborts the whole process). I removed it because (a) the only reachable panic class here is fixed, and (b) keeping it would leave the fallback branch untestable and uncovered — no input triggers it anymore, which trips the coverage gate. If you'd rather retain the guard as belt-and-suspenders against a future termimad regression, say the word and I'll restore it with a#[cfg]-gated or otherwise coverage-exempt shape.