Skip to content

fix(news): run fetcher in-process + harden dedup; alt-chord CLI keys#81

Open
distroinfinity wants to merge 4 commits into
mainfrom
newpipeline-audit
Open

fix(news): run fetcher in-process + harden dedup; alt-chord CLI keys#81
distroinfinity wants to merge 4 commits into
mainfrom
newpipeline-audit

Conversation

@distroinfinity

Copy link
Copy Markdown
Owner

Why

Two user-reported issues, audited end-to-end against code + the live prod DB.

  1. News was never live. The prod news_items table was empty and every source had last_fetched_at = null — the fetcher only existed in the standalone worker.ts, but prod only ever starts the HTTP API (node packages/api/dist/index.js). No worker service was deployed, so runFetchTick never ran and the CLI fell back to the offline demo slot.
  2. CLI keys collided with typing into Claude Code. The daemon mapped bare letters (d/s/k/...) to actions, so a d meant for Claude's prompt could fire discover.

What

News pipeline

  • Extract crons into scheduler.ts; run in-process from index.ts (RUN_INPROCESS_WORKER, default on). One Railway service now fetches news; per-source Redis locks prevent double-fetch.
  • Selection hard-excludes a short-lived news:offered:<device> set (marked at selection, not impression) and prefers fresh items over padding batches with already-seen ones — fixes the "same handful repeats" symptom.
  • news-health.service.ts: */15 Slack sweep + GET /admin/news-health surface stale/errored sources instead of failing silently.

CLI key capture

  • Only Alt/Option (Meta = ESC+letter) chords trigger actions; bare letters / Enter / Space / Ctrl+C pass through to Claude. Lone ESC still dismisses. Capture already stops on the Stop hook, so next-prompt typing isn't intercepted.
  • Footers, demo hint, and gitbook docs updated (incl. macOS "Option as Meta" caveat).

Test plan

  • typecheck + lint clean (api + cli)
  • unit tests pass (api 15, cli 120)
  • ran each fetcher against its live URL — all 10 sources returned items
  • after deploy: confirm news_items fills + /me/content/next serves live items
  • confirm ⌥D opens a slot URL and bare d reaches Claude

…ord cli keys

- api: extract crons into scheduler.ts, run in-process from index.ts (RUN_INPROCESS_WORKER, default on). the standalone worker was never deployed, so runFetchTick never ran and news_items stayed empty
- api: selection hard-excludes a short-lived "recently offered" set (marked at selection, not impression) and prefers fresh over padding with seen items — fixes the repeating-handful symptom
- api: news-health sweep + GET /admin/news-health surface stale/errored sources instead of failing silently
- cli: key capture only claims alt/option (meta) chords; bare letters pass through to claude code so typing isn't hijacked; footers, demo hint, docs updated
@vercel

vercel Bot commented May 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
devdrip Ready Ready Preview, Comment May 25, 2026 5:01am

@distroinfinity

Copy link
Copy Markdown
Owner Author

Review findings

I reviewed the feature end-to-end: the target is to make news fetch in prod without a separate worker, reduce repeated news batches, surface pipeline health, and stop CLI action keys from colliding with Claude typing. The scheduler extraction and in-process boot path are the right general shape for the single-Railway-service deployment, but I found a few issues that should be fixed before merge.

  1. P1: Ignored tty bytes are still consumed, so bare typing still loses characters.

    packages/cli/src/lib/daemon/input.ts:134 attaches a data reader to the same tty Claude Code is reading from, then packages/cli/src/lib/daemon/input.ts:141-147 returns when processByteChunk() says a bare letter / Enter / Space / Ctrl+C is not a Distro action. At that point the daemon has already won the read race and removed the bytes from the terminal input queue; returning null does not pass them through to Claude. This fixes the accidental discover/skip action, but it still drops user input whenever the daemon reader wins, including Ctrl+C. That directly conflicts with the PR goal/docs saying bare keys “pass straight through to Claude.” The key-capture design needs a non-competing input path, or it needs to stop reading the shared tty for ordinary typing windows; mapper-only filtering is not sufficient.

  2. P1: In-process scheduler can duplicate alert fanout across API instances.

    packages/api/src/index.ts:57 defaults every API process into startScheduler(), and packages/api/src/scheduler.ts:36-38 runs runAlertEvaluation() every minute without any distributed lock. News fetches are protected per source in the coordinator, but alert evaluation is not. The debounce path in packages/api/src/services/alert-evaluator.service.ts:153-179 does a read-then-lpush-then-insert, and packages/api/src/db/schema/alert_events.ts:21-24 only has non-unique indexes, so two API instances, or an API plus a mistakenly enabled worker, can both observe no recent event and enqueue/insert duplicate alerts for the same device/symbol. Either put the alert evaluator behind a Redis scheduler lock or make the debounce insert atomic with a uniqueness constraint/windowing strategy before moving it into every API process by default.

  3. P2: news:offered is documented as a per-item 4h holdback, but the implementation extends the whole set on every selection.

    packages/api/src/services/news-selection.service.ts:215-216 adds picked ids to one Redis set and then resets the key TTL to 4h. That means an active device that receives any new slots within the window keeps all previously offered ids suppressed indefinitely until the pool is exhausted and packages/api/src/services/news-selection.service.ts:203 deletes the whole set. This is materially different from “an item the device never rendered ages out of offered after ~4h,” and it can over-suppress older valid candidates, then abruptly allow very recent offers to recycle when the set is cleared. Use per-member timestamps, for example a sorted set pruned by score, or per-item keys, if the intended behavior is a true per-offer TTL.

Verification run locally:

  • git diff --check origin/main...HEAD
  • pnpm --filter @distrotv/api typecheck
  • pnpm --filter @distrotv/cli typecheck
  • pnpm --filter @distrotv/cli test -- src/lib/daemon/__tests__/input.test.ts (script ran the CLI suite: 120 tests)
  • pnpm --filter @distrotv/api test -- src/services/__tests__/news-selection.test.ts (15 API tests)

No code changes made.

…ging

addresses the three PR review findings:

- cli (#1): add `dtv run -- claude` — runs the child in a Distro-owned PTY so
  Distro is the sole reader of the terminal and forwards every key except its
  ⌥ chords to the child. true separation, no byte loss. the daemon skips its
  own (racy) tty capture for wrapped sessions: DISTRO_PTY=1 in the child env →
  hooks send idle-start wrapped=true → orchestrator gates keyCapture.start.
  slot rendering is unchanged (daemon writes to the pty slave, wrapper mirrors
  it to the real screen).
- api (#2): scheduler runs each tick behind a per-bucket redis leader lock, so
  the in-process scheduler in multiple api instances can't double-fire alerts
  (fanout had no per-row uniqueness).
- api (#3): news:offered is now a ZSET scored by offered-at and pruned per-item,
  so an active device no longer keeps older offers suppressed until the whole
  set is cleared — matches the documented ~4h per-item aging.

node-pty added as a native dep (shipped like better-sqlite3); `save` added to
the socket action vocabulary; docs updated.
# Conflicts:
#	packages/cli/src/commands/hook.ts
@distroinfinity

Copy link
Copy Markdown
Owner Author

Thanks for the review — all three addressed. Pushed in 8dd75de (+ merge ccddedb to pick up latest main).

#1 — ignored tty bytes are still consumed (true input separation). You're right that mapper-only filtering can't hand a byte back to Claude. Rather than a half-measure, this now ships real separation via a PTY wrapper: dtv run -- claude (packages/cli/src/commands/run.ts, node-pty). Distro becomes the sole reader of the real terminal and forwards every keystroke to the child PTY except its chords, which it dispatches to the daemon over the socket. Deterministic, zero byte loss. The daemon must not re-race Claude inside the PTY, so wrapped sessions are flagged (DISTRO_PTY=1 → hooks send idle-start wrapped:true → orchestrator gates keyCapture.start on !session.wrapped). Slot rendering is unchanged — the daemon writes to the PTY slave tty and the wrapper mirrors it to the screen. The legacy shared-tty capture remains as a best-effort fallback (now documented honestly as racy; race-free distro subcommands cover that path).

#2 — in-process scheduler can duplicate alert fanout. Each cron tick now runs behind a per-bucket Redis leader lock (runLocked in packages/api/src/scheduler.ts), so only one instance evaluates alerts per tick even with the scheduler in every API process. News fetch + health sweep are locked too; the boot fetch stays unlocked (per-source locks already dedupe writes).

#3news:offered whole-key TTL over-suppresses. Switched to a ZSET scored by offered-at: each selection prunes members older than now-4h (ZREMRANGEBYSCORE) before reading survivors, and ZADDs the new picks with their own timestamps. So each item ages out on its own clock — an active device no longer keeps older offers suppressed until the set is wiped. Matches the documented ~4h per-item window. (Added zset support to the in-memory TestRedis; Upstash is native.)

Also: node-pty ships like better-sqlite3 (external in tsup, in the release runtime package.json, prebuilds via npm install — one packaging note in the docs re: the macOS spawn-helper exec bit). save added to the socket action vocabulary. Typecheck + lint clean; CLI 120 / API 15 tests green. The PTY path needs a real interactive terminal to verify end-to-end — I confirmed node-pty spawns + the command wiring, but couldn't drive raw input in CI.

@distroinfinity

Copy link
Copy Markdown
Owner Author

Re-review findings

I re-reviewed PR #81 at ccddedb after the new changes. The previous API findings are addressed in the new approach: news:offered is now per-item ZSET aging, and recurring scheduler jobs now run behind Redis bucket locks. I found a few remaining issues in the new PTY/action path.

  1. P1: dtv run actions are sent without a tty, so they can hit the wrong Claude session.

    The normal fallback commands include tty: resolveTty() when sending an action (packages/cli/src/commands/action.ts:9-11), but the new PTY wrapper sends { type: "action", action } with no tty (packages/cli/src/commands/run.ts:111-115). That means the daemon falls back to resolveSession()'s no-tty heuristic (packages/cli/src/lib/daemon/orchestrator.ts:214-236), which picks the single active session or the most-recent active session. With two active sessions, especially two dtv run -- claude windows, ⌥D/⌥K/⌥S in one terminal can open/kill/skip the other terminal's current slot. This regresses the existing per-tty routing guarantee and makes the new “deterministic” input path ambiguous under the same multi-terminal case the daemon already supports. The wrapper needs to send the child PTY tty/session identity on action events, or otherwise establish a wrapper-owned session id that actions can target.

  2. P2: The PTY wrapper only recognizes Alt chords when ESC and the letter arrive in the same read chunk.

    interceptChord() requires data.length === 2 && data[0] === ESC (packages/cli/src/commands/run.ts:31-42), and onInput() forwards every non-matching chunk directly to the child PTY (packages/cli/src/commands/run.ts:110-118). Raw terminal reads do not guarantee that an ESC prefix and the following byte arrive in one data event. If they split, ⌥D becomes a forwarded ESC and then a forwarded d, so the action does not fire and Claude receives unexpected input. The wrapper should keep a tiny ESC-prefix state machine/timer so split ESC+letter sequences are handled consistently while still forwarding lone ESC/control sequences correctly.

  3. P2: Docs advertise race-free save/dismiss subcommands, but the CLI does not expose them.

    The updated docs tell users to use distro discover|skip|save|mute|kill-session|chart|dismiss as race-free shared-tty fallbacks (gitbook-docs/cli/daemon-and-hooks.md:106 and :120). The wire protocol now accepts both save and dismiss, but packages/cli/src/commands/action.ts:20-48 only defines discover, skip, kill-session, mute, and chart, and packages/cli/src/index.ts:48-52 only registers those. Users following the docs get the top-level help/unknown-command path for distro save and distro dismiss, so two of the advertised non-racy fallbacks are missing.

Verification run locally on the updated head:

  • git diff --check origin/main...origin/newpipeline-audit
  • pnpm --filter @distrotv/api typecheck
  • pnpm --filter @distrotv/cli typecheck
  • pnpm --filter @distrotv/api lint
  • pnpm --filter @distrotv/cli lint
  • pnpm --filter @distrotv/api test (15 passed)
  • pnpm --filter @distrotv/cli test (120 passed)
  • pnpm --filter @distrotv/api build
  • pnpm --filter @distrotv/cli build
  • PTY smoke: node packages/cli/dist/index.js run -- sh -lc 'echo PTY:$DISTRO_PTY; tty' produced PTY:1 and a PTY path.

No code changes made.

…d save/dismiss

re-review follow-ups on the pty/action path:

- run (#1): send the child PTY's tty on action events (resolveTtyForPid) so ⌥
  chords target that session — two `dtv run` windows no longer cross-fire
  open/skip/kill on each other's slots.
- run (#2): buffer a lone trailing ESC ~25ms, so an ⌥ chord whose ESC and letter
  land in separate read chunks still fires; a real Escape (no follow-up) still
  forwards to the child, and CSI/SS3 sequences forward verbatim.
- action (#3): register `distro save` and `distro dismiss` — the docs advertise
  them as race-free fallbacks and the wire protocol accepts them, but the
  subcommands were missing.
@distroinfinity

Copy link
Copy Markdown
Owner Author

Re-review addressed in 39909e1.

#1 — actions sent without a tty (multi-window cross-fire). The wrapper now resolves the child PTY's tty (resolveTtyForPid(ptyProc.pid) in packages/cli/src/lib/daemon/tty.ts) and sends it on every action event. The child is the PTY session leader, so that path is exactly what the child's own hooks report to the daemon — so ⌥D/⌥K/⌥S now target the originating session, and two dtv run -- claude windows no longer cross-fire. Falls back to no-tty only if resolution fails.

#2 — split ESC+letter chunks. Added a small ESC-prefix buffer in run.ts: a lone trailing ESC is held ~25ms; if the chord letter arrives in the next chunk it's stitched back and fires the action, otherwise it's forwarded as a real Escape keypress. ESC [ (CSI) / ESC O (SS3) still forward verbatim. 25ms matches typical terminal ESC-disambiguation timeouts, so a real Escape is delivered imperceptibly late.

#3 — advertised save/dismiss subcommands didn't exist. Registered distro save and distro dismiss (packages/cli/src/commands/action.ts + index.ts). All seven documented race-free fallbacks now resolve.

Typecheck + lint clean; CLI 120 / API 15 tests green; node packages/cli/dist/index.js --help lists run, save, dismiss. (The PTY input routing — per-tty targeting and ESC buffering — still wants a real two-window interactive check, which CI can't drive.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant