diff --git a/.project-docs/30-worklog/tasks/20260902-fix-bash-tool-bridge-9c4e7a12.md b/.project-docs/30-worklog/tasks/20260902-fix-bash-tool-bridge-9c4e7a12.md new file mode 100644 index 0000000..0128b53 --- /dev/null +++ b/.project-docs/30-worklog/tasks/20260902-fix-bash-tool-bridge-9c4e7a12.md @@ -0,0 +1,84 @@ +# Task: Fix Bash tool execution bridge + +## Identity + +- Task ID: 20260902-fix-bash-tool-bridge-9c4e7a12 +- Mode: Feature +- Branch: codex/20260902-fix-bash-tool-bridge-9c4e7a12-fix-bash-tool-bridge +- Worktree: /Users/inmanx/Documents/makelore-fix-bash-tool-bridge-9c4e7a12 +- Base commit: 6073bd6f4ce269b067b13e1eed0371b38dc2c054 +- Owner: codex +- Status: Completed + +## Scope + +- Reproduce and fix the Makelore-managed Pi tool-batch deadlock that delays + parallel Bash calls until the five-minute HTTP idle timeout. +- Preserve project-scoped write-lease isolation while making Bash/edit/write + tools execute sequentially inside one assistant tool batch. +- Add real Agent Server coverage for two Bash calls, including a command-level + timeout, and verify the same resource path used by packaged builds. + +## Intent And Constraints + +- Keep Pi `0.84.2` as the sole runtime and keep Renderer isolated from Pi wire. +- Do not replay an accepted mutation or weaken the cross-Conversation/project + write lease; only remove the same-batch pre-execution self-deadlock. +- Retain parallel execution for batches containing only read-only tools where + Pi supports it. +- Treat packaging as a verification path, not as a substitute for the source + correction. + +## Outcome + +- Reproduced the stall with the real Pi Agent Server process. Pi `0.84.2` + prepares every tool in a parallel batch before it executes any of them, while + Makelore acquires the project write lease in `tool_call` and releases it in + `tool_result`. Two Bash calls in one assistant response therefore self-deadlocked: + the first held the lease without starting and the second waited for that lease. +- Confirmed that the apparent five-minute Bash hang came from the Agent Server + HTTP idle timeout around the managed-extension request, not from a missing + shell dependency or the Bash command timeout. Packaging alone would retain the + same behavior because it ships this Agent Server resource and extension bundle. +- Marked built-in `bash`, `edit`, and `write` tools as sequential whenever an + Agent thread is rebound. Marked dynamically declared product tools that require + the same project write lease as sequential in the generated Makelore extension. + Read-only tool batches keep Pi's parallel behavior. +- Advanced the managed extension artifact from v4 to v5 and synchronized the + release proof and worker-opening expectations. +- Added a real-process regression that emits two Bash calls in one assistant + response. The first exercises its explicit command timeout and the second must + still run successfully without waiting for the HTTP idle timeout. + +## Verification + +- Pre-fix regression: the new real-process test failed with the second Bash result + equal to `fetch failed`, reproducing the production failure under a 250 ms HTTP + idle timeout. +- Focused regression: `pnpm exec vitest run + tests/unit/pi-agent-server-process-real.test.ts + tests/unit/pi-extension-bundle.test.ts + tests/unit/pi-managed-worker-opener.test.ts --maxWorkers=2` passed 3 files and + 9 tests. +- `pnpm run typecheck` passed. +- `pnpm test` passed 213 files and 1,733 tests; 3 tests were skipped. +- `pnpm run lint:check` passed with 0 errors and 5 existing warnings in + `src/pages/Home/index.tsx` and `src/pages/Makelore/index.tsx`. +- `pnpm run build:vite` passed for Renderer, Electron Main, Preload, and the + release utility worker, proving the fixed Agent Server resource is included by + the production build path. +- `git diff --check` passed. + +## Follow-ups + +- Integrate this feature branch into local `main`, update the canonical current + state/README, restart Makelore, and use the real-process regression as the + non-destructive smoke check instead of replaying the user's accepted turn. + +## Promotion Candidates + +- Promote the write-lease batch rule to README/current state: write-leased Pi + tools are serialized within one assistant batch, while read-only tools may + remain parallel. +- Record the full-suite, production-build, and real-process regression results in + the evidence index during integration. diff --git a/electron/coding-runtime/pi/extensions/makelore-runtime.ts b/electron/coding-runtime/pi/extensions/makelore-runtime.ts index 25718df..d8c554e 100644 --- a/electron/coding-runtime/pi/extensions/makelore-runtime.ts +++ b/electron/coding-runtime/pi/extensions/makelore-runtime.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import { atomicWriteText } from '../../../coding-projects/atomic-json'; -export const MAKELORE_PI_EXTENSION_VERSION = 4; +export const MAKELORE_PI_EXTENSION_VERSION = 5; export const MAKELORE_PI_EXTENSION_FILENAME = `makelore-runtime-v${MAKELORE_PI_EXTENSION_VERSION}.mjs`; const BUNDLE_SOURCE = String.raw` @@ -172,12 +172,13 @@ export function createMakeloreRuntime(runtimeDefaults = {}) { return response.result; } - function registerProductTool(name, label, description, parameters) { + function registerProductTool(name, label, description, parameters, projectWriteLease = false) { pi.registerTool({ name, label, description, parameters, + ...(projectWriteLease ? { executionMode: 'sequential' } : {}), async execute(toolCallId, params, signal) { return await invokeProduct(toolCallId, name, params, signal); }, @@ -219,6 +220,7 @@ export function createMakeloreRuntime(runtimeDefaults = {}) { declaration.label, declaration.description, declaration.inputSchema, + dynamicLeaseTools.has(declaration.name), ); } } diff --git a/electron/coding-runtime/pi/release-proof.ts b/electron/coding-runtime/pi/release-proof.ts index e2e7117..cfbbc15 100644 --- a/electron/coding-runtime/pi/release-proof.ts +++ b/electron/coding-runtime/pi/release-proof.ts @@ -1416,7 +1416,7 @@ export async function runFinalAsarExtensionProof(): Promise ( + PROJECT_WRITE_LEASE_TOOL_NAMES.has(tool.name) && tool.executionMode !== 'sequential' + ? { ...tool, executionMode: 'sequential' } + : tool + )); +} + async function credentialStoreFor(options, providerId) { const models = JSON.parse(await readFile(path.join(options.configDir, 'models.json'), 'utf8')); const provider = record(record(models)?.providers)?.[providerId]; @@ -368,6 +377,12 @@ class AgentThread { async rebind() { const session = this.runtime.session; + // Pi prepares every tool_call hook before starting a parallel tool batch. + // Makelore acquires its project write lease in that hook and releases it in + // tool_result, so two parallel mutation tools would otherwise wait on each + // other before either command can start. Mark only the built-in mutation + // tools sequential; read-only batches retain Pi's parallel execution. + serializeProjectWriteLeaseTools(session); await session.bindExtensions({ uiContext: this.createExtensionUiContext(), mode: 'rpc', diff --git a/tests/unit/pi-agent-server-process-real.test.ts b/tests/unit/pi-agent-server-process-real.test.ts index 7cbc584..4e076bc 100644 --- a/tests/unit/pi-agent-server-process-real.test.ts +++ b/tests/unit/pi-agent-server-process-real.test.ts @@ -6,6 +6,7 @@ import { tmpdir } from 'node:os'; import path from 'node:path'; import { afterEach, describe, expect, it } from 'vitest'; import { PiAgentServerProcess } from '../../electron/coding-runtime/pi/agent-server-process'; +import { PiManagedExtensionHost } from '../../electron/coding-runtime/pi/extension-host'; import { materializeMakelorePiExtension } from '../../electron/coding-runtime/pi/extensions/makelore-runtime'; import { MAKELORE_DEFAULT_LANGUAGE_PROMPT } from '../../electron/coding-runtime/pi/resource-loader'; import type { PiRpcEvent } from '../../electron/coding-runtime/pi/rpc-client'; @@ -81,11 +82,207 @@ async function startHeldProvider(): Promise<{ }; } +async function startBashBatchProvider(): Promise<{ + baseUrl: string; + requests: Array>; + close(): Promise; +}> { + const requests: Array> = []; + const provider = createServer((request, response) => { + const chunks: Buffer[] = []; + request.on('data', (chunk: Buffer) => chunks.push(chunk)); + request.on('end', () => { + const body = JSON.parse(Buffer.concat(chunks).toString('utf8')) as Record; + requests.push(body); + const messages = Array.isArray(body.messages) ? body.messages : []; + const hasToolResult = messages.some((message) => ( + message && typeof message === 'object' && !Array.isArray(message) + && (message as { role?: unknown }).role === 'tool' + )); + response.writeHead(200, { 'content-type': 'text/event-stream' }); + const writeChunk = (delta: Record, finishReason: string | null) => { + response.write(`data: ${JSON.stringify({ + id: 'chatcmpl-bash-batch-test', + object: 'chat.completion.chunk', + created: Math.floor(Date.now() / 1_000), + model: 'test-model', + choices: [{ index: 0, delta, finish_reason: finishReason }], + ...(finishReason ? { usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } } : {}), + })}\n\n`); + }; + if (!hasToolResult) { + writeChunk({ + role: 'assistant', + tool_calls: [ + { + index: 0, + id: 'bash-timeout', + type: 'function', + function: { + name: 'bash', + arguments: JSON.stringify({ command: 'sleep 1', timeout: 0.05 }), + }, + }, + { + index: 1, + id: 'bash-success', + type: 'function', + function: { + name: 'bash', + arguments: JSON.stringify({ command: 'printf SECOND_OK' }), + }, + }, + ], + }, null); + writeChunk({}, 'tool_calls'); + } else { + writeChunk({ role: 'assistant', content: 'BASH_BATCH_DONE' }, null); + writeChunk({}, 'stop'); + } + response.end('data: [DONE]\n\n'); + }); + }); + await new Promise((resolve, reject) => { + provider.once('error', reject); + provider.listen(0, '127.0.0.1', resolve); + }); + const address = provider.address(); + if (!address || typeof address === 'string') throw new Error('Test Provider did not bind'); + return { + baseUrl: `http://127.0.0.1:${address.port}/v1`, + requests, + close: async () => { + await new Promise((resolve, reject) => { + provider.close((error) => error ? reject(error) : resolve()); + provider.closeIdleConnections?.(); + }); + }, + }; +} + afterEach(async () => { await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); }); describe('Pi Agent Server real process', () => { + it('executes two write-leased Bash calls without waiting for the HTTP idle timeout', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'makelore-pi-agent-server-bash-batch-')); + roots.push(root); + const projectPath = path.join(root, 'project'); + const configDir = path.join(root, 'config'); + const sessionDir = path.join(root, 'sessions'); + const extensionDir = path.join(root, 'extensions'); + await Promise.all([ + mkdir(projectPath, { recursive: true }), + mkdir(configDir, { recursive: true }), + mkdir(sessionDir, { recursive: true }), + mkdir(extensionDir, { recursive: true }), + ]); + const provider = await startBashBatchProvider(); + await Promise.all([ + writeFile(path.join(configDir, 'settings.json'), JSON.stringify({ httpIdleTimeoutMs: 250 })), + writeFile(path.join(configDir, 'models.json'), JSON.stringify({ + providers: { + 'makelore-test': { + baseUrl: provider.baseUrl, + api: 'openai-completions', + apiKey: '$MAKELORE_TEST_KEY', + models: [{ + id: 'test-model', + name: 'Test model', + reasoning: false, + input: ['text'], + contextWindow: 32_000, + maxTokens: 4_096, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + }], + }, + }, + })), + ]); + const promptPath = path.join(root, 'system.md'); + const languagePromptPath = path.join(root, 'language.md'); + await Promise.all([ + writeFile(promptPath, 'Use the provided Bash calls.'), + writeFile(languagePromptPath, MAKELORE_DEFAULT_LANGUAGE_PROMPT), + ]); + const extensionHost = new PiManagedExtensionHost(); + const registration = await extensionHost.registerWorker({ + conversationId: 'bash-batch', + generation: 1, + projectId: 'project-a', + projectPath, + extensionsDir: extensionDir, + }); + const runtimeRoot = path.resolve('node_modules/@earendil-works/pi-coding-agent'); + const server = new PiAgentServerProcess({ + executablePath: process.execPath, + serverPath: path.resolve('resources/pi-agent-server.mjs'), + runtimeRoot, + configDir, + }); + const worker = server.createWorker({ + executablePath: process.execPath, + cliPath: path.join(runtimeRoot, 'dist', 'cli.js'), + cwd: projectPath, + configDir, + sessionDir, + conversationId: 'bash-batch', + workerGeneration: 1, + tools: ['bash'], + additionalArgs: [ + '--provider', 'makelore-test', + '--model', 'test-model', + '--thinking', 'off', + '--system-prompt', promptPath, + '--append-system-prompt', languagePromptPath, + '--extension', registration.extensionPath, + '--session-id', 'session-bash-batch', + ], + env: { MAKELORE_TEST_KEY: 'secret-bash-batch', ...registration.env }, + sensitiveValues: ['secret-bash-batch', ...registration.sensitiveValues], + }); + try { + await worker.start(); + await extensionHost.bindRun('bash-batch', 1, 'run-bash-batch'); + const settled = new Promise((resolve) => { + const unsubscribe = worker.subscribe((event) => { + if (event.type !== 'agent_settled') return; + unsubscribe(); + resolve(); + }); + }); + const startedAt = Date.now(); + await worker.request({ type: 'prompt', message: 'RUN_BASH_BATCH' }); + await settled; + expect(Date.now() - startedAt).toBeLessThan(2_000); + const messagesResponse = await worker.request<{ messages: Array> }>({ + type: 'get_messages', + }); + const toolResults = messagesResponse.data?.messages.filter(({ role }) => role === 'toolResult') ?? []; + const textFor = (toolCallId: string) => { + const message = toolResults.find((candidate) => candidate.toolCallId === toolCallId); + const content = Array.isArray(message?.content) ? message.content : []; + return content.flatMap((item) => ( + item && typeof item === 'object' && !Array.isArray(item) + && typeof (item as { text?: unknown }).text === 'string' + ? [(item as { text: string }).text] + : [] + )).join('\n'); + }; + expect(textFor('bash-timeout')).toContain('Command timed out after 0.05 seconds'); + expect(textFor('bash-success')).toContain('SECOND_OK'); + expect(JSON.stringify(toolResults)).not.toContain('fetch failed'); + expect(provider.requests).toHaveLength(2); + } finally { + await worker.stop('test_injection').catch(() => undefined); + await server.stop().catch(() => undefined); + await registration.dispose().catch(() => undefined); + await extensionHost.close().catch(() => undefined); + await provider.close().catch(() => undefined); + } + }, 10_000); + it('hosts isolated Conversation threads in one long-lived process', async () => { const root = await mkdtemp(path.join(tmpdir(), 'makelore-pi-agent-server-')); roots.push(root); diff --git a/tests/unit/pi-extension-bundle.test.ts b/tests/unit/pi-extension-bundle.test.ts index 8a8655d..2e6faaf 100644 --- a/tests/unit/pi-extension-bundle.test.ts +++ b/tests/unit/pi-extension-bundle.test.ts @@ -22,6 +22,7 @@ type ExtensionHandler = (...arguments_: unknown[]) => Promise | unknown type ExtensionTool = { name: string; parameters?: Record; + executionMode?: 'sequential'; execute?: (...arguments_: unknown[]) => Promise; }; @@ -213,6 +214,7 @@ describe('Makelore Pi extension bundle', () => { expect(tools.has('data_service_inspect')).toBe(true); expect(tools.has('data_service_put_document')).toBe(false); expect(tools.get('data_service_inspect')?.parameters).toEqual(pluginTool.inputSchema); + expect(tools.get('data_service_inspect')?.executionMode).toBe('sequential'); const context = JSON.parse(await readFile(registration.env.MAKELORE_PI_CONTEXT_FILE as string, 'utf8')); expect(context).toMatchObject({ catalogRevision: 19, diff --git a/tests/unit/pi-managed-worker-opener.test.ts b/tests/unit/pi-managed-worker-opener.test.ts index 211699d..029ddbd 100644 --- a/tests/unit/pi-managed-worker-opener.test.ts +++ b/tests/unit/pi-managed-worker-opener.test.ts @@ -227,7 +227,7 @@ describe('managed Pi worker opener', () => { expect(argv).toContain('grilling'); expect(argv).toContain('--session-id'); expect(argv).toContain('--extension'); - expect(argv).toContain('makelore-runtime-v4.mjs'); + expect(argv).toContain('makelore-runtime-v5.mjs'); expect(options.additionalArgs?.filter((argument) => argument === '--extension')).toHaveLength(1); expect(argv).not.toContain('PRIVATE MANAGED PROMPT'); expect(argv).not.toContain('provider-secret-value');