From dd7c10d0580556be80d669568ca1564952ef8331 Mon Sep 17 00:00:00 2001 From: whackur Date: Thu, 30 Jul 2026 22:51:48 +0900 Subject: [PATCH 1/9] fix: let a path with no repository fall back to the idle interval A path that is not a repository kept `changed` set, so the worker took the fast interval and reported the same error once a second forever. The tree watch is installed for any readable directory, so a repository appearing later is reported rather than polled for; nothing needs the fast rate. --- src/runtime/snapshot/worker.rs | 33 +++++++++++--- src/runtime/snapshot/worker_tests.rs | 64 ++++++++++++++++++++++++++++ src/runtime/snapshot_tests.rs | 9 ++-- 3 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 src/runtime/snapshot/worker_tests.rs diff --git a/src/runtime/snapshot/worker.rs b/src/runtime/snapshot/worker.rs index 11d34f0..918f406 100644 --- a/src/runtime/snapshot/worker.rs +++ b/src/runtime/snapshot/worker.rs @@ -91,7 +91,7 @@ impl Worker { } if awake && state.changed && due(state.last_read) { - if !self.read_once(&mut state) { + if !self.read_once(&mut state, &watch) { // The receiver is gone: nobody is left to read this. return; } @@ -153,6 +153,8 @@ impl Worker { /// reports and what [`wait`](Self::wait) reads to choose an interval. fn follow_git_dir(&self, state: &mut ReadState, roots: &mut Roots, watch: &mut Watches) { let Some(repo) = state.repo.as_ref() else { + // No handle to ask, and nothing to correct: the read that dropped it + // has already said what is being watched. return; }; // None means it is inside the work tree, where the recursive watch @@ -177,7 +179,10 @@ impl Worker { } /// Read the tree and send what it says. `false` once the receiver is gone. - fn read_once(&self, state: &mut ReadState) -> bool { + /// + /// `watch` is read for one answer only: what is left watching a directory + /// that turns out to hold no repository — see the failure below. + fn read_once(&self, state: &mut ReadState, watch: &Watches) -> bool { if state.reads_since_open >= REOPEN_REPO_EVERY_READS { state.repo = None; } @@ -188,9 +193,23 @@ impl Worker { state.reads_since_open = 0; } Err(err) => { - // `changed` is left standing, so the next permitted slot - // retries — a directory that is not a repository yet may - // become one. + // Counted as a read taken, `changed` and all. A directory that + // is not a repository yet may become one, but leaving + // `changed` standing to retry for that held the reader at + // `MIN_READ_INTERVAL` and sent this same error every second for + // as long as the session lived — the cost watching exists to + // remove, and enough on its own to make a test counting reads + // wait for a silence that never came. + // + // The watch is what notices instead: `git init` and a checkout + // appearing both write inside the tree being watched. And that + // watch is the whole of what can be watched here — the second + // one follows the git directory of a repository, and there is + // no repository — which is what earns the idle interval rather + // than the fixed fallback for a tree nothing reports on. See + // [`wait`](Self::wait). + state.changed = false; + self.watching.store(watch.tree.is_some(), Ordering::Release); state.last_read = Some(Instant::now()); let msg = SnapshotMsg::Err(format!("not a git repository: {err}")); return self.deliver(msg); @@ -259,3 +278,7 @@ impl Worker { fn due(last_read: Option) -> bool { last_read.is_none_or(|last| last.elapsed() >= MIN_READ_INTERVAL) } + +#[cfg(test)] +#[path = "worker_tests.rs"] +mod tests; diff --git a/src/runtime/snapshot/worker_tests.rs b/src/runtime/snapshot/worker_tests.rs new file mode 100644 index 0000000..0d48f1c --- /dev/null +++ b/src/runtime/snapshot/worker_tests.rs @@ -0,0 +1,64 @@ +//! What the reader does with a directory that holds no repository. +//! +//! Beside the worker rather than with the other reader tests, which are already +//! at the length this project splits at, and borrowing their helpers: the claim +//! is the same kind — how many reads arrive, not what they say. + +use crate::runtime::snapshot::tests::{SETTLE, next_read, reads_during}; +use crate::runtime::snapshot::{MIN_READ_INTERVAL, SnapshotChannel, SnapshotMsg}; +use crate::test_util::run_git; + +/// An empty directory, which no `git status` can be taken of. +fn not_a_repository() -> (tempfile::TempDir, String) { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path().to_string_lossy().to_string(); + (dir, path) +} + +#[test] +fn a_path_with_no_repository_is_reported_once_rather_than_every_second() { + // A pane opened on a directory that is not a checkout — a path typed wrong, a + // clone that has not landed — used to be told so once a second for as long as + // it stayed open, which is exactly the cost watching rather than polling + // exists to remove. + let (dir, path) = not_a_repository(); + let channel = SnapshotChannel::spawn(&path); + + assert!( + matches!(next_read(&channel), SnapshotMsg::Err(_)), + "a directory with no repository in it is reported as an error" + ); + + // Asserted rather than assumed, as the tests next door do: with no watch the + // reader falls back to the fixed interval by design, and the count below would + // then fail for a reason that has nothing to do with what it is checking. + assert!( + channel.is_watching(), + "this directory could not be watched, so this machine cannot run this test" + ); + assert_eq!( + reads_during(&channel, SETTLE), + 0, + "and reported once, not once per {MIN_READ_INTERVAL:?}" + ); + drop(dir); +} + +#[test] +fn a_repository_created_where_there_was_none_is_read() { + // The other half of the claim above: the reader goes quiet because the + // directory is watched, not because it gave up on it. `git init` writes + // inside that watch, so the repository is read when it appears rather than + // whenever the safety net next comes round. + let (dir, path) = not_a_repository(); + let channel = SnapshotChannel::spawn(&path); + assert!(matches!(next_read(&channel), SnapshotMsg::Err(_))); + + run_git(&path, &["init"]); + + assert!( + matches!(next_read(&channel), SnapshotMsg::Ok(..)), + "the repository that appeared must be read" + ); + drop(dir); +} diff --git a/src/runtime/snapshot_tests.rs b/src/runtime/snapshot_tests.rs index e4b104d..738d252 100644 --- a/src/runtime/snapshot_tests.rs +++ b/src/runtime/snapshot_tests.rs @@ -4,6 +4,9 @@ //! while nothing happens, so most of these are about *absence*: how many reads //! arrive, not what they say. They use short windows against a ten-second idle //! interval, so a slow machine delays them rather than failing them. +//! +//! The helpers that count reads are `pub(super)`: the same claim is made about a +//! path with no repository in it, and those tests sit beside the worker. use super::{IDLE_READ_INTERVAL, MIN_READ_INTERVAL, SnapshotChannel, SnapshotMsg}; use crate::test_util::{make_linked_worktree, make_repo, run_git}; @@ -12,11 +15,11 @@ use std::time::{Duration, Instant}; /// Long enough for a filesystem event to travel and a read to happen, without /// reaching the idle interval that would make the assertion meaningless. -const SETTLE: Duration = Duration::from_millis(2_500); +pub(super) const SETTLE: Duration = Duration::from_millis(2_500); /// Wait for one snapshot, or fail. Errors count: what is being timed is the read, /// not what it found. -fn next_read(channel: &SnapshotChannel) -> SnapshotMsg { +pub(super) fn next_read(channel: &SnapshotChannel) -> SnapshotMsg { let deadline = Instant::now() + SETTLE; while Instant::now() < deadline { match channel.try_recv() { @@ -59,7 +62,7 @@ fn quiesce(channel: &SnapshotChannel) { } /// How many snapshots arrive over `window`. -fn reads_during(channel: &SnapshotChannel, window: Duration) -> usize { +pub(super) fn reads_during(channel: &SnapshotChannel, window: Duration) -> usize { let deadline = Instant::now() + window; let mut reads = 0; while Instant::now() < deadline { From 39b0881f6a48841f6986ff66f2619e56d99a730c Mon Sep 17 00:00:00 2001 From: whackur Date: Thu, 30 Jul 2026 22:51:48 +0900 Subject: [PATCH 2/9] test: give each session fixture its own preferences file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fixture pointed every test at /nonexistent/nightcrow/viewer.json on the assumption it is unwritable. Running as root — the default in a container — create_dir_all makes it, so tests shared one accent on disk and the wrap test read a value that made its own write a no-op. The wrap test now reads the accent back instead of waiting for an announcement it may not be owed. --- src/application/session_terminals_tests.rs | 2 +- src/daemon/client_tests.rs | 7 +++--- src/daemon/serve_tests/accent.rs | 10 ++++++++ src/daemon/serve_tests/harness.rs | 5 +++- src/test_util.rs | 28 +++++++++++++++++----- src/web/viewer/server/tests/mod.rs | 7 ++++-- 6 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/application/session_terminals_tests.rs b/src/application/session_terminals_tests.rs index 26b9d9a..30333ea 100644 --- a/src/application/session_terminals_tests.rs +++ b/src/application/session_terminals_tests.rs @@ -27,7 +27,7 @@ const DEADLINE: Duration = Duration::from_secs(15); fn attached(dir: &tempfile::TempDir, repos: &[String]) -> (DaemonSocket, DaemonClient) { let socket = DaemonSocket::bind(&dir.path().join("d.sock")).expect("binds"); let listener = socket.listener().try_clone().expect("clones"); - let state = crate::test_util::session_state(repos); + let state = crate::test_util::session_state(repos, dir.path()); let session = crate::daemon::serve::start(state).expect("starts the watcher"); std::thread::spawn(move || crate::daemon::serve::serve(listener, session)); let client = DaemonClient::connect(socket.path()).expect("attaches"); diff --git a/src/daemon/client_tests.rs b/src/daemon/client_tests.rs index 870a1bc..55b9558 100644 --- a/src/daemon/client_tests.rs +++ b/src/daemon/client_tests.rs @@ -33,9 +33,10 @@ fn daemon(dir: &tempfile::TempDir, repos: &[String]) -> TestDaemon { persist: false, startup_commands: Vec::new(), hot: crate::config::AgentIndicatorConfig::default(), - prefs: PrefsStore::at(std::path::PathBuf::from( - "/nonexistent/nightcrow/viewer.json", - )), + // In the test's own directory, beside the socket: opening a repository + // records it as the active one, so this file is written, and a path + // shared with another test would make the two one session. + prefs: PrefsStore::at(dir.path().join("viewer.json")), })); let session = crate::daemon::serve::start(state).expect("starts the watcher"); std::thread::spawn(move || crate::daemon::serve::serve(listener, session)); diff --git a/src/daemon/serve_tests/accent.rs b/src/daemon/serve_tests/accent.rs index 7b70a2c..c819da0 100644 --- a/src/daemon/serve_tests/accent.rs +++ b/src/daemon/serve_tests/accent.rs @@ -53,6 +53,16 @@ fn an_accent_past_the_end_of_the_cycle_wraps_rather_than_being_refused() { accent: crate::config::Accent::ALL.len() + 2, }); + // Read back rather than waited for, which is the claim above: the watcher + // announces a *difference*, so a session already painted in the colour the + // index wraps to answers the change with silence — and a test waiting for a + // frame nobody owes it would sit out the read timeout and blame the wrap. A + // list is owed an answer whatever the accent is. This connection's requests + // are served in order, so the accent is already stored when the list is + // recorded as owed, and either frame that can come back — the announcement + // or the answer — carries what was stored. + client.send(ClientMessage::ListRepos); + assert_eq!(client.next_accent(), 2); drop(repo); } diff --git a/src/daemon/serve_tests/harness.rs b/src/daemon/serve_tests/harness.rs index a31d3b5..4eaefd1 100644 --- a/src/daemon/serve_tests/harness.rs +++ b/src/daemon/serve_tests/harness.rs @@ -33,7 +33,10 @@ pub(super) fn daemon(dir: &tempfile::TempDir, repos: &[String]) -> TestDaemon { let path = dir.path().join("d.sock"); let socket = DaemonSocket::bind(&path).expect("binds"); let listener = socket.listener().try_clone().expect("clones"); - let state = crate::test_util::session_state(repos); + // The same directory the socket is in, so this session's preferences — the + // accent among them — belong to this test rather than to whichever one wrote + // them last. + let state = crate::test_util::session_state(repos, dir.path()); let served = std::sync::Arc::clone(&state); let session = crate::daemon::serve::start(served).expect("starts the watcher"); std::thread::spawn(move || crate::daemon::serve::serve(listener, session)); diff --git a/src/test_util.rs b/src/test_util.rs index 04a54a8..a815e64 100644 --- a/src/test_util.rs +++ b/src/test_util.rs @@ -56,13 +56,31 @@ pub fn make_linked_worktree() -> (TempDir, TempDir, String) { (main, elsewhere, tree) } -/// A session serving `repos`, with no browser listener bound. +/// A session serving `repos`, with no browser listener bound, keeping its +/// preferences in `prefs_dir`. /// /// The state, not the server: everything the daemon serves hangs off this, so a /// test can drive a real session — catalog, terminal hubs and all — without a -/// TCP port. Never persists, so tests cannot touch the real +/// TCP port. The workspace file is never written, so tests cannot touch the real /// `~/.nightcrow/workspace.json`. -pub fn session_state(repos: &[String]) -> std::sync::Arc { +/// +/// `prefs_dir` is required, and must be the caller's own temporary directory. +/// The accent and the active project *are* preferences, so a session driven from +/// a test writes this file — and every session pointed at one path is one +/// session: a test would begin in the colour whichever test ran last chose, and +/// asking for that same colour is no change at all, which the watcher answers +/// with silence rather than a frame. Pass the directory that already holds the +/// test's socket and the file goes away with the test. +/// +/// This replaced a single fixed path outside any temporary directory, chosen +/// because it was expected to be unwritable. It is unwritable for an ordinary +/// user, which is what CI runs as; a test suite running as root — the default in +/// a bare container — created it and shared it, and the accent tests then +/// depended on each other's order across whole runs. +pub fn session_state( + repos: &[String], + prefs_dir: &Path, +) -> std::sync::Arc { std::sync::Arc::new(crate::web::viewer::server::ViewerState::new( crate::web::viewer::server::ViewerOptions { bind: "127.0.0.1".parse().unwrap(), @@ -72,9 +90,7 @@ pub fn session_state(repos: &[String]) -> std::sync::Arc ViewerServer { } /// `prefs_dir` keeps preference writes inside a temp directory. Left `None` -/// the store still points at the real `~/.nightcrow/viewer.json`, so any -/// test that *writes* a preference must pass one. +/// the store points at a fixed path every caller shares, so any test that +/// *writes* a preference must pass one — the path is not a safe elsewhere. It is +/// only unwritable for an ordinary user; a suite running as root, which is the +/// default in a bare container, creates it and then every session left without a +/// directory reads back what the last one stored. pub(super) fn server_with( paths: &[String], hot: crate::config::AgentIndicatorConfig, From 1cdd8ac875a6df26aec32cb83837438f569b19d8 Mon Sep 17 00:00:00 2001 From: whackur Date: Thu, 30 Jul 2026 22:51:48 +0900 Subject: [PATCH 3/9] chore: move the actions off the deprecated Node 20 runtime checkout and setup-node v4 bundle Node 20, which the runners now force onto Node 24. v7 targets it directly. --- .github/workflows/ci.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b9ab2f5..f99362f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,8 +13,8 @@ jobs: name: Package versions runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 with: node-version: 22 - name: Verify application package versions agree @@ -52,7 +52,7 @@ jobs: matrix: os: [ubuntu-latest, macos-latest] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - run: cargo build --locked @@ -63,7 +63,7 @@ jobs: name: Rustfmt runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@stable with: components: rustfmt @@ -73,8 +73,8 @@ jobs: name: Viewer frontend runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 with: node-version: 22 cache: npm From 8f40bf5a211690a35e655e3ce915e0d854b98fa7 Mon Sep 17 00:00:00 2001 From: whackur Date: Thu, 30 Jul 2026 23:00:31 +0900 Subject: [PATCH 4/9] docs: drop comments that restate what the code already shows --- src/daemon/client.rs | 1 - src/runtime/terminal/scroll.rs | 5 ++--- src/runtime/tree_watch.rs | 3 --- src/runtime/tree_watch_tests.rs | 2 -- src/ui/diff_pane/pane_impl.rs | 2 -- src/ui/diff_viewer/mod.rs | 1 - src/ui/file_view.rs | 2 -- src/ui/splash.rs | 3 --- 8 files changed, 2 insertions(+), 17 deletions(-) diff --git a/src/daemon/client.rs b/src/daemon/client.rs index 72677f1..9b4b543 100644 --- a/src/daemon/client.rs +++ b/src/daemon/client.rs @@ -24,7 +24,6 @@ use std::time::Duration; /// invented out of an idle session. const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(5); -/// A connection to the session daemon. #[derive(Debug)] pub struct DaemonClient { /// The write half. The reader thread owns the other. diff --git a/src/runtime/terminal/scroll.rs b/src/runtime/terminal/scroll.rs index 7ab521d..300dc22 100644 --- a/src/runtime/terminal/scroll.rs +++ b/src/runtime/terminal/scroll.rs @@ -154,9 +154,8 @@ impl TerminalState { pub(super) fn sync_scroll_pane(&mut self, id: PaneId) { let offset = self.scroll.get(&id).copied().unwrap_or(0); let actual = match self.emulators.get_mut(&id) { - // The emulator clamps the offset to the actual scrollback - // buffer size internally, so we can pass the full request - // through and read back what was applied. + // The emulator clamps internally, so pass the full request through + // and read back what was applied. Some(emulator) => emulator.set_scroll_offset(offset), None => return, }; diff --git a/src/runtime/tree_watch.rs b/src/runtime/tree_watch.rs index 5e77813..47d517c 100644 --- a/src/runtime/tree_watch.rs +++ b/src/runtime/tree_watch.rs @@ -131,7 +131,6 @@ impl TreeWatcher { /// was deleted between the listing and this call) is skipped, not retried, /// and never enters `watched`. pub fn sync(&mut self, workdir: &Path, desired: &BTreeSet) { - // Remembered so `drain_changed` can make event paths repo-relative. self.root = Some(workdir.to_path_buf()); let Some(debouncer) = self.debouncer.as_mut() else { // Inert watcher: track intent only so behaviour is observable in @@ -140,7 +139,6 @@ impl TreeWatcher { return; }; let watcher = debouncer.watcher(); - // Drop paths no longer desired. let stale: Vec = self.watched.difference(desired).cloned().collect(); for rel in stale { let abs = join_rel(workdir, &rel); @@ -149,7 +147,6 @@ impl TreeWatcher { let _ = watcher.unwatch(&abs); self.watched.remove(&rel); } - // Add newly desired paths. let fresh: Vec = desired.difference(&self.watched).cloned().collect(); for rel in fresh { let abs = join_rel(workdir, &rel); diff --git a/src/runtime/tree_watch_tests.rs b/src/runtime/tree_watch_tests.rs index 4bb92b3..b7daf00 100644 --- a/src/runtime/tree_watch_tests.rs +++ b/src/runtime/tree_watch_tests.rs @@ -11,7 +11,6 @@ fn inert_watcher_tracks_desired_set_without_os_calls() { w.sync(root, &desired); assert_eq!(w.watched, desired); - // Reconcile down to just the root. let mut smaller = BTreeSet::new(); smaller.insert(String::new()); w.sync(root, &smaller); @@ -52,7 +51,6 @@ fn drain_changed_reports_the_directories_that_changed_and_clears() { BTreeSet::from(["src".to_string(), String::new()]), "the repo root is the empty relative path" ); - // Drained: a second poll with nothing new reports no change. assert!(w.drain_changed().is_empty()); } diff --git a/src/ui/diff_pane/pane_impl.rs b/src/ui/diff_pane/pane_impl.rs index 4c0fdd8..f9290cf 100644 --- a/src/ui/diff_pane/pane_impl.rs +++ b/src/ui/diff_pane/pane_impl.rs @@ -16,13 +16,11 @@ impl DiffPane { self.line_count().saturating_sub(1) } - /// Move the active horizontal scroll target left by one tab stop. pub fn scroll_left(&mut self) { let target = self.scroll_x_target_mut(); *target = target.saturating_sub(4); } - /// Move the active horizontal scroll target right by one tab stop. /// Capped at `u16::MAX` because ratatui's `Paragraph::scroll` takes `u16`. pub fn scroll_right(&mut self) { let target = self.scroll_x_target_mut(); diff --git a/src/ui/diff_viewer/mod.rs b/src/ui/diff_viewer/mod.rs index 72f9ffe..9f3af1a 100644 --- a/src/ui/diff_viewer/mod.rs +++ b/src/ui/diff_viewer/mod.rs @@ -115,7 +115,6 @@ pub fn render( break; } - // Hunk header if flat_idx >= scroll_start && flat_idx < visible_end { lines.push(Line::from(Span::styled( hunk.header.as_str(), diff --git a/src/ui/file_view.rs b/src/ui/file_view.rs index 5409b55..c74a217 100644 --- a/src/ui/file_view.rs +++ b/src/ui/file_view.rs @@ -68,8 +68,6 @@ impl FileViewState { self.error = None; } - /// Largest legal `scroll` value: one less than `line_count`, or 0 when - /// the file is empty. pub fn max_scroll(&self) -> usize { self.line_count().saturating_sub(1) } diff --git a/src/ui/splash.rs b/src/ui/splash.rs index 2f8cb9f..6ef9249 100644 --- a/src/ui/splash.rs +++ b/src/ui/splash.rs @@ -54,7 +54,6 @@ pub fn draw(frame: &mut Frame, state: &SplashState, accent: Color) { ); let logo_h = LOGO.len() as u16; - // logo + gap + tagline + gap + bar let content_h = logo_h + 1 + 1 + 1 + 1; let outer = Layout::default() @@ -93,7 +92,6 @@ pub fn draw(frame: &mut Frame, state: &SplashState, accent: Color) { inner[0], ); - // Tagline let version = env!("CARGO_PKG_VERSION"); frame.render_widget( Paragraph::new(Line::from(vec![ @@ -109,7 +107,6 @@ pub fn draw(frame: &mut Frame, state: &SplashState, accent: Color) { inner[2], ); - // Loading bar let filled = ((progress * BAR_WIDTH as f64) as usize).min(BAR_WIDTH); let empty = BAR_WIDTH - filled; let bar = format!("{}{}", "█".repeat(filled), "░".repeat(empty)); From 0dedef760de766c990d1eb635807417adcdc101f Mon Sep 17 00:00:00 2001 From: whackur Date: Fri, 31 Jul 2026 00:12:20 +0900 Subject: [PATCH 5/9] feat: let a plugin watch a pane a provider signal names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pane opened by hand runs a bare shell, so a provider started inside it was invisible to any plugin and got no recovery at all. The pane token is injected into every pane's environment and inherited by the provider's own helpers, so a signal carrying it proves where it came from: that is now grounds to watch the pane, judged by the guard like any other request. Off by default via [[plugin]] watch_on_signal — a plain shell never produces a provider signal, so it is never watched, but the switch stays the operator's. Such a pane can never be relaunched: it has no launch command, so a relaunch would restart the shell, not the provider. --- config.example.toml | 23 ++- plugins/nightcrow-recovery/src/main.rs | 2 + plugins/nightcrow-recovery/src/protocol.rs | 17 +- .../nightcrow-recovery/src/protocol_tests.rs | 58 +++++- .../nightcrow-recovery/src/provider/mod.rs | 19 ++ .../src/provider/mod_tests.rs | 20 ++ plugins/nightcrow-recovery/src/runloop.rs | 160 ++++++++------- .../nightcrow-recovery/src/runloop_adopt.rs | 105 ++++++++++ .../src/runloop_adopt_tests.rs | 128 ++++++++++++ plugins/nightcrow-recovery/src/runloop_io.rs | 69 +++++++ .../nightcrow-recovery/src/state_resume.rs | 22 +- .../src/state_tests/resume.rs | 53 +++++ src/config/plugin.rs | 19 +- src/config/tests/plugin.rs | 28 +++ src/plugin/guard.rs | 68 +++--- src/plugin/guard_budget.rs | 35 +++- src/plugin/guard_refusal.rs | 24 +++ src/plugin/guard_tests/mod.rs | 23 +++ src/plugin/guard_tests/relaunch.rs | 54 ++++- src/plugin/guard_tests/watch.rs | 130 ++++++++++++ src/plugin/guard_watch.rs | 62 ++++++ src/plugin/mod.rs | 7 + src/plugin/protocol.rs | 28 ++- src/plugin/protocol_tests/bounds.rs | 106 ++++++++++ .../mod.rs} | 126 +++--------- src/plugin/registry.rs | 5 +- src/web/viewer/terminal/hub_events.rs | 5 + src/web/viewer/terminal/hub_plugins.rs | 37 +++- src/web/viewer/terminal/hub_recovery.rs | 25 ++- src/web/viewer/terminal/hub_relaunch.rs | 52 ++++- src/web/viewer/terminal/tests/mod.rs | 1 + src/web/viewer/terminal/tests/plugin_watch.rs | 194 ++++++++++++++++++ 32 files changed, 1454 insertions(+), 251 deletions(-) create mode 100644 plugins/nightcrow-recovery/src/runloop_adopt.rs create mode 100644 plugins/nightcrow-recovery/src/runloop_adopt_tests.rs create mode 100644 plugins/nightcrow-recovery/src/runloop_io.rs create mode 100644 src/plugin/guard_tests/watch.rs create mode 100644 src/plugin/guard_watch.rs create mode 100644 src/plugin/protocol_tests/bounds.rs rename src/plugin/{protocol_tests.rs => protocol_tests/mod.rs} (63%) create mode 100644 src/web/viewer/terminal/tests/plugin_watch.rs diff --git a/config.example.toml b/config.example.toml index be24840..0816a44 100644 --- a/config.example.toml +++ b/config.example.toml @@ -108,7 +108,8 @@ live_watch = true # watch expanded dirs and refresh the tree live; set f # section fits any provider or tool. # # Nothing here runs unless you both add an entry AND set enabled = true, and a -# plugin can only act on panes whose [[startup_command]] named it. +# plugin can only act on panes whose [[startup_command]] named it — unless you +# also set watch_on_signal, described further down. # # The bundled "recovery" plugin watches an opted-in pane for its provider's # usage-limit message, waits for the stated reset time to pass, and then resumes @@ -126,11 +127,31 @@ live_watch = true # watch expanded dirs and refresh the tree live; set f # Leave it empty (the default) and the plugin can still wait and type into a # live pane, but never relaunch one. # +# watch_on_signal covers the pane you did not configure: you opened a shell with +# t and started a coding CLI in it by hand. nightcrow gives every pane a +# random token and puts it in that pane's environment only, so the CLI's own hook +# — a child of the CLI, a grandchild of the pane — can quote it back to the +# plugin. The plugin then asks nightcrow for "the pane this token names", and +# nightcrow checks the token really is one of its panes before handing it over. +# +# What you are turning on, precisely: a pane becomes plugin-visible once +# something running inside it has spoken to that plugin. A plain shell never +# does, so your shells stay untouched — but anything you start in a pane that +# talks to this plugin is consenting on that pane's behalf. The plugin is never +# given a list of your panes and cannot ask for one it has not been told about, +# and a pane another plugin already watches is not taken away. Leave it off if +# you want the [[startup_command]] list to be the whole of what a plugin sees. +# +# A pane taken on this way can be waited for and typed into, but never +# relaunched: nightcrow never launched a command in it, so there is nothing to +# put back. allowed_resume_flags has no effect on such a pane. +# # [[plugin]] # name = "recovery" # required; the name panes opt in with # command = "nightcrow-recovery" # required; found on PATH or in the plugin dir # args = [] # passed to the plugin verbatim # enabled = false # off by default; set true to actually run it +# watch_on_signal = false # off by default; see the paragraphs above # allowed_resume_flags = ["--resume", "resume", "--session"] # # empty by default, which refuses relaunches. # # These three are what the bundled recovery diff --git a/plugins/nightcrow-recovery/src/main.rs b/plugins/nightcrow-recovery/src/main.rs index 1c86d52..8dafd8d 100644 --- a/plugins/nightcrow-recovery/src/main.rs +++ b/plugins/nightcrow-recovery/src/main.rs @@ -17,6 +17,8 @@ mod ipc; mod protocol; mod provider; mod runloop; +mod runloop_adopt; +mod runloop_io; mod state; mod wait; diff --git a/plugins/nightcrow-recovery/src/protocol.rs b/plugins/nightcrow-recovery/src/protocol.rs index ffa27e0..7f70e8e 100644 --- a/plugins/nightcrow-recovery/src/protocol.rs +++ b/plugins/nightcrow-recovery/src/protocol.rs @@ -9,7 +9,11 @@ use serde::{Deserialize, Serialize}; /// Contract version this plugin speaks. The host refuses anything else. -pub const PROTOCOL_VERSION: u32 = 1; +/// +/// 2 is the first version with [`PluginCommand::WatchPane`], which this plugin +/// needs: a pane somebody started a provider CLI in by hand is never named to +/// us, so asking for it is the only way to watch it at all. +pub const PROTOCOL_VERSION: u32 = 2; /// Longest line the host will read from us; also the cap we apply to what we /// read, so a corrupt stream cannot make this process allocate without bound. @@ -128,6 +132,10 @@ pub enum PluginCommand { deadline_epoch: Option, attempt: u32, }, + /// Ask for the pane this token names. Carries no generation: we are asking + /// about a pane the host has never described to us, so we cannot know which + /// spawn it is on — the `PaneOpened` the host answers with is what says. + WatchPane { v: u32, token: PaneToken }, Log { v: u32, level: LogLevel, @@ -187,6 +195,13 @@ pub fn encode_command(cmd: &PluginCommand) -> anyhow::Result { Ok(line) } +pub fn watch_pane(token: PaneToken) -> PluginCommand { + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token, + } +} + pub fn log(level: LogLevel, message: impl Into) -> PluginCommand { PluginCommand::Log { v: PROTOCOL_VERSION, diff --git a/plugins/nightcrow-recovery/src/protocol_tests.rs b/plugins/nightcrow-recovery/src/protocol_tests.rs index ff912ed..f41c8be 100644 --- a/plugins/nightcrow-recovery/src/protocol_tests.rs +++ b/plugins/nightcrow-recovery/src/protocol_tests.rs @@ -4,7 +4,7 @@ const TOKEN: &str = "0123456789abcdef0123456789abcdef"; fn opened_line() -> String { format!( - r#"{{"event":"pane_opened","v":1,"token":"{TOKEN}","generation":2,"title":null,"command":"claude","cwd":"/w/repo"}}"# + r#"{{"event":"pane_opened","v":{PROTOCOL_VERSION},"token":"{TOKEN}","generation":2,"title":null,"command":"claude","cwd":"/w/repo"}}"# ) } @@ -28,23 +28,42 @@ fn a_host_event_is_parsed_from_its_wire_shape() { #[test] fn an_event_from_another_protocol_version_is_refused_and_both_versions_named() { - let line = opened_line().replace("\"v\":1", "\"v\":2"); + let alien = PROTOCOL_VERSION + 1; + let line = opened_line().replace( + &format!("\"v\":{PROTOCOL_VERSION}"), + &format!("\"v\":{alien}"), + ); let err = decode_event(&line) .expect_err("a version this build cannot speak") .to_string(); - assert!(err.contains('2') && err.contains('1'), "{err}"); + assert!( + err.contains(&alien.to_string()) && err.contains(&PROTOCOL_VERSION.to_string()), + "{err}" + ); +} + +#[test] +fn an_event_from_the_previous_protocol_version_is_refused_too() { + // Refused in both directions on purpose. A host still speaking 1 has no + // `watch_pane` to honour, so the panes this plugin exists for would silently + // never be watched — better to fail at the first line than halfway. + let line = opened_line().replace(&format!("\"v\":{PROTOCOL_VERSION}"), "\"v\":1"); + assert!(decode_event(&line).is_err()); } #[test] fn an_unknown_event_is_refused_rather_than_guessed() { - let line = r#"{"event":"pane_teleported","v":1,"token":"abc","generation":1}"#; - assert!(decode_event(line).is_err()); + let line = format!( + r#"{{"event":"pane_teleported","v":{PROTOCOL_VERSION},"token":"abc","generation":1}}"# + ); + assert!(decode_event(&line).is_err()); } #[test] fn an_event_missing_a_required_field_is_refused() { - let line = r#"{"event":"pane_idle","v":1,"token":"abc","generation":1}"#; - assert!(decode_event(line).is_err(), "idle_ms is required"); + let line = + format!(r#"{{"event":"pane_idle","v":{PROTOCOL_VERSION},"token":"abc","generation":1}}"#); + assert!(decode_event(&line).is_err(), "idle_ms is required"); } #[test] @@ -56,7 +75,8 @@ fn a_line_over_the_length_limit_is_refused_before_it_is_parsed() { #[test] fn a_shutdown_names_no_pane() { - let event = decode_event(r#"{"event":"shutdown","v":1}"#).expect("a shutdown"); + let event = decode_event(&format!(r#"{{"event":"shutdown","v":{PROTOCOL_VERSION}}}"#)) + .expect("a shutdown"); assert_eq!(event.token(), None); assert_eq!(event.generation(), None); } @@ -72,7 +92,25 @@ fn a_command_is_encoded_as_one_line_tagged_with_its_name() { .expect("encodable"); assert!(!line.contains('\n')); assert!(line.contains("\"cmd\":\"relaunch\""), "{line}"); - assert!(line.contains("\"v\":1"), "{line}"); + assert!( + line.contains(&format!("\"v\":{PROTOCOL_VERSION}")), + "{line}" + ); +} + +#[test] +fn a_watch_pane_request_carries_only_the_token() { + // Deliberately no generation: this is asked about a pane the host has never + // described to us, so any generation we put here would be invented — and the + // host would be right to refuse it. + let line = encode_command(&watch_pane(TOKEN.to_string())).expect("encodable"); + assert!(line.contains("\"cmd\":\"watch_pane\""), "{line}"); + assert!(line.contains(&format!("\"token\":\"{TOKEN}\"")), "{line}"); + assert!(!line.contains("generation"), "{line}"); + assert!( + line.contains(&format!("\"v\":{PROTOCOL_VERSION}")), + "{line}" + ); } #[test] @@ -133,7 +171,7 @@ fn the_hosts_copy_of_the_contract_still_says_the_same_thing() { let source = std::fs::read_to_string(host) .unwrap_or_else(|e| panic!("cannot read the host's protocol at {host}: {e}")); - assert_eq!(host_const(&source, "PROTOCOL_VERSION"), "1"); + assert_eq!(host_const(&source, "PROTOCOL_VERSION"), "2"); assert_eq!(host_const(&source, "MAX_LINE_BYTES"), "64 * 1024"); assert_eq!(host_const(&source, "MAX_INPUT_BYTES"), "8 * 1024"); assert_eq!( diff --git a/plugins/nightcrow-recovery/src/provider/mod.rs b/plugins/nightcrow-recovery/src/provider/mod.rs index b7d9b8f..30c643a 100644 --- a/plugins/nightcrow-recovery/src/provider/mod.rs +++ b/plugins/nightcrow-recovery/src/provider/mod.rs @@ -159,6 +159,25 @@ pub fn detect(command: Option<&str>) -> Option> { } } +/// Pick an adapter from a signal that arrived over the IPC socket, for a pane +/// whose command line says nothing — the shell somebody opened and then started +/// a provider CLI inside by hand. +/// +/// Sound because a [`SignalKind`] is minted by exactly one provider's helper: +/// a `stop_failure` line can only have come from the Claude Code hook this +/// binary installed into Claude Code's own settings. The signal is therefore +/// evidence of what the pane is running, in a way terminal text never is — which +/// is why this is a lookup on the wire kind and deliberately not a second +/// sniffing path. A kind added later has to be classified here rather than +/// falling through to a guess. +pub fn detect_from_signal(kind: SignalKind) -> Option> { + match kind { + SignalKind::StopFailure | SignalKind::RateLimits => { + Some(Box::new(claude::Claude::default())) + } + } +} + /// The command's program name without its directory, so `/usr/local/bin/claude` /// and `claude --foo` both resolve to `claude`. fn first_word(command: &str) -> Option<&str> { diff --git a/plugins/nightcrow-recovery/src/provider/mod_tests.rs b/plugins/nightcrow-recovery/src/provider/mod_tests.rs index 9b52ebb..8ee1b9c 100644 --- a/plugins/nightcrow-recovery/src/provider/mod_tests.rs +++ b/plugins/nightcrow-recovery/src/provider/mod_tests.rs @@ -93,6 +93,26 @@ fn a_pane_running_something_else_is_not_watched_at_all() { } } +#[test] +fn every_signal_kind_names_the_adapter_whose_helper_minted_it() { + // A pane with no command line of its own — the shell somebody typed `claude` + // into — has only the signal to go on, and the signal's kind is enough: each + // one is written by exactly one provider's helper. + for kind in [SignalKind::StopFailure, SignalKind::RateLimits] { + let provider = + detect_from_signal(kind).unwrap_or_else(|| panic!("{kind:?} names an adapter")); + assert_eq!(provider.name(), "claude"); + } +} + +#[test] +fn a_signal_binds_an_adapter_where_the_command_line_cannot() { + // The pair that makes the late-adoption path work at all: `detect` gives up + // on a pane with no command, and the signal is what answers instead. + assert!(detect(None).is_none()); + assert!(detect_from_signal(SignalKind::StopFailure).is_some()); +} + #[test] fn a_signal_kind_round_trips_through_its_wire_name() { for kind in [SignalKind::StopFailure, SignalKind::RateLimits] { diff --git a/plugins/nightcrow-recovery/src/runloop.rs b/plugins/nightcrow-recovery/src/runloop.rs index 94a3ea6..20de066 100644 --- a/plugins/nightcrow-recovery/src/runloop.rs +++ b/plugins/nightcrow-recovery/src/runloop.rs @@ -7,17 +7,19 @@ //! wakeup, expired or not, ticks the state machines. //! //! Everything the plugin says goes out from this one thread, so the NDJSON -//! stream cannot interleave two half-written lines. +//! stream cannot interleave two half-written lines — see +//! [`runloop_io`](crate::runloop_io), which owns both ends of that stream. use crate::ipc::{Ipc, IpcMessage, socket_path}; -use crate::protocol::{LogLevel, PluginCommand, PluginEvent, decode_event, encode_command, log}; -use crate::provider::{PaneContext, Provider, detect}; +use crate::protocol::{LogLevel, PluginEvent, log}; +use crate::provider::{OutOfBand, PaneContext, Provider, detect, detect_from_signal}; +use crate::runloop_adopt::Adoptions; +use crate::runloop_io::{Message, emit, emit_all, spawn_stdin_reader}; use crate::state::{PaneRecovery, RecoveryState}; use crate::wait::now_epoch; use anyhow::Result; use std::collections::HashMap; -use std::io::{BufRead, BufReader, Write}; -use std::sync::mpsc::{Sender, channel}; +use std::sync::mpsc::channel; use std::time::{Duration, Instant}; /// How often the state machines are advanced when nothing else happens. @@ -31,20 +33,12 @@ const TICK: Duration = Duration::from_secs(1); /// /// A session holds a handful of panes. The cap exists so a host that somehow /// announced panes without ever closing them cannot grow this process without -/// bound; reaching it means dropping the *new* pane, which fails closed. +/// bound; reaching it means dropping the *new* pane, which fails closed. It +/// binds the panes we asked for as well as the ones we were given — the ask is +/// bounded separately (see [`Adoptions`]), but this is the ceiling on what any +/// number of asks can add up to. const MAX_TRACKED_PANES: usize = 64; -/// What the main thread waits on. -enum Message { - Host(PluginEvent), - /// A line from the host that could not be understood. Reported and skipped: - /// one bad line is not a reason to abandon a session's panes. - HostGarbage(String), - Signal(IpcMessage), - /// stdin ended. The host is gone, so there is nothing left to serve. - HostGone, -} - /// One tracked pane: its recovery progress and the adapter watching it. struct Watch { recovery: PaneRecovery, @@ -79,6 +73,7 @@ pub fn run() -> Result<()> { spawn_stdin_reader(tx); let mut panes: HashMap = HashMap::new(); + let mut adoptions = Adoptions::default(); emit(&log(LogLevel::Info, "nightcrow-recovery watching panes"))?; loop { let message = match rx.recv_timeout(TICK) { @@ -92,13 +87,14 @@ pub fn run() -> Result<()> { Some(Message::HostGone) | Some(Message::Host(PluginEvent::Shutdown { .. })) => { return farewell(&panes); } - Some(Message::Host(event)) => on_host_event(&mut panes, &event)?, + Some(Message::Host(event)) => on_host_event(&mut panes, &mut adoptions, &event)?, Some(Message::HostGarbage(reason)) => { emit(&log(LogLevel::Warn, reason))?; } - Some(Message::Signal(msg)) => on_signal(&mut panes, msg)?, + Some(Message::Signal(msg)) => on_signal(&mut panes, &mut adoptions, msg)?, None => {} } + adoptions.prune(Instant::now()); tick(&mut panes)?; } } @@ -123,33 +119,19 @@ fn farewell(panes: &HashMap) -> Result<()> { Ok(()) } -fn spawn_stdin_reader(tx: Sender) { - std::thread::spawn(move || { - let reader = BufReader::new(std::io::stdin()); - for line in reader.lines() { - let Ok(line) = line else { - break; - }; - if line.trim().is_empty() { - continue; - } - let message = match decode_event(&line) { - Ok(event) => Message::Host(event), - Err(e) => Message::HostGarbage(format!("ignoring a host line: {e}")), - }; - if tx.send(message).is_err() { - return; - } - } - let _ = tx.send(Message::HostGone); - }); -} - -fn on_host_event(panes: &mut HashMap, event: &PluginEvent) -> Result<()> { +fn on_host_event( + panes: &mut HashMap, + adoptions: &mut Adoptions, + event: &PluginEvent, +) -> Result<()> { let Some(token) = event.token().cloned() else { return Ok(()); }; let now = now_epoch(); + // The signal that won this pane its watcher, held back until the pane has + // been through the housekeeping below — an adapter must not be asked about a + // pane whose first event has not been applied yet. + let mut held = None; if let PluginEvent::PaneOpened { generation, command, @@ -157,7 +139,14 @@ fn on_host_event(panes: &mut HashMap, event: &PluginEvent) -> Res .. } = event { - open_pane(panes, &token, *generation, command.as_deref(), cwd)?; + held = open_pane( + panes, + adoptions, + &token, + *generation, + command.as_deref(), + cwd, + )?; } let Some(watch) = panes.get_mut(&token) else { return Ok(()); @@ -184,39 +173,57 @@ fn on_host_event(panes: &mut HashMap, event: &PluginEvent) -> Res if matches!(event, PluginEvent::PaneClosed { .. }) { panes.remove(&token); } + if let Some(signal) = held { + deliver_signal(panes, &token, &signal)?; + } Ok(()) } +/// Start watching the pane `token` names, answering with the signal that has been +/// waiting for it — see [`Adoptions`]. fn open_pane( panes: &mut HashMap, + adoptions: &mut Adoptions, token: &str, generation: u32, command: Option<&str>, cwd: &str, -) -> Result<()> { +) -> Result> { let ctx = PaneContext { token: token.to_string(), generation, cwd: cwd.to_string(), command: command.map(str::to_string), }; + // Claimed unconditionally: whether or not it decides the adapter below, a + // request that has been answered must not stay outstanding. + let claimed = adoptions.claim(token); if let Some(watch) = panes.get_mut(token) { // A relaunch reopens the same slot. The recovery state survives, so an // attempt budget cannot be reset by relaunching into the same limit. watch.ctx = ctx; - return Ok(()); + return Ok(claimed); } - let Some(provider) = detect(command) else { + // The command line first, since it is the host's own record of what the pane + // was launched as. Only when there is none does the signal decide, which is + // the pane somebody started a CLI in by hand: it says nothing about itself, + // but a provider's helper has just spoken for it. + let provider = detect(command).or_else(|| { + claimed + .as_ref() + .and_then(|signal| detect_from_signal(signal.kind)) + }); + let Some(provider) = provider else { // A pane running something this plugin knows nothing about is not // watched at all, which is the cheapest way to stay out of it. - return Ok(()); + return Ok(None); }; if panes.len() >= MAX_TRACKED_PANES { emit(&log( LogLevel::Warn, format!("already watching {MAX_TRACKED_PANES} panes; not watching another"), ))?; - return Ok(()); + return Ok(None); } emit(&log( LogLevel::Info, @@ -230,18 +237,39 @@ fn open_pane( ctx, }, ); + Ok(claimed) +} + +fn on_signal( + panes: &mut HashMap, + adoptions: &mut Adoptions, + msg: IpcMessage, +) -> Result<()> { + if panes.contains_key(&msg.token) { + let (token, signal) = msg.into_signal(); + return deliver_signal(panes, &token, &signal); + } + // Not a pane we were told about — and that is the common case rather than the + // odd one. The token is proof the sender runs inside one of the host's panes, + // so ask for it; a token from another nightcrow session simply goes + // unanswered. See [`Adoptions`] for why asking is bounded. + if let Some(command) = adoptions.request(msg, Instant::now()) { + emit(&command)?; + } Ok(()) } -fn on_signal(panes: &mut HashMap, msg: IpcMessage) -> Result<()> { - let (token, signal) = msg.into_signal(); - // A signal for a pane we do not track is ordinary: a provider helper from - // another nightcrow session, or a pane whose command we do not watch. - let Some(watch) = panes.get_mut(&token) else { +/// Hand one out-of-band signal to the adapter watching `token`. +fn deliver_signal( + panes: &mut HashMap, + token: &str, + signal: &OutOfBand, +) -> Result<()> { + let Some(watch) = panes.get_mut(token) else { return Ok(()); }; let now = now_epoch(); - if let Some(limit) = watch.provider.on_signal(&watch.ctx, &signal, now) { + if let Some(limit) = watch.provider.on_signal(&watch.ctx, signal, now) { let commands = watch.recovery.note_limit(limit, now, Instant::now()); emit_all(&commands)?; } @@ -263,27 +291,3 @@ fn tick(panes: &mut HashMap) -> Result<()> { } Ok(()) } - -fn emit_all(commands: &[PluginCommand]) -> Result<()> { - for command in commands { - emit(command)?; - } - Ok(()) -} - -/// Write one command as one NDJSON line. A command that cannot be framed is -/// dropped with a complaint rather than corrupting the stream. -fn emit(command: &PluginCommand) -> Result<()> { - let line = match encode_command(command) { - Ok(line) => line, - Err(e) => encode_command(&log( - LogLevel::Error, - format!("dropped an unencodable command: {e}"), - ))?, - }; - let mut out = std::io::stdout().lock(); - out.write_all(line.as_bytes())?; - out.write_all(b"\n")?; - out.flush()?; - Ok(()) -} diff --git a/plugins/nightcrow-recovery/src/runloop_adopt.rs b/plugins/nightcrow-recovery/src/runloop_adopt.rs new file mode 100644 index 0000000..bd0752f --- /dev/null +++ b/plugins/nightcrow-recovery/src/runloop_adopt.rs @@ -0,0 +1,105 @@ +//! Asking the host for a pane it never named to us. +//! +//! The dominant way a coding CLI gets started is by hand: the user opens a plain +//! shell and types `claude` into it. That pane's `[[startup_command]]` names no +//! plugin, so the host never mentions it — but the CLI's hook still reaches us +//! over the socket, carrying the token the host put in that pane's environment. +//! Presenting the token back is the whole request; the host decides whether to +//! honour it, and never tells us when it does not. +//! +//! Everything here exists because of that silence. A refusal is indistinguishable +//! from a token belonging to another nightcrow session's pane, so a request that +//! goes unanswered must not be repeated in a tight loop and must not leave state +//! behind that grows with every stranger that knocks. + +use crate::ipc::IpcMessage; +use crate::protocol::{PaneToken, PluginCommand, watch_pane}; +use crate::provider::OutOfBand; +use std::collections::HashMap; +use std::time::{Duration, Instant}; + +/// Most tokens we will have an outstanding request for at once. +/// +/// These are unsolicited: anything that can reach the socket can name a token we +/// have never seen. A session has a handful of panes and the host answers within +/// one of its ticks, so a backlog this deep already means the requests are not +/// being honoured — and dropping the newest is what keeps a stream of strangers +/// from growing this map. +const MAX_PENDING: usize = 8; + +/// How long one token's unanswered request suppresses another for that token. +/// +/// The host answers in milliseconds or never, so this is not a retry interval — +/// it is the rate at which a token that is not ours may cost us a command. +/// Claude Code's statusline runs on every render, so without it a foreign pane +/// would have us writing a request several times a second, all refused and every +/// one of them counted against the host's per-tick command budget alongside the +/// requests that matter. Half a minute is far longer than any honoured request +/// takes and short enough that a pane which only just became ours is not shut +/// out for long. +const REQUEST_COOLDOWN: Duration = Duration::from_secs(30); + +/// One outstanding request, and the signal that justified making it. +struct Pending { + /// Kept so the adapter still gets it. The signal arrives *before* the pane + /// does — it is the reason the pane arrives at all — and the host replays no + /// history to a pane it has just handed over, so dropping it would lose the + /// very limit being recovered from and leave the pane parked until the + /// provider happened to fail again. + signal: OutOfBand, + asked_at: Instant, +} + +/// The tokens we have asked about and not yet been given. +#[derive(Default)] +pub(crate) struct Adoptions(HashMap); + +impl Adoptions { + /// Turn a signal for an untracked pane into a request, or into nothing. + /// + /// Consumes the message either way: when the answer is nothing, the token and + /// its payload are dropped here rather than recorded, so a token that will + /// never be honoured costs one hash probe and no growth. + pub(crate) fn request(&mut self, msg: IpcMessage, now: Instant) -> Option { + let (token, signal) = msg.into_signal(); + // Already asked, and [`Self::prune`] has not yet given up on it. + if self.0.contains_key(&token) { + return None; + } + if self.0.len() >= MAX_PENDING { + return None; + } + self.0.insert( + token.clone(), + Pending { + signal, + asked_at: now, + }, + ); + Some(watch_pane(token)) + } + + /// Take back the signal that won `token` its request, now that the host has + /// described the pane. Answers `None` for a pane we never asked about, which + /// is every configured pane. + pub(crate) fn claim(&mut self, token: &str) -> Option { + self.0.remove(token).map(|pending| pending.signal) + } + + /// Give up on requests the host has not answered within + /// [`REQUEST_COOLDOWN`], which is also what lets that token be asked about + /// again. + /// + /// Without it a handful of foreign tokens would hold every slot for the + /// process's whole life, and a pane that later became ours could not get a + /// request in. Giving up is safe: a pane that really is ours signals again, + /// and the held signal is stale by then anyway. + pub(crate) fn prune(&mut self, now: Instant) { + self.0 + .retain(|_, p| now.saturating_duration_since(p.asked_at) < REQUEST_COOLDOWN); + } +} + +#[cfg(test)] +#[path = "runloop_adopt_tests.rs"] +mod tests; diff --git a/plugins/nightcrow-recovery/src/runloop_adopt_tests.rs b/plugins/nightcrow-recovery/src/runloop_adopt_tests.rs new file mode 100644 index 0000000..0a1efae --- /dev/null +++ b/plugins/nightcrow-recovery/src/runloop_adopt_tests.rs @@ -0,0 +1,128 @@ +//! Asking for a pane: what is asked once, what is not asked twice, and what a +//! stream of tokens we will never be given can cost this process. + +use super::*; +use crate::provider::SignalKind; + +fn signal(token: &str) -> IpcMessage { + IpcMessage { + token: token.to_string(), + kind: SignalKind::StopFailure, + payload: serde_json::json!({"error_type": "rate_limit"}), + } +} + +fn asked_token(command: &PluginCommand) -> String { + match command { + PluginCommand::WatchPane { token, .. } => token.clone(), + other => panic!("expected a watch_pane request, got {other:?}"), + } +} + +#[test] +fn a_signal_for_an_unknown_pane_asks_the_host_for_it_by_token() { + let mut a = Adoptions::default(); + let command = a + .request(signal("abc123"), Instant::now()) + .expect("a first signal asks"); + assert_eq!(asked_token(&command), "abc123"); +} + +#[test] +fn a_second_signal_for_the_same_token_asks_nothing_more() { + // The host answers in milliseconds or never, so a repeat inside the cooldown + // could only be noise — and Claude Code's statusline is noisy: it runs on + // every render. + let mut a = Adoptions::default(); + let now = Instant::now(); + assert!(a.request(signal("abc123"), now).is_some()); + for _ in 0..50 { + assert!(a.request(signal("abc123"), now).is_none()); + } +} + +#[test] +fn a_token_may_be_asked_about_again_once_its_request_has_been_given_up_on() { + let mut a = Adoptions::default(); + let now = Instant::now(); + assert!(a.request(signal("abc123"), now).is_some()); + + a.prune(now + REQUEST_COOLDOWN); + + assert!( + a.request(signal("abc123"), now + REQUEST_COOLDOWN) + .is_some(), + "a pane that only just became ours must not be shut out for good" + ); +} + +#[test] +fn a_request_still_inside_its_cooldown_survives_pruning() { + let mut a = Adoptions::default(); + let now = Instant::now(); + assert!(a.request(signal("abc123"), now).is_some()); + + a.prune(now + REQUEST_COOLDOWN / 2); + + assert!( + a.request(signal("abc123"), now + REQUEST_COOLDOWN / 2) + .is_none() + ); +} + +#[test] +fn a_flood_of_unknown_tokens_stops_at_the_pending_ceiling() { + // Unsolicited state: anything that can reach the socket can name a token we + // have never seen, so this must stop growing rather than stop working. + let mut a = Adoptions::default(); + let now = Instant::now(); + for i in 0..MAX_PENDING { + assert!( + a.request(signal(&format!("token{i}")), now).is_some(), + "the first {MAX_PENDING} tokens are asked about" + ); + } + assert!( + a.request(signal("onemore"), now).is_none(), + "past the ceiling a new token is dropped, not queued" + ); + // And the dropped token left nothing behind, so the ceiling is a ceiling on + // memory and not merely on requests. + a.prune(now + REQUEST_COOLDOWN); + assert!( + a.request(signal("onemore"), now + REQUEST_COOLDOWN) + .is_some() + ); +} + +#[test] +fn the_signal_that_won_a_pane_its_request_is_handed_back_when_the_pane_arrives() { + // The signal arrives before the pane does and the host replays no history, so + // losing it here would lose the very limit the recovery is about. + let mut a = Adoptions::default(); + assert!(a.request(signal("abc123"), Instant::now()).is_some()); + + let held = a.claim("abc123").expect("the signal was kept"); + + assert_eq!(held.kind, SignalKind::StopFailure); + assert_eq!(held.payload["error_type"], "rate_limit"); +} + +#[test] +fn a_claimed_request_is_not_handed_back_a_second_time() { + let mut a = Adoptions::default(); + assert!(a.request(signal("abc123"), Instant::now()).is_some()); + assert!(a.claim("abc123").is_some()); + assert!( + a.claim("abc123").is_none(), + "one signal must not be applied twice" + ); +} + +#[test] +fn a_pane_we_never_asked_about_has_no_signal_waiting_for_it() { + // Every configured pane takes this path: the host named it, so no request was + // ever made for it. + let mut a = Adoptions::default(); + assert!(a.claim("never-asked").is_none()); +} diff --git a/plugins/nightcrow-recovery/src/runloop_io.rs b/plugins/nightcrow-recovery/src/runloop_io.rs new file mode 100644 index 0000000..fa647ca --- /dev/null +++ b/plugins/nightcrow-recovery/src/runloop_io.rs @@ -0,0 +1,69 @@ +//! The plugin's two ends of the host's NDJSON stream. +//! +//! Split out of `runloop.rs` so that file is the loop's reasoning and this one is +//! its plumbing. Everything the plugin says leaves through [`emit`], called only +//! from the main thread, which is what keeps two half-written lines from +//! interleaving on stdout. + +use crate::ipc::IpcMessage; +use crate::protocol::{LogLevel, PluginCommand, PluginEvent, decode_event, encode_command, log}; +use anyhow::Result; +use std::io::{BufRead, BufReader, Write}; +use std::sync::mpsc::Sender; + +/// What the main thread waits on. +pub(crate) enum Message { + Host(PluginEvent), + /// A line from the host that could not be understood. Reported and skipped: + /// one bad line is not a reason to abandon a session's panes. + HostGarbage(String), + Signal(IpcMessage), + /// stdin ended. The host is gone, so there is nothing left to serve. + HostGone, +} + +pub(crate) fn spawn_stdin_reader(tx: Sender) { + std::thread::spawn(move || { + let reader = BufReader::new(std::io::stdin()); + for line in reader.lines() { + let Ok(line) = line else { + break; + }; + if line.trim().is_empty() { + continue; + } + let message = match decode_event(&line) { + Ok(event) => Message::Host(event), + Err(e) => Message::HostGarbage(format!("ignoring a host line: {e}")), + }; + if tx.send(message).is_err() { + return; + } + } + let _ = tx.send(Message::HostGone); + }); +} + +pub(crate) fn emit_all(commands: &[PluginCommand]) -> Result<()> { + for command in commands { + emit(command)?; + } + Ok(()) +} + +/// Write one command as one NDJSON line. A command that cannot be framed is +/// dropped with a complaint rather than corrupting the stream. +pub(crate) fn emit(command: &PluginCommand) -> Result<()> { + let line = match encode_command(command) { + Ok(line) => line, + Err(e) => encode_command(&log( + LogLevel::Error, + format!("dropped an unencodable command: {e}"), + ))?, + }; + let mut out = std::io::stdout().lock(); + out.write_all(line.as_bytes())?; + out.write_all(b"\n")?; + out.flush()?; + Ok(()) +} diff --git a/plugins/nightcrow-recovery/src/state_resume.rs b/plugins/nightcrow-recovery/src/state_resume.rs index 07131c5..8d6697e 100644 --- a/plugins/nightcrow-recovery/src/state_resume.rs +++ b/plugins/nightcrow-recovery/src/state_resume.rs @@ -31,6 +31,11 @@ fn is_safe_arg_char(c: char) -> bool { c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-' | ':' | '/' | '=' | '@' | '+') } +/// Said of a pane the host launched no command in, so the person reading the +/// status line learns why the wait ended without a resume. +const NO_COMMAND_TO_RELAUNCH: &str = + "this pane has no command to relaunch; start the session again by hand"; + fn args_are_safe(args: &[String]) -> bool { !args.is_empty() && args.len() <= MAX_RESUME_ARGS @@ -68,7 +73,7 @@ impl PaneRecovery { self.goto(RecoveryState::NeedsAttention) } ResumePlan::Input(data) => self.send_input(data, now_epoch, now), - ResumePlan::Relaunch(args) => self.relaunch(args, now), + ResumePlan::Relaunch(args) => self.relaunch(args, ctx, now), } } @@ -94,12 +99,25 @@ impl PaneRecovery { self.spend_attempt(command, now) } - fn relaunch(&mut self, args: Vec, now: Instant) -> Vec { + fn relaunch( + &mut self, + args: Vec, + ctx: &PaneContext, + now: Instant, + ) -> Vec { if self.alive { // The host refuses a relaunch of a live pane, so wait for the exit // rather than spending an attempt learning that again. return Vec::new(); } + if ctx.command.is_none() { + // A pane with no command of its own is a shell somebody started the + // provider inside by hand. Putting a process back would start the + // shell again, not the session, so the host refuses it outright — + // and there is nothing to wait for that would change that. + self.detail = Some(NO_COMMAND_TO_RELAUNCH.to_string()); + return self.goto(RecoveryState::NeedsAttention); + } if !args_are_safe(&args) { self.detail = Some("adapter offered resume args the host would refuse".to_string()); return self.goto(RecoveryState::NeedsAttention); diff --git a/plugins/nightcrow-recovery/src/state_tests/resume.rs b/plugins/nightcrow-recovery/src/state_tests/resume.rs index cae3d61..ead34ce 100644 --- a/plugins/nightcrow-recovery/src/state_tests/resume.rs +++ b/plugins/nightcrow-recovery/src/state_tests/resume.rs @@ -185,6 +185,59 @@ fn an_adapter_offering_unsafe_resume_args_is_refused_without_asking_the_host() { assert!(action(&out).is_none()); } +/// A pane the host launched no command in: the plain shell somebody started a +/// provider CLI inside by hand, which is the only kind of pane this plugin can be +/// given on a signal. +fn bare_shell() -> PaneContext { + PaneContext { + command: None, + ..ctx() + } +} + +#[test] +fn a_pane_the_host_launched_no_command_in_is_never_relaunched() { + // Putting a process back would start the shell again, not the session, so the + // host refuses it outright — and no amount of waiting changes that, which is + // why this ends in needs_attention instead of another backoff. + let mut rec = recovery(); + let provider = FakeProvider::relaunch_only(); + let mono = Instant::now(); + rec.note_limit(usage(Some(SESSION), Some(RESET)), T0, mono); + rec.on_event(&exited(1)).expect("current generation"); + let out = rec.tick( + &provider, + &bare_shell(), + T0 + AFTER_RESET, + at(mono, AFTER_RESET), + ); + assert_eq!(rec.state(), RecoveryState::NeedsAttention); + assert!(action(&out).is_none(), "the host is asked for nothing"); + assert_eq!(rec.attempt(), 0, "and no attempt is spent learning that"); +} + +#[test] +fn a_pane_with_no_command_is_still_typed_into_while_its_process_lives() { + // The other half of the same rule: typed input is the whole of the recovery + // such a pane can get, so the missing command line must not cost it that too. + let mut rec = recovery(); + let provider = FakeProvider::default(); + let mono = Instant::now(); + rec.note_limit(usage(Some(SESSION), Some(RESET)), T0, mono); + rec.on_event(&went_idle(1)).expect("current generation"); + let out = rec.tick( + &provider, + &bare_shell(), + T0 + AFTER_RESET, + at(mono, AFTER_RESET), + ); + assert_eq!(rec.state(), RecoveryState::Resuming); + match action(&out) { + Some(PluginCommand::SendInput { data, .. }) => assert_eq!(data, "continue\r"), + other => panic!("expected typed input, got {other:?}"), + } +} + #[test] fn an_adapter_with_nothing_to_say_costs_one_attempt_and_backs_off() { let mut rec = recovery(); diff --git a/src/config/plugin.rs b/src/config/plugin.rs index 99ae088..f603a76 100644 --- a/src/config/plugin.rs +++ b/src/config/plugin.rs @@ -4,8 +4,10 @@ use std::collections::{BTreeMap, BTreeSet}; /// An external plugin process (`[[plugin]]`). nightcrow itself knows nothing /// about what a plugin does: it launches the executable and speaks its protocol. -/// A plugin only ever sees a pane whose `[[startup_command]]` opted in by name, -/// so adding a plugin here does not hand it the whole session. +/// A plugin only ever sees a pane that opted in — by name in a +/// `[[startup_command]]`, or, with [`watch_on_signal`](Self::watch_on_signal), +/// by something inside the pane quoting the pane's own token — so adding a +/// plugin here does not hand it the whole session. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct PluginConfig { /// Name a startup pane refers to in its `plugin =` field. @@ -30,6 +32,19 @@ pub struct PluginConfig { /// Off unless explicitly turned on. #[serde(default)] pub enabled: bool, + /// Whether this plugin may be given a pane no `[[startup_command]]` named + /// it in, when a process *inside* that pane reports to it quoting the pane's + /// own token. + /// + /// Off by default, so an existing config keeps the property that the opt-in + /// list is the whole of what a plugin can see. Turning it on trades that for + /// a narrower one: a pane is reachable once something running in it has + /// spoken to the plugin, which a plain shell never does. The token is what + /// makes the difference — it is random, per pane, and only in that pane's + /// child environment, so it cannot be guessed from outside and the plugin is + /// never told which panes exist. + #[serde(default)] + pub watch_on_signal: bool, } /// Check the `[[plugin]]` list and every startup pane's opt-in against it. diff --git a/src/config/tests/plugin.rs b/src/config/tests/plugin.rs index 99e44b1..1268583 100644 --- a/src/config/tests/plugin.rs +++ b/src/config/tests/plugin.rs @@ -49,6 +49,34 @@ fn plugin_args_and_env_default_to_empty_when_omitted() { assert!(cfg.plugins[0].env.is_empty()); } +#[test] +fn watch_on_signal_is_off_unless_the_config_asks_for_it() { + // The default is what every existing config gets, and it has to keep meaning + // "the opt-in list is the whole of what this plugin can see". + let cfg: Config = + toml::from_str("[[plugin]]\nname = \"recovery\"\ncommand = \"run-me\"\n").unwrap(); + assert!(!cfg.plugins[0].watch_on_signal); + + let cfg: Config = toml::from_str( + "[[plugin]]\nname = \"recovery\"\ncommand = \"run-me\"\nwatch_on_signal = true\n", + ) + .unwrap(); + assert!(cfg.plugins[0].watch_on_signal); + validate_config(&cfg).unwrap(); +} + +#[test] +fn a_plugin_that_watches_on_signal_needs_no_pane_to_name_it() { + // Its panes are the ones that will speak to it, and none of them can be known + // in advance — so requiring an opt-in here would make the switch unusable. + let cfg: Config = toml::from_str( + "[[plugin]]\nname = \"recovery\"\ncommand = \"run-me\"\n\ + enabled = true\nwatch_on_signal = true\n", + ) + .unwrap(); + validate_config(&cfg).unwrap(); +} + #[test] fn a_plugin_without_name_or_command_fails_to_deserialize() { assert!(toml::from_str::("[[plugin]]\ncommand = \"run-me\"\n").is_err()); diff --git a/src/plugin/guard.rs b/src/plugin/guard.rs index b268fe4..9fde4e3 100644 --- a/src/plugin/guard.rs +++ b/src/plugin/guard.rs @@ -5,10 +5,14 @@ //! [`Guard::judge`], which returns either an [`Approved`] action naming a real //! [`PaneId`] or a [`Refused`] saying why not. There is no other way through: a //! [`PluginCommand`] carries a token, and only the guard turns one into a pane. +//! +//! That includes gaining a pane in the first place: `guard_watch` beside this +//! file holds the only rule that can widen what a plugin is allowed to see. use super::guard_budget::{Budgets, RateAction, RateLimits}; use super::guard_refusal::Refused; use super::guard_text::{is_forbidden_control, truncate_message}; +use super::guard_watch::judge_watch; use super::protocol::{LogLevel, MAX_INPUT_BYTES, PluginCommand}; use crate::backend::slot::resume_command_line; use crate::backend::{PaneGeneration, PaneId, PaneToken}; @@ -22,8 +26,17 @@ use std::time::{Duration, Instant}; pub struct PaneFacts { pub pane: PaneId, pub generation: PaneGeneration, - /// The pane's `[[startup_command]]` named *this* plugin. + /// This plugin already has the pane, by opt-in or by an earlier + /// [`PluginCommand::WatchPane`]. pub opted_in: bool, + /// Some *other* plugin has the pane. Distinct from `!opted_in`, which is + /// equally true of a pane no plugin has at all — and those two answer + /// [`PluginCommand::WatchPane`] differently. + pub watched_by_another: bool, + /// The requesting plugin's `watch_on_signal`: whether the operator allowed + /// it to be given a pane it was never named by. A property of the plugin + /// rather than of the pane, carried here so the rules stay one struct wide. + pub may_watch_on_signal: bool, /// The pane's process is still running. pub alive: bool, /// Since the pane last produced output. @@ -57,6 +70,11 @@ pub enum Approved { deadline_epoch: Option, attempt: u32, }, + /// The plugin may be given this pane. Recording that, and telling the + /// plugin, is the caller's job. + WatchPane { + pane: PaneId, + }, Log { level: LogLevel, message: String, @@ -127,6 +145,10 @@ impl Guard { let facts = pane_facts(&token, generation, facts)?; self.judge_relaunch(&token, facts, resume_args, allowed_resume_flags, now) } + // Deliberately outside `pane_facts`: this is the one command whose + // whole point is a pane that has *not* opted in, and it names no + // generation to check. + PluginCommand::WatchPane { token, .. } => judge_watch(&token, facts), PluginCommand::Status { token, generation, @@ -183,7 +205,8 @@ impl Guard { code: bad as u32, }); } - self.spend(token, pane, RateAction::SendInput, now)?; + self.budgets + .spend(token, pane, RateAction::SendInput, &self.limits, now)?; Ok(Approved::SendInput { pane, data: data.into_bytes(), @@ -204,6 +227,15 @@ impl Guard { // recovery acts on live panes, relaunch only on exited ones. return Err(Refused::PaneStillRunning { pane }); } + if facts.launch_command.is_none() { + // A bare shell. Putting a process back here would start the shell + // again, not whatever the person ran inside it, and the resume + // arguments would have nothing to attach to — so the pane's only + // recovery is the one typed into it while it is still alive. Checked + // before `resume_command_line`, which also refuses this, so the log + // says the pane was never relaunchable rather than blaming the args. + return Err(Refused::NoLaunchCommand { pane }); + } let command_line = resume_command_line( facts.launch_command.as_deref(), &resume_args, @@ -213,42 +245,14 @@ impl Guard { pane, reason: e.to_string(), })?; - self.spend(token, pane, RateAction::Relaunch, now)?; + self.budgets + .spend(token, pane, RateAction::Relaunch, &self.limits, now)?; Ok(Approved::Relaunch { pane, resume_args, command_line, }) } - - /// Charge the budget, as the last check before approval. - /// - /// Only approvals spend, deliberately: the budget bounds what a plugin - /// *does* to a pane, and a refused command did nothing. Charging refusals - /// would let noise — a stale generation the plugin could not have known - /// about, a flag config forbids — eat the budget a legitimate action needs, - /// losing the pane's one real attempt to a race. Spam is bounded elsewhere - /// and more cheaply: the outbound queue drops and every refusal is logged. - fn spend( - &mut self, - token: &PaneToken, - pane: PaneId, - action: RateAction, - now: Instant, - ) -> Result<(), Refused> { - if self.budgets.try_spend(token, action, &self.limits, now) { - return Ok(()); - } - Err(Refused::RateLimited { - pane, - action, - limit: match action { - RateAction::SendInput => self.limits.max_sends_per_window, - RateAction::Relaunch => self.limits.max_relaunches_per_window, - }, - window: self.limits.window, - }) - } } /// Rules 2, 3 and 4: the pane must exist, have opted in, and be the same spawn. diff --git a/src/plugin/guard_budget.rs b/src/plugin/guard_budget.rs index c17c1b2..4d1cdaf 100644 --- a/src/plugin/guard_budget.rs +++ b/src/plugin/guard_budget.rs @@ -10,7 +10,8 @@ //! on every exit would then loop with nothing to stop it. The token is what //! survives a relaunch, so it is what the ceiling has to be attached to. -use crate::backend::PaneToken; +use super::guard_refusal::Refused; +use crate::backend::{PaneId, PaneToken}; use std::collections::HashMap; use std::time::{Duration, Instant}; @@ -76,7 +77,7 @@ impl Budgets { /// Nothing is recorded when there is not, so a refusal here costs the slot /// nothing beyond the refusal itself. Each list is capped at its limit by /// construction, so this stays O(limit). - pub(super) fn try_spend( + fn try_spend( &mut self, token: &PaneToken, action: RateAction, @@ -96,6 +97,36 @@ impl Budgets { true } + /// Charge the budget, as the last check before an approval. + /// + /// Only approvals spend, deliberately: the budget bounds what a plugin + /// *does* to a pane, and a refused command did nothing. Charging refusals + /// would let noise — a stale generation the plugin could not have known + /// about, a flag config forbids — eat the budget a legitimate action needs, + /// losing the pane's one real attempt to a race. Spam is bounded elsewhere + /// and more cheaply: the outbound queue drops and every refusal is logged. + pub(super) fn spend( + &mut self, + token: &PaneToken, + pane: PaneId, + action: RateAction, + limits: &RateLimits, + now: Instant, + ) -> Result<(), Refused> { + if self.try_spend(token, action, limits, now) { + return Ok(()); + } + Err(Refused::RateLimited { + pane, + action, + limit: match action { + RateAction::SendInput => limits.max_sends_per_window, + RateAction::Relaunch => limits.max_relaunches_per_window, + }, + window: limits.window, + }) + } + pub(super) fn clear(&mut self, token: &PaneToken) { self.0.remove(token); } diff --git a/src/plugin/guard_refusal.rs b/src/plugin/guard_refusal.rs index 5729e0f..fc9bd04 100644 --- a/src/plugin/guard_refusal.rs +++ b/src/plugin/guard_refusal.rs @@ -38,8 +38,19 @@ pub enum Refused { ControlCharacter { pane: PaneId, code: u32 }, /// A relaunch for a pane whose process is still running. PaneStillRunning { pane: PaneId }, + /// A relaunch for a pane the host never launched a command for. + /// + /// Its own reason rather than one of `ResumeArgsRejected`'s, because nothing + /// about the arguments is wrong: the pane is a bare shell, so there is no + /// invocation to append them to and no amount of retrying changes that. + NoLaunchCommand { pane: PaneId }, /// The resume arguments did not survive the command-line rules. ResumeArgsRejected { pane: PaneId, reason: String }, + /// A plugin asked for a pane it was never named by, without the config + /// switch that allows it. + WatchNotAllowed { pane: PaneId, token: PaneToken }, + /// A plugin asked for a pane another plugin already has. + PaneWatchedByAnother { pane: PaneId }, RateLimited { pane: PaneId, action: RateAction, @@ -91,9 +102,22 @@ impl fmt::Display for Refused { "pane {pane} is still running, so it cannot be relaunched" ) } + Self::NoLaunchCommand { pane } => write!( + f, + "pane {pane} was launched with no command, so there is nothing to relaunch" + ), Self::ResumeArgsRejected { pane, reason } => { write!(f, "relaunch of pane {pane} refused: {reason}") } + Self::WatchNotAllowed { pane, token } => write!( + f, + "pane {pane} (token {}) was not opted in, and this plugin's config \ + does not let it watch a pane on a signal", + token.as_str() + ), + Self::PaneWatchedByAnother { pane } => { + write!(f, "pane {pane} is already watched by another plugin") + } Self::RateLimited { pane, action, diff --git a/src/plugin/guard_tests/mod.rs b/src/plugin/guard_tests/mod.rs index e939b63..2c8cd68 100644 --- a/src/plugin/guard_tests/mod.rs +++ b/src/plugin/guard_tests/mod.rs @@ -8,6 +8,7 @@ mod budget; mod identity; mod input; mod relaunch; +mod watch; pub(super) const PANE: PaneId = 4; pub(super) const GENERATION: PaneGeneration = 9; @@ -28,12 +29,27 @@ pub(super) fn facts() -> PaneFacts { pane: PANE, generation: GENERATION, opted_in: true, + watched_by_another: false, + may_watch_on_signal: false, alive: true, idle: MIN_IDLE, launch_command: Some(LAUNCH.to_string()), } } +/// A live pane no plugin has yet, in a session where the asking plugin is +/// allowed to ask — the one shape [`watch`] is meant to be approved for. +pub(super) fn adoptable_facts() -> PaneFacts { + PaneFacts { + opted_in: false, + may_watch_on_signal: true, + // A pane somebody started a CLI in by hand: the host launched no command + // of its own in it. + launch_command: None, + ..facts() + } +} + /// The same pane after its process ended, which is what a relaunch needs. pub(super) fn exited_facts() -> PaneFacts { PaneFacts { @@ -72,6 +88,13 @@ pub(super) fn status(token: &PaneToken) -> PluginCommand { } } +pub(super) fn watch(token: &PaneToken) -> PluginCommand { + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token.clone(), + } +} + pub(super) fn flags(names: &[&str]) -> Vec { names.iter().map(|n| n.to_string()).collect() } diff --git a/src/plugin/guard_tests/relaunch.rs b/src/plugin/guard_tests/relaunch.rs index f62e461..674850a 100644 --- a/src/plugin/guard_tests/relaunch.rs +++ b/src/plugin/guard_tests/relaunch.rs @@ -1,4 +1,5 @@ -//! Rules 8-9: when a pane's process may be replaced, and with what. +//! Rules 8-9: when a pane's process may be replaced, and with what — including +//! the pane that has no launch command to replace it with. use super::*; @@ -84,13 +85,56 @@ fn a_relaunch_with_an_allowed_flag_carries_the_built_command_line() { #[test] fn relaunching_a_pane_with_no_configured_command_is_refused() { + // Its own reason, not one about the arguments: a bare shell is a pane whose + // process cannot be put back at all, which is what a plugin given a pane on a + // signal has to be told about the only pane it will ever have. let mut g = guard(); let facts = PaneFacts { launch_command: None, ..exited_facts() }; - assert!(matches!( - g.judge(relaunch(&token(), &[]), Some(&facts), &[], Instant::now()), - Err(Refused::ResumeArgsRejected { pane: PANE, .. }) - )); + assert_eq!( + g.judge(relaunch(&token(), &[]), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::NoLaunchCommand { pane: PANE } + ); +} + +#[test] +fn a_relaunch_of_a_pane_with_no_command_is_refused_whatever_flags_are_allowed() { + // The flag list is what normally decides a relaunch, so the refusal must not + // be something a generous `allowed_resume_flags` can talk its way past. + let mut g = guard(); + let facts = PaneFacts { + launch_command: None, + ..exited_facts() + }; + assert_eq!( + g.judge( + relaunch(&token(), &["--resume", "abc123"]), + Some(&facts), + &flags(&["--resume"]), + Instant::now() + ) + .expect_err("refused"), + Refused::NoLaunchCommand { pane: PANE } + ); +} + +#[test] +fn a_refused_relaunch_of_a_bare_shell_costs_no_relaunch_budget() { + // Nothing was done to the pane, and a plugin that keeps asking must not be + // able to exhaust an allowance a legitimate pane would need. + let mut g = guard(); + let t = token(); + let now = Instant::now(); + let limits = RateLimits::default(); + let facts = PaneFacts { + launch_command: None, + ..exited_facts() + }; + for _ in 0..limits.max_relaunches_per_window + 1 { + assert!(g.judge(relaunch(&t, &[]), Some(&facts), &[], now).is_err()); + } + assert_eq!(g.budgets.spent(&t, RateAction::Relaunch, &limits, now), 0); } diff --git a/src/plugin/guard_tests/watch.rs b/src/plugin/guard_tests/watch.rs new file mode 100644 index 0000000..431dddb --- /dev/null +++ b/src/plugin/guard_tests/watch.rs @@ -0,0 +1,130 @@ +//! Rule 12: when a plugin may be given a pane nobody handed it. + +use super::*; + +#[test] +fn a_watch_request_for_a_live_unclaimed_pane_is_approved() { + let mut g = guard(); + assert_eq!( + g.judge( + watch(&token()), + Some(&adoptable_facts()), + &[], + Instant::now() + ) + .expect("allowed"), + Approved::WatchPane { pane: PANE } + ); +} + +#[test] +fn a_watch_request_for_a_token_with_no_live_pane_is_refused() { + // The token could not be resolved, so the sender is not inside any pane this + // host owns — another nightcrow session's helper, most likely. + let mut g = guard(); + let t = token(); + assert_eq!( + g.judge(watch(&t), None, &[], Instant::now()) + .expect_err("refused"), + Refused::UnknownPane { token: t } + ); +} + +#[test] +fn a_watch_request_from_a_plugin_the_config_did_not_allow_is_refused() { + // The switch is the whole difference between this and enumerating panes, so + // its absence has to refuse even a request that is otherwise perfect. + let mut g = guard(); + let t = token(); + let facts = PaneFacts { + may_watch_on_signal: false, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&t), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::WatchNotAllowed { + pane: PANE, + token: t + } + ); +} + +#[test] +fn a_watch_request_for_a_pane_another_plugin_watches_is_refused() { + let mut g = guard(); + let facts = PaneFacts { + watched_by_another: true, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::PaneWatchedByAnother { pane: PANE } + ); +} + +#[test] +fn a_watch_request_for_a_pane_whose_process_has_exited_is_refused() { + // Nothing left to watch: the only recovery such a pane can be given is typed + // into a live process, and this one has none. + let mut g = guard(); + let facts = PaneFacts { + alive: false, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::PaneNotRunning { pane: PANE } + ); +} + +#[test] +fn a_watch_request_for_a_pane_this_plugin_already_has_is_approved_again() { + // How a plugin recovers from having been given an opted-in pane it could not + // recognise: asking again is what gets it another `PaneOpened`. + let mut g = guard(); + let facts = PaneFacts { + opted_in: true, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .expect("allowed"), + Approved::WatchPane { pane: PANE } + ); +} + +#[test] +fn a_watch_request_does_not_spend_the_panes_input_budget() { + // Being given a pane is not something done *to* the pane. Charging it would + // spend the allowance the recovery it unlocks is about to need. + let mut g = guard(); + let t = token(); + let now = Instant::now(); + let limits = RateLimits::default(); + for _ in 0..limits.max_sends_per_window * 2 { + g.judge(watch(&t), Some(&adoptable_facts()), &[], now) + .expect("allowed"); + } + assert_eq!(g.budgets.spent(&t, RateAction::SendInput, &limits, now), 0); + assert_eq!(g.budgets.spent(&t, RateAction::Relaunch, &limits, now), 0); +} + +#[test] +fn a_watch_request_names_no_generation_so_it_cannot_be_stale() { + // A plugin asking for a pane it has never been told about cannot know which + // spawn it is looking at; the `PaneOpened` it is answered with is what says. + // A generation rule here would therefore refuse every honest request. + assert_eq!(watch(&token()).generation(), None); + let mut g = guard(); + let facts = PaneFacts { + generation: GENERATION + 5, + ..adoptable_facts() + }; + assert!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .is_ok() + ); +} diff --git a/src/plugin/guard_watch.rs b/src/plugin/guard_watch.rs new file mode 100644 index 0000000..83f3666 --- /dev/null +++ b/src/plugin/guard_watch.rs @@ -0,0 +1,62 @@ +//! Rule 12: when a plugin may be given a pane nobody handed it. +//! +//! Every other rule in this layer starts from a pane the operator already +//! assigned. This one is the single place an assignment can be *created* at +//! runtime, so it is kept apart from the rest and reads as one list of +//! conditions rather than as a branch inside a larger judgement. +//! +//! What makes it safe is where the token came from. A pane token is random, is +//! minted per slot, and is put only into that pane's child environment, so a +//! process able to quote one is a process running inside that pane. The pane's +//! own occupant asking for a watcher is a different thing from a plugin +//! enumerating the session, and only the first is allowed here — nothing in this +//! file looks at a list of panes. +//! +//! Still not authority by itself: the operator's config switch has to be on, and +//! a pane already spoken for is not taken away from the plugin that has it. + +use super::guard::{Approved, PaneFacts}; +use super::guard_refusal::Refused; +use crate::backend::PaneToken; + +/// Decide one [`PluginCommand::WatchPane`](super::protocol::PluginCommand). +/// +/// Takes no clock and charges no budget. Being given a pane is not something +/// done *to* the pane — it changes who is told about it, and every act that +/// follows is charged when it is asked for. Charging here would spend the very +/// allowance the recovery this unlocks is about to need. +pub(super) fn judge_watch( + token: &PaneToken, + facts: Option<&PaneFacts>, +) -> Result { + let Some(facts) = facts else { + // Overwhelmingly the ordinary case: a token from another nightcrow + // session's panes, reaching this plugin because both are the same user. + return Err(Refused::UnknownPane { + token: token.clone(), + }); + }; + if !facts.may_watch_on_signal { + return Err(Refused::WatchNotAllowed { + pane: facts.pane, + token: token.clone(), + }); + } + if facts.watched_by_another { + // One pane, one watcher. Two plugins driving the same keyboard would + // interleave their recoveries, and the second would be acting on a pane + // whose state the first is changing underneath it. + return Err(Refused::PaneWatchedByAnother { pane: facts.pane }); + } + if !facts.alive { + // Nothing left to watch: the slot may still exist, but the process that + // proved what the pane was running has gone, and the only recovery a + // pane taken on this way can get is typed into a live process. + return Err(Refused::PaneNotRunning { pane: facts.pane }); + } + // Deliberately allowed when this plugin already has the pane. That happens + // when an opted-in pane was handed over but its occupant could not be + // recognised from the command line, and asking again is how the plugin gets + // the `PaneOpened` it needs to try once more. + Ok(Approved::WatchPane { pane: facts.pane }) +} diff --git a/src/plugin/mod.rs b/src/plugin/mod.rs index aa08726..8f7eb3c 100644 --- a/src/plugin/mod.rs +++ b/src/plugin/mod.rs @@ -6,6 +6,12 @@ //! pane identity it names — before anything acts on it. This module is types //! and parsing only: no IO, no process spawning, no threads. //! +//! Which panes a plugin may see is settled here too. It is normally the +//! operator's list, and a plugin can add to it only by quoting a pane's own +//! token back — something only a process inside that pane can know — and only +//! where the operator turned that on. A plugin is never handed a list of panes +//! to choose from. +//! //! The contract is deliberately provider-agnostic. It speaks of panes, output, //! idleness and relaunching, and never of any particular tool; which program a //! pane runs stays the host's knowledge, which is what keeps @@ -23,6 +29,7 @@ pub mod registry; mod guard_budget; mod guard_refusal; mod guard_text; +mod guard_watch; mod host_pump; pub use guard::{Approved, Guard, PaneFacts}; diff --git a/src/plugin/protocol.rs b/src/plugin/protocol.rs index 558a261..3b59f1c 100644 --- a/src/plugin/protocol.rs +++ b/src/plugin/protocol.rs @@ -17,7 +17,12 @@ use serde::{Deserialize, Serialize}; /// Bumped when a field changes meaning. A plugin built against a version the /// host does not speak is refused rather than half-understood. -pub const PROTOCOL_VERSION: u32 = 1; +/// +/// 2 added [`PluginCommand::WatchPane`]. A version-1 plugin cannot ask for a +/// pane it was never named by, so an old build talking to this host would be +/// silently less capable rather than wrong — the bump is what makes that +/// difference loud instead. +pub const PROTOCOL_VERSION: u32 = 2; /// Longest line the host will read from a plugin. /// @@ -128,6 +133,15 @@ pub enum PluginCommand { deadline_epoch: Option, attempt: u32, }, + /// Ask to be given the pane this token names, so its events start arriving. + /// + /// The token is the whole of the argument: it is minted per pane and reaches + /// only that pane's child processes, so a plugin can present one merely by + /// having been told it from inside the pane. Carries no generation — a + /// plugin asking for a pane it has never been told about cannot know which + /// spawn it is looking at, and the [`PluginEvent::PaneOpened`] the host + /// answers with is what says. + WatchPane { v: u32, token: PaneToken }, /// A line for the host's log. Log { v: u32, @@ -152,6 +166,7 @@ impl PluginCommand { Self::SendInput { v, .. } | Self::Relaunch { v, .. } | Self::Status { v, .. } + | Self::WatchPane { v, .. } | Self::Log { v, .. } => *v, } } @@ -163,18 +178,23 @@ impl PluginCommand { match self { Self::SendInput { token, .. } | Self::Relaunch { token, .. } - | Self::Status { token, .. } => Some(token), + | Self::Status { token, .. } + | Self::WatchPane { token, .. } => Some(token), Self::Log { .. } => None, } } /// Which spawn of the slot this addresses, paired with [`Self::token`]. + /// + /// `None` for [`Self::WatchPane`] as well as [`Self::Log`]: naming a slot + /// and naming a spawn within it are separate claims, and that command makes + /// only the first. pub fn generation(&self) -> Option { match self { Self::SendInput { generation, .. } | Self::Relaunch { generation, .. } | Self::Status { generation, .. } => Some(*generation), - Self::Log { .. } => None, + Self::WatchPane { .. } | Self::Log { .. } => None, } } @@ -243,5 +263,5 @@ pub fn decode_command(line: &str) -> Result { } #[cfg(test)] -#[path = "protocol_tests.rs"] +#[path = "protocol_tests/mod.rs"] mod tests; diff --git a/src/plugin/protocol_tests/bounds.rs b/src/plugin/protocol_tests/bounds.rs new file mode 100644 index 0000000..59ad9d7 --- /dev/null +++ b/src/plugin/protocol_tests/bounds.rs @@ -0,0 +1,106 @@ +//! What `decode_command` refuses, and where each limit sits. +//! +//! Split from the shape tests beside it: those pin the wire's *names*, these pin +//! the wire's *edges*. A plugin is untrusted input on a stream with no length +//! prefix, so every one of these is a bound the host would otherwise not have. + +use super::{GENERATION, TOKEN, token}; +use crate::plugin::protocol::{ + MAX_INPUT_BYTES, MAX_LINE_BYTES, PROTOCOL_VERSION, PluginCommand, decode_command, is_blank_line, +}; + +#[test] +fn a_command_from_another_protocol_version_is_refused_and_both_versions_named() { + let line = format!( + r#"{{"cmd":"log","v":{},"level":"info","message":"hi"}}"#, + PROTOCOL_VERSION + 1 + ); + let err = decode_command(&line).expect_err("refused"); + let message = err.to_string(); + assert!( + message.contains(&(PROTOCOL_VERSION + 1).to_string()) + && message.contains(&PROTOCOL_VERSION.to_string()), + "message names only one version: {message}" + ); +} + +#[test] +fn a_line_over_the_length_limit_is_refused_before_it_is_parsed() { + // Refused on length alone, so nothing here depends on the payload being + // valid JSON — an unbounded line must not become an unbounded parse. + let line = "x".repeat(MAX_LINE_BYTES + 1); + let message = decode_command(&line).expect_err("refused").to_string(); + assert!(message.contains(&MAX_LINE_BYTES.to_string()), "{message}"); +} + +#[test] +fn a_line_at_the_length_limit_is_parsed() { + let padding = "p".repeat(MAX_INPUT_BYTES); + let line = format!(r#"{{"cmd":"log","v":2,"level":"debug","message":"{padding}"}}"#); + assert!(line.len() <= MAX_LINE_BYTES); + assert!(decode_command(&line).is_ok()); +} + +#[test] +fn an_unknown_cmd_is_refused_rather_than_guessed() { + let message = decode_command(r#"{"cmd":"drop_everything","v":2}"#) + .expect_err("refused") + .to_string(); + assert!(message.contains("drop_everything"), "{message}"); + assert!(message.contains("cmd"), "{message}"); +} + +#[test] +fn a_command_missing_a_required_field_is_refused() { + assert!(decode_command(r#"{"cmd":"send_input","v":2}"#).is_err()); +} + +#[test] +fn a_watch_pane_command_from_the_previous_protocol_version_is_refused() { + // The command did not exist at version 1, so a build that speaks 1 cannot + // have sent it — accepting one would be honouring a claim about a contract + // the sender never agreed to. + let line = format!(r#"{{"cmd":"watch_pane","v":1,"token":"{TOKEN}"}}"#); + assert!(decode_command(&line).is_err()); +} + +#[test] +fn send_input_data_over_the_input_limit_is_refused() { + let command = PluginCommand::SendInput { + v: PROTOCOL_VERSION, + token: token(), + generation: GENERATION, + data: "y".repeat(MAX_INPUT_BYTES + 1), + }; + let message = command.validate().expect_err("refused").to_string(); + assert!(message.contains(&MAX_INPUT_BYTES.to_string()), "{message}"); + + let line = serde_json::to_string(&command).unwrap(); + assert!( + line.len() <= MAX_LINE_BYTES, + "the length cap would mask this" + ); + assert!(decode_command(&line).is_err()); +} + +#[test] +fn send_input_data_at_the_input_limit_is_accepted() { + let command = PluginCommand::SendInput { + v: PROTOCOL_VERSION, + token: token(), + generation: GENERATION, + data: "y".repeat(MAX_INPUT_BYTES), + }; + assert!(command.validate().is_ok()); +} + +#[test] +fn a_blank_line_is_recognised_as_blank_and_carries_no_command() { + for line in ["", " ", "\t", "\r\n"] { + assert!(is_blank_line(line), "not seen as blank: {line:?}"); + assert!(decode_command(line).is_err()); + } + assert!(!is_blank_line( + r#"{"cmd":"log","v":2,"level":"info","message":"x"}"# + )); +} diff --git a/src/plugin/protocol_tests.rs b/src/plugin/protocol_tests/mod.rs similarity index 63% rename from src/plugin/protocol_tests.rs rename to src/plugin/protocol_tests/mod.rs index f32bd98..4a974be 100644 --- a/src/plugin/protocol_tests.rs +++ b/src/plugin/protocol_tests/mod.rs @@ -1,9 +1,11 @@ -use super::{ - LogLevel, MAX_INPUT_BYTES, MAX_LINE_BYTES, PROTOCOL_VERSION, PluginCommand, PluginEvent, - decode_command, encode_event, is_blank_line, -}; +//! The wire contract, and the fixtures both halves of it share: this file pins +//! the shapes and names, `bounds` beside it pins what is refused. + +use super::{LogLevel, PROTOCOL_VERSION, PluginCommand, PluginEvent, decode_command, encode_event}; use crate::backend::{PaneGeneration, PaneToken}; +mod bounds; + const TOKEN: &str = "0123456789abcdef0123456789abcdef"; const GENERATION: PaneGeneration = 2; @@ -77,6 +79,10 @@ fn every_command() -> Vec { deadline_epoch: Some(1_700_000_000), attempt: 1, }, + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token(), + }, PluginCommand::Log { v: PROTOCOL_VERSION, level: LogLevel::Warn, @@ -140,7 +146,7 @@ fn an_events_json_shape_is_the_wire_contract() { assert_eq!( encode_event(&event).unwrap(), format!( - r#"{{"event":"pane_idle","v":1,"token":"{TOKEN}","generation":2,"idle_ms":30000}}"# + r#"{{"event":"pane_idle","v":2,"token":"{TOKEN}","generation":2,"idle_ms":30000}}"# ) ); } @@ -155,7 +161,7 @@ fn a_commands_json_shape_is_the_wire_contract() { }; assert_eq!( serde_json::to_string(&command).unwrap(), - format!(r#"{{"cmd":"send_input","v":1,"token":"{TOKEN}","generation":2,"data":"go"}}"#) + format!(r#"{{"cmd":"send_input","v":2,"token":"{TOKEN}","generation":2,"data":"go"}}"#) ); } @@ -173,93 +179,6 @@ fn a_log_levels_json_shape_is_lowercase() { ); } -#[test] -fn a_command_from_another_protocol_version_is_refused_and_both_versions_named() { - let line = format!( - r#"{{"cmd":"log","v":{},"level":"info","message":"hi"}}"#, - PROTOCOL_VERSION + 1 - ); - let err = decode_command(&line).expect_err("refused"); - let message = err.to_string(); - assert!( - message.contains(&(PROTOCOL_VERSION + 1).to_string()) - && message.contains(&PROTOCOL_VERSION.to_string()), - "message names only one version: {message}" - ); -} - -#[test] -fn a_line_over_the_length_limit_is_refused_before_it_is_parsed() { - // Refused on length alone, so nothing here depends on the payload being - // valid JSON — an unbounded line must not become an unbounded parse. - let line = "x".repeat(MAX_LINE_BYTES + 1); - let message = decode_command(&line).expect_err("refused").to_string(); - assert!(message.contains(&MAX_LINE_BYTES.to_string()), "{message}"); -} - -#[test] -fn a_line_at_the_length_limit_is_parsed() { - let padding = "p".repeat(MAX_INPUT_BYTES); - let line = format!(r#"{{"cmd":"log","v":1,"level":"debug","message":"{padding}"}}"#); - assert!(line.len() <= MAX_LINE_BYTES); - assert!(decode_command(&line).is_ok()); -} - -#[test] -fn an_unknown_cmd_is_refused_rather_than_guessed() { - let message = decode_command(r#"{"cmd":"drop_everything","v":1}"#) - .expect_err("refused") - .to_string(); - assert!(message.contains("drop_everything"), "{message}"); - assert!(message.contains("cmd"), "{message}"); -} - -#[test] -fn a_command_missing_a_required_field_is_refused() { - assert!(decode_command(r#"{"cmd":"send_input","v":1}"#).is_err()); -} - -#[test] -fn send_input_data_over_the_input_limit_is_refused() { - let command = PluginCommand::SendInput { - v: PROTOCOL_VERSION, - token: token(), - generation: GENERATION, - data: "y".repeat(MAX_INPUT_BYTES + 1), - }; - let message = command.validate().expect_err("refused").to_string(); - assert!(message.contains(&MAX_INPUT_BYTES.to_string()), "{message}"); - - let line = serde_json::to_string(&command).unwrap(); - assert!( - line.len() <= MAX_LINE_BYTES, - "the length cap would mask this" - ); - assert!(decode_command(&line).is_err()); -} - -#[test] -fn send_input_data_at_the_input_limit_is_accepted() { - let command = PluginCommand::SendInput { - v: PROTOCOL_VERSION, - token: token(), - generation: GENERATION, - data: "y".repeat(MAX_INPUT_BYTES), - }; - assert!(command.validate().is_ok()); -} - -#[test] -fn a_blank_line_is_recognised_as_blank_and_carries_no_command() { - for line in ["", " ", "\t", "\r\n"] { - assert!(is_blank_line(line), "not seen as blank: {line:?}"); - assert!(decode_command(line).is_err()); - } - assert!(!is_blank_line( - r#"{"cmd":"log","v":1,"level":"info","message":"x"}"# - )); -} - #[test] fn only_a_pane_scoped_command_reports_an_identity() { for command in &every_command() { @@ -268,6 +187,13 @@ fn only_a_pane_scoped_command_reports_an_identity() { assert_eq!(command.token(), None); assert_eq!(command.generation(), None); } + // Names a slot but not a spawn within it: the plugin is asking about + // a pane the host has never described to it, so there is no + // generation it could honestly claim. + PluginCommand::WatchPane { .. } => { + assert_eq!(command.token(), Some(&token())); + assert_eq!(command.generation(), None); + } _ => { assert_eq!(command.token(), Some(&token())); assert_eq!(command.generation(), Some(GENERATION)); @@ -275,3 +201,17 @@ fn only_a_pane_scoped_command_reports_an_identity() { } } } + +#[test] +fn a_watch_pane_commands_json_shape_is_the_wire_contract() { + // Pinned literally, like the events above: a plugin that cannot be rebuilt + // with this host writes this exact line, and there is no generation in it. + let command = PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token(), + }; + assert_eq!( + serde_json::to_string(&command).unwrap(), + format!(r#"{{"cmd":"watch_pane","v":2,"token":"{TOKEN}"}}"#) + ); +} diff --git a/src/plugin/registry.rs b/src/plugin/registry.rs index 12964b6..4fbab8c 100644 --- a/src/plugin/registry.rs +++ b/src/plugin/registry.rs @@ -1,8 +1,9 @@ //! The on-disk set of installed plugin executables (`~/.nightcrow/plugins`). //! //! Installing a binary here does not switch it on: the host only ever launches -//! a plugin that `config.toml` declares in a `[[plugin]]` table *and* that a -//! `[[startup_command]]` opted a pane into by name. That edit is left to the +//! a plugin that `config.toml` declares in a `[[plugin]]` table *and* that some +//! pane can reach — either a `[[startup_command]]` opted one in by name, or +//! `watch_on_signal` lets a provider signal name one. That edit is left to the //! user on purpose — a plugin can drive a pane's terminal, so the file that //! grants it that has to be one a person read. //! diff --git a/src/web/viewer/terminal/hub_events.rs b/src/web/viewer/terminal/hub_events.rs index a609e3a..e7e3abc 100644 --- a/src/web/viewer/terminal/hub_events.rs +++ b/src/web/viewer/terminal/hub_events.rs @@ -186,6 +186,11 @@ impl Plugins { // one plugin must not be able to act through another's // opt-in. opted_in: self.owners.get(&pane).is_some_and(|name| name == plugin), + // The same lookup read the other way, which is a different + // question: an unclaimed pane may be asked for, one somebody + // else claimed may not. + watched_by_another: self.owners.get(&pane).is_some_and(|name| name != plugin), + may_watch_on_signal: self.watch_on_signal.contains(plugin), alive: backend.is_process_alive(pane), idle: slot.idle_for(now), launch_command: slot.launch.command.clone(), diff --git a/src/web/viewer/terminal/hub_plugins.rs b/src/web/viewer/terminal/hub_plugins.rs index bdafdda..6d7b304 100644 --- a/src/web/viewer/terminal/hub_plugins.rs +++ b/src/web/viewer/terminal/hub_plugins.rs @@ -6,10 +6,13 @@ //! command queue the worker already drains, and the only way out is //! [`crate::plugin::Guard`]. //! -//! A pane appears here **only** if its `[[startup_command]]` named a plugin by -//! hand. A pane a client opened has no association and never gains one, which is -//! what makes "an arbitrary shell is never plugin-controlled" a property of the -//! code rather than a promise. +//! A pane appears here only two ways: its `[[startup_command]]` named a plugin +//! by hand, or a plugin asked for it by quoting the pane's own token and the +//! guard allowed it (see `plugin::guard_watch`). Neither can be reached +//! by a plugin enumerating panes, because nothing ever tells a plugin what panes +//! exist — which is what keeps "an arbitrary shell is never plugin-controlled" a +//! property of the code. A shell stays untouched by doing what a shell does: +//! saying nothing to any plugin. use super::hub_helpers::PaneState; use crate::backend::{PaneId, PtyBackend}; @@ -88,6 +91,11 @@ pub(super) struct Plugins { pub(super) owners: HashMap, /// Each plugin's `allowed_resume_flags`, as the guard needs them. pub(super) allowed_flags: HashMap>, + /// The plugins whose config set `watch_on_signal`: those the operator allowed + /// to be given a pane they were never named by. Held as the set of names + /// rather than looked up in the config list, so the judgement reads it as a + /// hash probe on the same footing as every other fact it gathers. + pub(super) watch_on_signal: HashSet, pub(super) guard: Guard, pub(super) pending: HashMap, /// Panes whose plugin has already been told about the current quiet period, @@ -98,13 +106,17 @@ pub(super) struct Plugins { } impl Plugins { - /// Launch a host for every plugin that is enabled *and* that some startup - /// pane opted into. + /// Launch a host for every plugin that is enabled *and* has some pane it + /// could be given. /// /// Both conditions, because a host with no pane to watch is a child process - /// that can never be given anything to do. A plugin that will not launch is - /// logged and left out: its panes then behave exactly like unwatched ones, - /// so a broken plugin costs the operator a warning rather than a terminal. + /// that can never be given anything to do. `watch_on_signal` is the second + /// way to satisfy the first: such a plugin's panes are the ones that will + /// speak to it, so it has to be running *before* any of them does — waiting + /// for an opt-in that will never come would make the switch mean nothing. + /// A plugin that will not launch is logged and left out: its panes then + /// behave exactly like unwatched ones, so a broken plugin costs the operator + /// a warning rather than a terminal. pub(super) fn start(cwd: &str, configs: &[PluginConfig], startup: &[StartupCommand]) -> Self { let dir = crate::plugin::registry::default_plugins_dir() .inspect_err(|error| { @@ -113,16 +125,20 @@ impl Plugins { .ok(); let mut hosts = HashMap::new(); let mut allowed_flags = HashMap::new(); + let mut watch_on_signal = HashSet::new(); for cfg in configs { let opted_in = startup .iter() .any(|sc| sc.plugin.as_deref() == Some(cfg.name.as_str())); - if !cfg.enabled || !opted_in { + if !cfg.enabled || !(opted_in || cfg.watch_on_signal) { continue; } match PluginHost::spawn(cfg, dir.as_deref()) { Ok(host) => { allowed_flags.insert(cfg.name.clone(), cfg.allowed_resume_flags.clone()); + if cfg.watch_on_signal { + watch_on_signal.insert(cfg.name.clone()); + } hosts.insert(cfg.name.clone(), host); } Err(error) => tracing::warn!( @@ -136,6 +152,7 @@ impl Plugins { hosts, owners: HashMap::new(), allowed_flags, + watch_on_signal, guard: Guard::new(PANE_IDLE_THRESHOLD, RateLimits::default()), pending: HashMap::new(), idle_announced: HashSet::new(), diff --git a/src/web/viewer/terminal/hub_recovery.rs b/src/web/viewer/terminal/hub_recovery.rs index 189f5c4..c43b3c1 100644 --- a/src/web/viewer/terminal/hub_recovery.rs +++ b/src/web/viewer/terminal/hub_recovery.rs @@ -12,6 +12,20 @@ use super::hub_plugins::Plugins; use crate::backend::{PaneId, PtyBackend, TerminalBackend}; use std::time::Instant; +/// Whether `pane`'s process could be put back if it ended. +/// +/// A relaunch reproduces the pane's original invocation, so a pane the host +/// launched no command in has nothing to reproduce — which is exactly the pane a +/// plugin is given when its occupant asks to be watched. The guard refuses such a +/// relaunch outright, so the hold that exists solely to make one possible must not +/// be taken out for it either: that hold lasts days, and it would be days spent +/// keeping a shell's slot alive for a request that can never be granted. +pub(super) fn is_relaunchable(backend: &PtyBackend, pane: PaneId) -> bool { + backend + .slot(pane) + .is_some_and(|slot| slot.launch.command.is_some()) +} + /// The `state` the hub itself sends when a pane's recovery is over without /// having succeeded — cancelled by a person, or given up on when the hold ran /// out. Every client reads it as "stop showing a deadline for this pane". @@ -57,7 +71,8 @@ impl TerminalHub { /// For a pane no plugin watches this is the long-standing path: destroy it /// and tell everyone. For a watched one the slot has to survive, because its /// token is the only thing a relaunch can reuse — so the process alone is let - /// go and the slot is held until the plugin acts or the window closes. + /// go and the slot is held until the plugin acts or the window closes. Unless + /// there is nothing to put back: see [`is_relaunchable`]. pub(super) fn pane_exited( &self, backend: &mut PtyBackend, @@ -71,14 +86,14 @@ impl TerminalHub { } // Where it sat, read before the removal below takes it out of the order. match self.pane_spot(pane) { - Some(spot) => { + Some(spot) if is_relaunchable(backend, pane) => { backend.release_process(pane); plugins.hold_for_relaunch(pane, spot, Instant::now()); plugins.pane_exited(backend, pane); } - // Not in the client-visible order, so there is nowhere to put a - // replacement and no reason to keep the slot alive for one. - None => { + // Nowhere to put a replacement, or nothing to put there: either way + // there is no reason to keep the slot alive for one. + _ => { plugins.pane_closed(backend, pane); plugins.forget(backend, pane); backend.destroy_pane(pane); diff --git a/src/web/viewer/terminal/hub_relaunch.rs b/src/web/viewer/terminal/hub_relaunch.rs index a5b20a7..199edf9 100644 --- a/src/web/viewer/terminal/hub_relaunch.rs +++ b/src/web/viewer/terminal/hub_relaunch.rs @@ -79,12 +79,52 @@ impl TerminalHub { attempt, ); } + Ok(Approved::WatchPane { pane }) => { + self.watch_pane_for_plugin(backend, plugins, &plugin, pane) + } Ok(Approved::Log { level, message }) => log_plugin_line(&plugin, level, &message), Err(refused) => log_refusal(&plugin, &refused), } } } + /// Hand `pane` to `plugin` and announce it, so the plugin can start from the + /// same `PaneOpened` a configured pane begins with. + /// + /// Announcing is the whole point of the request: the association alone tells + /// the plugin nothing, and only a `PaneOpened` carries the generation every + /// later command has to name. The pane's history is not replayed — output + /// events carry fresh bytes only — so a plugin taken on mid-session sees the + /// pane from here forward, which is exactly what the signal that brought it + /// here is about. + fn watch_pane_for_plugin( + &self, + backend: &PtyBackend, + plugins: &mut Plugins, + plugin: &str, + pane: PaneId, + ) { + // The pane's name as the clients know it, so a plugin sees the same title + // whichever way it was given the pane. A client-opened pane has none. + let title = self.pane_spot(pane).and_then(|spot| spot.title); + if !plugins.adopt(pane, plugin) { + // Only reachable if the plugin's host died between the command being + // queued and this tick, since a dead host produces no commands. + tracing::debug!( + plugin = %plugin, + pane, + "viewer: nothing to hand a pane to; the plugin's host is gone" + ); + return; + } + tracing::info!( + plugin = %plugin, + pane, + "viewer: a plugin was given a pane by the token something inside it quoted" + ); + plugins.pane_opened(backend, pane, title.as_deref()); + } + /// Put a process back into the slot a plugin was holding. /// /// Only for a pane the hub is actually holding: that hold is the proof the @@ -214,9 +254,10 @@ fn log_plugin_line(plugin: &str, level: LogLevel, message: &str) { /// Log a refusal at the level that says whether anyone should look into it. /// /// A plugin decides asynchronously, so being late is ordinary traffic rather -/// than a fault: the pane moved on, or is not quiet yet. The rest mean the -/// plugin asked for something it was never allowed — a pane that is not its, an -/// oversized or control-laden payload, a flag the config does not list, or more +/// than a fault: the pane moved on, is not quiet yet, or was claimed by another +/// plugin first. The rest mean the plugin asked for something it was never +/// allowed — a pane that is not its, an oversized or control-laden payload, a +/// flag the config does not list, a bare shell it wanted to relaunch, or more /// attempts than the budget allows — and that is worth an operator's attention. /// Matched exhaustively on purpose, so a new refusal has to be classified rather /// than defaulting to silence. @@ -226,11 +267,14 @@ fn log_refusal(plugin: &str, refused: &Refused) { | Refused::StaleGeneration { .. } | Refused::PaneNotRunning { .. } | Refused::PaneBusy { .. } - | Refused::PaneStillRunning { .. } => true, + | Refused::PaneStillRunning { .. } + | Refused::PaneWatchedByAnother { .. } => true, Refused::NotOptedIn { .. } | Refused::InputTooLarge { .. } | Refused::ControlCharacter { .. } + | Refused::NoLaunchCommand { .. } | Refused::ResumeArgsRejected { .. } + | Refused::WatchNotAllowed { .. } | Refused::RateLimited { .. } => false, }; if ordinary { diff --git a/src/web/viewer/terminal/tests/mod.rs b/src/web/viewer/terminal/tests/mod.rs index ebc3248..e28cd41 100644 --- a/src/web/viewer/terminal/tests/mod.rs +++ b/src/web/viewer/terminal/tests/mod.rs @@ -4,6 +4,7 @@ mod behavior; mod plugin_rules; mod plugin_slots; +mod plugin_watch; mod plugins; mod recovery; mod scrollback_depth; diff --git a/src/web/viewer/terminal/tests/plugin_watch.rs b/src/web/viewer/terminal/tests/plugin_watch.rs new file mode 100644 index 0000000..dce3c05 --- /dev/null +++ b/src/web/viewer/terminal/tests/plugin_watch.rs @@ -0,0 +1,194 @@ +//! Being given a pane nobody opted in, driven against a real `PtyBackend`. +//! +//! The half that cannot be tested here is the token's journey: it reaches a +//! plugin through a provider's own hook, inside a pane, over a socket this crate +//! does not own. What *is* pinned here is everything the host decides once a token +//! comes back — which pane it names, whether the operator allowed it, and what +//! being given the pane does and does not entitle a plugin to. + +use super::plugin_rules::{COLS, LONG_RUNNING, PLUGIN, ROWS, opt_in, token_of}; +use super::plugins::{fixture, recorder}; +use crate::backend::{PaneId, PaneToken, PtyBackend, TerminalBackend}; +use crate::config::PluginConfig; +use crate::plugin::protocol::{PROTOCOL_VERSION, PluginCommand}; +use crate::plugin::{Approved, Refused}; +use crate::web::viewer::terminal::hub_plugins::Plugins; +use crate::web::viewer::terminal::hub_recovery::is_relaunchable; +use std::path::Path; +use std::time::Instant; + +/// The same recording plugin, with the operator's switch on. +fn signal_watcher(name: &str, log: &Path) -> PluginConfig { + PluginConfig { + watch_on_signal: true, + ..recorder(name, log) + } +} + +fn watch(token: &PaneToken) -> PluginCommand { + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token.clone(), + } +} + +/// A pane a client opened: no title, and no command of its own. +fn bare_pane(backend: &mut PtyBackend) -> PaneId { + backend + .open_pane(ROWS, COLS, None) + .expect("open a bare pane") +} + +#[test] +fn a_plugin_that_watches_on_signal_is_launched_with_no_pane_opted_in() { + // The panes it exists for cannot opt in, so it has to be running before any of + // them says anything. `adopt` succeeding is how "it has a live host" is read. + let f = fixture(); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + + assert!(plugins.adopt(1, PLUGIN)); +} + +#[test] +fn a_watch_request_bearing_a_live_panes_token_is_approved_and_the_pane_handed_over() { + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + let pane = backend + .open_pane(ROWS, COLS, Some(LONG_RUNNING)) + .expect("open a pane"); + let token = token_of(&backend, pane); + assert_eq!(plugins.owner(pane), None, "nothing handed it over yet"); + + let approved = plugins + .judge(PLUGIN, watch(&token), &backend, Instant::now()) + .expect("a live pane's own token is the evidence"); + + assert_eq!(approved, Approved::WatchPane { pane }); + // The association is the caller's to record, which is what the hub does next. + assert!(plugins.adopt(pane, PLUGIN)); + assert_eq!(plugins.owner(pane), Some(PLUGIN)); + backend.destroy_pane(pane); +} + +#[test] +fn a_watch_request_from_a_plugin_without_the_switch_is_refused() { + // The identical request, refused purely because the operator did not ask for + // it: this is the only thing standing between a signal and a new association. + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[recorder(PLUGIN, &f.log)], &[opt_in()]); + let pane = bare_pane(&mut backend); + let token = token_of(&backend, pane); + + let refused = plugins + .judge(PLUGIN, watch(&token), &backend, Instant::now()) + .expect_err("a plugin without the switch may not be given a pane"); + + assert!( + matches!(refused, Refused::WatchNotAllowed { .. }), + "{refused}" + ); + assert_eq!(plugins.owner(pane), None); + backend.destroy_pane(pane); +} + +#[test] +fn a_watch_request_for_a_pane_another_plugin_holds_is_refused() { + const OTHER: &str = "other"; + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start( + &f.cwd(), + &[ + signal_watcher(PLUGIN, &f.log), + signal_watcher(OTHER, &f.log), + ], + &[], + ); + let pane = bare_pane(&mut backend); + let token = token_of(&backend, pane); + assert!(plugins.adopt(pane, OTHER)); + + let refused = plugins + .judge(PLUGIN, watch(&token), &backend, Instant::now()) + .expect_err("one pane, one watcher"); + + assert!( + matches!(refused, Refused::PaneWatchedByAnother { .. }), + "{refused}" + ); + assert_eq!(plugins.owner(pane), Some(OTHER), "the holder keeps it"); + backend.destroy_pane(pane); +} + +#[test] +fn a_watch_request_for_a_token_this_host_never_minted_is_refused() { + // A helper from another nightcrow session on the same machine: the socket is + // per-user, so its tokens do reach us, and they must resolve to nothing. + let f = fixture(); + let backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + let stranger = PaneToken::new().expect("OS RNG"); + + let refused = plugins + .judge(PLUGIN, watch(&stranger), &backend, Instant::now()) + .expect_err("a token naming no pane of ours"); + + assert!(matches!(refused, Refused::UnknownPane { .. }), "{refused}"); +} + +#[test] +fn a_pane_handed_over_on_a_signal_can_never_be_relaunched() { + // The invariant that keeps late adoption from doing something worse than + // nothing: the pane is a shell, so a relaunch would restart the shell rather + // than the session the plugin was recovering. + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + let pane = bare_pane(&mut backend); + let token = token_of(&backend, pane); + assert!(plugins.adopt(pane, PLUGIN)); + // The exit the plugin would answer with a relaunch. + backend.release_process(pane); + + let refused = plugins + .judge( + PLUGIN, + PluginCommand::Relaunch { + v: PROTOCOL_VERSION, + token, + generation: 1, + resume_args: Vec::new(), + }, + &backend, + Instant::now(), + ) + .expect_err("a pane with no command of its own has nothing to relaunch"); + + assert!( + matches!(refused, Refused::NoLaunchCommand { .. }), + "{refused}" + ); + backend.retire_slot(pane); +} + +#[test] +fn an_exited_pane_with_no_command_is_not_worth_holding_a_slot_for() { + // The condition the hub branches on when a watched pane's process ends. A + // hold lasts days and exists only to make a relaunch possible, so a pane that + // can never be relaunched must take the closing path instead — otherwise a + // shell that exited would keep its slot for the whole window. + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let bare = bare_pane(&mut backend); + let launched = backend + .open_pane(ROWS, COLS, Some(LONG_RUNNING)) + .expect("open a pane"); + + assert!(!is_relaunchable(&backend, bare)); + assert!(is_relaunchable(&backend, launched)); + + backend.destroy_pane(bare); + backend.destroy_pane(launched); +} From aa3154a1afb99985582f45e437fa9dca7cac4e2f Mon Sep 17 00:00:00 2001 From: whackur Date: Fri, 31 Jul 2026 00:14:54 +0900 Subject: [PATCH 6/9] fix: keep the statusline the plugin displaced at install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude Code's statusLine is a single value, so installing took the user's over and printed our own line instead — the doc comment claiming otherwise was simply wrong. The displaced command was already recorded for uninstall; run it as well, with the bytes Claude Code sent us untouched so it sees what it would have seen. Any failure falls back to our line: a statusline that errors is worse than a plain one. --- plugins/nightcrow-recovery/src/helper.rs | 98 +++++---- .../nightcrow-recovery/src/helper_delegate.rs | 116 +++++++++++ .../src/helper_statusline.rs | 128 ++++++++++++ .../src/helper_statusline_tests.rs | 192 ++++++++++++++++++ .../nightcrow-recovery/src/helper_tests.rs | 89 +++++--- plugins/nightcrow-recovery/src/hooks.rs | 15 ++ plugins/nightcrow-recovery/src/main.rs | 3 +- 7 files changed, 572 insertions(+), 69 deletions(-) create mode 100644 plugins/nightcrow-recovery/src/helper_delegate.rs create mode 100644 plugins/nightcrow-recovery/src/helper_statusline.rs create mode 100644 plugins/nightcrow-recovery/src/helper_statusline_tests.rs diff --git a/plugins/nightcrow-recovery/src/helper.rs b/plugins/nightcrow-recovery/src/helper.rs index fd66b7c..05324eb 100644 --- a/plugins/nightcrow-recovery/src/helper.rs +++ b/plugins/nightcrow-recovery/src/helper.rs @@ -12,6 +12,11 @@ //! carries whatever else the provider decided to include. Only the fields the //! state machine actually reads cross the socket, so nothing else can be //! accidentally logged, buffered, or written down later. +//! +//! The one thing that leaves this process whole is the statusline payload handed +//! to the command we displaced (see [`status_line`]) — and that command was being +//! given the same bytes by Claude Code before this plugin was installed. We +//! narrow what we keep; we do not narrow what someone else was already told. use crate::ipc::{IpcMessage, send, socket_path}; use crate::protocol::PANE_TOKEN_ENV; @@ -19,6 +24,10 @@ use crate::provider::SignalKind; use serde_json::{Map, Value}; use std::io::Read; use std::process::ExitCode; +use std::time::Duration; + +#[path = "helper_statusline.rs"] +mod status_line; /// Most stdin a helper will read. /// @@ -35,16 +44,11 @@ const STOP_FAILURE_FIELDS: [&str; 3] = ["session_id", "error_type", "hook_event_ /// The only statusline field this plugin wants. const RATE_LIMITS_FIELD: &str = "rate_limits"; -/// Shown when the statusline payload carries no usage numbers — which is normal: -/// `rate_limits` is absent for accounts without a subscription window and before -/// the session's first response. -const STATUSLINE_FALLBACK: &str = "nightcrow: watching"; - /// Forward a `StopFailure` payload. Always succeeds from the caller's point of /// view; `StopFailure` ignores our exit code anyway, and a hook that fails /// loudly would be worse than one that does nothing. pub fn hook() -> ExitCode { - if let Some((token, payload)) = read_stdin_object().and_then(|body| { + if let Some((token, payload)) = parse_object(&read_stdin_bytes()).and_then(|body| { let token = pane_token()?; Some((token, pick(&body, &STOP_FAILURE_FIELDS))) }) { @@ -60,16 +64,15 @@ pub fn hook() -> ExitCode { ExitCode::SUCCESS } -/// Forward the statusline's `rate_limits` and print a line, so installing this -/// plugin does not cost the user their statusline. +/// Forward the statusline's `rate_limits` and print a line — the line the user's +/// own statusline command printed, whenever installing this plugin displaced one. +/// Claude Code's `statusLine` holds a single command, so the only way not to cost +/// the user their statusline is to run it from ours; see [`status_line`]. pub fn statusline() -> ExitCode { - let body = read_stdin_object(); - let rate_limits = body - .as_ref() - .and_then(|b| b.get(RATE_LIMITS_FIELD)) - .and_then(Value::as_object) - .cloned(); - if let (Some(token), Some(limits)) = (pane_token(), rate_limits.clone()) { + let raw = read_stdin_bytes(); + let displaced = status_line::displaced(); + let refresh = refresh(&raw, displaced.as_ref(), status_line::BUDGET); + if let (Some(token), Some(limits)) = (pane_token(), refresh.rate_limits) { let _ = send( &socket_path().unwrap_or_default(), &IpcMessage { @@ -79,30 +82,30 @@ pub fn statusline() -> ExitCode { }, ); } - println!("{}", render_statusline(rate_limits.as_ref())); + println!("{}", refresh.line); ExitCode::SUCCESS } -/// A short line built only from fields whose meaning is documented: the usage -/// percentage of each window the provider reported. -fn render_statusline(rate_limits: Option<&Map>) -> String { - let Some(limits) = rate_limits else { - return STATUSLINE_FALLBACK.to_string(); - }; - let mut parts = Vec::new(); - for (label, key) in [("5h", "five_hour"), ("7d", "seven_day")] { - if let Some(used) = limits - .get(key) - .and_then(|w| w.get("used_percentage")) - .and_then(Value::as_f64) - { - parts.push(format!("{label} {}%", used.round() as i64)); - } - } - if parts.is_empty() { - return STATUSLINE_FALLBACK.to_string(); - } - parts.join(" | ") +/// What one statusline refresh comes to: the usage numbers to forward, and the +/// line to print. +struct Refresh { + rate_limits: Option>, + line: String, +} + +/// Decide both halves of a refresh without touching the socket or stdout, so +/// every way a displaced command can disappoint us stays testable — and so the +/// usage numbers are read out of the payload before anything is delegated, which +/// is what keeps a misbehaving statusline command from costing us them. +fn refresh(raw: &[u8], displaced: Option<&Value>, budget: Duration) -> Refresh { + let rate_limits = parse_object(raw).and_then(rate_limits_of); + let line = status_line::line(displaced, raw, rate_limits.as_ref(), budget); + Refresh { rate_limits, line } +} + +/// The usage windows of a statusline payload, when it reported any. +fn rate_limits_of(body: Map) -> Option> { + body.get(RATE_LIMITS_FIELD)?.as_object().cloned() } /// The pane this helper belongs to, from the environment its provider inherited. @@ -114,13 +117,24 @@ fn pane_token() -> Option { .filter(|t| !t.trim().is_empty()) } -fn read_stdin_object() -> Option> { - let mut body = String::new(); - std::io::stdin() +/// Every byte the provider wrote, kept exactly as it wrote them. The statusline +/// helper hands these on to the command it displaced, and a re-serialised copy is +/// not the same thing: key order and number formatting are the provider's to +/// choose, and a command that was reading its input before we existed should not +/// find it rearranged now. A read that fails part-way keeps what did arrive — +/// unparseable, and treated as such below. +fn read_stdin_bytes() -> Vec { + let mut raw = Vec::new(); + let _ = std::io::stdin() .take(MAX_HELPER_STDIN_BYTES) - .read_to_string(&mut body) - .ok()?; - match serde_json::from_str::(&body) { + .read_to_end(&mut raw); + raw +} + +/// The payload as an object, when that is what it is. Anything else is not an +/// error here: there is simply nothing of ours to forward out of it. +fn parse_object(raw: &[u8]) -> Option> { + match serde_json::from_slice::(raw) { Ok(Value::Object(map)) => Some(map), _ => None, } diff --git a/plugins/nightcrow-recovery/src/helper_delegate.rs b/plugins/nightcrow-recovery/src/helper_delegate.rs new file mode 100644 index 0000000..28db083 --- /dev/null +++ b/plugins/nightcrow-recovery/src/helper_delegate.rs @@ -0,0 +1,116 @@ +//! Running a statusline command that is not ours, on a budget. +//! +//! Split out of `helper_statusline.rs` so that file decides *which* line gets +//! printed and this one is the process plumbing under it. Everything here is +//! written for a caller that must not be made to wait and must not be made to +//! fail: the child is bounded, killed when it overruns, reaped on every path, and +//! any disappointment comes back as `None`. + +use std::io::{Read, Write}; +use std::process::{Child, Command, Stdio}; +use std::sync::mpsc; +use std::time::{Duration, Instant}; + +/// A POSIX shell, resolved on `PATH`. Not `$SHELL`: an interactive shell would +/// read the user's rc files on every single refresh. +const SHELL: &str = "sh"; + +/// Most stdout to take from a displaced command. A statusline is one short line; +/// this only stops a runaway script from growing this process. +const MAX_DELEGATED_STDOUT_BYTES: u64 = 64 * 1024; + +/// How often to look for a child that has closed its stdout but not yet exited. +const EXIT_POLL: Duration = Duration::from_millis(2); + +/// Run `command` with `raw` on its stdin and bring back what it printed, or +/// `None` if it could not be started, did not end well, or overran `budget`. +/// +/// Through `sh -c`, not an argv we split ourselves: the provider documents that a +/// `statusLine` command "runs in a shell", and its own examples rely on it — a `~` +/// path, a `jq` pipeline, an inline `$(...)`. Re-splitting the string the user wrote +/// would quietly change what it means. This is the user's own configuration rather +/// than input from a stranger, but it is also not ours to reinterpret. +pub(super) fn capture(command: &str, raw: &[u8], budget: Duration) -> Option { + let deadline = Instant::now() + budget; + let mut child = Command::new(SHELL) + .arg("-c") + .arg(command) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + // Claude Code reads our stdout for the statusline, and a chatty script's + // stderr shares the terminal with it. Discarded, so a warning meant for a + // log cannot end up rendered as the statusline. + .stderr(Stdio::null()) + .spawn() + .ok()?; + + // Each pipe gets its own thread. Writing the payload first would deadlock + // against a command that answers before draining its input, and reading first + // would wedge a command that is still waiting for the rest of its input. + if let Some(mut stdin) = child.stdin.take() { + let payload = raw.to_vec(); + std::thread::spawn(move || { + // A command that ignores its input closes the pipe early. That is a + // choice it is allowed to make, not a failure of ours. + let _ = stdin.write_all(&payload); + }); + } + let Some(stdout) = child.stdout.take() else { + return abandon(child); + }; + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let mut captured = Vec::new(); + let _ = stdout + .take(MAX_DELEGATED_STDOUT_BYTES) + .read_to_end(&mut captured); + let _ = tx.send(captured); + }); + + let Ok(captured) = rx.recv_timeout(remaining(deadline)) else { + return abandon(child); + }; + if !exited_well(&mut child, deadline) { + return abandon(child); + } + // Output we cannot decode is output we cannot print; the caller has a line of + // its own for that. + let text = String::from_utf8(captured).ok()?; + // The child owns the content of the line; we own its framing, and the caller + // is the one that ends it with a newline. A command that printed nothing but + // whitespace did not render a statusline at all. + let printed = text.trim_end_matches(['\r', '\n']); + if printed.trim().is_empty() { + return None; + } + Some(printed.to_string()) +} + +/// Whether the child finished, and finished happily, before `deadline`. +/// +/// Polled rather than waited on: `wait` has no timeout, and a command that closes +/// its stdout and then sleeps must not get to hold a refresh open. Stdout is +/// already at EOF by the time this is called, so the first look nearly always +/// finds the child gone. +fn exited_well(child: &mut Child, deadline: Instant) -> bool { + loop { + match child.try_wait() { + Ok(Some(status)) => return status.success(), + Ok(None) if Instant::now() < deadline => std::thread::sleep(EXIT_POLL), + _ => return false, + } + } +} + +/// A child we are done waiting for. Killed so a wedged statusline command does not +/// outlive the refresh that started it, and reaped so it does not sit as a zombie +/// for whatever is left of this process's life. +fn abandon(mut child: Child) -> Option { + let _ = child.kill(); + let _ = child.wait(); + None +} + +fn remaining(deadline: Instant) -> Duration { + deadline.saturating_duration_since(Instant::now()) +} diff --git a/plugins/nightcrow-recovery/src/helper_statusline.rs b/plugins/nightcrow-recovery/src/helper_statusline.rs new file mode 100644 index 0000000..a432e01 --- /dev/null +++ b/plugins/nightcrow-recovery/src/helper_statusline.rs @@ -0,0 +1,128 @@ +//! The one line Claude Code renders, and who gets to write it. +//! +//! Installing this plugin necessarily takes the user's statusline away: +//! `statusLine` in `settings.json` holds one command, not a list, so ours replaces +//! whatever was there. Chaining is the only way to give it back — install recorded +//! the value it displaced in a sidecar, and every refresh runs that command with +//! the very bytes Claude Code sent us and prints what it printed. This plugin's own +//! two-number line ([`render_statusline`]) stands in only when there is nothing to +//! chain to. Running the command itself is [`delegate`]'s job. +//! +//! Nothing here fails upwards. A statusline that shows an error is worse than a +//! plain one, so a missing sidecar, a value we cannot execute, a spawn failure, a +//! non-zero exit, a wedged child and non-UTF-8 output all end in the same place: +//! our own line, printed as if no chaining had been attempted. + +use crate::hooks::{SettingsPaths, displaced_statusline, is_ours}; +use serde_json::{Map, Value}; +use std::time::Duration; + +#[path = "helper_delegate.rs"] +mod delegate; + +/// Shown when the statusline payload carries no usage numbers — which is normal: +/// `rate_limits` is absent for accounts without a subscription window and before +/// the session's first response. +const STATUSLINE_FALLBACK: &str = "nightcrow: watching"; + +/// How long a displaced statusline command may take before we give up on it. +/// +/// Claude Code documents no timeout for a statusline: it debounces updates at 300ms +/// and cancels an in-flight script when the next update arrives, so the provider is +/// already the one deciding we took too long. This bound is for the other direction +/// — a command that never returns must not make this process immortal, and our own +/// line has to get printed either way. Two seconds is many times that debounce and +/// generous even for the `git`-shelling scripts the provider's own guidance calls +/// slow, while keeping a wedged child's cost finite. It is also inside the five +/// seconds this plugin asks Claude Code to allow its hook, the most patience +/// anything here claims of the provider. +pub(super) const BUDGET: Duration = Duration::from_secs(2); + +const TYPE_KEY: &str = "type"; +const COMMAND_KEY: &str = "command"; +/// The only `type` of `statusLine` entry there is anything for us to run. +const COMMAND_TYPE: &str = "command"; + +/// The `statusLine` install displaced, when there is one to chain to. Every way +/// there can be nothing — no `HOME` to look under, no sidecar because we displaced +/// nothing, a sidecar we cannot read — reads the same from here. +pub(super) fn displaced() -> Option { + displaced_statusline(&SettingsPaths::discover().ok()?) +} + +/// The line to print for this refresh: the displaced command's, when there is one +/// that can produce it, and ours otherwise. +pub(super) fn line( + displaced: Option<&Value>, + raw: &[u8], + rate_limits: Option<&Map>, + budget: Duration, +) -> String { + delegated(displaced, raw, budget).unwrap_or_else(|| render_statusline(rate_limits)) +} + +fn delegated(displaced: Option<&Value>, raw: &[u8], budget: Duration) -> Option { + let command = command_of(displaced?)?; + // Our own command in the sidecar would be a chain that runs this binary from + // itself, and again from there. `is_ours` is the same substring test install and + // uninstall recognise our entries by, so what is refused here is exactly what + // those two already consider ours. + if is_ours(command) { + return None; + } + delegate::capture(command, raw, budget) +} + +/// The command string inside whatever Claude Code allowed as a `statusLine`. +/// +/// Install recorded that value verbatim, so this reads the shape the provider +/// documents and the one this plugin itself writes — an object with `type` and +/// `command` — and also accepts a bare string, which costs nothing and is the +/// obvious hand-written form. A value with some other `type` is a statusline we do +/// not know how to run, and guessing at it is worse than standing in for it; so is +/// `null`, which is what install records when it displaced nothing at all. +/// +/// The entry's other fields are Claude Code's to act on, not ours: `padding` and +/// `refreshInterval` describe how the provider treats a statusline, and the +/// provider is reading them off our entry now, not off this one. +fn command_of(value: &Value) -> Option<&str> { + let command = match value { + Value::String(command) => command.as_str(), + Value::Object(map) => { + let declared = map.get(TYPE_KEY).and_then(Value::as_str); + if declared.is_some_and(|kind| kind != COMMAND_TYPE) { + return None; + } + map.get(COMMAND_KEY)?.as_str()? + } + _ => return None, + }; + let command = command.trim(); + (!command.is_empty()).then_some(command) +} + +/// A short line built only from fields whose meaning is documented: the usage +/// percentage of each window the provider reported. +fn render_statusline(rate_limits: Option<&Map>) -> String { + let Some(limits) = rate_limits else { + return STATUSLINE_FALLBACK.to_string(); + }; + let mut parts = Vec::new(); + for (label, key) in [("5h", "five_hour"), ("7d", "seven_day")] { + if let Some(used) = limits + .get(key) + .and_then(|w| w.get("used_percentage")) + .and_then(Value::as_f64) + { + parts.push(format!("{label} {}%", used.round() as i64)); + } + } + if parts.is_empty() { + return STATUSLINE_FALLBACK.to_string(); + } + parts.join(" | ") +} + +#[cfg(test)] +#[path = "helper_statusline_tests.rs"] +mod tests; diff --git a/plugins/nightcrow-recovery/src/helper_statusline_tests.rs b/plugins/nightcrow-recovery/src/helper_statusline_tests.rs new file mode 100644 index 0000000..b1d362a --- /dev/null +++ b/plugins/nightcrow-recovery/src/helper_statusline_tests.rs @@ -0,0 +1,192 @@ +//! Who writes the line, and what happens when the displaced command will not. +//! +//! These tests really do spawn shells, which is the point of them: how a +//! statusline command behaves towards us — printing, failing, hanging, shouting on +//! stderr — is only worth pinning if a real one gets to do it. + +use super::*; +use std::time::Instant; + +/// A budget no test in here is meant to reach. Only the wedged case waits. +const ENOUGH: Duration = Duration::from_secs(5); + +/// The budget for a command that will never finish. Short enough not to be felt, +/// long enough that expiry is a decision rather than a lost race with `sh`. +const BRIEF: Duration = Duration::from_millis(150); + +/// What `render_statusline` makes of [`limits`], so `OURS` in an assertion reads +/// as "the chain was declined or came to nothing". +const OURS: &str = "5h 40%"; + +/// The shape this plugin writes into `statusLine`, and so the shape it records +/// when it displaces one. +fn entry(command: &str) -> Value { + serde_json::json!({ "type": "command", "command": command, "padding": 2 }) +} + +fn limits() -> Map { + match serde_json::json!({"five_hour": {"used_percentage": 40.0}}) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + } +} + +/// What a refresh prints when install recorded `displaced`, with usage numbers +/// always there to fall back on. +fn rendered(displaced: &Value, raw: &[u8], budget: Duration) -> String { + line(Some(displaced), raw, Some(&limits()), budget) +} + +#[test] +fn the_displaced_commands_own_line_becomes_our_line() { + let displaced = entry("echo 'hud | main | 12%'"); + + let printed = rendered(&displaced, b"{}", ENOUGH); + + assert_eq!(printed, "hud | main | 12%"); +} + +#[test] +fn the_bytes_claude_code_sent_reach_the_displaced_command_unchanged() { + // Key order, number formatting and string escapes are all the provider's, and + // a parsed and re-encoded copy would keep none of the three. + let raw = br#"{"zeta":1,"alpha":2.50,"big":1e3,"who":"a\/b"}"#; + + let printed = rendered(&entry("cat"), raw, ENOUGH); + + assert_eq!(printed.as_bytes(), raw); +} + +#[test] +fn a_displaced_statusline_recorded_as_a_bare_string_is_run_too() { + let displaced = Value::String("echo theirs".to_string()); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), "theirs"); +} + +#[test] +fn a_multi_line_displaced_statusline_keeps_its_own_line_breaks() { + let displaced = entry("printf 'top\\nbottom\\n'"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), "top\nbottom"); +} + +#[test] +fn with_nothing_displaced_our_own_line_is_printed() { + let numbers = limits(); + + assert_eq!(line(None, b"{}", Some(&numbers), ENOUGH), OURS); + // A JSON null is what install records when it displaced no statusline at all. + assert_eq!(rendered(&Value::Null, b"{}", ENOUGH), OURS); + assert_eq!(line(None, b"{}", None, ENOUGH), STATUSLINE_FALLBACK); +} + +#[test] +fn a_displaced_command_that_fails_falls_back_however_much_it_printed() { + let displaced = entry("echo half-a-line; exit 3"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), OURS); +} + +#[test] +fn a_displaced_command_that_cannot_be_run_falls_back() { + let displaced = entry("/nonexistent/statusline-c0ffee --now"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), OURS); +} + +#[test] +fn a_displaced_command_that_prints_nothing_usable_falls_back() { + for silent in ["true", "printf '\\n\\n'", "printf ' '"] { + let printed = rendered(&entry(silent), b"{}", ENOUGH); + + assert_eq!(printed, OURS, "{silent}"); + } +} + +#[test] +fn a_displaced_commands_stderr_never_reaches_the_statusline() { + let displaced = entry("echo noise >&2; echo theirs"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), "theirs"); +} + +#[test] +fn a_displaced_command_that_never_finishes_is_given_up_on_and_falls_back() { + let displaced = entry("sleep 30"); + let started = Instant::now(); + + let printed = rendered(&displaced, b"{}", BRIEF); + + assert_eq!(printed, OURS); + assert!( + started.elapsed() < Duration::from_secs(5), + "the refresh waited {:?} on a command it had given up on", + started.elapsed() + ); +} + +#[test] +fn our_own_command_is_never_run_from_our_own_statusline() { + // Both of these would print if they ran, and both carry the marker install + // and uninstall recognise us by, so neither may: a sidecar naming this binary + // would otherwise chain the statusline into itself. + for ours in [ + "echo nightcrow-recovery statusline", + "/opt/nightcrow/libexec/nightcrow-recovery statusline || echo theirs", + ] { + let printed = rendered(&entry(ours), b"{}", ENOUGH); + + assert_eq!(printed, OURS, "{ours}"); + } +} + +#[test] +fn a_displaced_value_we_cannot_execute_falls_back_rather_than_guessing() { + // The first is the case that matters: a future `type` may mean something + // entirely unlike a shell command, so its `command` is not ours to run. + for unrunnable in [ + serde_json::json!({"type": "some-future-kind", "command": "echo theirs"}), + serde_json::json!({"type": "command", "command": 42}), + serde_json::json!({"type": "command"}), + serde_json::json!({"command": " "}), + serde_json::json!([{"command": "echo theirs"}]), + serde_json::json!(7), + ] { + let printed = rendered(&unrunnable, b"{}", ENOUGH); + + assert_eq!(printed, OURS, "{unrunnable}"); + } +} + +#[test] +fn a_statusline_reports_the_usage_of_every_window_the_provider_gave() { + let limits = match serde_json::json!({ + "five_hour": {"used_percentage": 23.5, "resets_at": 1_767_225_600i64}, + "seven_day": {"used_percentage": 41.2, "resets_at": 1_767_657_600i64} + }) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + }; + assert_eq!(render_statusline(Some(&limits)), "5h 24% | 7d 41%"); +} + +#[test] +fn a_statusline_with_one_window_reports_only_that_window() { + let limits = match serde_json::json!({"seven_day": {"used_percentage": 8.0}}) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + }; + assert_eq!(render_statusline(Some(&limits)), "7d 8%"); +} + +#[test] +fn a_statusline_still_prints_a_line_when_the_provider_reported_no_windows() { + assert_eq!(render_statusline(None), STATUSLINE_FALLBACK); + assert_eq!(render_statusline(Some(&Map::new())), STATUSLINE_FALLBACK); + let unusable = match serde_json::json!({"five_hour": {"used_percentage": "lots"}}) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + }; + assert_eq!(render_statusline(Some(&unusable)), STATUSLINE_FALLBACK); +} diff --git a/plugins/nightcrow-recovery/src/helper_tests.rs b/plugins/nightcrow-recovery/src/helper_tests.rs index ab94279..e539f63 100644 --- a/plugins/nightcrow-recovery/src/helper_tests.rs +++ b/plugins/nightcrow-recovery/src/helper_tests.rs @@ -1,6 +1,8 @@ -//! The whitelisting and the statusline text. Reading stdin and connecting to the -//! socket are covered by `ipc_tests`; what matters here is that nothing outside -//! the whitelist can be forwarded, whatever a provider puts in its payload. +//! The whitelisting, and what one refresh decides. Reading stdin and connecting +//! to the socket are covered by `ipc_tests`; what matters here is that nothing +//! outside the whitelist can be forwarded, whatever a provider puts in its +//! payload, and that nothing a displaced statusline command does can lose the +//! usage numbers on the way. Which command gets to print is `helper_statusline`'s. use super::*; @@ -61,34 +63,69 @@ fn a_payload_missing_every_whitelisted_field_forwards_nothing() { assert!(pick(&Map::new(), &STOP_FAILURE_FIELDS).is_empty()); } +/// A statusline payload with usage numbers in it, as the bytes a provider would +/// have written them. +const STATUSLINE_BODY: &[u8] = + br#"{"session_id":"s","rate_limits":{"five_hour":{"used_percentage":40.0}}}"#; + +/// Only the command that hangs spends this, and no assertion below turns on +/// whether the others managed to print in time. +const BUDGET: Duration = Duration::from_millis(200); + +/// A generous budget, for the cases that are about what a command printed. +const ENOUGH: Duration = Duration::from_secs(5); + +fn statusline_entry(command: &str) -> Value { + serde_json::json!({ "type": "command", "command": command }) +} + +fn five_hour(refresh: &Refresh) -> Option<&Value> { + refresh.rate_limits.as_ref()?.get("five_hour") +} + #[test] -fn a_statusline_reports_the_usage_of_every_window_the_provider_gave() { - let limits = match serde_json::json!({ - "five_hour": {"used_percentage": 23.5, "resets_at": 1_767_225_600i64}, - "seven_day": {"used_percentage": 41.2, "resets_at": 1_767_657_600i64} - }) { - Value::Object(map) => map, - _ => unreachable!("the fixture is an object"), - }; - assert_eq!(render_statusline(Some(&limits)), "5h 24% | 7d 41%"); +fn a_refresh_forwards_the_usage_numbers_whatever_the_displaced_command_does() { + let expected = serde_json::json!({"used_percentage": 40.0}); + for displaced in [ + Value::Null, + statusline_entry("echo theirs"), + statusline_entry("exit 1"), + statusline_entry("sleep 30"), + statusline_entry("/x/nightcrow-recovery statusline"), + serde_json::json!({"type": "some-future-kind"}), + ] { + let refresh = refresh(STATUSLINE_BODY, Some(&displaced), BUDGET); + + let forwarded = five_hour(&refresh); + + assert_eq!(forwarded, Some(&expected), "lost them for {displaced}"); + } } #[test] -fn a_statusline_with_one_window_reports_only_that_window() { - let limits = match serde_json::json!({"seven_day": {"used_percentage": 8.0}}) { - Value::Object(map) => map, - _ => unreachable!("the fixture is an object"), - }; - assert_eq!(render_statusline(Some(&limits)), "7d 8%"); +fn a_refresh_prints_the_displaced_statuslines_line_rather_than_our_own() { + let displaced = statusline_entry("echo theirs"); + + let refresh = refresh(STATUSLINE_BODY, Some(&displaced), ENOUGH); + + assert_eq!(refresh.line, "theirs"); } #[test] -fn a_statusline_still_prints_a_line_when_the_provider_reported_no_windows() { - assert_eq!(render_statusline(None), STATUSLINE_FALLBACK); - assert_eq!(render_statusline(Some(&Map::new())), STATUSLINE_FALLBACK); - let unusable = match serde_json::json!({"five_hour": {"used_percentage": "lots"}}) { - Value::Object(map) => map, - _ => unreachable!("the fixture is an object"), - }; - assert_eq!(render_statusline(Some(&unusable)), STATUSLINE_FALLBACK); +fn a_refresh_with_nothing_displaced_prints_the_numbers_it_forwarded() { + let refresh = refresh(STATUSLINE_BODY, None, BUDGET); + + assert_eq!(refresh.line, "5h 40%"); +} + +#[test] +fn a_body_we_cannot_parse_still_reaches_the_displaced_command() { + // Parsing is for the fields we forward; the bytes are the displaced command's + // business, and a payload we cannot read may well be one it can. + let displaced = statusline_entry("cat"); + + let refresh = refresh(b"not json at all", Some(&displaced), ENOUGH); + + assert!(refresh.rate_limits.is_none()); + assert_eq!(refresh.line, "not json at all"); } diff --git a/plugins/nightcrow-recovery/src/hooks.rs b/plugins/nightcrow-recovery/src/hooks.rs index 42d3766..c59d40b 100644 --- a/plugins/nightcrow-recovery/src/hooks.rs +++ b/plugins/nightcrow-recovery/src/hooks.rs @@ -17,6 +17,11 @@ mod merge; use merge::{MARKER, STATUSLINE_KEY}; +/// Re-exported for the statusline helper, which must recognise our own command in +/// order to refuse chaining to it — the same check install and uninstall use to +/// recognise our entries. +pub(crate) use merge::is_ours; + const CLAUDE_DIR: &str = ".claude"; const SETTINGS_FILE: &str = "settings.json"; const BACKUP_FILE: &str = "settings.json.bak"; @@ -126,6 +131,16 @@ pub fn uninstall(paths: &SettingsPaths) -> Result> { Ok(changes) } +/// The `statusLine` install displaced, for the statusline helper to chain to. +/// +/// Read on every refresh, which is why it stays this cheap: one small file, and +/// any trouble reading it means no chain rather than a failure. The value can be +/// JSON `null` — that is what install records when it found no `statusLine` to +/// displace — so a caller must decide what a null means to it. +pub fn displaced_statusline(paths: &SettingsPaths) -> Option { + read_sidecar(&paths.sidecar) +} + /// Absent or empty means "no settings yet"; anything that is not a JSON object /// is a file we do not understand, and guessing at it is worse than stopping. fn read_settings(path: &Path) -> Result { diff --git a/plugins/nightcrow-recovery/src/main.rs b/plugins/nightcrow-recovery/src/main.rs index 8dafd8d..8ac9f0c 100644 --- a/plugins/nightcrow-recovery/src/main.rs +++ b/plugins/nightcrow-recovery/src/main.rs @@ -43,7 +43,8 @@ enum Mode { /// payload on stdin and forwards a few fields to the running plugin. Hook, /// Internal: the command Claude Code runs for its statusline. Forwards the - /// usage windows to the running plugin and prints a short line. + /// usage windows to the running plugin, then prints the statusline this + /// plugin displaced at install time, or a short line of its own. Statusline, } From 190fd1734fb4dd114bc8984190bc252182939669 Mon Sep 17 00:00:00 2001 From: whackur Date: Fri, 31 Jul 2026 00:14:54 +0900 Subject: [PATCH 7/9] ci: test and lint the workspace, not just the root crate cargo test --locked ran one binary, so the recovery plugin's tests never ran in CI at all; clippy only saw the root package. --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f99362f..ed7e8a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,9 +55,9 @@ jobs: - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - - run: cargo build --locked - - run: cargo test --locked - - run: cargo clippy --locked --all-targets --all-features -- -D warnings + - run: cargo build --locked --workspace + - run: cargo test --locked --workspace + - run: cargo clippy --locked --workspace --all-targets --all-features -- -D warnings fmt: name: Rustfmt @@ -67,7 +67,7 @@ jobs: - uses: dtolnay/rust-toolchain@stable with: components: rustfmt - - run: cargo fmt --check + - run: cargo fmt --all --check viewer-ui: name: Viewer frontend From 5e622852878dfcdf361d49eebb7f7d0e8acd3317 Mon Sep 17 00:00:00 2001 From: whackur Date: Fri, 31 Jul 2026 00:14:54 +0900 Subject: [PATCH 8/9] docs: record how a provider signal can name the pane it came from --- README.md | 41 +++++++++++---- docs/architecture.md | 119 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 146 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index c0a4284..89352c4 100644 --- a/README.md +++ b/README.md @@ -316,10 +316,12 @@ The two halves have two owners. The session writes which repositories are open a nightcrow itself knows nothing about the CLIs you run in its panes — an agent and a person get the same PTY. Behaviour that *does* need to know a particular tool lives in a plugin: a separate executable that nightcrow launches and talks -to over a pipe. Plugins are off unless you turn one on, and one can only ever -see a pane you handed it by name. +to over a pipe. Plugins are off unless you turn one on, and one only ever sees a +pane you handed it by name — or, if you also set `watch_on_signal`, a pane that +something running inside it spoke to the plugin from. A plugin is never given a +list of your panes either way. -The bundled plugin is `nightcrow-recovery`. When an opted-in pane's CLI hits its +The bundled plugin is `nightcrow-recovery`. When a watched pane's CLI hits its usage limit, it waits for the reset time the provider reported and then re-opens that exact session. It only waits — it does not bypass, raise, or work around any provider limit, and it sends nothing while a limit is in effect. Claude @@ -354,19 +356,34 @@ allowed_resume_flags = ["--resume", "resume", "--session"] [[startup_command]] name = "Claude" command = "claude" -plugin = "recovery" # without this line, no plugin ever sees this pane +plugin = "recovery" # without this line, no plugin sees this pane unless + # watch_on_signal is set (see below) ``` +That covers the panes you configured. For the pane you did not — you opened a +shell with ` t` and typed `claude` into it yourself — add +`watch_on_signal = true` to the `[[plugin]]` block. nightcrow puts a random token +in each pane's environment and nowhere else, so the CLI's own hook can quote it +back and the plugin can ask for "the pane this token names"; a plain shell never +speaks to a plugin, so your shells stay untouched. It is off by default. Such a +pane can be waited for and typed into but never relaunched — nightcrow launched +no command in it, so there is nothing to put back. + For Claude Code, let the plugin install its hook and statusline entries so it can read the exact session id and reset time instead of guessing from what is -printed on screen. It merges into your existing `~/.claude/settings.json` and -backs it up first: +printed on screen. With a reset time it waits exactly once; without one it falls +back to retrying on a backoff, which can give up. It merges into your existing +`~/.claude/settings.json` and backs it up first: ```bash nightcrow-recovery install-hooks nightcrow-recovery uninstall-hooks # removes only what it added ``` +Claude Code's `statusLine` holds one command, so installing does replace yours — +but it is then run from the plugin's own statusline with the same input, and what +it prints is what you see. `uninstall-hooks` puts it back. + A pane that is waiting shows its state and deadline on its tab. Cancel it with `` then the recovery key (see [Leader commands](#leader-commands-press-prefix-then-the-key)), or from the web viewer; typing into the pane yourself also cancels it. @@ -609,19 +626,25 @@ live_watch = true # watch expanded dirs and refresh the tree live; set f name = "Claude" # optional tab label; falls back to the command text command = "claude" # required; must not be empty plugin = "recovery" # optional; names the [[plugin]] allowed to act on this - # pane. Omitted — the default — means no plugin ever - # sees it. + # pane. Omitted — the default — means no plugin sees + # it unless that plugin sets watch_on_signal. [[startup_command]] command = "cargo test --watch" # External plugin processes — see "Plugins" above. Up to 8 entries, names unique. -# Nothing runs unless an entry exists AND enabled = true AND a pane opted in. +# Nothing runs unless an entry exists AND enabled = true AND either a pane opted +# in or watch_on_signal is set. [[plugin]] name = "recovery" # the name panes opt in with command = "nightcrow-recovery" # found on PATH or in ~/.nightcrow/plugins args = [] # passed to the plugin verbatim enabled = false # off by default +watch_on_signal = false # off by default; when true, a pane no + # [[startup_command]] named is also handed over + # once something inside it quotes that pane's + # token to this plugin. Such a pane is never + # relaunched, only typed into while it lives. allowed_resume_flags = [] # flags the plugin may append to re-open a # session; empty refuses every relaunch diff --git a/docs/architecture.md b/docs/architecture.md index fe8fcee..b87e5e9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -169,6 +169,7 @@ src/ │ ├── host_pump.rs # stdin/stdout/stderr pump threads + capped line reader │ ├── guard.rs # the trust boundary: PluginCommand -> Approved | Refused │ ├── guard_budget.rs # per-slot rate ceilings, keyed by PaneToken +│ ├── guard_watch.rs # the one rule that can widen what a plugin sees │ └── registry.rs # ~/.nightcrow/plugins: install / list / remove ├── git/ │ ├── mod.rs @@ -739,19 +740,116 @@ OpenCode를 아는 코드는 `plugins/nightcrow-recovery`에 산다. 코어 `src 버전이 어긋나는 순간 UB다. cargo feature 게이트는 재컴파일을 요구하므로 "설치·제거 가능"이 아니다. 남는 것은 프로세스 경계이고, 그 편이 신뢰 모델도 정직하다 — plugin은 우리 주소 공간에 없다. 프레이밍은 stdin/stdout의 개행 구분 JSON이고 버전(`v`)이 맞지 않는 줄은 거부한다. -- **opt-in이 곧 도달 범위**: plugin은 `[[startup_command]]`이 `plugin = "이름"`으로 지목한 pane만 - 본다. 클라이언트가 직접 연 pane에는 연결이 생기지 않는다 — 임의의 셸 pane이 자동으로 감지되고 - 조작되는 일은 구조적으로 불가능하다. `[[plugin]]`은 `enabled = false`가 기본이다. +- **도달 범위의 기본은 opt-in, 확장은 증거로만**: plugin은 `[[startup_command]]`이 + `plugin = "이름"`으로 지목한 pane을 본다. 여기에 `[[plugin]]`의 `watch_on_signal`(기본 + `false`)을 켜면 두 번째 경로가 열린다 — **pane 자신의 토큰을 제시한 요청**, 즉 + `PluginCommand::WatchPane { token }`이다. 토큰은 spawn 시각에 그 pane의 자식 환경에만 들어가고 + (`pty_spawn.rs`, 명령 없이 연 pane도 예외 없이) 자식들이 상속하므로, 토큰을 말할 수 있는 것은 + 그 pane 안에서 도는 프로세스뿐이다. 근거가 열거가 아니라 증명이라는 것이 핵심이다: plugin에게 + pane 목록을 주는 경로는 여전히 없고, 맨 셸은 어떤 provider helper도 띄우지 않으므로 영원히 + 채택되지 않는다. "임의의 셸 pane이 자동으로 조작되는 일은 없다"는 성질은 pane을 숨기는 것이 + 아니라 이 증명 요구로 유지된다. `[[plugin]]`은 `enabled = false`가 기본이다. +- **왜 그 확장이 필요했나**: 실제로 압도적으로 흔한 사용은 ` t`로 셸을 열고 `claude`를 + 손으로 치는 것이다. 그 pane은 `create_pane_with(None, None)`으로 열려 launch command가 없고, + `detect(None)`은 어떤 provider도 붙이지 못한다 — 그래서 이 경우 recovery는 **아무것도** 하지 + 않았다. `WatchPane`은 그 구멍만 메운다. `PROTOCOL_VERSION`은 그래서 2가 되었고, 이 명령은 + `generation`을 싣지 않는다: 들어본 적 없는 pane에 대해 어느 spawn인지 정직하게 주장할 수 + 없으므로, 답으로 오는 `PaneOpened`가 그것을 말한다. `Plugins::start`도 그래서 조건이 둘이다 — + enabled이고 **(opt-in됐거나 `watch_on_signal`이거나)**. 후자의 pane은 앞으로 말을 걸어올 + pane이므로 host가 그보다 먼저 떠 있어야 하고, 오지 않을 opt-in을 기다리면 스위치가 아무 뜻도 + 갖지 못한다. +- **요청은 plugin 쪽에서 먼저 줄인다**(`runloop_adopt.rs`): 거부는 응답이 없는 것과 구별되지 + 않으므로, 답을 못 받은 요청이 타이트 루프가 되거나 낯선 토큰마다 상태를 남기면 안 된다. + 미해결 요청은 `MAX_PENDING`개까지만 들고(초과분은 새 것을 버려 실패를 닫힌 방향으로 낸다), + 같은 토큰은 `REQUEST_COOLDOWN` 동안 다시 묻지 않는다 — Claude Code의 statusline은 매 렌더마다 + 돌기 때문에, 이것이 없으면 남의 pane 하나가 초당 몇 번씩 명령을 써서 host의 tick당 예산을 + 정작 필요한 요청과 함께 태운다. 그리고 요청을 정당화한 **신호는 버리지 않고 들고 있다가 + `PaneOpened` 뒤에 재생한다**: 신호가 pane보다 먼저 도착하고(그 신호가 pane이 도착한 이유다) + host는 새로 넘긴 pane에 어떤 history도 재생해 주지 않으므로, 버리면 지금 복구해야 할 그 한도가 + 사라져 provider가 다시 실패할 때까지 pane이 방치된다. 이때 provider는 명령줄이 아니라 + `detect_from_signal`이 고른다 — `SignalKind`는 정확히 한 adapter의 helper만 발행하므로 신호 + 종류 자체가 무엇이 돌고 있는지에 대한 증거이고, 그래서 두 번째 sniffing 경로가 아니라 wire + kind에 대한 lookup이다. +- **늦게 채택된 pane은 relaunch되지 않는다**: launch command가 `None`이므로 프로세스를 되돌려 + 놓으면 provider가 아니라 셸이 다시 뜬다. guard는 이것을 `Refused::NoLaunchCommand`로 — + 인자 문제와 구별되는 자기 이유로 — 거부하고, `allowed_resume_flags`를 어떻게 열어도 통과하지 + 않는다. hub도 같은 판단을 한다: watched pane이 종료했을 때 `is_relaunchable`이 거짓이면 + `PENDING_RELAUNCH_TTL` 동안 slot을 붙잡는 대신 곧바로 닫는 경로를 탄다 — 되돌릴 것이 없는 + slot을 9일 붙잡을 이유가 없다. 이런 pane이 받을 수 있는 recovery는 살아 있는 프로세스에 + 타이핑하는 것 하나뿐이고, plugin 쪽도 같은 결론을 미리 내려 `NeedsAttention`으로 간다 + (`state_resume.rs`). - **신뢰 경계는 `guard.rs` 하나다**: `protocol::decode_command`는 모양과 크기만 본다. 권한은 `Guard::judge`만 판단하고, plugin이 우회할 경로가 없다. 규칙: pane이 존재하고 opt-in했는가, `generation`이 현재와 같은가(이것이 교체된 프로세스에 대한 결정이 후임에게 닿는 것을 막는다), - 살아 있고 조용할 때만 입력을 넣는가, 죽었을 때만 relaunch하는가, 제어문자가 섞이지 않았는가, - slot당 횟수 상한 안인가. 거부는 로그로 남고 재시도되지 않는다. + 살아 있고 조용할 때만 입력을 넣는가, 죽었을 때만 relaunch하는가, 되돌릴 명령이 있는가, + 제어문자가 섞이지 않았는가, slot당 횟수 상한 안인가. 거부는 로그로 남고 재시도되지 않는다. +- **pane을 얻는 규칙만 따로 산다**(`guard_watch.rs`): 나머지 규칙이 모두 "이미 배정된 pane"에서 + 출발하는 데 반해 이것은 배정 자체를 만드는 유일한 자리라, 큰 판단 안의 분기가 아니라 조건 + 목록 하나로 읽히게 분리했다. 순서대로 — 토큰이 아는 pane인가, `watch_on_signal`이 켜졌는가, + 다른 plugin이 이미 보고 있지 않은가(pane 하나에 watcher 하나. 둘이 같은 키보드를 몰면 서로가 + 바꾸는 상태 위에서 recovery가 섞인다), 프로세스가 살아 있는가. 예산은 청구하지 않는다 — + pane을 받는 것은 pane에 하는 일이 아니고, 이어질 행위는 각각 청구되므로 여기서 세면 곧 쓸 + allowance를 미리 태우게 된다. 이미 자기 것인 pane을 다시 물으면 **거부가 아니라 승인**이다: + 명령줄로는 안에 있는 것을 알아볼 수 없었던 opt-in pane이 다시 시도할 유일한 방법이 + `PaneOpened`를 한 번 더 받는 것이기 때문이다. 알 수 없는 토큰이 압도적 다수라는 것도 이 + 설계의 전제다 — 같은 사용자의 다른 nightcrow 세션 pane들이 같은 소켓에 닿는다. - **`PaneToken`이 정체성인 이유**: `PaneId`는 backend별 카운터라 backend가 다시 만들어지면 1로 돌아간다. 프로세스 밖에서 pane을 가리키기에 부적합하고, cwd도 답이 못 된다 — 한 저장소에 여러 pane을 두는 것이 지원되는 레이아웃이다. 그래서 난수 토큰을 spawn 시각에 자식 환경 (`NIGHTCROW_PANE_TOKEN`)으로 넣는다. provider가 띄우는 hook/statusline 자식들이 이를 상속하므로, plugin은 어떤 pane에서 온 사건인지 추측 없이 안다. +- **provider의 설정 파일은 병합만 한다**(`hooks.rs` / `hooks_merge.rs`): `~/.claude/settings.json`은 + 사용자 것이고 우리가 모르는 키와 hook event를 담고 있을 수 있으므로, 모든 수정은 우리가 넣지 + 않은 것을 보존하는 병합이고, 파일을 이해할 수 없으면(JSON이 아니거나 top-level이 object가 + 아니면) 추측하는 대신 멈춘다. 쓰기는 같은 디렉터리의 temp file → rename이고 모드 `0600`은 + rename 전에 건다(대상이 잠깐이라도 world-readable이 되지 않도록), 첫 쓰기 전에 `.bak`을 + 남긴다. 등록하는 hook event는 정확히 하나다 — `HOOK_EVENT = "StopFailure"`, + `HOOK_MATCHER = "rate_limit"` 아래 + `{"type":"command","command":" hook","timeout":5}`. 최소 권한이라서 그렇다: + `authentication_failed`·`billing_error` 같은 무관한 실패의 payload는 이 프로세스에 아예 + 도달하지 않고, 그 대가로 일시적 `overloaded`/`server_error`는 pane 출력에서 알아본다. + `statusLine`도 같은 자리에서 등록한다. 우리 엔트리를 알아보는 표시는 `command` 문자열에 + `MARKER`가 들어 있는지 하나뿐이다 — provider의 스키마에서 자유 텍스트를 넣을 수 있는 필드가 + 거기뿐이고, 우리 마음대로 만든 키는 provider가 unknown으로 거부하거나 경고할 수 있다. 그래서 + install은 `current_exe()`로 해석한 절대 경로가 `MARKER`를 담지 않으면 **거부한다**: 나중에 + uninstall이 자기 엔트리를 알아볼 수 없게 되기 때문이다. 경로를 `argv[0]`이 아니라 해석해서 + 쓰는 이유는 그 파일을 읽는 것이 작업 디렉터리가 다른 다른 프로세스라는 것이다. +- **helper는 provider의 임계 경로에 있으므로 최소한만 한다**(`helper.rs`): 등록되는 명령은 이 + plugin의 바이너리를 내부 서브커맨드로 다시 부르는 것(`main.rs`의 `Mode::Hook` / + `Mode::Statusline`)이다. `hook()`은 stdin을 상한까지만 읽고 + `["session_id","error_type","hook_event_name"]`만 통과시킨다 — whitelisting이 프라이버시 + 경계다. `StopFailure` payload는 transcript 파일 경로와 provider의 에러 산문을 담으므로, + 상태 기계가 실제로 읽는 필드만 소켓을 건넌다. pane은 `NIGHTCROW_PANE_TOKEN`에서 읽고, 한 줄을 + unix socket으로 보내고 끝난다. 어느 실패도 호출자에게 보고하지 않는다 — 돌지 않는 recovery + plugin은 설치되지 않은 것과 정확히 같아 보여야 한다. +- **IPC 랑데부는 경로 규칙 하나다**(`ipc.rs`): `$XDG_RUNTIME_DIR/nightcrow/recovery.sock`, + 없으면 `~/.nightcrow/run/recovery.sock`. 디렉터리는 `0700`, 소켓은 `0600`이고 bind마다 다시 + 건다. 남아 있는 소켓 파일은 **아무도 듣고 있지 않을 때만** unlink한다(살아 있는 listener를 + 가로채지 않기 위해). `parse_line`은 줄 크기, JSON object 여부, `v` 일치, 토큰의 문자 집합과 + 길이, 아는 `kind`, object payload를 모두 검사하고 실패마다 무엇이 틀렸는지 말한다 — + 여기가 untrusted input이 상태가 되는 경계이므로 조용히 강제 변환하는 필드가 곧 버그다. + **토큰은 correlation key이고 authorisation이 아니다**: 소켓에 닿을 수 있는 것은 아무 pane이나 + 주장할 수 있고, 위조된 메시지가 할 수 있는 최대는 이 plugin이 host에게 무언가를 묻게 만드는 + 것이며 그것은 guard가 처음부터 다시 판단한다. +- **statusline은 가로채지 않고 이어붙인다**(`helper_statusline.rs` / `helper_delegate.rs`): + `statusLine`은 목록이 아니라 명령 하나라 install은 사용자 것을 반드시 밀어낸다. 예전에는 + 거기서 끝나 사용자가 자기 statusline을 잃었다. 지금은 `helper::statusline()`이 pass-through다 — + stdin 바이트를 **그대로** 보관하고, 사본만 파싱해 `rate_limits`를 IPC로 넘기고, install이 + sidecar에 기록해 둔 밀려난 명령을 그 원본 바이트를 stdin으로 주어 실행한 뒤 그 stdout을 + 출력한다. 재직렬화하지 않는 이유는 키 순서와 숫자 표기가 provider의 것이고, 우리가 생기기 + 전부터 그 입력을 읽던 명령이 재배열된 것을 보면 안 되기 때문이다. 실행은 `sh -c`로 한다 — + Claude Code가 `statusLine` 명령은 셸에서 돈다고 문서화하고 자기 예시가 `~`, `jq` 파이프, + 인라인 `$(...)`에 의존하므로 우리가 argv로 쪼개면 사용자가 쓴 뜻이 조용히 바뀐다. `$SHELL`이 + 아니라 `sh`인 것은 대화형 셸이면 refresh마다 rc 파일을 읽기 때문이다. 예산은 2초이고 넘기면 + 죽이고 우리 줄로 떨어진다 — provider는 statusline에 timeout을 문서화하지 않았지만 300ms로 + debounce하고 다음 갱신이 오면 진행 중 스크립트를 취소하므로, 이 상한은 반대 방향(끝나지 않는 + 명령이 이 프로세스를 불멸로 만들지 않게)을 위한 것이다. stderr는 버린다(로그용 경고가 + statusline으로 렌더링되면 안 된다). sidecar에 든 것이 우리 자신의 바이너리면 다시 실행하지 + 않는다(install/uninstall이 쓰는 `is_ours`를 그대로 재사용하므로, 거부되는 것이 그 둘이 자기 + 것으로 아는 것과 정확히 같다). 모든 실패 경로는 plugin 자신의 줄로 격하된다 — 에러를 띄우는 + statusline은 평범한 statusline보다 나쁘다. 비자명한 함정 하나: 밀어낼 `statusLine`이 애초에 + 없었으면 `merge_into`가 `Some(Value::Null)`을 돌려주므로 **sidecar가 `null`을 담을 수 있다**. + 없음(sidecar 없음/읽기 실패)만이 빈 경우가 아니고, `null`도 "실행할 것이 없다"로 읽어야 한다. - **횟수 상한은 slot(토큰) 기준으로 센다**: relaunch는 반드시 새 `PaneId`를 만든다. 상한을 id로 세면 relaunch마다 예산이 새로 생겨서, 즉시 끝나는 명령과 매 종료마다 relaunch하는 plugin이 만나면 상한에 영원히 닿지 않는다. 토큰은 relaunch를 건너 살아남는 유일한 값이라 상한이 @@ -772,6 +870,17 @@ OpenCode를 아는 코드는 `plugins/nightcrow-recovery`에 산다. 코어 `src 걸친 escape는 완전히 제거되지 않는다. 이것이 허용되는 이유는 출력 텍스트가 언제나 fallback 신호일 뿐이라는 것이다 — Claude는 hook과 statusline, Codex는 rollout JSONL, OpenCode는 로컬 서버의 세션 상태가 1차 신호다. +- **신호의 역할은 분리돼 있고, 이것이 하중을 받는 사실이다**(`provider/claude.rs`): 한도를 + **선언**할 수 있는 것은 `StopFailure`(`on_stop_failure`)와 출력 fallback뿐이다. statusline은 + 정확한 reset epoch만 공급하고 결코 선언하지 않는다 — `on_rate_limits`는 `resets_at`만 기억하고 + `used_percentage`는 100이어도 의도적으로 무시한다(꽉 찬 창은 한도를 뒷받침하지만 선언하지는 + 않는다). 여러 창이 보고되면 가장 이른 것이 유용한 deadline이다. 이 분리의 결과가 + `state_clock.rs`의 `arm_wait`에서 갈린다: `LimitKind::UsageLimit`이고 `resets_at`이 알려져 + 있으면 `WaitingForReset`으로 **정확히 한 번** 기다리고 resume attempt를 쓰지 않는다(아직 아무 + 것도 시도하지 않았으므로). 모르면 `arm_backoff`로 떨어지고, 그쪽은 attempt 예산에 묶인 + 재시도 루프라 `MAX_RESUME_ATTEMPTS`에 닿으면 `NeedsAttention`으로 끝난다. 그래서 hook과 + statusline을 둘 다 설치하는 것의 실질적 이득은 "감지"가 아니라 **기다림이 정확해지고 예산을 + 쓰지 않는다**는 것이다. - **OpenCode에는 개입하지 않는다**: 자체 재시도가 상한 없이 계속되므로 "재시도 소진"을 기다리는 설계가 성립하지 않는다. 프로세스가 끝났거나 상태가 `idle`로 바뀐 뒤에만 손을 댄다. - **와이어 계약이 두 벌 있다**: plugin은 독립 빌드라 `plugins/nightcrow-recovery`가 프로토콜 From c2db542fd9a79ebe78c853e8f56b200d6027d4cf Mon Sep 17 00:00:00 2001 From: whackur Date: Fri, 31 Jul 2026 00:26:18 +0900 Subject: [PATCH 9/9] test: wait for the reader to go quiet before counting the absence of reads The new no-repository test counted from the moment the watch came up, so the read that start owes it was counted as the once-a-second cadence the test denies. Failed on macOS only, where nothing else delayed that read. Same fix the two absence tests next door already use. --- src/runtime/snapshot/worker_tests.rs | 7 ++++++- src/runtime/snapshot_tests.rs | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/runtime/snapshot/worker_tests.rs b/src/runtime/snapshot/worker_tests.rs index 0d48f1c..a3ee95e 100644 --- a/src/runtime/snapshot/worker_tests.rs +++ b/src/runtime/snapshot/worker_tests.rs @@ -4,7 +4,7 @@ //! at the length this project splits at, and borrowing their helpers: the claim //! is the same kind — how many reads arrive, not what they say. -use crate::runtime::snapshot::tests::{SETTLE, next_read, reads_during}; +use crate::runtime::snapshot::tests::{SETTLE, next_read, quiesce, reads_during}; use crate::runtime::snapshot::{MIN_READ_INTERVAL, SnapshotChannel, SnapshotMsg}; use crate::test_util::run_git; @@ -36,6 +36,11 @@ fn a_path_with_no_repository_is_reported_once_rather_than_every_second() { channel.is_watching(), "this directory could not be watched, so this machine cannot run this test" ); + // Starting the watch owes a read for the gap it was not up for, and that read + // would be counted below as the once-a-second cadence this denies. Wait for the + // reader to actually go quiet rather than trusting it already is. + quiesce(&channel); + assert_eq!( reads_during(&channel, SETTLE), 0, diff --git a/src/runtime/snapshot_tests.rs b/src/runtime/snapshot_tests.rs index 738d252..d89b90a 100644 --- a/src/runtime/snapshot_tests.rs +++ b/src/runtime/snapshot_tests.rs @@ -44,7 +44,7 @@ pub(super) fn next_read(channel: &SnapshotChannel) -> SnapshotMsg { /// this is meant to remove right back in the way. Still far below /// `IDLE_READ_INTERVAL`, so the safety net cannot be what breaks the silence, /// and far below what the caller then waits for a read. -fn quiesce(channel: &SnapshotChannel) { +pub(super) fn quiesce(channel: &SnapshotChannel) { let quiet_for = MIN_READ_INTERVAL * 3; // Only reached if reads never stop, which no test here can cause; a failure // of the reader itself, not of the timing.