diff --git a/crates/pi-natives/src/shell.rs b/crates/pi-natives/src/shell.rs index 8e03bbe1d..55f475564 100644 --- a/crates/pi-natives/src/shell.rs +++ b/crates/pi-natives/src/shell.rs @@ -655,16 +655,6 @@ async fn run_shell_command( } else { minimizer::engine::MinimizerMode::None }; - let marker_state = if matches!(minimizer_mode, minimizer::engine::MinimizerMode::MarkedCommands) - { - Some(Arc::new(minimizer::markers::CommandMarkerState::new())) - } else { - None - }; - if let Some(marker_state) = marker_state.as_ref() { - let marker: Arc = marker_state.clone(); - params.set_command_output_marker(marker); - } let should_minimize = !matches!(minimizer_mode, minimizer::engine::MinimizerMode::None); let max_capture_bytes = if let Some(config) = options.minimizer.as_ref() { config.max_capture_bytes as usize @@ -779,46 +769,27 @@ async fn run_shell_command( let mut minimized_out: Option = None; if let Some(OutputRead::Buffered(output)) = reader_output && let Some(config) = options.minimizer.as_ref() + && !output.exceeded { - if output.exceeded { - // Exceeded captures still flow through the raw stream already. Only - // surface a replacement when we actually need to strip markers. - if let Some(marker_state) = marker_state.as_ref() { - let stripped = minimizer::engine::strip_markers(&output.text, marker_state); - if stripped != output.text { - let input_bytes = u32::try_from(output.text.len()).unwrap_or(u32::MAX); - let output_bytes = u32::try_from(stripped.len()).unwrap_or(u32::MAX); - minimized_out = Some(MinimizerResult { - filter: "marker-strip".to_string(), - text: stripped, - original_text: output.text, - input_bytes, - output_bytes, - }); - } - } - } else { - let minimized = match (minimizer_mode, marker_state.as_ref()) { - (minimizer::engine::MinimizerMode::WholeCommand, _) => { - minimizer::apply(&options.command, &output.text, exit_code(&result), config) - }, - (minimizer::engine::MinimizerMode::MarkedCommands, Some(marker_state)) => { - minimizer::engine::apply_marked(&output.text, config, marker_state) - }, - _ => minimizer::MinimizerOutput::passthrough(&output.text), - }; - if minimized.changed - && let Some(original) = minimized.original_text - { - let output_bytes = u32::try_from(minimized.text.len()).unwrap_or(u32::MAX); - minimized_out = Some(MinimizerResult { - filter: minimized.filter.to_string(), - text: minimized.text, - original_text: original, - input_bytes: u32::try_from(minimized.input_bytes).unwrap_or(u32::MAX), - output_bytes, - }); - } + let minimized = match minimizer_mode { + minimizer::engine::MinimizerMode::WholeCommand => { + minimizer::apply(&options.command, &output.text, exit_code(&result), config) + }, + minimizer::engine::MinimizerMode::None => { + minimizer::MinimizerOutput::passthrough(&output.text) + }, + }; + if minimized.changed + && let Some(original) = minimized.original_text + { + let output_bytes = u32::try_from(minimized.text.len()).unwrap_or(u32::MAX); + minimized_out = Some(MinimizerResult { + filter: minimized.filter.to_string(), + text: minimized.text, + original_text: original, + input_bytes: u32::try_from(minimized.input_bytes).unwrap_or(u32::MAX), + output_bytes, + }); } } Ok((result, minimized_out)) diff --git a/crates/pi-natives/src/shell/minimizer.rs b/crates/pi-natives/src/shell/minimizer.rs index 108d60477..7e7fcc24f 100644 --- a/crates/pi-natives/src/shell/minimizer.rs +++ b/crates/pi-natives/src/shell/minimizer.rs @@ -9,7 +9,6 @@ pub mod config; pub mod detect; pub mod engine; pub mod filters; -pub mod markers; pub mod primitives; pub mod pipeline; diff --git a/crates/pi-natives/src/shell/minimizer/engine.rs b/crates/pi-natives/src/shell/minimizer/engine.rs index b2a9a13ee..61a1321bb 100644 --- a/crates/pi-natives/src/shell/minimizer/engine.rs +++ b/crates/pi-natives/src/shell/minimizer/engine.rs @@ -10,7 +10,6 @@ use std::{ use crate::shell::minimizer::{ MinimizerConfig, MinimizerCtx, MinimizerOutput, detect, filters, - markers::{CommandMarkerState, MarkerKind, ParsedMarker}, pipeline::{self, CompiledPipeline, PipelineRegistry}, plan, }; @@ -22,8 +21,6 @@ pub enum MinimizerMode { None, /// Capture the whole command and apply one filter to the whole buffer. WholeCommand, - /// Capture the whole command and filter marked external-command segments. - MarkedCommands, } /// Return the minimization mode for a command. @@ -39,14 +36,9 @@ pub fn mode_for(command: &str, config: &MinimizerConfig) -> MinimizerMode { MinimizerMode::None } }, - plan::CommandPlan::Compound => { - if config.enabled { - MinimizerMode::MarkedCommands - } else { - MinimizerMode::None - } + plan::CommandPlan::Compound | plan::CommandPlan::Piped | plan::CommandPlan::Unsupported => { + MinimizerMode::None }, - plan::CommandPlan::Piped | plan::CommandPlan::Unsupported => MinimizerMode::None, } } @@ -80,9 +72,9 @@ pub fn apply( } // Structural guard: this whole-buffer path only handles single simple - // commands. Compound commands are handled by launch-scoped markers. - // Pipes almost always feed a downstream parser (awk, jq, rg, …) and - // rewriting their input is a correctness bug. + // commands. Compound commands and pipes can feed downstream parsers + // (awk, jq, rg, …), so rewriting their combined output is a correctness + // bug. match plan::analyze(command) { plan::CommandPlan::Single { .. } => {}, plan::CommandPlan::Piped => { @@ -103,97 +95,6 @@ pub fn apply( apply_identity(&identity, command, captured, exit_code, config) } -/// Apply filters to output marked around individual external command launches. -pub fn apply_marked( - captured: &str, - config: &MinimizerConfig, - markers: &CommandMarkerState, -) -> MinimizerOutput { - let mut text = String::with_capacity(captured.len()); - let mut original = String::with_capacity(captured.len()); - let mut cursor = 0; - let mut changed = false; - - while let Some(marker) = markers.find_marker(captured, cursor) { - text.push_str(&captured[cursor..marker.start]); - original.push_str(&captured[cursor..marker.start]); - - match marker.kind { - MarkerKind::Start { id } => { - let Some(end_marker) = find_matching_end(captured, markers, marker.end, id) else { - cursor = marker.end; - continue; - }; - let MarkerKind::End { exit_code, .. } = end_marker.kind else { - cursor = marker.end; - continue; - }; - let segment = &captured[marker.end..end_marker.start]; - original.push_str(segment); - if let Some(command) = markers.command(id) { - let minimized = - apply_identity(&command.identity, &command.command, segment, exit_code, config); - if minimized.changed { - changed = true; - } - text.push_str(&minimized.text); - } else { - text.push_str(segment); - } - cursor = end_marker.end; - }, - MarkerKind::End { .. } => { - cursor = marker.end; - }, - } - } - - text.push_str(&captured[cursor..]); - original.push_str(&captured[cursor..]); - - if changed { - let output_bytes = text.len(); - return MinimizerOutput { - text, - changed: true, - input_bytes: original.len(), - output_bytes, - filter: "compound", - original_text: Some(original), - }; - } - - MinimizerOutput::passthrough(original).labeled("compound-noop") -} - -/// Remove command-boundary markers without applying filters. -pub fn strip_markers(captured: &str, markers: &CommandMarkerState) -> String { - let mut text = String::with_capacity(captured.len()); - let mut cursor = 0; - while let Some(marker) = markers.find_marker(captured, cursor) { - text.push_str(&captured[cursor..marker.start]); - cursor = marker.end; - } - text.push_str(&captured[cursor..]); - text -} - -fn find_matching_end( - captured: &str, - markers: &CommandMarkerState, - from: usize, - id: u64, -) -> Option { - let mut cursor = from; - while let Some(marker) = markers.find_marker(captured, cursor) { - if matches!(marker.kind, MarkerKind::End { id: end_id, .. } if end_id == id) { - return Some(marker); - } - cursor = marker.end; - } - None -} - fn identity_has_filter(identity: &detect::CommandIdentity, config: &MinimizerConfig) -> bool { if !config.is_program_enabled(&identity.program) { return false; @@ -395,10 +296,7 @@ pub fn verify_builtin_filters() -> Vec { #[cfg(test)] mod tests { - use brush_core::{ExternalCommandInfo, ExternalCommandOutputMarker}; - use super::*; - use crate::shell::minimizer::markers::CommandMarkerState; #[test] fn disabled_config_does_not_minimize() { @@ -427,65 +325,12 @@ mod tests { } #[test] - fn compound_commands_use_marked_mode() { + fn compound_and_piped_commands_do_not_minimize() { let cfg = MinimizerConfig { enabled: true, ..Default::default() }; - assert_eq!(mode_for("echo start ; git status", &cfg), MinimizerMode::MarkedCommands); - assert_eq!(mode_for("false && git status", &cfg), MinimizerMode::MarkedCommands); + assert_eq!(mode_for("echo start ; git status", &cfg), MinimizerMode::None); + assert_eq!(mode_for("false && git status", &cfg), MinimizerMode::None); assert_eq!(mode_for("git status | cat", &cfg), MinimizerMode::None); } - - #[test] - fn marked_output_filters_segment_and_preserves_original() { - let cfg = MinimizerConfig { enabled: true, ..Default::default() }; - let markers = CommandMarkerState::new(); - let command_markers = markers - .markers_for_external_command(ExternalCommandInfo { - command_name: "git", - executable_path: "/usr/bin/git", - args: vec!["status"], - }) - .expect("git marker should be created"); - let captured = format!( - "before\n{}## main\n M file.rs\n{}0{}after\n", - command_markers.start_marker, - command_markers.end_marker_prefix, - command_markers.end_marker_suffix, - ); - - let out = apply_marked(&captured, &cfg, &markers); - - assert!(out.changed); - assert_eq!(out.filter, "compound"); - assert_eq!(out.original_text.as_deref(), Some("before\n## main\n M file.rs\nafter\n")); - assert!(out.text.contains("before\n")); - assert!(out.text.contains("modified: 1")); - assert!(out.text.contains("after\n")); - } - - #[test] - fn marked_output_strips_markers_without_supported_filter() { - let cfg = MinimizerConfig { enabled: true, ..Default::default() }; - let markers = CommandMarkerState::new(); - let command_markers = markers - .markers_for_external_command(ExternalCommandInfo { - command_name: "unknown-tool", - executable_path: "/tmp/unknown-tool", - args: vec!["hello"], - }) - .expect("unknown marker should still be created"); - let captured = format!( - "{}raw\n{}0{}", - command_markers.start_marker, - command_markers.end_marker_prefix, - command_markers.end_marker_suffix, - ); - - let out = apply_marked(&captured, &cfg, &markers); - - assert!(!out.changed); - assert_eq!(out.text, "raw\n"); - assert_eq!(strip_markers(&captured, &markers), "raw\n"); - } } #[cfg(test)] diff --git a/crates/pi-natives/src/shell/minimizer/markers.rs b/crates/pi-natives/src/shell/minimizer/markers.rs deleted file mode 100644 index 70e781c5a..000000000 --- a/crates/pi-natives/src/shell/minimizer/markers.rs +++ /dev/null @@ -1,159 +0,0 @@ -//! Marker state for launch-scoped command output minimization. - -use std::{ - collections::HashMap, - sync::atomic::{AtomicU64, Ordering}, -}; - -use brush_core::{ExternalCommandInfo, ExternalCommandOutputMarker, ExternalCommandOutputMarkers}; -use parking_lot::Mutex; - -use crate::shell::minimizer::detect::{self, CommandIdentity}; - -const MARKER_END: char = '\x1f'; -const MARKER_NAME: &str = "\x1ePI_MINIMIZER"; - -static NEXT_TOKEN: AtomicU64 = AtomicU64::new(1); - -/// Command metadata associated with one marked launch. -#[derive(Clone, Debug)] -pub struct MarkedCommand { - /// Detected command identity used for minimizer dispatch. - pub identity: CommandIdentity, - /// Reconstructed command string for filters that inspect token text. - pub command: String, -} - -/// Parsed marker kind from the captured output stream. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum MarkerKind { - /// Start marker for a command id. - Start { id: u64 }, - /// End marker for a command id and its exit status. - End { id: u64, exit_code: i32 }, -} - -/// Parsed marker location in the captured output stream. -#[derive(Clone, Debug)] -pub struct ParsedMarker { - /// Marker byte start offset. - pub start: usize, - /// Marker byte end offset. - pub end: usize, - /// Parsed marker payload. - pub kind: MarkerKind, -} - -/// Per-shell-run state shared with brush-core marker hooks. -pub struct CommandMarkerState { - token: String, - next_id: AtomicU64, - commands: Mutex>, -} - -impl CommandMarkerState { - /// Creates a fresh marker namespace for one shell command invocation. - pub fn new() -> Self { - let token = NEXT_TOKEN.fetch_add(1, Ordering::Relaxed); - Self { - token: format!("{}:{token}", std::process::id()), - next_id: AtomicU64::new(1), - commands: Mutex::new(HashMap::new()), - } - } - - /// Returns command metadata for `id`, if it was launched by this run. - pub fn command(&self, id: u64) -> Option { - self.commands.lock().get(&id).cloned() - } - - /// Finds the next marker at or after `from`. - pub fn find_marker(&self, text: &str, from: usize) -> Option { - let prefix = self.marker_prefix(); - let relative_start = text.get(from..)?.find(&prefix)?; - let start = from + relative_start; - let body_start = start + prefix.len(); - let relative_end = text.get(body_start..)?.find(MARKER_END)?; - let body_end = body_start + relative_end; - let end = body_end + MARKER_END.len_utf8(); - let body = text.get(body_start..body_end)?; - let kind = parse_marker_body(body)?; - Some(ParsedMarker { start, end, kind }) - } - - fn marker_prefix(&self) -> String { - format!("{MARKER_NAME}:{}:", self.token) - } -} - -impl Default for CommandMarkerState { - fn default() -> Self { - Self::new() - } -} - -impl ExternalCommandOutputMarker for CommandMarkerState { - fn markers_for_external_command( - &self, - info: ExternalCommandInfo<'_>, - ) -> Option { - let tokens = command_tokens(&info); - let identity = detect::detect_tokens(&tokens)?; - let command = tokens - .iter() - .map(|token| quote_command_token(token)) - .collect::>() - .join(" "); - let id = self.next_id.fetch_add(1, Ordering::Relaxed); - self - .commands - .lock() - .insert(id, MarkedCommand { identity, command }); - - Some(ExternalCommandOutputMarkers { - start_marker: format!("{MARKER_NAME}:{}:S:{id}{MARKER_END}", self.token), - end_marker_prefix: format!("{MARKER_NAME}:{}:E:{id}:", self.token), - end_marker_suffix: MARKER_END.to_string(), - }) - } -} - -fn command_tokens(info: &ExternalCommandInfo<'_>) -> Vec { - std::iter::once(info.command_name) - .chain(info.args.iter().copied()) - .map(ToOwned::to_owned) - .collect() -} - -fn parse_marker_body(body: &str) -> Option { - let mut pieces = body.split(':'); - match pieces.next()? { - "S" => { - let id = pieces.next()?.parse().ok()?; - if pieces.next().is_some() { - return None; - } - Some(MarkerKind::Start { id }) - }, - "E" => { - let id = pieces.next()?.parse().ok()?; - let exit_code = pieces.next()?.parse().ok()?; - if pieces.next().is_some() { - return None; - } - Some(MarkerKind::End { id, exit_code }) - }, - _ => None, - } -} - -fn quote_command_token(token: &str) -> String { - if token - .chars() - .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '-' | '_' | '.' | '/' | ':' | '=' | '+')) - { - return token.to_string(); - } - - format!("'{}'", token.replace('\'', "'\\''")) -} diff --git a/crates/pi-natives/src/shell/minimizer/plan.rs b/crates/pi-natives/src/shell/minimizer/plan.rs index acec3e820..835e9275f 100644 --- a/crates/pi-natives/src/shell/minimizer/plan.rs +++ b/crates/pi-natives/src/shell/minimizer/plan.rs @@ -11,10 +11,9 @@ //! regardless of what `bar` is. A user piping through `awk`, `jq`, `rg`, or //! any other consumer is almost certainly parsing the output; rewriting it //! would be a correctness bug. The engine falls back to passthrough. -//! - **Compound commands need launch markers.** `a && b`, `a ; b`, and `a || b` -//! cannot be minimized as one combined buffer. The shell runtime can still -//! minimize eligible external command launches by marking their output -//! boundaries before this engine filters each segment. +//! - **Compound commands are opaque.** `a && b`, `a ; b`, and `a || b` cannot +//! be minimized as one combined buffer without risking semantic corruption, +//! so they are left unchanged. //! - **Single simple commands** are safe for the whole-buffer path; the engine //! dispatches them through `detect.rs` as before. //! @@ -37,8 +36,8 @@ pub enum CommandPlan { /// safe minimization for this engine. Piped, /// The command has multiple segments joined by `&&`, `||`, `;`, or `&`. - /// Foreground external commands inside this shape may still be minimized - /// through launch-scoped output markers. + /// This shape is left unchanged; the minimizer only rewrites whole simple + /// command output. Compound, /// Parse failed, a compound shell construct (for loops, subshells, etc.) /// was encountered, or the command was empty. diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index ea1f49584..dc7dd8986 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -154,37 +154,10 @@ export class AcpAgent implements Agent { #disposePromise: Promise | undefined; #cleanupRegistered = false; - #listAllCache: { at: number; sessions: StoredSessionInfo[] } | null = null; - #listAllInFlight: Promise | null = null; - constructor(connection: AgentSideConnection, initialSession: AgentSession, createSession: CreateAcpSession) { this.#connection = connection; this.#initialSession = initialSession; this.#createSession = createSession; - // Prefetch the global session list so the first GUI request is instant. - void this.#getAllSessionsCached(); - } - - async #getAllSessionsCached(): Promise { - const TTL = 60_000; - const now = Date.now(); - if (this.#listAllCache && now - this.#listAllCache.at < TTL) { - return this.#listAllCache.sessions; - } - if (this.#listAllInFlight) return this.#listAllInFlight; - this.#listAllInFlight = SessionManager.listAll() - .then(sessions => { - this.#listAllCache = { at: Date.now(), sessions }; - return sessions; - }) - .finally(() => { - this.#listAllInFlight = null; - }); - return this.#listAllInFlight; - } - - #invalidateListAllCache(): void { - this.#listAllCache = null; } async initialize(_params: InitializeRequest): Promise { @@ -229,7 +202,6 @@ export class AcpAgent implements Agent { async newSession(params: NewSessionRequest): Promise { this.#assertAbsoluteCwd(params.cwd); - this.#invalidateListAllCache(); const record = await this.#createNewSessionRecord(params.cwd, params.mcpServers); const response: NewSessionResponse = { sessionId: record.session.sessionId, @@ -415,7 +387,7 @@ export class AcpAgent implements Agent { switch (method) { case "omp/sessions/listAll": { const limit = typeof params["limit"] === "number" ? Math.max(1, Math.min(5000, params["limit"] as number)) : 1000; - const sessions = await this.#getAllSessionsCached(); + const sessions = await SessionManager.listAll(); const sorted = sessions.sort((l, r) => r.modified.getTime() - l.modified.getTime()).slice(0, limit); return { sessions: sorted.map(s => this.#toSessionInfo(s)), @@ -423,7 +395,7 @@ export class AcpAgent implements Agent { }; } case "omp/projects/list": { - const sessions = await this.#getAllSessionsCached(); + const sessions = await SessionManager.listAll(); const buckets = new Map(); for (const s of sessions) { if (!s.cwd) continue; @@ -451,8 +423,7 @@ export class AcpAgent implements Agent { const cwd = typeof params["cwd"] === "string" ? (params["cwd"] as string) : undefined; if (!cwd) throw new Error("cwd required"); const limit = typeof params["limit"] === "number" ? Math.max(1, Math.min(500, params["limit"] as number)) : 100; - const all = await this.#getAllSessionsCached(); - const sessions = all.filter(s => s.cwd === cwd); + const sessions = await SessionManager.list(cwd); const sorted = sessions.sort((l, r) => r.modified.getTime() - l.modified.getTime()).slice(0, limit); return { sessions: sorted.map(s => this.#toSessionInfo(s)) }; } diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index b2cab7bef..6a835f30f 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -12,6 +12,7 @@ ### Changed - Changed the shell output minimizer to more aggressively compact successful test runs, git output, large listings, grep/find results, source reads, and dependency manifests +- Changed compound and piped shell commands to bypass output minimization entirely, keeping minimization limited to eligible whole-command output after the command exits ### Removed