feat(inbox): the machine taps you when a PR lands, not just the tab by jtomaszewski · Pull Request #31 · fullstackhouse/cerber

jtomaszewski · 2026-08-21T14:55:26Z

The problem

Cerber's arrival notification lives entirely in the cockpit page — new Notification() from web/src/notify.ts, no service worker, and nothing on the server side at all (grep osascript notify-send node-notifier dist/ → nothing). Every one of these has to hold for a popup to happen:

a cockpit tab is open and running JS,
Notification.permission === "granted" and the localStorage pref isn't off,
the tab isn't focused at that moment,
the key is new to that browser.

So it is quiet in exactly the situation it exists for: you're in an editor, the cockpit is a tab you closed this morning, and the PR that landed at 14:30 waits until you next think to look.

This was found from the outside, on a real box: serve up for 2.5h, permission allow, pref on, a live tab polling — and no taps. Sampling the cockpit's socket once a second showed why the "10s poll" isn't one: bursts 61 seconds apart, Chrome's intensive throttling of background timers. Alive, but at the browser's discretion. The whole channel is at the browser's discretion.

The change

The poll that discovers the PR announces it, through whatever the machine already has:

macOS — osascript -e 'display notification …'
Linux — notify-send
anywhere else — quiet, and said once in the log rather than every poll

On by default (daemon.notify), off with the Settings checkbox or --no-notify, re-read every poll like the other knobs.

The stub artifact is the ledger. A PR is announced on the poll that first writes one for it — no new state file, a restart re-announces nothing, and a PR that arrived while serve was down is still news the next time it runs.

One PR, one popup — but never one PR, no popup. status.notify, the flag the cockpit stands down on, means this machine gets told, not this machine was asked to tell: it carries the last attempt's answer, so a box with no notifier hands the bell straight back rather than leaving the arrival to nobody. Recomputed rather than latched, so a notifier that comes back takes the job with it.

One PR, one popup. The cockpit's bell is better where it can ring — it opens that review on click — so it keeps that job and stands down only where it would be a duplicate: same machine (loopback host), machine tap on. The bell in the top bar says so instead of claiming a job it isn't doing, and unticking the machine one hands it back. A cockpit served from a VPS (-H 0.0.0.0 --token) keeps ringing either way — deferring to a notifier nobody can see is how a notification quietly stops existing.

Worth a look in review

A PR title is somebody else's text, and neither notifier takes it as plain data. osascript takes a script, not arguments, so appleScriptLiteral escapes the backslash and the quote — the only two characters a literal can't hold — and flattens control characters. Tested against " & (do shell script "touch /tmp/pwned") & "; ran for real, no /tmp/pwned. notify-send has the milder version of the same problem: it parses options before positionals, so a title beginning with - would be read as a flag and cost the notification. An explicit -- ends option parsing ahead of both.
A notifier's failure never costs a poll: notify() returns false rather than throwing, is bounded by a 5s deadline so a wedged Notification Centre or an unanswering DBus can't hang the poll that awaits it, and the missing-notifier line is logged once per process.
Fired before reviewAll, not after — the arrival is the news, drafting it takes minutes.

Verification

pnpm typecheck && pnpm test && pnpm build — 417 tests, 28 files, all green. 24 new tests: the notice shapes and folding, the escaping (including the hostile title), platform selection, and on the daemon — announces a new arrival, folds a batch into one call, says nothing about a PR it already stubbed, obeys the toggle, survives a machine with no notifier, and publishes the flag the cockpit defers to.

Also run end to end on macOS: real notification delivered for a normal title and for the hostile one.

One flake found and fixed while delivering. The arrival tests failed about one run in three, notified called twice with the second call naming a PR from the previous test. handle.stop() clears the daemon's timer but doesn't wait for a poll already in flight, and every beforeEach rebinds CERBER_HOME — so a straggling poll did its discovering inside the next test's home and announced there. stopAndDrain waits for the poll to land before the test returns; applied at all five stop sites, since they all had the leak and the new tests were only the first to assert on something a straggler could touch. 16 consecutive clean suite runs since.

The arrival notification lived entirely in the cockpit page: a Web Notification fired from an open tab, with no service worker behind it and nothing on the server side. So it was silent exactly when it mattered — tab closed, permission never granted, browser restarted, or the tab throttled to a crawl in the background. The poll that discovers a PR now announces it through whatever this machine already has: osascript on macOS, notify-send on Linux, quiet elsewhere. The stub artifact is the ledger, so a restart re-announces nothing and a PR that landed while serve was down is still news. The cockpit's bell keeps its click-through-to-the-review and stands down only where it would be a second popup about one PR — same machine, machine tap on. A cockpit served from a non-loopback host keeps ringing: that tap would land on the VPS, where nobody is looking. On by default, off with daemon.notify, --no-notify, or the Settings checkbox. A PR title goes into an AppleScript literal, so it is escaped as one — a title is somebody else's text.

Copilot

Pull request overview

Adds a server-side (“machine”) arrival notification path to Cerber’s inbox so you still get tapped when new PRs land even if no cockpit tab is open, while preventing duplicate notifications by having the browser bell stand down when the daemon is announcing on the same machine.

Changes:

Introduces a Node-side notifier (src/core/notify.ts) and wires it into the daemon poll loop, controlled by new daemon.notify config and --no-notify.
Updates the cockpit to detect when the daemon is announcing locally and disables/relabels the browser notification toggle and bell accordingly.
Extends config/types/docs and adds test coverage across core notify logic, daemon behavior, and the web decision logic.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
web/src/types.ts	Adds `notify` to daemon status/config types used by the cockpit.
web/src/styles.css	Styles a non-clickable (“stood down”) bell state when daemon announces locally.
web/src/Settings.tsx	Adds machine-level notify toggle and gates browser notify toggle when daemon announces.
web/src/notify.ts	Fetches daemon status during polling; suppresses browser popups when daemon announces locally.
web/src/notify.test.ts	Tests daemon-vs-browser “who announces” decision logic.
web/src/inbox.test.ts	Updates daemon status fixtures to include the new `notify` field.
web/src/App.tsx	Threads `daemonAnnounces` into the bell and Settings; shows a non-switch bell state.
src/server/daemon.ts	Announces arrivals via OS notification on discovery; publishes `status.notify` for the cockpit.
src/server/daemon.test.ts	Adds daemon tests ensuring announcements trigger/fold/skip correctly and reflect `status.notify`.
src/core/notify.ts	New core module: builds notices, escapes AppleScript literals, and shells out to OS notifiers.
src/core/notify.test.ts	Tests notice formatting, AppleScript escaping, and platform command selection.
src/core/config.ts	Adds `daemon.notify` to config schema with default `true`.
src/core/config.test.ts	Updates default config expectations for new `daemon.notify`.
src/cli/index.ts	Adds `--no-notify` and plumbs `notify` into daemon options.
README.md	Documents machine notifications + browser bell behavior and when each is used.
CLAUDE.md	Updates architecture notes to include the new notifier and cockpit stand-down behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…d flag notify-send parses options before positionals, so a PR titled "--help me" would be read as a flag: the notification fails and the arrival is lost. An explicit `--` ends option parsing, so the title and body can only ever be the summary and the body. The macOS path already had this covered — appleScriptLiteral escapes the literal — but the Linux one passed somebody else's text straight into an argument list that still had opinions about it.

…home The arrival tests failed about one run in three: `notified` was called twice, and the extra call named a PR from the *previous* test. `handle.stop()` clears the timer but does not wait for a poll already in flight, and every `beforeEach` points CERBER_HOME at a fresh directory. So a straggling poll did its discovering inside the next test's home, found that test's PR unstubbed, and announced it against that test's spy. `stopAndDrain` waits for the poll to finish before the test returns, which is what gives each test its own world. Used at all five stop sites, since they all had the same leak — the new tests were only the first to assert on something a straggler could touch. 16 consecutive clean suite runs.

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

web/src/notify.ts:90

window.location.hostname returns IPv6 literals without brackets (e.g., "::1"), so the "[::1]" entry in LOOPBACK will never match and can mislead future readers about what’s being compared.

const LOOPBACK = new Set(["127.0.0.1", "localhost", "::1", "[::1]"]);

web/src/App.tsx:44

The daemon-mode bell is rendered as a non-focusable , so keyboard/screen-reader users can’t reach the explanatory aria-label/title text (unlike the normal bell button). Make it focusable and give it an explicit role so the explanation remains accessible when the bell stands down.

      <span className="topbar-link bell bell-daemon" title={DAEMON_BELL_TITLE} aria-label={DAEMON_BELL_TITLE}>

…s the risky one Two from review. The bell that stands down is a span rather than a button, and an aria-label on a generic span is skipped — so the one thing it exists to say, why it is lit but not a switch, was not being said to a screen reader at all. `role="img"` puts the label back in the reading order. The notify-send comment and its test both said "title", which reads as the notification's summary — but that one is cerber's own text. The PR's title arrives as the notice *body*, and the body is the argument carrying somebody else's words. Same behaviour, named accurately.

jtomaszewski · 2026-08-21T15:25:02Z

On the two findings in the review's suppressed section (no threads, so answering here):

The stood-down bell's accessibility — agreed, fixed in b80b82b. It is a <span> rather than a <button> because it genuinely isn't a switch any more, but an aria-label on a generic span is skipped, so the one thing it exists to say was not being said to a screen reader at all. role="img" puts the label back in the reading order. I stopped short of tabIndex={0} deliberately: it would add a tab stop with nothing to activate, and the title tooltip is not keyboard-reachable on any element anyway — the same explanation is in Settings in full.

[::1] in LOOPBACK — this one has it backwards. Per the WHATWG URL standard the hostname getter serializes an IPv6 host with its brackets, so [::1] is the entry that matches and the bare ::1 is the redundant one:

$ node -e 'console.log(new URL("http://[::1]:4820/").hostname)'
[::1]

Both are kept as belt-and-braces — the set costs nothing and the failure mode of guessing wrong is a cockpit that silently double-announces — but the bracketed form is the load-bearing one, not dead code. Leaving the set as it stands.

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/core/notify.ts:107

notify() shells out without any timeout. If osascript / notify-send hangs (e.g., DBus issues, wedged Notification Center), the daemon poll will also hang even though the PR description says notifier failure should “cost the poll nothing”. Consider adding a short timeout so this stays best-effort and cannot stall polling.

export async function notify(n: Notice): Promise<boolean> {
  const cmd = notifyCommand(n);
  if (!cmd) return false;
  try {
    await execFileAsync(cmd.file, cmd.args);

`notify()` shelled out with no deadline. The poll awaits it, and the daemon refuses to start a poll while one is running — so an `osascript` waiting on a stuck Notification Centre, or a `notify-send` waiting on a DBus that never answers, doesn't lose you a popup. It stops cerber finding PRs at all, for as long as the process lives, and the strip goes on saying it is polling. Five seconds, which a notification either beats or is broken. execFile's own timeout kills the child and rejects, so it arrives at the existing catch and reads as the same ordinary "not shown" as a machine with no notifier. Caught in review — the PR claimed a notifier's failure never costs a poll, and that was only true of the failures that fail quickly.

jtomaszewski · 2026-08-21T15:29:47Z

The suppressed finding on notify() and the missing timeout is a good one, and it was a real bug — fixed in 7c2d916.

It's worse than one lost notification, which is why the PR description's claim that a notifier's failure "never costs a poll" was only true of failures that fail fast. The poll awaits announce(), and poll() opens with if (status.polling) return — so an osascript waiting on a stuck Notification Centre, or a notify-send waiting on a DBus that never answers, doesn't drop a popup. It stops cerber discovering PRs at all for the life of the process, while the daemon strip goes on reporting that it polls.

execFile's own timeout is the whole fix: it kills the child and rejects, so a hang arrives at the existing catch and reads as the same ordinary "not shown" as a machine with no notifier at all. Five seconds — a notification either beats that or is broken.

Two tests, and I checked they gate the fix rather than describe it: dropping the option back off makes the first one fail.

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/server/daemon.ts:477

status.notify is used by the cockpit to decide whether to stand down its own browser notifications, but it never reflects notifierMissing. If notify() fails once (e.g., notify-send not installed / osascript unavailable), the daemon will keep reporting notify: true and the cockpit will keep deferring, leaving the user with no notifications at all.

      // What the cockpit's bell defers to: a loop that isn't polling announces
      // nothing, whatever the notify toggle says.
      const notifyOn = opts.notify && knobs.notify;
      status.notify = knobs.poll && notifyOn;

src/server/daemon.ts:398

When the notifier is missing (or notify() fails), the daemon sets notifierMissing but leaves status.notify unchanged until the next poll recomputes it. Updating status.notify immediately here avoids the cockpit continuing to stand down (and going silent) right after a failure.

    if (notifierMissing) return;
    notifierMissing = true;
    log("no desktop notifier here — arrivals will only show in the cockpit");
  }

…comes `status.notify` is what the cockpit stands its own bell down on, so it has to mean "this machine gets told" — but it only meant "this machine was asked to tell". On a box with no `notify-send`, the daemon went on claiming the job while the browser politely declined it, and a PR was announced by nobody at all: the exact silence this feature exists to end. It now carries the answer of the last attempt. Recomputed on the spot after announcing rather than left to the next poll — that is minutes away, and the cockpit would spend them deferring to a tap that never came — and recomputed rather than latched, so a machine whose notifier comes back takes the job back with it. Untried still counts as working, or both bells would ring everywhere until the first PR landed. Caught in review. Two tests, each checked against the version without the fix: the hand-back fails on the old expression, the recovery on a latch.

jtomaszewski · 2026-08-21T15:37:08Z

Both suppressed findings this round are the same bug, and they're right — fixed in 4db709c.

status.notify is what the cockpit stands its own bell down on, so it has to mean this machine gets told. It only meant this machine was asked to tell: on a box with no notify-send, the daemon kept claiming the job while the browser politely declined it, and the PR was announced by nobody at all. That is precisely the silence the PR opens by describing, reintroduced one field over — and worse than the old behaviour, because the browser's bell had been working until this landed.

status.notify now carries the answer of the last attempt:

Recomputed right after announcing, not left to the next poll — that's up to five minutes the cockpit would spend deferring to a tap that never came. This is the second finding's point and it's the load-bearing half.
Recomputed rather than latched, so a machine whose notifier comes back takes the job back with it. A single transient timeout shouldn't hand the bell over for the life of the process.
Untried counts as working, or both bells would ring on every machine until the first PR landed.

Two tests, each checked against the version without the fix rather than assumed: the hand-back test fails on the old expression, and the recovery test fails on a latching implementation.

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

jtomaszewski · 2026-08-21T15:50:18Z

+          const machineHasIt = daemonAnnouncesHere(daemon);
+          setDaemonAnnounces(machineHasIt);
          const before = seen.current;
          // Recorded even when we stay quiet, so turning the bell on later
          // announces what arrives next rather than everything already here.


Real, and I have not fixed it — leaving this thread open for @jtomaszewski, because every fix I can see trades one silence for a different cost and that is a call for you rather than me.

The race is exactly as described. seen.current advances before the machineHasIt early return, so a browser that defers has already forgotten the arrival by the time the daemon discovers it cannot tap.

How narrow it actually is. It needs the browser's status read to land between the stub being written and announce() finishing, and that notify attempt to fail. On a healthy machine the attempt succeeds, so nothing is lost. The case that bites is a machine with no notifier at all, where notifierWorks is still null and status.notify is optimistically true — so it is the first PR after serve starts, and only if a permitted cockpit tab happens to poll inside a window that is milliseconds wide (the failure is ENOENT, not a timeout).

Why I did not take the obvious one-liner. Requiring a proven notifier — notifierWorks === true rather than !== false — closes it by making the browser keep its bell until the daemon has landed one tap. But that buys a rare loss with a certain duplicate: every machine, every serve process, the first PR gets two popups. The bug is rarer than the cure, and "one PR, one popup" is a stated goal of this PR. It also would not close the working-then-broken transition, which has the same shape.

Why not the fallback buffer. It is the right shape, but it needs a policy I should not pick unilaterally: deferred-but-unannounced keys have to be held somewhere, and then the question is what happens when the machine bell is turned off deliberately — flushing the buffer announces a backlog of still-open PRs, which is the thing seen exists to prevent. That is a product decision about which failure you would rather have.

My read: the residual hole is one PR, once, on a machine that has no notifier — and that same machine now hands the bell back permanently on the very next poll, which is the fix that mattered (4db709c). Worth a follow-up issue rather than more surgery on this branch, but happy to build the buffer here if you would rather it not ship with the hole.

jtomaszewski added feature New capability review Ready for code review labels Aug 21, 2026

jtomaszewski requested a lite review from Copilot August 21, 2026 15:06

Copilot started reviewing on behalf of jtomaszewski August 21, 2026 15:06 View session