merge: fix Bash tool bridge deadlock
This commit is contained in:
@@ -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.
|
||||
@@ -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),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1416,7 +1416,7 @@ export async function runFinalAsarExtensionProof(): Promise<PiReleaseExtensionPr
|
||||
providerRequests: requestCounts,
|
||||
subagentStatus: 'complete',
|
||||
subagentSummary: 'REAL_CHILD_COMPLETE',
|
||||
materializedExtension: 'makelore-runtime-v4.mjs',
|
||||
materializedExtension: 'makelore-runtime-v5.mjs',
|
||||
providerFirstEventDelayMs: PROOF_PROVIDER_FIRST_EVENT_DELAY_MS,
|
||||
managedTurns,
|
||||
managedWorkerMilestones: composition.telemetry,
|
||||
|
||||
@@ -46,6 +46,7 @@ function configureNetwork(settingsManager) {
|
||||
|
||||
const SERVER_CHANNEL = '@makelore/server';
|
||||
const PROTOCOL_VERSION = 1;
|
||||
const PROJECT_WRITE_LEASE_TOOL_NAMES = new Set(['bash', 'edit', 'write']);
|
||||
const threads = new Map();
|
||||
const openingThreads = new Set();
|
||||
let shuttingDown = false;
|
||||
@@ -106,6 +107,14 @@ function referencedEnvironmentNames(value) {
|
||||
return names;
|
||||
}
|
||||
|
||||
function serializeProjectWriteLeaseTools(session) {
|
||||
session.agent.state.tools = session.agent.state.tools.map((tool) => (
|
||||
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',
|
||||
|
||||
@@ -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<Record<string, unknown>>;
|
||||
close(): Promise<void>;
|
||||
}> {
|
||||
const requests: Array<Record<string, unknown>> = [];
|
||||
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<string, unknown>;
|
||||
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<string, unknown>, 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<void>((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<void>((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<void>((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<Record<string, unknown>> }>({
|
||||
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);
|
||||
|
||||
@@ -22,6 +22,7 @@ type ExtensionHandler = (...arguments_: unknown[]) => Promise<unknown> | unknown
|
||||
type ExtensionTool = {
|
||||
name: string;
|
||||
parameters?: Record<string, unknown>;
|
||||
executionMode?: 'sequential';
|
||||
execute?: (...arguments_: unknown[]) => Promise<unknown>;
|
||||
};
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user