diff --git a/crates/brokerd/src/args.rs b/crates/brokerd/src/args.rs index 1c0a381..7c8b790 100644 --- a/crates/brokerd/src/args.rs +++ b/crates/brokerd/src/args.rs @@ -66,10 +66,39 @@ pub enum ToolArgs { command: String, cwd: Option, }, - HttpFetch { - url: String, - host: String, - }, + HttpFetch(FetchUrl), +} + +/// A URL that passed the checks, and its host. Only `parse` makes one, so the host policy matched +/// is always the host of the URL the tool fetches. +/// +/// ```compile_fail +/// let _ = brokerd::args::FetchUrl { +/// url: "https://evil.example/".to_string(), +/// host: "example.com".to_string(), +/// }; +/// ``` +/// +/// ``` +/// let args = brokerd::args::parse(brokerd::args::ToolName::HttpFetch, r#"{"url":"https://example.com/a"}"#); +/// let Ok(brokerd::args::ToolArgs::HttpFetch(target)) = args else { panic!("{args:?}") }; +/// assert_eq!((target.url(), target.host()), ("https://example.com/a", "example.com")); +/// ``` +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FetchUrl { + url: String, // private + host: String, // private +} + +impl FetchUrl { + /// The URL that passed the checks. + pub fn url(&self) -> &str { + &self.url + } + /// The host of that URL, as `parse` derived it. + pub fn host(&self) -> &str { + &self.host + } } impl ToolArgs { @@ -79,7 +108,7 @@ impl ToolArgs { ToolArgs::ReadFile { .. } => ToolName::ReadFile, ToolArgs::WriteFile { .. } => ToolName::WriteFile, ToolArgs::Shell { .. } => ToolName::Shell, - ToolArgs::HttpFetch { .. } => ToolName::HttpFetch, + ToolArgs::HttpFetch(_) => ToolName::HttpFetch, } } @@ -107,8 +136,10 @@ impl ToolArgs { }; serde_json::to_string(&value).unwrap_or_else(|_| "{}".to_string()) } - ToolArgs::HttpFetch { url, .. } => { - let value = HttpFetchArgs { url: url.clone() }; + ToolArgs::HttpFetch(target) => { + let value = HttpFetchArgs { + url: target.url.clone(), + }; serde_json::to_string(&value).unwrap_or_else(|_| "{}".to_string()) } } @@ -192,10 +223,10 @@ pub fn parse(tool: ToolName, arguments: &str) -> Result { Some(host) => host.to_string(), None => return Err(ArgsError::Url(value.url)), }; - Ok(ToolArgs::HttpFetch { + Ok(ToolArgs::HttpFetch(FetchUrl { url: value.url, host, - }) + })) } } } diff --git a/crates/brokerd/src/policy.rs b/crates/brokerd/src/policy.rs index 1ffa6d2..dd433d3 100644 --- a/crates/brokerd/src/policy.rs +++ b/crates/brokerd/src/policy.rs @@ -404,12 +404,12 @@ fn covers(args: &ToolArgs, grant: &Grant) -> Option> { } } ToolArgs::Shell { cwd: Some(cwd), .. } => best_path(grant, cwd, true), - ToolArgs::HttpFetch { host, .. } => { + ToolArgs::HttpFetch(target) => { if grant .constraints .hosts .iter() - .any(|pattern| host_matches(pattern, host)) + .any(|pattern| host_matches(pattern, target.host())) { Some(None) } else { diff --git a/crates/brokerd/src/runner.rs b/crates/brokerd/src/runner.rs index 12d4f1a..389ac9f 100644 --- a/crates/brokerd/src/runner.rs +++ b/crates/brokerd/src/runner.rs @@ -105,7 +105,7 @@ pub fn run(decision: Decision, runtime: &dyn Runtime) -> proto::ToolResponse { .collect(), None, ), - ToolArgs::HttpFetch { .. } => (Vec::new(), Some(decision.hosts().to_vec())), + ToolArgs::HttpFetch(_) => (Vec::new(), Some(decision.hosts().to_vec())), }; let spec = RunSpec { tool: args.tool(), diff --git a/crates/brokerd/tests/args.rs b/crates/brokerd/tests/args.rs index bbbc835..f952191 100644 --- a/crates/brokerd/tests/args.rs +++ b/crates/brokerd/tests/args.rs @@ -289,16 +289,16 @@ fn each_tool_parses_its_own_arguments() { cwd: Some("/home/kyle".to_string()) }) ); - assert_eq!( - parse( - ToolName::HttpFetch, - r#"{"url":"https://www.example.com/a"}"# - ), - Ok(ToolArgs::HttpFetch { - url: "https://www.example.com/a".to_string(), - host: "www.example.com".to_string() - }) + // A `FetchUrl` cannot be built outside `args`, so the parsed value is read through its getters. + let fetched = parse( + ToolName::HttpFetch, + r#"{"url":"https://www.example.com/a"}"#, ); + let Ok(ToolArgs::HttpFetch(target)) = fetched else { + panic!("{fetched:?}") + }; + assert_eq!(target.url(), "https://www.example.com/a"); + assert_eq!(target.host(), "www.example.com"); // Field order and white space in the request do not matter. assert_eq!( parse( diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 376a2a8..931f4cc 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone. | Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model | |---|---|---|---|---|---|---|---| +| M3b/02-brokerd-fetch-url | 2026-09-22 | done | 1 | pass | none | Sealed the fetch target: replaced the two-field `ToolArgs::HttpFetch { url, host }` variant with a tuple variant `HttpFetch(FetchUrl)` holding a new `FetchUrl { url, host }` struct whose fields are private and exposed only through `url()`/`host()`; `parse` is the only constructor. Added the two doctests word for word (a `compile_fail` proving the struct cannot be built outside `args`, and a passing one reading the parsed value through getters). Changed the four match sites that built or read the old variant: `tool` (`HttpFetch(_)`), `canonical_json` (`HttpFetch(target)` serialising `HttpFetchArgs { url: target.url.clone() }`, still only the URL), `parse` (`HttpFetch(FetchUrl { url: value.url, host })`), and `policy::covers` (`HttpFetch(target)` matching with `host_matches(pattern, target.host())`); `runner.rs` `HttpFetch { .. }` became `HttpFetch(_)`. Copied the new `tests/args.rs` (one test now reads the parsed value through getters). `grep "FetchUrl {"` shows the struct def, impl opener, the `parse` line and the doctest only; `grep "HttpFetch {"` shows nothing. All brokerd tests and doctests pass; `make gate` prints `gate: ok` on the first run. | ? | | M3b/01-proto-tools-hosts | 2026-09-22 | done | 1 | pass | none | Copied `tools.rs` and `hosts.rs` from the plan's `files/` into `crates/proto/tests/`; they fail to compile until the modules exist. Created `crates/proto/src/tools.rs` with the four public arg structs (ReadFile/Write/Shell/HttpFetch), each `#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]` and `#[serde(deny_unknown_fields)]`, `ShellArgs` keeping its `#[serde(default, skip_serializing_if = "Option::is_none")]` cwd; and `crates/proto/src/hosts.rs` with private `valid_label` plus `valid_host`, `valid_host_pattern`, `host_matches`, bodies and doc comments unchanged. Added `pub mod hosts;` and `pub mod tools;` to `proto/src/lib.rs` in alphabetical order (no re-exports). Deleted the private structs from `brokerd/src/args.rs`, added `use proto::tools::{...}` at the top and the `pub use proto::hosts::{...}` re-export where the functions were, and dropped the now-unused `use serde::{Deserialize, Serialize}`; `url_host`, `valid_path`, `inside` and `MAX_URL` stay. New suites 3 and 3 pass; brokerd args/grants/policy pass unchanged. `grep "fn valid_host\|struct ShellArgs"` shows one extra line, `brokerd/tests/args.rs:91`, the pre-existing test `valid_hosts_and_patterns` (substring match, not a duplicate definition). Gate passed on the first run. | ? | | M3a/23-review-fixes | 2026-09-22 | done | 1 | pass | none | Done by the design model, not an implementer run: Ornith was under heavy contention. Findings 1 to 13 and 16 fixed. Tests that failed without their fix cover 1, 2, 3, 4, 7, 8, 9, 10 and 16; two more (a torn first record, an empty later file) passed before the fix and are regression guards; 6, 11 and 13 were checked by reading. 14 moves to M3b's first task; of 15, the log-name rule, `MAX_PATH`'s doc and the unused push are fixed, and the two that change the baseline wait for an epoch change. The server expectations in the device tests were updated to the new layout. | Claude Opus 5.5 | | M3a/22-end-to-end | 2026-09-22 | done | 1 | pass | none | Copied `end_to_end.rs`, `Makefile` (workspace build then ignored e2e with `BOXMAKER_BROKERD`) and `tools/check-m3a-device.sh`. Without the variable: 0 passed, 1 ignored. With it: 1 passed five runs. `--ignored` without the variable fails with "set BOXMAKER_BROKERD". `make gate` ran the e2e line (`1 passed`) and prints `gate: ok`. Did not run the straylight script. | Grok 4.6 |