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

fix(minimizer): address PR 2176 review feedback

Browse Source
This commit is contained in:
David Andrews (LexGenius.ai)
2026-06-09 06:00:59 -04:00
committed by can1357
parent ce7ec45d42
commit 3a73107cce
11 changed files with 70 additions and 166 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pi-natives/src/shell.rs
-121
View File
@@ -344,85 +344,6 @@ pub fn apply_bash_fixups(command: String) -> BashFixupResult {
core_apply_bash_fixups(&command).into()
}
/// Inputs for [`apply_shell_minimizer`]: a captured command's text plus the
/// minimizer configuration to run against it.
#[napi(object)]
pub struct ShellMinimizerApplyOptions {
/// The command line that produced `captured` (used to select a filter).
pub command: String,
/// The full captured stdout/stderr to minimize.
pub captured: String,
/// The command's exit status; omitted is treated as success (`0`).
pub exit_code: Option<i32>,
/// Minimizer configuration; when omitted the call is a no-op (`null`).
pub minimizer: Option<MinimizerOptions>,
}
/// Run the shell-output minimizer over an already-captured command result,
/// without spawning a shell.
///
/// This is the one-shot counterpart to the minimization that
/// [`execute_shell`] performs inline: callers that captured a command's output
/// elsewhere can pass it here to obtain the same telemetry.
///
/// Returns [`MinimizerResult`] **only** when the minimizer actually rewrote the
/// output (`changed == true`) and retained the original buffer, mirroring the
/// persistent-shell path. Returns `null` for every no-op case: when
/// `minimizer` is omitted, when the config is disabled, or when the filter
/// passes the output through unchanged. A missing `exit_code` is treated as
/// success (`0`).
///
/// Async (returns a Promise): minimization can scan multi-megabyte captured
/// output, so the work runs on a blocking pool to avoid stalling the JS event
/// loop.
#[napi(ts_return_type = "Promise<MinimizerResult | null>")]
pub fn apply_shell_minimizer(
env: &Env,
options: ShellMinimizerApplyOptions,
) -> Result<PromiseRaw<'_, Option<MinimizerResult>>> {
// Returns a Promise rather than a sync value: minimization can run over a
// multi-megabyte capture buffer, and a sync `#[napi]` fn would do that CPU
// work on the JS main thread and stall the event loop. Run the whole pass on
// a blocking pool, mirroring `execute_shell`.
task::future(env, "shell.minimize", async move {
napi::tokio::task::spawn_blocking(move || run_shell_minimizer(options))
.await
.map_err(|err| Error::from_reason(err.to_string()))
})
}
/// Pure, blocking core of [`apply_shell_minimizer`], factored out so it can run
/// inside `spawn_blocking` and be unit-tested without an N-API `Env`.
///
/// Mirrors the persistent-shell path (`pi_shell::shell`): surface telemetry
/// only when the minimizer actually rewrote the output and kept the original
/// buffer. The disabled / passthrough cases report `changed: false` with no
/// `original_text`, and yield `None`.
fn run_shell_minimizer(options: ShellMinimizerApplyOptions) -> Option<MinimizerResult> {
let minimizer = options.minimizer?;
let minimizer_options: minimizer::MinimizerOptions = minimizer.into();
let config = minimizer::MinimizerConfig::from_options(&minimizer_options);
let output = minimizer::apply(
&options.command,
&options.captured,
options.exit_code.unwrap_or(0),
&config,
);
if output.changed
&& let Some(original_text) = output.original_text
{
let output_bytes = u32::try_from(output.text.len()).unwrap_or(u32::MAX);
return Some(MinimizerResult {
filter: output.filter.to_string(),
text: output.text,
original_text,
input_bytes: u32::try_from(output.input_bytes).unwrap_or(u32::MAX),
output_bytes,
});
}
None
}
#[cfg(test)]
mod tests {
use std::time::Duration;
@@ -435,48 +356,6 @@ mod tests {
use super::CoreShell;
#[test]
fn apply_shell_minimizer_surfaces_rewrite_with_original() {
let captured = "diff --git a/file.rs b/file.rs\n@@\n-old\n+new\n";
let result = super::run_shell_minimizer(super::ShellMinimizerApplyOptions {
command: "git diff".to_string(),
captured: captured.to_string(),
exit_code: Some(0),
minimizer: Some(super::MinimizerOptions { enabled: Some(true), ..Default::default() }),
})
.expect("an enabled, supported command should surface a rewrite");
assert_eq!(result.filter, "git");
// A genuine rewrite carries the untouched capture in `original_text`
// and a strictly different minimized `text`.
assert_eq!(result.original_text, captured);
assert_ne!(result.text, result.original_text);
assert_eq!(result.input_bytes as usize, captured.len());
}
#[test]
fn apply_shell_minimizer_returns_none_when_disabled() {
// `enabled: false` keeps the engine in passthrough — no telemetry.
assert!(
super::run_shell_minimizer(super::ShellMinimizerApplyOptions {
command: "git diff".to_string(),
captured: "diff --git a/file.rs b/file.rs\n@@\n-old\n+new\n".to_string(),
exit_code: Some(0),
minimizer: Some(super::MinimizerOptions { enabled: Some(false), ..Default::default() }),
})
.is_none()
);
// A missing minimizer handle is also a no-op.
assert!(
super::run_shell_minimizer(super::ShellMinimizerApplyOptions {
command: "git diff".to_string(),
captured: "diff --git a/file.rs b/file.rs\n".to_string(),
exit_code: Some(0),
minimizer: None,
})
.is_none()
);
}
mod child_session_action_tests {
use pi_shell::{ChildSessionAction, child_session_action};
crates/pi-shell/src/minimizer/filters/listing.rs
+16
View File
@@ -879,6 +879,7 @@ fn aggressive_strip_bodies(input: &str, path: &str) -> Option<String> {
.and_then(|e| e.to_str())
.unwrap_or("");
match ext {
"rs" if contains_rust_raw_string_literal(input) => None,
"rs" | "ts" | "tsx" | "js" | "jsx" | "go" => Some(strip_brace_bodies(input)),
"py" => Some(strip_python_bodies(input)),
_ => None,
@@ -926,6 +927,10 @@ fn strip_brace_bodies(input: &str) -> String {
out
}
fn contains_rust_raw_string_literal(input: &str) -> bool {
input.contains("r#") || input.contains("r\"") || input.contains("br#") || input.contains("br\"")
}
fn brace_delta(line: &str) -> i32 {
let mut delta: i32 = 0;
let mut in_str: Option<char> = None;
@@ -1510,6 +1515,17 @@ mod tests {
assert!(!out.text.contains("y * 2"));
}
#[test]
fn aggressive_rust_outline_bails_on_raw_strings() {
let cfg = aggressive_cfg();
let ctx = ctx_command("cat", "cat src/foo.rs", &cfg);
let body =
"pub fn shader() -> &'static str {\n r#\"fn main() { println!(\\\"hi\\\"); }\"#\n}\n";
let out = filter(&ctx, body, 0);
assert!(!out.changed, "raw-string Rust source should fall back to default outline");
assert_eq!(out.text, body);
}
#[test]
fn brace_in_line_comment_not_counted() {
let cfg = aggressive_cfg();
crates/pi-shell/src/minimizer/filters/rust_tools.rs
+1 -1
View File
@@ -70,7 +70,7 @@ fn condense_rustfmt(input: &str, exit_code: i32) -> String {
files.iter().filter(|f| seen.insert(**f)).collect()
};
let total = unique.len();
let _ = writeln!(out, "{total} files reformatted ({total} with diffs):");
let _ = writeln!(out, "{total} files reformatted:");
for file in unique.iter().take(3) {
out.push_str(" ");
out.push_str(file);
packages/coding-agent/CHANGELOG.md
+8
View File
@@ -133,6 +133,14 @@
- Fixed Windows stdio MCP servers launched through PATH shims such as `codegraph.cmd` so bare commands like `codegraph` resolve via `PATHEXT` before spawn ([#2174](https://github.com/can1357/oh-my-pi/issues/2174)).
- Fixed compiled-binary extensions failing to load `@oh-my-pi/pi-*` packages when `bun --compile` quietly dropped one of the extra entrypoints (observed on macOS arm64 release builds): the legacy-pi compat shim's package-root override branch returned the bunfs path without checking the target was present, so the rewrite emitted a `file://` URL to a missing module and the #1216 fallback (scoped to the throwing `getResolvedSpecifier` path) never ran. Override targets are now validated against the on-disk filesystem at module init, missing entries are dropped, and resolution falls through to canonical lookup so Bun resolves the import from the extension's own `node_modules` ([#2168](https://github.com/can1357/oh-my-pi/issues/2168)).
### Added
- Added opt-in `shellMinimizer.sourceOutlineLevel` and `shellMinimizer.legacyFilters` settings so shell minimization can tune source outlining and selectively fall back to conservative legacy routing.
### Changed
- Bash execution now preserves minimized shell output inline while saving the untouched capture as an `artifact://…` footer when shell minimization rewrites a command's output.
## [15.10.8] - 2026-06-09
### Added
packages/coding-agent/src/cli/shell-cli.ts
+1 -1
View File
@@ -52,7 +52,7 @@ export async function runShellCommand(cmd: ShellCommandArgs): Promise<void> {
const settings = await Settings.init({ cwd });
const { shell, env: shellEnv } = settings.getShellConfig();
const snapshotPath = cmd.noSnapshot || !shell.includes("bash") ? null : await getOrCreateSnapshot(shell, shellEnv);
const minimizer = await buildMinimizerOptions(settings.getGroup("shellMinimizer"));
const minimizer = buildMinimizerOptions(settings.getGroup("shellMinimizer"));
const shellSession = new Shell({ sessionEnv: shellEnv, snapshotPath: snapshotPath ?? undefined, minimizer });
let active = false;
packages/coding-agent/src/config/settings-schema.ts
+4 -3
View File
@@ -2068,8 +2068,9 @@ export const SETTINGS_SCHEMA = {
default: 4 * 1024 * 1024,
},
"shellMinimizer.sourceOutlineLevel": {
type: "string",
default: undefined,
type: "enum",
values: ["default", "aggressive"] as const,
default: "default",
ui: {
tab: "editing",
label: "Shell Minimizer Source Outline",
@@ -3479,7 +3480,7 @@ export interface ShellMinimizerSettings {
only: string[];
except: string[];
maxCaptureBytes: number;
sourceOutlineLevel: string | undefined;
sourceOutlineLevel: "default" | "aggressive";
legacyFilters: boolean | undefined;
}
packages/coding-agent/src/exec/bash-executor.ts
+1 -1
View File
@@ -95,7 +95,7 @@ export function buildMinimizerOptions(group: ShellMinimizerSettings): MinimizerO
only: group.only.length > 0 ? group.only : undefined,
except: group.except.length > 0 ? group.except : undefined,
maxCaptureBytes: group.maxCaptureBytes,
sourceOutlineLevel: group.sourceOutlineLevel || undefined,
sourceOutlineLevel: group.sourceOutlineLevel === "default" ? undefined : group.sourceOutlineLevel,
legacyFilters: group.legacyFilters,
};
}
packages/coding-agent/test/bash-executor.test.ts
+35 -2
View File
@@ -2,8 +2,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { executeBash } from "@oh-my-pi/pi-coding-agent/exec/bash-executor";
import { resetSettingsForTest, Settings, type ShellMinimizerSettings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { buildMinimizerOptions, executeBash } from "@oh-my-pi/pi-coding-agent/exec/bash-executor";
import { DEFAULT_MAX_BYTES } from "@oh-my-pi/pi-coding-agent/session/streaming-output";
import * as shellSnapshot from "@oh-my-pi/pi-coding-agent/utils/shell-snapshot";
import type { Shell } from "@oh-my-pi/pi-natives";
@@ -52,6 +52,39 @@ describe("executeBash", () => {
}
});
it("omits minimizer options when the feature is disabled", () => {
const group: ShellMinimizerSettings = {
enabled: false,
settingsPath: undefined,
only: [],
except: [],
maxCaptureBytes: 4096,
sourceOutlineLevel: "default",
legacyFilters: undefined,
};
expect(buildMinimizerOptions(group)).toBeUndefined();
});
it("forwards source outline and legacy filter settings to native minimizer options", () => {
const group: ShellMinimizerSettings = {
enabled: true,
settingsPath: "minimizer.toml",
only: ["git"],
except: ["docker"],
maxCaptureBytes: 1234,
sourceOutlineLevel: "aggressive",
legacyFilters: true,
};
expect(buildMinimizerOptions(group)).toEqual({
enabled: true,
settingsPath: "minimizer.toml",
only: ["git"],
except: ["docker"],
maxCaptureBytes: 1234,
sourceOutlineLevel: "aggressive",
legacyFilters: true,
});
});
it("returns non-zero exit codes without cancellation", async () => {
const result = await executeBash("exit 7", { cwd: tempDir, timeout: 5000 });
expect(result.exitCode).toBe(7);
packages/natives/CHANGELOG.md
+4
View File
@@ -18,6 +18,10 @@
- Fixed cross-line grep being a silent no-op on real files: `multiline` set the `(?m)` flag on the regex matcher but never enabled `multi_line` on the `Searcher`, which stayed line-oriented, so any pattern spanning a `\n` returned zero matches with no error.
### Added
- Added deterministic shell-output minimization to the native shell pipeline, including opt-in per-command rewrite telemetry surfaced through `executeShell().minimized` for callers that want compact inline output plus a separately persisted original capture.
## [15.10.5] - 2026-06-08
### Added
packages/natives/native/index.d.ts
Vendored
-36
View File
@@ -147,27 +147,6 @@ export declare function __piNativesV15_10_11(): void
*/
export declare function applyBashFixups(command: string): BashFixupResult
/**
* Run the shell-output minimizer over an already-captured command result,
* without spawning a shell.
*
* This is the one-shot counterpart to the minimization that
* [`execute_shell`] performs inline: callers that captured a command's output
* elsewhere can pass it here to obtain the same telemetry.
*
* Returns [`MinimizerResult`] **only** when the minimizer actually rewrote the
* output (`changed == true`) and retained the original buffer, mirroring the
* persistent-shell path. Returns `null` for every no-op case: when
* `minimizer` is omitted, when the config is disabled, or when the filter
* passes the output through unchanged. A missing `exit_code` is treated as
* success (`0`).
*
* Async (returns a Promise): minimization can scan multi-megabyte captured
* output, so the work runs on a blocking pool to avoid stalling the JS event
* loop.
*/
export declare function applyShellMinimizer(options: ShellMinimizerApplyOptions): Promise<MinimizerResult | null>
/**
* Apply ast-grep rewrite rules to matching files; honors `dryRun` and returns
* a promise.
@@ -1374,21 +1353,6 @@ export interface ShellExecuteOptions {
signal?: unknown
}
/**
* Inputs for [`apply_shell_minimizer`]: a captured command's text plus the
* minimizer configuration to run against it.
*/
export interface ShellMinimizerApplyOptions {
/** The command line that produced `captured` (used to select a filter). */
command: string
/** The full captured stdout/stderr to minimize. */
captured: string
/** The command's exit status; omitted is treated as success (`0`). */
exitCode?: number
/** Minimizer configuration; when omitted the call is a no-op (`null`). */
minimizer?: MinimizerOptions
}
/** Options for configuring a persistent shell session. */
export interface ShellOptions {
/** Environment variables to apply once per session. */
packages/natives/native/index.js
-1
View File
@@ -25,7 +25,6 @@ export const Shell = nativeBindings.Shell;
// functions
export const __piNativesV15_10_11 = nativeBindings.__piNativesV15_10_11;
export const applyBashFixups = nativeBindings.applyBashFixups;
export const applyShellMinimizer = nativeBindings.applyShellMinimizer;
export const astEdit = nativeBindings.astEdit;
export const astGrep = nativeBindings.astGrep;
export const astMatch = nativeBindings.astMatch;