diff --git a/apps/daemon/src/session/codex.rs b/apps/daemon/src/session/codex.rs index a45ff20..1d9bcd4 100644 --- a/apps/daemon/src/session/codex.rs +++ b/apps/daemon/src/session/codex.rs @@ -8,6 +8,7 @@ //! `codex --approval-mode full-auto -q ""` use super::runner::Runner; +use super::stream_guards::{is_rate_limit_notice, would_exceed_cap}; use crate::{ipc::event::EventBroadcaster, storage::Storage}; use anyhow::{Context, Result}; use async_trait::async_trait; @@ -98,13 +99,7 @@ impl CodexRunner { let mut lines = BufReader::new(stderr).lines(); while let Ok(Some(line)) = lines.next_line().await { debug!(target: "codex_stderr", "{}", line); - // Detect rate-limit patterns: "rate limit", "too many requests", "429". - let lower = line.to_lowercase(); - if lower.contains("rate limit") - || lower.contains("rate_limit") - || lower.contains("too many requests") - || lower.contains("429") - { + if is_rate_limit_notice(&line) { broadcaster_err.broadcast( "session.statusChanged", json!({ @@ -167,7 +162,9 @@ impl CodexRunner { trace!(session = %self.session_id, line = %line, "codex output"); // Cap accumulated output at 1 MB to prevent OOM on runaway Codex output. - if !truncated && accumulated.len() + line.len() + 1 > Self::MAX_ACCUMULATED_BYTES { + if !truncated + && would_exceed_cap(accumulated.len(), line.len(), Self::MAX_ACCUMULATED_BYTES) + { warn!( session = %self.session_id, bytes = accumulated.len(), diff --git a/apps/daemon/src/session/cursor.rs b/apps/daemon/src/session/cursor.rs index 382eae3..fc9d978 100644 --- a/apps/daemon/src/session/cursor.rs +++ b/apps/daemon/src/session/cursor.rs @@ -25,6 +25,7 @@ //! via the `CURSOR_TOKEN` env var when spawning the subprocess. use super::runner::Runner; +use super::stream_guards::{is_rate_limit_notice, would_exceed_cap}; use crate::{ipc::event::EventBroadcaster, storage::Storage}; use anyhow::{Context, Result}; use async_trait::async_trait; @@ -167,12 +168,7 @@ impl CursorRunner { let mut lines = BufReader::new(stderr).lines(); while let Ok(Some(line)) = lines.next_line().await { debug!(target: "cursor_stderr", "{}", line); - let lower = line.to_ascii_lowercase(); - if lower.contains("rate limit") - || lower.contains("rate_limit") - || lower.contains("too many requests") - || lower.contains("429") - { + if is_rate_limit_notice(&line) { broadcaster_err.broadcast( "session.statusChanged", json!({ @@ -236,7 +232,9 @@ impl CursorRunner { trace!(session = %self.session_id, line = %line, "cursor output"); // Cap accumulated output at 1 MB. - if !truncated && accumulated.len() + line.len() + 1 > Self::MAX_ACCUMULATED_BYTES { + if !truncated + && would_exceed_cap(accumulated.len(), line.len(), Self::MAX_ACCUMULATED_BYTES) + { warn!( session = %self.session_id, bytes = accumulated.len(), diff --git a/apps/daemon/src/session/mod.rs b/apps/daemon/src/session/mod.rs index 5ff3307..5792d66 100644 --- a/apps/daemon/src/session/mod.rs +++ b/apps/daemon/src/session/mod.rs @@ -5,6 +5,7 @@ pub mod cursor; pub mod events; pub mod router; pub mod runner; +pub mod stream_guards; pub mod system_prompt; pub mod telemetry; pub mod worktree; diff --git a/apps/daemon/src/session/stream_guards.rs b/apps/daemon/src/session/stream_guards.rs new file mode 100644 index 0000000..c833cc3 --- /dev/null +++ b/apps/daemon/src/session/stream_guards.rs @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: MIT +//! Guards applied to a provider's output stream as it is read. +//! +//! Both the Codex and Cursor runners read a child process's stdout and stderr +//! line by line, and both apply the same two rules to every line: notice when +//! the provider is telling us it has been rate limited, and stop accumulating +//! once the captured output would pass a byte cap. +//! +//! The rules lived inline and identically in both runners, which meant they +//! could only be exercised by spawning a real child process — so in practice +//! they were not exercised at all. Pulling them out here makes them ordinary +//! pure functions with ordinary tests, and leaves one definition instead of +//! two to keep in step. + +/// Substrings that mean a provider is refusing work because of a rate limit. +/// +/// Matched case-insensitively against a single line of provider output. +const RATE_LIMIT_MARKERS: [&str; 4] = ["rate limit", "rate_limit", "too many requests", "429"]; + +/// Returns `true` if `line` looks like a rate-limit notice from a provider. +/// +/// # Inputs +/// * `line` — one raw line of provider stderr/stdout, any case. +/// +/// # Outputs +/// `true` when any known marker appears anywhere in the line. +/// +/// # Constraints +/// Matching is case-insensitive and substring-based, so it is deliberately +/// permissive: a false positive costs one spurious `RATE_LIMITED` status +/// event, whereas a false negative means a stalled session with no +/// explanation. +pub(crate) fn is_rate_limit_notice(line: &str) -> bool { + let lower = line.to_lowercase(); + RATE_LIMIT_MARKERS + .iter() + .any(|marker| lower.contains(marker)) +} + +/// Returns `true` if appending `line_len` bytes to `accumulated_len` bytes +/// would push the captured output past `cap`. +/// +/// # Inputs +/// * `accumulated_len` — bytes captured so far. +/// * `line_len` — bytes in the line about to be appended. +/// * `cap` — the hard ceiling, in bytes. +/// +/// # Outputs +/// `true` when the line must be refused. +/// +/// # Constraints +/// The `+ 1` accounts for the newline the caller appends after each line, so +/// the check matches what is actually stored rather than what was read. The +/// comparison is strict: landing exactly on `cap` is allowed, since the cap is +/// the largest permitted size and not the first forbidden one. +pub(crate) fn would_exceed_cap(accumulated_len: usize, line_len: usize, cap: usize) -> bool { + accumulated_len + line_len + 1 > cap +} + +#[cfg(test)] +mod tests; diff --git a/apps/daemon/src/session/stream_guards/tests.rs b/apps/daemon/src/session/stream_guards/tests.rs new file mode 100644 index 0000000..9e6b4da --- /dev/null +++ b/apps/daemon/src/session/stream_guards/tests.rs @@ -0,0 +1,102 @@ +//! Tests for the provider output-stream guards. +//! +//! These two rules previously lived inline in the Codex and Cursor runners, +//! where reaching them meant spawning a real child process — so every mutant +//! the gate generated for them survived. Each test below is pinned to a value +//! that a specific mutation would change. + +use super::*; + +// ─── is_rate_limit_notice ──────────────────────────────────────────────────── + +/// Every marker is pinned separately, so no single one can be dropped from the +/// list unnoticed. Each string here matches exactly one marker: dropping that +/// marker makes the line unrecognised and the session stalls with no status +/// event to explain why. +#[test] +fn each_rate_limit_marker_is_recognised_on_its_own() { + assert!(is_rate_limit_notice("error: rate limit exceeded")); + assert!(is_rate_limit_notice("error: rate_limit_exceeded")); + assert!(is_rate_limit_notice("HTTP 400: too many requests")); + assert!(is_rate_limit_notice("server responded 429")); +} + +/// Matching is case-insensitive — pins the `to_lowercase()` call, without +/// which a provider shouting about a RATE LIMIT would be ignored. +#[test] +fn markers_are_matched_regardless_of_case() { + assert!(is_rate_limit_notice("RATE LIMIT EXCEEDED")); + assert!(is_rate_limit_notice("Too Many Requests")); + assert!(is_rate_limit_notice("Rate_Limit")); +} + +/// The negative direction, so the assertions above cannot be satisfied by a +/// function that returns `true` for everything. +#[test] +fn ordinary_output_is_not_a_rate_limit_notice() { + assert!(!is_rate_limit_notice("")); + assert!(!is_rate_limit_notice("compiling 12 crates")); + assert!(!is_rate_limit_notice("HTTP 200 OK")); + // Near misses: a different 4xx, and the words apart from each other. + assert!(!is_rate_limit_notice("server responded 42")); + assert!(!is_rate_limit_notice("HTTP 409: conflict")); + assert!(!is_rate_limit_notice("too many open files")); +} + +// ─── would_exceed_cap ──────────────────────────────────────────────────────── + +/// Kills both `+` -> `*` and `+` -> `-` mutations of the size sum. +/// +/// The numbers are chosen so the true sum is distinguishable from every +/// mutation of it: 10 + 5 + 1 = 16, against 10 * 5 + 1 = 51, 10 - 5 + 1 = 6, +/// 10 + 5 * 1 = 15 and 10 + 5 - 1 = 14. With a cap of 15 the real answer is +/// "yes, this line would exceed it", while `+ 5 * 1`, `+ 5 - 1` and `10 - 5` +/// all say no, and `10 * 5` says yes for the wrong reason — so the companion +/// test below pins a case where the products and the sum disagree the other +/// way round. +#[test] +fn a_line_that_would_pass_the_cap_is_refused() { + assert!(would_exceed_cap(10, 5, 15)); +} + +/// The other side of the same sum: 10 + 5 + 1 = 16 fits under a cap of 16, +/// but 10 * 5 + 1 = 51 does not. Together with the test above this pins every +/// arithmetic mutation in both directions. +#[test] +fn a_line_that_exactly_fills_the_cap_is_accepted() { + assert!(!would_exceed_cap(10, 5, 16)); +} + +/// Kills `>` -> `>=`, `==` and `<`. +/// +/// Landing exactly on the cap must be allowed, which separates `>` from `>=` +/// and from `==`; being well under it must be allowed, which separates `>` +/// from `<`. The cap is the largest permitted size, not the first forbidden +/// one, and an off-by-one here silently truncates output that fit. +#[test] +fn the_cap_is_an_inclusive_ceiling() { + // Exactly at the cap: allowed. + assert!(!would_exceed_cap(100, 99, 200)); + // One byte past it: refused. + assert!(would_exceed_cap(100, 100, 200)); + // Far below it: allowed. + assert!(!would_exceed_cap(1, 1, 200)); + // Far above it: refused. + assert!(would_exceed_cap(500, 1, 200)); +} + +/// The newline the caller appends is counted. Without the `+ 1` a line that +/// exactly fills the cap would be accepted and then stored one byte over. +#[test] +fn the_trailing_newline_counts_against_the_cap() { + // 4 + 5 = 9 bytes of text, plus the newline, is 10 — one past a cap of 9. + assert!(would_exceed_cap(4, 5, 9)); + assert!(!would_exceed_cap(4, 5, 10)); +} + +/// An empty line still costs its newline. +#[test] +fn an_empty_line_still_costs_one_byte() { + assert!(!would_exceed_cap(9, 0, 10)); + assert!(would_exceed_cap(10, 0, 10)); +}