From 5ecd041bfedc64331867bf9feabfeca46489fbba Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 30 Apr 2026 05:51:01 +0200 Subject: [PATCH] fix: patched bash interceptor, LSP shutdown, and concurrent command tracking - Fixed bash interceptor to check both raw and cwd-normalized commands, catching commands hidden behind leading `cd ... &&` wrappers. - Fixed LSP client shutdown to await graceful shutdown with a 5s timeout before killing the process, and parallelized `shutdownAll` via `Promise.allSettled`. - Fixed concurrent bash command tracking by replacing a single abort controller with a Set, preventing premature cancellation of parallel commands. - Removed `./hooks` and `./hooks/*` export entries from the coding-agent package exports map. - Updated pinned Rust nightly toolchain from `nightly-2026-03-27` to `nightly-2026-04-29` in `rust-toolchain.toml` and CI workflow. - Replaced custom already-published detection in `ci-release-publish.ts` with `bun publish --tolerate-republish` flag. --- .github/workflows/ci.yml | 13 ++- packages/coding-agent/CHANGELOG.md | 11 +++ packages/coding-agent/package.json | 9 +- packages/coding-agent/src/lsp/client.ts | 62 ++++++------- .../coding-agent/src/session/agent-session.ts | 86 ++++++++++++------- packages/coding-agent/src/tools/bash.ts | 13 ++- .../test/tools/bash-interceptor.test.ts | 58 +++++++++++++ rust-toolchain.toml | 2 +- scripts/ci-release-publish.ts | 17 +--- scripts/sync-versions.ts | 8 +- 10 files changed, 175 insertions(+), 104 deletions(-) create mode 100644 packages/coding-agent/test/tools/bash-interceptor.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b8844126..2e64682db 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,9 +55,9 @@ jobs: - uses: actions/checkout@v4 with: lfs: true - - uses: dtolnay/rust-toolchain@nightly # TODO: unpin once nightly codegen regression is fixed + - uses: dtolnay/rust-toolchain@nightly with: - toolchain: nightly-2026-03-27 + toolchain: nightly-2026-04-29 components: ${{ matrix.rust_checks && 'clippy, rustfmt' || '' }} targets: ${{ matrix.target }} - name: Ensure cross-compilation target is installed @@ -110,9 +110,9 @@ jobs: - uses: oven-sh/setup-bun@v2 with: bun-version: "1.3" - - uses: dtolnay/rust-toolchain@nightly # TODO: unpin once nightly codegen regression is fixed + - uses: dtolnay/rust-toolchain@nightly with: - toolchain: nightly-2026-03-27 + toolchain: nightly-2026-04-29 - uses: Swatinem/rust-cache@v2 with: cache-workspace-crates: true @@ -149,9 +149,9 @@ jobs: - uses: oven-sh/setup-bun@v2 with: bun-version: "1.3" - - uses: dtolnay/rust-toolchain@nightly # TODO: unpin once nightly codegen regression is fixed + - uses: dtolnay/rust-toolchain@nightly with: - toolchain: nightly-2026-03-27 + toolchain: nightly-2026-04-29 - uses: Swatinem/rust-cache@v2 with: cache-workspace-crates: true @@ -251,7 +251,6 @@ jobs: runs-on: ubuntu-22.04 permissions: contents: write - id-token: write steps: - uses: actions/checkout@v4 with: diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a0101b0e9..019eff99f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## [Unreleased] + ### Breaking Changes - Removed the `worktree` parameter from `github` `pr_checkout`. Worktrees are now always written to `~/.omp/wt//pr-/`, derived from the primary repository path @@ -19,6 +20,16 @@ - Changed `pr_checkout` to run `gh pr view` calls in parallel for batch invocations while serializing the in-repo git mutations to keep the operation race-free - Changed `pr_checkout` to auto-derive the worktree location and local branch name (see Breaking Changes), removing the per-call overrides that previously let callers pin a worktree path or local branch +### Removed + +- Removed the `./hooks` and `./hooks/*` package export entries + +### Fixed + +- Fixed bash interceptor rules to also check the original command before `cd` normalization, so leading `cd ... &&` wrappers no longer bypass interception +- Fixed LSP client shutdown to properly await the language server's exit instead of fire-and-forget, preventing premature process termination on SIGINT and SIGTERM +- Fixed concurrent bash commands being tracked independently so aborting one no longer silently drops tracking of others + ## [14.5.9] - 2026-04-30 ### Added diff --git a/packages/coding-agent/package.json b/packages/coding-agent/package.json index 74f662a56..4687204f2 100644 --- a/packages/coding-agent/package.json +++ b/packages/coding-agent/package.json @@ -516,14 +516,7 @@ "types": "./src/web/search/providers/*.ts", "import": "./src/web/search/providers/*.ts" }, - "./hooks": { - "types": "./src/extensibility/hooks/index.ts", - "import": "./src/extensibility/hooks/index.ts" - }, - "./hooks/*": { - "types": "./src/extensibility/hooks/*.ts", - "import": "./src/extensibility/hooks/*.ts" - }, + "./*.js": "./src/*.ts" } } diff --git a/packages/coding-agent/src/lsp/client.ts b/packages/coding-agent/src/lsp/client.ts index 2d5ce74ed..1afb911e5 100644 --- a/packages/coding-agent/src/lsp/client.ts +++ b/packages/coding-agent/src/lsp/client.ts @@ -47,7 +47,7 @@ function startIdleChecker(): void { const now = Date.now(); for (const [key, client] of Array.from(clients.entries())) { if (now - client.lastActivity > idleTimeoutMs) { - shutdownClient(key); + void shutdownClient(key); } } }, IDLE_CHECK_INTERVAL_MS); @@ -762,22 +762,25 @@ export async function refreshFile(client: LspClient, filePath: string, signal?: /** * Shutdown a specific client by key. */ -export function shutdownClient(key: string): void { - const client = clients.get(key); - if (!client) return; - - // Reject all pending requests +async function shutdownClientInstance(client: LspClient): Promise { + const err = new Error("LSP client shutdown"); for (const pending of Array.from(client.pendingRequests.values())) { - pending.reject(new Error("LSP client shutdown")); + pending.reject(err); } client.pendingRequests.clear(); - // Send shutdown request (best effort, don't wait) - sendRequest(client, "shutdown", null).catch(() => {}); - - // Kill process + const timeout = Bun.sleep(5_000); + const shutdown = sendRequest(client, "shutdown", null).catch(() => {}); + await Promise.race([shutdown, timeout]); client.proc.kill(); + await Promise.race([client.proc.exited.catch(() => {}), Bun.sleep(1_000)]); +} + +export async function shutdownClient(key: string): Promise { + const client = clients.get(key); + if (!client) return; clients.delete(key); + await shutdownClientInstance(client); } // ============================================================================= @@ -890,27 +893,10 @@ export async function sendNotification(client: LspClient, method: string, params /** * Shutdown all LSP clients. */ -export function shutdownAll(): void { +export async function shutdownAll(): Promise { const clientsToShutdown = Array.from(clients.values()); clients.clear(); - - const err = new Error("LSP client shutdown"); - for (const client of clientsToShutdown) { - /// Reject all pending requests - const reqs = Array.from(client.pendingRequests.values()); - client.pendingRequests.clear(); - for (const pending of reqs) { - pending.reject(err); - } - - void (async () => { - // Send shutdown request (best effort, don't wait) - const timeout = Bun.sleep(5_000); - const result = sendRequest(client, "shutdown", null).catch(() => {}); - await Promise.race([result, timeout]); - client.proc.kill(); - })().catch(() => {}); - } + await Promise.allSettled(clientsToShutdown.map(client => shutdownClientInstance(client))); } /** Status of an LSP server */ @@ -938,13 +924,19 @@ export function getActiveClients(): LspServerStatus[] { // Register cleanup on module unload if (typeof process !== "undefined") { - process.on("beforeExit", shutdownAll); + process.on("beforeExit", () => { + void shutdownAll(); + }); process.on("SIGINT", () => { - shutdownAll(); - process.exit(0); + void (async () => { + await shutdownAll(); + process.exit(0); + })(); }); process.on("SIGTERM", () => { - shutdownAll(); - process.exit(0); + void (async () => { + await shutdownAll(); + process.exit(0); + })(); }); } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index afb557dc1..e2ef3301b 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -472,7 +472,7 @@ export class AgentSession { #toolChoiceQueue = new ToolChoiceQueue(); // Bash execution state - #bashAbortController: AbortController | undefined = undefined; + #bashAbortControllers = new Set(); #pendingBashMessages: BashExecutionMessage[] = []; // Python execution state @@ -530,8 +530,7 @@ export class AgentSession { #ttsrRetryToken = 0; #ttsrResumePromise: Promise | undefined = undefined; #ttsrResumeResolve: (() => void) | undefined = undefined; - #postPromptTaskCounter = 0; - #postPromptTaskIds = new Set(); + #postPromptTasks = new Set>(); #postPromptTasksPromise: Promise | undefined = undefined; #postPromptTasksResolve: (() => void) | undefined = undefined; #postPromptTasksAbortController = new AbortController(); @@ -1149,14 +1148,13 @@ export class AgentSession { } #trackPostPromptTask(task: Promise): void { - const taskId = ++this.#postPromptTaskCounter; - this.#postPromptTaskIds.add(taskId); + this.#postPromptTasks.add(task); this.#ensurePostPromptTasksPromise(); void task .catch(() => {}) .finally(() => { - this.#postPromptTaskIds.delete(taskId); - if (this.#postPromptTaskIds.size === 0) { + this.#postPromptTasks.delete(task); + if (this.#postPromptTasks.size === 0) { this.#resolvePostPromptTasks(); } }); @@ -1240,11 +1238,21 @@ export class AgentSession { ); } - #cancelPostPromptTasks(): void { + async #cancelPostPromptTasks(): Promise { this.#postPromptTasksAbortController.abort(); this.#postPromptTasksAbortController = new AbortController(); - this.#postPromptTaskIds.clear(); - this.#resolvePostPromptTasks(); + this.#resolveTtsrResume(); + + const pendingTasks = Array.from(this.#postPromptTasks); + if (pendingTasks.length === 0) { + this.#resolvePostPromptTasks(); + return; + } + + await Promise.allSettled(pendingTasks); + if (this.#postPromptTasks.size === 0) { + this.#resolvePostPromptTasks(); + } } /** * Wait for retry, TTSR resume, and any background continuation to settle. @@ -1942,7 +1950,7 @@ export class AgentSession { } catch (error) { logger.warn("Failed to emit session_shutdown event", { error: String(error) }); } - this.#cancelPostPromptTasks(); + await this.#cancelPostPromptTasks(); this.#clearTodoClearTimers(); const drained = await this.#asyncJobManager?.dispose({ timeoutMs: 3_000 }); const deliveryState = this.#asyncJobManager?.getDeliveryState(); @@ -3417,9 +3425,13 @@ export class AgentSession { this.abortRetry(); this.#promptGeneration++; this.#scheduledHiddenNextTurnGeneration = undefined; - this.#resolveTtsrResume(); - this.#cancelPostPromptTasks(); + this.abortCompaction(); + this.abortHandoff(); + this.abortBash(); + this.abortPython(); + const postPromptDrain = this.#cancelPostPromptTasks(); this.agent.abort(); + await postPromptDrain; await this.agent.waitForIdle(); // Clear prompt-in-flight state: waitForIdle resolves when the agent loop's finally // block runs, but nested prompt setup/finalizers may still be unwinding. Without this, @@ -3937,9 +3949,13 @@ export class AgentSession { * @param options Optional callbacks for completion/error handling */ async compact(customInstructions?: string, options?: CompactOptions): Promise { + if (this.#compactionAbortController) { + throw new Error("Compaction already in progress"); + } this.#disconnectFromAgent(); await this.abort(); - this.#compactionAbortController = new AbortController(); + const compactionAbortController = new AbortController(); + this.#compactionAbortController = compactionAbortController; try { if (!this.model) { @@ -3977,7 +3993,7 @@ export class AgentSession { preparation, branchEntries: pathEntries, customInstructions, - signal: this.#compactionAbortController.signal, + signal: compactionAbortController.signal, })) as SessionBeforeCompactResult | undefined; if (result?.cancel) { @@ -4024,7 +4040,7 @@ export class AgentSession { compactionModel, apiKey, customInstructions, - this.#compactionAbortController.signal, + compactionAbortController.signal, { promptOverride: hookPrompt, extraContext: hookContext, remoteInstructions: this.#baseSystemPrompt }, ); summary = result.summary; @@ -4035,7 +4051,7 @@ export class AgentSession { preserveData = { ...(preserveData ?? {}), ...(result.preserveData ?? {}) }; } - if (this.#compactionAbortController.signal.aborted) { + if (compactionAbortController.signal.aborted) { throw new Error("Compaction cancelled"); } @@ -4082,7 +4098,9 @@ export class AgentSession { options?.onError?.(err); throw error; } finally { - this.#compactionAbortController = undefined; + if (this.#compactionAbortController === compactionAbortController) { + this.#compactionAbortController = undefined; + } this.#reconnectToAgent(); } } @@ -5669,15 +5687,16 @@ export class AgentSession { this.agent.replaceMessages(messages.slice(0, -1)); } - // Wait with exponential backoff (abortable) - // Properly abort and null existing controller before replacing - if (this.#retryAbortController) { - this.#retryAbortController.abort(); - } - this.#retryAbortController = new AbortController(); + // Wait with exponential backoff (abortable). + const retryAbortController = new AbortController(); + this.#retryAbortController?.abort(); + this.#retryAbortController = retryAbortController; try { - await abortableSleep(delayMs, this.#retryAbortController.signal); + await abortableSleep(delayMs, retryAbortController.signal); } catch { + if (this.#retryAbortController !== retryAbortController) { + return false; + } // Aborted during sleep - emit end event so UI can clean up const attempt = this.#retryAttempt; this.#retryAttempt = 0; @@ -5691,7 +5710,9 @@ export class AgentSession { this.#resolveRetry(); return false; } - this.#retryAbortController = undefined; + if (this.#retryAbortController === retryAbortController) { + this.#retryAbortController = undefined; + } // Retry via continue() outside the agent_end event callback chain. this.#scheduleAgentContinue({ delayMs: 1, generation }); @@ -5783,12 +5804,13 @@ export class AgentSession { } } - this.#bashAbortController = new AbortController(); + const abortController = new AbortController(); + this.#bashAbortControllers.add(abortController); try { const result = await executeBashCommand(command, { onChunk, - signal: this.#bashAbortController.signal, + signal: abortController.signal, sessionKey: this.sessionId, timeout: clampTimeout("bash") * 1000, onMinimizedSave: originalText => this.#saveBashOriginalArtifact(originalText), @@ -5797,7 +5819,7 @@ export class AgentSession { this.recordBashResult(command, result, options); return result; } finally { - this.#bashAbortController = undefined; + this.#bashAbortControllers.delete(abortController); } } @@ -5836,12 +5858,14 @@ export class AgentSession { * Cancel running bash command. */ abortBash(): void { - this.#bashAbortController?.abort(); + for (const abortController of this.#bashAbortControllers) { + abortController.abort(); + } } /** Whether a bash command is currently running */ get isBashRunning(): boolean { - return this.#bashAbortController !== undefined; + return this.#bashAbortControllers.size > 0; } /** Whether there are pending bash messages waiting to be flushed */ diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 44f757d72..9130c7d58 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -508,12 +508,17 @@ export class BashTool implements AgentTool { const headLines = head; const tailLines = tail; - // Check interception if enabled and available tools are known + // Check both the original command and the cwd-normalized command so + // leading `cd ... &&` wrappers do not hide either shell-navigation rules + // or the dedicated-tool command that follows the directory change. if (this.session.settings.get("bashInterceptor.enabled")) { const rules = this.session.settings.getBashInterceptorRules(); - const interception = checkBashInterception(command, ctx?.toolNames ?? [], rules); - if (interception.block) { - throw new ToolError(interception.message ?? "Command blocked"); + const commandsToCheck = rawCommand === command ? [command] : [rawCommand, command]; + for (const commandToCheck of commandsToCheck) { + const interception = checkBashInterception(commandToCheck, ctx?.toolNames ?? [], rules); + if (interception.block) { + throw new ToolError(interception.message ?? "Command blocked"); + } } } diff --git a/packages/coding-agent/test/tools/bash-interceptor.test.ts b/packages/coding-agent/test/tools/bash-interceptor.test.ts new file mode 100644 index 000000000..48d2aab12 --- /dev/null +++ b/packages/coding-agent/test/tools/bash-interceptor.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "bun:test"; +import type { AgentToolContext } from "@oh-my-pi/pi-agent-core"; +import type { BashInterceptorRule } from "../../src/config/settings-schema"; +import type { ToolSession } from "../../src/tools"; +import { BashTool } from "../../src/tools/bash"; + +function createBashTool(rules: BashInterceptorRule[]): BashTool { + const session = { + settings: { + get(key: string) { + if (key === "bashInterceptor.enabled") return true; + if (key === "async.enabled") return false; + if (key === "bash.autoBackground.enabled") return false; + if (key === "bash.autoBackground.thresholdMs") return 60_000; + return undefined; + }, + getBashInterceptorRules() { + return rules; + }, + }, + } as unknown as ToolSession; + + return new BashTool(session); +} + +describe("BashTool interception", () => { + it("checks the original command before leading cd normalization", async () => { + const tool = createBashTool([ + { + pattern: "^\\s*cd\\s+", + tool: "bash", + message: "Do not hide directory changes in the command string.", + }, + ]); + + await expect( + tool.execute("tool-call", { command: "cd packages/coding-agent && echo ok" }, undefined, undefined, { + toolNames: ["bash"], + } as AgentToolContext), + ).rejects.toThrow("Do not hide directory changes"); + }); + + it("checks the cwd-normalized command after leading cd normalization", async () => { + const tool = createBashTool([ + { + pattern: "^\\s*cat\\s+", + tool: "read", + message: "Use read instead.", + }, + ]); + + await expect( + tool.execute("tool-call", { command: "cd packages/coding-agent && cat package.json" }, undefined, undefined, { + toolNames: ["read"], + } as AgentToolContext), + ).rejects.toThrow("Use read instead"); + }); +}); diff --git a/rust-toolchain.toml b/rust-toolchain.toml index dd8d2c8b2..302c0cd54 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "nightly-2026-03-27" +channel = "nightly-2026-04-29" components = ["rustfmt", "clippy", "rust-analyzer"] targets = ["x86_64-unknown-linux-gnu", "x86_64-pc-windows-msvc"] diff --git a/scripts/ci-release-publish.ts b/scripts/ci-release-publish.ts index 7a884d7ed..7981b769e 100644 --- a/scripts/ci-release-publish.ts +++ b/scripts/ci-release-publish.ts @@ -22,15 +22,7 @@ const packageDirs: PublishPackage[] = [ { dir: "packages/agent" }, { dir: "packages/coding-agent" }, ]; -const alreadyPublishedPatterns = [ - "previously published", - "cannot publish over", - "You cannot publish over", -]; -function isAlreadyPublished(output: string): boolean { - return alreadyPublishedPatterns.some((pattern) => output.includes(pattern)); -} async function readPackageJson(packageDir: string): Promise { return (await Bun.file(path.join(repoRoot, packageDir, "package.json")).json()) as PackageJson; @@ -45,22 +37,19 @@ async function publishPackage(pkg: PublishPackage): Promise { } if (isDryRun) { - console.log(`DRY RUN bun publish --access public (${pkg.dir})`); + console.log(`DRY RUN bun publish --access public --tolerate-republish (${pkg.dir})`); return; } console.log(`Publishing ${packageName}...`); - const result = await $`bun publish --access public`.cwd(path.join(repoRoot, pkg.dir)).quiet().nothrow(); + const result = await $`bun publish --access public --tolerate-republish`.cwd(path.join(repoRoot, pkg.dir)).quiet().nothrow(); const output = `${result.stdout.toString()}${result.stderr.toString()}`.trim(); if (result.exitCode === 0) { if (output) console.log(output); return; } if (output) console.log(output); - if (isAlreadyPublished(output)) { - console.log("Already published, skipping"); - return; - } + process.exit(result.exitCode ?? 1); } diff --git a/scripts/sync-versions.ts b/scripts/sync-versions.ts index 25df41c57..89be40166 100755 --- a/scripts/sync-versions.ts +++ b/scripts/sync-versions.ts @@ -50,10 +50,10 @@ for (const [name, version] of Object.entries(versionMap).sort()) { const versions = new Set(Object.values(versionMap)); if (versions.size > 1) { console.error("\n❌ ERROR: Not all packages have the same version!"); - console.error("Expected lockstep versioning. Run one of:"); - console.error(" npm run version:patch"); - console.error(" npm run version:minor"); - console.error(" npm run version:major"); + console.error("Expected lockstep versioning. Run the release script with the next version:"); + console.error(" bun scripts/release.ts "); + console.error("Or update all package versions consistently before running this script."); + process.exit(1); }