diff --git a/CHANGELOG.md b/CHANGELOG.md index 2540ec8..ca51080 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ ## Unreleased +### Features and behavior changes + +- Give writable Pi nodes built-in shell tools for dependency installation, builds, + tests, and repair. Shell calls participate in cancellation and the workspace + write barrier before checkpointing; prompts describe workspace boundaries and + shared resources. Read-only nodes still have no shell, and parent extension/MCP + tools are not inherited. +- Checkpoint tracked changes and non-ignored new files using normal Git staging + semantics. Ignored dependencies, caches, and build outputs are no longer + automatically archived or passed to successors; already tracked and deliberately + force-added files remain tracked. + +These changes require the next minor release under the compatibility policy. +See [migration guidance](docs/compatibility.md#upcoming-workspace-capability-changes) +for callers relying on shell-free workers or preservation of ignored outputs. + ## 0.2.1 — 2026-10-04 ### Maintenance diff --git a/README.md b/README.md index 1cdf40b..a650df0 100644 --- a/README.md +++ b/README.md @@ -416,10 +416,11 @@ provides `request.merge.sources` and `request.merge.finish(dispositions)` to merge agents. Adapters must enforce workspace capabilities and wrap mutating file tools in `request.withWorkspaceWrite(operation)`, so cleanup waits for in-flight writes and rejects later writes. Core Git mutations use this barrier. -The included Pi adapter provides guarded `write`/`edit` alongside its read tools. +The included Pi adapter provides guarded `write`/`edit` and Pi shell tools alongside its read tools. For read-only workspaces, adapters must omit mutating tools; the core write -barrier also rejects writes. This includes all nodes outside Git. Pi never provides -`bash`, `powershell`, or a test runner to nodes. +barrier also rejects writes. This includes all nodes outside Git. Writable Pi nodes +can use `bash` (and `powershell` on Windows) to build and run tests. Shell calls +participate in the write barrier and forward cancellation to their processes. `request.predecessors` contains direct active predecessors in incoming-edge order, including failures on unconditional edges with an `error` field. Each source appears once: @@ -467,7 +468,10 @@ The model-facing Git tool separates `command` from `args`, for example accepts the complete argument array. Duplicate command prefixes, network Git, branch switching, and filesystem-boundary overrides are rejected. -Instances checkpoint before downstream admission. All source checkpoints remain +Instances checkpoint tracked changes and non-ignored new files before downstream +admission. Ignored dependencies, caches, and build products are discarded on cleanup; +files already tracked or explicitly force-added remain tracked under normal Git rules. +All source checkpoints remain reusable; no merge consumes or deletes a predecessor's result. Job cleanup archives and removes owned worktrees, retaining refs under `refs/braid/checkpoints/`. Integration additionally captures a pre-write @@ -489,19 +493,22 @@ isolated from those source edits. See [execution control](docs/execution-control **The adapter is a trust boundary, not a security sandbox.** It must avoid shared conversation state, expose only its declared capabilities, and forward `signal` to its provider. -The core never gives the model arbitrary code execution or a recursive Braid -tool. An optional Pi adapter translates this same contract without changing the -runtime; the core package does not depend on Pi. See +The core does not supply shell tools or a recursive Braid tool; adapters choose +their tool capabilities. The Pi adapter supplies shell tools to writable nodes +without changing the runtime; the core package does not depend on Pi. See [`integrations/pi/README.md`](integrations/pi/README.md) for installation and testing. The included OpenAI-compatible adapter uses fresh Chat Completions contexts, a strict `decide({ choice })` tool and one tool-free continuation for decisions. Merge/integrate nodes use a local `git` / `finish_merge` tool loop; ordinary OpenAI nodes -have no filesystem tools. Pi exposes read and guarded write tools plus these -core Git/merge tools. Tool errors go back to merge agents for recovery. Both +have no filesystem tools. Pi exposes read, guarded write, and shell tools plus +these core Git/merge tools. Tool errors go back to nodes for recovery. Both adapters sum usage across their model calls and forward cancellation. Pi writes reject external paths, Git metadata, symlinks, hard links, and special -files. These checks are not an OS sandbox against concurrent filesystem attacks. +files. Shell tools can bypass these checks and run with host permissions; prompts +require nodes to respect workspace boundaries, shared Git state, and external +resources. Parent extension/MCP tools and hooks are not inherited. These checks +and cooperation rules are not an OS sandbox. See the [Pi filesystem capabilities](integrations/pi/README.md#node-filesystem-capabilities). Pi tool and time budgets are unlimited by default. Its `options.maxToolRounds` diff --git a/SECURITY.md b/SECURITY.md index e7d34a2..6deb05d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -44,17 +44,21 @@ answer that question. - Braid isolates invocation context; it is not a process or filesystem sandbox. A custom runner is trusted code with the host process's permissions. - The core manages Git snapshots, worktrees, checkpoint refs, and merge tools. - Pi workers can write/edit their assigned Git worktree. Integrate agents can apply + Writable Pi workers can write/edit and run shell commands in their assigned Git + worktree. Shells run with host permissions; prompts constrain workspace writes, + shared Git state, and external side effects. Integrate agents can apply changes to the source checkout; merge agents use isolated worktrees; they are not restricted to read-only analysis. Outside Git, Pi file tools stay read-only. Read paths can expose files outside the checkout and disclose content to a model provider. - Execute/decision nodes can request `workspace: "read-only"` inside Git. Pi - omits write/edit tools, and core rejects write-barrier operations. Git inspection + omits write/edit and shell tools, and core rejects write-barrier operations. Git inspection remains available. These nodes read an isolated predecessor snapshot; custom runners must honor this capability themselves. - Guarded write tools reject external paths, Git metadata, symlinks, hard links, and special files, but are not an OS sandbox against concurrent filesystem - attacks. Avoid concurrent external source edits while integrate agents run. A + attacks. Shell commands can bypass these guards. Parent extension/MCP tools and + their interception policies are not inherited by nodes. Avoid concurrent external + source edits while integrate agents run. A cancellation or failed merge can leave partial integration/conflicts for review; checkpoint and backup refs support recovery. - Prompts, predecessor outputs, tool results, errors, and model answers may be @@ -68,6 +72,11 @@ answer that question. - Cancellation and timeout cannot stop synchronous JavaScript or remote work that ignores the abort signal. Enforce provider quotas and host-side admission limits for untrusted callers. See [resource limits](docs/resource-limits.md). + Pi shell calls are drained before checkpointing and stop their process group + (Windows uses `taskkill /T`); daemonized processes outside that group and external + services can outlive a call. Windows cleanup after parent exit is best effort. + Ignored new files are excluded from checkpoints and discarded with the worktree; + explicitly staged files still follow Git tracking semantics. The OpenAI-compatible adapter sends its API key only to the configured base URL. Treat that URL as trusted configuration. Keep credentials out of graphs, logs, diff --git a/docs/compatibility.md b/docs/compatibility.md index d26f9b2..a7952a3 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -71,6 +71,31 @@ separate gate reminder; when both preferences are enabled it supplies the single completion reminder for that instance. Cancellation still sends failure reminders for opted-in running instances. +## Upcoming workspace capability changes + +The Unreleased changes target the next minor release; they are not part of 0.2.1: + +- Writable Pi nodes now receive `bash` and, on Windows, `powershell`, allowing + dependency installation, builds, and tests within a node. If a graph relies on + workers having no shell, set execute/decision nodes to `workspace: "read-only"` + (which also disables file writes), or use a custom runner with the required + capability policy. Writable shell access uses host permissions and prompt-based + cooperation rules; worktrees are not a security sandbox. Parent extension/MCP + tools and their hooks are not inherited. +- Core checkpoints now follow `git add --all` semantics instead of force-adding + every output. Move artifacts needed by successors or for recovery to non-ignored + paths, or deliberately track them. Already tracked files, including files + explicitly staged with `git add --force`, remain tracked even when they match + ignore rules. Untracked ignored outputs are discarded when worktrees are removed, + including after failed or cancelled executions. This applies to every adapter. +- Shell commands must run in the foreground. On POSIX, Braid stops remaining + children in the command's process group even on normal completion; Windows uses + best-effort process-tree cleanup. Run a temporary server and its checks within + one foreground command and clean it up before returning. Detached daemons and + external services are not contained by this lifecycle. + +No graph field, result shape, or event schema changes are required. + ## Versioning Core and Pi release together with matching versions. During 0.x, patch releases diff --git a/integrations/pi/README.md b/integrations/pi/README.md index d93cd1d..65f81c9 100644 --- a/integrations/pi/README.md +++ b/integrations/pi/README.md @@ -130,8 +130,8 @@ an explicit per-turn planning policy to Pi's system prompt and tool metadata: changes spanning multiple files, call Braid first when two or more concerns can be handled independently; use `workspace: "read-only"` for analysis, review, routing, and synthesis, and worktrees for implementation; -- do not use Braid for simple one-step answers, trivial direct edits, or shell - work; keep tests and shell commands in the parent agent; +- do not use Braid for simple one-step answers, trivial direct edits, or a single + shell command; writable nodes can implement, test, and fix their own work; - the user does not need to say “Braid” or design the graph; - when Braid fits, the model should submit a graph and refine it with live updates when needed, continue independent work, and retrieve the terminal outputs after the completion reminder. @@ -142,16 +142,18 @@ or strengthen the project/system prompt for that model. The adapter explicitly asks the model to make the delegation choice before directly inspecting the repository. Do not add a generic `always call braid` rule: that would waste model calls and bypass direct tools. -Merge nodes combine snapshots; integrate nodes apply selected changes to the caller; the parent reviews results and runs tests. +Merge nodes combine and validate snapshots; integrate nodes apply selected changes to the caller; the parent reviews results and performs any remaining validation. Each node gets a new Pi AI context containing only the Braid goal, its node prompt, labelled direct predecessor outputs, and workspace metadata. It receives Pi's `read` and `ls`, plus `grep` when local `rg` is available and `find` when local `fd`/`fdfind` is available. Missing search dependencies are reported in the node prompt, with `ls`/`read` as alternatives. Dependencies are checked before exposing search tools and again before executing them; missing tools are not -installed by Braid. Git worktrees additionally receive `write` and `edit`. -It receives no parent transcript, shell tools, test runner, skills, or arbitrary -code execution. Decision nodes additionally receive `decide`. Git nodes receive +installed automatically by Braid. Writable workspaces additionally receive `write`, +`edit`, and Pi's `bash` tool (`powershell` is also exposed on Windows), so nodes +can install local dependencies, build, and run tests. Read-only nodes have no shell. +Nodes receive no parent transcript, skills, or inherited extension/MCP tools. +Decision nodes additionally receive `decide`. Git nodes receive local Git inspection; merge/integrate nodes also receive Git integration commands and `finish_merge`. Merge agents receive bounded changed-file lists, diff statistics and previews; integrate also receives the source checkout's dirty status. The model-facing `git` @@ -237,7 +239,8 @@ Root executions use the initial job snapshot, including tracked and non-ignored untracked caller edits. Later loop rounds get new worktrees; they never reuse a previous invocation's workspace. `workspace: "read-only"` disables write/edit while retaining an isolated snapshot. Outside Git, all filesystem access is -read-only. Search tools require installed `rg`/`fd`; shell/tests are unavailable. +read-only, including no shell tools. Search tools require installed `rg`/`fd`. +Writable nodes can run shell commands and tests in their assigned working directory. `merge` combines predecessor results into a new isolated worktree. `integrate` applies selected changes to the invoking checkout and preserves user edits. Both @@ -261,6 +264,12 @@ results, explicitly connect them to an integrate node: } ``` +Checkpoints save tracked changes and non-ignored new files, following normal +`git add --all` semantics. Existing tracked files remain tracked even when they +match ignore rules. Ignored dependencies, caches, and build outputs are discarded +with the worktree and are not carried to successors; keep required outputs in +non-ignored paths. Files deliberately staged with `git add --force` remain tracked. + `braid_status` retains execution-keyed workspace paths, checkpoint refs, and target merge dispositions. Cleanup removes worktrees while keeping immutable checkpoints. Inspect with `git show :path`. Integration additionally saves a @@ -268,10 +277,22 @@ pre-write backup ref. A failed integration can leave partial source changes or conflicts; core does not reset the caller's checkout. Integrations serialize within the process, without locking parent edits or other processes. -Pi's guarded writes reject external paths, Git metadata, symlinks, hard links, -and special files. This is a capability boundary, not an OS sandbox. A custom -runner must honor the core write barrier so cancellation can drain writes before -cleanup. Calling the Pi runner without an assigned workspace stays read-only. +Pi's guarded `write`/`edit` reject external paths, Git metadata, symlinks, hard links, +and special files. Shell tools run with host permissions and can bypass those +checks. Prompts require writes to stay in the assigned workspace, reserve source +checkout changes for integrate nodes, and protect shared Git refs/configuration, +other nodes, ports, databases, caches, and external services. These are cooperation +rules, not an OS sandbox. Shell tools are created for each invocation; parent +extension/MCP tools and their hooks are not inherited. + +Shell calls join the core write barrier: cancellation/timeout stops the process +group (Windows uses `taskkill /T`), and checkpointing waits for the call to settle. +On POSIX, remaining children in the command's process group are also stopped on +normal command exit. Windows cleanup after the parent process exits is best effort. +Run commands in the foreground; do not daemonize or leave servers/watchers running. +Processes that detach from the group and external services are not contained by +this mechanism. A custom runner must honor the core write barrier as well. +Calling the Pi runner without an assigned workspace stays read-only. Tool and time budgets are unlimited by default in Pi. To set finite hard limits, pass any of these fields in the `braid` tool's `options`: diff --git a/integrations/pi/index.ts b/integrations/pi/index.ts index 2d4fb6a..f6eb745 100644 --- a/integrations/pi/index.ts +++ b/integrations/pi/index.ts @@ -68,7 +68,7 @@ const BRAID_FILESYSTEM_GUIDANCE = "Every execute/decision activation gets a fresh worktree in Git, based on its predecessor execution checkpoint (roots use the initial job snapshot). Set workspace=read-only to disable writes. Multiple independent code snapshots require an explicit merge node. " + "merge combines selected predecessor checkpoints into a new isolated worktree; integrate writes selected changes to the invoking checkout while preserving user edits. Both must call finish_merge using executionId for each source. There is no automatic final integration. " + "Do not set workspace on merge/integrate nodes. Checkpoints remain recoverable after cleanup. Optional failed predecessors pass errors and partial work along unconditional edges; requireSuccess=true makes failure abort the job. " + - "Nodes have local read/ls and Git inspection; writable nodes have write/edit, merge/integrate have local Git integration tools. Search tools require local rg/fd. Outside Git all filesystem access is read-only. Shell, tests, network Git, and recursive Braid calls are unavailable. Run tests in the parent after explicit integration."; + "Nodes have local read/ls and Git inspection; writable nodes have write/edit and Pi shell tools for dependencies, builds, and tests, while merge/integrate also have local Git integration tools. Search tools require local rg/fd. Outside Git all filesystem access is read-only; read-only nodes have no shell. Worktrees are not an OS sandbox: prompts constrain shell writes and shared resources. Checkpoints omit ignored new files. Parent extension/MCP tools and recursive Braid calls are not provided. Nodes should verify their changes; the parent reviews results and performs any remaining validation after integration."; const BRAID_USAGE_GUIDANCE = [ "Braid is a proactive execution primitive, not only a user-requested command.", @@ -77,7 +77,7 @@ const BRAID_USAGE_GUIDANCE = [ "For repeated instructions, define promptTemplates once and use prompt={template: name, variables: {name: value}} on nodes. Values are strings inserted literally into {{name}} placeholders; plain-string prompts remain supported.", "When Braid fits, submit a graph: use parallel execute nodes for independent analysis or implementation, execute nodes to synthesize findings, merge nodes to combine code snapshots, and integrate nodes to apply changes to the working branch. The tool returns a jobId immediately. Continue independent work or finish your turn while it runs; do not poll repeatedly. A completion reminder will resume you. Use braid_status with the jobId to retrieve terminal outputs before relying on them.", "Set notifyOnCompletion=true on selected nodes to receive intermediate success/failure reminders. Use braid_status({jobId, executionId}) to retrieve the exact execution; nodeId selects the latest instance. Use pauseAfter=true to hold outgoing scheduling. Definitions can always be changed with braid_update using expectedRevision; existing executions retain their captured inputs. Use resume in the same update to apply changes and release held executions atomically, or braid_resume for no graph changes.", - "Do not use braid for a simple one-step answer, a trivial direct edit, shell work, or when decomposition adds no value. The parent reviews results, runs tests, and executes shell commands after Braid completes.", + "Do not use braid for a simple one-step answer, a trivial direct edit, a single shell command, or when decomposition adds no value. Writable nodes can implement and test their work. The parent reviews results and performs any remaining validation after Braid completes.", ].join("\n"); export function createBraidTools(jobs: BraidJobs) { @@ -92,7 +92,7 @@ export function createBraidTools(jobs: BraidJobs) { "For repeated prompts, define promptTemplates and set node prompt to {template: name, variables: {name: value}}; core renders {{name}} placeholders using explicit string variables before execution. " + "Unlabelled edges are unconditional. Joins wait for all possible predecessor paths to resolve. " + "Nodes see only the goal, their prompt, labelled direct-predecessor outputs, and their filesystem capabilities: " + - "no parent history, shell, tests, or recursive Braid calls. " + + "no parent history, inherited extension/MCP tools, or recursive Braid tools. " + BRAID_FILESYSTEM_GUIDANCE + " " + "Do not use it for a simple one-step answer or trivial direct edit. " + "Use braid_status(jobId) for progress and results, or braid_cancel(jobId) to stop it. A completion reminder resumes the agent if idle; do independent work or end your turn instead of polling. Humans can open /braid for the live flow panel. " + @@ -103,7 +103,7 @@ export function createBraidTools(jobs: BraidJobs) { promptGuidelines: [ "Call braid before direct repository inspection when a code task has two or more separable review, debugging, design, test-planning, or implementation concerns; the Braid nodes can inspect the project and edit isolated Git worktrees.", "Use parallel execute nodes for independent analysis or implementation, execute nodes to synthesize findings, merge nodes to combine code snapshots, and integrate nodes to apply changes to the working branch. The user does not need to mention Braid or design the graph.", - "Do not use braid for simple one-step answers or trivial direct edits. Keep shell commands and test execution in the parent; use merge nodes for integration.", + "Do not use braid for simple one-step answers or trivial direct edits. Let writable nodes run shell commands and tests for their work; use merge nodes to combine and validate snapshots before explicit integration.", BRAID_FILESYSTEM_GUIDANCE, "Decision nodes additionally receive decide. Nodes cannot call recursive Braid.", "Tool and time budgets are unlimited by default. Set maxToolRounds, maxToolCalls, nodeTimeoutMs, or graphTimeoutMs in options to impose hard limits; nodes receive system reminders of their remaining budgets before each model call.", diff --git a/integrations/pi/runner.ts b/integrations/pi/runner.ts index 16322aa..9188fbd 100644 --- a/integrations/pi/runner.ts +++ b/integrations/pi/runner.ts @@ -25,6 +25,7 @@ import { } from "@chrok/braid"; import { createWorktreeWriteTools } from "./write-tools.js"; import { createAvailableReadTools } from "./read-tools.js"; +import { createWorkspaceShellTools } from "./shell-tools.js"; export interface PiNodeProgress { nodeId: string; @@ -118,7 +119,10 @@ export function createPiRunner( const fileTools = [ ...readTools.tools, ...(writeRoot - ? await createWorktreeWriteTools(workingDirectory, writeRoot, request.signal, readOnlyPaths) + ? [ + ...await createWorktreeWriteTools(workingDirectory, writeRoot, request.signal, readOnlyPaths), + ...createWorkspaceShellTools(workingDirectory), + ] : []), ]; // Send only serializable definitions to the model, not execute functions. @@ -159,12 +163,20 @@ export function createPiRunner( : workspace.mode === "worktree" ? "You may write and edit files inside your own isolated Git worktree. Use workingDirectory as your cwd; do not write to sourceRoot or any other node's worktree. " + "Each execution starts from its predecessor checkpoint; root executions use the initial job snapshot. Repeated loop executions get new worktrees. Inspect exact predecessor checkpoints with git show. " + - "Describe your changes in your final answer. Core will save your checkpoint and clean up the worktree. Only explicit integrate nodes write to the source checkout. " + "Describe your changes and verification in your final answer. Core will save your checkpoint and clean up the worktree. Leave source checkout changes to explicit integrate nodes. " : "This node has no writable workspace assigned. Its filesystem tools are read-only; you cannot write or edit files. " + "Read workingDirectory directly; in Git this is an isolated predecessor snapshot. Outside Git it is the source directory. " + (request.git ? "Use Git inspection to review changes or predecessor checkpoints; the assigned snapshot contains predecessor edits. " : "")) + mergeInstructions(request) + - "You cannot run shell commands, run tests, or call arbitrary tools. " + + (writeRoot + ? "You may use Pi's shell tools to install local dependencies, build, run tests, and fix failures in workingDirectory. " + + "Worktrees isolate code snapshots, not host permissions: shell access is not sandboxed. Keep file changes within your assigned workspace; integrate alone may edit sourceRoot. " + + "Git refs, configuration, hooks, and object storage are shared with the caller and other nodes. Do not change shared Git configuration, hooks, branches, Braid refs, or worktree registrations, and do not switch branches. Prefer the provided git tool for inspection and merge operations. " + + "Nodes run concurrently and loop executions start fresh: ports, databases, caches, credentials, and external services are shared. Use execution-specific temporary resources, avoid global installs and destructive or externally visible actions unless explicitly requested, and do not spawn recursive Braid/Pi agents. " + + "Run commands in the foreground; command completion, timeout, or cancellation stops their process group. Do not daemonize or leave servers/watchers running. " + + "Checkpoints include tracked changes and non-ignored new files; ignored dependencies, caches, and build products are not carried to successors and are removed during cleanup. " + : "You cannot run shell commands or tests in this read-only workspace. ") + + "Parent extension/MCP tools, skills, and session history are not inherited; use only the tools provided here. " + ((request.node.type === "merge" || request.node.type === "integrate") && workspace.mode === "read-only" ? "This merge has no Git sources; call finish_merge with an empty dispositions array before answering." : request.node.type === "decision" @@ -313,7 +325,7 @@ export function createPiRunner( request.signal, undefined, ); - const result = ["write", "edit"].includes(call.name) && request.withWorkspaceWrite + const result = ["write", "edit", "bash", "powershell"].includes(call.name) && request.withWorkspaceWrite ? await request.withWorkspaceWrite(execute) : await execute(); request.signal.throwIfAborted(); @@ -323,7 +335,7 @@ export function createPiRunner( toolName: call.name, content: result.content, ...(result.details === undefined ? {} : { details: result.details }), - isError: false, + isError: result.isError ?? false, timestamp: Date.now(), }; } catch (error) { diff --git a/integrations/pi/shell-tools.ts b/integrations/pi/shell-tools.ts new file mode 100644 index 0000000..3b1be4a --- /dev/null +++ b/integrations/pi/shell-tools.ts @@ -0,0 +1,98 @@ +import { execFile, spawn } from "node:child_process"; +import { constants } from "node:os"; +import { join } from "node:path"; +import { + createBashTool, createPowerShellTool, getShellConfig, getPowerShellConfig, + type BashOperations, +} from "@earendil-works/pi-coding-agent"; + +/** Pi owns the tool schema/output handling; Braid owns the command lifetime. */ +function managedOperations(resolveShell: typeof getShellConfig, prefix = ""): BashOperations { + return { + async exec(command, cwd, { onData, signal, timeout, env }) { + signal?.throwIfAborted(); + if (timeout !== undefined && (!Number.isFinite(timeout) || timeout <= 0 || timeout * 1000 > 2_147_483_647)) + throw new Error("Invalid timeout: expected positive seconds within the timer limit"); + const config = resolveShell(); + signal?.throwIfAborted(); + const stdin = config.commandTransport === "stdin"; + const child = spawn(config.shell, stdin ? config.args : [...config.args, prefix + command], { + cwd, env, detached: process.platform !== "win32", windowsHide: true, + stdio: [stdin ? "pipe" : "ignore", "pipe", "pipe"], + }); + const closed = new Promise(resolve => child.once("close", () => resolve())); + const exited = new Promise((resolve, reject) => { + child.once("error", reject); + child.once("exit", resolve); + }); + child.stdout!.on("data", onData); + child.stderr!.on("data", onData); + if (stdin) { + child.stdin!.on("error", () => {}); + child.stdin!.end(prefix + command); + } + let termination: Promise | undefined; + const terminate = (): Promise => termination ??= (async () => { + if (!child.pid) return; + if (process.platform === "win32") { + await new Promise((resolve, reject) => { + execFile(join(process.env.SystemRoot ?? "C:\\Windows", "System32", "taskkill.exe"), + ["/F", "/T", "/PID", String(child.pid)], { windowsHide: true }, error => { + // taskkill reports 128 when the process has already exited. + if (error && error.code !== 128) reject(error); + else resolve(); + }); + }); + } else { + try { process.kill(-child.pid, "SIGKILL"); } + catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + } + } + })(); + // The completion path awaits termination; event handlers must not create + // unhandled rejections while the shell is still exiting. + const stop = () => { void terminate().catch(() => {}); }; + let timedOut = false; + const timer = timeout === undefined ? undefined : setTimeout(() => { + timedOut = true; + stop(); + }, timeout * 1000); + signal?.addEventListener("abort", stop, { once: true }); + if (signal?.aborted) stop(); + let exitCode: number | null; + try { + exitCode = await exited; + } finally { + clearTimeout(timer); + signal?.removeEventListener("abort", stop); + // Stop leftover children even on successful command completion. Commands + // cannot leave a server/watch process writing during checkpoint/cleanup. + try { await terminate(); } + finally { + // A daemon can leave the process group while holding inherited pipes. + // Bound pipe draining; this process-group cleanup is not an OS sandbox. + const drainTimer = setTimeout(() => { + child.stdout!.destroy(); + child.stderr!.destroy(); + }, 250); + try { await closed; } + finally { clearTimeout(drainTimer); } + } + } + if (signal?.aborted) throw new Error("aborted"); + if (timedOut) throw new Error(`timeout:${timeout}`); + return { exitCode: exitCode ?? (child.signalCode ? 128 + (constants.signals[child.signalCode] ?? 0) : 1) }; + }, + }; +} + +export function createWorkspaceShellTools(cwd: string) { + return [ + createBashTool(cwd, { operations: managedOperations(getShellConfig), exposeSessionEnvironment: false }), + ...(process.platform === "win32" ? [createPowerShellTool(cwd, { + operations: managedOperations(getPowerShellConfig, "try { [Console]::OutputEncoding=[System.Text.Encoding]::UTF8 } catch {}\n"), + exposeSessionEnvironment: false, + })] : []), + ]; +} diff --git a/integrations/pi/test/runner.test.ts b/integrations/pi/test/runner.test.ts index c980462..9d972ed 100644 --- a/integrations/pi/test/runner.test.ts +++ b/integrations/pi/test/runner.test.ts @@ -326,7 +326,7 @@ test("Pi runner executes read-only node tools and returns their results to the m ); }); -test("Pi runner keeps write and shell tools unavailable", async () => { +test("Pi runner keeps write and shell tools unavailable without an assigned writable workspace", async () => { const fake = fakeRegistry([ message( [ diff --git a/integrations/pi/test/shell-tools.test.ts b/integrations/pi/test/shell-tools.test.ts new file mode 100644 index 0000000..4a693de --- /dev/null +++ b/integrations/pi/test/shell-tools.test.ts @@ -0,0 +1,63 @@ +import assert from "node:assert/strict"; +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test, { type TestContext } from "node:test"; +import { createWorkspaceShellTools } from "../shell-tools.js"; + +async function fixture(t: TestContext) { + const cwd = await mkdtemp(join(tmpdir(), "braid-shell-test-")); + t.after(() => rm(cwd, { recursive: true, force: true })); + return { cwd, tools: createWorkspaceShellTools(cwd) }; +} + +test("Pi shell tools execute in the assigned cwd and retain output/error semantics", async t => { + const { cwd, tools } = await fixture(t); + for (const tool of tools) { + const command = tool.name === "powershell" + ? "Set-Content -Path result.txt -Value done; Write-Output shell-output; exit 7" + : "printf done > result.txt; printf shell-output; printf shell-error >&2; exit 7"; + const result = await tool.execute("run", { command }); + assert.match(await readFile(join(cwd, "result.txt"), "utf8"), /done/); + assert.equal(result.isError, true); + assert.match(JSON.stringify(result.content), /shell-output/); + assert.match(JSON.stringify(result.content), /exited with code 7/); + } +}); + +for (const end of ["abort", "timeout"] as const) { + test(`shell ${end} waits for the command to stop`, { timeout: 10_000 }, async t => { + const { tools } = await fixture(t); + for (const tool of tools) { + const controller = new AbortController(); + const command = tool.name === "powershell" + ? "Write-Output ready; Start-Sleep -Seconds 30" + : "printf ready; sleep 30"; + const call = tool.execute("run", { command, ...(end === "timeout" ? { timeout: 0.5 } : {}) }, controller.signal, + update => { + if (end === "abort" && JSON.stringify(update.content).includes("ready")) controller.abort(); + }); + await assert.rejects(call, end === "abort" ? /aborted/ : /timed out/); + } + }); +} + +test("successful shell commands stop background children before returning", { + skip: process.platform === "win32", timeout: 10_000, +}, async t => { + const { cwd, tools } = await fixture(t); + const result = await tools[0]!.execute("background", { + command: "sleep 30 > /dev/null 2>&1 & child=$!; printf '%s' \"$child\" > child.pid; printf done", + }); + assert.match(JSON.stringify(result.content), /done/); + const pid = Number(await readFile(join(cwd, "child.pid"), "utf8")); + // A killed child can briefly remain a zombie until the host reaps it. Poll its + // existence instead of relying on a scheduling delay or the shell's exit alone. + const deadline = Date.now() + 3000; + while (Date.now() < deadline) { + try { process.kill(pid, 0); } + catch (error) { assert.equal((error as NodeJS.ErrnoException).code, "ESRCH"); return; } + await new Promise(resolve => setTimeout(resolve, 20)); + } + assert.fail(`Background process ${pid} survived shell completion`); +}); diff --git a/integrations/pi/test/workspaces.test.ts b/integrations/pi/test/workspaces.test.ts index 1e49e25..70bb078 100644 --- a/integrations/pi/test/workspaces.test.ts +++ b/integrations/pi/test/workspaces.test.ts @@ -13,6 +13,7 @@ import { createWorktreeWriteTools } from "../write-tools.js"; import { BraidJobs } from "../jobs.js"; import { createBraidTools } from "../index.js"; import { createAvailableReadTools } from "../read-tools.js"; +import { createWorkspaceShellTools } from "../shell-tools.js"; import { context, deferred, input, response } from "./helpers.js"; const exec = promisify(execFile); @@ -57,6 +58,105 @@ function toolResponse(...calls: ToolCall[]) { return { ...response(), stopReason: "toolUse" as const, content: calls }; } +test("writable nodes run and repair shell commands before passing non-ignored changes to successors", async t => { + const fixture = await repository(t); + const turns = new Map(); + const ctx = context(async () => response()); + t.mock.method(ctx.modelRegistry, "complete", async (_model: unknown, worker: Context) => { + const { nodeId, workspace } = JSON.parse(worker.messages[0]!.content as string) as { + nodeId: string; workspace: PiNodeWorkspace; + }; + const turn = turns.get(nodeId) ?? 0; + turns.set(nodeId, turn + 1); + if (nodeId === "review") { + assert.ok(!worker.tools!.some(tool => tool.name === "bash" || tool.name === "powershell")); + assert.equal(await readFile(join(workspace.worktreeRoot!, "src", "tested.txt"), "utf8"), "verified"); + await assert.rejects(readFile(join(workspace.worktreeRoot!, "ignored.txt")), { code: "ENOENT" }); + return response("Reviewed the tested checkpoint"); + } + assert.ok(worker.tools!.some(tool => tool.name === "bash")); + assert.ok(!worker.tools!.some(tool => tool.name === "braid")); + assert.match(worker.systemPrompt!, /shell access is not sandboxed/); + assert.match(worker.systemPrompt!, /Git refs, configuration, hooks/); + assert.match(worker.systemPrompt!, /Parent extension\/MCP tools/); + if (turn === 0) return toolResponse({ type: "toolCall", id: "fail", name: "bash", arguments: { + command: "printf failing-test; exit 7", + } }); + const result = worker.messages.at(-1)!; + assert.equal(result.role, "toolResult"); + if (result.role === "toolResult") assert.equal(result.isError, turn === 1); + if (turn === 1) return toolResponse({ type: "toolCall", id: "fix", name: "bash", arguments: { + command: "printf verified > tested.txt; printf cache > ../ignored.txt; test -f tested.txt", + } }); + return response("Implemented and tested"); + }); + const result = await braid({ + goal: "Implement and verify", nodes: [ + { type: "execute", id: "work", prompt: "Implement and test" }, + { type: "execute", id: "review", prompt: "Review", workspace: "read-only" }, + ], edges: [{ from: "work", to: "review" }], + }, { + cwd: join(fixture.root, "src"), defaultModel: "fake/model", + runner: createPiRunner(ctx.modelRegistry, { cwd: join(fixture.root, "src"), onWorkspace: fixture.onWorkspace }), + }); + assert.equal(result.nodes.work!.status, "completed", JSON.stringify(result)); + assert.equal(result.nodes.review!.status, "completed", JSON.stringify(result)); + assert.equal(turns.get("work"), 3); + await assert.rejects(readFile(join(fixture.root, "src", "tested.txt")), { code: "ENOENT" }); +}); + +test("cancelling a shell call drains it before checkpointing partial work and removing its worktree", { timeout: 15_000 }, async t => { + const fixture = await repository(t); + const controller = new AbortController(); + t.after(() => controller.abort()); + const ready = deferred(); + let drained = false; + let drainedAtFailure = false; + const ctx = context(async () => toolResponse({ type: "toolCall", id: "shell", name: "bash", arguments: { + command: "printf partial > partial.txt; printf cache > ignored.txt; sleep 30", + } })); + const piRunner = createPiRunner(ctx.modelRegistry, { + cwd: fixture.root, onWorkspace: workspace => { + fixture.onWorkspace(workspace); + ready.resolve(workspace); + }, + }); + const pending = braid({ + goal: "Cancel a running command", nodes: [{ type: "execute", id: "work", prompt: "Run" }], edges: [], + }, { + cwd: fixture.root, defaultModel: "fake/model", signal: controller.signal, + runner: request => { + const write = request.withWorkspaceWrite!; + request.withWorkspaceWrite = operation => write(async () => { + try { return await operation(); } + finally { drained = true; } + }); + return piRunner(request); + }, + onEvent: event => { + if (event.type === "node_failed") drainedAtFailure = drained; + }, + }); + const workspace = await ready.promise; + const deadline = Date.now() + 5000; + while (true) { + try { await readFile(join(workspace.worktreeRoot!, "ignored.txt")); break; } + catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT" || Date.now() >= deadline) throw error; + await new Promise(resolve => setTimeout(resolve, 10)); + } + } + controller.abort(); + const result = await pending; + assert.equal(drained, true); + assert.equal(drainedAtFailure, true); + assert.equal(result.nodes.work!.error?.code, "CANCELLED"); + const saved = result.nodes.work!.workspace!; + assert.equal(await git(fixture.root, "show", `${saved.checkpointRef}:partial.txt`), "partial"); + await assert.rejects(git(fixture.root, "show", `${saved.checkpointRef}:ignored.txt`)); + await assert.rejects(readFile(join(saved.worktreeRoot!, "partial.txt")), { code: "ENOENT" }); +}); + test("Pi braid tool submits read-only review and decision nodes with Git inspection but no write tools", async t => { const fixture = await repository(t); await writeFile(join(fixture.root, "src/file.txt"), "parent change\n"); @@ -88,11 +188,12 @@ test("Pi braid tool submits read-only review and decision nodes with Git inspect { type: "toolCall", id: "write", name: "write", arguments: { path: "file.txt", content: "bad" } }, { type: "toolCall", id: "edit", name: "edit", arguments: { path: "file.txt", edits: [{ oldText: "parent", newText: "bad" }] } }, { type: "toolCall", id: "add", name: "git", arguments: { command: "add", args: ["."] } }, + { type: "toolCall", id: "shell", name: "bash", arguments: { command: "echo forbidden > forbidden.txt" } }, ); } const results = worker.messages.filter(message => message.role === "toolResult"); if (nodeId === "review") { - assert.deepEqual(results.map(result => result.isError), [false, false, true, true, true]); + assert.deepEqual(results.map(result => result.isError), [false, false, true, true, true, true]); assert.match(JSON.stringify(results[0]!.content), /parent change/); assert.doesNotMatch(JSON.stringify(results[1]!.content), /parent change/); } else assert.equal(results[0]!.isError, false); @@ -166,9 +267,9 @@ test("parallel Git nodes write and edit separate worktrees from the same dirty s }; const turn = turns.get(payload.nodeId) ?? 0; turns.set(payload.nodeId, turn + 1); - assert.deepEqual(worker.tools!.map(tool => tool.name), [...(await createAvailableReadTools(fixture.root)).tools.map(tool => tool.name), "write", "edit", "git"]); + assert.deepEqual(worker.tools!.map(tool => tool.name), [...(await createAvailableReadTools(fixture.root)).tools.map(tool => tool.name), "write", "edit", ...createWorkspaceShellTools(fixture.root).map(tool => tool.name), "git"]); assert.match(worker.systemPrompt!, /own isolated Git worktree/); - assert.match(worker.systemPrompt!, /cannot run shell commands/); + assert.match(worker.systemPrompt!, /run tests, and fix failures/); if (turn === 0) { assert.equal(await readFile(join(payload.workingDirectory, "file.txt"), "utf8"), "unstaged\n"); assert.equal(await readFile(join(payload.workspace.worktreeRoot!, "new.txt"), "utf8"), "untracked\n"); diff --git a/src/workspaces.ts b/src/workspaces.ts index 9b8557f..d5254f3 100644 --- a/src/workspaces.ts +++ b/src/workspaces.ts @@ -181,7 +181,7 @@ export class GitWorkspaces { // A temporary index captures tracked edits/deletions and non-ignored new files // without changing the parent's real index, branch, or working files. await git(sourceRoot, ["add", "--all", "--", "."], options); - // Include ignored files contributed by selected sources, without capturing the + // Include ignored files deliberately tracked by selected sources, without capturing the // caller's unrelated ignored build products or dependencies. const existing: string[] = []; for (const file of extraFiles) { @@ -330,7 +330,7 @@ export class GitWorkspaces { .map(workspace => workspace.executionId ?? workspace.nodeId); } - /** Checkpoint every file, including ignored node outputs, before releasing a worktree. */ + /** Preserve tracked changes and non-ignored new files before releasing a worktree. */ private async checkpoint(workspace: NodeWorkspace): Promise { if (workspace.checkpointRef) return; const location = this.locations.get(workspace.executionId ?? workspace.nodeId)!; @@ -348,7 +348,7 @@ export class GitWorkspaces { if (!["ENOENT", "ENOTDIR"].includes((error as NodeJS.ErrnoException).code ?? "")) throw error; } } - await git(cwd, ["add", "--force", "--all", "--", "."], options); + await git(cwd, ["add", "--all", "--", "."], options); const tree = await git(cwd, ["write-tree"], options); const baseTree = await git(cwd, ["rev-parse", `${workspace.snapshotCommit}^{tree}`]); const parents = [...new Set([workspace.snapshotCommit!, ...(this.mergeParents.get(workspace.executionId ?? workspace.nodeId) ?? [])])]; diff --git a/test/merge.test.ts b/test/merge.test.ts index ad60949..6b779dd 100644 --- a/test/merge.test.ts +++ b/test/merge.test.ts @@ -186,7 +186,7 @@ test("failed nodes without changes retain checkpoints and preserve errors for co await noWorktrees(cwd, result); }); -test("explicit integration receives selected sources, including ignored output from failed nodes", async t => { +test("explicit integration preserves deliberately force-added files from failed nodes", async t => { const cwd = await repository(t); let sources: string[] = []; const result = await braid(graph([execute("unchanged"), execute("changed"), { type: "integrate", id: "integrate" }], [{ from: "changed", to: "integrate" }]), { cwd, runner: async request => { @@ -197,7 +197,10 @@ test("explicit integration receives selected sources, including ignored output f return { output: "Recovered partial work" }; } await request.withWorkspaceWrite!(async () => { - if (request.node.id === "changed") await writeFile(join(request.workspace!.worktreeRoot!, "ignored.txt"), "partial work"); + if (request.node.id === "changed") { + await writeFile(join(request.workspace!.worktreeRoot!, "ignored.txt"), "partial work"); + await git(request.workspace!.worktreeRoot!, "add", "--force", "ignored.txt"); + } else { await writeFile(join(request.workspace!.worktreeRoot!, "file.txt"), "temporary edit\n"); await writeFile(join(request.workspace!.worktreeRoot!, "file.txt"), "original\n"); @@ -376,7 +379,7 @@ for (const outcome of ["discard", "throw", "unfinished"] as const) { const cwd = await repository(t); const result = await braid(graph([execute("work"), { type: "integrate", id: "integrate", requireSuccess: true }], [{ from: "work", to: "integrate" }]), { cwd, runner: async request => { if (request.node.type !== "integrate") { - await request.withWorkspaceWrite!(() => writeFile(join(request.workspace!.worktreeRoot!, "ignored.txt"), "recover this ignored output")); + await request.withWorkspaceWrite!(() => writeFile(join(request.workspace!.worktreeRoot!, "recovery.txt"), "recover this output")); return { output: "made changes" }; } if (outcome === "throw") throw new Error("merge agent failed"); @@ -386,9 +389,9 @@ for (const outcome of ["discard", "throw", "unfinished"] as const) { assert.equal(result.status, outcome === "discard" ? "completed" : "failed"); const source = result.nodes.work!.workspace!; assert.equal(source.state, "archived"); - assert.equal(await git(cwd, "show", `${source.checkpointRef}:ignored.txt`), "recover this ignored output"); + assert.equal(await git(cwd, "show", `${source.checkpointRef}:recovery.txt`), "recover this output"); assert.ok(result.nodes.integrate!.workspace!.backupRef); - await assert.rejects(readFile(join(cwd, "ignored.txt")), { code: "ENOENT" }); + await assert.rejects(readFile(join(cwd, "recovery.txt")), { code: "ENOENT" }); await noWorktrees(cwd, result); }); } diff --git a/test/workspaces.test.ts b/test/workspaces.test.ts index e312e0a..7063f98 100644 --- a/test/workspaces.test.ts +++ b/test/workspaces.test.ts @@ -53,6 +53,32 @@ function request(id: string, signal = new AbortController().signal, runId = "run }; } +test("checkpoints honor ignore rules while preserving tracked edits and deletions", async t => { + const fixture = await repository(t); + await writeFile(join(fixture.root, ".gitignore"), "ignored.txt\ntracked.txt\nnode_modules/\n"); + await writeFile(join(fixture.root, "tracked.txt"), "tracked despite ignore rules\n"); + await git(fixture.root, "add", "--force", "tracked.txt"); + const manager = new GitWorkspaces(fixture.root, fixture.onWorkspace); + const invocation = request("build"); + invocation.workspace = await manager.prepare(invocation); + const cwd = invocation.workspace.worktreeRoot!; + await writeFile(join(cwd, "tracked.txt"), "tracked edit\n"); + await writeFile(join(cwd, "ignored.txt"), "temporary output\n"); + await mkdir(join(cwd, "node_modules")); + await writeFile(join(cwd, "node_modules", "dependency.js"), "dependency\n"); + await writeFile(join(cwd, "new.txt"), "new source\n"); + await rm(join(cwd, "deleted.txt")); + await manager.seal(invocation); + const checkpoint = manager.all().build!.checkpointRef!; + assert.equal(await git(fixture.root, "show", `${checkpoint}:tracked.txt`), "tracked edit"); + assert.equal(await git(fixture.root, "show", `${checkpoint}:new.txt`), "new source"); + for (const path of ["ignored.txt", "node_modules/dependency.js", "deleted.txt"]) + await assert.rejects(git(fixture.root, "show", `${checkpoint}:${path}`)); + await manager.archivePending("Test completed"); + await manager.close(); + await assert.rejects(readFile(join(cwd, "ignored.txt")), { code: "ENOENT" }); +}); + for (const source of ["unborn", "linked"] as const) { test(`read-only allocation supports a ${source} checkout with an isolated snapshot`, async t => { const fixture = await repository(t, source === "unborn");