diff --git a/CHANGELOG.md b/CHANGELOG.md index 281d399..b3bae4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -175,6 +175,16 @@ reach because the first Read was blocked and never entered its cache. - On a 1,442-line fixture the scenario costs 444 tokens (a 211-token redirect and a 233-token `recall_file`) against 13,807 for the released Read. +### The settings file is read with a limit, and a bad value is repeated back in short + +`lumen_settings.json` is read on every tool call, and it was read whole, whatever it was: a very +large file was read into memory each time, and a named pipe or a device where the file should be +could block every call. A file larger than 64 KiB, or one that is not a regular file, is now +reported as unreadable: repeat reads are off, as for any unreadable file, and the problem says +why. A wrong-typed value in the file, and a `LUMEN_REPEAT_READS` that is not `on`, `off` or `ab`, +are repeated back in the fault row and on stderr as written when that is at most 64 characters, +and otherwise as its first 64 characters and an ellipsis. A valid file is read exactly as before. + ### A rewind the daemon could not see is now treated as one When the daemon tailed a transcript from a byte offset without that transcript's tracker (an ingest diff --git a/crates/lumen-core/src/settings.rs b/crates/lumen-core/src/settings.rs index 6ddf3b1..33dbdaf 100644 --- a/crates/lumen-core/src/settings.rs +++ b/crates/lumen-core/src/settings.rs @@ -13,7 +13,8 @@ //! ``` //! //! It is read on every `tools/call`, not once at startup: a setting change takes effect on the -//! very next read, with no restart of Claude Code. The read is one small `open + read`. +//! very next read, with no restart of Claude Code. The read is one stat and one small read, +//! never more than 64 KiB. //! //! Anything that cannot be read as an answer is the safe side, which is OFF (every read in //! full, nothing stubbed) and is reported, not swallowed: [`Resolved::problem`] says what was @@ -29,6 +30,10 @@ pub const REPEAT_READS_ENV: &str = "LUMEN_REPEAT_READS"; pub const FILE_NAME: &str = "lumen_settings.json"; /// The key in the file. pub const REPEAT_READS_KEY: &str = "repeat_reads"; +/// The most the settings file may hold. It is read on every call; a larger one is a mistake. +pub const MAX_FILE_BYTES: u64 = 64 * 1024; +/// The most characters of a bad value that a problem repeats back. +const ECHO_MAX: usize = 64; /// Where the setting file is for a ledger at `db`. pub fn settings_path(db: &Path) -> PathBuf { @@ -97,12 +102,41 @@ pub enum FileState { Unreadable(String), } -/// Read the settings file at `path`. +/// The text of the regular file at `path`, read to [`MAX_FILE_BYTES`], or why it cannot be used. pub fn read_file(path: &Path) -> FileState { - match std::fs::read_to_string(path) { - Ok(t) => FileState::Text(t), + let opened = std::fs::metadata(path).and_then(|m| { + if m.is_file() { + std::fs::File::open(path) + } else { + Err(std::io::Error::other("not a regular file")) + } + }); + match opened.and_then(read_capped) { Err(e) if e.kind() == std::io::ErrorKind::NotFound => FileState::Absent, Err(e) => FileState::Unreadable(e.to_string()), + Ok(bytes) if bytes.len() as u64 > MAX_FILE_BYTES => { + FileState::Unreadable(format!("larger than {MAX_FILE_BYTES} bytes")) + } + Ok(bytes) => match String::from_utf8(bytes) { + Ok(t) => FileState::Text(t), + Err(_) => FileState::Unreadable("stream did not contain valid UTF-8".to_string()), + }, + } +} + +/// At most one byte past the cap: enough to tell a file over it from one at it. +fn read_capped(r: impl std::io::Read) -> std::io::Result> { + use std::io::Read; + let mut bytes = Vec::new(); + r.take(MAX_FILE_BYTES + 1).read_to_end(&mut bytes)?; + Ok(bytes) +} + +/// `s` as it is, or its first [`ECHO_MAX`] characters and an ellipsis. +fn bounded(s: &str) -> String { + match s.char_indices().nth(ECHO_MAX) { + Some((cut, _)) => format!("{}…", &s[..cut]), + None => s.to_string(), } } @@ -118,7 +152,7 @@ fn parse_env(v: Option<&str>) -> Option> { "on" | "1" | "true" => Ok(Mode::On), "off" | "0" | "false" => Ok(Mode::Off), "ab" => Ok(Mode::Ab), - _ => Err(t.to_string()), + _ => Err(bounded(t)), }) } @@ -137,7 +171,8 @@ fn parse_file(state: &FileState) -> Result, String> { None => Ok(None), Some(serde_json::Value::Bool(b)) => Ok(Some(*b)), Some(other) => Err(format!( - "{REPEAT_READS_KEY} must be true or false, not {other}" + "{REPEAT_READS_KEY} must be true or false, not {}", + bounded(&other.to_string()) )), } } @@ -212,7 +247,7 @@ fn with_env(file: &FileState) -> Resolved { Err(std::env::VarError::NotUnicode(raw)) => Resolved { mode: Mode::Off, source: Source::Env, - problem: Some(Problem::Env(raw.to_string_lossy().into_owned())), + problem: Some(Problem::Env(bounded(&raw.to_string_lossy()))), }, } } @@ -513,4 +548,106 @@ mod tests { write_repeat_reads(&db, false).unwrap(); assert_eq!(read_file_mode(&db), Mode::Off); } + + fn padded_to(len: usize) -> String { + let bare = r#"{"repeat_reads":false,"pad":""}"#.len(); + format!( + r#"{{"repeat_reads":false,"pad":"{}"}}"#, + "x".repeat(len - bare) + ) + } + + #[test] + fn a_file_at_the_cap_is_read_and_one_byte_over_is_not() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(FILE_NAME); + let cap = MAX_FILE_BYTES as usize; + std::fs::write(&path, padded_to(cap)).unwrap(); + assert_eq!(resolve(None, &read_file(&path)).mode, Mode::Off); + assert!( + resolve(None, &read_file(&path)).problem.is_none(), + "a file at the cap is a setting" + ); + std::fs::write(&path, padded_to(cap + 1)).unwrap(); + let r = resolve(None, &read_file(&path)); + assert_eq!(r.mode, Mode::Off); + assert!( + matches!(&r.problem, Some(Problem::File(why)) if why.contains("larger than")), + "{r:?}" + ); + } + + #[cfg(unix)] + #[test] + fn a_pipe_where_the_file_should_be_is_a_problem_and_never_blocks() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(FILE_NAME); + let made = std::process::Command::new("mkfifo") + .arg(&path) + .status() + .unwrap(); + assert!(made.success()); + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || tx.send(read_file(&path)).unwrap()); + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(FileState::Unreadable(why)) => { + assert!(why.contains("not a regular file"), "{why}") + } + other => panic!("{other:?}"), + } + } + + #[test] + fn a_reader_that_never_ends_is_read_only_up_to_the_cap() { + struct Endless(u64); + impl std::io::Read for Endless { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + if self.0 > MAX_FILE_BYTES + 1 { + return Err(std::io::Error::other("read past the cap")); + } + buf.fill(b'x'); + self.0 += buf.len() as u64; + Ok(buf.len()) + } + } + let bytes = read_capped(Endless(0)).unwrap(); + assert_eq!(bytes.len() as u64, MAX_FILE_BYTES + 1); + } + + #[test] + fn a_wrong_typed_value_is_echoed_short_and_a_short_one_whole() { + let prefix = "repeat_reads must be true or false, not "; + let long = "y".repeat(10_000); + let why = parse_file(&text(&format!(r#"{{"repeat_reads":"{long}"}}"#))).unwrap_err(); + assert_eq!(why, format!("{prefix}\"{}…", "y".repeat(63))); + let why = parse_file(&text(&format!(r#"{{"repeat_reads":[{long:?}]}}"#))).unwrap_err(); + assert_eq!( + why.chars().count(), + prefix.chars().count() + 64 + 1, + "{why}" + ); + let short = parse_file(&text(r#"{"repeat_reads":"yes"}"#)).unwrap_err(); + assert_eq!(short, format!("{prefix}\"yes\"")); + } + + #[test] + fn an_echo_keeps_64_characters_and_cuts_the_65th() { + assert_eq!(bounded(&"é".repeat(64)), "é".repeat(64)); + assert_eq!(bounded(&"é".repeat(65)), format!("{}…", "é".repeat(64))); + } + + #[test] + fn an_env_value_that_means_nothing_is_echoed_short_and_a_short_one_whole() { + for long in ["z".repeat(10_000), "é".repeat(10_000)] { + let first_64: String = long.chars().take(64).collect(); + assert_eq!( + resolve(Some(&long), &FileState::Absent).problem, + Some(Problem::Env(format!("{first_64}…"))) + ); + } + assert_eq!( + resolve(Some("bogus"), &FileState::Absent).problem, + Some(Problem::Env("bogus".into())) + ); + } }