Skip to content

feat(run): control Bash environment forwarding and cancellation #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
byapparov wants to merge 1 commit into from
Closed

feat(run): control Bash environment forwarding and cancellation #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/cli/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,40 @@ Provider failures and stream timeouts retain **1**; SIGINT/SIGTERM retain

In headless mode, Aictrl automatically rejects permissions that would otherwise prompt a user.

### Bash environment forwarding (unreleased)

These examples require a build containing this option; Windows execution is not
verified. Choose the exact environment names forwarded to the built-in `bash`
tool's shell and its path-resolution helper:

```bash
aictrl run "Run the trusted task" # existing parent + hook inheritance
aictrl run --bash-env-allowlist '[]' "Run the trusted task"
aictrl run --bash-env-allowlist '["PATH","LANG"]' "Run the trusted task"
```

Omitting the option keeps existing inheritance. `[]` forwards no parent or hook
entries; a shell can still synthesize its own variables. Nothing automatically
adds `PATH`, `HOME`, proxy or CA settings. Missing selected names are omitted,
existing empty values remain empty, and duplicates are removed. POSIX names are
case-sensitive. Only the caller option sets this policy; project config and
`.env` cannot widen it.

The helper receives selected parent values. After permission checks, the final
shell also considers the `shell.env` hook's overrides, then filters by name.
Opt-in hook records require valid environment names and string values without
NUL; invalid hook data fails that Bash launch. Invalid, missing or repeated
option values are configuration errors (exit **2**). Any explicit policy,
including `[]`, fails early with `--attach` because receiver enforcement is not
supported.

This controls forwarding, not access to secrets: a command can still read
same-UID parent processes or credential files. `PATH` and `HOME` can grant
executable and file access. Trusted plugins, command-template shell expansion,
MCP, formatters and other bootstrap helpers retain separate parent authority.
Use separate credential phases and OS isolation; this option is not a sandbox
or a guarantee that an untrusted checkout is safe.

### Skill Scope

`aictrl run` lists project skills (`.aictrl/skill(s)/`, and `.claude/skills/`,
Expand Down
11 changes: 8 additions & 3 deletions packages/cli/src/cli/cmd/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { Flag } from "../../flag/flag"
import { HeadlessRun } from "../../flag/headless-run"
import { bootstrap } from "../bootstrap"
import { EOL } from "os"
import { AsyncLocalStorage } from "node:async_hooks"
import { Filesystem } from "../../util/filesystem"
import { createAictrlClient } from "@aictrl/sdk"
import type { ToolPart } from "@aictrl/sdk/v2"
Expand Down Expand Up @@ -377,6 +378,12 @@ export const RunCommand = cmd({
.strictOptions()
.strictCommands(false)
.parserConfiguration({ "populate--": true, "parse-positional-numbers": false })
.option("bash-env-allowlist", {
type: "string",
nargs: 1,
requiresArg: true,
describe: "JSON array of exact environment variable names forwarded by native Bash",
})
.option("tools", {
type: "string",
nargs: 1,
Expand Down Expand Up @@ -1265,9 +1272,7 @@ export const RunCommand = cmd({
})
return { data: session }
},
abort(opts: any) {
return SessionPrompt.cancel(opts.sessionID)
},
abort: AsyncLocalStorage.bind((opts: { sessionID: string }) => SessionPrompt.cancel(opts.sessionID)),
async prompt(opts: any) {
promptResult = SessionPrompt.prompt({
sessionID: opts.sessionID,
Expand Down
70 changes: 47 additions & 23 deletions packages/cli/src/headless.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@ import { Global } from "./global"
import { JsonMigration } from "./storage/json-migration"
import { Database } from "./storage/db"
import { Shutdown } from "./cli/shutdown"
import { BashEnvPolicy } from "./util/bash-env-policy"

process.on("unhandledRejection", (e) => {
const formatted = BashEnvPolicy.format(e)
if (formatted) {

@aictrl-dev aictrl-dev Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Policy-error exit(2) skips log record and stderr flush.

🤖 Fix with your agent 
Fix this code review finding (aictrl-dev/cli PR #259, packages/cli/src/headless.ts:27-31):

Problem: Policy-error exit(2) skips log record and stderr flush
Detail: The new unhandledRejection/uncaughtException preamble writes the ConfigError diagnostic with an unawaited process.stderr.write and immediately calls process.exit(2). When stderr is a pipe — the normal headless automation case — the write may not drain before exit, so the documented exit-2 + stderr contract can be truncated. The path also records nothing via Log.Default.error, leaving misconfiguration without a persistent trace, unlike every other fatal path in these files, and skips Shutdown.flush that the main parse catch still performs.
Suggested fix: Flush before exiting and keep a trace: process.stderr.write(formatted + EOL, () => process.exit(2)) (or await write completion), and emit a redacted Log.Default.error record before exit. Extract the guard into a shared helper so headless.ts and index.ts use one implementation.

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

The new unhandledRejection/uncaughtException preamble writes the ConfigError diagnostic with an unawaited process.stderr.write and immediately calls process.exit(2). When stderr is a pipe — the normal headless automation case — the write may not drain before exit, so the documented exit-2 + stderr contract can be truncated. The path also records nothing via Log.Default.error, leaving misconfiguration without a persistent trace, unlike every other fatal path in these files, and skips Shutdown.flush that the main parse catch still performs.

process.on("unhandledRejection", (e) => {
  const formatted = BashEnvPolicy.format(e)
  if (formatted) {
    process.stderr.write(formatted + EOL)
    process.exit(2)
  }
  Log.Default.error("rejection", {

process.stderr.write(formatted + EOL)
process.exit(2)
}
Log.Default.error("rejection", {
e: e instanceof Error ? e.message : e,
stack: e instanceof Error ? e.stack : undefined,
Expand All @@ -30,6 +36,11 @@ process.on("unhandledRejection", (e) => {
})

process.on("uncaughtException", (e) => {
const formatted = BashEnvPolicy.format(e)
if (formatted) {
process.stderr.write(formatted + EOL)
process.exit(2)
}
Log.Default.error("exception", {
e: e instanceof Error ? e.message : e,
stack: e instanceof Error ? e.stack : undefined,
Expand All @@ -55,6 +66,7 @@ let cli = yargs(hideBin(process.argv))
choices: ["DEBUG", "INFO", "WARN", "ERROR"],
})
.middleware(async (opts) => {
BashEnvPolicy.initialize(opts)
await Log.init({
print: process.argv.includes("--print-logs"),
dev: Installation.isLocal(),
Expand All @@ -70,7 +82,7 @@ let cli = yargs(hideBin(process.argv))

Log.Default.info("aictrl-headless", {
version: Installation.VERSION,
args: process.argv.slice(2),
args: BashEnvPolicy.explicit(opts) ? { command: "run", bashEnvPolicy: "allowlist" } : process.argv.slice(2),
})

const storageDir = path.join(Global.Path.data, "storage")
Expand Down Expand Up @@ -112,6 +124,12 @@ let cli = yargs(hideBin(process.argv))

cli = cli
.fail((msg, err) => {
const missing = BashEnvPolicy.parserError(cli as typeof cli & BashEnvPolicy.Parser, msg ?? err?.message)

@aictrl-dev aictrl-dev Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 BashEnvPolicy entry wiring duplicated in both entrypoints.

🤖 Fix with your agent 
Fix this code review finding (aictrl-dev/cli PR #259, packages/cli/src/headless.ts:127-132):

Problem: BashEnvPolicy entry wiring duplicated in both entrypoints
Detail: Five identical behavioral blocks (format guard with exit(2), initialize in middleware, args redaction, .fail preamble, catch restructure) are added to both src/headless.ts and src/index.ts (~25 new lines each). The entrypoints already mirror each other for legacy reasons, but this widens the duplicated behavioral surface and test/cli/exception-handler.test.ts now needs per-file compiler-API extraction to test both copies. Since the logic already lives in BashEnvPolicy, a small shared entry bootstrap would keep the entrypoints from silently diverging on future fixes.
Suggested fix: Extract the shared pieces (the format-and-exit guard, the .fail preamble, and the policy-vs-fatal catch split) into a shared module (e.g. src/cli/entry-policy.ts) imported by both index.ts and headless.ts, parameterized by the entrypoint-specific parts (ResolveMessage handling, log name).

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

Five identical behavioral blocks (format guard with exit(2), initialize in middleware, args redaction, .fail preamble, catch restructure) are added to both src/headless.ts and src/index.ts (~25 new lines each). The entrypoints already mirror each other for legacy reasons, but this widens the duplicated behavioral surface and test/cli/exception-handler.test.ts now needs per-file compiler-API extraction to test both copies. Since the logic already lives in BashEnvPolicy, a small shared entry bootstrap would keep the entrypoints from silently diverging on future fixes.

cli = cli
  .fail((msg, err) => {
    const missing = BashEnvPolicy.parserError(cli as typeof cli & BashEnvPolicy.Parser, msg ?? err?.message)
    if (missing) throw missing
    // Use yargs' actual parse result even when validation fails before middleware.
    if (cli.parsed && BashEnvPolicy.explicit(cli.parsed.argv)) {

if (missing) throw missing
// Use yargs' actual parse result even when validation fails before middleware.
if (cli.parsed && BashEnvPolicy.explicit(cli.parsed.argv)) {
throw new BashEnvPolicy.ConfigError("Invalid run options; check --bash-env-allowlist and other run arguments.")
}
if (
msg?.startsWith("Unknown argument") ||
msg?.startsWith("Not enough non-option arguments") ||
Expand All @@ -128,31 +146,37 @@ cli = cli
try {
await cli.parse()
} catch (e) {
let data: Record<string, any> = {}
if (e instanceof NamedError) {
const obj = e.toObject()
Object.assign(data, {
...obj.data,
})
}
const policyError = BashEnvPolicy.format(e)
if (policyError) {
process.stderr.write(policyError + EOL)
process.exitCode = 2
} else {
let data: Record<string, any> = {}
if (e instanceof NamedError) {
const obj = e.toObject()
Object.assign(data, {
...obj.data,
})
}

if (e instanceof Error) {
Object.assign(data, {
name: e.name,
message: e.message,
cause: e.cause?.toString(),
stack: e.stack,
})
}
if (e instanceof Error) {
Object.assign(data, {
name: e.name,
message: e.message,
cause: e.cause?.toString(),
stack: e.stack,
})
}

Log.Default.error("fatal", data)
const formatted = FormatError(e)
if (formatted) UI.error(formatted)
if (formatted === undefined) {
UI.error("Unexpected error, check log file at " + Log.file() + " for more details" + EOL)
process.stderr.write((e instanceof Error ? e.message : String(e)) + EOL)
Log.Default.error("fatal", data)
const formatted = FormatError(e)
if (formatted) UI.error(formatted)
if (formatted === undefined) {
UI.error("Unexpected error, check log file at " + Log.file() + " for more details" + EOL)
process.stderr.write((e instanceof Error ? e.message : String(e)) + EOL)
}
process.exitCode = 1
}
process.exitCode = 1
} finally {
await Shutdown.flush()
process.exit()
Expand Down
92 changes: 58 additions & 34 deletions packages/cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,14 @@ import { Global } from "./global"
import { JsonMigration } from "./storage/json-migration"
import { Database } from "./storage/db"
import { Shutdown } from "./cli/shutdown"
import { BashEnvPolicy } from "./util/bash-env-policy"

process.on("unhandledRejection", (e) => {
const formatted = BashEnvPolicy.format(e)
if (formatted) {
process.stderr.write(formatted + EOL)
process.exit(2)
}
Log.Default.error("rejection", {
e: e instanceof Error ? e.message : e,
stack: e instanceof Error ? e.stack : undefined,
Expand All @@ -38,6 +44,11 @@ process.on("unhandledRejection", (e) => {
})

process.on("uncaughtException", (e) => {
const formatted = BashEnvPolicy.format(e)
if (formatted) {
process.stderr.write(formatted + EOL)
process.exit(2)
}
Log.Default.error("exception", {
e: e instanceof Error ? e.message : e,
stack: e instanceof Error ? e.stack : undefined,
Expand All @@ -63,6 +74,7 @@ let cli = yargs(hideBin(process.argv))
choices: ["DEBUG", "INFO", "WARN", "ERROR"],
})
.middleware(async (opts) => {
BashEnvPolicy.initialize(opts)
await Log.init({
print: process.argv.includes("--print-logs"),
dev: Installation.isLocal(),
Expand All @@ -80,7 +92,7 @@ let cli = yargs(hideBin(process.argv))

Log.Default.info("aictrl", {
version: Installation.VERSION,
args: process.argv.slice(2),
args: BashEnvPolicy.explicit(opts) ? { command: "run", bashEnvPolicy: "allowlist" } : process.argv.slice(2),
})

const storageDir = path.join(Global.Path.data, "storage")
Expand Down Expand Up @@ -154,6 +166,12 @@ let cli = yargs(hideBin(process.argv))

cli = cli
.fail((msg, err) => {
const missing = BashEnvPolicy.parserError(cli as typeof cli & BashEnvPolicy.Parser, msg ?? err?.message)
if (missing) throw missing
// Use yargs' actual parse result even when validation fails before middleware.
if (cli.parsed && BashEnvPolicy.explicit(cli.parsed.argv)) {
throw new BashEnvPolicy.ConfigError("Invalid run options; check --bash-env-allowlist and other run arguments.")
}
if (
msg?.startsWith("Unknown argument") ||
msg?.startsWith("Not enough non-option arguments") ||
Expand All @@ -170,42 +188,48 @@ cli = cli
try {
await cli.parse()
} catch (e) {
let data: Record<string, any> = {}
if (e instanceof NamedError) {
const obj = e.toObject()
Object.assign(data, {
...obj.data,
})
}
const policyError = BashEnvPolicy.format(e)
if (policyError) {
process.stderr.write(policyError + EOL)
process.exitCode = 2
} else {
let data: Record<string, any> = {}
if (e instanceof NamedError) {
const obj = e.toObject()
Object.assign(data, {
...obj.data,
})
}

if (e instanceof Error) {
Object.assign(data, {
name: e.name,
message: e.message,
cause: e.cause?.toString(),
stack: e.stack,
})
}
if (e instanceof Error) {
Object.assign(data, {
name: e.name,
message: e.message,
cause: e.cause?.toString(),
stack: e.stack,
})
}

if (e instanceof ResolveMessage) {
Object.assign(data, {
name: e.name,
message: e.message,
code: e.code,
specifier: e.specifier,
referrer: e.referrer,
position: e.position,
importKind: e.importKind,
})
}
Log.Default.error("fatal", data)
const formatted = FormatError(e)
if (formatted) UI.error(formatted)
if (formatted === undefined) {
UI.error("Unexpected error, check log file at " + Log.file() + " for more details" + EOL)
process.stderr.write((e instanceof Error ? e.message : String(e)) + EOL)
if (e instanceof ResolveMessage) {
Object.assign(data, {
name: e.name,
message: e.message,
code: e.code,
specifier: e.specifier,
referrer: e.referrer,
position: e.position,
importKind: e.importKind,
})
}
Log.Default.error("fatal", data)
const formatted = FormatError(e)
if (formatted) UI.error(formatted)
if (formatted === undefined) {
UI.error("Unexpected error, check log file at " + Log.file() + " for more details" + EOL)
process.stderr.write((e instanceof Error ? e.message : String(e)) + EOL)
}
process.exitCode = 1
}
process.exitCode = 1
} finally {
// Some subprocesses don't react properly to SIGTERM and similar signals.
// Most notably, some docker-container-based MCP servers don't handle such signals unless
Expand Down
7 changes: 5 additions & 2 deletions packages/cli/src/shell/shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ import { Flag } from "@/flag/flag"
import { lazy } from "@/util/lazy"
import { Filesystem } from "@/util/filesystem"
import path from "path"
import { spawn, type ChildProcess } from "child_process"
import { spawn } from "child_process"

const SIGKILL_TIMEOUT_MS = 200

export namespace Shell {
export async function killTree(proc: ChildProcess, opts?: { exited?: () => boolean }): Promise<void> {
export async function killTree(
proc: { pid?: number; kill(signal: "SIGTERM" | "SIGKILL"): unknown },
opts?: { exited?: () => boolean },
): Promise<void> {
const pid = proc.pid
if (!pid || opts?.exited?.()) return

Expand Down
Loading
Loading