From 28e455f968f756a9e283dae95149d2f507b20e16 Mon Sep 17 00:00:00 2001 From: Metal Agent Date: Tue, 1 Sep 2026 12:51:25 -0700 Subject: [PATCH] Layer settings over a file that documents itself Compiled default, then the file, then the environment -- with the environment winning, which is the reverse of the usual arrangement and is the point: LUMBRIDGE_CLAUDE_OAUTH=0 is documented as one switch off, and a switch a configuration file can silently turn back on is not a switch. Where a variable has pinned a value the settings pane shows that row disabled and names the variable rather than accepting an edit that would do nothing. default.toml carries every key Lumbridge understands, commented out, showing the compiled default, and a test uncomments them and asserts the key set is exactly the set the binary knows -- so neither half can drift from the other. toml reads and toml_edit writes, because serialising the parsed model back over the file would strip every comment and every key this build does not recognise, and here the comments are the documentation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SPYebLiN2w4TqnHUYGdECq --- crates/lumbridge-settings/Cargo.toml | 8 + crates/lumbridge-settings/assets/default.toml | 145 ++ crates/lumbridge-settings/src/file.rs | 1564 +++++++++++++++++ crates/lumbridge-settings/src/lib.rs | 13 + crates/lumbridge-storage/src/lib.rs | 779 +++++++- 5 files changed, 2464 insertions(+), 45 deletions(-) create mode 100644 crates/lumbridge-settings/assets/default.toml create mode 100644 crates/lumbridge-settings/src/file.rs diff --git a/crates/lumbridge-settings/Cargo.toml b/crates/lumbridge-settings/Cargo.toml index 72fd630..5b69a66 100644 --- a/crates/lumbridge-settings/Cargo.toml +++ b/crates/lumbridge-settings/Cargo.toml @@ -10,6 +10,14 @@ repository.workspace = true [dependencies] serde = { version = "1.0.228", features = ["derive"] } toml = "0.9" +# `toml` reads; `toml_edit` writes. Serialising the parsed model back over the +# file would strip every comment and every key this build does not recognise, +# and in this design the comments are the documentation. +toml_edit = "0.25" [lints] workspace = true + +[dev-dependencies] +# Already in the workspace's dependency closure via lumbridge-storage's tests. +tempfile = "3.27.0" diff --git a/crates/lumbridge-settings/assets/default.toml b/crates/lumbridge-settings/assets/default.toml new file mode 100644 index 0000000..c4ff95d --- /dev/null +++ b/crates/lumbridge-settings/assets/default.toml @@ -0,0 +1,145 @@ +# Lumbridge settings. +# +# This file is the documentation. Every key Lumbridge understands appears below, +# commented out, showing the value Lumbridge uses when you say nothing — so the +# whole configuration surface can be read without leaving the file. +# +# Two kinds of comment appear here and the difference is load-bearing: +# +# #key = value a setting, commented out, showing the compiled default. +# Remove the '#' to take it over. +# # a sentence explanation. Never a setting. +# +# A test uncomments the first kind, parses the result, and checks that the set +# of keys is exactly the set Lumbridge knows about. Neither half can drift. +# +# --------------------------------------------------------------------------- +# Layers +# +# compiled default -> this file -> the environment +# +# The environment wins, which is the reverse of the usual arrangement and is +# deliberate. LUMBRIDGE_CLAUDE_OAUTH=0 is documented as one switch off, and a +# switch a configuration file can silently turn back on is not a switch. Where a +# variable has pinned a value the settings pane shows that row disabled and +# names the variable, rather than accepting an edit that would do nothing. +# +# --------------------------------------------------------------------------- +# What Lumbridge does to this file +# +# Changing something in the settings pane rewrites the one key you changed and +# leaves everything else exactly as you left it: your comments, your ordering, +# your spacing, and any key this build does not recognise. +# +# An unrecognised key is kept, not deleted, and listed on the Advanced page — +# a file written by a newer Lumbridge still opens in an older one. A syntax +# error is a different matter: Lumbridge keeps the last configuration it read +# and shows a banner naming the line, because a typo must never quietly reset +# your settings to defaults. +# +# --------------------------------------------------------------------------- +# Who may write each key +# +# Every key below says either "a person only" or "a person, or an agent holding +# the Configure capability". Anything that names a program, a filesystem path, +# or a network destination is a person only, and a request from anywhere else is +# refused rather than quietly downgraded. An agent that can arrange panes must +# not be able to choose what runs inside them. +# +# --------------------------------------------------------------------------- +# What is deliberately not here +# +# Four things could plausibly be settings and never will be, because each would +# turn this file into an attack: +# +# - The address an access token is sent to. A configuration file that can +# redirect it is a credential exfiltration path with a friendly name. +# - The file that token is read from. The same attack from the other end. +# - The name Lumbridge gives when it calls a provider. Lumbridge identifies +# itself instead of borrowing the harness's identity, so that its requests +# stay distinguishable from the harness's in the provider's own logs. +# - The program a new pane launches. A layout-only agent cannot choose the +# shell you get, and a settings key would hand it exactly that at one +# remove. +# +# Nothing here turns telemetry on or off, because there is none to turn off. +# The Advanced page lists every file, endpoint, and child process Lumbridge +# reads, by name. + + +[appearance] + +# The syntax theme every interface colour is derived from. Lumbridge does not +# store a palette; it takes five anchors from the theme and computes the rest, +# so a theme name changes the whole window and not just the editor. +# Written by a person, or by an agent holding the Configure capability. +# Takes effect immediately. +#theme = "lumbridge-slate" + +# The action colour: focus rings, selection, the active tab. Kept separate from +# the theme so that the colour meaning "you are here" can be chosen once and +# survive a change of theme. +# Written by a person, or by an agent holding the Configure capability. +# Takes effect immediately. +#accent = "blue" + + +[usage] + +# Read the access token Claude Code stored on this machine, and use it to ask +# Anthropic one documented question about your own subscription: the per-model +# weekly limits a Max plan meters separately, and an answer on a cold start +# before any session has taken a turn. +# +# The token is never persisted, never logged, never copied into application +# state, never written to a crash report, and never passed as a command-line +# argument. It is released as soon as the request is made. No refresh token is +# ever used — renewing a credential is the harness's job. The request identifies +# Lumbridge as the caller. +# +# LUMBRIDGE_CLAUDE_OAUTH=0 in the environment turns this off and outranks +# whatever this file says. +# Written by a person, or by an agent holding the Configure capability. +# Restarts the probe. +#claude_account_endpoint = true + +# Follow the session transcripts Claude Code writes, for token spend. This is +# the only local surface that reports what that harness actually spent. The +# reader models four token counters and nothing else, so the conversations in +# those files are not representable in a Lumbridge value. It reports nothing at +# all until it has read the existing backlog to the end, because a partly-read +# backlog is indistinguishable from a burst of spend. +# Written by a person, or by an agent holding the Configure capability. +# Restarts the probe. +#claude_transcripts = true + +# Launch `codex app-server` and ask it for the quota windows it already knows. +# The client cannot express a request outside a two-variant enum and answers +# every server-to-client request with "method not found", so a harness asking +# Lumbridge for a credential is refused by construction. +# Written by a person, or by an agent holding the Configure capability. +# Restarts the probe. +#codex_app_server = true + +# Where the installed status-line bridge writes Claude Code's rate-limit +# windows. Left unset, Lumbridge looks where the bridge installer puts it; set +# this only if you installed the bridge somewhere else. It has to match what the +# bridge was installed with exactly, or the windows read as unavailable forever. +# +# The example below is an example, not a default — there is no compiled default +# for this key. LUMBRIDGE_CLAUDE_FEED in the environment overrides it. +# Written by a person only: it names a filesystem path. +# Restarts the probe. +#claude_rate_limit_feed = "/example/path/to/claude-rate-limits.jsonl" + + +[terminal] + +# Let a program running in a pane write your system clipboard through the OSC 52 +# escape sequence. Off unless you ask for it: OSC 52 is available to anything +# that can write to your terminal, which includes the contents of a log file you +# happen to cat, so leaving it on means a file can replace what you are about to +# paste. +# Written by a person only: it hands a program a capability outside its pane. +# Applies to new panes; panes already open keep what they were launched with. +#allow_osc52_clipboard = false diff --git a/crates/lumbridge-settings/src/file.rs b/crates/lumbridge-settings/src/file.rs new file mode 100644 index 0000000..2de3397 --- /dev/null +++ b/crates/lumbridge-settings/src/file.rs @@ -0,0 +1,1564 @@ +//! The settings file on disk: reading it, and rewriting it without damage. +//! +//! Three things here are less obvious than they look. +//! +//! **An absent file is not a failure.** A fresh install has no `settings.toml` +//! and is correctly configured. [`LoadOutcome`] therefore separates "not there, +//! use the compiled defaults" from "there and unreadable" from "there and not +//! valid TOML". A parse error in particular never falls back to defaults: the +//! caller keeps the last content it successfully read and shows a banner +//! naming the line, because a typo must never silently reset a configuration. +//! +//! **Writes go through [`toml_edit`], not through `serde`.** Serialising a +//! `SettingsContent` back over the file would produce a semantically identical +//! document with every comment and every unrecognised key stripped out — and in +//! this design the comments *are* the documentation, so that is data loss. A +//! `DocumentMut` edit changes the one key that changed and leaves the rest of +//! the bytes alone. +//! +//! **A write has to be able to recognise its own echo.** The file is watched so +//! that a hand edit shows up without a restart. A watcher cannot tell our write +//! apart from a person's, so a debounced write bounces off its own event and +//! reloads, which schedules another write, forever. [`SettingsFile`] records a +//! fingerprint of the bytes it last wrote and [`SettingsFile::is_own_echo`] +//! answers the watcher's question before any reload work happens. + +use std::fmt; +use std::fs; +use std::io; +use std::io::Write as _; +use std::path::{Path, PathBuf}; + +use toml_edit::{DocumentMut, Item, Table}; + +use crate::paths::SettingsPaths; +use crate::{FORBIDDEN_PATHS, SETTINGS, SettingItem, SettingsContent, UnknownKey, WriteAuthority}; + +/// The commented reference file, embedded in the binary. +/// +/// This is the file's own documentation, and it is what Lumbridge writes when +/// it has to create a `settings.toml` that was not there. Shipping a file full +/// of commented-out defaults rather than active keys matters: an active key +/// would report its origin as `settings.toml` when nobody had chosen it, and +/// the pane would show every row as user-set on a machine nobody had +/// configured. +pub const DEFAULT_TOML: &str = include_str!("../assets/default.toml"); + +/// What reading the settings file produced. +/// +/// Not a `Result`, because the three outcomes call for three different +/// behaviours and collapsing them into "error" loses the distinction that +/// matters: a missing file is normal, an unparseable one must not be +/// overwritten, and an unreadable one is a problem with the machine rather than +/// with the configuration. +#[derive(Clone, Debug)] +pub enum LoadOutcome { + /// No file. Use the compiled defaults; this is not an error. + Absent, + /// A file this build could read. + Loaded { + /// The keys this build understands. + content: SettingsContent, + /// The keys it does not, kept so the Advanced page can report them. + unknown: Vec, + }, + /// A file that is not valid TOML. + /// + /// The caller keeps whatever it last loaded and shows the failure. It must + /// not substitute defaults, and [`SettingsFile`] will not write over the + /// file while it is in this state. + Malformed(ParseFailure), + /// A file that exists and could not be read at all: permissions, a + /// directory where a file should be, bytes that are not UTF-8. + Unreadable { + /// The operating system's description of what went wrong. + message: String, + }, +} + +/// A settings file that is not valid TOML. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ParseFailure { + /// The parser's description of what it found. + pub message: String, + /// The 1-based line, when the parser reported a position. + /// + /// `Option` rather than a `0` placeholder. A parser that reported no span + /// has told us it does not know where the problem is, and a banner pointing + /// at line 0 — or at line 1 — would send someone to look at a line that is + /// not the problem. + pub line: Option, + /// The 1-based column, on the same terms as [`Self::line`]. + pub column: Option, +} + +impl ParseFailure { + fn at(message: String, text: &str, span: Option>) -> Self { + let position = span.and_then(|span| position_of(text, span.start)); + Self { + message, + line: position.map(|(line, _)| line), + column: position.map(|(_, column)| column), + } + } + + fn from_toml(error: &toml::de::Error, text: &str) -> Self { + Self::at(error.message().to_owned(), text, error.span()) + } + + fn from_toml_edit(error: &toml_edit::TomlError, text: &str) -> Self { + Self::at(error.message().to_owned(), text, error.span()) + } +} + +impl fmt::Display for ParseFailure { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match (self.line, self.column) { + (Some(line), Some(column)) => { + write!(formatter, "line {line}, column {column}: {}", self.message) + } + (Some(line), None) => write!(formatter, "line {line}: {}", self.message), + _ => write!( + formatter, + "at a position the parser did not report: {}", + self.message + ), + } + } +} + +impl std::error::Error for ParseFailure {} + +/// Translates a byte offset into a 1-based line and column. +/// +/// Returns `None` when the offset does not land on a character boundary, which +/// would make any answer a guess. Columns are counted in characters, because +/// that is what a person counts when they look at the line. +fn position_of(text: &str, offset: usize) -> Option<(usize, usize)> { + let before = text.get(..offset)?; + let line = before.matches('\n').count() + 1; + let column = before + .rsplit('\n') + .next() + .map_or(0, |partial| partial.chars().count()) + + 1; + Some((line, column)) +} + +/// Reads the settings file named by `paths`. +/// +/// A file that is not there yields [`LoadOutcome::Absent`] rather than an +/// error, because a machine nobody has configured is configured correctly. +#[must_use] +pub fn load(paths: &impl SettingsPaths) -> LoadOutcome { + load_from(&paths.settings_file()) +} + +/// Reads a settings file at an exact path. +/// +/// [`load`] is the one to call; this exists for a caller that already resolved +/// the path, and for tests that put a file in a temporary directory. +#[must_use] +pub fn load_from(path: &Path) -> LoadOutcome { + let text = match fs::read_to_string(path) { + Ok(text) => text, + Err(error) if error.kind() == io::ErrorKind::NotFound => return LoadOutcome::Absent, + Err(error) => { + return LoadOutcome::Unreadable { + message: error.to_string(), + }; + } + }; + match crate::parse(&text) { + Ok((content, unknown)) => LoadOutcome::Loaded { content, unknown }, + Err(error) => LoadOutcome::Malformed(ParseFailure::from_toml(&error, &text)), + } +} + +/// Who is asking for a write. +/// +/// The counterpart to [`WriteAuthority`], which says who is *allowed*. Keeping +/// them as two types means a caller cannot pass "this field is agent-writable" +/// where "an agent is asking" was wanted. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum WriteOrigin { + /// A person at the keyboard. + Human, + /// An agent or automation holding the Configure capability. + Configure, +} + +impl WriteOrigin { + /// Whether this origin may write a field with the given authority. + #[must_use] + pub const fn satisfies(self, required: WriteAuthority) -> bool { + matches!( + (self, required), + (Self::Human, _) | (Self::Configure, WriteAuthority::Configure) + ) + } +} + +/// A value a setting can be given. +/// +/// Deliberately two variants rather than a re-exported `toml_edit::Value`. A +/// caller cannot ask for an array, a table, or a datetime in a field whose +/// schema is a boolean, so the shapes the file schema cannot hold are not +/// expressible at the call site. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum SettingValue { + /// A boolean. + Flag(bool), + /// A string. + Text(String), +} + +impl SettingValue { + fn into_item(self) -> Item { + match self { + Self::Flag(flag) => toml_edit::value(flag), + Self::Text(text) => toml_edit::value(text), + } + } +} + +/// Why a write did not happen. +/// +/// Every variant means nothing was written. There is no partial-failure case: +/// the document is edited in memory, checked, and only then does a single +/// rename publish it. +#[derive(Debug)] +pub enum WriteError { + /// No setting has that path in this build. + UnknownPath(String), + /// A path `FORBIDDEN_PATHS` names, which is never a setting. + Forbidden { + /// The path that was asked for. + path: String, + /// Why it is not, and will not become, a setting. + reason: &'static str, + }, + /// The field is human-only and something else asked to write it. + /// + /// Refused, not downgraded and not silently dropped. A caller that learns + /// its write was ignored can say so; a caller told "done" cannot. + AuthorityRefused { + /// The path that was asked for. + path: String, + /// The authority that path requires. + required: WriteAuthority, + /// The origin that asked. + origin: WriteOrigin, + }, + /// The file already on disk is not valid TOML. + /// + /// Refusing here is the point: a person with a typo in their file has a + /// file full of their own comments and choices, and rewriting it from a + /// parsed model would throw all of that away to fix a missing bracket. + Malformed(ParseFailure), + /// A section in the file is not a table — `usage = 3`, say. + NotASection { + /// The section name that is occupied by something else. + section: String, + }, + /// The edit produced a document this build cannot read back. + /// + /// A last check before the rename. No edit this API can express reaches it + /// today — [`SettingValue`] cannot name a shape the schema will not hold, + /// and a file that was already unreadable is caught as + /// [`WriteError::Malformed`] before the edit. It stays because writing a + /// file we would then refuse to load is the one failure that locks a person + /// out of their own settings with our bug rather than their typo, and that + /// is worth checking rather than reasoning about. + Rejected(ParseFailure), + /// The filesystem refused. + Io { + /// What was being written. + path: PathBuf, + /// The underlying failure. + source: io::Error, + }, +} + +impl fmt::Display for WriteError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::UnknownPath(path) => write!(formatter, "no setting named {path}"), + Self::Forbidden { path, reason } => { + write!(formatter, "{path} is never a setting: {reason}") + } + Self::AuthorityRefused { + path, + required, + origin, + } => write!( + formatter, + "{path} may be written by {required:?} only, and {origin:?} asked" + ), + Self::Malformed(failure) => write!( + formatter, + "the settings file on disk is not valid TOML ({failure}); \ + nothing was written, so nothing was lost" + ), + Self::NotASection { section } => { + write!(formatter, "[{section}] is not a table in the settings file") + } + Self::Rejected(failure) => write!( + formatter, + "the edit would have produced a file this build cannot read ({failure})" + ), + Self::Io { path, source } => write!(formatter, "writing {}: {source}", path.display()), + } + } +} + +impl std::error::Error for WriteError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Io { source, .. } => Some(source), + Self::Malformed(failure) | Self::Rejected(failure) => Some(failure), + _ => None, + } + } +} + +/// A fingerprint of file content, for telling our own write apart from a +/// person's edit. +/// +/// FNV-1a over the bytes, plus their length, rather than SHA-256. The property +/// needed is "are these the bytes I just wrote", against an accidental +/// collision and not against an adversary: anyone who could craft a colliding +/// settings file already has permission to write the settings file, so a +/// cryptographic hash buys nothing here and costs a dependency. `std`'s +/// `DefaultHasher` is excluded for the opposite reason — it is documented as +/// unstable across releases, and a fingerprint is only useful if it means the +/// same thing every time. Comparing the length as well makes an accidental +/// collision require agreement on both. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +struct Fingerprint { + length: usize, + hash: u64, +} + +impl Fingerprint { + const OFFSET_BASIS: u64 = 0xcbf2_9ce4_8422_2325; + const PRIME: u64 = 0x0000_0100_0000_01b3; + + fn of(bytes: &[u8]) -> Self { + let mut hash = Self::OFFSET_BASIS; + for byte in bytes { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(Self::PRIME); + } + Self { + length: bytes.len(), + hash, + } + } +} + +/// The settings file, and the memory of what we last wrote to it. +/// +/// Holds no configuration of its own: [`Self::load`] hands back content and the +/// caller resolves it against the environment. What it does own is the +/// fingerprint of the last write, which is the only state that cannot be +/// recomputed from the filesystem. +#[derive(Clone, Debug)] +pub struct SettingsFile { + path: PathBuf, + last_written: Option, +} + +impl SettingsFile { + /// Opens the settings file named by `paths`. + /// + /// Touches nothing on disk; the file may not exist yet. + #[must_use] + pub fn at(paths: &impl SettingsPaths) -> Self { + Self::at_path(paths.settings_file()) + } + + /// Opens a settings file at an exact path. + #[must_use] + pub fn at_path(path: PathBuf) -> Self { + Self { + path, + last_written: None, + } + } + + /// The file being read and written. + #[must_use] + pub fn path(&self) -> &Path { + &self.path + } + + /// The directory a watcher should watch. + /// + /// The parent, not the file: an atomic write replaces the file by rename, + /// so a watch on the inode stops seeing anything after the first write. + #[must_use] + pub fn watch_directory(&self) -> Option<&Path> { + self.path.parent() + } + + /// Reads the file. + #[must_use] + pub fn load(&self) -> LoadOutcome { + load_from(&self.path) + } + + /// Whether these bytes are the ones this instance last wrote. + /// + /// The watcher's first question, asked before any parse or reload. Answers + /// `false` until something has been written, and keeps answering `true` for + /// as long as the content matches — one write can produce several watcher + /// events, so this must not be a one-shot token. Bytes that match a write + /// we made are bytes identical to what we already believe, so suppressing a + /// person who happened to type the file back to exactly that costs nothing. + #[must_use] + pub fn is_own_echo(&self, bytes: &[u8]) -> bool { + self.last_written == Some(Fingerprint::of(bytes)) + } + + /// Writes the commented reference file if there is nothing there yet. + /// + /// Returns whether it created the file. Creating it records the echo + /// fingerprint, because the watcher will report the creation and that + /// report is ours. + /// + /// The check is a plain `exists` and not an atomic create-if-absent. The + /// only writer that can lose that race is a second Lumbridge starting at + /// the same moment, and it would write byte-identical content. + /// + /// # Errors + /// + /// [`WriteError::Io`] if the directory cannot be created or the file cannot + /// be written. An existing file — valid or not — is left exactly alone. + pub fn create_from_default_if_absent(&mut self) -> Result { + if self.path.exists() { + return Ok(false); + } + self.publish(DEFAULT_TOML)?; + Ok(true) + } + + /// Sets one setting. + /// + /// # Errors + /// + /// See [`WriteError`]. Every variant leaves the file untouched. + pub fn set( + &mut self, + path: &str, + value: SettingValue, + origin: WriteOrigin, + ) -> Result<(), WriteError> { + self.apply(vec![(path.to_owned(), value)], origin) + } + + /// Sets several settings as one write. + /// + /// One document, one check, one rename, one watcher echo — rather than a + /// burst of writes that a debounced watcher would coalesce into a reload + /// whose fingerprint matches only the last of them. + /// + /// # Errors + /// + /// See [`WriteError`]. If any edit is refused, none of them are applied. + pub fn apply( + &mut self, + edits: Vec<(String, SettingValue)>, + origin: WriteOrigin, + ) -> Result<(), WriteError> { + let mut text = self.read_or_seed()?; + // Checked before the edit rather than only after it, so that a file + // that was already broken is reported as the person's typo instead of + // as our edit having broken it. + if let Err(error) = crate::parse(&text) { + return Err(WriteError::Malformed(ParseFailure::from_toml( + &error, &text, + ))); + } + // Authorise the whole batch before touching anything, so that a refused + // edit halfway down cannot leave the earlier ones applied. + let authorised = edits + .into_iter() + .map(|(path, value)| authorise(&path, origin).map(|item| (item, value))) + .collect::, _>>()?; + for (item, _) in &authorised { + if let Some(revived) = revive_placeholder(&text, item.path) { + text = revived; + } + } + let mut document = text + .parse::() + .map_err(|error| WriteError::Malformed(ParseFailure::from_toml_edit(&error, &text)))?; + for (item, value) in authorised { + set_in_document(&mut document, item.path, value)?; + } + let rendered = document.to_string(); + if let Err(error) = crate::parse(&rendered) { + return Err(WriteError::Rejected(ParseFailure::from_toml( + &error, &rendered, + ))); + } + self.publish(&rendered) + } + + /// The text to edit: what is on disk, or the reference file if nothing is. + /// + /// Seeding from the reference file rather than from an empty string means a + /// person who first changes a setting in the pane and then opens the file + /// finds it documented, instead of finding one bare line. + fn read_or_seed(&self) -> Result { + match fs::read_to_string(&self.path) { + Ok(text) => Ok(text), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(DEFAULT_TOML.to_owned()), + Err(source) => Err(WriteError::Io { + path: self.path.clone(), + source, + }), + } + } + + fn publish(&mut self, text: &str) -> Result<(), WriteError> { + write_atomically(&self.path, text.as_bytes()).map_err(|source| WriteError::Io { + path: self.path.clone(), + source, + })?; + self.last_written = Some(Fingerprint::of(text.as_bytes())); + Ok(()) + } +} + +/// Checks that this path is writable at all, and by this origin. +fn authorise(path: &str, origin: WriteOrigin) -> Result<&'static SettingItem, WriteError> { + if let Some((_, reason)) = FORBIDDEN_PATHS.iter().find(|(known, _)| *known == path) { + return Err(WriteError::Forbidden { + path: path.to_owned(), + reason, + }); + } + let item = SETTINGS + .iter() + .find(|item| item.path == path) + .ok_or_else(|| WriteError::UnknownPath(path.to_owned()))?; + if origin.satisfies(item.authority) { + Ok(item) + } else { + Err(WriteError::AuthorityRefused { + path: path.to_owned(), + required: item.authority, + origin, + }) + } +} + +/// Turns `#theme = "lumbridge-slate"` back into a live `theme` key, in place. +/// +/// The reference file documents each setting as a commented-out line directly +/// below the prose that explains it. Letting `toml_edit` append a live +/// `theme = "nord"` elsewhere in the section would leave the file arguing with +/// itself: one active value, one commented value for the same key, and the +/// explanation attached to the wrong one. Reviving the placeholder puts the +/// chosen value exactly where the file says that key belongs. +/// +/// Returns `None` — leaving the append to `toml_edit` — when the key is already +/// live in that section or has no placeholder. The convention is the one stated +/// at the top of the reference file: `#key = value` is a commented-out setting, +/// `# a sentence` is prose, and only the first is ever touched. +fn revive_placeholder(text: &str, path: &str) -> Option { + let (section, key) = path.split_once('.')?; + let mut current = ""; + let mut offset = 0; + let mut marker = None; + for line in text.split_inclusive('\n') { + let start = offset; + offset += line.len(); + let trimmed = line.trim(); + if let Some(name) = trimmed + .strip_prefix('[') + .and_then(|rest| rest.strip_suffix(']')) + { + current = name; + } else if current == section { + if assignment_to(trimmed, key) { + // Already live; editing its value in place is `toml_edit`'s job. + return None; + } + if marker.is_none() + && trimmed + .strip_prefix('#') + .is_some_and(|rest| assignment_to(rest, key)) + { + marker = Some(start + line.find('#')?); + } + } + } + // Deleting the single `#` rather than rebuilding the line keeps the + // indentation and the line ending exactly as the file had them; rebuilding + // from `lines()` would quietly rewrite a CRLF file as LF. + let marker = marker?; + Some(format!("{}{}", &text[..marker], &text[marker + 1..])) +} + +/// Whether this line assigns to exactly `key`, and not to a key it prefixes. +fn assignment_to(line: &str, key: &str) -> bool { + line.strip_prefix(key) + .is_some_and(|rest| rest.trim_start().starts_with('=')) +} + +/// Writes one dotted path into the document, keeping everything around it. +fn set_in_document( + document: &mut DocumentMut, + path: &str, + value: SettingValue, +) -> Result<(), WriteError> { + let (section, key) = path + .split_once('.') + .ok_or_else(|| WriteError::UnknownPath(path.to_owned()))?; + let table = section_table(document, section)?; + // The value's decor is its surrounding whitespace and any comment on the + // same line. Replacing the item would drop both, so a file written as + // theme = "gruvbox" # the one that matches my terminal + // would lose the note explaining the choice the moment the theme changed. + let existing = table + .get(key) + .and_then(Item::as_value) + .map(|value| value.decor().clone()); + let mut item = value.into_item(); + if let (Some(decor), Some(new)) = (existing, item.as_value_mut()) { + *new.decor_mut() = decor; + } + // `Table::insert` reformats the key, which erases the comment block sitting + // above it; the entry API keeps the key exactly as the file spelled it. + match table.entry(key) { + toml_edit::Entry::Occupied(mut occupied) => { + occupied.insert(item); + } + toml_edit::Entry::Vacant(vacant) => { + vacant.insert(item); + } + } + Ok(()) +} + +/// Finds or creates the `[section]` table. +fn section_table<'document>( + document: &'document mut DocumentMut, + section: &str, +) -> Result<&'document mut Table, WriteError> { + let item = document + .entry(section) + .or_insert_with(|| Item::Table(Table::new())); + let table = item.as_table_mut().ok_or_else(|| WriteError::NotASection { + section: section.to_owned(), + })?; + // A table created on the way to a key is implicit and renders as nothing at + // all, so the key would be emitted at the top level under a bare name and + // land in a different place on the next read. + table.set_implicit(false); + Ok(table) +} + +/// Writes `bytes` to `path` so that no reader ever sees a partial file. +/// +/// A temporary file in the *same* directory — a rename across filesystems is +/// not atomic and `/tmp` is very often a different filesystem — then fsync, then +/// rename over the destination. A reader either sees the whole old file or the +/// whole new one. Truncating the real file and writing into it would expose an +/// empty settings file to anyone who read during the window, and to everyone +/// forever if the machine lost power inside it. +fn write_atomically(path: &Path, bytes: &[u8]) -> io::Result<()> { + let directory = path.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + "a settings file must have a parent directory", + ) + })?; + fs::create_dir_all(directory)?; + let (temporary, mut file) = create_private_file(directory)?; + let written = file + .write_all(bytes) + .and_then(|()| file.sync_all()) + .and_then(|()| { + drop(file); + fs::rename(&temporary, path) + }); + if let Err(error) = written { + // The temporary file is ours and half-written; leaving it behind would + // accumulate one per failure in the user's config directory. + drop(fs::remove_file(&temporary)); + return Err(error); + } + sync_directory(directory) +} + +/// Creates a uniquely named, owner-only file in `directory`. +/// +/// Named for the settings file it will become, and hidden, so that a person who +/// finds one after a crash can tell what it was. +fn create_private_file(directory: &Path) -> io::Result<(PathBuf, fs::File)> { + let mut last = None; + for attempt in 0..32_u32 { + let candidate = directory.join(format!( + ".settings.toml.{}.{attempt}.tmp", + std::process::id() + )); + match private_options().open(&candidate) { + Ok(file) => return Ok((candidate, file)), + // `create_new` failing this way means a previous run died here or a + // second Lumbridge is writing; try the next name rather than + // truncating a file we did not create. + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => last = Some(error), + Err(error) => return Err(error), + } + } + Err(last.unwrap_or_else(|| { + io::Error::new( + io::ErrorKind::AlreadyExists, + "no free temporary name in the settings directory", + ) + })) +} + +/// Open options that create a file only the owner can read. +/// +/// The settings file is not a secret, but it names paths on this machine and it +/// sits in a directory that is ours alone; mode 0600 is what the rest of a +/// user's config directory looks like and there is no reason to be looser. +#[cfg(unix)] +fn private_options() -> fs::OpenOptions { + use std::os::unix::fs::OpenOptionsExt as _; + + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true).mode(0o600); + options +} + +/// The same, on a platform with no Unix mode bits. +/// +/// The file inherits the directory's access control, which on Windows means the +/// user profile's. Reported honestly rather than pretending 0600 was applied. +#[cfg(not(unix))] +fn private_options() -> fs::OpenOptions { + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true); + options +} + +/// Flushes the rename itself. +/// +/// The rename is already atomic with respect to readers; this is about the +/// power cut afterwards, where an unsynced directory entry can leave the old +/// file back in place. Nothing is ever half-written either way. +#[cfg(unix)] +fn sync_directory(directory: &Path) -> io::Result<()> { + fs::File::open(directory)?.sync_all() +} + +/// The same, where opening a directory as a file is not allowed. +#[cfg(not(unix))] +fn sync_directory(_directory: &Path) -> io::Result<()> { + Ok(()) +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::{Path, PathBuf}; + + use tempfile::TempDir; + + use super::{ + DEFAULT_TOML, Fingerprint, LoadOutcome, ParseFailure, SettingValue, SettingsFile, + WriteError, WriteOrigin, create_private_file, load_from, + }; + use crate::{FORBIDDEN_PATHS, MapEnv, Origin, SETTINGS, Settings, WriteAuthority}; + + /// A settings file in a directory that disappears with the test. + struct Scratch { + directory: TempDir, + } + + impl Scratch { + fn new() -> Self { + Self { + directory: TempDir::new().expect("a temporary directory"), + } + } + + fn path(&self) -> PathBuf { + self.directory.path().join("settings.toml") + } + + fn file(&self) -> SettingsFile { + SettingsFile::at_path(self.path()) + } + + fn seed(&self, text: &str) -> SettingsFile { + fs::write(self.path(), text).expect("writing the fixture"); + self.file() + } + + fn text(&self) -> String { + fs::read_to_string(self.path()).expect("reading the file back") + } + } + + fn configure(file: &mut SettingsFile, path: &str, value: SettingValue) { + file.set(path, value, WriteOrigin::Configure) + .unwrap_or_else(|error| panic!("{path} should have been writable: {error}")); + } + + // ---- loading ----------------------------------------------------------- + + #[test] + fn a_file_that_is_not_there_is_not_an_error() { + let scratch = Scratch::new(); + assert!( + matches!(load_from(&scratch.path()), LoadOutcome::Absent), + "a machine nobody has configured is configured correctly, and must \ + not be reported as a failure" + ); + } + + #[test] + fn a_loaded_file_carries_both_what_this_build_knows_and_what_it_does_not() { + let scratch = Scratch::new(); + scratch.seed("[appearance]\ntheme = \"gruvbox\"\nsparkles = true\n"); + let LoadOutcome::Loaded { content, unknown } = load_from(&scratch.path()) else { + panic!("a valid file must load"); + }; + assert_eq!( + content.appearance.theme.as_deref(), + Some("gruvbox"), + "the known key must reach the caller" + ); + assert_eq!( + unknown + .iter() + .map(|key| key.path.as_str()) + .collect::>(), + vec!["appearance.sparkles"], + "a key from a newer build is reported, not dropped in silence" + ); + } + + #[test] + fn a_typo_is_reported_with_its_line_and_never_as_defaults() { + let scratch = Scratch::new(); + scratch.seed("[appearance]\ntheme = \"ok\"\n\n[usage\ncodex_app_server = true\n"); + let LoadOutcome::Malformed(failure) = load_from(&scratch.path()) else { + panic!( + "a typo must be an error: falling back to defaults would silently \ + discard the whole configuration" + ); + }; + assert_eq!( + failure.line, + Some(4), + "the banner has to name the line the person must go and fix" + ); + assert!( + !failure.message.is_empty(), + "a banner with no message tells nobody anything" + ); + } + + #[test] + fn an_unreported_position_renders_as_unknown_rather_than_as_line_one() { + let failure = ParseFailure { + message: "something went wrong".to_owned(), + line: None, + column: None, + }; + assert_eq!( + failure.to_string(), + "at a position the parser did not report: something went wrong", + "pointing at line 1 would send someone to a line that is not the problem" + ); + } + + #[test] + fn a_file_that_cannot_be_read_is_not_confused_with_one_that_is_not_there() { + let scratch = Scratch::new(); + fs::create_dir(scratch.path()).expect("a directory where the file should be"); + assert!( + matches!(load_from(&scratch.path()), LoadOutcome::Unreadable { .. }), + "treating an unreadable file as absent would quietly replace a \ + configuration the user still has" + ); + } + + // ---- comments and order survive --------------------------------------- + + const HAND_WRITTEN: &str = "\ +# what I actually run + +[appearance] +# this one matches my terminal +theme = \"gruvbox-dark\" +accent = \"amber\" + +[usage] +codex_app_server = false +sparkles = \"from a build newer than this one\" +"; + + #[test] + fn a_commented_file_keeps_its_comments_after_a_programmatic_edit() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("catppuccin-mocha".to_owned()), + ); + let text = scratch.text(); + assert!( + text.contains("# what I actually run"), + "a comment above the first section is the file's title and must \ + survive an edit; got:\n{text}" + ); + assert!( + text.contains("# this one matches my terminal"), + "the comment explaining a choice must outlive the choice; got:\n{text}" + ); + assert!( + text.contains("theme = \"catppuccin-mocha\""), + "the edit must actually have happened; got:\n{text}" + ); + } + + #[test] + fn key_order_is_unchanged_by_an_edit() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("catppuccin-mocha".to_owned()), + ); + let text = scratch.text(); + let theme = text.find("theme =").expect("theme is still there"); + let accent = text.find("accent =").expect("accent is still there"); + assert!( + theme < accent, + "a file re-serialised from a parsed model comes back in the \ + schema's order, not the person's; got:\n{text}" + ); + } + + #[test] + fn a_key_this_build_does_not_understand_survives_a_write_by_this_build() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + configure( + &mut file, + "usage.codex_app_server", + SettingValue::Flag(true), + ); + assert!( + scratch + .text() + .contains("sparkles = \"from a build newer than this one\""), + "an older build must not delete a newer build's settings, or \ + downgrading once loses them permanently" + ); + } + + #[test] + fn a_note_written_beside_a_value_outlives_the_value_changing() { + let scratch = Scratch::new(); + let mut file = scratch.seed("[appearance]\ntheme = \"gruvbox\" # chosen deliberately\n"); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let text = scratch.text(); + assert!( + text.contains("# chosen deliberately"), + "an inline note is the reason for the setting, not decoration; got:\n{text}" + ); + assert!(text.contains("theme = \"nord\""), "got:\n{text}"); + } + + #[test] + fn a_setting_the_file_did_not_mention_is_added_to_its_section() { + let scratch = Scratch::new(); + let mut file = scratch.seed("[appearance]\ntheme = \"nord\"\n"); + configure( + &mut file, + "usage.codex_app_server", + SettingValue::Flag(false), + ); + let LoadOutcome::Loaded { content, .. } = file.load() else { + panic!("the file must still load after an edit"); + }; + assert_eq!( + content.usage.codex_app_server, + Some(false), + "a section the file never had must be created rather than dropped" + ); + assert_eq!( + content.appearance.theme.as_deref(), + Some("nord"), + "and the sections it did have must survive" + ); + } + + #[test] + fn a_commented_out_default_becomes_the_live_key_where_the_file_documents_it() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let text = scratch.text(); + assert!( + text.contains("theme = \"nord\""), + "the chosen value must be live; got:\n{text}" + ); + assert!( + !text.contains("#theme = "), + "leaving the commented default behind makes the file argue with \ + itself: one active value, one commented value, and the \ + explanation attached to the wrong one; got:\n{text}" + ); + let explanation = text + .find("# The syntax theme every interface colour") + .expect("the prose is still there"); + let live = text.find("theme = \"nord\"").expect("the value is there"); + assert!( + explanation < live, + "the value must stay directly under the paragraph explaining it" + ); + } + + #[test] + fn reviving_a_placeholder_changes_nothing_but_the_comment_marker() { + let before = "[appearance]\r\n#theme = \"lumbridge-slate\"\r\n"; + let revived = super::revive_placeholder(before, "appearance.theme") + .expect("the placeholder is there to revive"); + assert_eq!( + revived, "[appearance]\r\ntheme = \"lumbridge-slate\"\r\n", + "rebuilding the file from its lines would silently rewrite a CRLF \ + file as LF, which is a change nobody asked for" + ); + } + + #[test] + fn a_placeholder_for_a_longer_key_is_not_mistaken_for_this_one() { + let text = "[appearance]\n#theme_variant = \"dark\"\n"; + assert!( + super::revive_placeholder(text, "appearance.theme").is_none(), + "a key that another key merely starts with is a different key" + ); + } + + #[test] + fn a_placeholder_in_another_section_is_left_alone() { + let text = "[usage]\n#theme = \"nord\"\n\n[appearance]\n"; + assert!( + super::revive_placeholder(text, "appearance.theme").is_none(), + "keys are dotted paths, and the section half of the path counts" + ); + } + + #[test] + fn prose_that_happens_to_look_like_an_assignment_is_left_as_prose() { + let text = "[appearance]\n# theme = whatever you like\n"; + assert!( + super::revive_placeholder(text, "appearance.theme").is_none(), + "'# words' is a sentence and '#key = value' is a setting; \ + uncommenting a sentence would corrupt the file" + ); + } + + // ---- write authority --------------------------------------------------- + + #[test] + fn an_agent_cannot_write_a_field_that_names_a_path() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + let refused = file.set( + "usage.claude_rate_limit_feed", + SettingValue::Text("/somewhere/an/agent/chose".to_owned()), + WriteOrigin::Configure, + ); + assert!( + matches!( + refused, + Err(WriteError::AuthorityRefused { + required: WriteAuthority::Human, + .. + }) + ), + "decision 0006 guarantees a layout-only agent cannot choose what \ + Lumbridge reads or runs; got {refused:?}" + ); + assert_eq!( + scratch.text(), + HAND_WRITTEN, + "a refused write must be refused, not downgraded and not partial" + ); + } + + #[test] + fn a_person_may_write_a_field_an_agent_may_not() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + file.set( + "usage.claude_rate_limit_feed", + SettingValue::Text("/home/example/feed.jsonl".to_owned()), + WriteOrigin::Human, + ) + .expect("a person at the keyboard may set a path"); + assert!(scratch.text().contains("/home/example/feed.jsonl")); + } + + #[test] + fn a_path_that_must_never_be_a_setting_is_refused_at_the_write() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + for (path, reason) in FORBIDDEN_PATHS { + let refused = file.set( + path, + SettingValue::Text("anything".to_owned()), + WriteOrigin::Human, + ); + assert!( + matches!(refused, Err(WriteError::Forbidden { .. })), + "{path} must be refused by name and not merely absent: {reason}" + ); + } + assert_eq!(scratch.text(), HAND_WRITTEN, "and nothing may be written"); + } + + #[test] + fn a_path_this_build_has_no_setting_for_is_refused() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + let refused = file.set( + "appearance.sparkles", + SettingValue::Flag(true), + WriteOrigin::Human, + ); + assert!( + matches!(refused, Err(WriteError::UnknownPath(_))), + "writing a key with no schema would produce a file that reports \ + itself unknown on the next load; got {refused:?}" + ); + } + + #[test] + fn one_refused_edit_in_a_batch_applies_none_of_them() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + let refused = file.apply( + vec![ + ( + "appearance.theme".to_owned(), + SettingValue::Text("nord".to_owned()), + ), + ( + "usage.claude_rate_limit_feed".to_owned(), + SettingValue::Text("/agent/chose/this".to_owned()), + ), + ], + WriteOrigin::Configure, + ); + assert!(refused.is_err(), "the batch names a human-only field"); + assert_eq!( + scratch.text(), + HAND_WRITTEN, + "half an applied batch is a configuration nobody asked for" + ); + } + + // ---- refusing to damage a file ---------------------------------------- + + #[test] + fn a_file_with_a_typo_in_it_is_never_rewritten() { + let broken = "[appearance\ntheme = \"gruvbox\"\n# a comment worth keeping\n"; + let scratch = Scratch::new(); + let mut file = scratch.seed(broken); + let refused = file.set( + "appearance.theme", + SettingValue::Text("nord".to_owned()), + WriteOrigin::Human, + ); + assert!( + matches!(refused, Err(WriteError::Malformed(_))), + "rewriting from a parsed model would fix the bracket by throwing \ + away everything else in the file; got {refused:?}" + ); + assert_eq!(scratch.text(), broken, "the person's file is left alone"); + } + + #[test] + fn a_file_whose_value_has_the_wrong_type_is_reported_rather_than_repaired() { + // Valid TOML, invalid against the schema: `parse` rejects it, so an + // edit must not quietly drop the offending key on its way past. + let wrong = "[usage]\ncodex_app_server = \"yes please\"\n"; + let scratch = Scratch::new(); + let mut file = scratch.seed(wrong); + let refused = file.set( + "appearance.theme", + SettingValue::Text("nord".to_owned()), + WriteOrigin::Human, + ); + assert!( + matches!(refused, Err(WriteError::Malformed(_))), + "got {refused:?}" + ); + assert_eq!(scratch.text(), wrong); + } + + // ---- atomic write ------------------------------------------------------ + + #[test] + fn the_temporary_file_shares_a_directory_with_its_destination() { + let scratch = Scratch::new(); + let (temporary, _handle) = + create_private_file(scratch.directory.path()).expect("a temporary file"); + assert_eq!( + temporary.parent(), + Some(scratch.directory.path()), + "a rename across filesystems is not atomic, so the temporary file \ + cannot live in the system temporary directory" + ); + } + + #[test] + fn a_write_leaves_no_temporary_file_behind() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let names: Vec = fs::read_dir(scratch.directory.path()) + .expect("listing the settings directory") + .map(|entry| { + entry + .expect("an entry") + .file_name() + .to_string_lossy() + .into_owned() + }) + .collect(); + assert_eq!( + names, + vec!["settings.toml".to_owned()], + "a temporary file per write would accumulate in the user's config \ + directory forever" + ); + } + + #[cfg(unix)] + #[test] + fn a_written_file_is_readable_only_by_its_owner() { + use std::os::unix::fs::PermissionsExt as _; + + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let mode = fs::metadata(scratch.path()) + .expect("the written file") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o600, + "the settings file names paths on this machine and sits in a \ + directory that is ours alone" + ); + } + + // ---- seeding from the reference file ----------------------------------- + + #[test] + fn a_missing_file_is_seeded_with_the_documented_reference_file() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + assert!( + file.create_from_default_if_absent() + .expect("seeding a fresh directory"), + "it must report that it created the file" + ); + assert_eq!( + scratch.text(), + DEFAULT_TOML, + "the file a person opens is the reference file, comments and all" + ); + } + + #[test] + fn an_existing_file_is_never_replaced_by_the_reference_file() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + assert!( + !file + .create_from_default_if_absent() + .expect("an existing file"), + "it must report that it created nothing" + ); + assert_eq!(scratch.text(), HAND_WRITTEN); + } + + #[test] + fn a_first_edit_writes_a_documented_file_rather_than_one_bare_line() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let text = scratch.text(); + assert!( + text.contains("Lumbridge settings."), + "changing a setting in the pane must not produce a file that \ + documents nothing; got:\n{text}" + ); + assert!(text.contains("theme = \"nord\""), "got:\n{text}"); + } + + // ---- echo suppression -------------------------------------------------- + + #[test] + fn nothing_is_an_echo_before_anything_has_been_written() { + let scratch = Scratch::new(); + let file = scratch.seed(HAND_WRITTEN); + assert!( + !file.is_own_echo(HAND_WRITTEN.as_bytes()), + "a file we never wrote is somebody else's, however familiar it looks" + ); + } + + #[test] + fn the_bytes_we_just_wrote_are_recognised_as_our_own_echo() { + let scratch = Scratch::new(); + let mut file = scratch.seed(HAND_WRITTEN); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + assert!( + file.is_own_echo(scratch.text().as_bytes()), + "without this the watcher reloads our own write, which schedules \ + another write, forever" + ); + } + + #[test] + fn an_echo_is_recognised_every_time_because_one_write_raises_several_events() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let written = scratch.text(); + for event in 0..3 { + assert!( + file.is_own_echo(written.as_bytes()), + "event {event} of one rename must still be recognised; a \ + one-shot token would let create-then-modify through" + ); + } + } + + #[test] + fn a_hand_edit_is_not_mistaken_for_our_own_echo() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let edited = format!("{}\n# and then I changed my mind\n", scratch.text()); + assert!( + !file.is_own_echo(edited.as_bytes()), + "suppressing a person's edit is worse than the loop it prevents" + ); + } + + #[test] + fn only_the_most_recent_write_echoes() { + let scratch = Scratch::new(); + let mut file = scratch.file(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("nord".to_owned()), + ); + let first = scratch.text(); + configure( + &mut file, + "appearance.theme", + SettingValue::Text("gruvbox".to_owned()), + ); + assert!(file.is_own_echo(scratch.text().as_bytes())); + assert!( + !file.is_own_echo(first.as_bytes()), + "content we overwrote reappearing means somebody restored it, and \ + that is news" + ); + } + + #[test] + fn a_fingerprint_covers_length_as_well_as_content() { + assert_ne!( + Fingerprint::of(b"theme"), + Fingerprint::of(b"theme\n"), + "two files that differ only by a trailing newline are two files" + ); + assert_eq!( + Fingerprint::of(b"theme = \"nord\""), + Fingerprint::of(b"theme = \"nord\""), + "the same bytes must fingerprint the same on every run, which is \ + why this is not std's DefaultHasher" + ); + } + + // ---- the reference file is the documentation -------------------------- + + /// The settings the reference file spells out, as dotted paths. + /// + /// `#key = value` is a commented-out setting; `# a sentence` is prose. The + /// space after the `#` is the whole difference, and it is stated at the top + /// of the file itself. + fn documented_paths() -> Vec { + let mut section = String::new(); + let mut paths = Vec::new(); + for line in DEFAULT_TOML.lines() { + let line = line.trim(); + if let Some(name) = line + .strip_prefix('[') + .and_then(|rest| rest.strip_suffix(']')) + { + section = name.to_owned(); + } else if let Some(setting) = commented_setting(line) { + let key = setting.split('=').next().unwrap_or_default().trim(); + paths.push(format!("{section}.{key}")); + } + } + paths + } + + fn commented_setting(line: &str) -> Option<&str> { + let rest = line.strip_prefix('#')?; + // Prose is `# words`; a setting is `#key = value`. + if rest.starts_with(' ') || !rest.contains('=') { + None + } else { + Some(rest) + } + } + + #[test] + fn the_reference_file_documents_every_setting_this_build_has() { + let documented = documented_paths(); + for item in SETTINGS { + assert!( + documented.iter().any(|path| path == item.path), + "{} exists but the reference file never mentions it, so the \ + file that is supposed to be the documentation is not", + item.path + ); + } + } + + #[test] + fn the_reference_file_documents_nothing_this_build_does_not_have() { + for path in documented_paths() { + assert!( + SETTINGS.iter().any(|item| item.path == path), + "the reference file documents {path}, which Lumbridge would \ + ignore — documentation of a key that does nothing is a lie" + ); + } + } + + #[test] + fn uncommenting_the_reference_file_produces_a_document_this_build_understands() { + let mut text = String::new(); + for line in DEFAULT_TOML.lines() { + if let Some(setting) = commented_setting(line.trim()) { + text.push_str(setting); + } else if !line.trim_start().starts_with('#') { + text.push_str(line); + } + text.push('\n'); + } + let (_, unknown) = crate::parse(&text).unwrap_or_else(|error| { + panic!("the documented example values must be valid: {error}\n{text}") + }); + assert!( + unknown.is_empty(), + "every documented key must land somewhere this build reads: {unknown:?}" + ); + } + + #[test] + fn the_reference_file_as_shipped_chooses_nothing() { + let (content, unknown) = crate::parse(DEFAULT_TOML).expect("the reference file is valid"); + assert!(unknown.is_empty(), "unrecognised: {unknown:?}"); + let settings = Settings::resolve(&content, unknown, &MapEnv::default()); + assert_eq!( + settings.theme.origin, + Origin::Default, + "the reference file ships with every key commented out; an active \ + key would make the pane report a value as user-chosen on a \ + machine nobody had configured" + ); + assert_eq!(settings.accent.origin, Origin::Default); + assert_eq!(settings.claude_transcripts.origin, Origin::Default); + assert_eq!(settings.allow_osc52_clipboard.origin, Origin::Default); + } + + #[test] + fn the_reference_file_never_names_a_path_that_must_not_be_a_setting() { + for (path, reason) in FORBIDDEN_PATHS { + assert!( + !DEFAULT_TOML.contains(path), + "the reference file names {path}; someone reading it would \ + believe they could set it, and uncommenting it would be an \ + invitation: {reason}" + ); + } + } + + #[test] + fn the_watcher_is_pointed_at_the_directory_and_not_at_the_file() { + let scratch = Scratch::new(); + let file = scratch.file(); + assert_eq!( + file.watch_directory(), + Some(scratch.directory.path()), + "an atomic write replaces the file by rename, so a watch on the \ + old inode stops seeing anything after the first write" + ); + } + + #[test] + fn a_settings_file_reports_the_path_it_was_opened_on() { + let scratch = Scratch::new(); + assert_eq!(scratch.file().path(), Path::new(&scratch.path())); + } +} diff --git a/crates/lumbridge-settings/src/lib.rs b/crates/lumbridge-settings/src/lib.rs index 133d1b8..586990f 100644 --- a/crates/lumbridge-settings/src/lib.rs +++ b/crates/lumbridge-settings/src/lib.rs @@ -27,14 +27,27 @@ //! file that can redirect where an access token is sent is a credential //! exfiltration path with a friendly name. +//! +//! **The file is edited, never regenerated.** [`file`] rewrites the one key +//! that changed through `toml_edit`, so a hand-written file keeps its comments, +//! its ordering, and any key a newer build wrote that this one does not +//! understand. `assets/default.toml` is the reference file, and it is the +//! documentation: a test asserts it names every setting in [`SETTINGS`]. + #![forbid(unsafe_code)] +pub mod file; pub mod paths; use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; +pub use file::{ + DEFAULT_TOML, LoadOutcome, ParseFailure, SettingValue, SettingsFile, WriteError, WriteOrigin, + load, +}; + /// Who is allowed to write a field. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum WriteAuthority { diff --git a/crates/lumbridge-storage/src/lib.rs b/crates/lumbridge-storage/src/lib.rs index 8fc7626..371c388 100644 --- a/crates/lumbridge-storage/src/lib.rs +++ b/crates/lumbridge-storage/src/lib.rs @@ -1,8 +1,23 @@ //! Local, device-owned persistence for Lumbridge. //! -//! This database stores workspace metadata and remote routing profiles. API -//! keys, subscription credentials, SSH private keys, and Tailscale credentials -//! are intentionally outside its contract. +//! This database stores workspace metadata, remote routing profiles, and the +//! durable interface state described below. API keys, subscription credentials, +//! SSH private keys, and Tailscale credentials are intentionally outside its +//! contract. +//! +//! # Interface state is not configuration +//! +//! Two kinds of value survive a restart and they do not live in the same place. +//! If a person would want it in a dotfile and copied to their other machine, it +//! is configuration and belongs in `settings.toml`, which `lumbridge-settings` +//! owns. If losing it is *annoying rather than wrong* — the sidebar comes back +//! at its default width instead of the width you dragged it to — it is +//! interface state and belongs here, in a file that is device-local by design +//! and never synchronised. +//! +//! That is what [`UiStateKey`] and [`Store::set_ui_state`] are for. Decision +//! 0021 listed sidebar width, collapsed sections and visibility as the first +//! three, under "Not in this pass". use std::path::Path; @@ -10,21 +25,36 @@ use lumbridge_core::{RemoteHost, RemoteTransport}; use rusqlite::{Connection, OptionalExtension, params}; /// The schema version this build writes and understands. -pub const SUPPORTED_SCHEMA_VERSION: u32 = 2; +pub const SUPPORTED_SCHEMA_VERSION: u32 = 3; -/// The baseline schema. +/// The version-stamp table, created before anything else can be decided. +/// +/// [`Store::migrate`] has to read the stamp before it applies a single +/// statement, so this one table cannot be part of [`SCHEMA`] — it must already +/// exist to answer the question that chooses between creating and migrating. +/// It is a named constant so that the table a v2 file already has and the table +/// a fresh file gets are the same text rather than two copies that can drift. +const APP_META_TABLE: &str = r" +CREATE TABLE IF NOT EXISTS app_meta ( + key TEXT PRIMARY KEY NOT NULL, + value TEXT NOT NULL +) STRICT; +"; + +/// The baseline schema, as of version 2. /// /// Creation only. It no longer stamps the version, because it used to do so /// unconditionally inside the same batch as the `CREATE TABLE IF NOT EXISTS` /// statements — so opening an older file added no columns but flipped the stamp /// forward anyway, and opening a *newer* file silently stamped it back down. /// Both produced a database whose recorded version was a lie. +/// +/// It is deliberately frozen at version 2. Everything a later version adds is +/// its own constant, applied by the fresh-create path and by the migration step +/// that earns it — the *same* constant in both, which is what makes "created +/// fresh" and "upgraded from v2" produce an identical schema instead of two +/// that merely look alike. const SCHEMA: &str = r" -CREATE TABLE IF NOT EXISTS app_meta ( - key TEXT PRIMARY KEY NOT NULL, - value TEXT NOT NULL -) STRICT; - CREATE TABLE IF NOT EXISTS remote_hosts ( id TEXT PRIMARY KEY NOT NULL, label TEXT NOT NULL, @@ -90,6 +120,116 @@ CREATE TABLE IF NOT EXISTS workspace_snapshots ( ) STRICT; "; +/// The `ui_state` table, added at schema version 3. +/// +/// Run exactly once per database: either by the fresh-create path or by the +/// v2 → v3 step, never both. Unlike [`SCHEMA`] it is therefore *not* +/// `IF NOT EXISTS`. A file that already has something called `ui_state` at the +/// moment we would create it is not the file we think it is, and adopting an +/// object of unknown shape because its name happened to match is exactly the +/// class of quiet lie the version stamp was rebuilt to stop. Failing here rolls +/// the whole migration back and leaves the file at the version it really is. +/// +/// `PRIMARY KEY NOT NULL` because `SQLite` still admits a NULL into a `TEXT` +/// primary key otherwise, and a row nobody can name is a row nobody can delete. +/// +/// The `CHECK` is a backstop, not the cap. [`Store::set_ui_state`] rejects in +/// bytes, and `length()` counts characters, so the Rust check is always the +/// stricter of the two and always fires first; this one exists so a value +/// cannot grow without bound through some future path that writes the table +/// directly. +const UI_STATE_TABLE: &str = r" +CREATE TABLE ui_state ( + key TEXT PRIMARY KEY NOT NULL, + value TEXT NOT NULL CHECK (length(value) <= 16384), + updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP +) STRICT; +"; + +/// The largest value [`Store::set_ui_state`] will store, in bytes. +/// +/// Interface state is a width, a flag, a short list of collapsed section names. +/// Sixteen kibibytes is far more than any of those and far less than anything +/// that would make this table a place to park a transcript, a scrollback buffer +/// or a serialised workspace — all of which have their own homes, with their +/// own retention. +pub const MAX_UI_STATE_VALUE_BYTES: usize = 16 * 1024; + +// [`UI_STATE_TABLE`] has shipped, so the 16384 in its CHECK can never be +// edited. Raising the cap above it would turn a clean refusal naming the key +// into a bare constraint violation from SQLite, so the drift is caught here, at +// compile time, rather than by whoever writes the first oversized value. +const _: () = { + const { assert!(MAX_UI_STATE_VALUE_BYTES <= 16384) }; +}; + +/// The complete set of durable interface-state keys. +/// +/// Closed on purpose. The obvious design is a free-form `&str` key, and it +/// means a typo is a silent miss at runtime — you read `sidebar.wdith`, get +/// `None`, and fall back to the default forever without anything going wrong +/// loudly enough to notice. Here a typo does not compile, and the stored names +/// live in one place where a rename is visibly a schema change. +/// +/// Values are opaque strings at this boundary. Storage does not know that a +/// width is a number or that a collapsed-section set is a list; the caller +/// serialises and parses, and must treat an unparseable value as absent rather +/// than as a zero. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub enum UiStateKey { + /// How wide the sidebar was, in pixels, when the drag was released. + SidebarWidth, + /// Whether the sidebar was showing at all. + SidebarVisible, + /// Which sidebar sections the user had folded away. + SidebarCollapsedSections, +} + +impl UiStateKey { + /// Every key, for exhaustive tests and for dumping the table. + /// + /// A `match` in [`Self::storage_key`] makes a *new* key impossible to + /// forget; this array makes it impossible to forget here too, because + /// adding a variant without extending it fails the length assertion in the + /// round-trip test. + pub const ALL: [Self; 3] = [ + Self::SidebarWidth, + Self::SidebarVisible, + Self::SidebarCollapsedSections, + ]; + + /// The name this key is stored under. + /// + /// Part of the on-disk format: changing one of these strings orphans the + /// value a user already has, so it is a migration, not a rename. + #[must_use] + pub const fn storage_key(self) -> &'static str { + match self { + Self::SidebarWidth => "sidebar.width", + Self::SidebarVisible => "sidebar.visible", + Self::SidebarCollapsedSections => "sidebar.collapsed_sections", + } + } + + /// Recover a key from its stored name, or `None` if this build has no such + /// key. + /// + /// `None` is the honest answer for a row written by a newer build: an + /// unknown key is left alone rather than guessed at or deleted. + #[must_use] + pub fn from_storage_key(value: &str) -> Option { + Self::ALL + .into_iter() + .find(|candidate| candidate.storage_key() == value) + } +} + +impl std::fmt::Display for UiStateKey { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(self.storage_key()) + } +} + #[derive(Debug)] pub enum StorageError { Database(rusqlite::Error), @@ -111,6 +251,17 @@ pub enum StorageError { MissingMigration { from: u32, }, + /// A durable interface-state value exceeded [`MAX_UI_STATE_VALUE_BYTES`]. + /// + /// Refused whole rather than truncated. A width or a list of collapsed + /// sections cut off in the middle would come back as unparseable at best + /// and as a plausible wrong value at worst, and this table is the one place + /// where losing state is supposed to be merely annoying. + UiStateValueTooLarge { + key: UiStateKey, + bytes: usize, + limit: usize, + }, } impl std::fmt::Display for StorageError { @@ -122,6 +273,10 @@ impl std::fmt::Display for StorageError { formatter, "no migration is defined from schema {from}; this build cannot upgrade the file" ), + Self::UiStateValueTooLarge { key, bytes, limit } => write!( + formatter, + "interface state for {key} is {bytes} bytes, over the {limit}-byte limit" + ), Self::SchemaTooNew { found, supported } => write!( formatter, "this workspace was written by a newer Lumbridge (schema {found}; this build understands {supported})" @@ -140,6 +295,12 @@ impl From for StorageError { pub type Result = std::result::Result; +/// The columns every `remote_hosts` query selects, in order. +/// +/// Named so that the two queries share one reader: a `SELECT` list and a +/// positional `row.get(n)` that disagree is a bug no type checks. +type RemoteHostColumns = (String, String, String, Option, Option, String); + /// The one `SQLite` connection owned by a runtime instance. pub struct Store { connection: Connection, @@ -181,12 +342,7 @@ impl Store { /// The version is read *before* anything is applied, which is the whole /// point: a migration has to know where it is starting from. fn migrate(connection: &Connection) -> Result<()> { - connection.execute_batch( - "CREATE TABLE IF NOT EXISTS app_meta ( - key TEXT PRIMARY KEY NOT NULL, - value TEXT NOT NULL - ) STRICT;", - )?; + connection.execute_batch(APP_META_TABLE)?; let found: Option = connection .query_row( "SELECT value FROM app_meta WHERE key = 'schema_version'", @@ -211,7 +367,15 @@ impl Store { connection.execute_batch("BEGIN IMMEDIATE;")?; let applied = (|| -> Result<()> { if found.is_none() { + // A fresh file is created at the current version directly + // rather than by replaying every step: the steps exist to move + // files that already hold data, and replaying them on an empty + // database is a slower way to reach the same place with more + // chances to diverge. What keeps the two paths honest is that + // each version's tables are one constant, used here and by the + // step below. connection.execute_batch(SCHEMA)?; + connection.execute_batch(UI_STATE_TABLE)?; } // Ordered, forward-only steps. Each takes the database from the // version named to the next one; none may be edited once shipped. @@ -235,14 +399,17 @@ impl Store { /// One forward step, from the named version to the next. /// - /// There are none yet: version 2 is the baseline. An unhandled step is an - /// error rather than a silent success, so raising + /// Version 2 is the baseline; `from = 2` adds the `ui_state` table. An + /// unhandled step is an error rather than a silent success, so raising /// [`SUPPORTED_SCHEMA_VERSION`] without writing the step that earns it /// fails at the first open instead of stamping a version the file has not /// actually reached. A step must never be edited once it has shipped. - fn migration_step(_connection: &Connection, from: u32) -> Result<()> { - // 1 => connection.execute_batch("ALTER TABLE …")?, - Err(StorageError::MissingMigration { from }) + fn migration_step(connection: &Connection, from: u32) -> Result<()> { + match from { + 2 => connection.execute_batch(UI_STATE_TABLE)?, + _ => return Err(StorageError::MissingMigration { from }), + } + Ok(()) } /// Return the currently installed schema version. @@ -301,32 +468,122 @@ impl Store { .query_row( "SELECT id, label, hostname, username, port, transport FROM remote_hosts WHERE id = ?1", [id], - |row| { - Ok(( - row.get::<_, String>(0)?, - row.get::<_, String>(1)?, - row.get::<_, String>(2)?, - row.get::<_, Option>(3)?, - row.get::<_, Option>(4)?, - row.get::<_, String>(5)?, - )) - }, + Self::remote_host_columns, ) .optional()?; - row.map(|(id, label, hostname, username, port, transport)| { - let transport = RemoteTransport::from_storage_name(&transport) - .ok_or_else(|| StorageError::UnknownTransport(transport.clone()))?; - Ok(RemoteHost { - id, - label, - hostname, - username, - port, - transport, - }) + row.map(Self::remote_host_from_columns).transpose() + } + + /// Load every saved remote host profile, ordered by label. + /// + /// Ordered here rather than by the caller so that two lists of the same + /// hosts are never in two different orders. `NOCASE` sorts `alpha` beside + /// `Alpha` instead of putting every capital letter first, which is what a + /// person reading a list of their own machines expects; it folds ASCII + /// only, so a non-ASCII label sorts by code point. The identifier breaks + /// ties, because two hosts may share a label and a list that reshuffles + /// between reads is a list you cannot click. + /// + /// A row naming a transport this build does not know is an error for the + /// whole call rather than a silently dropped host: a devices list that is + /// quietly missing an entry is worse than one that refuses to render. + /// + /// # Errors + /// + /// Returns an error when a row cannot be read or names a transport this + /// build does not understand. + pub fn remote_hosts(&self) -> Result> { + let mut statement = self.connection.prepare( + "SELECT id, label, hostname, username, port, transport + FROM remote_hosts + ORDER BY label COLLATE NOCASE, id", + )?; + let rows = statement.query_map([], Self::remote_host_columns)?; + + let mut hosts = Vec::new(); + for row in rows { + hosts.push(Self::remote_host_from_columns(row?)?); + } + Ok(hosts) + } + + /// The raw column tuple, shared so one query cannot drift from the other. + fn remote_host_columns(row: &rusqlite::Row<'_>) -> rusqlite::Result { + Ok(( + row.get(0)?, + row.get(1)?, + row.get(2)?, + row.get(3)?, + row.get(4)?, + row.get(5)?, + )) + } + + fn remote_host_from_columns(columns: RemoteHostColumns) -> Result { + let (id, label, hostname, username, port, transport) = columns; + let transport = RemoteTransport::from_storage_name(&transport) + .ok_or(StorageError::UnknownTransport(transport))?; + Ok(RemoteHost { + id, + label, + hostname, + username, + port, + transport, }) - .transpose() + } + + /// Read one piece of durable interface state, if it has ever been written. + /// + /// `None` means "never written", and the caller must answer it with its own + /// default rather than with a zero. The value is an opaque string; this + /// boundary does not know what a sidebar width looks like. + /// + /// # Errors + /// + /// Returns an error when `SQLite` cannot read the row. + pub fn ui_state(&self, key: UiStateKey) -> Result> { + self.connection + .query_row( + "SELECT value FROM ui_state WHERE key = ?1", + [key.storage_key()], + |row| row.get(0), + ) + .optional() + .map_err(StorageError::from) + } + + /// Write one piece of durable interface state, replacing any previous + /// value. + /// + /// The caller owns the encoding. Storage stores strings, so that adding a + /// field to the sidebar's own types is not a schema change here. + /// + /// # Errors + /// + /// Returns [`StorageError::UiStateValueTooLarge`] when the value exceeds + /// [`MAX_UI_STATE_VALUE_BYTES`], in which case nothing is written, and a + /// database error when `SQLite` cannot store the row. + pub fn set_ui_state(&self, key: UiStateKey, value: &str) -> Result<()> { + if value.len() > MAX_UI_STATE_VALUE_BYTES { + return Err(StorageError::UiStateValueTooLarge { + key, + bytes: value.len(), + limit: MAX_UI_STATE_VALUE_BYTES, + }); + } + self.connection.execute( + r" + INSERT INTO ui_state (key, value) + VALUES (?1, ?2) + ON CONFLICT(key) DO UPDATE SET + value = excluded.value, + updated_at = CURRENT_TIMESTAMP + ", + params![key.storage_key(), value], + )?; + Ok(()) } /// Save a credential-free UI snapshot for a workspace. @@ -380,19 +637,72 @@ impl Store { } Ok(false) } + + /// Every object the file declares, as `(type, name, sql)`, name-ordered. + /// + /// Ordered by name rather than by creation order precisely because the two + /// paths under test create the same objects in different orders. + #[cfg(test)] + fn schema_objects(&self) -> Result)>> { + let mut statement = self + .connection + .prepare("SELECT type, name, sql FROM sqlite_master ORDER BY name")?; + let rows = statement.query_map([], |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)))?; + rows.collect::>>() + .map_err(StorageError::from) + } } #[cfg(test)] mod tests { + use std::path::Path; + use lumbridge_core::{RemoteHost, RemoteTransport}; - use super::{SUPPORTED_SCHEMA_VERSION, StorageError, Store}; + use super::{ + APP_META_TABLE, MAX_UI_STATE_VALUE_BYTES, SCHEMA, SUPPORTED_SCHEMA_VERSION, StorageError, + Store, UiStateKey, + }; use rusqlite::Connection; + /// Build the file a version-2 build would have left behind. + /// + /// Deliberately assembled from the same constants that build shipped — + /// [`APP_META_TABLE`] then [`SCHEMA`], stamped by hand — so that "upgraded + /// from v2" in these tests means the real thing and not a v3 file with a + /// table removed. + fn version_two_database(path: &Path) { + let connection = Connection::open(path).expect("a v2 file should open"); + connection + .execute_batch(APP_META_TABLE) + .expect("the stamp table"); + connection.execute_batch(SCHEMA).expect("the v2 baseline"); + connection + .execute( + "INSERT INTO app_meta (key, value) VALUES ('schema_version', '2')", + [], + ) + .expect("stamp version 2"); + } + + fn a_host(id: &str, label: &str) -> RemoteHost { + RemoteHost { + id: id.to_owned(), + label: label.to_owned(), + hostname: format!("{id}.example"), + username: None, + port: None, + transport: RemoteTransport::OpenSsh, + } + } + #[test] fn initializes_schema_and_round_trips_remote_host() { let store = Store::open_in_memory().expect("store should initialize"); - assert_eq!(store.schema_version().expect("schema version"), 2); + assert_eq!( + store.schema_version().expect("schema version"), + SUPPORTED_SCHEMA_VERSION + ); let host = RemoteHost { id: "metal".into(), @@ -535,4 +845,383 @@ mod tests { SUPPORTED_SCHEMA_VERSION ); } + + #[test] + fn a_version_two_database_is_upgraded_in_place_and_keeps_its_rows() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let path = directory.path().join("workspace.db"); + version_two_database(&path); + + let host = a_host("build", "Build box"); + { + let connection = Connection::open(&path).expect("the v2 file"); + connection + .execute( + "INSERT INTO remote_hosts (id, label, hostname, transport) + VALUES (?1, ?2, ?3, 'openssh')", + rusqlite::params![host.id, host.label, host.hostname], + ) + .expect("a row written by the older build"); + } + + let store = Store::open(&path).expect("a v2 file should upgrade, not be refused"); + assert_eq!( + store.schema_version().expect("version"), + 3, + "the upgraded file must claim the version it actually reached" + ); + assert_eq!( + store.remote_host("build").expect("host should load"), + Some(host), + "a migration must carry the user's existing rows forward, not reset them" + ); + + store + .set_ui_state(UiStateKey::SidebarWidth, "312") + .expect("the migrated file should have ui_state"); + assert_eq!( + store.ui_state(UiStateKey::SidebarWidth).expect("read back"), + Some("312".to_owned()) + ); + } + + /// The two ways to reach version 3 must arrive at the same place. + /// + /// A fresh database is created directly rather than by replaying steps, so + /// nothing but this test stops the creation path and the migration path + /// from drifting into two schemas that merely look alike — the failure + /// mode being a column that exists only on machines old enough to have + /// been upgraded. + #[test] + fn an_upgraded_database_has_the_same_schema_as_a_freshly_created_one() { + let directory = tempfile::tempdir().expect("a temporary directory"); + + let fresh_path = directory.path().join("fresh.db"); + let fresh = Store::open(&fresh_path).expect("a fresh store"); + + let upgraded_path = directory.path().join("upgraded.db"); + version_two_database(&upgraded_path); + let upgraded = Store::open(&upgraded_path).expect("an upgraded store"); + + assert_eq!( + fresh.schema_objects().expect("fresh schema"), + upgraded.schema_objects().expect("upgraded schema"), + "creating a v3 file and upgrading a v2 file must produce one schema" + ); + } + + /// The half-way case: the step applied, and then the run stopped. + /// + /// This is the one the transaction exists for. The step's `CREATE TABLE` + /// succeeds and the stamp that would record it does not, which without a + /// transaction leaves a file carrying a v3 table under a v2 stamp — a lie + /// in the other direction, and one that would make the next open try the + /// step again and fail forever. The abort is forced by a temporary trigger + /// so that the failure lands between the two writes, where no natural error + /// could be aimed. + #[test] + fn a_migration_interrupted_after_its_step_rolls_the_step_back_too() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let path = directory.path().join("workspace.db"); + version_two_database(&path); + + { + let connection = Connection::open(&path).expect("the v2 file"); + connection + .execute_batch( + "CREATE TEMP TRIGGER stop_the_stamp BEFORE UPDATE ON main.app_meta + BEGIN SELECT RAISE(ABORT, 'interrupted'); END;", + ) + .expect("a trigger that stops the run between its two writes"); + assert!( + Store::migrate(&connection).is_err(), + "a migration whose stamp cannot be written must not report success" + ); + } + + let connection = Connection::open(&path).expect("reopen"); + let stamp: String = connection + .query_row( + "SELECT value FROM app_meta WHERE key = 'schema_version'", + [], + |row| row.get(0), + ) + .expect("a stamp"); + assert_eq!( + stamp, "2", + "the file is still a version 2 file and must say so" + ); + let tables: i64 = connection + .query_row( + "SELECT count(*) FROM sqlite_master WHERE type = 'table' AND name = 'ui_state'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + tables, 0, + "a version 2 stamp over a version 3 table is exactly the half-way state the transaction prevents" + ); + } + + /// An interrupted migration must leave the file at the version it was. + /// + /// The interruption is a real one rather than a simulated panic: a file + /// that already holds an object called `ui_state` makes the v2 → v3 step + /// fail before it can apply. What matters is what is left behind — a file + /// still stamped 2, with its own `ui_state` untouched, that a fixed build + /// can try again on. + #[test] + fn a_migration_that_cannot_apply_leaves_the_file_at_version_two() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let path = directory.path().join("workspace.db"); + version_two_database(&path); + { + let connection = Connection::open(&path).expect("the v2 file"); + connection + .execute_batch("CREATE TABLE ui_state (something_else TEXT NOT NULL) STRICT;") + .expect("an object in the way"); + } + + let Err(error) = Store::open(&path) else { + panic!("a migration that cannot complete must not report success"); + }; + assert!( + matches!(error, StorageError::Database(_)), + "expected the failing statement to surface, got {error}" + ); + + let connection = Connection::open(&path).expect("reopen"); + let stamp: String = connection + .query_row( + "SELECT value FROM app_meta WHERE key = 'schema_version'", + [], + |row| row.get(0), + ) + .expect("a stamp"); + assert_eq!( + stamp, "2", + "a rolled-back migration must leave the file at the version it really is" + ); + + let column: String = connection + .query_row( + "SELECT name FROM pragma_table_info('ui_state')", + [], + |row| row.get(0), + ) + .expect("the pre-existing table"); + assert_eq!( + column, "something_else", + "the rollback must undo the migration's own work, not the file's" + ); + } + + #[test] + fn ui_state_is_absent_until_written_and_then_round_trips() { + let store = Store::open_in_memory().expect("store should initialize"); + assert_eq!( + store + .ui_state(UiStateKey::SidebarCollapsedSections) + .expect("read"), + None, + "a key nobody has written is unknown, and the caller supplies the default" + ); + + store + .set_ui_state(UiStateKey::SidebarCollapsedSections, "detached,quota") + .expect("write"); + assert_eq!( + store + .ui_state(UiStateKey::SidebarCollapsedSections) + .expect("read back") + .as_deref(), + Some("detached,quota") + ); + } + + #[test] + fn writing_a_key_twice_replaces_it_rather_than_failing() { + let store = Store::open_in_memory().expect("store should initialize"); + store + .set_ui_state(UiStateKey::SidebarVisible, "false") + .expect("first write"); + store + .set_ui_state(UiStateKey::SidebarVisible, "true") + .expect("a second write must update the row, not collide with it"); + assert_eq!( + store.ui_state(UiStateKey::SidebarVisible).expect("read"), + Some("true".to_owned()), + "the last write wins; interface state has no history" + ); + } + + #[test] + fn keys_are_independent_of_one_another() { + let store = Store::open_in_memory().expect("store should initialize"); + store + .set_ui_state(UiStateKey::SidebarWidth, "240") + .expect("write"); + assert_eq!( + store.ui_state(UiStateKey::SidebarVisible).expect("read"), + None, + "writing one key must not appear to have written another" + ); + } + + #[test] + fn a_value_at_the_cap_is_stored_and_a_larger_one_is_refused_whole() { + let store = Store::open_in_memory().expect("store should initialize"); + let at_cap = "x".repeat(MAX_UI_STATE_VALUE_BYTES); + store + .set_ui_state(UiStateKey::SidebarCollapsedSections, &at_cap) + .expect("the cap itself is allowed; the limit is inclusive"); + + let over_cap = "y".repeat(MAX_UI_STATE_VALUE_BYTES + 1); + let Err(error) = store.set_ui_state(UiStateKey::SidebarCollapsedSections, &over_cap) else { + panic!("a value over the cap must be refused"); + }; + assert!( + matches!( + error, + StorageError::UiStateValueTooLarge { + key: UiStateKey::SidebarCollapsedSections, + bytes, + limit: MAX_UI_STATE_VALUE_BYTES, + } if bytes == MAX_UI_STATE_VALUE_BYTES + 1 + ), + "expected UiStateValueTooLarge naming the key and the size, got {error}" + ); + assert_eq!( + store + .ui_state(UiStateKey::SidebarCollapsedSections) + .expect("read back"), + Some(at_cap), + "a refused write must leave the previous value intact, not truncate over it" + ); + } + + /// The cap counts bytes, so a multi-byte value cannot slip past it. + #[test] + fn the_cap_counts_bytes_rather_than_characters() { + let store = Store::open_in_memory().expect("store should initialize"); + // Four-byte characters: a quarter of the cap in characters, but one + // byte over it once encoded. + let value = "𝄞".repeat(MAX_UI_STATE_VALUE_BYTES / 4 + 1); + assert!( + store + .set_ui_state(UiStateKey::SidebarWidth, &value) + .is_err(), + "a value whose character count is under the cap but whose byte count is over it must still be refused" + ); + } + + #[test] + fn ui_state_keys_round_trip_through_their_stored_names() { + assert_eq!( + UiStateKey::ALL.len(), + 3, + "a new key must be added to ALL, or nothing here covers it" + ); + for key in UiStateKey::ALL { + assert_eq!( + UiStateKey::from_storage_key(key.storage_key()), + Some(key), + "the stored name of {key} is part of the on-disk format" + ); + } + assert_eq!( + UiStateKey::from_storage_key("sidebar.wdith"), + None, + "a name this build does not know is unknown, never the nearest match" + ); + } + + #[test] + fn remote_hosts_is_empty_rather_than_an_error_when_none_are_saved() { + let store = Store::open_in_memory().expect("store should initialize"); + assert_eq!( + store.remote_hosts().expect("listing"), + Vec::new(), + "no saved hosts is a fact, not a failure" + ); + } + + #[test] + fn remote_hosts_are_listed_ordered_by_label() { + let store = Store::open_in_memory().expect("store should initialize"); + for (id, label) in [ + ("c", "zenith"), + ("a", "Build box"), + ("b", "apex"), + ("d", "apex"), + ] { + store.save_remote_host(&a_host(id, label)).expect("save"); + } + + let labels: Vec<(String, String)> = store + .remote_hosts() + .expect("listing") + .into_iter() + .map(|host| (host.label, host.id)) + .collect(); + assert_eq!( + labels, + vec![ + ("apex".to_owned(), "b".to_owned()), + ("apex".to_owned(), "d".to_owned()), + ("Build box".to_owned(), "a".to_owned()), + ("zenith".to_owned(), "c".to_owned()), + ], + "labels sort case-insensitively, and a shared label is broken by identifier so the order never reshuffles" + ); + } + + /// A host we cannot route to must not be silently omitted from the list. + #[test] + fn a_host_with_an_unreadable_transport_fails_the_listing_rather_than_vanishing() { + let directory = tempfile::tempdir().expect("a temporary directory"); + let path = directory.path().join("workspace.db"); + { + let store = Store::open(&path).expect("a fresh store"); + store + .save_remote_host(&a_host("a", "Build box")) + .expect("save"); + } + { + // This build's CHECK constraint keeps an unknown transport out, so + // the fixture has to be the schema a *newer* build would have left: + // the constraint widened, and a row written against it. A separate + // connection then reads the amended schema. + let connection = Connection::open(&path).expect("the file"); + connection + .execute_batch( + "PRAGMA writable_schema = ON; + UPDATE sqlite_master + SET sql = replace(sql, \"transport IN ('openssh', 'tailscale-ssh')\", \"1\") + WHERE name = 'remote_hosts'; + PRAGMA writable_schema = OFF;", + ) + .expect("widen the constraint the way a newer schema would"); + } + { + let connection = Connection::open(&path).expect("the amended file"); + connection + .execute( + "INSERT INTO remote_hosts (id, label, hostname, transport) + VALUES ('z', 'Zebra', 'z.example', 'quantum-ssh')", + [], + ) + .expect("a row from a build that knows more transports than this one"); + } + + let store = Store::open(&path).expect("reopen"); + let Err(error) = store.remote_hosts() else { + panic!("an unknown transport must be reported, not skipped"); + }; + assert!( + matches!(error, StorageError::UnknownTransport(ref name) if name == "quantum-ssh"), + "expected UnknownTransport naming the value, got {error}" + ); + } }