From 92b30fbde2fec3ae0aecfb44d8aa4c92a8694185 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 15 Oct 2026 06:27:32 +0200 Subject: [PATCH 1/2] fix(sandbox): keep Arc in Jobs, guard attribute list and remove unwraps Fixes #299 Co-authored-by: Cursor --- CHANGELOG.md | 1 + native/sandbox-helper/src/file_ops/unix.rs | 64 +++++--- native/sandbox-helper/src/file_ops/windows.rs | 37 ++--- native/sandbox-helper/src/pipe_io.rs | 2 +- native/sandbox-helper/src/text.rs | 10 +- native/sandbox-helper/src/win.rs | 144 +++++++++++++----- project-map/data.js | 10 +- project-map/views/10-focus.js | 4 +- 8 files changed, 176 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c777c7..1ab16cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ Patch's changes are consolidated below as an unreleased baseline, not published ### Fixed +- Windows sandbox helper (`native/sandbox-helper`): `Jobs` holds an `Arc` for each running job instead of a raw `usize`, `LPPROC_THREAD_ATTRIBUTE_LIST` is freed on early returns as well as after process creation, and mutex locks recover from poison instead of panicking (fixes #299). - A file the chat read that changed right as a run ended (a formatter or file watcher writing as soon as the agent stops) was not reported to the model as changed outside the chat: its size and time were taken in the background after the run, so the change could become part of the baseline. They are now taken before the run reports it is done (fixes #290). - The native file helper read a whole file into memory before Patch's size checks ran, so an edit of, or a read raced against, a multi-gigabyte file could exhaust memory in the helper and the app. It now refuses files over 16 MB (to read or write) without reading past the limit, caps its request at 128 MB, and the app stops collecting its output past 24 MB; the sandbox helper caps each request line at 16 MB (fixes #297). - An edit to a protected top-level entry (`.github`, `.vscode`, `AGENTS.md`) by an absolute path that spells the project root through an alias (`/var` for `/private/var` on macOS, a Windows 8.3 name such as `RUNNER~1`) was not recorded as Patch's own write, so the sandbox guard could move the approved edit aside as if a command had created it. The record now uses the same alias-aware relative path as the write (refs #276). diff --git a/native/sandbox-helper/src/file_ops/unix.rs b/native/sandbox-helper/src/file_ops/unix.rs index 7823d10..5bde3fb 100644 --- a/native/sandbox-helper/src/file_ops/unix.rs +++ b/native/sandbox-helper/src/file_ops/unix.rs @@ -59,7 +59,9 @@ fn write_in_place(file: &mut File, bytes: &[u8]) -> io::Result<()> { // The new file gets the old one's mode, and its owner where that is allowed. When no file can be created in the folder // (it is not writable), it is written in place as before. fn replace(change: &mut Pending, bytes: &[u8]) -> io::Result<()> { - let leaf = change.parts.last().unwrap().clone(); + let Some(leaf) = change.parts.last().cloned() else { + return Err(invalid("A file path is required.")); + }; let (Some(parent), Some(old)) = (change.parent.as_ref(), change.file.as_mut()) else { return Err(invalid("Missing the file to replace.")); }; @@ -160,14 +162,13 @@ impl Root { } pub fn read(&self, parts: &[String]) -> io::Result>> { + let Some(leaf) = parts.last() else { + return Err(invalid("A file path is required.")); + }; let Some(parent) = self.parent(parts, false, &mut Vec::new())? else { return Ok(None); }; - let mut file = match open_at( - &parent, - parts.last().unwrap(), - libc::O_RDONLY | libc::O_NONBLOCK, - ) { + let mut file = match open_at(&parent, leaf, libc::O_RDONLY | libc::O_NONBLOCK) { Ok(file) => file, Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(None), Err(e) => return Err(e), @@ -182,13 +183,12 @@ impl Root { before: Option>, after: Option>, ) -> io::Result { + let Some(leaf) = parts.last() else { + return Err(invalid("A file path is required.")); + }; let parent = self.parent(&parts, false, &mut Vec::new())?; let mut file = match &parent { - Some(parent) => match open_at( - parent, - parts.last().unwrap(), - libc::O_RDWR | libc::O_NONBLOCK, - ) { + Some(parent) => match open_at(parent, leaf, libc::O_RDWR | libc::O_NONBLOCK) { Ok(file) => Some(file), Err(e) if e.kind() == io::ErrorKind::NotFound => None, Err(e) => return Err(e), @@ -218,20 +218,28 @@ impl Root { } pub fn apply(&self, change: &mut Pending) -> io::Result<()> { - let leaf = change.parts.last().unwrap(); + let Some(leaf) = change.parts.last().cloned() else { + return Err(invalid("A file path is required.")); + }; if let Some(bytes) = &change.after { - if change.file.is_none() { - if change.parent.is_none() { - change.parent = self.parent(&change.parts, true, &mut change.created_dirs)?; + let file = match change.file.as_mut() { + Some(file) => file, + None => { + let parent = match change.parent.as_ref() { + Some(parent) => parent, + None => { + let parent = self + .parent(&change.parts, true, &mut change.created_dirs)? + .ok_or_else(|| invalid("Missing parent folder."))?; + change.parent.insert(parent) + } + }; + let file = + open_at(parent, &leaf, libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL)?; + change.created_file = true; + change.file.insert(file) } - change.file = Some(open_at( - change.parent.as_ref().unwrap(), - leaf, - libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL, - )?); - change.created_file = true; - } - let file = change.file.as_mut().unwrap(); + }; regular(file, true)?; change.written = true; if change.created_file { @@ -242,7 +250,11 @@ impl Root { replace(change, &bytes)?; } } else if change.file.is_some() { - unlink_at(change.parent.as_ref().unwrap(), leaf, 0)?; + let parent = change + .parent + .as_ref() + .ok_or_else(|| invalid("Missing parent folder."))?; + unlink_at(parent, &leaf, 0)?; change.deleted = true; } Ok(()) @@ -252,7 +264,9 @@ impl Root { // bytes, a created file and the folders created for it are removed, and a deleted file is written again with its // old bytes and mode. pub fn undo(&self, change: &mut Pending) -> io::Result<()> { - let leaf = change.parts.last().unwrap().clone(); + let Some(leaf) = change.parts.last().cloned() else { + return Err(invalid("A file path is required.")); + }; if change.deleted { let (Some(parent), Some(before)) = (change.parent.as_ref(), &change.before) else { return Err(invalid("Missing the original content to restore.")); diff --git a/native/sandbox-helper/src/file_ops/windows.rs b/native/sandbox-helper/src/file_ops/windows.rs index 1fe918e..93b9e1d 100644 --- a/native/sandbox-helper/src/file_ops/windows.rs +++ b/native/sandbox-helper/src/file_ops/windows.rs @@ -47,9 +47,7 @@ pub struct Pending { // Marks the file behind a handle for deletion when its last handle closes, or takes the mark back. fn set_delete_on_close(file: &File, delete: bool) -> io::Result<()> { - let disposition = FILE_DISPOSITION_INFO { - DeleteFile: delete.into(), - }; + let disposition = FILE_DISPOSITION_INFO { DeleteFile: delete }; unsafe { SetFileInformationByHandle( HANDLE(file.as_raw_handle()), @@ -353,9 +351,12 @@ impl Root { create: bool, created: &mut Vec, ) -> io::Result<(PathBuf, Vec)> { + let Some((leaf, parents)) = parts.split_last() else { + return Err(invalid("A file path is required.")); + }; let mut path = self.path.clone(); let mut handles = Vec::new(); - for component in &parts[..parts.len() - 1] { + for component in parents { path.push(component); match directory(&path) { Ok(handle) => handles.push(handle), @@ -370,7 +371,7 @@ impl Root { Err(e) => return Err(e), } } - path.push(parts.last().unwrap()); + path.push(leaf); Ok((path, handles)) } @@ -458,23 +459,25 @@ impl Root { pub fn apply(&self, change: &mut Pending) -> io::Result<()> { if let Some(bytes) = &change.after { - if change.file.is_none() { - // Rewalk (and retain) any previously missing parents before creating the file exclusively. - let (path, parents) = self.parent(&change.parts, true, &mut change.created_dirs)?; - change.path = path; - change._parents = parents; - change.file = Some( - OpenOptions::new() + let file = match change.file.as_mut() { + Some(file) => file, + None => { + // Rewalk (and retain) any previously missing parents before creating the file exclusively. + let (path, parents) = + self.parent(&change.parts, true, &mut change.created_dirs)?; + change.path = path; + change._parents = parents; + let file = OpenOptions::new() .write(true) .create_new(true) .access_mode(CREATED_FILE_ACCESS) .share_mode(FILE_SHARE_READ.0) .custom_flags(FILE_FLAG_OPEN_REPARSE_POINT.0) - .open(&change.path)?, - ); - change.created_file = true; - } - let file = change.file.as_mut().unwrap(); + .open(&change.path)?; + change.created_file = true; + change.file.insert(file) + } + }; regular(file, true)?; change.written = true; if change.created_file { diff --git a/native/sandbox-helper/src/pipe_io.rs b/native/sandbox-helper/src/pipe_io.rs index ace0130..3281f9e 100644 --- a/native/sandbox-helper/src/pipe_io.rs +++ b/native/sandbox-helper/src/pipe_io.rs @@ -17,7 +17,7 @@ pub fn wide(text: &str) -> Vec { } fn io_error(error: windows::core::Error) -> io::Error { - io::Error::from_raw_os_error((error.code().0 & 0xffff) as i32) + io::Error::from_raw_os_error(error.code().0 & 0xffff) } // A manual-reset event: once set, every wait that includes it returns at once. diff --git a/native/sandbox-helper/src/text.rs b/native/sandbox-helper/src/text.rs index a0a30e6..26c2469 100644 --- a/native/sandbox-helper/src/text.rs +++ b/native/sandbox-helper/src/text.rs @@ -11,18 +11,18 @@ pub fn quote_arg(arg: &str) -> String { match c { '\\' => backslashes += 1, '"' => { - out.extend(std::iter::repeat('\\').take(backslashes * 2 + 1)); + out.extend(std::iter::repeat_n('\\', backslashes * 2 + 1)); out.push('"'); backslashes = 0; } _ => { - out.extend(std::iter::repeat('\\').take(backslashes)); + out.extend(std::iter::repeat_n('\\', backslashes)); out.push(c); backslashes = 0; } } } - out.extend(std::iter::repeat('\\').take(backslashes * 2)); + out.extend(std::iter::repeat_n('\\', backslashes * 2)); out.push('"'); out } @@ -93,7 +93,9 @@ impl Utf8Chunker { } Err(e) => { let valid = e.valid_up_to(); - out.push_str(std::str::from_utf8(&self.pending[..valid]).unwrap()); + if let Ok(text) = std::str::from_utf8(&self.pending[..valid]) { + out.push_str(text); + } match e.error_len() { Some(len) => { out.push('\u{FFFD}'); diff --git a/native/sandbox-helper/src/win.rs b/native/sandbox-helper/src/win.rs index 7d1cc18..5c0e970 100644 --- a/native/sandbox-helper/src/win.rs +++ b/native/sandbox-helper/src/win.rs @@ -9,7 +9,7 @@ use std::io::{BufRead, Read, Write}; use std::os::windows::fs::{MetadataExt, OpenOptionsExt}; use std::os::windows::io::AsRawHandle; use std::path::{Path, PathBuf}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, PoisonError}; use std::thread; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use windows::core::{Error, PCWSTR, PWSTR}; @@ -76,6 +76,38 @@ impl Drop for LocalMem { } } +struct AttributeList { + _buffer: Vec, + list: LPPROC_THREAD_ATTRIBUTE_LIST, +} + +impl AttributeList { + fn new(count: u32) -> Result { + let mut size = 0usize; + unsafe { + let _ = InitializeProcThreadAttributeList(None, count, None, &mut size); + } + let mut buffer = vec![0u8; size]; + let list = LPPROC_THREAD_ATTRIBUTE_LIST(buffer.as_mut_ptr() as *mut c_void); + unsafe { + InitializeProcThreadAttributeList(Some(list), count, None, &mut size) + .map_err(|e| describe("InitializeProcThreadAttributeList", e))?; + } + Ok(Self { + _buffer: buffer, + list, + }) + } +} + +impl Drop for AttributeList { + fn drop(&mut self) { + unsafe { + DeleteProcThreadAttributeList(self.list); + } + } +} + struct ProjectDrive { name: Vec, target: Vec, @@ -240,21 +272,32 @@ pub struct Emitter(Arc>); impl Emitter { fn send(&self, event: &Event) { - let mut out = self.0.lock().unwrap(); + let mut out = self.0.lock().unwrap_or_else(PoisonError::into_inner); let _ = writeln!(out, "{}", encode_event(event)); let _ = out.flush(); } } -// Jobs of the commands in flight, so end of input or a kill message can stop them. -// Some(0) is preparation, None is cancellation, Some(handle) is a running job. -type Jobs = Arc>>>; +// Jobs of the commands in flight, so end of input or a kill message can stop them. Holding an `Arc` keeps +// the kernel job handle alive while `kill_job` calls `TerminateJobObject` (#299). +#[derive(Clone)] +enum JobState { + Preparing, + Running(Arc), + Cancelled, +} + +type Jobs = Arc>>; fn ensure_active(jobs: &Jobs, id: u64) -> Result<()> { - if jobs.lock().unwrap().get(&id).copied().flatten().is_none() { - Err("sandbox command stopped during preparation".to_string()) - } else { + let active = jobs.lock().unwrap_or_else(PoisonError::into_inner); + if matches!( + active.get(&id), + Some(JobState::Preparing | JobState::Running(_)) + ) { Ok(()) + } else { + Err("sandbox command stopped during preparation".to_string()) } } @@ -295,7 +338,9 @@ pub fn serve() { match parse_message(&line) { Ok(Message::Run(request)) => { let (emitter, jobs) = (emitter.clone(), jobs.clone()); - jobs.lock().unwrap().insert(request.id, Some(0)); + jobs.lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(request.id, JobState::Preparing); workers.push(thread::spawn(move || { let id = request.id; if let Err(message) = run(&request, &emitter, &jobs) { @@ -304,7 +349,9 @@ pub fn serve() { message: &message, }); } - jobs.lock().unwrap().remove(&id); + jobs.lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(&id); })); } // On a worker like a run, so a slow revoke (it waits for the permission lock) never delays reading the @@ -365,28 +412,34 @@ pub fn serve_stdio(path: &str) { } }; let jobs: Jobs = Arc::new(Mutex::new(HashMap::new())); - jobs.lock().unwrap().insert(request.id, Some(0)); + jobs.lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(request.id, JobState::Preparing); match run_inner(&request, None, &jobs) { Ok(code) => std::process::exit(code), Err(message) => { eprintln!("Patch sandbox: {message}"); - jobs.lock().unwrap().remove(&request.id); + jobs.lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(&request.id); std::process::exit(126); } } } fn kill_job(jobs: &Jobs, id: Option) { - for (job_id, job) in jobs.lock().unwrap().iter_mut() { + for (job_id, job) in jobs + .lock() + .unwrap_or_else(PoisonError::into_inner) + .iter_mut() + { if id.is_none() || id == Some(*job_id) { - if let Some(handle) = *job { - if handle != 0 { - unsafe { - let _ = TerminateJobObject(HANDLE(handle as *mut c_void), 1); - } + if let JobState::Running(handle) = job { + unsafe { + let _ = TerminateJobObject(handle.0, 1); } } - *job = None; + *job = JobState::Cancelled; } } } @@ -2529,17 +2582,10 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul let input_write = if stdio { Some(input_write) } else { None }; let mut inherited = [input.0, stdout.write.0, stderr.write.0]; - let mut list_size = 0usize; - unsafe { - let _ = InitializeProcThreadAttributeList(None, 2, None, &mut list_size); - } - let mut list_buffer = vec![0u8; list_size]; - let list = LPPROC_THREAD_ATTRIBUTE_LIST(list_buffer.as_mut_ptr() as *mut c_void); + let attributes = AttributeList::new(2)?; unsafe { - InitializeProcThreadAttributeList(Some(list), 2, None, &mut list_size) - .map_err(|e| describe("InitializeProcThreadAttributeList", e))?; UpdateProcThreadAttribute( - list, + attributes.list, 0, PROC_THREAD_ATTRIBUTE_SECURITY_CAPABILITIES as usize, Some(&mut security as *mut _ as *const c_void), @@ -2549,7 +2595,7 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul ) .map_err(|e| describe("UpdateProcThreadAttribute (container)", e))?; UpdateProcThreadAttribute( - list, + attributes.list, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST as usize, Some(inherited.as_mut_ptr() as *const c_void), @@ -2566,7 +2612,7 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul startup.StartupInfo.hStdInput = input.0; startup.StartupInfo.hStdOutput = stdout.write.0; startup.StartupInfo.hStdError = stderr.write.0; - startup.lpAttributeList = list; + startup.lpAttributeList = attributes.list; // An ordinary AppContainer cannot traverse a Windows 11 volume root, even when its project // itself is granted. A temporary drive makes that project the root without exposing its parents. @@ -2708,7 +2754,7 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul &mut info, ) }; - unsafe { DeleteProcThreadAttributeList(list) }; + drop(attributes); created.map_err(|e| describe(&format!("Starting {}", request.command), e))?; let process = Handle(info.hProcess); let thread_handle = Handle(info.hThread); @@ -2717,7 +2763,7 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul let job = match make_job(request).and_then(|job| { unsafe { AssignProcessToJobObject(job.0, process.0) } .map_err(|e| describe("AssignProcessToJobObject", e))?; - Ok(job) + Ok(Arc::new(job)) }) { Ok(job) => job, Err(message) => { @@ -2728,14 +2774,17 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul } }; { - let mut active = jobs.lock().unwrap(); - if active.get(&id).copied().flatten().is_none() { + let mut active = jobs.lock().unwrap_or_else(PoisonError::into_inner); + if !matches!( + active.get(&id), + Some(JobState::Preparing | JobState::Running(_)) + ) { unsafe { let _ = TerminateProcess(process.0, 1); } return Err("sandbox command stopped during preparation".to_string()); } - active.insert(id, Some(job.0 .0 as usize)); + active.insert(id, JobState::Running(job.clone())); } // Only the command holds the write ends now, so the readers end when it and its children are gone. @@ -2833,7 +2882,9 @@ fn run_inner(request: &Request, emitter: Option<&Emitter>, jobs: &Jobs) -> Resul let _ = reader.join(); } } - jobs.lock().unwrap().remove(&id); + jobs.lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(&id); drop(job); drop(toolchain_drive); // Cleanup runs under the permission lock, like setup. A failure is reported with the command's output, not @@ -3020,13 +3071,30 @@ mod tests { #[test] fn cancelled_preparation_never_registers_a_running_command() { - let jobs: Jobs = Arc::new(Mutex::new(HashMap::from([(7, Some(0))]))); + let jobs: Jobs = Arc::new(Mutex::new(HashMap::from([(7, JobState::Preparing)]))); ensure_active(&jobs, 7).unwrap(); kill_job(&jobs, Some(7)); assert!(ensure_active(&jobs, 7) .unwrap_err() .contains("stopped during preparation")); - assert_eq!(jobs.lock().unwrap().get(&7), Some(&None)); + assert!(matches!( + jobs.lock().unwrap().get(&7), + Some(JobState::Cancelled) + )); + // A poisoned lock still lets ensure_active and kill_job inspect and stop commands (#299). + let poisoned = jobs.clone(); + let _ = thread::spawn(move || { + let _guard = poisoned.lock().unwrap(); + panic!("poison"); + }) + .join(); + assert!(jobs.lock().is_err()); + jobs.lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(8, JobState::Preparing); + ensure_active(&jobs, 8).unwrap(); + kill_job(&jobs, Some(8)); + assert!(ensure_active(&jobs, 8).is_err()); } #[test] diff --git a/project-map/data.js b/project-map/data.js index e7745fb..773e265 100644 --- a/project-map/data.js +++ b/project-map/data.js @@ -608,14 +608,6 @@ const D = { 0, '10-13', ], - [ - 299, - 'Windows sandbox helper: Jobs stores raw HANDLE as usize; attribute list and mutex unwraps', - 'sandbox', - 'low', - 0, - '10-13', - ], [ 294, 'Sandboxed commands can replace a submodule’s .git, which host Git recurses into', @@ -730,7 +722,7 @@ const D = { other: 'Bugs', docs: 'Docs & naming', }, - closedCount: 228, + closedCount: 229, }; // src/renderer/src/styles.css, counted with the renderer's source lines. const RENDERER_CSS_LINES = 2797; diff --git a/project-map/views/10-focus.js b/project-map/views/10-focus.js index 7a19c96..7839390 100644 --- a/project-map/views/10-focus.js +++ b/project-map/views/10-focus.js @@ -8,9 +8,9 @@ builders.focus = () => { 'Fix the Oct 13 Electron + Rust audit findings (#299–#305)', 'An Electron, IPC and native-helper audit found strong process isolation, typed IPC schemas and handle-confined file operations, plus 10 issues across the Rust sidecar helpers, main-thread directory walks and browser DNS caching. The high one, #296 (sandbox-helper --stdio closed its own standard handles), is fixed, and so is #298: net-bridge is pinned by digest like the other helpers, and its Windows copy is read-only to the command. #297 is fixed too: the native helpers refuse files over 16 MB and cap their input.', [ - '#299, #300, #302 and #305 (low): Jobs stores raw HANDLE as usize and misses a Drop guard on LPPROC_THREAD_ATTRIBUTE_LIST; isAppPageUrl checks devRendererUrl with startsWith; openInEditor passes unscrubbed env and allows %VAR% in file paths on Windows; runtime renderer packages sit in devDependencies', + '#300, #302 and #305 (low): isAppPageUrl checks devRendererUrl with startsWith; openInEditor passes unscrubbed env and allows %VAR% in file paths on Windows; runtime renderer packages sit in devDependencies', ], - '#301 is fixed: a public page in the agent browser goes through a proxy that pins each connection to the address it checked, and #304: the network relay and net-bridge serve at most 64 connections at once, and #303: folder sizes for Windows grants are measured off the main thread. The low-severity items remain.', + '#301 is fixed: a public page in the agent browser goes through a proxy that pins each connection to the address it checked, #304: the network relay and net-bridge serve at most 64 connections at once, #303: folder sizes for Windows grants are measured off the main thread, and #299: the Windows sandbox helper keeps Arc in Jobs, frees its attribute list on early returns and recovers poisoned mutexes. The remaining low-severity items are next.', ], [ 'Fix the Oct 11 audit findings: the low ones next', From 37f8351123d03630eb0d7a60be07d372b27c7903 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 15 Oct 2026 06:54:53 +0200 Subject: [PATCH 2/2] docs: record #286 query redaction in docs, changelog and project map Co-authored-by: Cursor --- CHANGELOG.md | 1 + docs/ARCHITECTURE.md | 2 +- project-map/data.js | 15 +++++++-------- project-map/views/10-focus.js | 3 ++- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ab16cb..d5f66ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ Patch's changes are consolidated below as an unreleased baseline, not published ### Fixed +- `search_code` now runs the query through `redactSecrets` before sending it to OpenRouter for embedding and reranking, like the indexed chunks and `web_search` (fixes #286). - Windows sandbox helper (`native/sandbox-helper`): `Jobs` holds an `Arc` for each running job instead of a raw `usize`, `LPPROC_THREAD_ATTRIBUTE_LIST` is freed on early returns as well as after process creation, and mutex locks recover from poison instead of panicking (fixes #299). - A file the chat read that changed right as a run ended (a formatter or file watcher writing as soon as the agent stops) was not reported to the model as changed outside the chat: its size and time were taken in the background after the run, so the change could become part of the baseline. They are now taken before the run reports it is done (fixes #290). - The native file helper read a whole file into memory before Patch's size checks ran, so an edit of, or a read raced against, a multi-gigabyte file could exhaust memory in the helper and the app. It now refuses files over 16 MB (to read or write) without reading past the limit, caps its request at 128 MB, and the app stops collecting its output past 24 MB; the sandbox helper caps each request line at 16 MB (fixes #297). diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fd4bafa..a71e8db 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -191,7 +191,7 @@ The Windows helper also journals a private per-command root under `%LOCALAPPDATA ## Code search (`src/main/search/`) -`CodeIndex` walks the project (respecting ignore rules, up to _Maximum files to index_), skips credential files (`isSecretPath` in `tools/guard.ts`: `.env`, keys, `.npmrc`, `.ssh` and similar), splits text files into overlapping 60-line chunks, masks secrets in each chunk with `redactSecrets` (the reranker's documents too, #234), embeds them with Voyage `voyage-code-4` through OpenRouter's embeddings endpoint (`openRouterEmbedder`: `input_type` `document` for chunks and `query` for the search text, as Voyage recommends for retrieval; 1024 dimensions, the model's default) and stores Float32 vectors in `userData/indexes/.json`. Updates re-embed only files whose size or modification time changed, and each search refreshes the index once, reporting indexing progress to the tool card. A file is stored as soon as all its chunks are embedded, and the index file is saved every five batches and when the update ends, also after a failure or a stop, so a later run embeds only what is left. Concurrent searches share one update: each caller's stop ends only its own wait, and the update stops when every waiting caller has stopped. Cached indexes are dropped only when the OpenRouter key changes (#170); `CodeIndex.dispose()` then stops a running update after its current batch and keeps the dropped index from saving, so the old key is not used any more and the new index file is not overwritten (#248). The index is built on the first `search_code` call, never in the background. Search ranks by cosine similarity, at most two snippets per file. The best 30 of those go to Voyage `rerank-3` through OpenRouter's rerank endpoint (`openRouterReranker`, same key), which picks the final results; if reranking fails (anything but a stop), the results stay in embedding order and the tool output says why. Changing `INDEX_VERSION` or the embedding model rebuilds indexes. Settings shows the current project's index status (`index:status`: files and chunks, or why it is unavailable) and a Reindex button (`index:rebuild`, `CodeIndex.rebuild()`), which clears the index and embeds everything again. +`CodeIndex` walks the project (respecting ignore rules, up to _Maximum files to index_), skips credential files (`isSecretPath` in `tools/guard.ts`: `.env`, keys, `.npmrc`, `.ssh` and similar), splits text files into overlapping 60-line chunks, masks secrets in each chunk, the reranker's documents and the search query itself with `redactSecrets` (#234, #286), embeds them with Voyage `voyage-code-4` through OpenRouter's embeddings endpoint (`openRouterEmbedder`: `input_type` `document` for chunks and `query` for the search text, as Voyage recommends for retrieval; 1024 dimensions, the model's default) and stores Float32 vectors in `userData/indexes/.json`. Updates re-embed only files whose size or modification time changed, and each search refreshes the index once, reporting indexing progress to the tool card. A file is stored as soon as all its chunks are embedded, and the index file is saved every five batches and when the update ends, also after a failure or a stop, so a later run embeds only what is left. Concurrent searches share one update: each caller's stop ends only its own wait, and the update stops when every waiting caller has stopped. Cached indexes are dropped only when the OpenRouter key changes (#170); `CodeIndex.dispose()` then stops a running update after its current batch and keeps the dropped index from saving, so the old key is not used any more and the new index file is not overwritten (#248). The index is built on the first `search_code` call, never in the background. Search ranks by cosine similarity, at most two snippets per file. The best 30 of those go to Voyage `rerank-3` through OpenRouter's rerank endpoint (`openRouterReranker`, same key), which picks the final results; if reranking fails (anything but a stop), the results stay in embedding order and the tool output says why. Changing `INDEX_VERSION` or the embedding model rebuilds indexes. Settings shows the current project's index status (`index:status`: files and chunks, or why it is unavailable) and a Reindex button (`index:rebuild`, `CodeIndex.rebuild()`), which clears the index and embeds everything again. ## Chat list (`src/renderer/src/views/sidebar.ts`) diff --git a/project-map/data.js b/project-map/data.js index 773e265..063c58d 100644 --- a/project-map/data.js +++ b/project-map/data.js @@ -217,11 +217,11 @@ const D = { proc: 'main', files: 2, tests: 1, - lines: 580, - testLines: 606, + lines: 583, + testLines: 622, desc: 'Embedding code index for the search_code tool: Voyage voyage-code-4 embeddings and rerank-3 reranking through OpenRouter.', list: [ - 'code_index.ts (551; single index refresh per search with progress, #186; credential files skipped and secrets masked before embedding, #234; dispose() on a key change, #248)', + 'code_index.ts (554; single index refresh per search with progress, #186; credential files skipped and secrets masked in chunks, reranker documents and the query itself, #234, #286; dispose() on a key change, #248)', 'chunker.ts', ], }, @@ -398,7 +398,7 @@ const D = { { a: 'main/llm', f: 9, c: 120, src: 2302, tl: 3223 }, { a: 'main/tools', f: 35, c: 796, src: 6531, tl: 9644 }, { a: 'main/panels', f: 4, c: 87, src: 829, tl: 1314 }, - { a: 'main/other', f: 16, c: 244, src: 3911, tl: 3706 }, + { a: 'main/other', f: 16, c: 245, src: 3914, tl: 3722 }, { a: 'shared', f: 8, c: 100, src: 1445, tl: 1149 }, { a: 'renderer', f: 7, c: 23, src: 6695, tl: 242 }, { a: 'preload', f: 0, c: 0, src: 37, tl: 0 }, @@ -459,13 +459,13 @@ const D = { [629, 'main/index.ts'], [615, 'renderer/src/views/transcript.ts'], [600, 'main/chat_manager.ts'], + [554, 'main/search/code_index.ts'], [551, 'main/agent/claude_code.ts'], - [551, 'main/search/code_index.ts'], [532, 'main/panels/git.ts'], [511, 'main/llm/codex_auth.ts'], [485, 'main/settings.ts'], ], - changelogEntries: 312, + changelogEntries: 313, docs: [ ['docs/PERFORMANCE.md', 539], ['docs/USAGE.md', 321], @@ -636,7 +636,6 @@ const D = { [289, 'Claude Code chats: subagent tool cards end as "(stopped)"', 'other', 'low', 0, '10-11'], [288, 'Read tools and the code index follow hard links out of the project', 'security', 'low', 0, '10-11'], [287, 'ChatGPT tokens and MCP secrets saved as plain text without confirmation', 'security', 'low', 0, '10-11'], - [286, 'search_code sends its query to OpenRouter without redaction', 'security', 'low', 0, '10-11'], [285, 'apply_patch drops the BOM when line 1 changes; write_file drops CRLF and BOM', 'tools', 'low', 0, '10-11'], [284, 'An unreadable settings or projects file is treated as missing and later erased', 'other', 'low', 0, '10-11'], [ @@ -722,7 +721,7 @@ const D = { other: 'Bugs', docs: 'Docs & naming', }, - closedCount: 229, + closedCount: 230, }; // src/renderer/src/styles.css, counted with the renderer's source lines. const RENDERER_CSS_LINES = 2797; diff --git a/project-map/views/10-focus.js b/project-map/views/10-focus.js index 7839390..9fb8e6a 100644 --- a/project-map/views/10-focus.js +++ b/project-map/views/10-focus.js @@ -25,8 +25,9 @@ builders.focus = () => { "#280 is fixed: the app page's CSP allows no requests, so an injected script cannot fetch local files over file://", "#281 is fixed: a file:// page in the user's Browser session loads only files in its own folder and gets no network", '#282 is fixed: project:open opens a folder that is not in the project list only after a native confirmation', + '#286 is fixed: search_code masks secrets in the query before sending it for embedding and reranking', ], - 'Order: the low findings (#283 to #293), then #294.', + 'Order: the remaining low findings (#283 to #293), then #294.', ], [ 'The Oct 10 audit is fixed: approval bypasses and untrusted Claude Code settings',