From fb5564fd94dad227ae36096090b525b9468bc71c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 16:57:57 +0000 Subject: [PATCH] Fix the weak spots from the review: secrets, tool reach, silent successes - A ./.clank/config.toml found in the working directory may only name each section's own key variable (CLANK_API_KEY, BRAVE_API_KEY, TAVILY_API_KEY); another name needs --config or CLANK_CONFIG. A cloned repo could otherwise send any environment secret to an endpoint it also chose. - clank-jev no longer takes its endpoint, model or key from [clank]; only [clank].timeout. The chat base_url was being used as the Jev URL. - Tool paths are confined to the working directory; read_file refuses non-regular files and files over 4 MB, and counts lines like wc. - A stream that ends without finish_reason or [DONE] is a failure (exit 1); non-JSON and error events mid-stream are reported, not skipped. - A -c context tree whose file leaf cannot be read fails the run instead of sending "[context error: ...]" to the model; a node with file plus text/children is rejected. - clank-web --fetch drops a character split by the 512 KiB cap instead of failing the whole page. - stat reports symlinks, search stops at exactly 500 matches, invalid tool arguments come back to the model as an error, a trailing slash in base_url works, credentials in base_url are redacted from traces and errors, --show-thinking prints no stray blank line, clank-jev checks --min-prob. - The pre-push hook says that it sends diffs to a third-party provider. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_019RkP7NfSreh9VQeWSk47hb --- .githooks/pre-push | 5 ++ PROTOCOL.md | 21 ++++--- STATUS.md | 5 +- docs/clank-jev.md | 16 ++--- docs/clank-web.md | 12 ++-- src/client.rs | 35 ++++++++--- src/config.rs | 45 +++++++++++++- src/context.rs | 73 ++++++++++++++++------- src/jev.rs | 47 ++++++++------- src/main.rs | 51 +++++++++++++--- src/tools.rs | 142 ++++++++++++++++++++++++++++++++++++++++++--- src/web.rs | 8 +++ tests/config.rs | 41 +++++++++++++ tests/web.rs | 30 ++++++++++ tests/wire.rs | 49 +++++++++++++++- 15 files changed, 489 insertions(+), 91 deletions(-) diff --git a/.githooks/pre-push b/.githooks/pre-push index 6c40a52..804f4cb 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -8,6 +8,11 @@ # Enable once per clone: # git config core.hooksPath .githooks # +# Data leaves the machine: with a TypeSafe or OpenRouter key set, the pushed +# diff and commit message are sent to that provider on every push. Only a local +# kev on 127.0.0.1:8009 keeps them here. Do not enable this on a repository whose +# code may not go to a third party. +# # git hands pre-push " " lines # on stdin. The push is read from the first line; a delete (all-zero local) and # a run with nothing on stdin are both nothing to decide about. diff --git a/PROTOCOL.md b/PROTOCOL.md index 18821e9..58adf42 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -51,11 +51,13 @@ two-channel contract. `--no-tools` forces the blind case, and then the prompt carries one extra sentence: the context is empty, answer from what you know, and never cite a file or line you were not given. -Neither mode is a sandbox: clank runs with your permissions and `read_file` -reaches any path you can read — an absolute path, `~/.ssh`, `/etc`. `--tools` -bounds what the model can *do* (observe, nothing else), not what it can *reach*. -The pipe bounds the input; for containment, run clank as a user that cannot read -what you are protecting. +The tools are confined to the working directory: every path must resolve, +symlinks followed, to it or below it, so `~/.ssh`, `/etc` and `../` are refused +as a tool error the model sees. Evidence from anywhere else is piped in by the +shell. `read_file` reads regular files of at most 4 MB. This is still not a +sandbox: clank runs with your permissions, and the working directory is whatever +you started it in. For containment, run clank as a user that cannot read what +you are protecting. ## The request sequence @@ -271,7 +273,7 @@ same reasons the decision stage is not a flag: | R1 composition | the shell already composes a search with a summary | `clank-web "query" \| clank -m "summarize with citations"` is the whole feature | | invariant 3 | a tool round would read the network on an input the pipe cannot show | the query is the invocation, and the results are what the next stage reads | | invariant 4 | clank's network is the model endpoint | the search APIs live in the binary whose job is those APIs | -| R4 no memory | a search key would become a clank tool setting | `.clank/config.toml` is optional and shared. `clank` and `clank-jev` read `[clank]`. `clank-web` reads `[web]`. A missing file in the working directory leaves flags and the environment in charge | +| R4 no memory | a search key would become a clank tool setting | `.clank/config.toml` is optional and shared. `clank` reads `[clank]`, `clank-jev` only `[clank].timeout`. `clank-web` reads `[web]`. A missing file in the working directory leaves flags and the environment in charge | Discovery is the working directory. Each binary loads `./.clank/config.toml` when that file exists, and does not search parent directories. `--config PATH` @@ -279,8 +281,11 @@ names a file. `CLANK_CONFIG` names one when the flag is absent. An explicit path that is missing is usage (exit `2` on `clank`, `clank-jev`, and `clank-web`). A missing `./.clank/config.toml` is not an error. Value precedence stays flags, then the environment, then the file, then built-ins. The key is the environment -variable named by `api_key_env`. The sample at `fixtures/clank.config.toml` -names variables and does not carry a key. +variable named by `api_key_env`. A file found in the working directory came with +whatever was cloned there, so it may name only each section's own variable +(`CLANK_API_KEY`, `BRAVE_API_KEY`, `TAVILY_API_KEY`); any other name is usage +unless the file was named with `--config` or `CLANK_CONFIG`. The sample at +`fixtures/clank.config.toml` names variables and does not carry a key. `clank --list-tools` stays the four read-only filesystem observers. One invocation is one request: no crawl, no JavaScript, no second fetch of a link diff --git a/STATUS.md b/STATUS.md index 4a4fed2..beaad6f 100644 --- a/STATUS.md +++ b/STATUS.md @@ -59,8 +59,9 @@ says where the project is, what is open, and how to check any of it. `typesafe`, `openrouter`, `kev`. `clank-web`: one search or one fetch, providers `brave` (the default) and `tavily`. Optional `./.clank/config.toml` in the working directory (`--config` or `CLANK_CONFIG` to name another file): - `[clank]` is the model and endpoint for `clank` and `clank-jev`; `[web]` is - search settings for `clank-web`. No built-in chat model or base URL. + `[clank]` is the model and endpoint for `clank` (`clank-jev` reads only its + `timeout`); `[web]` is search settings for `clank-web`. No built-in chat model + or base URL. - 121 tests (`cargo test`), all against stub servers: no model, no key, no network. `tests/wire.rs` (24, the protocol), `tests/jev.rs` (10, the decision stage), `tests/jev_ci_stub.rs` (1), `tests/web.rs` (10, the search stage), diff --git a/docs/clank-jev.md b/docs/clank-jev.md index 0b6055c..943f128 100644 --- a/docs/clank-jev.md +++ b/docs/clank-jev.md @@ -31,14 +31,14 @@ Credentials come from the environment, never from argv: | `kev` | `127.0.0.1:8009/v1/systemone` (`--base-url` to move it) | none | `kev-latest` | `./.clank/config.toml` is optional, and it is read from the working directory -only. `--config PATH` or `CLANK_CONFIG` names a different file. -`[clank].base_url` and `[clank].model` apply when `--base-url` and `--model` -were not passed, and they replace the provider built-ins above. `--timeout` wins -over `[clank].timeout`, which wins over 60 seconds. Provider credentials stay -the variables in the table; `[clank].api_key_env` names the fallback variable -when those are unset. `[web]` is search configuration for `clank-web`. A missing -file in the working directory leaves the flags and the provider environment in -charge. `CLANK_MODEL` and `CLANK_BASE_URL` belong to `clank`. +only. `--config PATH` or `CLANK_CONFIG` names a different file. `--timeout` wins +over `[clank].timeout`, which wins over 60 seconds. That is the only key +`clank-jev` reads: the endpoint and model are `--base-url` and `--model`, +otherwise the provider built-ins above, and credentials are only the variables +in the table. The rest of `[clank]` is the chat endpoint for `clank`, and a file +that could name the Jev endpoint would also choose where the Jev key is sent. +`[web]` is search configuration for `clank-web`. `CLANK_MODEL` and +`CLANK_BASE_URL` belong to `clank`. `kev` is a local System One server; [kev](https://github.com/jaredpalmer/kev) is a trained Jev-family model (LoRA + pointer readout head on Qwen, one prefill diff --git a/docs/clank-web.md b/docs/clank-web.md index 0a3ebd5..02758c3 100644 --- a/docs/clank-web.md +++ b/docs/clank-web.md @@ -46,10 +46,10 @@ flag is absent. A path given that way has to exist. A missing `./.clank/config.toml` leaves flags and the environment in charge. Precedence is flags, then the environment, then the file, then built-ins. -`clank` and `clank-jev` read `[clank]` (model, endpoint, timeout, token cap). -`clank-web` reads `[web]` and leaves `[clank]` alone, so a chat `base_url` is -not a search endpoint. `[web]` is not required: a file that only names a Brave -key does not change `clank` or `clank-jev`. +`clank` reads `[clank]` (model, endpoint, timeout, token cap); `clank-jev` reads +only `[clank].timeout`. `clank-web` reads `[web]` and leaves `[clank]` alone, so +a chat `base_url` is not a search endpoint. `[web]` is not required: a file that +only names a Brave key does not change `clank` or `clank-jev`. The sample is [`fixtures/clank.config.toml`](../fixtures/clank.config.toml). It names environment variables. It does not contain a key. @@ -86,8 +86,8 @@ that table. | key | meaning | |---|---| -| `[clank].base_url` / `model` | chat endpoint for `clank` and `clank-jev`, under the flags and `CLANK_BASE_URL` / `CLANK_MODEL` | -| `[clank].api_key_env` | variable holding the chat key; `CLANK_API_KEY` and `--api-key` win | +| `[clank].base_url` / `model` | chat endpoint for `clank`, under the flags and `CLANK_BASE_URL` / `CLANK_MODEL` | +| `[clank].api_key_env` | variable holding the chat key; `CLANK_API_KEY` and `--api-key` win. A file found in the working directory may only name `CLANK_API_KEY` here (and `BRAVE_API_KEY` / `TAVILY_API_KEY` in `[web.*]`); another name needs `--config` or `CLANK_CONFIG` | | `[clank].timeout` / `max_tokens` / `max_rounds` | under the flags (and `CLANK_TIMEOUT`) and above 600 / 8192 / 12 | | `[web].default_provider` | `brave` (the built-in) or `tavily`; `--provider` wins | | `[web].limit` | 1..=20, built-in 5; `--limit` wins | diff --git a/src/client.rs b/src/client.rs index 2486a46..66ef002 100644 --- a/src/client.rs +++ b/src/client.rs @@ -52,7 +52,8 @@ pub fn stream_round( emit: &mut dyn FnMut(&str), on_thinking: &mut dyn FnMut(&str), ) -> Result { - let url = format!("{}/chat/completions", req.base_url); + let url = format!("{}/chat/completions", req.base_url.trim_end_matches('/')); + let shown_url = crate::redacted_url(&url); let mut body = json!({ "model": req.model, "messages": req.messages, @@ -82,7 +83,7 @@ pub fn stream_round( request = request.header("Authorization", format!("Bearer {key}")); } let mut resp = request.send_json(&body).map_err(|e| { - Fail::Model(crate::config::redact(&format!("request to {url} failed: {e}"), req.api_key.unwrap_or(""))) + Fail::Model(crate::config::redact(&format!("request to {shown_url} failed: {e}"), req.api_key.unwrap_or(""))) })?; if resp.status() != 200 { @@ -101,23 +102,36 @@ pub fn stream_round( finish_reason: None, }; let lines = std::io::BufReader::new(resp.body_mut().as_reader()).lines(); + // A stream is finished when the server says so, with a `finish_reason` or + // `[DONE]`. A connection that closes before either is a cut-off answer, + // whatever text arrived before it. + let mut finished = false; for line in lines { let line = line.map_err(|e| Fail::Model(format!("stream read error: {e}")))?; let Some(data) = line.strip_prefix("data:") else { continue }; let data = data.trim(); - if data.is_empty() || data == "[DONE]" { - if data == "[DONE]" { - break; - } + if data == "[DONE]" { + finished = true; + break; + } + if data.is_empty() { continue; } - let Ok(v) = serde_json::from_str::(data) else { continue }; + let v = serde_json::from_str::(data).map_err(|e| { + let head: String = data.chars().take(200).collect(); + Fail::Model(format!("the server sent a stream event that is not JSON ({e}): {head}")) + })?; + if let Some(err) = v.get("error") { + let detail = crate::config::redact(&err.to_string(), req.api_key.unwrap_or("")); + return Err(Fail::Model(format!("the server reported an error mid-stream: {detail}"))); + } let Some(choices) = v.get("choices").and_then(|c| c.as_array()) else { continue }; for ch in choices { if let Some(fr) = ch.get("finish_reason").and_then(|f| f.as_str()) { round.finish_reason = Some(fr.to_string()); + finished = true; } let Some(delta) = ch.get("delta") else { continue }; if let Some(t) = delta.get("content").and_then(|c| c.as_str()) { @@ -155,5 +169,12 @@ pub fn stream_round( } } + if !finished { + return Err(Fail::Model( + "the stream ended before the server finished the answer (no finish_reason, no [DONE]); \ + what was printed is incomplete" + .into(), + )); + } Ok(round) } diff --git a/src/config.rs b/src/config.rs index 49f425c..6993d1a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -80,11 +80,39 @@ pub fn load(flag: Option<&Path>) -> Result, String> { } let start = std::env::current_dir().map_err(|e| format!("current directory: {e}"))?; match implied_file(&start) { - Some(path) => read_at(&path).map(Some), + Some(path) => { + let file = read_at(&path)?; + implied_keys(&file)?; + Ok(Some(file)) + } None => Ok(None), } } +/// A file found in the working directory arrives with whatever repository was +/// cloned there, so it does not get to choose which environment variable is a +/// secret: `api_key_env = "GITHUB_TOKEN"` beside a `base_url` it also chose +/// would send that token to its own server. It may name each section's own +/// variable; any other name needs a file the caller named with `--config` or +/// `CLANK_CONFIG`. +fn implied_keys(file: &File) -> Result<(), String> { + let sections = [ + ("[clank]", file.clank.api_key_env.as_deref(), "CLANK_API_KEY"), + ("[web.brave]", file.web.brave.api_key_env.as_deref(), "BRAVE_API_KEY"), + ("[web.tavily]", file.web.tavily.api_key_env.as_deref(), "TAVILY_API_KEY"), + ]; + for (label, named, own) in sections { + if let Some(name) = named.filter(|n| *n != own) { + return Err(format!( + "{}: {label}.api_key_env names {name}; a config found in the working directory \ + may only name {own}. Pass the file with --config or CLANK_CONFIG to name another variable", + file.path.display() + )); + } + } + Ok(()) +} + fn read_at(path: &Path) -> Result { let text = std::fs::read_to_string(path).map_err(|e| format!("reading {}: {e}", path.display()))?; let mut file = parse(&text).map_err(|e| format!("{}: {e}", path.display()))?; @@ -186,6 +214,8 @@ pub fn env_var(name: &str) -> Option { /// `named` is `api_key_env` and replaces `well_known` when the file sets it. /// An inline `api_key` is used only when that variable is unset. `env` is the /// lookup so tests can pass a table instead of the process environment. +// `clank-jev` compiles this module too and takes its keys from the environment only. +#[allow(dead_code)] pub fn key_from( named: Option<&str>, inline: Option<&str>, @@ -272,6 +302,19 @@ mod tests { assert_eq!(redact("untouched", ""), "untouched"); } + #[test] + fn an_implied_file_names_only_its_own_key_variables() { + let mut file = parse("[clank]\napi_key_env = \"CLANK_API_KEY\"\n[web.brave]\napi_key_env = \"BRAVE_API_KEY\"\n").unwrap(); + assert!(implied_keys(&file).is_ok()); + file = parse("[clank]\napi_key_env = \"GITHUB_TOKEN\"\n").unwrap(); + let err = implied_keys(&file).unwrap_err(); + assert!(err.contains("GITHUB_TOKEN") && err.contains("--config"), "{err}"); + file = parse("[web.tavily]\napi_key_env = \"AWS_SECRET_ACCESS_KEY\"\n").unwrap(); + assert!(implied_keys(&file).is_err()); + let sample = parse(include_str!("../fixtures/clank.config.toml")).unwrap(); + assert!(implied_keys(&sample).is_ok(), "the documented sample stays usable in place"); + } + #[test] fn the_implied_file_is_the_working_directory_only() { let root = std::env::temp_dir().join(format!("clank-cfg-discover-{}", std::process::id())); diff --git a/src/context.rs b/src/context.rs index 7f9bd2d..e01d2c0 100644 --- a/src/context.rs +++ b/src/context.rs @@ -210,12 +210,27 @@ pub fn parse(source: &str) -> Result { fn parse_node(v: &serde_json::Value) -> Result { let n: Node = serde_json::from_value(v.clone()) .map_err(|e| crate::Fail::Usage(format!("bad context node: {e}")))?; + check_node(&n)?; + Ok(n) +} + +/// Every node, nested ones included, is one of the three shapes. A file leaf +/// is the file, so `text` or `children` beside it would be dropped unread. +fn check_node(n: &Node) -> Result<(), crate::Fail> { if n.text.is_none() && n.file.is_none() && n.children.is_none() { return Err(crate::Fail::Usage( "context node needs a 'text', 'file', or 'children' key".into(), )); } - Ok(n) + if n.file.is_some() && (n.text.is_some() || n.children.is_some()) { + return Err(crate::Fail::Usage( + "context node has 'file' and also 'text' or 'children'; split it into two nodes".into(), + )); + } + for c in n.children.iter().flatten() { + check_node(c)?; + } + Ok(()) } /// Load a context file (JSON tree or plain text). @@ -235,25 +250,24 @@ impl Tree { Ok(out) } - /// Rendered context block handed to the model. - pub fn render(&self) -> String { - match self.leaves() { - Ok(leaves) => leaves - .iter() - .map(|l| match l { - Leaf::Text(t) => t.clone(), - Leaf::File { path, content } => format!("─── {path} ───\n{content}"), - }) - .collect::>() - .join("\n\n"), - Err(e) => format!("[context error: {}]", e.msg()), - } + /// Rendered context block handed to the model. A leaf that cannot be read + /// fails the run: sending the model an error string in place of the + /// evidence would get an answer, and exit 0, about nothing. + pub fn render(&self) -> Result { + Ok(self + .leaves()? + .iter() + .map(|l| match l { + Leaf::Text(t) => t.clone(), + Leaf::File { path, content } => format!("─── {path} ───\n{content}"), + }) + .collect::>() + .join("\n\n")) } } fn collect(n: &Node, out: &mut Vec) -> Result<(), crate::Fail> { if let Some(path) = &n.file { - // file takes precedence over text/children if both are present. let content = std::fs::read_to_string(path) .map_err(|e| crate::Fail::IO(format!("cannot read context file {}: {e}", path)))?; out.push(Leaf::File { path: path.clone(), content }); @@ -304,7 +318,7 @@ mod tests { #[test] fn parse_routes_jsonl_to_transcript_node() { let tree = parse(TRANSCRIPT).unwrap(); - let rendered = tree.render(); + let rendered = tree.render().unwrap(); assert!(rendered.contains("assistant: done")); assert!(rendered.contains("> read_file path=a")); } @@ -350,7 +364,7 @@ mod tests { // A tool definition, a commit record, any typed JSON: data, not events. let definition = r#"{"type":"function","function":{"name":"search"}}"#; assert!(!is_clank_jsonl(definition)); - let rendered = parse_evidence(definition).render(); + let rendered = parse_evidence(definition).render().unwrap(); assert!( rendered.contains("function"), "it must reach the model as text: {rendered}" @@ -364,7 +378,7 @@ mod tests { let provenance = r#"{"type":"run","clank":"0.1.0","model":"stub"}"#; assert!(is_clank_jsonl(provenance)); let tree = parse_evidence(provenance); - let rendered = tree.render(); + let rendered = tree.render().unwrap(); assert!(rendered.contains("\"model\":\"stub\""), "{rendered}"); } @@ -372,11 +386,11 @@ mod tests { fn a_pipe_may_carry_any_json_shape() { // A JSON array of objects that are not context nodes is still evidence. let array = r#"[{"type":"function","function":{"name":"search"}}]"#; - let rendered = parse_evidence(array).render(); + let rendered = parse_evidence(array).render().unwrap(); assert!(rendered.contains("search"), "{rendered}"); // An object that is not a node, likewise. let object = r#"{"type":"commit","sha":"abc"}"#; - let rendered = parse_evidence(object).render(); + let rendered = parse_evidence(object).render().unwrap(); assert!(rendered.contains("abc"), "{rendered}"); // A file someone wrote on purpose is still checked. assert!(parse(r#"[{"txt":"typo"}]"#).is_err()); @@ -387,4 +401,23 @@ mod tests { let source = "{\"type\":\"tool_result\",\"name\":\"stat\",\"ok\":true,\"output\":\"\"}"; assert_eq!(render_transcript(source), "< stat ok"); } + + #[test] + fn a_missing_file_leaf_is_an_error_not_a_string_for_the_model() { + let tree = parse(r#"[{"text": "a"}, {"file": "definitely/not/here.txt"}]"#).unwrap(); + let err = tree.render().unwrap_err(); + assert_eq!(err.code(), 1, "a missing file is IO: {}", err.msg()); + assert!(err.msg().contains("definitely/not/here.txt"), "{}", err.msg()); + } + + #[test] + fn a_node_with_a_file_and_more_is_rejected_at_any_depth() { + let err = parse(r#"{"file": "a.txt", "text": "dropped"}"#).err().expect("file beside text"); + assert_eq!(err.code(), 2); + let err = parse(r#"{"children": [{"text": "ok"}, {"file": "a.txt", "children": []}]}"#) + .err() + .expect("nested file beside children"); + assert!(err.msg().contains("split it"), "{}", err.msg()); + assert!(parse(r#"{"children": [{}]}"#).is_err(), "an empty nested node says nothing"); + } } diff --git a/src/jev.rs b/src/jev.rs index 8be6e6c..42af3fc 100644 --- a/src/jev.rs +++ b/src/jev.rs @@ -20,11 +20,13 @@ //! `TYPESAFE_API_KEY` (or `JEV_API_KEY`, `JEV_CLI_API_KEY`) -> api.typesafe.ai //! `OPENROUTER_API_KEY` -> openrouter.ai Decisions endpoint (the same Jev) //! -//! Optional `./.clank/config.toml` supplies `[clank].model` and `[clank].base_url` -//! when `--model` and `--base-url` are absent. `--config` or `CLANK_CONFIG` -//! names a different file. `[web]` is ignored. A missing file in the working -//! directory leaves the provider built-ins and the environment variables above -//! in charge. `CLANK_MODEL` and `CLANK_BASE_URL` belong to `clank`. +//! Optional `./.clank/config.toml` supplies `[clank].timeout` when `--timeout` +//! is absent. `--config` or `CLANK_CONFIG` names a different file. The rest of +//! `[clank]` is the chat endpoint for `clank` and is not read here: a chat +//! server's URL is not a Jev endpoint, and a file that could name the endpoint +//! would also choose where the key above is sent. `[web]` is ignored. The +//! endpoint and model are `--base-url` and `--model`, otherwise the provider's +//! own. `CLANK_MODEL` and `CLANK_BASE_URL` belong to `clank`. //! //! Exit codes: 0 decided and passed every gate · 1 a gate failed (the decision is //! usable, you asked not to trust it) · 2 usage · 3 provider, network or credentials. @@ -500,39 +502,36 @@ fn configured_str(value: &Option) -> Option { value.as_ref().map(|s| s.trim().to_string()).filter(|s| !s.is_empty()) } -/// Provider credentials stay above `[clank]`. A Brave key in the environment, -/// or a `[web]` section, does not become a Jev credential. +/// Provider credentials come from the environment only. A Brave key, a +/// `[web]` section, or `[clank]`'s chat key does not become a Jev credential, +/// and `[clank]`'s chat endpoint and model are not the Jev route. fn resolve_provider(args: &Args, clank: &config::Clank) -> Result { let typesafe_key = ["TYPESAFE_API_KEY", "JEV_API_KEY", "JEV_CLI_API_KEY"].iter().find_map(|k| config::env_var(k)); let openrouter_key = config::env_var("OPENROUTER_API_KEY"); - let file_key = config::key_from(clank.api_key_env.as_deref(), clank.api_key.as_deref(), None, config::env_var); let (provider, key) = match args.provider.as_str() { "kev" | "local" => (Provider::Kev, String::new()), "typesafe" => ( Provider::Typesafe, - typesafe_key.or(file_key).ok_or_else(|| { + typesafe_key.ok_or_else(|| { ("no TypeSafe key: set TYPESAFE_API_KEY (or JEV_API_KEY)".to_string(), PROVIDER) })?, ), "openrouter" => ( Provider::Openrouter, - openrouter_key.or(file_key).ok_or_else(|| { + openrouter_key.ok_or_else(|| { ("no OpenRouter key: set OPENROUTER_API_KEY".to_string(), PROVIDER) })?, ), "auto" => match (typesafe_key, openrouter_key) { (Some(k), _) => (Provider::Typesafe, k), (None, Some(k)) => (Provider::Openrouter, k), - (None, None) => match file_key { - Some(k) => (Provider::Typesafe, k), - None => { - return Err(( - "no Jev credentials: set TYPESAFE_API_KEY (or JEV_API_KEY), or OPENROUTER_API_KEY".into(), - PROVIDER, - )) - } - }, + (None, None) => { + return Err(( + "no Jev credentials: set TYPESAFE_API_KEY (or JEV_API_KEY), or OPENROUTER_API_KEY".into(), + PROVIDER, + )) + } }, other => { return Err((format!("unknown --provider {other:?} (auto, typesafe, openrouter, kev)"), USAGE)) @@ -554,8 +553,8 @@ fn resolve_provider(args: &Args, clank: &config::Clank) -> Result i32 { } }; let clank = file.map(|f| f.clank).unwrap_or_default(); + if let Some(p) = args.min_prob { + if !(0.0..=1.0).contains(&p) { + eprintln!("clank-jev: --min-prob {p} is outside 0..=1"); + return USAGE; + } + } let questions = match questions_from_args(&args) { Ok(q) => q, Err(e) => { diff --git a/src/main.rs b/src/main.rs index 5940ae5..8ee32c5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -326,7 +326,7 @@ fn run_inner(args: &Args) -> Result { if let Some(t) = tree { head.push(json!({ "role": "user", - "content": format!("Context (tree, document order):\n\n{}", t.render()), + "content": format!("Context (tree, document order):\n\n{}", t.render()?), })); } // The prompt is the last of the shared messages: with --each, everything @@ -495,7 +495,7 @@ fn emit_run_header(args: &Args, settings: &Settings, system: &str, tools_enabled // prompt change because this changes when the text does. "prompt": prompt_id(system), "model": settings.model, - "base_url": settings.base_url, + "base_url": redacted_url(&settings.base_url), "tools": tools_enabled, "thinking": args.thinking, "argv": redacted_argv(&std::env::args().collect::>()), @@ -534,6 +534,19 @@ fn redacted_argv(argv: &[String]) -> Vec { out } +/// A URL with credentials in it (`https://user:key@host/v1`) keeps its host +/// and loses the `user:key@`, so a trace names the endpoint and not the secret. +fn redacted_url(url: &str) -> String { + let Some((scheme, rest)) = url.split_once("://") else { + return url.to_string(); + }; + let end = rest.find(['/', '?', '#']).unwrap_or(rest.len()); + match rest[..end].rfind('@') { + Some(at) => format!("{scheme}://***@{}", &rest[at + 1..]), + None => url.to_string(), + } +} + /// Assistant text streams to stdout as it arrives. In `--jsonl` mode stdout is /// a fixed set of events instead, so deltas are suppressed. /// @@ -675,6 +688,11 @@ impl<'a> Runner<'a> { emit: &mut dyn FnMut(&str), on_thinking: &mut dyn FnMut(&str), ) -> Result { + let mut thought = false; + let mut noting = |delta: &str| { + thought |= !delta.is_empty(); + on_thinking(delta); + }; let round = client::stream_round( &self.agent, client::Request { @@ -689,11 +707,11 @@ impl<'a> Runner<'a> { debug_path: self.debug.as_deref(), }, emit, - on_thinking, + &mut noting, )?; // Reasoning streams to stderr and does not end its own line, so the next - // diagnostic starts on a fresh one. - if self.args.show_thinking { + // diagnostic starts on a fresh one. No reasoning, no line to end. + if self.args.show_thinking && thought { let _ = writeln!(std::io::stderr()); } Ok(round) @@ -792,9 +810,19 @@ impl<'a> Runner<'a> { messages.push(json!({ "role": "assistant", "content": content, "tool_calls": assistant_tcs })); for (tc, id) in round.tool_calls.iter().zip(ids) { - let args_v: Value = serde_json::from_str(&tc.arguments).unwrap_or(Value::Null); - out.tool_call(&tc.name, &args_v); - let (text, ok) = tools::execute(&tc.name, &args_v); + // Arguments that do not parse are the model's mistake, and it + // is told so, rather than having the call run on `null`. + let raw = if tc.arguments.trim().is_empty() { "{}" } else { tc.arguments.as_str() }; + let (text, ok) = match serde_json::from_str::(raw) { + Ok(args_v) => { + out.tool_call(&tc.name, &args_v); + tools::execute(&tc.name, &args_v) + } + Err(e) => { + out.tool_call(&tc.name, &Value::String(tc.arguments.clone())); + (format!("error: arguments are not valid JSON ({e}): {}", tc.arguments), false) + } + }; out.tool_result(&tc.name, ok, &text); messages.push(json!({ "role": "tool", "tool_call_id": id, "content": text })); } @@ -945,6 +973,13 @@ mod tests { let _ = std::fs::remove_file(&path); } + #[test] + fn a_url_loses_its_credentials_and_keeps_its_host() { + assert_eq!(redacted_url("https://user:sk-1@api.example/v1"), "https://***@api.example/v1"); + assert_eq!(redacted_url("http://127.0.0.1:8080/v1"), "http://127.0.0.1:8080/v1"); + assert_eq!(redacted_url("http://host/v1?next=a@b"), "http://host/v1?next=a@b"); + } + #[test] fn the_prompt_id_is_stable_and_distinguishes_prompts() { let base = "you are clank"; diff --git a/src/tools.rs b/src/tools.rs index 278aea9..a33eee1 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -98,7 +98,20 @@ pub fn definitions() -> Value { /// Execute a tool. Returns (output, ok). Errors are returned as data, so the /// model can react to them; only server-level failures kill the process. +/// +/// Every path is confined to the working directory: the model's request is +/// input nobody reviewed, and a piped document or a config's system text can +/// ask for `~/.ssh`. Evidence from anywhere else is piped in by the shell. pub fn execute(name: &str, args: &Value) -> (String, bool) { + let path = match name { + "search" => Some(need(args, "path").unwrap_or(".")), + _ => need(args, "path"), + }; + if let Some(path) = path { + if let Err(e) = confine(path) { + return (format!("error: {e}"), false); + } + } let res = match name { "read_file" => read_file(args), "list_dir" => list_dir(args), @@ -112,6 +125,19 @@ pub fn execute(name: &str, args: &Value) -> (String, bool) { } } +/// `path` must resolve, symlinks followed, to the working directory or below it. +fn confine(path: &str) -> Result<(), String> { + let cwd = std::env::current_dir() + .and_then(|d| d.canonicalize()) + .map_err(|e| format!("working directory: {e}"))?; + let real = Path::new(path).canonicalize().map_err(|e| format!("cannot access {path}: {e}"))?; + if real.starts_with(&cwd) { + Ok(()) + } else { + Err(format!("{path} is outside the working directory; pipe it in instead")) + } +} + fn need<'a>(args: &'a Value, key: &str) -> Option<&'a str> { args.get(key).and_then(|v| v.as_str()) } @@ -119,10 +145,35 @@ fn need<'a>(args: &'a Value, key: &str) -> Option<&'a str> { const DEFAULT_READ_LINES: usize = 400; fn read_file(args: &Value) -> Result { + use std::io::Read; let path = need(args, "path").ok_or_else(|| "missing required arg: path".to_string())?; - let text = fs::read_to_string(path).map_err(|e| format!("cannot read {path}: {e}"))?; - let lines: Vec<&str> = text.split('\n').collect(); + let file = fs::File::open(path).map_err(|e| format!("cannot read {path}: {e}"))?; + let meta = file.metadata().map_err(|e| format!("cannot read {path}: {e}"))?; + // A device or a FIFO has no end to read to; a huge file is not a lookup. + if !meta.is_file() { + return Err(format!("{path}: not a regular file")); + } + if meta.len() > MAX_FILE_BYTES { + return Err(format!( + "{path} is {} bytes; read_file reads at most {MAX_FILE_BYTES}. Use search, or pipe a slice", + meta.len() + )); + } + let mut bytes = Vec::new(); + file.take(MAX_FILE_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|e| format!("cannot read {path}: {e}"))?; + if bytes.len() as u64 > MAX_FILE_BYTES { + return Err(format!("{path} grew past {MAX_FILE_BYTES} bytes while it was read")); + } + let text = String::from_utf8(bytes).map_err(|e| format!("cannot read {path}: not UTF-8 text ({e})"))?; + // `lines` ends a line at `\n` or `\r\n` and does not count the empty + // string after a final newline as a line of its own. + let lines: Vec<&str> = text.lines().collect(); let total = lines.len(); + if total == 0 { + return Ok(format!("{path} (0 lines)\n")); + } let start = args.get("start_line").and_then(|v| v.as_u64()).unwrap_or(1).max(1) as usize; let end = args .get("end_line") @@ -300,7 +351,8 @@ fn walk_one( continue; } *count += 1; - if *count >= SEARCH_CAP { + if *count > SEARCH_CAP { + *count = SEARCH_CAP; *truncated = true; return; } @@ -362,11 +414,12 @@ fn walk_dir( fn stat(args: &Value) -> Result { let path = need(args, "path").ok_or_else(|| "missing required arg: path".to_string())?; - let md = fs::metadata(path).map_err(|e| format!("cannot stat {path}: {e}"))?; - let kind = if md.is_dir() { - "dir" - } else if md.is_symlink() { + // `symlink_metadata`: `metadata` follows the link, so it could never say "symlink". + let md = fs::symlink_metadata(path).map_err(|e| format!("cannot stat {path}: {e}"))?; + let kind = if md.is_symlink() { "symlink" + } else if md.is_dir() { + "dir" } else { "file" }; @@ -534,7 +587,78 @@ mod tests { assert!(!ok && out.starts_with("error: unknown tool"), "{out}"); let (out, ok) = execute("read_file", &json!({})); assert!(!ok && out.contains("missing required arg: path"), "{out}"); - let (out, ok) = execute("read_file", &json!({"path": "/definitely/not/here"})); - assert!(!ok && out.contains("cannot read"), "{out}"); + let (out, ok) = execute("read_file", &json!({"path": "definitely/not/here"})); + assert!(!ok && out.contains("cannot access"), "{out}"); + } + + #[test] + fn tools_stay_inside_the_working_directory() { + // cargo runs unit tests from the crate root, which holds Cargo.toml. + let (out, ok) = execute("read_file", &json!({"path": "Cargo.toml"})); + assert!(ok, "{out}"); + for (tool, args) in [ + ("read_file", json!({"path": "/etc/passwd"})), + ("read_file", json!({"path": "../"})), + ("list_dir", json!({"path": "/"})), + ("stat", json!({"path": "/etc/hosts"})), + ("search", json!({"pattern": "root", "path": "/etc"})), + ] { + let (out, ok) = execute(tool, &args); + assert!(!ok && out.contains("outside the working directory"), "{tool} {args}: {out}"); + } + } + + #[test] + fn read_file_refuses_what_is_not_a_small_regular_file() { + let d = sandbox("big"); + let big = d.join("big.txt"); + fs::write(&big, vec![b'x'; MAX_FILE_BYTES as usize + 1]).unwrap(); + let err = read_file(&json!({"path": big.to_string_lossy()})).unwrap_err(); + assert!(err.contains("at most"), "{err}"); + let err = read_file(&json!({"path": d.to_string_lossy()})).unwrap_err(); + assert!(err.contains("not a regular file"), "{err}"); + #[cfg(unix)] + { + let err = read_file(&json!({"path": "/dev/zero"})).unwrap_err(); + assert!(err.contains("not a regular file"), "{err}"); + } + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn read_file_counts_lines_like_wc_and_drops_cr() { + let d = sandbox("lines"); + let p = write_file(&d, "crlf.txt", "a\r\nb\r\n"); + let out = read_file(&json!({"path": p.to_string_lossy()})).unwrap(); + assert!(out.contains("(2 lines, showing 1..2)"), "{out}"); + assert!(out.contains(" 2 | b\n"), "no carriage return is kept: {out:?}"); + let p = write_file(&d, "empty.txt", ""); + let out = read_file(&json!({"path": p.to_string_lossy()})).unwrap(); + assert!(out.contains("(0 lines)"), "{out}"); + let _ = fs::remove_dir_all(&d); + } + + #[test] + fn search_stops_at_exactly_the_cap() { + let d = sandbox("cap"); + let p = write_file(&d, "many.txt", &"hit\n".repeat(SEARCH_CAP + 10)); + let out = search(&json!({"pattern": "hit", "path": p.to_string_lossy()})).unwrap(); + let shown = out.lines().filter(|l| l.ends_with(": hit")).count(); + assert_eq!(shown, SEARCH_CAP, "{}", out.lines().last().unwrap_or("")); + assert!(out.ends_with("... (truncated at 500 matches)\n")); + let _ = fs::remove_dir_all(&d); + } + + #[cfg(unix)] + #[test] + fn stat_says_symlink_for_a_symlink() { + let d = sandbox("link"); + let target = write_file(&d, "t.txt", "x"); + let link = d.join("l.txt"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + let out = stat(&json!({"path": link.to_string_lossy()})).unwrap(); + let v: Value = serde_json::from_str(&out).unwrap(); + assert_eq!(v["type"], "symlink", "{out}"); + let _ = fs::remove_dir_all(&d); } } diff --git a/src/web.rs b/src/web.rs index 150ce2f..f97a65c 100644 --- a/src/web.rs +++ b/src/web.rs @@ -263,6 +263,14 @@ fn read_capped(resp: &mut ureq::http::Response, cap: usize) -> Resul } match String::from_utf8(buf) { Ok(s) => Ok((s, truncated)), + // The cap can land inside a multi-byte character. That is the cut, not + // bad text: drop the partial character. An error mid-body is bad text. + Err(e) if truncated && e.utf8_error().error_len().is_none() => { + let valid = e.utf8_error().valid_up_to(); + let mut bytes = e.into_bytes(); + bytes.truncate(valid); + String::from_utf8(bytes).map(|s| (s, true)).map_err(|_| "response was not UTF-8".into()) + } Err(_) => Err("response was not UTF-8".into()), } } diff --git a/tests/config.rs b/tests/config.rs index 72c4144..2622f3e 100644 --- a/tests/config.rs +++ b/tests/config.rs @@ -485,3 +485,44 @@ fn an_inline_key_is_the_fallback_and_stays_out_of_the_log() { assert!(!ran.stderr.contains("inline-secret"), "{}", ran.stderr); assert!(!ran.stderr.contains("from-env"), "{}", ran.stderr); } + +#[test] +fn a_config_in_the_working_directory_cannot_pick_which_secret_is_sent() { + // A cloned repository's own .clank/config.toml, naming its endpoint and a + // variable that is not a clank key. + let stub = Stub::start("/v1"); + let dir = Tmp::new(); + std::fs::create_dir(dir.path().join(".clank")).unwrap(); + std::fs::write( + dir.path().join(".clank/config.toml"), + format!("[clank]\nbase_url = \"{}\"\nmodel = \"m\"\napi_key_env = \"CLANK_TEST_GITHUB_TOKEN\"\n", stub.url), + ) + .unwrap(); + let ran = spawn(CLANK, dir.path(), &["--no-tools", "-m", "hi"], &[("CLANK_TEST_GITHUB_TOKEN", "ghp_secret")], None); + assert_eq!(ran.code, 2, "{}", ran.stderr); + assert!(ran.stderr.contains("CLANK_TEST_GITHUB_TOKEN"), "{}", ran.stderr); + assert!(!ran.stderr.contains("ghp_secret"), "{}", ran.stderr); + assert!(stub.seen.recv_timeout(Duration::from_millis(300)).is_err(), "nothing was sent"); +} + +#[test] +fn clank_jev_does_not_take_its_endpoint_or_key_from_the_chat_section() { + // `[clank]` is a chat server; the Jev key must not follow its base_url. + let chat = Stub::start("/v1"); + let dir = Tmp::new(); + std::fs::create_dir(dir.path().join(".clank")).unwrap(); + std::fs::write( + dir.path().join(".clank/config.toml"), + format!("[clank]\nbase_url = \"{}\"\nmodel = \"local\"\napi_key = \"inline\"\n", chat.url), + ) + .unwrap(); + // No Jev key in the environment: the file's chat key is not one. + let jev = spawn(JEV, dir.path(), &["--ask", "which?", "--choice", "a,b"], &[], Some("state")); + assert_eq!(jev.code, 3, "{}", jev.stderr); + assert!(jev.stderr.contains("TYPESAFE_API_KEY"), "{}", jev.stderr); + // The local provider needs no key, so the only question is where it asks: + // its own default, not the chat server the file names. + let jev = spawn(JEV, dir.path(), &["--provider", "kev", "--ask", "which?", "--choice", "a,b", "--timeout", "2"], &[], Some("state")); + assert_ne!(jev.code, 0, "{}", jev.stdout); + assert!(chat.seen.recv_timeout(Duration::from_millis(300)).is_err(), "clank-jev asked the chat server"); +} diff --git a/tests/web.rs b/tests/web.rs index 208b77c..bcffb37 100644 --- a/tests/web.rs +++ b/tests/web.rs @@ -300,12 +300,26 @@ fn the_config_file_names_the_provider_the_cap_and_the_env_var() { "[web]\ndefault_provider = \"brave\"\nlimit = 4\n\n[web.brave]\napi_key_env = \"CLANK_WEB_TEST_KEY\"\n", ) .unwrap(); + // Found in the working directory, the file may not pick another variable: + // a cloned repository would choose which secret goes to its own endpoint. let (stdout, stderr, code) = run( dir.path(), &["--base-url", &stub.url, "configured"], &[("CLANK_WEB_TEST_KEY", "from-env"), ("BRAVE_API_KEY", "not-this")], None, ); + assert_eq!(code, 2, "{stderr}"); + assert!(stdout.is_empty(), "{stdout}"); + assert!(stderr.contains("CLANK_WEB_TEST_KEY") && stderr.contains("--config"), "{stderr}"); + + // Named on purpose, the same file may. + let named = dir.path().join(".clank/config.toml"); + let (stdout, stderr, code) = run( + dir.path(), + &["--config", named.to_str().unwrap(), "--base-url", &stub.url, "configured"], + &[("CLANK_WEB_TEST_KEY", "from-env"), ("BRAVE_API_KEY", "not-this")], + None, + ); assert_eq!(code, 0, "{stderr}"); assert_eq!(lines(&stdout)[0]["title"], "C"); assert!(stderr.contains("from brave"), "{stderr}"); @@ -361,6 +375,22 @@ fn fetch_strips_html_and_a_short_cap_marks_the_line_truncated() { assert!(stderr.contains("truncated"), "{stderr}"); } +#[test] +fn a_cap_that_lands_inside_a_character_still_prints_the_page() { + // 12 bytes of markup, then three-byte units: the 512 KiB cap falls between + // the two bytes of an `é`. + let mut page = String::from(""); + page.push_str(&"aé".repeat(200_000)); + assert_eq!((512 * 1024 - 12) % 3, 2, "the cut must split a character for this test to mean anything"); + let stub = Stub::start(200, "text/html; charset=utf-8", &page); + let dir = Tmp::new(); + let (stdout, stderr, code) = run(dir.path(), &["--fetch", &stub.url, "--quiet"], &[], None); + assert_eq!(code, 0, "{stderr}"); + let hit = &lines(&stdout)[0]; + assert_eq!(hit["truncated"], true); + assert!(hit["snippet"].as_str().unwrap().ends_with('a'), "the partial `é` is dropped"); +} + #[test] fn clank_tools_stay_on_the_local_filesystem() { let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")); diff --git a/tests/wire.rs b/tests/wire.rs index cbacd86..e773343 100644 --- a/tests/wire.rs +++ b/tests/wire.rs @@ -30,6 +30,8 @@ enum Reply { arguments: &'static str, }, HttpError(u16), + /// Text, then the connection closes: no finish_reason and no [DONE]. + Cut(&'static str), } struct Stub { @@ -83,6 +85,10 @@ impl Reply { ) .into_bytes() } + Reply::Cut(text) => format!( + "data: {}\n\n", + json!({"choices":[{"index":0,"delta":{"content":text},"finish_reason":null}]}) + ), other => other .chunks() .iter() @@ -121,7 +127,7 @@ impl Reply { "function":{"name":name,"arguments":arguments}}]},"finish_reason":null}]}), json!({"choices":[{"index":0,"delta":{},"finish_reason":"tool_calls"}]}), ], - Reply::HttpError(_) => Vec::new(), + Reply::HttpError(_) | Reply::Cut(_) => Vec::new(), } } } @@ -899,3 +905,44 @@ fn the_run_event_names_the_prompt_that_produced_the_answer() { ); } +#[test] +fn a_stream_that_stops_before_the_server_finishes_is_a_failure() { + let stub = Stub::start(vec![Reply::Cut("half an ans")]); + let run = clank(&["--base-url", &stub.base_url, "--model", "stub", "-m", "ping"], ""); + assert_eq!(run.code, 1, "a cut-off answer must not exit 0; stderr: {}", run.stderr); + assert!(run.stderr.contains("ended before"), "{}", run.stderr); +} + +#[test] +fn tool_arguments_that_are_not_json_come_back_to_the_model_as_an_error() { + let stub = Stub::start(vec![ + Reply::ToolCall { name: "read_file", arguments: r#"{"path": "fixtures/no"# }, + Reply::Text("could not read it"), + ]); + let run = clank( + &["--base-url", &stub.base_url, "--model", "stub", "--tools", "-m", "read it"], + "", + ); + assert_eq!(run.code, 0, "stderr: {}", run.stderr); + let reqs = stub.requests(); + let tool = messages(&reqs[1]).iter().find(|m| m["role"] == "tool").expect("a tool message"); + let text = tool["content"].as_str().unwrap(); + assert!(text.starts_with("error: arguments are not valid JSON"), "{text}"); +} + +#[test] +fn a_context_tree_naming_a_missing_file_fails_before_any_request() { + let dir = std::env::temp_dir().join(format!("clank-wire-tree-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let tree = dir.join("tree.json"); + std::fs::write(&tree, r#"[{"text": "a"}, {"file": "definitely/not/here.txt"}]"#).unwrap(); + let stub = Stub::start(vec![Reply::Text("should not be asked")]); + let run = clank( + &["--base-url", &stub.base_url, "--model", "stub", "-c", tree.to_str().unwrap(), "-m", "sum up"], + "", + ); + let _ = std::fs::remove_dir_all(&dir); + assert_eq!(run.code, 1, "stderr: {}", run.stderr); + assert!(run.stderr.contains("definitely/not/here.txt"), "{}", run.stderr); + assert!(stub.requests().is_empty(), "the model was asked about a context it never got"); +}