diff --git a/crates/vibescrobble-mcp/build.rs b/crates/vibescrobble-mcp/build.rs new file mode 100644 index 00000000..19505d77 --- /dev/null +++ b/crates/vibescrobble-mcp/build.rs @@ -0,0 +1,45 @@ +//! Records where this binary came from, so a doctor can say whether the host +//! that is running is the one the current checkout built. +//! +//! The same reasoning as `vibescrobble-hookd`'s build script, for a different +//! reason: this crate compiles the scrobble tool's description in, so editing +//! that prompt needs a rebuild *and* a restart of the host, and a developer +//! who has done neither has no way to tell which they are waiting on. +//! +//! A failed lookup is not an error. A build from a tarball, or with no `git` +//! on the path, reports `unknown`. + +use std::process::Command; + +fn main() { + println!("cargo:rerun-if-changed=build.rs"); + // Via git rather than a guessed `../../.git/HEAD`: in a worktree `.git` is + // a file naming a directory elsewhere, so the guess watches nothing and + // the stamp silently never moves again. + for path in ["HEAD", "logs/HEAD"] { + if let Some(resolved) = git(&["rev-parse", "--git-path", path]) { + if std::path::Path::new(&resolved).exists() { + println!("cargo:rerun-if-changed={resolved}"); + } + } + } + + let describe = git(&["describe", "--always", "--dirty", "--abbrev=12"]).unwrap_or_else(unknown); + println!("cargo:rustc-env=VIBESCROBBLE_BUILD_COMMIT={describe}"); +} + +/// Runs git and returns its trimmed output, or `None` for anything unusual. +fn git(args: &[&str]) -> Option { + let output = Command::new("git").args(args).output().ok()?; + if !output.status.success() { + return None; + } + let text = String::from_utf8(output.stdout).ok()?; + let text = text.trim(); + (!text.is_empty()).then(|| text.to_owned()) +} + +/// What a build from a tarball, or with no `git` on the path, reports. +fn unknown() -> String { + "unknown".to_owned() +} diff --git a/crates/vibescrobble-mcp/src/lib.rs b/crates/vibescrobble-mcp/src/lib.rs index f0f36e8c..935c9b58 100644 --- a/crates/vibescrobble-mcp/src/lib.rs +++ b/crates/vibescrobble-mcp/src/lib.rs @@ -1265,12 +1265,120 @@ pub const MCP_PATH: &str = "/mcp"; /// Split out of the binary so a test can exercise the mounted route without /// binding a socket. pub fn http_router(server: ScrobbleServer) -> Router { + let pds = server.pds().base_url().to_string(); let service = StreamableHttpService::new( move || Ok(server.clone()), Arc::new(LocalSessionManager::default()), StreamableHttpServerConfig::default(), ); - Router::new().nest_service(MCP_PATH, service) + Router::new() + .route(HEALTH_PATH, axum::routing::get(move || health(pds.clone()))) + .nest_service(MCP_PATH, service) +} + +/// The path a running host answers questions about itself on. +/// +/// Deliberately outside the protocol. The Model Context Protocol has no way to +/// ask a server what it was built from, and the question is not the model's — +/// it is the developer's, and it is the one a doctor has to be able to ask +/// without speaking a session handshake to get an answer. +pub const HEALTH_PATH: &str = "/health"; + +/// What this host is, for whoever is asking why it is behaving oddly. +/// +/// # Why the tool description is a digest +/// +/// [`SCROBBLE_INSTRUCTIONS`] is compiled in. It is served as the tool +/// description and quoted into the hook's start-of-session context from one +/// copy, so changing it is a rebuild rather than a restart — and a harness +/// reads a tool description when the session starts, so a rebuilt host does +/// not reach a model that is already running. +/// +/// Two facts and neither is obvious from outside, so a developer who edits the +/// prompt and sees no change has no way to tell which of the two they are +/// waiting on. Reporting a digest lets `vibescrobble-setup check` compare the +/// host that is running against the checkout in front of them and say so. +/// +/// A digest rather than the text: the description is long, this is a line in a +/// doctor's output, and the only question being asked is whether two copies +/// are the same. +async fn health(pds: String) -> axum::Json { + axum::Json(serde_json::json!({ + "pds": pds, + "build": BUILD_COMMIT, + "instructions": instructions_digest(), + })) +} + +/// What this binary was built from, as `git describe` saw it. +pub const BUILD_COMMIT: &str = env!("VIBESCROBBLE_BUILD_COMMIT"); + +/// A short, stable fingerprint of the tool description this host serves. +/// +/// FNV-1a rather than a cryptographic hash: nothing here is defended against +/// an adversary choosing a collision, the two things being compared are two +/// builds of the same string, and a dependency for it would be a dependency +/// for a diagnostic. +pub fn instructions_digest() -> String { + digest(tool_description()) +} + +/// FNV-1a over some text, rendered as sixteen hex digits. +fn digest(text: &str) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in text.as_bytes() { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x1000_0000_01b3); + } + format!("{hash:016x}") +} + +#[cfg(test)] +mod prompt_digest { + //! What the digest has to do, and the one thing it does not. + + use super::*; + + #[test] + fn the_same_text_always_digests_the_same() { + assert_eq!( + digest("a scrobble is a status update"), + digest("a scrobble is a status update") + ); + assert_eq!(instructions_digest(), instructions_digest()); + } + + #[test] + fn a_changed_prompt_changes_the_digest() { + // The whole purpose: an edit to the description has to be visible from + // outside the process serving it. + let before = digest(tool_description()); + let after = digest(&format!("{} And one more sentence.", tool_description())); + assert_ne!(before, after); + } + + #[test] + fn a_digest_is_short_enough_for_one_line_of_a_doctor() { + let rendered = instructions_digest(); + assert_eq!(rendered.len(), 16, "{rendered}"); + assert!( + rendered + .chars() + .all(|c| c.is_ascii_hexdigit() && !c.is_uppercase()), + "{rendered}" + ); + } + + /// It is not a security boundary and is not claimed to be one. + /// + /// Nothing here is defended against somebody choosing a collision: the two + /// things being compared are two builds of one string, and a doctor that + /// pulled in a cryptographic hash for that would be paying a dependency + /// for a diagnostic. + #[test] + fn the_digest_is_not_claimed_to_resist_anything() { + assert_ne!(digest(""), digest("\0")); + } } #[cfg(test)] diff --git a/crates/vibescrobble-setup/Cargo.toml b/crates/vibescrobble-setup/Cargo.toml index 52b0a33a..5c2203f3 100644 --- a/crates/vibescrobble-setup/Cargo.toml +++ b/crates/vibescrobble-setup/Cargo.toml @@ -17,6 +17,7 @@ time.workspace = true tokio.workspace = true vibescrobble-hook.workspace = true vibescrobble-hookd.workspace = true +vibescrobble-mcp.workspace = true vibescrobble-stack.workspace = true [lints] diff --git a/crates/vibescrobble-setup/README.md b/crates/vibescrobble-setup/README.md index db202519..f7b59992 100644 --- a/crates/vibescrobble-setup/README.md +++ b/crates/vibescrobble-setup/README.md @@ -153,6 +153,23 @@ vibescrobble-setup debug # what it is now it started with, so the terminals need restarting; the hook is a fresh process per event and needs nothing. +## Which of three steps you are waiting on + +The scrobble tool's description is compiled in. Editing it needs a rebuild, and +a restart of the host, and a new session — a harness reads a tool description +once, when the session opens. Three steps, none of them visible, and a +developer who edits the prompt and sees nothing change has no way to tell which +one they are waiting on. + +So the host reports a digest of what it serves, and `check` compares it against +what this checkout compiles: + +```text + ok scrobble host build mcp was built from 68efa7d and serves this checkout's tool description + WARN scrobble host build mcp was built from a1b2c3d and serves a different tool description + than this checkout compiles. Restart scripts/dev-mcp.sh… +``` + ## Verify, rather than "the port answered" `verify` provisions an agent through the same client the hook uses, writes a diff --git a/crates/vibescrobble-setup/src/check.rs b/crates/vibescrobble-setup/src/check.rs index cb4fa665..3a67f07a 100644 --- a/crates/vibescrobble-setup/src/check.rs +++ b/crates/vibescrobble-setup/src/check.rs @@ -82,6 +82,7 @@ pub async fn run(dir: &Path, stack: &Stack, config_path: &Path) -> Findings { findings.push(hook_binary(dir)); findings.extend(wiring(dir, stack)); findings.extend(transport(dir, stack)); + findings.extend(prompt_text(dir, stack).await); findings.extend(stale_accounts(dir, stack).await); findings } @@ -149,7 +150,6 @@ async fn services(dir: &Path, stack: &Stack) -> Findings { // the wrong path would report a live service as dead. let path = match service.kind { ServiceKind::Web => "/", - ServiceKind::Mcp => "/mcp", _ => "/health", }; let url = format!("{}{path}", service.url()); @@ -453,6 +453,58 @@ fn transport(dir: &Path, stack: &Stack) -> Findings { Findings::new() } +/// Whether the running scrobble host serves this checkout's prompt. +/// +/// The tool description is compiled in, so editing it needs a rebuild *and* a +/// restart of the host — and a harness reads a tool description when a session +/// starts, so a rebuilt host still does not reach a model already running. +/// Three steps, none of them visible, and a developer who edits the prompt and +/// sees no change has no way to tell which one they are waiting on. +/// +/// So the host reports a digest of what it serves and this compares it against +/// what this checkout compiles. +async fn prompt_text(dir: &Path, stack: &Stack) -> Findings { + let Some((name, service)) = stack.mcp_for(dir) else { + return Findings::new(); + }; + let Ok(response) = client() + .get(format!("{}/health", service.url())) + .send() + .await + else { + // Already reported as a service that is not answering. + return Findings::new(); + }; + let Ok(body) = response.json::().await else { + return vec![Finding::new( + Level::Warn, + "scrobble host build", + format!("{name} is answering but older than its /health endpoint — restart it"), + )]; + }; + + let running = body["instructions"].as_str().unwrap_or(""); + let built = body["build"].as_str().unwrap_or("unknown"); + if running == vibescrobble_mcp::instructions_digest() { + return vec![Finding::ok( + "scrobble host build", + format!("{name} was built from {built} and serves this checkout's tool description"), + )]; + } + vec![Finding::new( + Level::Warn, + "scrobble host build", + format!( + concat!( + "{} was built from {} and serves a different tool description than this ", + "checkout compiles. Restart scripts/dev-mcp.sh to rebuild it, and start a ", + "new session: a harness reads a tool description once, when it opens" + ), + name, built + ), + )] +} + /// Whether the accounts on file still exist on the server they name. /// /// The one failure the hook cannot detect for itself. Restarting a development diff --git a/plan/dev-setup.md b/plan/dev-setup.md index f39560ab..aa4f6a0a 100644 --- a/plan/dev-setup.md +++ b/plan/dev-setup.md @@ -97,10 +97,6 @@ session register itself would let a model claim to be any session. ## The parts that are not the server -- [ ] **Prompt text is compiled in.** `SCROBBLE_INSTRUCTIONS` is served as the - tool description and quoted into injected context from one copy, so - changing it is a rebuild, and the harness reads the description when the - session starts. Say which of those two a change needs. ## Working in it @@ -111,6 +107,13 @@ session register itself would let a model claim to be any session. ## Done +- [x] **Prompt text is compiled in, and the doctor says which step you are + waiting on.** Editing `SCROBBLE_INSTRUCTIONS` needs a rebuild, a restart + of the scrobble host, and a new session, because a harness reads a tool + description once when the session opens. None of the three is visible, so + the host reports a digest of what it serves on `/health` beside what it + was built from, and `check` compares that against what this checkout + compiles. - [x] **The three tables are written a flag at a time.** `service add`, `profile new`, `bind`, `unbind` and `show`, and nothing writes a configuration that `check` would then report as broken — a file naming a