feat(logs): let the sink serve on_output hooks

[jdx]

Stacked on #667 — review that one first; this PR's diff is against it.

The last exclusion

After #667, an on_output hook was the only non-PTY reason a daemon still used in-process capture, and so the only remaining reason for the supervisor to sit in a daemon's data path. Killing the supervisor left the pipe readerless, the daemon took SIGPIPE, and both its logging and its hook stopped — with the hook's owner, who is usually watching for something going wrong, none the wiser.

The change

The sink already reads every line, so it applies the hook's filter/regex and its debounce, and reports the lines that qualify. The supervisor fires the hook on what it is told.

Why the debounce moves to the sink. A hook with neither filter nor regex fires on every line, so leaving the debounce supervisor-side would mean one IPC message per line of output. Applying it in the process that sees every line makes it one message per window.

Why the supervisor does not re-apply it. Its on_output_last_fired clock starts when the monitoring task does, which is later than the sink's, so re-deciding would suppress firings the sink correctly allowed. Relayed lines are fired directly; locally-read lines take the old path unchanged.

OutputLine now carries an OutputSource rather than the persist bool #667 introduced, because the answer drives two decisions rather than one: whether the line still needs storing, and whether it still needs filtering. Local and Sink say that plainly where a bool named for one of them would not.

The hook is validated before its config is handed to a sink, so an invalid one is dropped rather than passed along — the same thing in-process capture does with it, instead of falling through to the "no filter, no regex" arm and firing on every line.

Coverage after this

	capture	crash-safe
plain daemons	sink	yes (2.19.0)
ready_output	sink	yes (#667)
on_output	sink	yes, here
pty = true	in-process	no

PTY is genuinely different — output arrives on a terminal master, not a pipe — so it needs the sink to be handed the master fd rather than a pipe read end. Left alone, and the module docs say so.

Tests

• on_output fires from a sink rather than in-process capture — hook fires, a sink owns the stream, and the line is stored exactly once
• an on_output daemon keeps logging through a supervisor crash — SIGKILL the supervisor, daemon lives, output keeps accruing
• Unit tests for the hook matcher: repeat firings (unlike readiness, which happens once) and debounce suppression within a window

Both bats tests were mutation-tested — restoring on_output_hook.is_none() to is_supported fails both. 539 unit tests; 56 bats across hooks, logs and pty.

---

This PR was generated by Claude Code.

---

Note

Medium Risk
Changes daemon lifecycle, IPC, and when user-configured hook commands run; behavior is well-tested but mistakes could miss hooks or fire them incorrectly.

Overview
on_output hooks now run with out-of-process log capture, so daemons with hooks no longer force the supervisor to read stdout/stderr in-process (PTY daemons still do).

The log-sink sidecar gains --report-output plus filter/regex/debounce flags. It applies the hook’s matching and rate limit while draining the pipe, then reports qualifying lines over IPC as SinkOutputLine (replacing readiness-only SinkReadyMatch) with a fires_hook bit so readiness-only lines do not trigger the hook.

Supervisor wiring uses WatchFor to pass ready_output and validated hook config into the sink, registers an output relay when either is present, and replaces OutputLine::persist with OutputSource (Local vs Sink { fires_hook }) for persistence and hook firing. The monitoring loop trusts fires_hook for sink-relayed lines and still debounces locally on fallback paths.

Docs/CLI specs are regenerated; bats and unit tests cover sink-driven hooks, crash-survivable logging, debounce, and readiness vs hook separation.

^{Reviewed by Cursor Bugbot for commit af2056f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Warning

Review limit reached

@jdx, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: bd454cd6-5899-4f0b-b291-0c05df7f5996

📥 Commits

Reviewing files that changed from the base of the PR and between f161795 and af2056f.

📒 Files selected for processing (10)

• docs/cli/commands.json
• pitchfork.usage.kdl
• src/cli/log_sink.rs
• src/ipc/client.rs
• src/ipc/mod.rs
• src/supervisor/ipc_handlers.rs
• src/supervisor/lifecycle.rs
• src/supervisor/log_sink.rs
• src/supervisor/mod.rs
• test/hooks.bats

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 75233b0. Configure here.}

[greptile-apps]

Greptile Summary

The PR moves non-PTY on_output matching and debounce into the log sink while preserving supervisor-side hook execution.

• Adds sink-side output filtering, regex matching, debounce, and IPC reporting.
• Distinguishes readiness-only reports from lines that should fire on_output.
• Retains supervisor-side debounce state across sink replacement.
• Adds unit and integration coverage for sink-owned hooks and crash-safe logging.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; readiness-only reports no longer fire differently filtered output hooks, and the retained supervisor debounce prevents replacement sinks from firing twice within one semantic debounce window.

Important Files Changed

Filename	Overview
src/cli/log_sink.rs	Adds hook matching and per-line fires_hook reporting while keeping readiness and hook qualification independent.
src/supervisor/lifecycle.rs	Correctly honors sink-provided hook qualification and retains the monitoring task's debounce clock across replacement sinks.
src/supervisor/log_sink.rs	Passes validated hook configuration to sinks and preserves relay-token scoping through sink replacement.
src/ipc/mod.rs	Extends sink output reports with an explicit hook-qualification flag carried through the IPC protocol.
test/hooks.bats	Covers sink-owned hook execution, single persistence, and continued logging after supervisor failure.

_{Reviews (4): Last reviewed commit: "chore: re-render the usage file for clap..." | Re-trigger Greptile}

[jdx]

Both findings were real; fixed in 17589b6.

Readiness reports triggering output hooks (@greptile-apps P1, @cursor). Straightforwardly my bug. A daemon can have ready_output = "LISTENING" and on_output = { filter = "ERROR", ... }; the sink reports the line that announced readiness, and from_sink treated any relayed line as hook-worthy, so the hook's command ran for a line that never matched its filter.

The report now says why it was sent. SinkOutputLine carries fires_hook, set by the sink from its own filter and debounce, and the supervisor fires the hook only when that is true. This is the answer it cannot re-derive — re-running the filter would be easy, but the debounce belongs to the process that sees every line.

Sink replacement resetting the debounce (@greptile-apps P1). Also right. A replacement sink starts with last_reported: None, so a hook could fire twice inside one configured window.

The debounce is now applied on both sides instead of bypassed supervisor-side. My original reasoning for the bypass — that the supervisor's clock starts later and would suppress a firing the sink allowed — turns out to be worth very little: its last_reported begins as None, so the first relayed line always fires, and from then on the two clocks differ only by IPC latency. Trading a few milliseconds of skew at a window boundary for not double-firing after a sink crash is the right way round.

New unit test, a_readiness_only_line_does_not_fire_the_hook, covers exactly the reported case — readiness pattern and hook filter differing, and the readiness line reported with fires_hook: false — plus the converse, where one line matches both and says so.

541 unit tests; 52 bats across hooks and logs.

On the CI check: the only non-passing entry is Cursor Bugbot: skipping, which is the bot reviewing by comment rather than a failure.

---

This comment was generated by Claude Code.

[greptile-apps]

Both fixes look correct.

For the readiness/hook conflation: fires_hook on SinkOutputLine is the right shape — the sink is the only process that can evaluate filter + debounce together, so having it assert the result and the supervisor trust it (rather than re-deriving) is cleaner than any alternative.

For the double-fire after sink replacement: the reasoning holds. last_reported: None means the first relayed line always fires regardless, so the "supervisor clock starts later" concern only applies to subsequent lines where the IPC latency difference is negligible. Dual-side debounce is the right tradeoff.

The a_readiness_only_line_does_not_fire_the_hook test covering both the misfire case and the converse (one line matching both, fires_hook: true) is good coverage of the exact boundary.

Both P1s are resolved cleanly. No further concerns from me on this PR.

[jdx]

Rebased onto main (f161795) and re-rendered — af2056f.

The build failure here was not this PR's code. #666 bumped clap_usage 2 → 4, which writes a multi-paragraph long_help as a KDL raw string instead of one escaped line. This PR's CI tests the branch merged with current main, so it built against clap_usage 4 while pitchfork.usage.kdl still held v2-format text — for --ready-pattern and --relay-token from #667, and now --report-output from this PR. main itself is red for the same reason; #670 fixes that independently.

The three inline comments still showing on this PR are from 75233b0 and were fixed in the commit after it; GitHub re-anchors them onto the current diff. Cursor Bugbot and CodeRabbit both pass on the current head.

551 unit tests, lint clean.

---

This comment was generated by Claude Code.