smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(pi-natives/shell): resolved shell minimizer bypass for compound/piped

Browse Source 
- Updated minimizer mode selection so `Compound`, `Piped`, and `Unsupported` plans now skip minimization.
- Removed `MarkedCommands` mode and marker-stripping paths from shell minimizer and buffering flow.
- Updated minimizer tests and changelog to document compound/piped commands bypassing minimization.
- Deleted marker metadata and helper infrastructure (`markers.rs` types/helpers) and related imports.
This commit is contained in:
can1357
2026-04-24 13:58:13 +02:00
parent ac76f0d66a
commit ec8405bb2c
7 changed files with 37 additions and 410 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pi-natives/src/shell.rs
+20 -49
View File
@@ -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<dyn brush_core::ExternalCommandOutputMarker> = 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<MinimizerResult> = 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))
crates/pi-natives/src/shell/minimizer.rs
-1
View File
@@ -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;
crates/pi-natives/src/shell/minimizer/engine.rs
+8 -163
View File
@@ -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<ParsedMarker> {
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<pipeline::TestOutcome> {
#[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)]
crates/pi-natives/src/shell/minimizer/markers.rs
-159
View File
@@ -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<HashMap<u64, MarkedCommand>>,
}
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<MarkedCommand> {
self.commands.lock().get(&id).cloned()
}
/// Finds the next marker at or after `from`.
pub fn find_marker(&self, text: &str, from: usize) -> Option<ParsedMarker> {
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<ExternalCommandOutputMarkers> {
let tokens = command_tokens(&info);
let identity = detect::detect_tokens(&tokens)?;
let command = tokens
.iter()
.map(|token| quote_command_token(token))
.collect::<Vec<_>>()
.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<String> {
std::iter::once(info.command_name)
.chain(info.args.iter().copied())
.map(ToOwned::to_owned)
.collect()
}
fn parse_marker_body(body: &str) -> Option<MarkerKind> {
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('\'', "'\\''"))
}
crates/pi-natives/src/shell/minimizer/plan.rs
+5 -6
View File
@@ -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.
packages/coding-agent/src/modes/acp/acp-agent.ts
+3 -32
View File
@@ -154,37 +154,10 @@ export class AcpAgent implements Agent {
#disposePromise: Promise<void> | undefined;
#cleanupRegistered = false;
#listAllCache: { at: number; sessions: StoredSessionInfo[] } | null = null;
#listAllInFlight: Promise<StoredSessionInfo[]> | 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<StoredSessionInfo[]> {
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<InitializeResponse> {
@@ -229,7 +202,6 @@ export class AcpAgent implements Agent {
async newSession(params: NewSessionRequest): Promise<NewSessionResponse> {
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<string, { cwd: string; sessionCount: number; lastActivityAt: number; lastTitle: string }>();
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)) };
}
packages/natives/CHANGELOG.md
+1
View File
@@ -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