feat: add external-command hooks (pipe-to, exec_on_new, macros)#39
Merged
Conversation
Users keep asking for "save to wallabag", "pipe to yt-dlp",
"notify on new article", and so on. Chasing one integration at
a time is a losing game.
So instead, three newsboat-style primitives:
* `pipe-to` pipes the focused article to a shell command's
stdin (body / title / url / metadata, configurable), with
the TUI suspended so pagers and editors behave.
* `exec_on_new` fires a command per newly-seen item after
each refresh. The first fetch of a feed is seeded silently
so enabling this on a 200-item feed doesn't notify-spam
you into oblivion.
* Macros bind a chord (default prefix `,` then key) to a
sequence of actions. Newsboat-compatible string syntax,
so you can paste their configs.
Templates are tokenized once via shlex and `%X` is substituted
into individual argv slots — *not* run through `sh -c`. That
means you can't put a `;` in your template and have a pipeline.
Wrap it in a script. The alternative is letting a malicious
feed title execute arbitrary commands, and that's not a trade
I'm willing to make.
Macros queue every step (Action / PipeTo / Exec) onto a FIFO
list which the TUI loop drains in order, so newsboat-style
sequential semantics are preserved across blocking steps. The
terminal-suspend dance uses an RAII guard so a panicking child
can't leave the user stuck in raw mode.
Config and SavedData are fully back-compat — all new sections
and fields are \`serde(default)\`. 103 unit tests + 4 integration
tests pass; clippy clean.
Six bugs and a pile of polish from the principal review on the hooks branch. Nothing here changes the user-facing shape of macros or exec_on_new — it's all the things that go wrong when you stare at the code for an hour. The biggest one: `exec_on_new` could fire twice for the same item after a crash. The reason was that `mark_feed_seen` updates the in-memory seen-set as each feed arrives in a refresh batch, but `save_data` only ran at the end of the batch. Kill feedr mid-batch and the persisted seen-set is stale — next launch, the same item gets re-fired. For a `notify-send` hook that's annoying. For `wallabag-cli add` it's actively wrong. Persist before the spawn loop. At-most-once beats at-least-once when you're firing arbitrary external commands. The next one: the macro prefix had no escape hatch. Press `,` and walk away, and the next keystroke is silently consumed as a macro follow-up — including a `j` you meant as move-down. ESC now cancels cleanly, the prefix wait times out on tick, and unmatched follow-ups still error rather than falling through, because I'd rather surface a typo than silently jump-top. While at it: pre-tokenize the exec_on_new template once at startup instead of re-shlex-ing it for every feed arrival; surface friendly dashed action names (`open-in-browser` not `OpenInBrowser`) in errors and the help overlay; track first-failure with a local bool so the comment matches the code; switch `pending_macro_steps` to a VecDeque so the drain isn't O(n) per step. Tidy the byte-level \` -- \` separator check while I was in there. Inline help in `config.rs` now spells out the supported actions inside macros, warns that \`~\`/\`\$HOME\` are *not* expanded (no shell, no expansion, please don't be surprised), points out that wrapping in \`sh -c\` re-introduces shell injection through item titles, and documents the at-most-once semantics for exec_on_new so users choose idempotent commands. Tests for the new behavior: ESC cancellation, prefix-then-unbound, prefix-then-bound, \`dispatch_action\` rejecting unsupported actions with the friendly name, the \`KeyAction::as_str\` <-> \`FromStr\` round-trip so future variants can't silently drift, and a SavedData round-trip for the new persistence fields plus back-compat with the old shape. 110 tests pass, clippy clean with -D warnings.
A code review on top of the first review-feedback commit turned up three things that are *not* OK to ship, plus a few smaller ones. The big one: dispatch_action read app.selected_feed for ToggleStar / ToggleRead, but selected_feed is only ever set when you drill into FeedItems or FeedItemDetail. So a macro like `,s = toggle-star` silently no-op'd on Dashboard and Starred views. The per-key handlers in those views *do* resolve the focused item correctly via active_dashboard_items() / get_starred_dashboard_items(), but the macro path was happily ignoring all of that. Two entry points, two answers for "which item is focused." Not great. Add App::current_article_indices() as the single source of truth, have current_article_context() delegate to it, and route the toggle helpers through it. Now keypress and macro agree on what "current" means. The other big one: spawn_detached spawned a child and dropped the Child without ever waiting on it. On Unix this leaves the kernel holding a zombie until the parent exits. Feedr is a long-running TUI; a user with exec_on_new = 'notify-send …' and a handful of active feeds would accumulate <defunct> processes indefinitely. Stick the Child in a reaper thread that just waits. One thread per child is fine for our scale and it's cross-platform. While at it: fire_exec_on_new was mutating seen_items and calling save_data() on *every* feed arrival even when no hook was configured, which means users who never opted in were paying for an unbounded growing set on disk for absolutely no reason. Short- circuit at the top when the template is None. Also: suspend_for_command was writing the stdin payload synchronously on the main thread before waiting on the child, which can deadlock if the payload is bigger than the pipe buffer (64 KiB on Linux) and the child hasn't started reading yet. Spawn a writer thread, let it run in parallel with child.wait(). Drop the dead stdin_payload parameter from spawn_detached while I'm in there — both call sites pass None. Add regression tests for the two dispatch_action cases. Fix a stale doc comment referencing a macro_prefix_timeout config that doesn't exist.
drain_macro_steps documented "on first error the rest of the queue is dropped" but only PipeTo spawn failures actually obeyed: Exec spawn failures and Action errors (e.g. open-in-browser with no URL) silently continued through the chain. A chain like `open-in-browser ; toggle-star` should not silently toggle-star when the browser launch failed — the user only sees the error and would not expect side-effecting follow-up steps to have run. Track app.error before each step and clear/break the queue when a step introduces a new error. Same break applied to Exec spawn failures for parity with PipeTo. Three drain-path tests added covering halt-on-action-error, clean-chain runs every step, and that a preexisting error isn't mistaken for a step-introduced failure.
Four things turned up on the third pass through the hooks branch. Two of them were latent bugs the earlier reviews missed; two were the kind of write-amplification and resource-leak issues that don't bite anyone in a 100-test suite but get embarrassing in production. The biggest one: a `,r = toggle-read` macro on the Dashboard with "unread only" filtered didn't drop the just-read item from the list. The Dashboard keypress called `apply_filters()` inline; the macro path went through `dispatch_action -> handle_toggle_read_current` and silently skipped it. Two execution paths, two answers for "did the dashboard refresh." Move `apply_filters()` into the helper so every caller -- macro, FeedItems, FeedItemDetail -- agrees. Next: `mark_feed_seen` flipped `feeds_seeded` even when the first fetch returned zero items. So a transiently-empty fetch (server hiccup, parser edge case) silently armed the firehose: the *next* non-empty fetch treated every backlog item as new. The first-fetch suppression existed precisely to prevent this. One-line guard on `!feed.items.is_empty()`. Please don't congratulate yourself for seeding a feed you haven't actually seen. Then: `fire_exec_on_new` ran `save_data()` once per feed arrival. With 50 bookmarks and the hook configured, that's 50 whole-file JSON writes per refresh. Split into `collect_exec_on_new` (in-memory only) and `flush_exec_on_new` (one save, then spawn); the receive loop accumulates argv across the batch and flushes once. AT-MOST-ONCE crash semantics are preserved -- save still happens before any spawn, just at batch granularity now. Finally: `remove_current_feed` never dropped the URL from `feeds_seeded` or its item IDs from `seen_items`. So the persisted JSON grew monotonically across feed churn for absolutely no reason. Drop them before removing the feed itself. Three regression tests added: dashboard filter reapply after macro toggle-read, empty-first-fetch suppression, and feed removal cleanup. 118 tests pass, clippy clean, fmt clean.
Five commits of feature work landed on this branch and the docs never caught up. Anyone reading the README would have no idea feedr now does newsboat-style macros, pipe-to, exec, or exec_on_new notifications. CLAUDE.md still described the codebase as if none of this existed. Let's fix that. README gets a Features bullet plus a dedicated subsection covering the no-shell argv expansion model, the %X template variables, macro chain syntax + step kinds, exec_on_new with its at-most-once crash semantics, and the security footguns. The `sh -c "... %t..."` warning is worth repeating outside of the in-code config comments — that is the one mistake that turns a safe feature into shell injection through item titles, and I would rather say it twice than once. CLAUDE.md gets two new "Key patterns" bullets explaining the engine architecture future-me will want: queue-fill in events.rs, drain in tui.rs because pipe-to needs the terminal handle to suspend the TUI; save-then-spawn ordering for at-most-once semantics; first-fetch seeding to prevent the firehose; transiently-empty-fetch guard so we do not seed prematurely. Module descriptions updated to mention the new responsibilities. Not glamorous, but the feature is not shipped until the docs say it is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds three newsboat-style extensibility primitives so users can self-serve the long tail of "save to X / notify on Y / pipe to Z" workflows without feedr chasing one-off integrations:
pipe-to— pipes the focused article (body / title / url / metadata, configurable per macro step) to a shell command's stdin. The TUI is suspended for the duration so pagers and editors take over the terminal cleanly.exec_on_new— fires a per-item command after each refresh. The first successful fetch of a feed seeds the seen set silently, so enabling this on a 200-item feed doesn't notify-spam.,then key) to an ordered chain of actions / pipe-to / exec steps. Newsboat-compatible string syntax (open-in-browser ; pipe-to "yt-dlp %u"); existing newsboat macros mostly paste in directly.Design notes
Safety stance. Templates are tokenized once via
shlex::splitand%Xplaceholders are substituted into individual argv slots. Commands are not run throughsh -c. That meanspipe-to "tee out.txt | wc -l"won't pipeline — wrap it in a script. The alternative is letting a malicious feed title execute arbitrary commands, which is not a trade I was willing to make. Documented in the auto-generated config comments.Sequential macro semantics. All three step kinds (
Action,PipeTo,Exec) are queued ontoapp.pending_macro_steps(FIFO) and drained in order by the TUI loop. Earlier draft ranActionsteps inline and queued only the externals — that brokepipe-to "X" ; toggle-readordering. Refactored before commit.Terminal suspend.
tui::suspend_for_commanddoes the verified ratatui dance:LeaveAlternateScreen → DisableMouseCapture → disable_raw_mode → spawn → wait → enable_raw_mode → EnterAlternateScreen → EnableMouseCapture → terminal.clear(), with an RAII guard so a panicking child can't strand the user in raw mode. Pending input drained on return to absorb terminal-probe responses (less/vimbackground-color queries).exec_on_newfirst-fetch suppression. A new persistedfeeds_seeded: HashSet<String>records URLs that have been fetched at least once. The very first successful fetch silently seedsseen_itemswithout firing hooks. Persists across restarts viafeedr_data.json(additiveserde(default)field, fully back-compat with v0.7.0 data files).Config schema (all
serde(default))Template variables:
%ttitle,%uurl,%aauthor,%ddate,%ffeed-title,%Ffeed-url,%%literal%.Files touched
src/keybindings.rs—MacroStep,MacroBinding,StdinKind,MacroOptions,parse_macro_string,build_macros,binding_displayhelpersrc/config.rs—[hooks],[macros],[macro_options]sectionssrc/app.rs—seen_items/feeds_seededpersistence,ArticleContext,current_article_context,mark_feed_seen,expand_argv_template,make_pipe_payloadsrc/events.rs— macro-prefix detection inhandle_key_event,run_macroqueueing,dispatch_actionsrc/tui.rs—suspend_for_command,spawn_detached,drain_macro_steps,fire_exec_on_newwith first-fetch suppressionsrc/ui/modals.rs— Macros section in the help overlayCargo.toml—shlex = "1.3"(for argv tokenization)Test plan
cargo test --lib— 103 tests passing (12 new for macro parser, 8 new for app helpers, 2 new for config back-compat / round-trip)cargo test --tests— 4 integration tests passingcargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— cleancargo build --release— clean[macros] x = 'pipe-to "cat"', press,xon an article — body appears in stdout, terminal restores cleanly on Ctrl+D.[hooks] exec_on_new = 'tee -a /tmp/feedr.log', refresh — log file is silent on first fetch of a feed, then gets one line per genuinely new item on subsequent refreshes.[macros] y = 'open-in-browser ; pipe-to "yt-dlp %u"'on a YouTube link — opens browser, then pipes URL to yt-dlp.Notes for reviewers
sh -cis the most consequential design decision here. Open to revisiting if there's pushback, but a per-step opt-inshell = truewould be the right shape rather than a global flag.