From 36eedf48fab984c64ebff50b5436355c1e349f74 Mon Sep 17 00:00:00 2001 From: Praveen Perera Date: Wed, 21 Oct 2026 22:45:34 -0500 Subject: [PATCH 1/3] Ask once for several 1Password finds An agent ran each `op find` as its own command and was prompted for every one, including searches it repeated after misreading the output. `op find` now takes `--also` for extra queries and sends them together, so the daemon's batching asks once. An approved find is kept for 10 minutes, so the same query with the same reason answers without asking. The agent guidance says to combine searches and rerun instead of rewording. --- docs/design.md | 6 +- docs/operations.md | 6 +- src/companion.rs | 4 +- src/companion/find.rs | 34 ++++++++++- src/env_files.rs | 3 +- src/guidance.rs | 10 +++- src/main.rs | 116 +++++++++++++++++++++++++++++-------- src/secrets.rs | 75 ++++++++++++++++++++---- tests/gateway/companion.rs | 68 ++++++++++++++++++++++ 9 files changed, 278 insertions(+), 44 deletions(-) diff --git a/docs/design.md b/docs/design.md index 150e183..e0ed50e 100644 --- a/docs/design.md +++ b/docs/design.md @@ -283,7 +283,11 @@ approved wherever the owner is with the same cleaned reason that arrive within 250 ms of the first join one batch of up to 8 queries (`FindBatches`, keyed by reason so an unrelated program's find never shares an approval or a Deny), asked on its own task so a caller that gives up cannot strand the rest; the batch is dropped once every caller - has. One prompt quotes every query; a companion answers `{"found_each": [...]}`, one `Found` per query in + has. `op find --also` sends each query as its own `/admin/op/find` request at once, so the batch joins them. + An approved batch's answers are kept for 10 minutes in `KeptValues::finds`, by cleaned reason and query, and + a later find with both the same is answered from there without asking; a forget clears them, and a batch + approved while a forget ran keeps nothing. On a hub the reason carries the spoke's name, so a spoke's kept + answers serve only that spoke One prompt quotes every query; a companion answers `{"found_each": [...]}`, one `Found` per query in order, and one from before batches, which ignores `also` and answers `found`, is reported as needing an update. The Mac lists items once per batch and opens an item two queries match once The Mac that answers runs `op item list --format json`, which holds no values, and scores items by how many query words diff --git a/docs/operations.md b/docs/operations.md index 0150ce8..4a56192 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -216,7 +216,11 @@ the prompt quotes the words and the reason. The phone cannot search 1Password, s reference picked afterwards asks again and names that item. Scripts can do the same with the `secrets` server's `find_references(query, reason)`. Finds with the same reason that arrive within 250 ms of each other, such as the calls a script starts together under `asyncio.gather`, share one prompt that quotes every query, up to 8 of them; -each call still gets its own matches. +each call still gets its own matches. One `op find` can do the same with `--also`: +`codemode-mcp op find apple signing --also pushover --reason "set up the release build"` asks once and prints a +JSON list with each query, its items and `more`. Once approved, the same query with the same reason is answered +again for 10 minutes without asking, so a program rerun after misreading the output does not prompt again; a +different query or reason asks, and `codemode-mcp service restart --fresh` forgets every kept answer. A file attached to an item, such as a signing certificate's `.p12`, is binary. Write it with `codemode-mcp op read "op://Personal/Signing/cert.p12" --out-file cert.p12`, which keeps its exact bytes and adds no diff --git a/src/companion.rs b/src/companion.rs index fc84940..187dcd7 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -31,7 +31,9 @@ pub use identity::{IdentityError, Role, load_or_create}; pub use invite::{InviteStatus, MAX_INVITE_LIFETIME, OpenedInvite}; pub use join::{JoinError, Joined, Joining, join}; pub use pair::{PairError, Paired, Pairing, companion_id, pair}; -pub use program::{ProgramAsk, ProgramRead, ProgramReadError, ProgramTarget, ProgramValues}; +pub use program::{ + ProgramAsk, ProgramRead, ProgramReadError, ProgramTarget, ProgramValues, checked_query, +}; pub use server::{CompanionHandle, ServeError, serve}; pub use ticket::{ AnyTicket, INVITE_PREFIX, InviteSecret, InviteTicket, TICKET_PREFIX, Ticket, TicketError, diff --git a/src/companion/find.rs b/src/companion/find.rs index 56571ff..3ee62e7 100644 --- a/src/companion/find.rs +++ b/src/companion/find.rs @@ -10,7 +10,13 @@ //! //! Finds with the same reason that arrive within [`FIND_BATCH_WINDOW`] of //! the first are asked as one, up to [`MAX_FIND_QUERIES`], so the searches a -//! script starts together under `asyncio.gather` need one approval +//! script starts together under `asyncio.gather`, or the queries of one +//! `op find`, need one approval +//! +//! An approved answer is kept for [`FIND_KEEP`], so the same query with the +//! same reason, such as a program run again after it misread the output, is +//! answered without asking. It names items and references, never a value, +//! that the owner already let a program with that reason see use std::{ collections::HashMap, @@ -46,6 +52,10 @@ const CANCEL_TITLE: &str = "Deny"; /// milliseconds const FIND_BATCH_WINDOW: Duration = Duration::from_millis(250); +/// How long an approved find answers the same query and reason again +/// without asking; long enough for a program to be fixed and rerun +const FIND_KEEP: Duration = Duration::from_secs(10 * 60); + /// What a program looks for, as the admin route and the `secrets` builtin /// server pass it on #[derive(Debug, Clone, Default)] @@ -57,7 +67,7 @@ pub struct FindAsk { } /// The items a find matched and the Mac that searched -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ProgramFound { /// `this Mac`, or the name of the companion that answered pub source: String, @@ -107,6 +117,11 @@ impl OnePassword { } let query = checked_query(&ask.query).map_err(ProgramReadError::Invalid)?; let reason = clean_reason(ask.reason.as_deref()); + if let Some(found) = self.kept_find(reason.as_deref(), &query).await { + tracing::info!("find answered from one approved earlier"); + + return Ok(found); + } let (answer, answered) = oneshot::channel(); let waiting = Waiting { query, answer }; @@ -169,6 +184,9 @@ impl OnePassword { let mut queries: Vec = waiting.iter().map(|find| find.query.clone()).collect(); queries.sort(); queries.dedup(); + // read before asking, so a forget while the prompt is up drops this + // answer instead of keeping it + let forgotten = self.forgotten().await; // every caller giving up takes the prompts down, as a lone find's did let all_gone = async { for find in &mut waiting { @@ -179,6 +197,18 @@ impl OnePassword { answer = self.find_each(&queries, reason.as_deref()) => answer, () = all_gone => return, }; + if let Ok((source, found)) = &answer { + let answers = queries.iter().zip(found).map(|(query, found)| { + let found = ProgramFound { + source: source.clone(), + found: found.clone(), + }; + + (query.clone(), found) + }); + self.keep_finds(reason.as_deref(), answers, FIND_KEEP, forgotten) + .await; + } for find in waiting { let own = match &answer { diff --git a/src/env_files.rs b/src/env_files.rs index 81a21f0..effe85d 100644 --- a/src/env_files.rs +++ b/src/env_files.rs @@ -340,7 +340,8 @@ impl SecretsServer { one you want to `write_env`, which asks the owner again naming that item. \ The owner approves the search with Touch ID on this Mac or a companion Mac; \ to look for several things, start one call for each together under \ - `asyncio.gather` with the same reason, and one prompt approves them all." + `asyncio.gather` with the same reason, and one prompt approves them all. \ + The same query and reason is answered again for 10 minutes without asking." )] async fn find_references( &self, diff --git a/src/guidance.rs b/src/guidance.rs index 7e4505a..c9097e7 100644 --- a/src/guidance.rs +++ b/src/guidance.rs @@ -69,9 +69,13 @@ macro_rules! onepassword_rule { `codemode-mcp op find WORDS --reason WHY` (or the `secrets` server's `find_references` \ tool) lists matching items' op:// references and types, never values, after the user \ approves on a Mac; then use the reference you picked. A query matches items holding any of \ - its words, up to 80 characters and 10 items, so to look for several things, start one \ - `find_references` call for each together under `asyncio.gather` with the same `reason`: \ - the user approves them all with one prompt. Or pass a description in one step: \ + its words, up to 80 characters and 10 items, so put other names for the same thing in one \ + query. To look for several things, pass each extra query as `--also WORDS` to one `op \ + find`, or start one `find_references` call for each together under `asyncio.gather` with \ + the same `reason`: the user approves them all with one prompt. Never run separate finds \ + one after another, since each asks the user again. The same query and reason is answered \ + again for 10 minutes without asking, so rerun it rather than rewording it if you misread \ + the output. Or pass a description in one step: \ `codemode-mcp op read --describe \"Apple signing cert\" --out-file PATH` or `write_env`'s \ `describe`. If the user approves on a Mac it searches instead, writes nothing and answers the \ matches (`op read` exits 3), so run again with the exact reference; if they approve on their \ diff --git a/src/main.rs b/src/main.rs index 8aa07e2..9c8c2ae 100644 --- a/src/main.rs +++ b/src/main.rs @@ -423,10 +423,21 @@ enum OpCommand { /// the JSON printed, then read it with `op read`, `op inject` or `op run`, /// which ask again and name that item. Words are matched in item titles, /// vaults, tags and categories, ignoring case; at most 10 items are listed + /// per query + /// + /// Each `--also` is another query searched under the same approval, and + /// the JSON printed is then a list of each query with its items. The same + /// query with the same reason is answered again for 10 minutes after an + /// approval without asking Find { - /// Words to look for, such as `apple signing` + /// Words to look for, such as `apple signing`; an item matching any + /// of them is listed #[arg(required = true, num_args = 1..)] query: Vec, + /// Another query to search under the same approval, such as `--also + /// "signing cert"`; up to 7 + #[arg(long, value_name = "WORDS")] + also: Vec, /// Why the program is looking; every prompt shows it #[arg(long)] reason: Option, @@ -2263,36 +2274,95 @@ async fn op_command(config: &Config, command: OpCommand) -> Result<()> { } } } - OpCommand::Find { query, reason } => { - let request = admin::OpFindRequest { - query: query.join(" "), - reason, - }; + OpCommand::Find { + query, + also, + reason, + } => { + let several = !also.is_empty(); + let queries: Vec = std::iter::once(query.join(" ")).chain(also).collect(); + if queries.len() > companion::MAX_FIND_QUERIES { + bail!( + "at most {} queries share one approval; pass the query and up to {} --also", + companion::MAX_FIND_QUERIES, + companion::MAX_FIND_QUERIES - 1 + ); + } + // checked before any is sent, so a malformed query cannot leave + // the others asking for nothing + for query in &queries { + companion::checked_query(query).map_err(|message| eyre!("refused: {message}"))?; + } + eprintln!( "Asking the owner to approve a 1Password search: Touch ID on this Mac or a \ companion Mac..." ); - let response: admin::OpFindResponse = - admin_post_as_owner(config, "/admin/op/find", &request).await?; - match response { - admin::OpFindResponse::Found { - source, - items, - more, - } => { - eprintln!("Approved on {source}"); - println!( - "{}", - serde_json::to_string_pretty(&serde_json::json!({ + // the daemon asks once for finds with the same reason that arrive + // together, so every query is sent at once + let mut finds = tokio::task::JoinSet::new(); + for (index, query) in queries.iter().enumerate() { + let config = config.clone(); + let request = admin::OpFindRequest { + query: query.clone(), + reason: reason.clone(), + }; + finds.spawn(async move { + let response: Result = + admin_post_as_owner(&config, "/admin/op/find", &request).await; + + (index, response) + }); + } + let mut responses: Vec> = std::iter::repeat_with(|| None) + .take(queries.len()) + .collect(); + while let Some(joined) = finds.join_next().await { + let (index, response) = joined?; + responses[index] = Some(response?); + } + + let mut sources = Vec::new(); + let mut answers = Vec::new(); + for (query, response) in queries.iter().zip(responses) { + match response { + Some(admin::OpFindResponse::Found { + source, + items, + more, + }) => { + if !sources.contains(&source) { + sources.push(source); + } + answers.push(serde_json::json!({ + "query": query, "items": items, "more": more, - }))? - ); + })); + } + Some(admin::OpFindResponse::Invalid { message }) => { + bail!("refused: {message}") + } + Some(admin::OpFindResponse::Denied { message }) => bail!("denied: {message}"), + Some(admin::OpFindResponse::Failed { message }) => { + bail!("no items: {message}") + } + None => bail!("the daemon did not answer the search for {query:?}"), } - admin::OpFindResponse::Invalid { message } => bail!("refused: {message}"), - admin::OpFindResponse::Denied { message } => bail!("denied: {message}"), - admin::OpFindResponse::Failed { message } => bail!("no items: {message}"), } + + eprintln!("Approved on {}", sources.join(", ")); + let printed = if several { + serde_json::Value::Array(answers) + } else { + let mut answer = answers.pop().unwrap_or_default(); + if let Some(answer) = answer.as_object_mut() { + answer.remove("query"); + } + + answer + }; + println!("{}", serde_json::to_string_pretty(&printed)?); } OpCommand::Inject { in_file, diff --git a/src/secrets.rs b/src/secrets.rs index f8dc127..2237dc1 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -26,7 +26,9 @@ use tokio::{sync::Mutex, task::JoinSet}; use self::approved::{Approved, Consumer}; use crate::{ - companion::{Companions, CreatedItem, DeviceTrust, FindBatches, NewItem, ProgramValue}, + companion::{ + Companions, CreatedItem, DeviceTrust, FindBatches, NewItem, ProgramFound, ProgramValue, + }, config::{Config, SecretsConfig}, hub::client::HubClient, mac::{OnMac, TouchId}, @@ -894,23 +896,26 @@ pub struct OnePassword { hub: Option>, } -/// The keep grants, and how many times they were forgotten, so a read -/// approved before a forget cannot keep its values after it +/// The keep grants and approved finds, and how many times they were +/// forgotten, so a read or find approved before a forget cannot be kept +/// after it #[derive(Default)] struct KeptValues { - grants: HashMap<(Option, String), Kept>, + grants: HashMap<(Option, String), Kept>, + /// Find answers by cleaned reason and query + finds: HashMap<(Option, String), Kept>, forgotten: u64, } -/// A value a program may get again without asking until both clocks pass -/// its end: `Instant` stops while the Mac sleeps, the wall clock can be set -struct Kept { - value: ProgramValue, +/// What a program may get again without asking until both clocks pass its +/// end: `Instant` stops while the Mac sleeps, the wall clock can be set +struct Kept { + value: T, until: std::time::Instant, until_wall: std::time::SystemTime, } -impl Kept { +impl Kept { fn live(&self) -> bool { std::time::Instant::now() < self.until && std::time::SystemTime::now() < self.until_wall } @@ -1056,13 +1061,14 @@ impl OnePassword { Ok(count) } - /// Forgets every value read, and every value kept for programs, so the - /// next read of each asks again, as after a rotated key; how many config - /// values were held + /// Forgets every value read, every value kept for programs and every + /// find answer kept, so the next read or find asks again, as after a + /// rotated key; how many config values were held pub async fn forget(&self) -> usize { { let mut kept = self.kept.lock().await; kept.grants.clear(); + kept.finds.clear(); kept.forgotten += 1; } let mut known = self.known.lock().await; @@ -1104,6 +1110,7 @@ impl OnePassword { } /// How many times the grants were forgotten; pass it to [`Self::keep`] + /// or [`Self::keep_finds`] pub(crate) async fn forgotten(&self) -> u64 { self.kept.lock().await.forgotten } @@ -1138,6 +1145,50 @@ impl OnePassword { } } + /// The answer kept for a find of `query` with `reason`, both as cleaned, + /// while it is live + pub(crate) async fn kept_find( + &self, + reason: Option<&str>, + query: &str, + ) -> Option { + let mut kept = self.kept.lock().await; + kept.finds.retain(|_, entry| entry.live()); + + kept.finds + .get(&(reason.map(str::to_owned), query.to_owned())) + .map(|entry| entry.value.clone()) + } + + /// Keeps each approved find answer of `answers`, by query, for finds + /// with the same `reason` to get again without asking until `keep` has + /// passed; nothing is kept when the finds were forgotten since + /// `forgotten` was read + pub(crate) async fn keep_finds( + &self, + reason: Option<&str>, + answers: impl IntoIterator, + keep: Duration, + forgotten: u64, + ) { + let until = std::time::Instant::now() + keep; + let until_wall = std::time::SystemTime::now() + keep; + let mut kept = self.kept.lock().await; + if kept.forgotten != forgotten { + return; + } + for (query, found) in answers { + kept.finds.insert( + (reason.map(str::to_owned), query), + Kept { + value: found, + until, + until_wall, + }, + ); + } + } + /// Where the owner's presence is read and Touch ID asked, when this /// reader was set up with one pub fn on_mac(&self) -> Option<&OnMac> { diff --git a/tests/gateway/companion.rs b/tests/gateway/companion.rs index ee61915..32dfe06 100644 --- a/tests/gateway/companion.rs +++ b/tests/gateway/companion.rs @@ -1964,6 +1964,74 @@ async fn finds_a_script_starts_together_share_one_prompt() -> TestResult { rig.stop().await } +#[tokio::test(flavor = "multi_thread")] +async fn one_op_find_asks_once_for_every_query_and_a_rerun_asks_nothing() -> TestResult { + let Some(rig) = Rig::start(Options::default()).await? else { + return Ok(()); + }; + signing_items(&rig, "hunter2-signing-secret")?; + std::fs::write( + rig.companion_log.with_file_name("item-tok1.json"), + r#"{"id":"tok1","title":"Pushover","category":"API_CREDENTIAL", + "vault":{"id":"v1","name":"Personal"}, + "fields":[{"id":"credential","type":"CONCEALED","label":"credential", + "reference":"op://Personal/Pushover/credential"}]}"#, + )?; + let find_prompts = || { + rig.companion_mac + .shown() + .into_iter() + .filter(|prompt| prompt.contains("1Password items match")) + .count() + }; + let args = [ + "op", + "find", + "signing", + "--also", + "pushover", + "--reason", + "set up the release build", + ]; + + // an agent ran each of these as its own `op find` and got a prompt each + let (ok, stdout, stderr) = cli(&rig, &args).await?; + assert!(ok, "{stderr}"); + let found: Value = serde_json::from_str(&stdout)?; + let titles: Vec<(&str, &str)> = found + .as_array() + .into_iter() + .flatten() + .flat_map(|answer| { + let query = answer["query"].as_str().unwrap_or_default(); + answer["items"] + .as_array() + .into_iter() + .flatten() + .map(move |item| (query, item["title"].as_str().unwrap_or_default())) + }) + .collect(); + assert_eq!( + titles, + [("signing", "Apple Signing"), ("pushover", "Pushover")] + ); + assert_eq!(find_prompts(), 1); + + // the same searches again, as after misreading the output, are answered + // without asking; another reason is asked + rig.companion_mac.set_answer(Some(TouchId::Review)); + let (ok, again, stderr) = cli(&rig, &args).await?; + assert!(ok, "{stderr}"); + assert_eq!(again, stdout); + assert_eq!(find_prompts(), 1); + + let (ok, _, stderr) = cli(&rig, &["op", "find", "signing", "--reason", "other"]).await?; + assert!(!ok && stderr.contains("denied"), "{stderr}"); + assert_eq!(find_prompts(), 2); + + rig.stop().await +} + #[tokio::test(flavor = "multi_thread")] async fn a_description_approved_on_a_mac_answers_its_matches_and_writes_nothing() -> TestResult { let Some(rig) = Rig::start(Options { From 0d7e8912dae32d1bdc730f2a878e2e168a7eaa3d Mon Sep 17 00:00:00 2001 From: Praveen Perera Date: Wed, 21 Oct 2026 22:49:49 -0500 Subject: [PATCH 2/3] Scope batched and kept finds by requester A find was batched and kept under its reason alone. On a hub, a local program that claimed a spoke's prefixed reason could join that spoke's batch or get its kept answers. Finds are now keyed by the requester's scope, the spoke's ID on a hub, as well as the reason. --- docs/design.md | 9 ++++--- src/admin.rs | 1 + src/companion/find.rs | 57 ++++++++++++++++++++++++++++++++++--------- src/env_files.rs | 1 + src/hub/server.rs | 2 ++ src/secrets.rs | 23 ++++++++--------- 6 files changed, 65 insertions(+), 28 deletions(-) diff --git a/docs/design.md b/docs/design.md index e0ed50e..d0af6da 100644 --- a/docs/design.md +++ b/docs/design.md @@ -284,10 +284,11 @@ approved wherever the owner is (`FindBatches`, keyed by reason so an unrelated program's find never shares an approval or a Deny), asked on its own task so a caller that gives up cannot strand the rest; the batch is dropped once every caller has. `op find --also` sends each query as its own `/admin/op/find` request at once, so the batch joins them. - An approved batch's answers are kept for 10 minutes in `KeptValues::finds`, by cleaned reason and query, and - a later find with both the same is answered from there without asking; a forget clears them, and a batch - approved while a forget ran keeps nothing. On a hub the reason carries the spoke's name, so a spoke's kept - answers serve only that spoke One prompt quotes every query; a companion answers `{"found_each": [...]}`, one `Found` per query in + An approved batch's answers are kept for 10 minutes in `KeptValues::finds`, by batch key and query, and a + later find with both the same is answered from there without asking; a forget clears them, and a batch + approved while a forget ran keeps nothing. On a hub, batches and kept answers are keyed by the spoke's ID as + well as the reason (`FindAsk::scope`), so neither a local program nor another spoke can join a spoke's batch + or get its kept answers by claiming its reason One prompt quotes every query; a companion answers `{"found_each": [...]}`, one `Found` per query in order, and one from before batches, which ignores `also` and answers `found`, is reported as needing an update. The Mac lists items once per batch and opens an item two queries match once The Mac that answers runs `op item list --format json`, which holds no values, and scores items by how many query words diff --git a/src/admin.rs b/src/admin.rs index 624a32f..5bca772 100644 --- a/src/admin.rs +++ b/src/admin.rs @@ -1449,6 +1449,7 @@ async fn op_find( .find_for_program(&companion::FindAsk { query: request.query, reason: request.reason, + scope: None, }) .await; diff --git a/src/companion/find.rs b/src/companion/find.rs index 3ee62e7..d421b8e 100644 --- a/src/companion/find.rs +++ b/src/companion/find.rs @@ -64,6 +64,10 @@ pub struct FindAsk { pub query: String, /// Why, shown quoted as the program's claim pub reason: Option, + /// Whose finds these are, such as a spoke's ID, so one requester's find + /// never joins another's batch or gets its kept answer whatever reason + /// it claims; `None` for this machine + pub scope: Option, } /// The items a find matched and the Mac that searched @@ -75,7 +79,8 @@ pub struct ProgramFound { pub found: Found, } -/// The finds waiting to be asked together, by their cleaned reason +/// The finds waiting to be asked together, by their scope and cleaned +/// reason, as [`batch_key`] joins them /// /// Keying by reason keeps one script's searches together without letting an /// unrelated program's find ride on the same approval or fall with its Deny @@ -117,7 +122,8 @@ impl OnePassword { } let query = checked_query(&ask.query).map_err(ProgramReadError::Invalid)?; let reason = clean_reason(ask.reason.as_deref()); - if let Some(found) = self.kept_find(reason.as_deref(), &query).await { + let key = batch_key(ask.scope.as_deref(), reason.as_deref()); + if let Some(found) = self.kept_find(key.as_deref(), &query).await { tracing::info!("find answered from one approved earlier"); return Ok(found); @@ -129,19 +135,19 @@ impl OnePassword { // one lock, so a batch cannot be taken for asking between the // check and the push let mut forming = self.find_batches().forming(); - match forming.get(&reason).cloned() { + match forming.get(&key).cloned() { Some(batch) => { let mut queued = batch.waiting(); queued.push(waiting); if queued.len() >= MAX_FIND_QUERIES { - forming.remove(&reason); + forming.remove(&key); } None } None => { let batch = Arc::new(Forming(Mutex::new(vec![waiting]))); - forming.insert(reason.clone(), Arc::clone(&batch)); + forming.insert(key.clone(), Arc::clone(&batch)); Some(batch) } @@ -151,7 +157,7 @@ impl OnePassword { // cannot strand the others that joined it if let Some(batch) = opened { let onepassword = self.clone(); - tokio::spawn(async move { onepassword.ask_batch(&batch, reason).await }); + tokio::spawn(async move { onepassword.ask_batch(&batch, key, reason).await }); } answered.await.unwrap_or_else(|_| { @@ -162,18 +168,18 @@ impl OnePassword { } /// Waits out the window, then asks once for every find that joined - /// `batch` and hands each its own items - async fn ask_batch(&self, batch: &Arc, reason: Option) { + /// `batch`, filed under `key`, and hands each its own items + async fn ask_batch(&self, batch: &Arc, key: Option, reason: Option) { tokio::time::sleep(FIND_BATCH_WINDOW).await; let mut waiting = { let mut forming = self.find_batches().forming(); // a full batch already left the map, and a newer one may hold // its place if forming - .get(&reason) + .get(&key) .is_some_and(|current| Arc::ptr_eq(current, batch)) { - forming.remove(&reason); + forming.remove(&key); } std::mem::take(&mut *batch.waiting()) @@ -206,7 +212,7 @@ impl OnePassword { (query.clone(), found) }); - self.keep_finds(reason.as_deref(), answers, FIND_KEEP, forgotten) + self.keep_finds(key.as_deref(), answers, FIND_KEEP, forgotten) .await; } @@ -339,6 +345,16 @@ impl OnePassword { } } +/// What finds are batched and kept under: the cleaned reason alone for this +/// machine, otherwise the scope and the reason on separate lines; a cleaned +/// reason holds no line break, so a scoped key never equals an unscoped one +fn batch_key(scope: Option<&str>, reason: Option<&str>) -> Option { + match scope { + Some(scope) => Some(format!("{scope}\n{}", reason.unwrap_or(""))), + None => reason.map(str::to_owned), + } +} + impl Companions { /// Asks the companion Mac `peer` to search for `find.query` and each of /// `find.also`, answering their items in that order @@ -398,3 +414,22 @@ impl Companions { } } } + +#[cfg(test)] +mod tests { + use super::{batch_key, clean_reason}; + + #[test] + fn no_reason_this_machine_claims_files_its_find_under_a_spokes_key() { + let spoke = batch_key(Some("spoke1"), clean_reason(Some("set up")).as_deref()); + for claimed in [ + "set up", + "spoke1\nset up", + "spoke1\r\nset up", + "spoke1: set up", + ] { + let local = batch_key(None, clean_reason(Some(claimed)).as_deref()); + assert_ne!(local, spoke, "{claimed:?}"); + } + } +} diff --git a/src/env_files.rs b/src/env_files.rs index effe85d..c668c66 100644 --- a/src/env_files.rs +++ b/src/env_files.rs @@ -351,6 +351,7 @@ impl SecretsServer { let ask = FindAsk { query: params.query, reason: Some(params.reason), + scope: None, }; let found = tokio::select! { found = self.0.onepassword.find_for_program(&ask) => found.map_err(failed)?, diff --git a/src/hub/server.rs b/src/hub/server.rs index fc41ce3..6e971bd 100644 --- a/src/hub/server.rs +++ b/src/hub/server.rs @@ -1014,6 +1014,8 @@ async fn op_find( .find_for_program(&FindAsk { query: find.query, reason, + // spokes that share a name never share a batch or kept answers + scope: Some(spoke.id.clone()), }) .await; diff --git a/src/secrets.rs b/src/secrets.rs index 2237dc1..5333534 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -902,7 +902,8 @@ pub struct OnePassword { #[derive(Default)] struct KeptValues { grants: HashMap<(Option, String), Kept>, - /// Find answers by cleaned reason and query + /// Find answers by the key their batch was filed under, its scope and + /// cleaned reason, and query finds: HashMap<(Option, String), Kept>, forgotten: u64, } @@ -1145,28 +1146,24 @@ impl OnePassword { } } - /// The answer kept for a find of `query` with `reason`, both as cleaned, - /// while it is live - pub(crate) async fn kept_find( - &self, - reason: Option<&str>, - query: &str, - ) -> Option { + /// The answer kept for a find of `query` filed under `key`, the scope + /// and cleaned reason of its batch, while it is live + pub(crate) async fn kept_find(&self, key: Option<&str>, query: &str) -> Option { let mut kept = self.kept.lock().await; kept.finds.retain(|_, entry| entry.live()); kept.finds - .get(&(reason.map(str::to_owned), query.to_owned())) + .get(&(key.map(str::to_owned), query.to_owned())) .map(|entry| entry.value.clone()) } /// Keeps each approved find answer of `answers`, by query, for finds - /// with the same `reason` to get again without asking until `keep` has - /// passed; nothing is kept when the finds were forgotten since + /// filed under the same `key` to get again without asking until `keep` + /// has passed; nothing is kept when the finds were forgotten since /// `forgotten` was read pub(crate) async fn keep_finds( &self, - reason: Option<&str>, + key: Option<&str>, answers: impl IntoIterator, keep: Duration, forgotten: u64, @@ -1179,7 +1176,7 @@ impl OnePassword { } for (query, found) in answers { kept.finds.insert( - (reason.map(str::to_owned), query), + (key.map(str::to_owned), query), Kept { value: found, until, From 78ad64cb53f680a39529ff6cc0d0024c84715015 Mon Sep 17 00:00:00 2001 From: Praveen Perera Date: Wed, 21 Oct 2026 22:53:48 -0500 Subject: [PATCH 3/3] Clear kept grants after forgetting known values A read or find kept while forget waited for the known values could outlast the forget. Clearing the kept grants and finds last means nothing kept during that wait survives it. --- src/secrets.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/secrets.rs b/src/secrets.rs index 5333534..e7cb713 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -1066,15 +1066,19 @@ impl OnePassword { /// find answer kept, so the next read or find asks again, as after a /// rotated key; how many config values were held pub async fn forget(&self) -> usize { - { - let mut kept = self.kept.lock().await; - kept.grants.clear(); - kept.finds.clear(); - kept.forgotten += 1; - } - let mut known = self.known.lock().await; - let count = known.len(); - known.clear(); + let count = { + let mut known = self.known.lock().await; + let count = known.len(); + known.clear(); + + count + }; + // cleared last, after any wait for `known`, so nothing kept while + // this waited outlasts the forget + let mut kept = self.kept.lock().await; + kept.grants.clear(); + kept.finds.clear(); + kept.forgotten += 1; count }