From 6ac5e4e1ceb0e7cf766efa038c868c9164313525 Mon Sep 17 00:00:00 2001 From: zolszabo Date: Thu, 14 May 2026 16:27:37 +0200 Subject: [PATCH 01/22] fix: exclude OpenCode providers from synthetic reasoning_content injection for Kimi models OpenCode-Go and OpenCode-Zen handle reasoning content internally and reject client-supplied reasoning_content in message history. When retry fallback forwards conversation history to opencode-go/kimi-k2.6, the compat layer was injecting synthetic reasoning_content: '.' on assistant tool-call turns, causing HTTP 400: 'Extra inputs are not permitted'. Gate isKimiModel in requiresReasoningContentForToolCalls on !isOpenCodeProvider so the injection path is skipped for OpenCode providers while preserving existing behavior for native Kimi API, DeepSeek, and OpenRouter. --- packages/ai/src/providers/openai-completions-compat.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/ai/src/providers/openai-completions-compat.ts b/packages/ai/src/providers/openai-completions-compat.ts index 8b4b706d6..efb19600c 100644 --- a/packages/ai/src/providers/openai-completions-compat.ts +++ b/packages/ai/src/providers/openai-completions-compat.ts @@ -90,6 +90,7 @@ export function detectOpenAICompat(model: Model<"openai-completions">, resolvedB provider === "opencode-zen" || provider === "opencode-go" || baseUrl.includes("opencode.ai"); + const isOpenCodeProvider = provider === "opencode-go" || provider === "opencode-zen"; const useMaxTokens = provider === "mistral" || @@ -182,13 +183,15 @@ export function detectOpenAICompat(model: Model<"openai-completions">, resolvedB : "openai", reasoningContentField: "reasoning_content", // Backends that 400 follow-up requests when prior assistant tool-call turns lack `reasoning_content`: - // - Kimi: documented invariant on its native API and via OpenCode-Go. + // - Kimi: documented invariant on its native API. // - Any reasoning-capable model reached through OpenRouter: DeepSeek V4 Pro and similar enforce // this server-side whenever the request is in thinking mode. We can't translate Anthropic's // redacted/encrypted reasoning into DeepSeek's plaintext form, so cross-provider continuations // rely on a placeholder — see `convertMessages` for the placeholder injection. + // - OpenCode-Go and OpenCode-Zen handle reasoning content internally and reject + // `reasoning_content` in client-sent messages — exclude them even for Kimi models. requiresReasoningContentForToolCalls: - isKimiModel || + (isKimiModel && !isOpenCodeProvider) || (isDeepseekFamily && Boolean(model.reasoning)) || ((provider === "openrouter" || baseUrl.includes("openrouter.ai")) && Boolean(model.reasoning)), // DeepSeek V4 rejects synthetic reasoning_content placeholders (".") on tool-call turns. From bea95b056d9a00067ace5987bdc2eb8c807e0d77 Mon Sep 17 00:00:00 2001 From: ephraimduncan Date: Thu, 14 May 2026 15:19:18 +0000 Subject: [PATCH 02/22] fix(ai): recover stalled lazy provider streams --- packages/ai/CHANGELOG.md | 5 ++ .../ai/src/providers/register-builtins.ts | 43 ++++++++-- packages/ai/src/utils/idle-iterator.ts | 2 +- packages/ai/test/register-builtins.test.ts | 80 ++++++++++++++++++ .../test/agent-session-retry-fallback.test.ts | 82 +++++++++++++++++++ 5 files changed, 203 insertions(+), 9 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index e8fbc0668..6c53e986e 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -22,6 +22,11 @@ - Fixed OAuth credentials being silently disabled when two omp processes (or any two `AuthStorage` instances sharing a `agent.db`) race on token refresh. Anthropic rotates refresh tokens on every use, so the loser's `invalid_grant` response previously soft-deleted the row that the winner just rotated, forcing the user to `/login` again. `#tryOAuthCredential` now re-reads the row from disk before declaring a definitive failure: if the persisted `refresh` differs from the snapshot it tried, the peer-rotated credential is reloaded and the request retries against the fresh token instead of disabling the live row. - Closed a remaining race window in OAuth refresh-failure handling: between re-reading the credential row to check for peer rotation and the subsequent soft-delete, another process could still complete a refresh and rotate the row, leaving us to disable the freshly-rotated credential by `id`. The disable now runs as a single CAS update conditioned on the row's `data` still matching the snapshot we tried to refresh, and on `disabled_cause IS NULL`. If the CAS reports 0 rows changed (peer rotation, or row already disabled by a concurrent failure on the same snapshot), we reload from disk and retry instead of mutating the wrong row or emitting a spurious `credential_disabled` event. +### Changed +- Lowered the default steady-state stream idle timeout from 120s to 30s while preserving the existing environment overrides. + +### Fixed +- Lazy built-in provider streams now enforce the shared idle watchdog and abort stalled provider requests, so session auto-retry can continue after transient network drops instead of remaining stuck. Caller aborts still terminate as aborted. ## [14.9.3] - 2026-05-10 diff --git a/packages/ai/src/providers/register-builtins.ts b/packages/ai/src/providers/register-builtins.ts index bf9d85103..9b2895cdb 100644 --- a/packages/ai/src/providers/register-builtins.ts +++ b/packages/ai/src/providers/register-builtins.ts @@ -19,7 +19,9 @@ import type { Model, OptionsForApi, } from "../types"; +import { type AbortSourceTracker, createAbortSourceTracker } from "../utils/abort"; import { AssistantMessageEventStream as EventStreamImpl } from "../utils/event-stream"; +import { getStreamFirstEventTimeoutMs, getStreamIdleTimeoutMs, iterateWithIdleTimeout } from "../utils/idle-iterator"; import type { BedrockOptions } from "./amazon-bedrock"; import type { AnthropicOptions } from "./anthropic"; import type { AzureOpenAIResponsesOptions } from "./azure-openai-responses"; @@ -155,6 +157,9 @@ export function setBedrockProviderModule(module: BedrockProviderModule): void { // Stream forwarding / error helpers // --------------------------------------------------------------------------- +const LAZY_STREAM_IDLE_TIMEOUT_ERROR = "Provider stream stalled while waiting for the next event"; +const LAZY_STREAM_FIRST_EVENT_TIMEOUT_ERROR = "Provider stream timed out while waiting for the first event"; + function hasFinalResult( source: AsyncIterable, ): source is AsyncIterable & { result(): Promise } { @@ -165,10 +170,23 @@ function forwardStream( target: EventStreamImpl, source: AsyncIterable, model: Model, + options: OptionsForApi, + abortTracker: AbortSourceTracker, ): void { (async () => { try { - for await (const event of source) { + const idleTimeoutMs = options.streamIdleTimeoutMs ?? getStreamIdleTimeoutMs(); + const watchedSource = iterateWithIdleTimeout(source, { + idleTimeoutMs, + firstItemTimeoutMs: options.streamFirstEventTimeoutMs ?? getStreamFirstEventTimeoutMs(idleTimeoutMs), + errorMessage: LAZY_STREAM_IDLE_TIMEOUT_ERROR, + firstItemErrorMessage: LAZY_STREAM_FIRST_EVENT_TIMEOUT_ERROR, + onIdle: () => abortTracker.abortLocally(new Error(LAZY_STREAM_IDLE_TIMEOUT_ERROR)), + onFirstItemTimeout: () => abortTracker.abortLocally(new Error(LAZY_STREAM_FIRST_EVENT_TIMEOUT_ERROR)), + abortSignal: options.signal, + }); + + for await (const event of watchedSource) { target.push(event); } if (hasFinalResult(source)) { @@ -177,14 +195,19 @@ function forwardStream( target.end(); } } catch (error) { - const message = createLazyLoadErrorMessage(model, error); - target.push({ type: "error", reason: "error", error: message }); + const stopReason = abortTracker.wasCallerAbort() ? "aborted" : "error"; + const message = createLazyLoadErrorMessage(model, error, stopReason); + target.push({ type: "error", reason: stopReason, error: message }); target.end(message); } })(); } -function createLazyLoadErrorMessage(model: Model, error: unknown): AssistantMessage { +function createLazyLoadErrorMessage( + model: Model, + error: unknown, + stopReason: Extract = "error", +): AssistantMessage { return { role: "assistant", content: [], @@ -199,8 +222,9 @@ function createLazyLoadErrorMessage(model: Model, error: totalTokens: 0, cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, }, - stopReason: "error", - errorMessage: error instanceof Error ? error.message : String(error), + stopReason, + errorMessage: + stopReason === "aborted" ? "Request was aborted" : error instanceof Error ? error.message : String(error), timestamp: Date.now(), }; } @@ -214,11 +238,14 @@ function createLazyStream( ): (model: Model, context: Context, options: OptionsForApi) => EventStreamImpl { return (model, context, options) => { const outer = new EventStreamImpl(); + const streamOptions = (options ?? {}) as OptionsForApi; loadModule() .then(module => { - const inner = module.stream(model, context, options); - forwardStream(outer, inner, model); + const abortTracker = createAbortSourceTracker(streamOptions.signal); + const providerOptions = { ...streamOptions, signal: abortTracker.requestSignal } as OptionsForApi; + const inner = module.stream(model, context, providerOptions); + forwardStream(outer, inner, model, streamOptions, abortTracker); }) .catch(error => { const message = createLazyLoadErrorMessage(model, error); diff --git a/packages/ai/src/utils/idle-iterator.ts b/packages/ai/src/utils/idle-iterator.ts index 44bd532da..61d06063e 100644 --- a/packages/ai/src/utils/idle-iterator.ts +++ b/packages/ai/src/utils/idle-iterator.ts @@ -1,6 +1,6 @@ import { $env } from "@oh-my-pi/pi-utils"; -const DEFAULT_STREAM_IDLE_TIMEOUT_MS = 120_000; +const DEFAULT_STREAM_IDLE_TIMEOUT_MS = 30_000; const DEFAULT_STREAM_FIRST_EVENT_TIMEOUT_MS = 100_000; function normalizeIdleTimeoutMs(value: string | undefined, fallback: number): number | undefined { diff --git a/packages/ai/test/register-builtins.test.ts b/packages/ai/test/register-builtins.test.ts index f3911edcb..15bdf6a21 100644 --- a/packages/ai/test/register-builtins.test.ts +++ b/packages/ai/test/register-builtins.test.ts @@ -92,4 +92,84 @@ describe("register-builtins lazy streams", () => { expect(result.stopReason).toBe("error"); expect(result.errorMessage).toContain("bedrock exploded"); }); + + it("turns idle lazy provider streams into retryable terminal errors", async () => { + const partialMessage = createAssistantMessage("stop"); + let providerSignal: AbortSignal | undefined; + const source = { + async *[Symbol.asyncIterator]() { + yield { type: "start", partial: partialMessage } as const; + const { promise, reject } = Promise.withResolvers(); + if (providerSignal?.aborted) { + reject(new Error("Request was aborted")); + } + providerSignal?.addEventListener("abort", () => reject(new Error("Request was aborted")), { + once: true, + }); + await promise; + }, + } as unknown as AssistantMessageEventStream; + + setBedrockProviderModule({ + streamBedrock: (_model, _context, options) => { + providerSignal = options.signal; + return source; + }, + }); + + const stream = streamBedrock(createModel(), baseContext, { streamIdleTimeoutMs: 10 }); + const result = await Promise.race([stream.result(), Bun.sleep(500).then(() => "timeout" as const)]); + + expect(result).not.toBe("timeout"); + if (result === "timeout") { + throw new Error("Timed out waiting for forwarded stream stall result"); + } + expect(providerSignal?.aborted).toBe(true); + expect(result.stopReason).toBe("error"); + expect(result.errorMessage).toBe("Provider stream stalled while waiting for the next event"); + }); + + it("preserves caller aborts while forwarding lazy provider streams", async () => { + const abortController = new AbortController(); + const partialMessage = createAssistantMessage("stop"); + let providerSignal: AbortSignal | undefined; + const source = { + async *[Symbol.asyncIterator]() { + yield { type: "start", partial: partialMessage } as const; + const { promise, reject } = Promise.withResolvers(); + if (providerSignal?.aborted) { + reject(new Error("Request was aborted")); + } + providerSignal?.addEventListener("abort", () => reject(new Error("Request was aborted")), { + once: true, + }); + await promise; + }, + } as unknown as AssistantMessageEventStream; + + setBedrockProviderModule({ + streamBedrock: (_model, _context, options) => { + providerSignal = options.signal; + return source; + }, + }); + + const stream = streamBedrock(createModel(), baseContext, { + signal: abortController.signal, + streamIdleTimeoutMs: 500, + }); + const iterator = stream[Symbol.asyncIterator](); + const firstEvent = await iterator.next(); + expect(firstEvent.value?.type).toBe("start"); + + abortController.abort(); + const result = await Promise.race([stream.result(), Bun.sleep(500).then(() => "timeout" as const)]); + + expect(result).not.toBe("timeout"); + if (result === "timeout") { + throw new Error("Timed out waiting for forwarded caller abort result"); + } + expect(result.stopReason).toBe("aborted"); + expect(result.errorMessage).toBe("Request was aborted"); + }); }); diff --git a/packages/coding-agent/test/agent-session-retry-fallback.test.ts b/packages/coding-agent/test/agent-session-retry-fallback.test.ts index ab29a1187..a94e7886a 100644 --- a/packages/coding-agent/test/agent-session-retry-fallback.test.ts +++ b/packages/coding-agent/test/agent-session-retry-fallback.test.ts @@ -337,6 +337,88 @@ describe("AgentSession retry fallback", () => { expect(lastAssistant.content).toContainEqual({ type: "text", text: "Recovered after OpenAI timeout" }); }); + it("auto-retries stream stall errors", async () => { + const model = getBundledModel("openai", "gpt-4o-mini"); + if (!model) { + throw new Error("Expected bundled OpenAI test model to exist"); + } + + const stallMessage = "Provider stream stalled while waiting for the next event"; + const requestedModels: string[] = []; + let attemptCount = 0; + + const agent = new Agent({ + getApiKey: provider => `${provider}-test-key`, + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + }, + streamFn: requestedModel => { + requestedModels.push(`${requestedModel.provider}/${requestedModel.id}`); + const stream = new MockAssistantStream(); + queueMicrotask(() => { + attemptCount += 1; + if (attemptCount === 1) { + const message = createAssistantMessage(requestedModel, { + stopReason: "error", + errorMessage: stallMessage, + }); + stream.push({ type: "start", partial: message }); + stream.push({ type: "error", reason: "error", error: message }); + return; + } + if (attemptCount === 2) { + const message = createAssistantMessage(requestedModel, { + text: "Recovered after stream stall", + stopReason: "stop", + }); + stream.push({ + type: "start", + partial: createAssistantMessage(requestedModel, { text: "", stopReason: "stop" }), + }); + stream.push({ type: "done", reason: "stop", message }); + return; + } + throw new Error(`Unexpected retry attempt in stream stall test: ${attemptCount}`); + }); + return stream; + }, + }); + + const settings = Settings.isolated({ + "compaction.enabled": false, + "retry.baseDelayMs": 5, + "retry.maxRetries": 1, + }); + settings.setModelRole("default", `${model.provider}/${model.id}`); + + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings, + modelRegistry, + }); + const { retryStartEvents, retryEndEvents } = trackRetryEvents(session); + + await session.prompt("Retry stream stall"); + await session.waitForIdle(); + + expect(requestedModels).toEqual([`${model.provider}/${model.id}`, `${model.provider}/${model.id}`]); + expect(retryStartEvents).toHaveLength(1); + expect(retryStartEvents[0]).toMatchObject({ + attempt: 1, + maxAttempts: 1, + errorMessage: stallMessage, + }); + expect(retryEndEvents).toHaveLength(1); + expect(retryEndEvents[0]).toMatchObject({ success: true, attempt: 1 }); + const lastAssistant = getLastAssistantMessage(session); + expect(lastAssistant.stopReason).toBe("stop"); + expect(lastAssistant.content).toContainEqual({ type: "text", text: "Recovered after stream stall" }); + }); + it("auto-retries OpenAI processing-request transient errors", async () => { const model = getBundledModel("openai", "gpt-4o-mini"); if (!model) { From f8ea641cdb8b96497d2c0c093368cc259c3da814 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 01:09:26 +0200 Subject: [PATCH 03/22] fix(stats): ignored messages without IDs when parsing session entries - Added message-ID checks in both assistant and user message parsers so entries without valid ids are ignored. - This filtering prevents legacy, unlinked message records from being treated as parseable assistant or user messages. --- packages/stats/src/parser.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/stats/src/parser.ts b/packages/stats/src/parser.ts index d99d43305..42dd1eab3 100644 --- a/packages/stats/src/parser.ts +++ b/packages/stats/src/parser.ts @@ -31,6 +31,10 @@ function extractFolderFromPath(sessionPath: string): string { function isAssistantMessage(entry: SessionEntry): entry is SessionMessageEntry { if (entry.type !== "message") return false; const msgEntry = entry as SessionMessageEntry; + // Legacy sessions (pre-id tracking) recorded message entries without an `id`. + // They're not linkable and would violate the messages.entry_id NOT NULL + // constraint, so skip them at the parser boundary. + if (typeof msgEntry.id !== "string" || msgEntry.id.length === 0) return false; return msgEntry.message?.role === "assistant"; } @@ -40,6 +44,7 @@ function isAssistantMessage(entry: SessionEntry): entry is SessionMessageEntry { function isUserMessage(entry: SessionEntry): entry is SessionMessageEntry { if (entry.type !== "message") return false; const msgEntry = entry as SessionMessageEntry; + if (typeof msgEntry.id !== "string" || msgEntry.id.length === 0) return false; return msgEntry.message?.role === "user"; } From 92c42f44f71e05081570491d22a172d72339a3fc Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 01:26:26 +0200 Subject: [PATCH 04/22] fix(coding-agent/tools): resolved bash fixup parsing for head/tail chunks - Added top-level parsing and segment splitting to apply bash fixups only on safe command chunks. - Replaced `stripTrailingHeadTail` usage with `applyBashFixups` and array-based notice formatting. - Fixed terminal `| head`/`| tail` and redundant `2>&1` stripping while preserving command semantics. - Updated fixup tests for cross-command cases and removed superseded head-tail-only test coverage. --- crates/pi-natives/src/shell.rs | 30 +- crates/pi-shell/src/fixup.rs | 448 ++++++++++++++++++ crates/pi-shell/src/lib.rs | 1 + packages/coding-agent/CHANGELOG.md | 6 + .../src/tools/bash-command-fixup.ts | 89 ++-- packages/coding-agent/src/tools/bash.ts | 24 +- .../test/tools/bash-command-fixup.test.ts | 144 ++++++ .../test/tools/bash-head-tail-strip.test.ts | 100 ---- .../test/tools/bash-interceptor.test.ts | 2 +- packages/natives/CHANGELOG.md | 1 + packages/natives/native/index.d.ts | 20 + packages/natives/native/index.js | 1 + 12 files changed, 692 insertions(+), 174 deletions(-) create mode 100644 crates/pi-shell/src/fixup.rs create mode 100644 packages/coding-agent/test/tools/bash-command-fixup.test.ts delete mode 100644 packages/coding-agent/test/tools/bash-head-tail-strip.test.ts diff --git a/crates/pi-natives/src/shell.rs b/crates/pi-natives/src/shell.rs index 5489764f9..6755cd9de 100644 --- a/crates/pi-natives/src/shell.rs +++ b/crates/pi-natives/src/shell.rs @@ -13,7 +13,9 @@ use pi_shell::{ MinimizerResult as CoreMinimizerResult, Shell as CoreShell, ShellExecuteOptions as CoreShellExecuteOptions, ShellOptions as CoreShellOptions, ShellRunOptions as CoreShellRunOptions, ShellRunResult as CoreShellRunResult, - execute_shell as core_execute_shell, minimizer, + execute_shell as core_execute_shell, + fixup::{BashFixupResult as CoreBashFixupResult, apply_bash_fixups as core_apply_bash_fixups}, + minimizer, }; use crate::task; @@ -285,6 +287,32 @@ fn bridge_chunks( (Some(tx), Some(handle)) } +/// Result of [`apply_bash_fixups`]: a possibly-rewritten command plus the +/// substrings that were removed (in source order). +#[napi(object)] +pub struct BashFixupResult { + /// Possibly-rewritten command. Equal to the input when no fixup fired. + pub command: String, + /// Substrings removed, in source order — suitable for a user-facing notice. + pub stripped: Vec, +} + +impl From for BashFixupResult { + fn from(value: CoreBashFixupResult) -> Self { + Self { command: value.command, stripped: value.stripped } + } +} + +/// Apply conservative pre-execution rewrites to a bash command. +/// +/// Strips trailing `| head|tail [safe-args]` and redundant trailing `2>&1` +/// from each top-level pipeline. The full rules and bail conditions live in +/// `pi_shell::fixup`. Synchronous and cheap (one parse pass over the input). +#[napi] +pub fn apply_bash_fixups(command: String) -> BashFixupResult { + core_apply_bash_fixups(&command).into() +} + #[cfg(test)] mod tests { use std::time::Duration; diff --git a/crates/pi-shell/src/fixup.rs b/crates/pi-shell/src/fixup.rs new file mode 100644 index 000000000..ace9f8cf0 --- /dev/null +++ b/crates/pi-shell/src/fixup.rs @@ -0,0 +1,448 @@ +//! Conservative pre-execution rewrites for bash commands. +//! +//! Two fixups are applied, each anchored to the end of a top-level pipeline +//! (segments split on `;`, `&&`, `||`, and background `&`): +//! +//! 1. Trailing `| head [args]` / `| tail [args]` (and the `|&` variant) — +//! these pipes exist purely to limit output length. The harness already +//! truncates bash output and exposes the full result via an artifact, so +//! the pipe just hides content the agent wanted. +//! +//! 2. A redundant trailing `2>&1` on a segment that has no remaining pipe +//! or other redirect. The harness already merges stderr into stdout, so +//! the duplication is purely cosmetic — and often a leftover after fixup +//! (1) drops a downstream pipe. +//! +//! The implementation is AST-driven: `brush-parser` handles tokenization, +//! quoting, heredocs, command substitution, and nested compound commands. We +//! never re-implement those by hand. Source spans on `Pipeline`/`Command` +//! nodes give us byte-exact edit ranges; `IoRedirect` currently lacks a span, +//! so the `2>&1` strip uses a bounded textual scan inside the enclosing +//! simple command's source span. +//! +//! On any parse failure, multi-line input, or absence of an applicable +//! pattern, the function returns the input verbatim with `stripped` empty. + +use std::{io::BufReader, sync::LazyLock}; + +use brush_parser::{Parser, ParserOptions, SourceInfo, ast::*}; +use regex::Regex; + +/// Result of [`apply_bash_fixups`]. +#[derive(Debug, Clone, Default)] +pub struct BashFixupResult { + /// Possibly-rewritten command. Equal to the input when no fixup fired. + pub command: String, + /// Substrings removed, in source order. Suitable for a user-facing notice. + pub stripped: Vec, +} + +/// Apply the bash fixups to `cmd`. See module docs for full rules. +pub fn apply_bash_fixups(cmd: &str) -> BashFixupResult { + // Multi-line input is out of scope: heredoc/loop bodies can't be safely + // rewritten and the agent rarely passes them as bash tool input. Bailing + // early also keeps the per-call cost bounded. + if cmd.contains('\n') || cmd.contains('\r') { + return BashFixupResult { command: cmd.to_owned(), stripped: vec![] }; + } + + let options = ParserOptions::default(); + let source_info = SourceInfo::default(); + let mut reader = BufReader::new(cmd.as_bytes()); + let mut parser = Parser::new(&mut reader, &options, &source_info); + let Ok(program) = parser.parse_program() else { + return BashFixupResult { command: cmd.to_owned(), stripped: vec![] }; + }; + + // `ranges` drives output construction; `stripped` is reported to the + // caller. We keep them separate so reporting can stay in fixup order + // (head/tail before `2>&1`) while edits sort by source position. + let mut ranges: Vec<(usize, usize)> = Vec::new(); + let mut stripped: Vec = Vec::new(); + + // Walk only the top-level pipelines. Recursing into compound bodies (`if`, + // loops, subshells) would risk changing semantics: e.g. stripping `head` + // from `if cmd | head -5; then …; fi` swaps a header-check for a full + // stream-check. + for complete in &program.complete_commands { + for CompoundListItem(and_or, _sep) in &complete.0 { + walk_andor(and_or, cmd, &mut ranges, &mut stripped); + } + } + + if ranges.is_empty() { + return BashFixupResult { command: cmd.to_owned(), stripped: vec![] }; + } + + ranges.sort_by_key(|(s, _)| *s); + let mut out = String::with_capacity(cmd.len()); + let mut cursor = 0; + for (s, e) in ranges { + // Defensive: ranges should be disjoint by construction. + if s < cursor { + continue; + } + out.push_str(&cmd[cursor..s]); + cursor = e; + } + out.push_str(&cmd[cursor..]); + // Trim trailing horizontal whitespace introduced by removals at EOS. + while matches!(out.as_bytes().last(), Some(b' ' | b'\t')) { + out.pop(); + } + + BashFixupResult { command: out, stripped } +} + +fn walk_andor( + list: &AndOrList, + cmd: &str, + ranges: &mut Vec<(usize, usize)>, + stripped: &mut Vec, +) { + process_pipeline(&list.first, cmd, ranges, stripped); + for ao in &list.additional { + let pipe = match ao { + AndOr::And(p) | AndOr::Or(p) => p, + }; + process_pipeline(pipe, cmd, ranges, stripped); + } +} + +fn process_pipeline( + p: &Pipeline, + cmd: &str, + ranges: &mut Vec<(usize, usize)>, + stripped: &mut Vec, +) { + let outcome = try_strip_head_tail(p, cmd, ranges, stripped); + try_strip_2to1(p, cmd, outcome, ranges, stripped); +} + +/// Outcome of the head/tail strip — the 2>&1 pass needs the effective tail. +struct HeadTailOutcome { + stripped: bool, + /// Index into `p.seq` of the new effective last command. Equals + /// `seq.len()-1` when no strip fired, `seq.len()-2` when it did. + last_idx: usize, +} + +fn try_strip_head_tail( + p: &Pipeline, + cmd: &str, + ranges: &mut Vec<(usize, usize)>, + stripped: &mut Vec, +) -> HeadTailOutcome { + let n = p.seq.len(); + let default = HeadTailOutcome { stripped: false, last_idx: n.saturating_sub(1) }; + if n < 2 { + return default; + } + let last = &p.seq[n - 1]; + if !is_safe_head_tail(last) { + return default; + } + let Some(last_loc) = last.location() else { return default }; + + // Pipeline-internal separators are always `|` or `|&` — never `||`. The + // real parser already validated structure, so scanning backwards from the + // start of `last` for the first `|` is unambiguous: AndOr operators + // (`||`, `&&`) only live *between* pipelines, not inside one. We anchor + // here rather than on `prev.location().end` because `SimpleCommand`'s + // span under-reports when its suffix contains unlocated `IoRedirect`s + // (e.g. the synthetic `2>&1` inserted by `|&`). + let bytes = cmd.as_bytes(); + let last_start = last_loc.start.index; + let Some(head) = cmd.get(..last_start) else { return default }; + let Some(pipe_pos) = head.rfind('|') else { return default }; + // Defense in depth against `||`. + if pipe_pos > 0 && bytes[pipe_pos - 1] == b'|' { + return default; + } + if pipe_pos + 1 < bytes.len() && bytes[pipe_pos + 1] == b'|' { + return default; + } + + // Reported text starts at the pipe and is right-trimmed. The deletion + // range walks back through any leading whitespace so the rewrite is + // contiguous. + let stripped_text = cmd[pipe_pos..last_loc.end.index].trim_end().to_owned(); + if stripped_text.is_empty() { + return default; + } + let mut delete_start = pipe_pos; + while delete_start > 0 && matches!(bytes[delete_start - 1], b' ' | b'\t') { + delete_start -= 1; + } + ranges.push((delete_start, last_loc.end.index)); + stripped.push(stripped_text); + HeadTailOutcome { stripped: true, last_idx: n - 2 } +} + +fn try_strip_2to1( + p: &Pipeline, + cmd: &str, + outcome: HeadTailOutcome, + ranges: &mut Vec<(usize, usize)>, + stripped: &mut Vec, +) { + // `2>&1` is only redundant when no downstream pipe remains. After the + // head/tail strip the effective tail is `outcome.last_idx`; if any other + // command sits to its right, abort. + if outcome.stripped { + if outcome.last_idx != 0 { + return; + } + } else if p.seq.len() != 1 { + return; + } + + let target = &p.seq[outcome.last_idx]; + let Command::Simple(simple) = target else { return }; + let Some(name_word) = simple.word_or_name.as_ref() else { return }; + if name_word.value.is_empty() { + return; + } + let Some(suffix) = &simple.suffix else { return }; + if suffix.0.is_empty() { + return; + } + + // The last suffix item must be the `2>&1` redirect, and it must be the + // only redirect on the command (no `> file 2>&1` or `2>&1 > file`). + let Some(last_item) = suffix.0.last() else { return }; + let CommandPrefixOrSuffixItem::IoRedirect(io) = last_item else { return }; + if !is_stderr_to_stdout(io) { + return; + } + for item in &suffix.0[..suffix.0.len() - 1] { + if matches!(item, CommandPrefixOrSuffixItem::IoRedirect(_)) { + return; + } + } + if let Some(prefix) = &simple.prefix + && prefix + .0 + .iter() + .any(|item| matches!(item, CommandPrefixOrSuffixItem::IoRedirect(_))) + { + return; + } + + // `IoRedirect` doesn't carry a source span, so locate the literal + // `2>&1` by scanning forward from the rightmost located item in the + // command — that's either `word_or_name`'s end or the last suffix item + // whose location() is `Some`. Anything before the anchor is already + // accounted for by the AST; the gap between the anchor and `2>&1` is + // guaranteed to be just whitespace by the precondition that `2>&1` is + // the last suffix item and no other redirects exist. + let Some(name_loc) = name_word.loc.as_ref() else { return }; + let mut anchor = name_loc.end.index; + for item in &suffix.0 { + if let Some(loc) = item.location() { + anchor = anchor.max(loc.end.index); + } + } + let bytes = cmd.as_bytes(); + let mut pos = anchor; + while pos < bytes.len() && matches!(bytes[pos], b' ' | b'\t') { + pos += 1; + } + if !cmd.get(pos..).is_some_and(|rest| rest.starts_with("2>&1")) { + return; + } + if pos == 0 { + return; + } + if !matches!(bytes[pos - 1], b' ' | b'\t') { + return; + } + // Walk back through any additional leading whitespace so the rewrite is + // contiguous with neighboring tokens. + let mut delete_start = pos - 1; + while delete_start > 0 && matches!(bytes[delete_start - 1], b' ' | b'\t') { + delete_start -= 1; + } + ranges.push((delete_start, pos + 4)); + stripped.push("2>&1".to_owned()); +} + +fn is_stderr_to_stdout(io: &IoRedirect) -> bool { + let IoRedirect::File(Some(2), IoFileRedirectKind::DuplicateOutput, target) = io else { + return false; + }; + match target { + IoFileRedirectTarget::Fd(1) => true, + IoFileRedirectTarget::Duplicate(w) => w.value == "1", + _ => false, + } +} + +fn is_safe_head_tail(c: &Command) -> bool { + let Command::Simple(simple) = c else { return false }; + let Some(name) = simple.word_or_name.as_ref() else { return false }; + if name.value != "head" && name.value != "tail" { + return false; + } + // Variable assignments / redirects in the prefix would change observable + // shell behavior even with `head` removed. + if let Some(prefix) = &simple.prefix + && !prefix.0.is_empty() + { + return false; + } + let Some(suffix) = &simple.suffix else { return true }; + for item in &suffix.0 { + let CommandPrefixOrSuffixItem::Word(w) = item else { return false }; + if !SAFE_ARG_RE.is_match(&w.value) { + return false; + } + } + true +} + +/// Token shapes that are pure "limit output" flags for `head`/`tail`: +/// `-nN`, `-n=N`, `-cN`, `-c=N` — short flag with attached value +/// `-N` — BSD-style line count +/// `-n`, `-c` — short flag (paired value comes next) +/// `-q`, `-v` — quiet/verbose +/// `--lines[=N]`, `--bytes[=N]` — long flag, optionally attached value +/// `--quiet`, `--verbose` +/// `N` — bare integer (the value half of `-n N`) +/// +/// `+N` offsets (skip-first semantics for `tail`), `-f`/`-F`/`--follow`, +/// `--help`, and any filename token are deliberately rejected — they would +/// change semantics if their host command were removed. +static SAFE_ARG_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"^(?:-[nc]=?\d+|-[nc]|-\d+|-[qv]|--lines(?:=\d+)?|--bytes(?:=\d+)?|--quiet|--verbose|\d+)$") + .expect("static safe-arg regex compiles") +}); + +#[cfg(test)] +mod tests { + use super::*; + + fn run(cmd: &str) -> (String, Vec) { + let r = apply_bash_fixups(cmd); + (r.command, r.stripped) + } + + #[test] + fn strips_trailing_head_tail() { + let cases: &[(&str, &str, &[&str])] = &[ + ("ls | head", "ls", &["| head"]), + ("ls | head -5", "ls", &["| head -5"]), + ("ls | head -n 5", "ls", &["| head -n 5"]), + ("ls | head -n5", "ls", &["| head -n5"]), + ("ls | head -n=5", "ls", &["| head -n=5"]), + ("ls | head -c 100", "ls", &["| head -c 100"]), + ("ls | head --lines=20", "ls", &["| head --lines=20"]), + ("ls | head --lines 20", "ls", &["| head --lines 20"]), + ("ls | head --quiet -5", "ls", &["| head --quiet -5"]), + ("ls | tail -5", "ls", &["| tail -5"]), + ("ls | tail --bytes=200", "ls", &["| tail --bytes=200"]), + ("ls|head", "ls", &["|head"]), + ("ls | tail -20 ", "ls", &["| tail -20"]), + ("git log --oneline | head -20", "git log --oneline", &["| head -20"]), + ("echo a | tr a b | head -3", "echo a | tr a b", &["| head -3"]), + ("just build |& head -5", "just build", &["|& head -5"]), + ]; + for (input, want_cmd, want_stripped) in cases { + let (cmd, stripped) = run(input); + assert_eq!(cmd, *want_cmd, "input: {input:?}"); + assert_eq!(stripped, *want_stripped, "input: {input:?}"); + } + } + + #[test] + fn strips_redundant_2to1() { + let cases: &[(&str, &str, &[&str])] = &[ + ("cmd 2>&1", "cmd", &["2>&1"]), + ("just build 2>&1", "just build", &["2>&1"]), + ( + "just build 2>&1 | tail -3", + "just build", + &["| tail -3", "2>&1"], + ), + ( + "cargo build 2>&1 | head -50", + "cargo build", + &["| head -50", "2>&1"], + ), + ]; + for (input, want_cmd, want_stripped) in cases { + let (cmd, stripped) = run(input); + assert_eq!(cmd, *want_cmd, "input: {input:?}"); + assert_eq!(stripped, *want_stripped, "input: {input:?}"); + } + } + + #[test] + fn strips_across_compound_commands() { + let cases: &[(&str, &str, &[&str])] = &[ + ( + "just build 2>&1 | tail -3 && just up && sleep 4 && just healthz", + "just build && just up && sleep 4 && just healthz", + &["| tail -3", "2>&1"], + ), + ( + "cmd1 | head -5 && cmd2 && cmd3 | tail -3", + "cmd1 && cmd2 && cmd3", + &["| head -5", "| tail -3"], + ), + ( + "echo a; cmd | head -5; echo b", + "echo a; cmd; echo b", + &["| head -5"], + ), + ( + "cmd | head -5 || fallback | tail -3", + "cmd || fallback", + &["| head -5", "| tail -3"], + ), + ( + "cmd1 | head -5 && cmd2 2>&1 | grep err", + "cmd1 && cmd2 2>&1 | grep err", + &["| head -5"], + ), + ]; + for (input, want_cmd, want_stripped) in cases { + let (cmd, stripped) = run(input); + assert_eq!(cmd, *want_cmd, "input: {input:?}"); + assert_eq!(stripped, *want_stripped, "input: {input:?}"); + } + } + + #[test] + fn preserves_semantics_bearing_pipelines() { + let untouched: &[&str] = &[ + "tail -f /var/log/system.log", + "tail -F file.log", + "ls | tail -f -", + "ls | head -5 | sort", + "cat file | head -5 | wc -l", + "cat file | tail -n +2", + "cat file | tail +5", + "ls | head -5 > /tmp/out.txt", + "ls | head -5 2>/dev/null", + "echo \"ls | head -5\"", + "echo $(ls | head -5)", + "head -5 file.txt", + "head /etc/hosts", + "head -5", + "cmd 2>&1 | grep err", + "cmd > file 2>&1", + "cmd >& file", + "cmd 2>&1 > file", + "for f in *.txt; do\n echo $f\ndone | head -5", + "cat <]` footer the bash wrapper already emits when the minimizer rewrites output. Affects pipeline Stage 5 (`truncate_lines_at` in `defs/*.toml`) and the internal callers in `filters/git.rs`, `filters/listing.rs`, and `filters/lint.rs`. ([#1046](https://github.com/can1357/oh-my-pi/issues/1046)) +- Changed bash command preprocessing to use the real `brush-parser` AST via `pi-natives` `applyBashFixups` instead of a hand-rolled top-level mask scanner. The previous regex/character-walking implementation reimplemented quote/heredoc/`$(...)` tracking with conservative bail-outs (notably refusing to fixup commands containing here-strings); the AST-driven version inherits the full shell parser, so semantics-preserving rewrites like stripping `| head -5` off `cat <<<'content' | head -5` now succeed instead of being skipped. No public API change — `applyBashFixups(command)` returns the same `{ command, stripped }` shape. ### Fixed +- Fixed bash command fixups to remove a redundant standalone trailing `2>&1` redirect when no other pipe or redirection remains +- Fixed command-fixup notices to list all stripped segments instead of reporting only one - Fixed summarized `read` output stalling agents on elided regions by appending an explicit footer like `[NN lines across MM elided regions; read :raw or a line range like :1-9999 for verbatim content]`. The footer fires whenever the structural summarizer elided at least one span, so the model gets a concrete recovery selector instead of having to guess from a bare `...` / `{ .. }` marker. Surfaces `elidedLines` on `ReadToolDetails.summary` alongside the existing `elidedSpans`. ([#1046](https://github.com/can1357/oh-my-pi/issues/1046)) - Updated the `read` tool prompt to describe the new elision footer and instruct the model to follow `:raw` (or an explicit line range) when the elided body is actually needed, rather than guessing. - Fixed plugin extensions failing to load when their `peerDependencies` reference internal `pi-*` packages under any scope other than `@mariozechner` (e.g. `Cannot find module '@earendil-works/pi-tui'` from `@juicesharp/rpiv-ask-user-question`, or `Cannot find module '@oh-my-pi/pi-utils'` from `@oh-my-pi/swarm-extension`). The legacy-pi specifier shim now treats `@mariozechner`, `@earendil-works`, **and** the canonical `@oh-my-pi` itself as aliases for the same set of bundled in-process packages (`pi-agent-core`, `pi-ai`, `pi-coding-agent`, `pi-natives`, `pi-tui`, `pi-utils`), and additionally rewrites the upstream-only `pi-ai/oauth` subpath onto our `pi-ai/utils/oauth` layout. Restored the `Key` runtime helper export on `@oh-my-pi/pi-tui` to match upstream — plugins using `Key.enter` / `Key.ctrl("c")` (e.g. `@plannotator/pi-extension`, `@juicesharp/rpiv-ask-user-question`) no longer fail with `Export named 'Key' not found`. End-to-end verified against `@juicesharp/rpiv-ask-user-question`, `@oh-my-pi/swarm-extension`, and `@plannotator/pi-extension` — each now loads cleanly with all of its tools/commands/handlers registered. Plugins importing any of those scopes are remapped to the omp binary's own copy at load time, so peer deps are no longer dragged in from npm and there is exactly one module instance per package regardless of which scope name the plugin's manifest happened to declare. diff --git a/packages/coding-agent/src/tools/bash-command-fixup.ts b/packages/coding-agent/src/tools/bash-command-fixup.ts index 8cc8615c4..bc470d7fe 100644 --- a/packages/coding-agent/src/tools/bash-command-fixup.ts +++ b/packages/coding-agent/src/tools/bash-command-fixup.ts @@ -1,78 +1,47 @@ /** * Conservative transforms applied to a bash command before execution. * - * Currently strips trailing `| head [args]` / `| tail [args]` pipelines that - * exist purely to limit output length: the harness already truncates bash - * output and exposes the full result via an artifact, so these pipes only - * hide content the agent wanted. We refuse to strip in any case where the - * pipe could carry real semantics (multi-line scripts, follow flags, file - * arguments, downstream commands, redirects, subshells, etc.). + * Two fixups are applied, each anchored to the end of a top-level segment + * (segments split on `;`, `&&`, `||`, and background `&`): + * + * 1. Trailing `| head [args]` / `| tail [args]` (and the `|&` variant) — these + * pipes exist purely to limit output length. The harness already truncates + * bash output and exposes the full result via an artifact, so they only + * hide content the agent wanted. + * + * 2. A redundant trailing `2>&1` left on a segment that has no remaining pipe + * or other redirect. The harness already merges stderr into stdout, so the + * duplication is purely cosmetic — and often a leftover after fixup (1) + * drops a downstream pipe. + * + * The heavy lifting (tokenization, quoting, heredoc handling, command + * substitution, nested compound commands) lives in Rust under + * `pi_shell::fixup`, driven by the real `brush-parser` AST. This module is a + * thin sync wrapper plus user-facing notice formatting. */ +import { applyBashFixups as nativeApplyBashFixups } from "@oh-my-pi/pi-natives"; export interface BashFixupResult { /** Possibly-rewritten command. */ command: string; - /** Original substring that was removed, if any (verbatim, including the leading `|`). */ - stripped?: string; + /** Substrings that were stripped, in the order they were removed. */ + stripped: string[]; } /** - * Token shapes for `head`/`tail` that we recognize as pure "limit output" flags. - * - * We deliberately reject `-f`, `-F`, `--follow`, `+N` line offsets, filenames, - * and anything else that could change semantics when removed. - * - * -nN, -n N, -n=N, -cN, -c N, -c=N - * -N (BSD-style `head -5`) - * -q, -v, --quiet, --verbose - * --lines[=N| N], --bytes[=N| N] - * bare integer (the value half of `--lines 5` / `-n 5`) + * Apply both fixups to a bash command. On any parse failure, multi-line input, + * or no-op transform, returns the input verbatim with `stripped: []`. */ -const SAFE_HEAD_TAIL_ARG = String.raw`(?:-[nc]=?\s*\d+|-\d+|-[qv]|--lines(?:=?\s*\d+)?|--bytes(?:=?\s*\d+)?|--quiet|--verbose|\d+)`; - -/** - * Matches a trailing `| head|tail [safe-args]` segment anchored to the end of - * the command. The leading `\s*` is bounded to inline whitespace (no newline) - * so a `|` on its own line never gets swallowed. - */ -const TRAILING_HEAD_TAIL_RE = new RegExp( - String.raw`[ \t]*\|[ \t]*(?:head|tail)(?:[ \t]+${SAFE_HEAD_TAIL_ARG})*[ \t]*$`, -); - -/** - * Strip a trailing `| head` / `| tail` from a single-line bash command. - * - * Bail-out conditions (all preserve the original verbatim): - * - command contains any newline (multi-line scripts may legitimately end a - * pipeline with `head`/`tail` to bound a generator); - * - the matched segment is not the entire command (we never reduce a command - * to an empty string); - * - the `head`/`tail` carries any flag we don't recognize (e.g. `-f`, `-F`, - * `+N`, filenames, redirects) — the regex simply won't match. - */ -export function stripTrailingHeadTail(command: string): BashFixupResult { - // Single-line guard. We check the raw string for any newline anywhere, not - // just at the boundary, because shell continuations, heredocs, function - // bodies, and `for ... done | head` blocks all live behind a newline. - if (command.includes("\n")) return { command }; - - const match = TRAILING_HEAD_TAIL_RE.exec(command); - if (!match || match.index === undefined) return { command }; - - const remainder = command.slice(0, match.index).replace(/[ \t]+$/, ""); - // Never reduce the command to nothing — that would execute as a no-op and - // almost certainly indicates a false-positive match (e.g. the LLM wrote - // `head -5` standalone hoping to read its own stdin). - if (remainder === "") return { command }; - - return { command: remainder, stripped: match[0].trim() }; +export function applyBashFixups(command: string): BashFixupResult { + return nativeApplyBashFixups(command); } /** - * Human-readable notice for the stripped segment. Mirrors the shape of + * Human-readable notice for the fixups that fired. Mirrors the shape of * `formatTimeoutClampNotice` so it can ride alongside the other bash notices. */ -export function formatHeadTailStripNotice(stripped: string | undefined): string | undefined { - if (!stripped) return undefined; - return `Stripped trailing \`${stripped}\` — bash output is truncated automatically and the full result is available via \`artifact://\`.`; +export function formatBashFixupNotice(stripped: readonly string[]): string | undefined { + if (!stripped.length) return undefined; + const quoted = stripped.map(s => `\`${s}\``).join(", "); + return `Stripped redundant ${quoted} — bash output is truncated automatically and stderr is already merged into stdout.`; } diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 1340589b5..41563a0cd 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -17,7 +17,7 @@ import { renderStatusLine } from "../tui"; import { CachedOutputBlock } from "../tui/output-block"; import { getSixelLineMask } from "../utils/sixel"; import type { ToolSession } from "."; -import { formatHeadTailStripNotice, stripTrailingHeadTail } from "./bash-command-fixup"; +import { applyBashFixups, formatBashFixupNotice } from "./bash-command-fixup"; import { type BashInteractiveResult, runInteractiveBashPty } from "./bash-interactive"; import { checkBashInterception } from "./bash-interceptor"; import { expandInternalUrls, type InternalUrlExpansionOptions } from "./bash-skill-urls"; @@ -292,7 +292,7 @@ export class BashTool implements AgentTool { #buildCompletedResult( result: BashResult | BashInteractiveResult, timeoutSec: number, - options: { requestedTimeoutSec?: number; notices?: string[]; terminalId?: string } = {}, + options: { requestedTimeoutSec?: number; notices?: readonly string[]; terminalId?: string } = {}, ): AgentToolResult { const outputLines = [this.#formatResultOutput(result)]; const notices = options.notices?.filter(Boolean) ?? []; @@ -315,7 +315,7 @@ export class BashTool implements AgentTool { label: string, previewText: string, timeoutSec: number, - options: { requestedTimeoutSec?: number; notices?: string[] } = {}, + options: { requestedTimeoutSec?: number; notices?: readonly string[] } = {}, ): AgentToolResult { const details: BashToolDetails = { timeoutSeconds: timeoutSec, @@ -484,15 +484,15 @@ export class BashTool implements AgentTool { let command = rawCommand; const env = normalizeBashEnv(rawEnv); - // Drop trailing `| head|tail` pipes that exist purely to limit output — - // the harness already truncates bash output. Single-line only; the helper - // refuses anything that could change semantics. - let headTailStripped: string | undefined; + // Apply conservative bash fixups (strip trailing `| head|tail` and redundant + // `2>&1`). The helper is single-line only and refuses anything that could + // change semantics. + let bashFixups: string[] = []; if (this.session.settings.get("bash.stripTrailingHeadTail")) { - const fixup = stripTrailingHeadTail(command); - if (fixup.stripped) { + const fixup = applyBashFixups(command); + if (fixup.stripped.length > 0) { command = fixup.command; - headTailStripped = fixup.stripped; + bashFixups = fixup.stripped; } } @@ -574,8 +574,8 @@ export class BashTool implements AgentTool { const pendingNotices: string[] = []; const timeoutClampNotice = formatTimeoutClampNotice(requestedTimeoutSec, timeoutSec); if (timeoutClampNotice) pendingNotices.push(timeoutClampNotice); - const headTailStripNotice = formatHeadTailStripNotice(headTailStripped); - if (headTailStripNotice) pendingNotices.push(headTailStripNotice); + const bashFixupNotice = formatBashFixupNotice(bashFixups); + if (bashFixupNotice) pendingNotices.push(bashFixupNotice); if (asyncRequested) { if (!AsyncJobManager.instance()) { diff --git a/packages/coding-agent/test/tools/bash-command-fixup.test.ts b/packages/coding-agent/test/tools/bash-command-fixup.test.ts new file mode 100644 index 000000000..4ea44ed4c --- /dev/null +++ b/packages/coding-agent/test/tools/bash-command-fixup.test.ts @@ -0,0 +1,144 @@ +import { describe, expect, it } from "bun:test"; +import { applyBashFixups, type BashFixupResult, formatBashFixupNotice } from "../../src/tools/bash-command-fixup"; + +function fixup(command: string): BashFixupResult { + return applyBashFixups(command); +} + +describe("applyBashFixups — strips harmless trailing head/tail", () => { + const cases: Array<[string, string, string[]]> = [ + // [input, expected command, expected stripped list] + ["ls | head", "ls", ["| head"]], + ["ls | head -5", "ls", ["| head -5"]], + ["ls | head -n 5", "ls", ["| head -n 5"]], + ["ls | head -n5", "ls", ["| head -n5"]], + ["ls | head -n=5", "ls", ["| head -n=5"]], + ["ls | head -c 100", "ls", ["| head -c 100"]], + ["ls | head --lines=20", "ls", ["| head --lines=20"]], + ["ls | head --lines 20", "ls", ["| head --lines 20"]], + ["ls | head --quiet -5", "ls", ["| head --quiet -5"]], + ["ls | tail", "ls", ["| tail"]], + ["ls | tail -5", "ls", ["| tail -5"]], + ["ls | tail -n 5", "ls", ["| tail -n 5"]], + ["ls | tail --bytes=200", "ls", ["| tail --bytes=200"]], + ["ls|head", "ls", ["|head"]], + ["ls | tail -20 ", "ls", ["| tail -20"]], + ["git log --oneline | head -20", "git log --oneline", ["| head -20"]], + ["echo a | tr a b | head -3", "echo a | tr a b", ["| head -3"]], + // `|&` (pipe stdout+stderr) is recognized as a pipe too. + ["just build |& head -5", "just build", ["|& head -5"]], + ]; + + for (const [input, expectedCommand, expectedStripped] of cases) { + it(`strips: ${input}`, () => { + const out = fixup(input); + expect(out.command).toBe(expectedCommand); + expect(out.stripped).toEqual(expectedStripped); + }); + } +}); + +describe("applyBashFixups — strips redundant 2>&1", () => { + const cases: Array<[string, string, string[]]> = [ + ["cmd 2>&1", "cmd", ["2>&1"]], + ["just build 2>&1", "just build", ["2>&1"]], + // Combined: trailing `| tail -3` then leftover `2>&1`. + ["just build 2>&1 | tail -3", "just build", ["| tail -3", "2>&1"]], + ["cargo build 2>&1 | head -50", "cargo build", ["| head -50", "2>&1"]], + ]; + + for (const [input, expectedCommand, expectedStripped] of cases) { + it(`strips: ${input}`, () => { + const out = fixup(input); + expect(out.command).toBe(expectedCommand); + expect(out.stripped).toEqual(expectedStripped); + }); + } +}); + +describe("applyBashFixups — strips across compound commands", () => { + const cases: Array<[string, string, string[]]> = [ + [ + "just build 2>&1 | tail -3 && just up && sleep 4 && just healthz", + "just build && just up && sleep 4 && just healthz", + ["| tail -3", "2>&1"], + ], + ["cmd1 | head -5 && cmd2 && cmd3 | tail -3", "cmd1 && cmd2 && cmd3", ["| head -5", "| tail -3"]], + ["echo a; cmd | head -5; echo b", "echo a; cmd; echo b", ["| head -5"]], + ["cmd | head -5 || fallback | tail -3", "cmd || fallback", ["| head -5", "| tail -3"]], + // Only the head/tail-bearing segment gets touched; cmd2's stderr merge survives. + ["cmd1 | head -5 && cmd2 2>&1 | grep err", "cmd1 && cmd2 2>&1 | grep err", ["| head -5"]], + ]; + + for (const [input, expectedCommand, expectedStripped] of cases) { + it(`strips: ${input}`, () => { + const out = fixup(input); + expect(out.command).toBe(expectedCommand); + expect(out.stripped).toEqual(expectedStripped); + }); + } +}); + +describe("applyBashFixups — preserves semantics-bearing pipelines", () => { + const untouched: string[] = [ + // follow-mode and file readers + "tail -f /var/log/system.log", + "tail -F file.log", + "ls | tail -f -", + // non-trailing head/tail + "ls | head -5 | sort", + "cat file | head -5 | wc -l", + // +N offset (skip-first semantics, not a limit) + "cat file | tail -n +2", + "cat file | tail +5", + // redirects on head's output + "ls | head -5 > /tmp/out.txt", + "ls | head -5 2>/dev/null", + // inside a string / subshell — top-level end is `"` or `)` + 'echo "ls | head -5"', + "echo $(ls | head -5)", + // no `|` at all + "head -5 file.txt", + "head /etc/hosts", + // would reduce to empty + "| head -5", + "head -5", + // 2>&1 with other redirects or piped consumer — must stay + "cmd 2>&1 | grep err", + "cmd > file 2>&1", + "cmd >& file", + "cmd 2>&1 > file", + // bail-outs: multi-line / heredoc / unbalanced quotes + "for f in *.txt; do\n echo $f\ndone | head -5", + "cat < { + const out = fixup(input); + expect(out.command).toBe(input); + expect(out.stripped).toEqual([]); + }); + } +}); + +describe("formatBashFixupNotice", () => { + it("returns undefined when nothing was stripped", () => { + expect(formatBashFixupNotice([])).toBeUndefined(); + }); + + it("embeds a single stripped segment in the notice", () => { + const notice = formatBashFixupNotice(["| head -5"]); + expect(notice).toContain("`| head -5`"); + expect(notice).toContain("stderr is already merged"); + }); + + it("joins multiple stripped segments with commas", () => { + const notice = formatBashFixupNotice(["| tail -3", "2>&1"]); + expect(notice).toContain("`| tail -3`"); + expect(notice).toContain("`2>&1`"); + expect(notice).toMatch(/`\| tail -3`,\s*`2>&1`/); + }); +}); diff --git a/packages/coding-agent/test/tools/bash-head-tail-strip.test.ts b/packages/coding-agent/test/tools/bash-head-tail-strip.test.ts deleted file mode 100644 index 53e41391f..000000000 --- a/packages/coding-agent/test/tools/bash-head-tail-strip.test.ts +++ /dev/null @@ -1,100 +0,0 @@ -import { describe, expect, it } from "bun:test"; -import { - type BashFixupResult, - formatHeadTailStripNotice, - stripTrailingHeadTail, -} from "../../src/tools/bash-command-fixup"; - -function strip(command: string): BashFixupResult { - return stripTrailingHeadTail(command); -} - -describe("stripTrailingHeadTail — strips harmless trailing limits", () => { - const cases: Array<[string, string, string]> = [ - // [input, expected command, expected stripped suffix] - ["ls | head", "ls", "| head"], - ["ls | head -5", "ls", "| head -5"], - ["ls | head -n 5", "ls", "| head -n 5"], - ["ls | head -n5", "ls", "| head -n5"], - ["ls | head -n=5", "ls", "| head -n=5"], - ["ls | head -c 100", "ls", "| head -c 100"], - ["ls | head --lines=20", "ls", "| head --lines=20"], - ["ls | head --lines 20", "ls", "| head --lines 20"], - ["ls | head --quiet -5", "ls", "| head --quiet -5"], - ["ls | tail", "ls", "| tail"], - ["ls | tail -5", "ls", "| tail -5"], - ["ls | tail -n 5", "ls", "| tail -n 5"], - ["ls | tail --bytes=200", "ls", "| tail --bytes=200"], - ["ls|head", "ls", "|head"], - ["ls | tail -20 ", "ls", "| tail -20"], - // cd/sub-pipeline preserved; only the trailing limit goes - ["git log --oneline | head -20", "git log --oneline", "| head -20"], - ["echo a | tr a b | head -3", "echo a | tr a b", "| head -3"], - // command with stderr redirect before the limit stays intact - ["cargo build 2>&1 | head -50", "cargo build 2>&1", "| head -50"], - ]; - - for (const [input, expectedCommand, expectedStripped] of cases) { - it(`strips: ${input}`, () => { - const out = strip(input); - expect(out.command).toBe(expectedCommand); - expect(out.stripped).toBe(expectedStripped); - }); - } -}); - -describe("stripTrailingHeadTail — preserves semantics-bearing pipelines", () => { - const untouched: string[] = [ - // follow-mode and file readers - "tail -f /var/log/system.log", - "tail -F file.log", - "ls | tail -f -", - // non-trailing head/tail - "ls | head -5 | sort", - "cat file | head -5 | wc -l", - // +N offset (skip-first semantics, not a limit) - "cat file | tail -n +2", - "cat file | tail +5", - // downstream commands / operators - "ls | head -5 && echo done", - "ls | head -5 || echo failed", - "ls | head -5 ; echo done", - "ls | head -5 &", - // redirects on head's output - "ls | head -5 > /tmp/out.txt", - "ls | head -5 2>/dev/null", - // inside a string / subshell — anchored end is `"` or `)` - 'echo "ls | head -5"', - "echo $(ls | head -5)", - // no `|` at all - "head -5 file.txt", - "head /etc/hosts", - // would reduce to empty - "| head -5", - "head -5", - // multiline scripts: head bounds a loop body, must stay - "for f in *.txt; do\n echo $f\ndone | head -5", - "cat < { - const out = strip(input); - expect(out.command).toBe(input); - expect(out.stripped).toBeUndefined(); - }); - } -}); - -describe("formatHeadTailStripNotice", () => { - it("returns undefined when nothing was stripped", () => { - expect(formatHeadTailStripNotice(undefined)).toBeUndefined(); - }); - - it("embeds the stripped segment in the notice", () => { - const notice = formatHeadTailStripNotice("| head -5"); - expect(notice).toContain("| head -5"); - expect(notice).toContain("artifact://"); - }); -}); diff --git a/packages/coding-agent/test/tools/bash-interceptor.test.ts b/packages/coding-agent/test/tools/bash-interceptor.test.ts index 27a949e2d..ccd869e2d 100644 --- a/packages/coding-agent/test/tools/bash-interceptor.test.ts +++ b/packages/coding-agent/test/tools/bash-interceptor.test.ts @@ -88,7 +88,7 @@ describe("BashTool head/tail stripping", () => { } as AgentToolContext); const text = result.content.find(b => b.type === "text")?.text ?? ""; expect(text).toContain("100"); - expect(text).toContain("Stripped trailing `| head -3`"); + expect(text).toContain("Stripped redundant `| head -3`"); }); it("does not strip when the setting is disabled", async () => { diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 21d4dddc6..2a1f939f9 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -5,6 +5,7 @@ ### Added - Added a per-release version sentinel napi export (`__piNativesV{major}_{minor}_{patch}`). The Rust `js_name` is bumped in lock-step with the package version by `scripts/release.ts`; the JS loader computes the expected name from `package.json#version` and throws an actionable error when the on-disk `.node` doesn't expose it. This converts the silent ` is not a function` crash from a stale addon into a load-time failure pointing at the real fix. +- Added `applyBashFixups(command)` — a synchronous brush-parser-driven rewrite that strips trailing `| head|tail …`, redundant `2>&1`, and the `|&` shorthand from top-level pipelines, returning `{ command, stripped }`. Replaces the hand-rolled top-level mask scanner in `pi-coding-agent`; tokenization, quoting, heredocs, command substitution, and nested compound commands are now handled by the real shell AST instead of regex/character-walking. Lives in `pi_shell::fixup` on the Rust side. ### Fixed diff --git a/packages/natives/native/index.d.ts b/packages/natives/native/index.d.ts index 44f3fae3a..68edfe5a4 100644 --- a/packages/natives/native/index.d.ts +++ b/packages/natives/native/index.d.ts @@ -138,6 +138,15 @@ export declare class Shell { */ export declare function __piNativesV15_0_1(): void +/** + * Apply conservative pre-execution rewrites to a bash command. + * + * Strips trailing `| head|tail [safe-args]` and redundant trailing `2>&1` + * from each top-level pipeline. The full rules and bail conditions live in + * `pi_shell::fixup`. Synchronous and cheap (one parse pass over the input). + */ +export declare function applyBashFixups(command: string): BashFixupResult + /** * Apply ast-grep rewrite rules to matching files; honors `dryRun` and returns * a promise. @@ -324,6 +333,17 @@ export interface AstReplaceResult { parseErrors?: Array } +/** + * Result of [`apply_bash_fixups`]: a possibly-rewritten command plus the + * substrings that were removed (in source order). + */ +export interface BashFixupResult { + /** Possibly-rewritten command. Equal to the input when no fixup fired. */ + command: string + /** Substrings removed, in source order — suitable for a user-facing notice. */ + stripped: Array +} + /** Clipboard image payload encoded as PNG bytes. */ export interface ClipboardImage { /** PNG-encoded image bytes. */ diff --git a/packages/natives/native/index.js b/packages/natives/native/index.js index 381206cda..6ce347978 100644 --- a/packages/natives/native/index.js +++ b/packages/natives/native/index.js @@ -24,6 +24,7 @@ export const Shell = nativeBindings.Shell; // functions export const __piNativesV15_0_1 = nativeBindings.__piNativesV15_0_1; +export const applyBashFixups = nativeBindings.applyBashFixups; export const astEdit = nativeBindings.astEdit; export const astGrep = nativeBindings.astGrep; export const copyToClipboard = nativeBindings.copyToClipboard; From 7cd05c374c1a7d693f6dc2bab101207747d74fb9 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 02:12:34 +0200 Subject: [PATCH 05/22] feat(coding-agent/eval): enabled top-level await execution in Python runner cells - Added a persistent asyncio event loop and coroutine-aware compiled-code execution for runner cells. - Enabled compiling notebook cells with top-level await flags and awaiting coroutine results before rendering. - Added an integration test confirming top-level await works across kernel cells with preserved state. --- packages/coding-agent/src/eval/py/runner.py | 53 +++++++++++++++---- .../core/python-runner.integration.test.ts | 18 +++++++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/src/eval/py/runner.py b/packages/coding-agent/src/eval/py/runner.py index c947527a5..280590c6f 100644 --- a/packages/coding-agent/src/eval/py/runner.py +++ b/packages/coding-agent/src/eval/py/runner.py @@ -25,9 +25,11 @@ when installed. from __future__ import annotations +import asyncio import ast import base64 import builtins +import inspect import io import json import os @@ -120,6 +122,7 @@ class _RunnerState: "__builtins__": builtins, } self.last_install_marker: int = 0 + self.loop: asyncio.AbstractEventLoop | None = None _STATE = _RunnerState() @@ -688,13 +691,41 @@ _install_builtins(_STATE.user_ns) # --------------------------------------------------------------------------- +_TLA_FLAG = getattr(ast, "PyCF_ALLOW_TOP_LEVEL_AWAIT", 0x2000) + + +def _get_event_loop() -> asyncio.AbstractEventLoop: + loop = _STATE.loop + if loop is None or loop.is_closed(): + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + _STATE.loop = loop + return loop + + +def _run_compiled(code, ns: dict, *, want_value: bool) -> Any: + """Execute a code object, awaiting it if compiled as a coroutine. + + ``want_value`` is True for the trailing expression — we return ``eval``'s + result (or the awaited coroutine's value). For statement blocks the + return is always ``None``. + """ + if code.co_flags & inspect.CO_COROUTINE: + coro = eval(code, ns) + result = _get_event_loop().run_until_complete(coro) + return result if want_value else None + if want_value: + return eval(code, ns) + exec(code, ns) + return None + + def _exec_source(source: str, ns: dict) -> None: """Compile + execute ``source``; if the last node is an expression, route - its value through ``__omp_display`` so dataframes/figures render rich.""" - try: - module = ast.parse(source, mode="exec") - except SyntaxError: - raise + its value through ``__omp_display`` so dataframes/figures render rich. + Top-level ``await`` / ``async for`` / ``async with`` is permitted; the + cell is driven through the runner's persistent event loop.""" + module = ast.parse(source, mode="exec") if not module.body: return @@ -704,16 +735,16 @@ def _exec_source(source: str, ns: dict) -> None: body_module = ast.Module(body=module.body[:-1], type_ignores=[]) expr_module = ast.Expression(body=last.value) ast.copy_location(expr_module, last) - body_code = compile(body_module, "", "exec") - expr_code = compile(expr_module, "", "eval") - exec(body_code, ns) - value = eval(expr_code, ns) + body_code = compile(body_module, "", "exec", flags=_TLA_FLAG) + expr_code = compile(expr_module, "", "eval", flags=_TLA_FLAG) + _run_compiled(body_code, ns, want_value=False) + value = _run_compiled(expr_code, ns, want_value=True) if value is not None: __omp_display(value, kind="result") return - code = compile(module, "", "exec") - exec(code, ns) + code = compile(module, "", "exec", flags=_TLA_FLAG) + _run_compiled(code, ns, want_value=False) # --------------------------------------------------------------------------- diff --git a/packages/coding-agent/test/core/python-runner.integration.test.ts b/packages/coding-agent/test/core/python-runner.integration.test.ts index 6b5cb86c7..fb82cfc33 100644 --- a/packages/coding-agent/test/core/python-runner.integration.test.ts +++ b/packages/coding-agent/test/core/python-runner.integration.test.ts @@ -90,6 +90,24 @@ describe.skipIf(!SHOULD_RUN)("python runner subprocess", () => { } }); + it("supports top-level await across cells", async () => { + using tempDir = TempDir.createSync("@python-runner-await-"); + const kernel = await PythonKernel.start({ cwd: tempDir.path() }); + try { + const first = await executePythonWithKernel( + kernel, + ["import asyncio", "x = await asyncio.sleep(0, result=21)", "x * 2"].join("\n"), + ); + expect(first.exitCode).toBe(0); + expect(first.output).toContain("42"); + const second = await executePythonWithKernel(kernel, "x + 1"); + expect(second.exitCode).toBe(0); + expect(second.output).toContain("22"); + } finally { + await kernel.shutdown(); + } + }); + it("translates %pwd magic to the user namespace", async () => { using tempDir = TempDir.createSync("@python-runner-magic-"); const kernel = await PythonKernel.start({ cwd: tempDir.path() }); From f9098cff398aa19914b8475e889538f4a5c390b0 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 02:18:28 +0200 Subject: [PATCH 06/22] config(coding-agent/lsp): disabled rust-analyzer check-on-save in LSP defaults - Updated the Rust LSP default settings to set rust-analyzer.checkOnSave to false. --- packages/coding-agent/src/lsp/defaults.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/lsp/defaults.json b/packages/coding-agent/src/lsp/defaults.json index db96a964f..41d0821dd 100644 --- a/packages/coding-agent/src/lsp/defaults.json +++ b/packages/coding-agent/src/lsp/defaults.json @@ -5,7 +5,11 @@ "fileTypes": [".rs"], "rootMarkers": ["Cargo.toml", "rust-analyzer.toml"], "initOptions": {}, - "settings": {}, + "settings": { + "rust-analyzer": { + "checkOnSave": false + } + }, "capabilities": { "flycheck": true, "ssr": true, From 7754607561457d4a435ea86859bf5a8b6f0b8fbc Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 02:23:35 +0200 Subject: [PATCH 07/22] fix(coding-agent/tools): wrapped bash fixup notice in system-warning tags - Updated `formatBashFixupNotice` to wrap the stripped-pattern warning in a `` wrapper. - Reworded the notice to clarify output is already truncated and stderr is merged into stdout. - Extended the Bash interceptor test to verify the warning tag appears for head/tail stripping. --- packages/coding-agent/src/tools/bash-command-fixup.ts | 2 +- packages/coding-agent/test/tools/bash-interceptor.test.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/tools/bash-command-fixup.ts b/packages/coding-agent/src/tools/bash-command-fixup.ts index bc470d7fe..cd1bf4f38 100644 --- a/packages/coding-agent/src/tools/bash-command-fixup.ts +++ b/packages/coding-agent/src/tools/bash-command-fixup.ts @@ -43,5 +43,5 @@ export function applyBashFixups(command: string): BashFixupResult { export function formatBashFixupNotice(stripped: readonly string[]): string | undefined { if (!stripped.length) return undefined; const quoted = stripped.map(s => `\`${s}\``).join(", "); - return `Stripped redundant ${quoted} — bash output is truncated automatically and stderr is already merged into stdout.`; + return `Stripped redundant ${quoted} — bash output is already truncated and stderr is already merged into stdout. NEVER use these patterns.`; } diff --git a/packages/coding-agent/test/tools/bash-interceptor.test.ts b/packages/coding-agent/test/tools/bash-interceptor.test.ts index ccd869e2d..161b3a766 100644 --- a/packages/coding-agent/test/tools/bash-interceptor.test.ts +++ b/packages/coding-agent/test/tools/bash-interceptor.test.ts @@ -88,6 +88,7 @@ describe("BashTool head/tail stripping", () => { } as AgentToolContext); const text = result.content.find(b => b.type === "text")?.text ?? ""; expect(text).toContain("100"); + expect(text).toContain(""); expect(text).toContain("Stripped redundant `| head -3`"); }); From 2ea1bd96b87aa76fd701ddef4ca2c86ea778f5ad Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 02:23:43 +0200 Subject: [PATCH 08/22] feat(python/omp-rpc): added user and group options to RpcClient startup - Added user, group, and extra_groups parameters to RpcClient initialization. - Propagated those parameters through to subprocess startup calls. - Added tests to verify the new kwargs are passed correctly, including None defaults and empty extra groups. --- python/omp-rpc/src/omp_rpc/client.py | 9 +++++ python/omp-rpc/tests/test_user_group.py | 45 +++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 python/omp-rpc/tests/test_user_group.py diff --git a/python/omp-rpc/src/omp_rpc/client.py b/python/omp-rpc/src/omp_rpc/client.py index eaac8921f..e1906465c 100644 --- a/python/omp-rpc/src/omp_rpc/client.py +++ b/python/omp-rpc/src/omp_rpc/client.py @@ -278,6 +278,9 @@ class RpcClient: session_dir: str | Path | None = None, cwd: str | Path | None = None, env: Mapping[str, str] | None = None, + user: int | str | None = None, + group: int | str | None = None, + extra_groups: Sequence[int | str] | None = None, thinking: ThinkingLevel | None = None, append_system_prompt: str | None = None, provider_session_id: str | None = None, @@ -302,6 +305,9 @@ class RpcClient: self._session_dir = Path(session_dir) if session_dir is not None else None self._cwd = Path(cwd) if cwd is not None else None self._env = dict(env or {}) + self._user = user + self._group = group + self._extra_groups = list(extra_groups) if extra_groups is not None else None self._thinking = thinking self._append_system_prompt = append_system_prompt self._provider_session_id = provider_session_id @@ -403,6 +409,9 @@ class RpcClient: list(self._build_command()), cwd=str(self._cwd) if self._cwd is not None else None, env={**os.environ, **self._env}, + user=self._user, + group=self._group, + extra_groups=self._extra_groups, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, diff --git a/python/omp-rpc/tests/test_user_group.py b/python/omp-rpc/tests/test_user_group.py new file mode 100644 index 000000000..8f6b41fdc --- /dev/null +++ b/python/omp-rpc/tests/test_user_group.py @@ -0,0 +1,45 @@ +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from omp_rpc import RpcClient + + +class _Sentinel(Exception): + pass + + +def _start_and_capture(**kwargs): + client = RpcClient(**kwargs) + with patch("omp_rpc.client.subprocess.Popen", side_effect=_Sentinel("aborted")) as mock_popen: + with pytest.raises(_Sentinel): + client.start() + assert mock_popen.call_count == 1 + return mock_popen.call_args + + +def test_no_user_group_defaults_to_none(): + call = _start_and_capture(executable="omp") + assert call.kwargs["user"] is None + assert call.kwargs["group"] is None + assert call.kwargs["extra_groups"] is None + + +def test_user_and_group_kwargs_threaded(): + call = _start_and_capture( + executable="omp", + user=2001, + group="omp", + extra_groups=[2000, "docker"], + ) + assert call.kwargs["user"] == 2001 + assert call.kwargs["group"] == "omp" + assert call.kwargs["extra_groups"] == [2000, "docker"] + + +def test_extra_groups_none_distinct_from_empty(): + call = _start_and_capture(executable="omp", extra_groups=[]) + # [] means an empty supplementary group list and differs from None. + assert call.kwargs["extra_groups"] == [] From 1b40197b8c52c876f56d563a57daaea580e312e0 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 15 May 2026 01:06:59 +0000 Subject: [PATCH 09/22] fix(discovery): respect disabledProviders in discoverAgents for claude-plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit discoverAgents() called listClaudePluginRoots() unconditionally, so agents from Claude Code marketplace plugins appeared in /agents and the Agent Control Center even when claude-plugins was listed in disabledProviders. Guard the listClaudePluginRoots() call with isProviderEnabled("claude-plugins"), returning an empty roots array when the provider is disabled — matching how filterProviders() handles every other capability's provider set. Added regression test that verifies both the enabled path (agents visible) and the disabled path (agents absent) using a real temp-directory plugin registry fixture. Fixes #1075 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/task/discovery.ts | 7 +- ...agent-discovery-disabled-providers.test.ts | 80 +++++++++++++++++++ 3 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/discovery/agent-discovery-disabled-providers.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3aa4adaa6..401b05c7f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,7 @@ - Updated the `read` tool prompt to describe the new elision footer and instruct the model to follow `:raw` (or an explicit line range) when the elided body is actually needed, rather than guessing. - Fixed plugin extensions failing to load when their `peerDependencies` reference internal `pi-*` packages under any scope other than `@mariozechner` (e.g. `Cannot find module '@earendil-works/pi-tui'` from `@juicesharp/rpiv-ask-user-question`, or `Cannot find module '@oh-my-pi/pi-utils'` from `@oh-my-pi/swarm-extension`). The legacy-pi specifier shim now treats `@mariozechner`, `@earendil-works`, **and** the canonical `@oh-my-pi` itself as aliases for the same set of bundled in-process packages (`pi-agent-core`, `pi-ai`, `pi-coding-agent`, `pi-natives`, `pi-tui`, `pi-utils`), and additionally rewrites the upstream-only `pi-ai/oauth` subpath onto our `pi-ai/utils/oauth` layout. Restored the `Key` runtime helper export on `@oh-my-pi/pi-tui` to match upstream — plugins using `Key.enter` / `Key.ctrl("c")` (e.g. `@plannotator/pi-extension`, `@juicesharp/rpiv-ask-user-question`) no longer fail with `Export named 'Key' not found`. End-to-end verified against `@juicesharp/rpiv-ask-user-question`, `@oh-my-pi/swarm-extension`, and `@plannotator/pi-extension` — each now loads cleanly with all of its tools/commands/handlers registered. Plugins importing any of those scopes are remapped to the omp binary's own copy at load time, so peer deps are no longer dragged in from npm and there is exactly one module instance per package regardless of which scope name the plugin's manifest happened to declare. +- Fixed `discoverAgents()` ignoring `disabledProviders` for the `claude-plugins` provider. Plugin roots from `~/.claude/plugins/` were scanned unconditionally, so agents from Claude Code marketplace plugins continued to appear in `/agents` and the Agent Control Center even when `disabledProviders: [claude-plugins]` was set. The discovery path now checks `isProviderEnabled("claude-plugins")` before calling `listClaudePluginRoots()`, matching how every other capability respects the disabled-providers set. ([#1075](https://github.com/can1357/oh-my-pi/issues/1075)) ## [15.0.1] - 2026-05-14 ### Breaking Changes diff --git a/packages/coding-agent/src/task/discovery.ts b/packages/coding-agent/src/task/discovery.ts index adb2bf1d9..c1ce37136 100644 --- a/packages/coding-agent/src/task/discovery.ts +++ b/packages/coding-agent/src/task/discovery.ts @@ -17,6 +17,7 @@ import * as path from "node:path"; import { logger } from "@oh-my-pi/pi-utils"; import { findAllNearestProjectConfigDirs, getConfigDirs } from "../config"; import { listClaudePluginRoots } from "../discovery/helpers"; +import { isProviderEnabled } from "../capability"; import { loadBundledAgents, parseAgent } from "./agents"; import type { AgentDefinition, AgentSource } from "./types"; @@ -87,8 +88,10 @@ export async function discoverAgents(cwd: string, home: string = os.homedir()): if (user) orderedDirs.push({ dir: user.path, source: "user" }); } - // Load agents from Claude Code marketplace plugins - const { roots: pluginRoots } = await listClaudePluginRoots(home, resolvedCwd); + // Load agents from Claude Code marketplace plugins (respects disabledProviders) + const { roots: pluginRoots } = isProviderEnabled("claude-plugins") + ? await listClaudePluginRoots(home, resolvedCwd) + : { roots: [] }; const sortedPluginRoots = [...pluginRoots].sort((a, b) => { if (a.scope === b.scope) return 0; return a.scope === "project" ? -1 : 1; diff --git a/packages/coding-agent/test/discovery/agent-discovery-disabled-providers.test.ts b/packages/coding-agent/test/discovery/agent-discovery-disabled-providers.test.ts new file mode 100644 index 000000000..cdca0d86a --- /dev/null +++ b/packages/coding-agent/test/discovery/agent-discovery-disabled-providers.test.ts @@ -0,0 +1,80 @@ +/** + * Regression test for #1075: + * discoverAgents() must skip Claude plugin roots when claude-plugins is disabled. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { disableProvider, enableProvider } from "../../src/capability"; +import { clearCache as clearFsCache } from "../../src/capability/fs"; +import { clearClaudePluginRootsCache } from "../../src/discovery/helpers"; +import { discoverAgents } from "../../src/task/discovery"; + +const PLUGIN_AGENT_MD = [ + "---", + "name: simplifier", + "description: A code simplifier agent from a Claude plugin", + "---", + "Simplify code.", +].join("\n"); + +describe("discoverAgents — claude-plugins disabled provider", () => { + let tempHome: string; + + beforeEach(() => { + tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "pi-agent-disco-home-")); + + // Build a fake Claude plugin install with an agents/ subdirectory. + const pluginInstallPath = path.join(tempHome, "plugin-cache", "code-simplifier"); + const agentsDir = path.join(pluginInstallPath, "agents"); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.writeFileSync(path.join(agentsDir, "simplifier.md"), PLUGIN_AGENT_MD); + + // Register the plugin in the Claude registry so listClaudePluginRoots picks it up. + const claudePluginsDir = path.join(tempHome, ".claude", "plugins"); + fs.mkdirSync(claudePluginsDir, { recursive: true }); + fs.writeFileSync( + path.join(claudePluginsDir, "installed_plugins.json"), + JSON.stringify({ + version: 2, + plugins: { + "code-simplifier@claude-plugins-official": [ + { + installPath: pluginInstallPath, + version: "1.0.0", + scope: "user", + installedAt: "2025-01-01T00:00:00Z", + lastUpdated: "2025-01-01T00:00:00Z", + }, + ], + }, + }), + ); + + // Start each test with a clean provider + cache state. + enableProvider("claude-plugins"); + clearFsCache(); + clearClaudePluginRootsCache(); + }); + + afterEach(() => { + fs.rmSync(tempHome, { recursive: true, force: true }); + // Restore global state so other tests in the suite are not affected. + enableProvider("claude-plugins"); + clearFsCache(); + clearClaudePluginRootsCache(); + }); + + test("includes plugin agents when claude-plugins is enabled", async () => { + const { agents } = await discoverAgents(tempHome, tempHome); + expect(agents.map(a => a.name)).toContain("simplifier"); + }); + + test("excludes plugin agents when claude-plugins is disabled", async () => { + disableProvider("claude-plugins"); + clearClaudePluginRootsCache(); + const { agents } = await discoverAgents(tempHome, tempHome); + expect(agents.map(a => a.name)).not.toContain("simplifier"); + }); +}); From 8b22d6de6530cfdcd1706c7adb10bc353f846b30 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 15 May 2026 01:07:19 +0000 Subject: [PATCH 10/22] style: bun run fix --- packages/coding-agent/src/task/discovery.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/coding-agent/src/task/discovery.ts b/packages/coding-agent/src/task/discovery.ts index c1ce37136..b78d04bea 100644 --- a/packages/coding-agent/src/task/discovery.ts +++ b/packages/coding-agent/src/task/discovery.ts @@ -15,9 +15,9 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { logger } from "@oh-my-pi/pi-utils"; +import { isProviderEnabled } from "../capability"; import { findAllNearestProjectConfigDirs, getConfigDirs } from "../config"; import { listClaudePluginRoots } from "../discovery/helpers"; -import { isProviderEnabled } from "../capability"; import { loadBundledAgents, parseAgent } from "./agents"; import type { AgentDefinition, AgentSource } from "./types"; From 1519224949b1a8c46b609aa0105eaaed6b208b62 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 15 May 2026 01:08:09 +0000 Subject: [PATCH 11/22] fix(bash): accept readonly string[] for notices in buildCompletedResult MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TypeScript 6 tightens mutability checks; the notices parameter was typed as string[] but the caller held a readonly string[] (from the options object). Widening both private method overloads to readonly string[] satisfies the type checker without any runtime change — filter(Boolean) works on readonly arrays. --- packages/coding-agent/src/tools/bash.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 1340589b5..3139224c3 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -292,7 +292,7 @@ export class BashTool implements AgentTool { #buildCompletedResult( result: BashResult | BashInteractiveResult, timeoutSec: number, - options: { requestedTimeoutSec?: number; notices?: string[]; terminalId?: string } = {}, + options: { requestedTimeoutSec?: number; notices?: readonly string[]; terminalId?: string } = {}, ): AgentToolResult { const outputLines = [this.#formatResultOutput(result)]; const notices = options.notices?.filter(Boolean) ?? []; @@ -315,7 +315,7 @@ export class BashTool implements AgentTool { label: string, previewText: string, timeoutSec: number, - options: { requestedTimeoutSec?: number; notices?: string[] } = {}, + options: { requestedTimeoutSec?: number; notices?: readonly string[] } = {}, ): AgentToolResult { const details: BashToolDetails = { timeoutSeconds: timeoutSec, From f0ff398607093166bf215ecb8ff0661b809c74e8 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 03:14:52 +0200 Subject: [PATCH 12/22] fix(coding-agent/tools): stripped duplicate output notices from TUI tool renderers - Added stripOutputNotice to output-meta to remove appended truncation notices when output metadata is available. - Updated bash, eval, browser, read, and ssh renderers to strip the notice before display so the styled warning line is not duplicated. - Left fallback behavior unchanged so outputs without a notice continue through unchanged. --- packages/coding-agent/src/tools/bash.ts | 9 +- .../coding-agent/src/tools/browser/render.ts | 4 +- packages/coding-agent/src/tools/eval.ts | 12 +- .../coding-agent/src/tools/output-meta.ts | 26 ++++ packages/coding-agent/src/tools/read.ts | 5 +- packages/coding-agent/src/tools/ssh.ts | 5 +- .../test/tools/strip-output-notice.test.ts | 112 ++++++++++++++++++ 7 files changed, 163 insertions(+), 10 deletions(-) create mode 100644 packages/coding-agent/test/tools/strip-output-notice.test.ts diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 41563a0cd..f2a73306f 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -21,7 +21,7 @@ import { applyBashFixups, formatBashFixupNotice } from "./bash-command-fixup"; import { type BashInteractiveResult, runInteractiveBashPty } from "./bash-interactive"; import { checkBashInterception } from "./bash-interceptor"; import { expandInternalUrls, type InternalUrlExpansionOptions } from "./bash-skill-urls"; -import { formatStyledTruncationWarning, type OutputMeta } from "./output-meta"; +import { formatStyledTruncationWarning, type OutputMeta, stripOutputNotice } from "./output-meta"; import { resolveToCwd } from "./path-utils"; import { formatToolWorkingDirectory, replaceTabs } from "./render-utils"; import { ToolAbortError, ToolError } from "./tool-errors"; @@ -977,8 +977,11 @@ export function createShellRenderer(config: ShellRendererConfig) { const expanded = renderContext?.expanded ?? options.expanded; const previewLines = renderContext?.previewLines ?? BASH_DEFAULT_PREVIEW_LINES; - // Get output from context (preferred) or fall back to result content - const output = renderContext?.output ?? result.content?.find(c => c.type === "text")?.text ?? ""; + // Get output from context (preferred) or fall back to result content. + // Strip the LLM-facing notice appended by wrappedExecute so we don't + // double-print it alongside the styled warning line below. + const rawOutput = renderContext?.output ?? result.content?.find(c => c.type === "text")?.text ?? ""; + const output = stripOutputNotice(rawOutput, details?.meta); const displayOutput = output.trimEnd(); const showingFullOutput = expanded && renderContext?.isFullOutput === true; diff --git a/packages/coding-agent/src/tools/browser/render.ts b/packages/coding-agent/src/tools/browser/render.ts index 8eda81b4f..91c362917 100644 --- a/packages/coding-agent/src/tools/browser/render.ts +++ b/packages/coding-agent/src/tools/browser/render.ts @@ -11,7 +11,7 @@ import type { RenderResultOptions } from "../../extensibility/custom-tools/types import type { Theme } from "../../modes/theme/theme"; import { Hasher, renderCodeCell, renderStatusLine } from "../../tui"; import type { BrowserToolDetails } from "../browser"; -import { formatStyledTruncationWarning } from "../output-meta"; +import { formatStyledTruncationWarning, stripOutputNotice } from "../output-meta"; import { replaceTabs, shortenPath } from "../render-utils"; const BROWSER_DEFAULT_PREVIEW_LINES = 10; @@ -195,7 +195,7 @@ export const browserToolRenderer = { const details = result.details; const action = details?.action ?? argsObj.action; const isError = result.isError === true; - const output = extractTextOutput(result.content); + const output = stripOutputNotice(extractTextOutput(result.content), details?.meta); if (action === "run") { let component = renderRunCell(argsObj, details, options, output, isError, theme); diff --git a/packages/coding-agent/src/tools/eval.ts b/packages/coding-agent/src/tools/eval.ts index f8ed5c0ea..85adcce1d 100644 --- a/packages/coding-agent/src/tools/eval.ts +++ b/packages/coding-agent/src/tools/eval.ts @@ -16,7 +16,12 @@ import evalDescription from "../prompts/tools/eval.md" with { type: "text" }; import { DEFAULT_MAX_BYTES, OutputSink, type OutputSummary, TailBuffer } from "../session/streaming-output"; import { getTreeBranch, getTreeContinuePrefix, renderCodeCell } from "../tui"; import { resolveEvalBackends, type ToolSession } from "."; -import { formatStyledTruncationWarning, resolveOutputMaxColumns, resolveOutputSinkHeadBytes } from "./output-meta"; +import { + formatStyledTruncationWarning, + resolveOutputMaxColumns, + resolveOutputSinkHeadBytes, + stripOutputNotice, +} from "./output-meta"; import { formatTitle, replaceTabs, shortenPath, truncateToWidth, wrapBrackets } from "./render-utils"; import { ToolAbortError, ToolError } from "./tool-errors"; import { toolResult } from "./tool-result"; @@ -922,8 +927,11 @@ export const evalToolRenderer = { ): Component { const details = result.details; - const output = + const rawOutput = options.renderContext?.output ?? (result.content?.find(c => c.type === "text")?.text ?? "").trimEnd(); + // Strip the LLM-facing notice (appended by wrappedExecute) before display; + // the styled `warningLine` below carries the same text in ⟨…⟩ form. + const output = stripOutputNotice(rawOutput, details?.meta).trimEnd(); const jsonOutputs = details?.jsonOutputs ?? []; const jsonLines = jsonOutputs.flatMap((value, index) => { diff --git a/packages/coding-agent/src/tools/output-meta.ts b/packages/coding-agent/src/tools/output-meta.ts index 04dca5dff..942328072 100644 --- a/packages/coding-agent/src/tools/output-meta.ts +++ b/packages/coding-agent/src/tools/output-meta.ts @@ -489,6 +489,32 @@ export function formatStyledTruncationWarning(meta: OutputMeta | undefined, them return theme.fg("warning", wrapBrackets(message, theme)); } +/** + * Strip the trailing notice that {@link appendOutputNotice} bakes into the + * LLM-facing content body. Renderers should call this before printing + * `result.content` text in the TUI, because they emit a styled warning line of + * their own; without this, users see the same `[Showing lines …]` string twice + * (once verbatim from the body, once as the styled `⟨…⟩` warning). + * + * Safe to call eagerly: returns the input unchanged when no notice is present + * (e.g. during streaming, before {@link wrappedExecute} runs). + */ +export function stripOutputNotice(text: string, meta: OutputMeta | undefined): string { + const notice = formatOutputNotice(meta); + if (!notice) return text; + // Trim trailing whitespace from `text` and from the notice itself so we + // match regardless of whether: (a) the caller already trimEnd()'d, (b) + // extra blank lines slipped in after the notice (diagnostics blocks add + // `\n\n` between sections, OutputSink may pad), or (c) neither. Returns + // the prefix before the notice so the caller can re-trim as needed. + const trimmedText = text.trimEnd(); + const trimmedNotice = notice.trimEnd(); + if (trimmedText.endsWith(trimmedNotice)) { + return trimmedText.slice(0, -trimmedNotice.length); + } + return text; +} + // ============================================================================= // Tool wrapper // ============================================================================= diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 9f89f611e..687fba964 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -60,6 +60,7 @@ import { formatStyledTruncationWarning, type OutputMeta, resolveOutputMaxColumns, + stripOutputNotice, } from "./output-meta"; import { expandPath, formatPathRelativeToCwd, resolveReadPath, splitPathAndSel } from "./path-utils"; import { formatBytes, replaceTabs, shortenPath, wrapBrackets } from "./render-utils"; @@ -2194,7 +2195,9 @@ export const readToolRenderer = { const rawText = result.content?.find(c => c.type === "text")?.text ?? ""; // Prefer structured `displayContent` from details when available so the TUI // shows clean file content (no model-only hashline anchors) without parsing the formatted text. - const contentText = details?.displayContent?.text ?? rawText; + // Fall back to the raw text, but strip the LLM-facing notice so it doesn't + // echo next to the styled warning line below. + const contentText = details?.displayContent?.text ?? stripOutputNotice(rawText, details?.meta); const imageContent = result.content?.find(c => c.type === "image"); const rawPath = args?.file_path || args?.path || ""; const filePath = shortenPath(rawPath); diff --git a/packages/coding-agent/src/tools/ssh.ts b/packages/coding-agent/src/tools/ssh.ts index 8175c15a1..d4dcfa31e 100644 --- a/packages/coding-agent/src/tools/ssh.ts +++ b/packages/coding-agent/src/tools/ssh.ts @@ -16,7 +16,7 @@ import { executeSSH } from "../ssh/ssh-executor"; import { renderStatusLine } from "../tui"; import { CachedOutputBlock } from "../tui/output-block"; import type { ToolSession } from "."; -import { formatStyledTruncationWarning, type OutputMeta } from "./output-meta"; +import { formatStyledTruncationWarning, type OutputMeta, stripOutputNotice } from "./output-meta"; import { ToolError } from "./tool-errors"; import { toolResult } from "./tool-result"; import { clampTimeout } from "./tool-timeouts"; @@ -253,7 +253,8 @@ export const sshToolRenderer = { render: (width: number): string[] => { // REACTIVE: read mutable options at render time const { expanded, renderContext } = options; - const output = textContent.trimEnd(); + // Strip LLM-facing notice so we don't echo it next to the styled warning. + const output = stripOutputNotice(textContent, details?.meta).trimEnd(); const outputLines: string[] = []; if (output) { diff --git a/packages/coding-agent/test/tools/strip-output-notice.test.ts b/packages/coding-agent/test/tools/strip-output-notice.test.ts new file mode 100644 index 000000000..1a5a03018 --- /dev/null +++ b/packages/coding-agent/test/tools/strip-output-notice.test.ts @@ -0,0 +1,112 @@ +/** + * Round-trip contract between `appendOutputNotice` (via `formatOutputNotice`) + * and `stripOutputNotice`: anything the tool wrapper bakes into the LLM-facing + * content body, the TUI renderer must be able to peel off so the styled + * `⟨…⟩` warning line doesn't double-print next to the verbatim body text. + * + * Regression: bash/eval/ssh/browser/read all printed the same `[Showing …]` + * string twice — once from the body content, once as the styled warning line. + */ +import { describe, expect, it } from "bun:test"; +import { formatOutputNotice, type OutputMeta, stripOutputNotice } from "../../src/tools/output-meta"; + +const truncation: OutputMeta = { + truncation: { + direction: "middle", + truncatedBy: "middle", + totalLines: 8, + totalBytes: 320, + outputLines: 4, + outputBytes: 105, + headRange: { start: 1, end: 2 }, + tailRange: { start: 7, end: 8 }, + elidedLines: 4, + elidedBytes: 215, + }, +}; + +const tailTruncation: OutputMeta = { + truncation: { + direction: "tail", + truncatedBy: "bytes", + totalLines: 100, + totalBytes: 10_000, + outputLines: 40, + outputBytes: 4_000, + maxBytes: 4_000, + shownRange: { start: 61, end: 100 }, + artifactId: "abc123", + }, +}; + +const limitsOnly: OutputMeta = { + limits: { matchLimit: { reached: 50, suggestion: 100 } }, +}; + +describe("stripOutputNotice", () => { + it("removes the exact notice appended by the wrapper for middle elision", () => { + const body = "line1\nline2\n[… 4 lines elided (215B) …]\nline7\nline8"; + const notice = formatOutputNotice(truncation); + const combined = body + notice; + + // Round-trip: the wrapper appends, the renderer peels off exactly. + expect(stripOutputNotice(combined, truncation)).toBe(body); + }); + + it("removes the notice for tail truncation including artifact reference", () => { + const body = "long output…"; + const notice = formatOutputNotice(tailTruncation); + expect(notice).toContain("artifact://abc123"); + + expect(stripOutputNotice(body + notice, tailTruncation)).toBe(body); + }); + + it("removes the notice for limit-only meta (no truncation)", () => { + const body = "results…"; + const notice = formatOutputNotice(limitsOnly); + expect(notice).toContain("matches limit reached"); + + expect(stripOutputNotice(body + notice, limitsOnly)).toBe(body); + }); + + it("matches the trimEnd()'d body the renderer actually sees", () => { + // bash.ts/eval.ts call `.trimEnd()` on the body before passing it in. + // The notice itself ends with `]`, so trimEnd is a no-op on its tail; + // confirm the strip still succeeds when the renderer hands us either + // the trimmed or untrimmed form. + const body = "the output"; + const combined = `${body}${formatOutputNotice(truncation)}\n\n`; + + // renderer trims, then strips + expect(stripOutputNotice(combined.trimEnd(), truncation)).toBe(body); + // renderer strips first + expect(stripOutputNotice(combined, truncation).trimEnd()).toBe(body); + }); + + it("returns input unchanged when meta is undefined", () => { + expect(stripOutputNotice("plain text", undefined)).toBe("plain text"); + }); + + it("returns input unchanged when meta has no notice-emitting fields", () => { + // e.g. meta carries only `source` info; formatOutputNotice yields "". + const sourceOnly: OutputMeta = { source: { type: "path", value: "/tmp/x" } }; + expect(formatOutputNotice(sourceOnly)).toBe(""); + expect(stripOutputNotice("plain text", sourceOnly)).toBe("plain text"); + }); + + it("returns input unchanged when body does not actually carry the notice (streaming case)", () => { + // During streaming, `renderContext.output` is the live sink content + // before wrappedExecute has appended anything. Calling stripOutputNotice + // eagerly must not corrupt that prefix. + const streaming = "partial output so far…"; + expect(stripOutputNotice(streaming, truncation)).toBe(streaming); + }); + + it("only strips the trailing occurrence, not a coincidental earlier match", () => { + const noticeText = formatOutputNotice(truncation); + // The same notice text appearing mid-body (unlikely but possible if the + // command literally printed it) must be preserved when not at the tail. + const body = `prefix${noticeText} middle suffix`; + expect(stripOutputNotice(body, truncation)).toBe(body); + }); +}); From 565a6cf9712d8cd2c37b9adbfda9b02c2b9c5e2d Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 15 May 2026 01:19:48 +0000 Subject: [PATCH 13/22] fix(web-search): thread AbortSignal to fetch() in anthropic, exa, jina, zai, gemini providers - anthropic: add signal to AnthropicSearchParams, callSearch(), and AnthropicProvider.search() - exa: add signal to ExaSearchParams, callExaSearch(), and ExaProvider.search() - jina: add signal to JinaSearchParams, callJinaSearch(), and JinaProvider.search() - zai: add signal to ZaiSearchParams, callZaiTool(), and ZaiProvider.search() - gemini: add signal to GeminiSearchParams, callGeminiSearch() and buildInit() so both fetchWithRetry calls (initial + auth-refresh retry) carry the signal The five providers listed above never forwarded SearchParams.signal to the underlying HTTP layer, so pressing Esc during a web_search call had no effect and the session froze until the request resolved or Ctrl+C was pressed. brave, kimi, perplexity, searxng, tavily, synthetic, codex, kagi, and parallel already thread the signal correctly and are unchanged. Fixes #1044 --- .../coding-agent/src/web/search/providers/anthropic.ts | 5 +++++ packages/coding-agent/src/web/search/providers/exa.ts | 3 +++ packages/coding-agent/src/web/search/providers/gemini.ts | 5 +++++ packages/coding-agent/src/web/search/providers/jina.ts | 7 +++++-- packages/coding-agent/src/web/search/providers/zai.ts | 7 +++++-- 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/src/web/search/providers/anthropic.ts b/packages/coding-agent/src/web/search/providers/anthropic.ts index c24b0e0af..b0aa9dfb3 100644 --- a/packages/coding-agent/src/web/search/providers/anthropic.ts +++ b/packages/coding-agent/src/web/search/providers/anthropic.ts @@ -38,6 +38,7 @@ export interface AnthropicSearchParams { max_tokens?: number; /** Sampling temperature (0–1). Lower = more focused/factual. */ temperature?: number; + signal?: AbortSignal; } /** @@ -86,6 +87,7 @@ async function callSearch( systemPrompt?: string, maxTokens?: number, temperature?: number, + signal?: AbortSignal, ): Promise { const url = buildAnthropicUrl(auth); const headers = buildAnthropicSearchHeaders(auth); @@ -116,6 +118,7 @@ async function callSearch( method: "POST", headers, body: JSON.stringify(body), + signal, }); if (!response.ok) { @@ -253,6 +256,7 @@ export async function searchAnthropic(params: AnthropicSearchParams): Promise>> { @@ -235,6 +236,7 @@ async function callGeminiSearch( maxOutputTokens?: number, temperature?: number, toolParams: GeminiToolParams = {}, + signal?: AbortSignal, ): Promise<{ answer: string; sources: SearchSource[]; @@ -308,6 +310,7 @@ async function callGeminiSearch( ...headers, }, body: JSON.stringify(requestBody), + signal, }); const urlFor = (attempt: number) => `${endpoints[Math.min(attempt, endpoints.length - 1)]}/v1internal:streamGenerateContent?alt=sse`; @@ -500,6 +503,7 @@ export async function searchGemini(params: GeminiSearchParams): Promise { +async function callJinaSearch(apiKey: string, query: string, signal?: AbortSignal): Promise { const requestUrl = `${JINA_SEARCH_URL}/${encodeURIComponent(query)}`; const response = await fetch(requestUrl, { headers: { Accept: "application/json", Authorization: `Bearer ${apiKey}`, }, + signal, }); if (!response.ok) { @@ -58,7 +60,7 @@ export async function searchJina(params: JinaSearchParams): Promise { return findCredential(getEnvApiKey("zai"), "zai"); } -async function callZaiTool(apiKey: string, args: Record): Promise { +async function callZaiTool(apiKey: string, args: Record, signal?: AbortSignal): Promise { const response = await fetch(ZAI_MCP_URL, { method: "POST", headers: { @@ -72,6 +73,7 @@ async function callZaiTool(apiKey: string, args: Record): Promi arguments: args, }, }), + signal, }); if (!response.ok) { @@ -157,7 +159,7 @@ async function callZaiSearch(apiKey: string, params: ZaiSearchParams): Promise Date: Fri, 15 May 2026 03:46:31 +0200 Subject: [PATCH 14/22] style(pi-shell): formatted fixup shell cleanup code and tests for readability - Converted several single-line guard-style `if let`/`let Some` exits in `fixup.rs` into multiline blocks for consistent style. - Reformatted the safe-argument regex definition and normalized test case formatting in `fixup.rs` without altering assertions. - Reordered `lib.rs` module exports by moving `fixup` ahead of `minimizer`. --- crates/pi-shell/src/fixup.rs | 101 ++++++++++++++++++----------------- crates/pi-shell/src/lib.rs | 2 +- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/crates/pi-shell/src/fixup.rs b/crates/pi-shell/src/fixup.rs index ace9f8cf0..86b6974d4 100644 --- a/crates/pi-shell/src/fixup.rs +++ b/crates/pi-shell/src/fixup.rs @@ -8,10 +8,10 @@ //! truncates bash output and exposes the full result via an artifact, so //! the pipe just hides content the agent wanted. //! -//! 2. A redundant trailing `2>&1` on a segment that has no remaining pipe -//! or other redirect. The harness already merges stderr into stdout, so -//! the duplication is purely cosmetic — and often a leftover after fixup -//! (1) drops a downstream pipe. +//! 2. A redundant trailing `2>&1` on a segment that has no remaining pipe or +//! other redirect. The harness already merges stderr into stdout, so the +//! duplication is purely cosmetic — and often a leftover after fixup (1) +//! drops a downstream pipe. //! //! The implementation is AST-driven: `brush-parser` handles tokenization, //! quoting, heredocs, command substitution, and nested compound commands. We @@ -142,7 +142,9 @@ fn try_strip_head_tail( if !is_safe_head_tail(last) { return default; } - let Some(last_loc) = last.location() else { return default }; + let Some(last_loc) = last.location() else { + return default; + }; // Pipeline-internal separators are always `|` or `|&` — never `||`. The // real parser already validated structure, so scanning backwards from the @@ -153,8 +155,12 @@ fn try_strip_head_tail( // (e.g. the synthetic `2>&1` inserted by `|&`). let bytes = cmd.as_bytes(); let last_start = last_loc.start.index; - let Some(head) = cmd.get(..last_start) else { return default }; - let Some(pipe_pos) = head.rfind('|') else { return default }; + let Some(head) = cmd.get(..last_start) else { + return default; + }; + let Some(pipe_pos) = head.rfind('|') else { + return default; + }; // Defense in depth against `||`. if pipe_pos > 0 && bytes[pipe_pos - 1] == b'|' { return default; @@ -198,8 +204,12 @@ fn try_strip_2to1( } let target = &p.seq[outcome.last_idx]; - let Command::Simple(simple) = target else { return }; - let Some(name_word) = simple.word_or_name.as_ref() else { return }; + let Command::Simple(simple) = target else { + return; + }; + let Some(name_word) = simple.word_or_name.as_ref() else { + return; + }; if name_word.value.is_empty() { return; } @@ -210,8 +220,12 @@ fn try_strip_2to1( // The last suffix item must be the `2>&1` redirect, and it must be the // only redirect on the command (no `> file 2>&1` or `2>&1 > file`). - let Some(last_item) = suffix.0.last() else { return }; - let CommandPrefixOrSuffixItem::IoRedirect(io) = last_item else { return }; + let Some(last_item) = suffix.0.last() else { + return; + }; + let CommandPrefixOrSuffixItem::IoRedirect(io) = last_item else { + return; + }; if !is_stderr_to_stdout(io) { return; } @@ -236,7 +250,9 @@ fn try_strip_2to1( // accounted for by the AST; the gap between the anchor and `2>&1` is // guaranteed to be just whitespace by the precondition that `2>&1` is // the last suffix item and no other redirects exist. - let Some(name_loc) = name_word.loc.as_ref() else { return }; + let Some(name_loc) = name_word.loc.as_ref() else { + return; + }; let mut anchor = name_loc.end.index; for item in &suffix.0 { if let Some(loc) = item.location() { @@ -279,8 +295,12 @@ fn is_stderr_to_stdout(io: &IoRedirect) -> bool { } fn is_safe_head_tail(c: &Command) -> bool { - let Command::Simple(simple) = c else { return false }; - let Some(name) = simple.word_or_name.as_ref() else { return false }; + let Command::Simple(simple) = c else { + return false; + }; + let Some(name) = simple.word_or_name.as_ref() else { + return false; + }; if name.value != "head" && name.value != "tail" { return false; } @@ -291,9 +311,13 @@ fn is_safe_head_tail(c: &Command) -> bool { { return false; } - let Some(suffix) = &simple.suffix else { return true }; + let Some(suffix) = &simple.suffix else { + return true; + }; for item in &suffix.0 { - let CommandPrefixOrSuffixItem::Word(w) = item else { return false }; + let CommandPrefixOrSuffixItem::Word(w) = item else { + return false; + }; if !SAFE_ARG_RE.is_match(&w.value) { return false; } @@ -314,8 +338,10 @@ fn is_safe_head_tail(c: &Command) -> bool { /// `--help`, and any filename token are deliberately rejected — they would /// change semantics if their host command were removed. static SAFE_ARG_RE: LazyLock = LazyLock::new(|| { - Regex::new(r"^(?:-[nc]=?\d+|-[nc]|-\d+|-[qv]|--lines(?:=\d+)?|--bytes(?:=\d+)?|--quiet|--verbose|\d+)$") - .expect("static safe-arg regex compiles") + Regex::new( + r"^(?:-[nc]=?\d+|-[nc]|-\d+|-[qv]|--lines(?:=\d+)?|--bytes(?:=\d+)?|--quiet|--verbose|\d+)$", + ) + .expect("static safe-arg regex compiles") }); #[cfg(test)] @@ -359,16 +385,8 @@ mod tests { let cases: &[(&str, &str, &[&str])] = &[ ("cmd 2>&1", "cmd", &["2>&1"]), ("just build 2>&1", "just build", &["2>&1"]), - ( - "just build 2>&1 | tail -3", - "just build", - &["| tail -3", "2>&1"], - ), - ( - "cargo build 2>&1 | head -50", - "cargo build", - &["| head -50", "2>&1"], - ), + ("just build 2>&1 | tail -3", "just build", &["| tail -3", "2>&1"]), + ("cargo build 2>&1 | head -50", "cargo build", &["| head -50", "2>&1"]), ]; for (input, want_cmd, want_stripped) in cases { let (cmd, stripped) = run(input); @@ -385,26 +403,13 @@ mod tests { "just build && just up && sleep 4 && just healthz", &["| tail -3", "2>&1"], ), - ( - "cmd1 | head -5 && cmd2 && cmd3 | tail -3", - "cmd1 && cmd2 && cmd3", - &["| head -5", "| tail -3"], - ), - ( - "echo a; cmd | head -5; echo b", - "echo a; cmd; echo b", - &["| head -5"], - ), - ( - "cmd | head -5 || fallback | tail -3", - "cmd || fallback", - &["| head -5", "| tail -3"], - ), - ( - "cmd1 | head -5 && cmd2 2>&1 | grep err", - "cmd1 && cmd2 2>&1 | grep err", - &["| head -5"], - ), + ("cmd1 | head -5 && cmd2 && cmd3 | tail -3", "cmd1 && cmd2 && cmd3", &[ + "| head -5", + "| tail -3", + ]), + ("echo a; cmd | head -5; echo b", "echo a; cmd; echo b", &["| head -5"]), + ("cmd | head -5 || fallback | tail -3", "cmd || fallback", &["| head -5", "| tail -3"]), + ("cmd1 | head -5 && cmd2 2>&1 | grep err", "cmd1 && cmd2 2>&1 | grep err", &["| head -5"]), ]; for (input, want_cmd, want_stripped) in cases { let (cmd, stripped) = run(input); diff --git a/crates/pi-shell/src/lib.rs b/crates/pi-shell/src/lib.rs index 26a621fd4..5e41db71d 100644 --- a/crates/pi-shell/src/lib.rs +++ b/crates/pi-shell/src/lib.rs @@ -1,6 +1,6 @@ pub mod cancel; -pub mod minimizer; pub mod fixup; +pub mod minimizer; pub mod process; pub mod shell; #[cfg(windows)] From 864f056b4501f4db2f4c3899f6ecf6d145e6cab0 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:01:42 +0200 Subject: [PATCH 15/22] fix(coding-agent/hashline): fixed hashline parser recovery for blank payload lines - Updated `collectPayload` to treat blank lines inside a payload run as empty entries when subsequent payload text is present. - Added lookahead-based end-of-run detection so blank lines before a non-payload operation remain section separators. - Added tests verifying blank-line recovery inside payloads and non-consumption of trailing blanks between hashline sections. --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/hashline/parser.ts | 24 ++++++++++++++++--- .../coding-agent/test/core/hashline.test.ts | 23 ++++++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a29fa0921..68ca658b7 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,6 +24,7 @@ - Fixed summarized `read` output stalling agents on elided regions by appending an explicit footer like `[NN lines across MM elided regions; read :raw or a line range like :1-9999 for verbatim content]`. The footer fires whenever the structural summarizer elided at least one span, so the model gets a concrete recovery selector instead of having to guess from a bare `...` / `{ .. }` marker. Surfaces `elidedLines` on `ReadToolDetails.summary` alongside the existing `elidedSpans`. ([#1046](https://github.com/can1357/oh-my-pi/issues/1046)) - Updated the `read` tool prompt to describe the new elision footer and instruct the model to follow `:raw` (or an explicit line range) when the elided body is actually needed, rather than guessing. - Fixed plugin extensions failing to load when their `peerDependencies` reference internal `pi-*` packages under any scope other than `@mariozechner` (e.g. `Cannot find module '@earendil-works/pi-tui'` from `@juicesharp/rpiv-ask-user-question`, or `Cannot find module '@oh-my-pi/pi-utils'` from `@oh-my-pi/swarm-extension`). The legacy-pi specifier shim now treats `@mariozechner`, `@earendil-works`, **and** the canonical `@oh-my-pi` itself as aliases for the same set of bundled in-process packages (`pi-agent-core`, `pi-ai`, `pi-coding-agent`, `pi-natives`, `pi-tui`, `pi-utils`), and additionally rewrites the upstream-only `pi-ai/oauth` subpath onto our `pi-ai/utils/oauth` layout. Restored the `Key` runtime helper export on `@oh-my-pi/pi-tui` to match upstream — plugins using `Key.enter` / `Key.ctrl("c")` (e.g. `@plannotator/pi-extension`, `@juicesharp/rpiv-ask-user-question`) no longer fail with `Export named 'Key' not found`. End-to-end verified against `@juicesharp/rpiv-ask-user-question`, `@oh-my-pi/swarm-extension`, and `@plannotator/pi-extension` — each now loads cleanly with all of its tools/commands/handlers registered. Plugins importing any of those scopes are remapped to the omp binary's own copy at load time, so peer deps are no longer dragged in from npm and there is exactly one module instance per package regardless of which scope name the plugin's manifest happened to declare. +- Fixed hashline payload parsing to silently treat truly-blank lines as empty `~`-prefixed payload lines when more payload follows in the same run. The previous behavior broke at the blank ("payload line has no preceding +, <, or = operation.") even though the intent is obvious — the only ambiguity is between in-payload blanks and end-of-section blanks, and a one-line lookahead resolves it: blanks that precede a non-payload op still end the run cleanly as section separators. Recovers the common case of forgetting the leading separator on a blank inserted line without changing how trailing blanks between ops behave. ## [15.0.1] - 2026-05-14 ### Breaking Changes diff --git a/packages/coding-agent/src/hashline/parser.ts b/packages/coding-agent/src/hashline/parser.ts index 6e5f6e653..9cd62d8d8 100644 --- a/packages/coding-agent/src/hashline/parser.ts +++ b/packages/coding-agent/src/hashline/parser.ts @@ -86,9 +86,27 @@ function collectPayload( let index = startIndex; while (index < lines.length) { const line = stripTrailingCarriageReturn(lines[index]); - if (!line.startsWith(HL_EDIT_SEP)) break; - payload.push(line.slice(1)); - index++; + if (line.startsWith(HL_EDIT_SEP)) { + payload.push(line.slice(1)); + index++; + continue; + } + // Silently recover from a missing payload prefix on an otherwise blank + // line: if more payload follows (possibly past further blanks), treat + // each intervening blank as an empty `${HL_EDIT_SEP}` payload line. + // Trailing blanks before a non-payload op stay as section separators. + if (line.length === 0) { + let lookahead = index + 1; + while (lookahead < lines.length && stripTrailingCarriageReturn(lines[lookahead]).length === 0) { + lookahead++; + } + if (lookahead < lines.length && stripTrailingCarriageReturn(lines[lookahead]).startsWith(HL_EDIT_SEP)) { + for (let j = index; j < lookahead; j++) payload.push(""); + index = lookahead; + continue; + } + } + break; } if (payload.length === 0 && requirePayload) { throw new Error(`line ${opLineNum}: + and < operations require at least one ${HL_EDIT_SEP}TEXT payload line.`); diff --git a/packages/coding-agent/test/core/hashline.test.ts b/packages/coding-agent/test/core/hashline.test.ts index a0f41b32a..3e2a8b558 100644 --- a/packages/coding-agent/test/core/hashline.test.ts +++ b/packages/coding-agent/test/core/hashline.test.ts @@ -313,6 +313,29 @@ describe("hashline parser — block op syntax", () => { expect(applyDiff(content, diff)).toBe("aaa\n\n# not a header\n+ not an op\n spaced\nccc"); }); + it("treats blank lines inside a payload run as empty payload lines", () => { + // Truly blank lines (no leading separator) inside an active payload run + // are silently rewritten to empty payload lines as long as more payload + // follows. This recovers from a common typo where the model forgets the + // separator on what should be a blank inserted line. + const diff = [`= ${sameLineRange(tag(2, "bbb"))}`, pl("first"), "", "", pl("after")].join("\n"); + expect(applyDiff(content, diff)).toBe("aaa\nfirst\n\n\nafter\nccc"); + }); + + it("does not consume trailing blank lines between sections as payload", () => { + // Blanks that precede a non-payload op (here, another `=`) end the + // payload run cleanly — they're section separators, not payload. + const diff = [ + `= ${sameLineRange(tag(1, "aaa"))}`, + pl("AAA"), + "", + "", + `= ${sameLineRange(tag(3, "ccc"))}`, + pl("CCC"), + ].join("\n"); + expect(applyDiff(content, diff)).toBe("AAA\nbbb\nCCC"); + }); + it("rejects missing payloads and orphan payload lines", () => { expect(() => parseHashline(`+ ${tag(1, "aaa")}`)).toThrow(/require at least one/); expect(() => parseHashline(pl("orphan"))).toThrow(/payload line has no preceding/); From 8108394516458138961df53fc2c6c01a73aad55c Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 15 May 2026 01:59:32 +0000 Subject: [PATCH 16/22] fix(discovery): honor manifest `commands` key for claude plugin slash commands The ClaudePluginManifest interface was missing the `commands` field (the standard Claude plugin format), and resolvePluginDir only checked the legacy `slash-commands` key. Plugins declaring their command path via `"commands": "..."` silently fell back to the hardcoded `/commands/` directory, which doesn't exist for Claude-format plugins, so no slash commands were ever loaded. Changes: - Added `commands?: string` to ClaudePluginManifest. - Changed resolvePluginDir to accept ReadonlyArray and iterate in priority order; the first non-empty match wins. - loadSlashCommands now passes ["commands", "slash-commands"] so the canonical Claude plugin key takes precedence over the legacy one. - Added two regression tests: one covering the `commands` key in isolation, one verifying its precedence over `slash-commands` when both fields are present. Fixes #1076 --- .../src/discovery/claude-plugins.ts | 26 ++++-- .../test/discovery/claude-plugins.test.ts | 79 +++++++++++++++++++ 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/packages/coding-agent/src/discovery/claude-plugins.ts b/packages/coding-agent/src/discovery/claude-plugins.ts index ae2073adb..9e8f9fae6 100644 --- a/packages/coding-agent/src/discovery/claude-plugins.ts +++ b/packages/coding-agent/src/discovery/claude-plugins.ts @@ -31,6 +31,7 @@ const PRIORITY = 70; // Below claude.ts (80) so user .claude/ overrides win interface ClaudePluginManifest { skills?: string; "slash-commands"?: string; + commands?: string; } interface ResolvedPluginDir { @@ -59,24 +60,35 @@ function isWithinPluginRoot(rootPath: string, targetPath: string): boolean { async function resolvePluginDir( root: ClaudePluginRoot, - manifestKey: keyof ClaudePluginManifest, + manifestKeys: ReadonlyArray, fallback: string, ): Promise { const manifest = await readPluginManifest(root); const fallbackDir = path.join(root.path, fallback); - const configured = manifest?.[manifestKey]; - if (typeof configured !== "string" || !configured.trim()) { + + let configured: string | undefined; + let matchedKey: keyof ClaudePluginManifest | undefined; + for (const key of manifestKeys) { + const val = manifest?.[key]; + if (typeof val === "string" && val.trim()) { + configured = val.trim(); + matchedKey = key; + break; + } + } + + if (configured === undefined) { return { dir: fallbackDir }; } - const resolved = path.resolve(root.path, configured.trim()); + const resolved = path.resolve(root.path, configured); if (isWithinPluginRoot(root.path, resolved)) { return { dir: resolved }; } return { dir: fallbackDir, - warning: `[claude-plugins] Ignoring ${String(manifestKey)} path outside plugin root for ${root.id}: ${configured}`, + warning: `[claude-plugins] Ignoring ${String(matchedKey)} path outside plugin root for ${root.id}: ${configured}`, }; } @@ -93,7 +105,7 @@ async function loadSkills(ctx: LoadContext): Promise> { const results = await Promise.all( roots.map(async root => { - const { dir: skillsDir, warning } = await resolvePluginDir(root, "skills", "skills"); + const { dir: skillsDir, warning } = await resolvePluginDir(root, ["skills"], "skills"); const result = await scanSkillsFromDir(ctx, { dir: skillsDir, providerId: PROVIDER_ID, @@ -128,7 +140,7 @@ async function loadSlashCommands(ctx: LoadContext): Promise { - const { dir: commandsDir, warning } = await resolvePluginDir(root, "slash-commands", "commands"); + const { dir: commandsDir, warning } = await resolvePluginDir(root, ["commands", "slash-commands"], "commands"); const result = await loadFilesFromDir(ctx, commandsDir, PROVIDER_ID, root.scope, { extensions: ["md"], transform: (name, content, filePath, source) => { diff --git a/packages/coding-agent/test/discovery/claude-plugins.test.ts b/packages/coding-agent/test/discovery/claude-plugins.test.ts index b923cecbe..0f7e23c24 100644 --- a/packages/coding-agent/test/discovery/claude-plugins.test.ts +++ b/packages/coding-agent/test/discovery/claude-plugins.test.ts @@ -393,6 +393,85 @@ describe("listClaudePluginRoots", () => { expect(found).toBeDefined(); expect(found?.path).toContain(path.join(".claude", "commands", "ship.md")); }); + + test("reads slash commands directory from plugin manifest commands field (standard Claude plugin format)", async () => { + const pluginsDir = path.join(tempDir, ".claude", "plugins"); + const pluginPath = path.join(tempDir, "plugins", "manifest-commands-key"); + await fs.mkdir(path.join(pluginsDir), { recursive: true }); + await fs.mkdir(path.join(pluginPath, ".claude-plugin"), { recursive: true }); + await fs.mkdir(path.join(pluginPath, ".claude", "commands"), { recursive: true }); + + const registry = { + version: 2, + plugins: { + "manifest-commands-key@market": [ + { + scope: "user", + installPath: pluginPath, + version: "1.0.0", + installedAt: "2025-01-01T00:00:00Z", + lastUpdated: "2025-01-01T00:00:00Z", + }, + ], + }, + }; + + await fs.writeFile(path.join(pluginsDir, "installed_plugins.json"), JSON.stringify(registry)); + await fs.writeFile( + path.join(pluginPath, ".claude-plugin", "plugin.json"), + JSON.stringify({ commands: "./.claude/commands" }), + ); + await fs.writeFile(path.join(pluginPath, ".claude", "commands", "plan.md"), "Plan it\n"); + + const result = await loadCapability("slash-commands", { cwd: tempDir }); + expect(result.warnings).toEqual([]); + const found = result.all.find(command => command.name === "manifest-commands-key:plan"); + + expect(found).toBeDefined(); + expect(found?.path).toContain(path.join(".claude", "commands", "plan.md")); + }); + + test("commands field takes precedence over slash-commands field when both are present", async () => { + const pluginsDir = path.join(tempDir, ".claude", "plugins"); + const pluginPath = path.join(tempDir, "plugins", "manifest-commands-precedence"); + await fs.mkdir(path.join(pluginsDir), { recursive: true }); + await fs.mkdir(path.join(pluginPath, ".claude-plugin"), { recursive: true }); + // commands points to .claude/commands, slash-commands points to a different dir + await fs.mkdir(path.join(pluginPath, ".claude", "commands"), { recursive: true }); + await fs.mkdir(path.join(pluginPath, "legacy-commands"), { recursive: true }); + + const registry = { + version: 2, + plugins: { + "manifest-commands-precedence@market": [ + { + scope: "user", + installPath: pluginPath, + version: "1.0.0", + installedAt: "2025-01-01T00:00:00Z", + lastUpdated: "2025-01-01T00:00:00Z", + }, + ], + }, + }; + + await fs.writeFile(path.join(pluginsDir, "installed_plugins.json"), JSON.stringify(registry)); + await fs.writeFile( + path.join(pluginPath, ".claude-plugin", "plugin.json"), + JSON.stringify({ commands: "./.claude/commands", "slash-commands": "./legacy-commands" }), + ); + await fs.writeFile(path.join(pluginPath, ".claude", "commands", "ship.md"), "Ship it\n"); + // This file exists only under the legacy dir — should NOT be found + await fs.writeFile(path.join(pluginPath, "legacy-commands", "old.md"), "Old\n"); + + const result = await loadCapability("slash-commands", { cwd: tempDir }); + expect(result.warnings).toEqual([]); + const found = result.all.find(command => command.name === "manifest-commands-precedence:ship"); + const notFound = result.all.find(command => command.name === "manifest-commands-precedence:old"); + + expect(found).toBeDefined(); + expect(notFound).toBeUndefined(); + }); test("ignores manifest skills directory that resolves outside plugin root", async () => { const pluginsDir = path.join(tempDir, ".claude", "plugins"); const pluginPath = path.join(tempDir, "plugins", "manifest-skills-outside"); From 16542063c4b50c548e0e44ca1632dc6fb0090cc9 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:20:21 +0200 Subject: [PATCH 17/22] docs(tools): added guidance note for eval tool usage in docs - Added an explicit notice in the eval tool docs cautioning against using one-off `-c`/`-e` shell executions. - Documented that the eval tool should be used instead for persistent runtimes, structured outputs, and cancellation or timeout support. --- docs/tools/eval.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/tools/eval.md b/docs/tools/eval.md index 5d62721e2..da9f37f36 100644 --- a/docs/tools/eval.md +++ b/docs/tools/eval.md @@ -2,6 +2,8 @@ > Execute Python or JavaScript code in persistent cell-based runtimes. +> **Notice:** Do not shell out to `python -c`/`python -e`, `bun -e`, or `node -e` via the `bash` tool for ad-hoc code execution. Use this tool instead — it gives you persistent state across cells, structured `display()` output, image/JSON capture, and proper cancellation/timeout handling that one-shot `-e`/`-c` invocations cannot provide. + ## Source - Entry: `packages/coding-agent/src/tools/eval.ts` - Model-facing prompt: `packages/coding-agent/src/prompts/tools/eval.md` From 78b4bbb93b0ee448c7ad052ee9d32b367ef386ac Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:20:30 +0200 Subject: [PATCH 18/22] Rewrite hashline prompt examples to ASCII-only Mr/Mrs/Dr motif MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous examples used " • " in the source file and inserted "·" as the new content. Some agents were copying the middle-dot literally into real edits as if it were format scaffolding, since the demo inserts were near-twins of the existing string. The new example uses TITLE = "Mr" → "Mrs" with "Dr" inserts: ASCII-only, clearly distinct from any payload separator, and obviously domain content rather than punctuation the model could confuse for syntax. Every original op shape is preserved (single-line replace, multiline replace, insert AFTER/BEFORE, append, delete, blank, both anti-patterns). Pure prompt change; parser/schema/runtime untouched. --- packages/coding-agent/CHANGELOG.md | 1 + .../src/prompts/tools/hashline.md | 48 +++++++++---------- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 68ca658b7..b4e0e051f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -25,6 +25,7 @@ - Updated the `read` tool prompt to describe the new elision footer and instruct the model to follow `:raw` (or an explicit line range) when the elided body is actually needed, rather than guessing. - Fixed plugin extensions failing to load when their `peerDependencies` reference internal `pi-*` packages under any scope other than `@mariozechner` (e.g. `Cannot find module '@earendil-works/pi-tui'` from `@juicesharp/rpiv-ask-user-question`, or `Cannot find module '@oh-my-pi/pi-utils'` from `@oh-my-pi/swarm-extension`). The legacy-pi specifier shim now treats `@mariozechner`, `@earendil-works`, **and** the canonical `@oh-my-pi` itself as aliases for the same set of bundled in-process packages (`pi-agent-core`, `pi-ai`, `pi-coding-agent`, `pi-natives`, `pi-tui`, `pi-utils`), and additionally rewrites the upstream-only `pi-ai/oauth` subpath onto our `pi-ai/utils/oauth` layout. Restored the `Key` runtime helper export on `@oh-my-pi/pi-tui` to match upstream — plugins using `Key.enter` / `Key.ctrl("c")` (e.g. `@plannotator/pi-extension`, `@juicesharp/rpiv-ask-user-question`) no longer fail with `Export named 'Key' not found`. End-to-end verified against `@juicesharp/rpiv-ask-user-question`, `@oh-my-pi/swarm-extension`, and `@plannotator/pi-extension` — each now loads cleanly with all of its tools/commands/handlers registered. Plugins importing any of those scopes are remapped to the omp binary's own copy at load time, so peer deps are no longer dragged in from npm and there is exactly one module instance per package regardless of which scope name the plugin's manifest happened to declare. - Fixed hashline payload parsing to silently treat truly-blank lines as empty `~`-prefixed payload lines when more payload follows in the same run. The previous behavior broke at the blank ("payload line has no preceding +, <, or = operation.") even though the intent is obvious — the only ambiguity is between in-payload blanks and end-of-section blanks, and a one-line lookahead resolves it: blanks that precede a non-payload op still end the run cleanly as section separators. Recovers the common case of forgetting the leading separator on a blank inserted line without changing how trailing blanks between ops behave. +- Rewrote the hashline edit prompt examples to use an ASCII-only `TITLE = "Mr"` → `"Mrs"` / `"Dr"` motif instead of the previous `" • "` and `"·"` separators. Some agents had been copying the middle-dot literal characters into real edits as if they were format scaffolding (e.g. emitting payload lines like `~ ·`), since the demo inserts were near-twins of the existing string. The new example keeps every original op shape (single-line replace, multiline replace, insert AFTER/BEFORE, append, delete, blank, plus both anti-patterns) but uses content that is obviously domain-specific and clearly distinct from any payload separator. Pure prompt change; no parser, schema, or runtime behavior is affected. ## [15.0.1] - 2026-05-14 ### Breaking Changes diff --git a/packages/coding-agent/src/prompts/tools/hashline.md b/packages/coding-agent/src/prompts/tools/hashline.md index d3fc02c20..d037183f7 100644 --- a/packages/coding-agent/src/prompts/tools/hashline.md +++ b/packages/coding-agent/src/prompts/tools/hashline.md @@ -66,36 +66,35 @@ When braces bound your edit, you SHOULD prefer these shapes: -{{hline 1 "const DEF = \"guest\";"}} -{{hline 2 "export function label(name) {"}} +{{hline 1 "const TITLE = \"Mr\";"}} +{{hline 2 "export function greet(name) {"}} {{hline 3 "\treturn ["}} -{{hline 4 "\t\tname?.trim() || DEF,"}} -{{hline 5 "\t\t\" • \","}} -{{hline 6 "\t].join(\"\");"}} +{{hline 4 "\t\tTITLE,"}} +{{hline 5 "\t\tname?.trim() || \"guest\","}} +{{hline 6 "\t].join(\" \");"}} {{hline 7 "}"}} # Replace one line (the payload must re-emit the original indentation) @@ mod.ts -= {{hrefr 4}}..{{hrefr 4}} -{{hsep}} name?.trim().toUpperCase() || DEF, += {{hrefr 1}}..{{hrefr 1}} +{{hsep}}const TITLE = "Mrs"; # Replace a full multiline statement (widen to a self-contained boundary) @@ mod.ts = {{hrefr 3}}..{{hrefr 6}} {{hsep}} return [ -{{hsep}} name?.trim() || DEF, -{{hsep}} "·", -{{hsep}} " • ", -{{hsep}} ].join(""); +{{hsep}} "Mrs", +{{hsep}} name?.trim() || "guest", +{{hsep}} ].join(" "); # Insert AFTER/BEFORE a line @@ mod.ts -+ {{hrefr 3}} -{{hsep}} "·", ++ {{hrefr 4}} +{{hsep}} "Dr", < {{hrefr 5}} -{{hsep}} "·", +{{hsep}} "Dr", # Append to file @@ mod.ts @@ -112,13 +111,12 @@ When braces bound your edit, you SHOULD prefer these shapes: -# WRONG — replaces 3 lines just to add one. +# WRONG — replaces 2 lines just to add one. @@ mod.ts -= {{hrefr 1}}..{{hrefr 3}} -{{hsep}}const DEF = "guest"; += {{hrefr 1}}..{{hrefr 2}} +{{hsep}}const TITLE = "Mr"; {{hsep}}const DEBUG = false; -{{hsep}}export function label(name) { -{{hsep}} return [ +{{hsep}}export function greet(name) { # RIGHT — same effect, one-line insert @@ mod.ts + {{hrefr 1}} @@ -127,17 +125,15 @@ When braces bound your edit, you SHOULD prefer these shapes: # WRONG — replace from the middle of a larger statement (error-prone) @@ mod.ts = {{hrefr 4}}..{{hrefr 5}} -{{hsep}} name?.trim() || DEF, -{{hsep}} "·", -{{hsep}} " • ", +{{hsep}} "Dr", +{{hsep}} name?.trim() || "guest", # RIGHT — widen to the full statement @@ mod.ts = {{hrefr 3}}..{{hrefr 6}} {{hsep}} return [ -{{hsep}} name?.trim() || DEF, -{{hsep}} "·", -{{hsep}} " • ", -{{hsep}} ].join(""); +{{hsep}} "Dr", +{{hsep}} name?.trim() || "guest", +{{hsep}} ].join(" "); From 2b9590730a7356a15363002f2f3114bb0f9c360c Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:37:36 +0200 Subject: [PATCH 19/22] build(dockerfile): added Docker build artifacts pipeline image and ignore rules - Added a new `.dockerignore` to keep build context clean by excluding targets, node_modules, logs, IDE files, OS junk, generated outputs, and secrets. - Created a multi-stage Dockerfile that builds Linux `pi_natives.linux-*.node` and `omp-rpc` wheel artifacts in dedicated builder stages. - Published a minimal scratch artifacts stage that only exports the compiled native addon and wheel into `/out/` for downstream image consumption. --- .dockerignore | 63 +++++++++++++++++++++++++++++++ Dockerfile | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 .dockerignore create mode 100644 Dockerfile diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 000000000..b52fc61c3 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,63 @@ +# Heavy build outputs — must never reach the build context. `target/` alone is +# >100 GB on a dev machine. +target/ +node_modules/ +dist/ +runs/ + +# Per-host scratch the pi codebase uses for parallel agents / worktrees. +.fallow/ +.worktrees/ +.wt/ +.opencode/ +.pi_config/ +.omp/plugins/ + +# VCS, editors, IDEs — irrelevant to the build, churn on every IDE keystroke. +.git/ +.npm/ +.vscode/ +.zed/ +.idea/ + +# OS + transient noise. Finder rewrites .DS_Store whenever you peek at a +# folder; profilers drop `CPU.*` blobs at random times. Letting any of these +# into the build context busts BuildKit's content hash and forces a full +# native rebuild for no good reason. +.DS_Store +*.swp +*.swo +*~ +*.tmp + +# Logs + profiling artifacts. +*.log +*.cpuprofile +*.heapprofile +*.heapsnapshot +CPU.* + +# Build / test side outputs. +*.tsbuildinfo +coverage/ +.nyc_output/ +__pycache__/ +compaction-results/ +changes/ + +# Generated files (the in-image build regenerates them). +packages/coding-agent/src/internal-urls/docs-index.generated.ts +packages/natives/native/.build/ +packages/natives/native/pi_natives.darwin-*.node +packages/natives/native/pi_natives.dev.node +packages/ai/test/.temp-images/ +python/omp-rpc/src/omp_rpc.egg-info/ + +# Scratch files the repo creates ad-hoc. +syntax.jsonl +out.jsonl +out.html +pi-*.html + +# Secrets. Should never be in the image regardless. +.env diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 000000000..8634a9a07 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,100 @@ +# syntax=docker/dockerfile:1.7 +############################################################################### +# oh-my-pi — build-artifacts image +# +# Produces, in `/out/`, the cross-host build outputs that downstream consumers +# bake into their runtime images: +# +# - pi_natives.linux-.node — N-API addon compiled from `crates/pi-natives` +# - omp_rpc--py3-none-any.whl — Python RPC wheel from `python/omp-rpc` +# +# This image deliberately has no entrypoint and no apt-installed extras: it is +# meant to be referenced as a `COPY --from=` stage by other Dockerfiles. +# +# Build: +# docker build -t oh-my-pi/artifacts:dev . +# +# Consume from another Dockerfile: +# ARG PI_ARTIFACTS_IMAGE=oh-my-pi/artifacts:dev +# FROM ${PI_ARTIFACTS_IMAGE} AS pi-artifacts +# COPY --from=pi-artifacts /out/pi_natives.linux-*.node /opt/bun/bin/ +# COPY --from=pi-artifacts /out/*.whl /tmp/wheels/ +############################################################################### + +############################ +# 1) natives-builder — Rust + Bun → pi_natives.linux-.node +############################ +FROM rust:1.86-slim-bookworm AS natives-builder + +ARG BUN_VERSION=1.3.14 +ENV BUN_INSTALL=/opt/bun \ + PATH=/opt/bun/bin:/usr/local/cargo/bin:/usr/local/bin:/usr/bin:/bin \ + CARGO_TERM_COLOR=never + +RUN apt-get update \ + && apt-get install -y --no-install-recommends \ + curl ca-certificates pkg-config libssl-dev unzip git \ + && rm -rf /var/lib/apt/lists/* + +RUN curl -fsSL https://bun.sh/install | bash -s "bun-v${BUN_VERSION}" \ + && /opt/bun/bin/bun --version + +WORKDIR /pi + +# ─── Layer 1: workspace manifests + lockfiles only ─────────────────────────── +# Editing source files (under `packages//src/…` or `crates//src/…`) won't +# bust the `bun install` layer below, because none of those globs match. Only +# touching a `package.json`, `Cargo.toml`, or a root lockfile invalidates this +# layer. `--parents` preserves the matched path under /pi/ (BuildKit 1.7+). +COPY --parents \ + package.json bun.lock bunfig.toml \ + tsconfig.base.json tsconfig.json \ + Cargo.toml Cargo.lock rust-toolchain.toml \ + packages/*/package.json \ + packages/tsconfig.workspace.json \ + crates/*/Cargo.toml \ + /pi/ + +# ─── Layer 2: hydrate node_modules from the manifests above ────────────────── +RUN bun install --frozen-lockfile --ignore-scripts + +# ─── Layer 3: full source ──────────────────────────────────────────────────── +# `.dockerignore` keeps `target/`, `node_modules/`, `dist/`, `runs/`, editor / +# OS noise (`.DS_Store`, `CPU.*`, `*.cpuprofile`, …), and pre-built host-only +# natives output out of the build context. node_modules from Layer 2 is +# preserved across this COPY because it's never in the context to begin with. +COPY . /pi/ + +# ─── Layer 4: compile pi-natives to a Linux N-API addon ────────────────────── +# Persistent caches make repeat builds incremental even when the source layer +# invalidates: cargo's package index + git-deps + the workspace's target dir. +RUN --mount=type=cache,target=/root/.cargo/registry \ + --mount=type=cache,target=/root/.cargo/git \ + --mount=type=cache,target=/pi/target \ + set -eux; \ + rustup show; \ + bun --cwd=packages/natives run build; \ + mkdir -p /out; \ + cp packages/natives/native/pi_natives.linux-*.node /out/ + +############################ +# 2) python-builder — omp-rpc wheel +############################ +FROM python:3.12-slim-bookworm AS python-builder + +RUN apt-get update \ + && apt-get install -y --no-install-recommends git \ + && rm -rf /var/lib/apt/lists/* + +RUN pip install --upgrade pip build + +WORKDIR /src +COPY python/omp-rpc /src +RUN python -m build --wheel --outdir /out + +############################ +# 3) artifacts — final image, nothing but the two outputs. +############################ +FROM scratch AS artifacts +COPY --from=natives-builder /out/pi_natives.linux-*.node /out/ +COPY --from=python-builder /out/*.whl /out/ From 4c494aaa3ab50a22f711869ad24060443be7d15e Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:38:14 +0200 Subject: [PATCH 20/22] feat(coding-agent/tools): defaulted GitHub search repo scope to the current checkout - Added a `resolveSearchRepoScope` helper that uses an explicit `repo` when provided, skips defaulting when a query already contains a repo/org/user/owner scope qualifier, and otherwise resolves the current checkout via `resolveDefaultRepoMemoized`. - Updated `search_issues`, `search_prs`, `search_code`, and `search_commits` to use the resolver before composing API queries, defaulting `repo` when omitted but silently falling back to an unscoped search on resolution failure. - Documented the new search-repo defaulting rules in tool prompts, user docs, and the package changelog. --- docs/tools/github.md | 10 ++- packages/coding-agent/CHANGELOG.md | 2 + .../coding-agent/src/prompts/tools/github.md | 8 +- packages/coding-agent/src/tools/gh.ts | 41 +++++++++- packages/coding-agent/test/tools/gh.test.ts | 74 +++++++++++++++++++ 5 files changed, 126 insertions(+), 9 deletions(-) diff --git a/docs/tools/github.md b/docs/tools/github.md index 7cf753f4b..0ac6a3f1f 100644 --- a/docs/tools/github.md +++ b/docs/tools/github.md @@ -18,7 +18,7 @@ | Field | Type | Required | Description | | --- | --- | --- | --- | | `op` | `"repo_view" \| "pr_create" \| "pr_checkout" \| "pr_push" \| "search_issues" \| "search_prs" \| "search_code" \| "search_commits" \| "search_repos" \| "run_watch"` | Yes | Dispatch selector. `GithubTool.execute()` switches only on this field. | -| `repo` | `string` | No | `owner/repo` override. Ignored when the identifier argument is already a full GitHub URL. Required in practice when `gh` cannot infer repo context from the current checkout. | +| `repo` | `string` | No | `owner/repo` override. Ignored when the identifier argument is already a full GitHub URL. For `search_issues`/`search_prs`/`search_code`/`search_commits`, defaults to the current checkout's `owner/repo` when omitted (skipped when the query already contains a `repo:`/`org:`/`user:`/`owner:` qualifier or when current-repo resolution fails). Required in practice when `gh` cannot infer repo context from the current checkout. | | `branch` | `string` | No | Used by `repo_view`, `pr_push`, and `run_watch`. `run_watch` falls back to current git branch when `run` is omitted; `pr_push` falls back to current branch. | | `pr` | `string \| string[]` | No | Used by `pr_checkout`. Each item may be a PR number, branch name, or GitHub PR URL. Array form enables batching. Omitted means current branch PR. | | `force` | `boolean` | No | Used only by `pr_checkout`. Defaults to `false`; allows resetting an existing `pr-` local branch to the PR head commit. | @@ -148,6 +148,8 @@ Push target resolution reads the `branch..ompPrHeadRef`, `pushRemote`/`rem | Batching | None | | Output | `# GitHub issues search`, echoed query, optional repo, result count, then one bullet per issue with repo/state/author/labels/timestamps/URL. | +`repo` defaults to the current checkout's `owner/repo` via `resolveSearchRepoScope()` when omitted. The default is suppressed when the query already contains a leading `repo:`/`org:`/`user:`/`owner:` qualifier or when `gh repo view` fails to resolve the current checkout (e.g. outside a github remote). + ### `search_prs` | Aspect | Value | @@ -158,6 +160,8 @@ Push target resolution reads the `branch..ompPrHeadRef`, `pushRemote`/`rem | Batching | None | | Output | Same shape as `search_issues`, labeled as pull requests. | +`repo` defaults to the current checkout's `owner/repo` as in `search_issues`. + ### `search_code` | Aspect | Value | @@ -168,6 +172,8 @@ Push target resolution reads the `branch..ompPrHeadRef`, `pushRemote`/`rem | Batching | None | | Output | `# GitHub code search`, result count, then one bullet per match with path, repo, short commit SHA, URL, and first normalized text-match fragment line when present. | +`repo` defaults to the current checkout's `owner/repo` as in `search_issues`. + ### `search_commits` | Aspect | Value | @@ -178,6 +184,8 @@ Push target resolution reads the `branch..ompPrHeadRef`, `pushRemote`/`rem | Batching | None | | Output | `# GitHub commits search`, result count, then one bullet per commit: short SHA + first commit-message line, repo, author, date, URL. | +`repo` defaults to the current checkout's `owner/repo` as in `search_issues`. + ### `search_repos` | Aspect | Value | diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b4e0e051f..e60f6d8bc 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,8 @@ ### Changed +- Changed the `github` tool's search ops (`search_issues`, `search_prs`, `search_code`, `search_commits`) to default the `repo` scope to the current checkout's `owner/repo` when `repo` is omitted. The auto-scope is skipped when the query already carries an explicit `repo:`/`org:`/`user:`/`owner:` qualifier or when `gh repo view` cannot resolve a github remote (in which case the search proceeds across all of GitHub as before). `search_repos` is unchanged — repository-scoping there must live in the query. + - Changed bash command preprocessing to strip trailing `| head` and `| tail` pipelines (including `|&`) from each top-level segment in command chains separated by `;`, `&&`, `||`, or `&` - Changed bash fixup notices to state that stderr is already merged into stdout and to reflect that fixes were applied for multiple stripped segments when several transforms fire - Changed shell-minimizer per-line truncation marker from a bare `…` to `…[+N]`, where `N` is the count of dropped Unicode scalars. The bracketed tally disambiguates minimizer-driven cuts from genuine `…` characters in the source (paths, JSON, stack traces, etc.) and gives the agent an exact count so it can decide whether the missing tail is recoverable inline or warrants reading the `[raw output: artifact://]` footer the bash wrapper already emits when the minimizer rewrites output. Affects pipeline Stage 5 (`truncate_lines_at` in `defs/*.toml`) and the internal callers in `filters/git.rs`, `filters/listing.rs`, and `filters/lint.rs`. ([#1046](https://github.com/can1357/oh-my-pi/issues/1046)) diff --git a/packages/coding-agent/src/prompts/tools/github.md b/packages/coding-agent/src/prompts/tools/github.md index 021f71b6a..e873ca83f 100644 --- a/packages/coding-agent/src/prompts/tools/github.md +++ b/packages/coding-agent/src/prompts/tools/github.md @@ -6,10 +6,10 @@ Pick the operation via `op`. Each op uses a subset of the parameters: - `pr_create` — Create a pull request. Either provide `title` (and optional `body`) or set `fill: true` to auto-fill from commits. Optional `base` (target, defaults to repo default), `head` (source, defaults to current branch), `draft`, `repo`, `reviewer[]`, `assignee[]`, `label[]`. Returns the new PR URL plus a summary. - `pr_checkout` — Check one or more pull requests out into dedicated git worktrees. Optional `pr` (number, URL, branch, or array of any of those — pass an array to batch-check-out multiple PRs in one call), `repo`, `force` (reset existing local branch). - `pr_push` — Push a checked-out PR branch back to its source branch. Requires the branch to have been checked out via `op: pr_checkout` (carries push metadata). Optional `branch`; defaults to the current checked-out git branch. Optional `forceWithLease`. -- `search_issues` — Search issues using normal GitHub issue search syntax. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`, `dateField`. -- `search_prs` — Search pull requests using normal GitHub PR search syntax. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`, `dateField`. -- `search_code` — Search code with GitHub code search syntax. Required `query`. Optional `repo`, `limit`. Returns matching paths with surrounding fragments. Date filtering (`since`/`until`) is **not** supported by GitHub code search. -- `search_commits` — Search commits across GitHub. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`. `dateField` is ignored — always uses `committer-date`. +- `search_issues` — Search issues using normal GitHub issue search syntax. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`, `dateField`. Defaults `repo` to the current checkout's `owner/repo` when omitted; pass an explicit `repo:`/`org:`/`user:` qualifier in `query` to search outside it. +- `search_prs` — Search pull requests using normal GitHub PR search syntax. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`, `dateField`. Defaults `repo` to the current checkout's `owner/repo` when omitted; pass an explicit `repo:`/`org:`/`user:` qualifier in `query` to search outside it. +- `search_code` — Search code with GitHub code search syntax. Required `query`. Optional `repo`, `limit`. Returns matching paths with surrounding fragments. Defaults `repo` to the current checkout's `owner/repo` when omitted; pass an explicit `repo:`/`org:`/`user:` qualifier in `query` to search outside it. Date filtering (`since`/`until`) is **not** supported by GitHub code search. +- `search_commits` — Search commits across GitHub. Optional `query` (required unless `since`/`until` is set), `repo`, `limit`, `since`, `until`. `dateField` is ignored — always uses `committer-date`. Defaults `repo` to the current checkout's `owner/repo` when omitted; pass an explicit `repo:`/`org:`/`user:` qualifier in `query` to search outside it. - `search_repos` — Search repositories across GitHub. Optional `query` (required unless `since`/`until` is set), `limit`, `since`, `until`, `dateField` (use query qualifiers like `org:`, `language:` instead of `repo`). - Date filter format for `since` / `until`: relative duration `` (`m`/`h`/`d`/`w`/`mo`/`y`, e.g. `3d`, `12h`, `2w`), an ISO date `YYYY-MM-DD`, or an ISO datetime. Translated to a single GitHub-search qualifier (`created:≥…`, `created:≤…`, or `created:since..until`). `dateField: "updated"` maps to `updated:` for issues/prs and `pushed:` for repos. When you only want a date filter and no keywords, omit `query` entirely. - `run_watch` — Watch a GitHub Actions workflow run. Optional `run` (id or URL). Omitting `run` watches all workflow runs for the current HEAD commit; `branch` falls back to the current branch. Optional `tail` (log lines per failed job). Streams snapshots, fast-fails on the first detected job failure (with a brief grace period to capture concurrent failures), then fetches tailed logs for the failed jobs. The full failed-job logs are saved as a session artifact for on-demand reads. diff --git a/packages/coding-agent/src/tools/gh.ts b/packages/coding-agent/src/tools/gh.ts index b6f587a61..c16e03f56 100644 --- a/packages/coding-agent/src/tools/gh.ts +++ b/packages/coding-agent/src/tools/gh.ts @@ -1774,6 +1774,39 @@ export async function resolveDefaultRepoMemoized(cwd: string, signal?: AbortSign return untilAborted(signal, pending); } +/** + * Matches search-query qualifiers that already scope to a repository, org, or + * user. When present, callers should avoid layering a default `repo:` + * on top — the user has already expressed an explicit scope. + * + * Only the leading `repo:`/`org:`/`user:`/`owner:` token is treated as a + * scope marker; arbitrary substrings (e.g. inside quoted text) are ignored. + */ +const REPO_SCOPE_QUALIFIER_PATTERN = /(?:^|\s)-?(?:repo|org|user|owner):\S/i; + +/** + * Resolve the effective `repo:` scope for a search op. Returns the explicit + * `repo` when set, `undefined` when the query already carries a scoping + * qualifier, and otherwise the current checkout's `owner/repo` via + * `resolveDefaultRepoMemoized`. Resolution failures (no git/gh context, no + * configured remote) silently fall back to `undefined` so the search proceeds + * across all of GitHub instead of throwing. + */ +async function resolveSearchRepoScope( + cwd: string, + repo: string | undefined, + query: string | undefined, + signal: AbortSignal | undefined, +): Promise { + if (repo) return repo; + if (query && REPO_SCOPE_QUALIFIER_PATTERN.test(query)) return undefined; + try { + return await resolveDefaultRepoMemoized(cwd, signal); + } catch { + return undefined; + } +} + async function resolveGitHubBranchHead( cwd: string, repo: string, @@ -3267,11 +3300,11 @@ async function executeSearchIssues( params: GithubInput, signal: AbortSignal | undefined, ): Promise> { - const repo = normalizeOptionalString(params.repo); const limit = resolveSearchLimit(params.limit); const dateField = resolveSearchDateField("issues", params.dateField); const dateQualifier = buildSearchDateQualifier(dateField, params.since, params.until); const displayQuery = composeSearchQuery([params.query, dateQualifier]); + const repo = await resolveSearchRepoScope(session.cwd, normalizeOptionalString(params.repo), displayQuery, signal); const apiQuery = composeSearchQuery([displayQuery, repo ? `repo:${repo}` : undefined, "is:issue"]); const args = buildGhApiSearchArgs("issues", apiQuery, limit); @@ -3285,11 +3318,11 @@ async function executeSearchPrs( params: GithubInput, signal: AbortSignal | undefined, ): Promise> { - const repo = normalizeOptionalString(params.repo); const limit = resolveSearchLimit(params.limit); const dateField = resolveSearchDateField("prs", params.dateField); const dateQualifier = buildSearchDateQualifier(dateField, params.since, params.until); const displayQuery = composeSearchQuery([params.query, dateQualifier]); + const repo = await resolveSearchRepoScope(session.cwd, normalizeOptionalString(params.repo), displayQuery, signal); const apiQuery = composeSearchQuery([displayQuery, repo ? `repo:${repo}` : undefined, "is:pr"]); const args = buildGhApiSearchArgs("issues", apiQuery, limit); @@ -3307,8 +3340,8 @@ async function executeSearchCode( if (params.since !== undefined || params.until !== undefined) { throw new ToolError("search_code does not support since/until; GitHub code search has no date qualifier."); } - const repo = normalizeOptionalString(params.repo); const limit = resolveSearchLimit(params.limit); + const repo = await resolveSearchRepoScope(session.cwd, normalizeOptionalString(params.repo), query, signal); const apiQuery = composeSearchQuery([query, repo ? `repo:${repo}` : undefined]); const args = buildGhApiSearchArgs("code", apiQuery, limit, ["Accept: application/vnd.github.text-match+json"]); @@ -3322,11 +3355,11 @@ async function executeSearchCommits( params: GithubInput, signal: AbortSignal | undefined, ): Promise> { - const repo = normalizeOptionalString(params.repo); const limit = resolveSearchLimit(params.limit); const dateField = resolveSearchDateField("commits", params.dateField); const dateQualifier = buildSearchDateQualifier(dateField, params.since, params.until); const displayQuery = composeSearchQuery([params.query, dateQualifier]); + const repo = await resolveSearchRepoScope(session.cwd, normalizeOptionalString(params.repo), displayQuery, signal); const apiQuery = composeSearchQuery([displayQuery, repo ? `repo:${repo}` : undefined]); const args = buildGhApiSearchArgs("commits", apiQuery, limit); diff --git a/packages/coding-agent/test/tools/gh.test.ts b/packages/coding-agent/test/tools/gh.test.ts index a311e05f3..24e6d8d5c 100644 --- a/packages/coding-agent/test/tools/gh.test.ts +++ b/packages/coding-agent/test/tools/gh.test.ts @@ -604,6 +604,80 @@ describe("github tool", () => { expect(reposArgs.some(arg => typeof arg === "string" && arg.includes("repo:ignored/value"))).toBe(false); }); + it("search_prs: defaults `repo:` to the current checkout when `repo` is omitted", async () => { + const textSpy = vi.spyOn(git.github, "text").mockResolvedValue("acme/widgets\n"); + const jsonSpy = vi.spyOn(git.github, "json").mockResolvedValue({ items: [] }); + const tool = new GithubTool(createSession("/tmp/gh-default-prs")); + await tool.execute("search-prs", { + op: "search_prs", + query: "is:open", + limit: 1, + }); + + // `gh repo view --json nameWithOwner` runs against the session cwd to fetch the + // default scope; the resolved owner/repo gets layered onto the API query. + expect(textSpy).toHaveBeenCalled(); + const repoViewArgs = textSpy.mock.calls[0]?.[1] ?? []; + expect(repoViewArgs.slice(0, 2)).toEqual(["repo", "view"]); + expect(repoViewArgs).toContain("nameWithOwner"); + + const apiArgs = jsonSpy.mock.calls[0]?.[1] ?? []; + expect(apiArgs).toContain("q=is:open repo:acme/widgets is:pr"); + }); + + it("search_issues: skips the current-repo default when the query already carries a scope qualifier", async () => { + const textSpy = vi.spyOn(git.github, "text").mockResolvedValue("acme/widgets\n"); + const jsonSpy = vi.spyOn(git.github, "json").mockResolvedValue({ items: [] }); + const tool = new GithubTool(createSession("/tmp/gh-default-skip-qualifier")); + await tool.execute("search-issues", { + op: "search_issues", + query: "is:open org:torvalds", + limit: 1, + }); + + // Explicit `org:` qualifier suppresses the auto-resolved `repo:` injection. + expect(textSpy).not.toHaveBeenCalled(); + const apiArgs = jsonSpy.mock.calls[0]?.[1] ?? []; + expect(apiArgs).toContain("q=is:open org:torvalds is:issue"); + expect(apiArgs.some(a => typeof a === "string" && a.startsWith("q=") && a.includes("repo:acme/widgets"))).toBe( + false, + ); + }); + + it("search_code: falls back to global search when `gh repo view` cannot resolve the current checkout", async () => { + const textSpy = vi.spyOn(git.github, "text").mockRejectedValue(new Error("not a git repository")); + const jsonSpy = vi.spyOn(git.github, "json").mockResolvedValue({ items: [] }); + const tool = new GithubTool(createSession("/tmp/gh-default-no-remote")); + await tool.execute("search-code", { + op: "search_code", + query: "findThing", + limit: 1, + }); + + expect(textSpy).toHaveBeenCalled(); + const apiArgs = jsonSpy.mock.calls[0]?.[1] ?? []; + // No `repo:` should be injected — resolution failed, so the search proceeds globally. + expect(apiArgs).toContain("q=findThing"); + expect(apiArgs.some(a => typeof a === "string" && a.startsWith("q=") && a.includes("repo:"))).toBe(false); + }); + + it("search_commits: honors an explicit `repo` override over the current-checkout default", async () => { + const textSpy = vi.spyOn(git.github, "text").mockResolvedValue("acme/widgets\n"); + const jsonSpy = vi.spyOn(git.github, "json").mockResolvedValue({ items: [] }); + const tool = new GithubTool(createSession("/tmp/gh-default-explicit-override")); + await tool.execute("search-commits", { + op: "search_commits", + query: "fix", + repo: "other/project", + limit: 1, + }); + + // Explicit `repo` short-circuits resolution — no `gh repo view` invocation. + expect(textSpy).not.toHaveBeenCalled(); + const apiArgs = jsonSpy.mock.calls[0]?.[1] ?? []; + expect(apiArgs).toContain("q=fix repo:other/project"); + }); + it("checks out a pull request into a worktree and configures contributor push metadata", async () => { const fixture = await createPrFixture(); const tempHome = await setupTempHome(); From 995ba8e51e48b9e081ca7b6c4abf14b2957c81d7 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:52:05 +0200 Subject: [PATCH 21/22] fix(coding-agent/tools): suppressed repeated bash fixup notices per session - Tracked emission state so the bash fixup notice is shown at most once per BashTool instance. --- packages/coding-agent/src/tools/bash.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index f2a73306f..075a1b442 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -246,6 +246,7 @@ export class BashTool implements AgentTool { readonly #asyncEnabled: boolean; readonly #autoBackgroundEnabled: boolean; readonly #autoBackgroundThresholdMs: number; + #bashFixupNoticeEmitted = false; constructor(private readonly session: ToolSession) { this.#asyncEnabled = this.session.settings.get("async.enabled"); @@ -574,8 +575,11 @@ export class BashTool implements AgentTool { const pendingNotices: string[] = []; const timeoutClampNotice = formatTimeoutClampNotice(requestedTimeoutSec, timeoutSec); if (timeoutClampNotice) pendingNotices.push(timeoutClampNotice); - const bashFixupNotice = formatBashFixupNotice(bashFixups); - if (bashFixupNotice) pendingNotices.push(bashFixupNotice); + const bashFixupNotice = this.#bashFixupNoticeEmitted ? undefined : formatBashFixupNotice(bashFixups); + if (bashFixupNotice) { + pendingNotices.push(bashFixupNotice); + this.#bashFixupNoticeEmitted = true; + } if (asyncRequested) { if (!AsyncJobManager.instance()) { From 5de25bf51b3a770b141a7622afa64bb1841f71b2 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 04:56:22 +0200 Subject: [PATCH 22/22] build(dockerfile): updated syntax directive to dockerfile 1.7-labs --- Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 8634a9a07..222184503 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -# syntax=docker/dockerfile:1.7 +# syntax=docker/dockerfile:1.7-labs ############################################################################### # oh-my-pi — build-artifacts image # @@ -45,7 +45,7 @@ WORKDIR /pi # Editing source files (under `packages//src/…` or `crates//src/…`) won't # bust the `bun install` layer below, because none of those globs match. Only # touching a `package.json`, `Cargo.toml`, or a root lockfile invalidates this -# layer. `--parents` preserves the matched path under /pi/ (BuildKit 1.7+). +# layer. `--parents` preserves the matched path under /pi/ (dockerfile 1.7-labs). COPY --parents \ package.json bun.lock bunfig.toml \ tsconfig.base.json tsconfig.json \