Skip to content

Commit 113dcde

Browse files
committed
fix(dev): address container runtime feedback
1 parent 6743887 commit 113dcde

4 files changed

Lines changed: 147 additions & 43 deletions

File tree

src/core/dev/container.test.ts

Lines changed: 68 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { createHash } from "node:crypto";
33
import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
44
import { tmpdir } from "node:os";
55
import { join, resolve } from "node:path";
6-
import { InputValidationError } from "../../errors";
6+
import { InputValidationError, InvalidEnvironmentError } from "../../errors";
77
import type { DevEvent, DevServerInput } from "../../handlers/project/dev/types";
88
import {
99
MissingToolError,
@@ -125,7 +125,7 @@ function commandCall(calls: ProcessCall[], operation: "build" | "run"): ProcessC
125125
}
126126

127127
describe("ContainerDevRunner", () => {
128-
test("builds with a widened context and keeps build arg values out of argv", async () => {
128+
test("builds with a widened context and redacts build arg values from errors", async () => {
129129
const projectRuntime = runtime({
130130
buildContextPath: ".",
131131
dockerfile: "docker/Dockerfile",
@@ -148,18 +148,17 @@ describe("ContainerDevRunner", () => {
148148
"-t",
149149
imageTag(root),
150150
"--build-arg",
151-
"AGENT_NAME",
151+
"AGENT_NAME=hello-world",
152152
"--build-arg",
153-
"TARGET",
153+
"TARGET=development",
154154
".",
155155
]);
156156
expect(build.options.cwd).toBe(root);
157-
expect(build.options.env).toMatchObject({
158-
AGENT_NAME: "hello-world",
159-
TARGET: "development",
160-
});
161-
expect(build.command.join(" ")).not.toContain("hello-world");
162-
expect(build.command.join(" ")).not.toContain("development");
157+
expect(build.options.env).toBe(process.env);
158+
expect(build.options.redactedCommand).toContain("AGENT_NAME=<redacted>");
159+
expect(build.options.redactedCommand).toContain("TARGET=<redacted>");
160+
expect(build.options.redactedCommand?.join(" ")).not.toContain("hello-world");
161+
expect(build.options.redactedCommand?.join(" ")).not.toContain("development");
163162

164163
const dockerignore = await readFile(join(root, ".dockerignore"), "utf8");
165164
for (const pattern of [".env", "**/.env", "**/node_modules", "agentcore/"]) {
@@ -190,21 +189,17 @@ describe("ContainerDevRunner", () => {
190189
"-p",
191190
`127.0.0.1:3000:${containerPort}`,
192191
"-e",
193-
"API_KEY",
192+
"API_KEY=super-secret",
194193
"-e",
195-
"PORT",
194+
`PORT=${containerPort}`,
196195
"-e",
197-
"LOCAL_DEV",
198-
...(protocol === "MCP" ? ["-e", "FASTMCP_PORT"] : []),
196+
"LOCAL_DEV=1",
197+
...(protocol === "MCP" ? ["-e", "FASTMCP_PORT=8000"] : []),
199198
imageTag(root),
200199
]);
201-
expect(run.options.env).toMatchObject({
202-
API_KEY: "super-secret",
203-
PORT: String(containerPort),
204-
LOCAL_DEV: "1",
205-
});
206-
expect(run.options.env?.FASTMCP_PORT).toBe(protocol === "MCP" ? "8000" : undefined);
207-
expect(run.command.join(" ")).not.toContain("super-secret");
200+
expect(run.options.env).toBe(process.env);
201+
expect(run.options.redactedCommand).toContain("API_KEY=<redacted>");
202+
expect(run.options.redactedCommand?.join(" ")).not.toContain("super-secret");
208203
});
209204

210205
test("preserves an existing build context .dockerignore", async () => {
@@ -237,6 +232,33 @@ describe("ContainerDevRunner", () => {
237232
expect(containerName(firstRoot)).not.toBe(containerName(secondRoot));
238233
});
239234

235+
test("limits image names to two consecutive underscores", async () => {
236+
const projectRuntime = runtime({ name: "Hello___World" });
237+
const root = await projectRoot(projectRuntime);
238+
const { calls, runner } = harness();
239+
240+
await collect(runner.run(input(root, projectRuntime)));
241+
242+
expect(commandCall(calls, "build").command).toContain(
243+
`agentcore-dev/hello__world-${hashString(resolve(root))}`,
244+
);
245+
});
246+
247+
test("keeps app variables out of the container CLI environment", async () => {
248+
const projectRuntime = runtime();
249+
const root = await projectRoot(projectRuntime);
250+
const { calls, runner } = harness();
251+
const runInput = input(root, projectRuntime);
252+
runInput.env = { ...runInput.env, DOCKER_HOST: "tcp://application-value" };
253+
254+
await collect(runner.run(runInput));
255+
256+
const run = commandCall(calls, "run");
257+
expect(run.command).toContain("DOCKER_HOST=tcp://application-value");
258+
expect(run.options.env).toBe(process.env);
259+
expect(run.options.redactedCommand).toContain("DOCKER_HOST=<redacted>");
260+
});
261+
240262
test("selects the first tool that supports container builds", async () => {
241263
const probes: Array<[string, string[] | undefined]> = [];
242264
const projectRuntime = runtime();
@@ -286,6 +308,31 @@ describe("ContainerDevRunner", () => {
286308
expect(commandCall(calls, "build").command[0]).toBe("finch");
287309
});
288310

311+
test("passes explicit build arg values to finch", async () => {
312+
const projectRuntime = runtime({ customDockerBuildArgs: { AGENT_NAME: "hello-world" } });
313+
const root = await projectRoot(projectRuntime);
314+
const { calls, runner } = harness({
315+
available: async (tool) => tool === "finch",
316+
});
317+
318+
await collect(runner.run(input(root, projectRuntime)));
319+
320+
expect(commandCall(calls, "build").command).toContain("AGENT_NAME=hello-world");
321+
});
322+
323+
test("suggests initializing the Finch VM when its build probe fails", async () => {
324+
const projectRuntime = runtime();
325+
const root = await projectRoot(projectRuntime);
326+
const { runner } = harness({
327+
available: async (tool, probeArgs) => tool === "finch" && probeArgs === undefined,
328+
});
329+
330+
const promise = collect(runner.run(input(root, projectRuntime)));
331+
332+
await expect(promise).rejects.toBeInstanceOf(InvalidEnvironmentError);
333+
await expect(promise).rejects.toThrow("finch vm init");
334+
});
335+
289336
test("throws a useful error when no container runtime is available", async () => {
290337
const projectRuntime = runtime();
291338
const root = await projectRoot(projectRuntime);

src/core/dev/container.ts

Lines changed: 58 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { createHash } from "node:crypto";
22
import { existsSync, statSync, writeFileSync } from "node:fs";
33
import { join, resolve } from "node:path";
4-
import { InputValidationError } from "../../errors";
4+
import { InputValidationError, InvalidEnvironmentError } from "../../errors";
55
import type { DevEvent, DevRunner, DevServerInput } from "../../handlers/project/dev/types";
66
import {
77
MissingToolError,
@@ -82,24 +82,39 @@ export class ContainerDevRunner implements DevRunner {
8282

8383
const runtimeName = input.runtime.name.toLowerCase();
8484
const projectId = hashString(resolve(input.projectRoot));
85-
const imageTag = `agentcore-dev/${runtimeName}-${projectId}`;
85+
const imageTag = `agentcore-dev/${sanitizeImageNameComponent(runtimeName)}-${projectId}`;
8686
const containerName = `agentcore-dev-${runtimeName}-${projectId}`;
8787
await this.removeContainer(tool, containerName, context);
8888
input.signal.throwIfAborted();
8989

9090
const buildArgs = input.runtime.customDockerBuildArgs ?? {};
91-
const buildArgFlags = Object.keys(buildArgs).flatMap((key) => ["--build-arg", key]);
91+
const buildArgFlags = Object.entries(buildArgs).flatMap(([key, value]) => [
92+
"--build-arg",
93+
`${key}=${value}`,
94+
]);
95+
const redactedBuildArgFlags = Object.keys(buildArgs).flatMap((key) => [
96+
"--build-arg",
97+
`${key}=<redacted>`,
98+
]);
99+
const buildCommand = [tool, "build", "-f", dockerfile, "-t", imageTag, ...buildArgFlags, "."];
92100
const buildOptions: StreamProcessOptions = {
93101
cwd: context,
94-
env: { ...process.env, ...buildArgs },
102+
env: process.env,
103+
redactedCommand: [
104+
tool,
105+
"build",
106+
"-f",
107+
dockerfile,
108+
"-t",
109+
imageTag,
110+
...redactedBuildArgFlags,
111+
".",
112+
],
95113
signal: input.signal,
96114
};
97115

98116
yield { type: "status", message: `Building image with ${tool}` };
99-
yield* this.streamProcess(
100-
[tool, "build", "-f", dockerfile, "-t", imageTag, ...buildArgFlags, "."],
101-
buildOptions,
102-
);
117+
yield* this.streamProcess(buildCommand, buildOptions);
103118

104119
const containerPort = portForProtocol(input.runtime.protocol);
105120
const forwardedEnv: Record<string, string> = {
@@ -110,28 +125,44 @@ export class ContainerDevRunner implements DevRunner {
110125
if (input.runtime.protocol === "MCP") {
111126
forwardedEnv.FASTMCP_PORT = String(containerPort);
112127
}
113-
const envNameFlags = Object.keys(forwardedEnv).flatMap((key) => ["-e", key]);
128+
const envFlags = Object.entries(forwardedEnv).flatMap(([key, value]) => [
129+
"-e",
130+
`${key}=${value}`,
131+
]);
132+
const redactedEnvFlags = Object.keys(forwardedEnv).flatMap((key) => [
133+
"-e",
134+
`${key}=<redacted>`,
135+
]);
136+
const runCommand = [
137+
tool,
138+
"run",
139+
"--rm",
140+
"--name",
141+
containerName,
142+
"-p",
143+
`127.0.0.1:${input.port}:${containerPort}`,
144+
...envFlags,
145+
imageTag,
146+
];
114147

115148
yield { type: "status", message: "Starting container" };
116149
try {
117-
yield* this.streamProcess(
118-
[
150+
yield* this.streamProcess(runCommand, {
151+
cwd: context,
152+
env: process.env,
153+
redactedCommand: [
119154
tool,
120155
"run",
121156
"--rm",
122157
"--name",
123158
containerName,
124159
"-p",
125160
`127.0.0.1:${input.port}:${containerPort}`,
126-
...envNameFlags,
161+
...redactedEnvFlags,
127162
imageTag,
128163
],
129-
{
130-
cwd: context,
131-
env: { ...process.env, ...forwardedEnv },
132-
signal: input.signal,
133-
},
134-
);
164+
signal: input.signal,
165+
});
135166
} finally {
136167
await this.removeContainer(tool, containerName, context);
137168
}
@@ -146,6 +177,11 @@ export class ContainerDevRunner implements DevRunner {
146177
const canBuild = await this.toolAvailable(tool, ["build", "--help"]);
147178
signal.throwIfAborted();
148179
if (canBuild) return tool;
180+
if (tool === "finch") {
181+
throw new InvalidEnvironmentError(
182+
"Finch is installed but its VM is not initialized. Run 'finch vm init' and retry.",
183+
);
184+
}
149185
}
150186
throw new MissingToolError("container runtime", CONTAINER_RUNTIME_INSTALL_HINT);
151187
}
@@ -200,3 +236,7 @@ function ensureBuildContextDockerignore(context: string): string | undefined {
200236
function hashString(value: string): string {
201237
return createHash("sha256").update(value).digest("hex").slice(0, 12);
202238
}
239+
240+
function sanitizeImageNameComponent(value: string): string {
241+
return value.replace(/_{3,}/g, "__");
242+
}

src/io/exec.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,20 @@ describe("streamProcess", () => {
111111
await expect(iterator.next()).rejects.toThrow(/exit code 3/);
112112
});
113113

114+
test("redacts sensitive command arguments from process errors", async () => {
115+
const failing = await script("stream-redacted-fail.js", "process.exit(3)");
116+
const iterator = streamProcess(["node", failing, "super-secret"], {
117+
cwd: process.cwd(),
118+
redactedCommand: ["node", failing, "<redacted>"],
119+
});
120+
121+
const error = await iterator.next().catch((caught: unknown) => caught);
122+
123+
expect(error).toBeInstanceOf(ProcessFailedError);
124+
expect(String(error)).toContain("<redacted>");
125+
expect(String(error)).not.toContain("super-secret");
126+
});
127+
114128
test("throws ProcessFailedError when the executable cannot spawn", async () => {
115129
await expect(
116130
collect(streamProcess(["definitely-not-a-real-tool-xyz"], { cwd: process.cwd() })),

src/io/exec.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ export type StreamProcessOptions = {
6666
cwd: string;
6767
env?: NodeJS.ProcessEnv;
6868
signal?: AbortSignal;
69+
/** Command rendered in errors when the actual arguments contain sensitive values. */
70+
redactedCommand?: string[];
6971
/** Required on Windows for command scripts such as npm.cmd. */
7072
shell?: boolean;
7173
};
@@ -115,8 +117,9 @@ export async function* streamProcess(
115117
options: StreamProcessOptions,
116118
): AsyncGenerator<ProcessEvent, void> {
117119
const [executable, ...args] = command;
120+
const errorCommand = options.redactedCommand ?? command;
118121
if (!executable) {
119-
throw new ProcessFailedError(command, options.cwd, null, "command is empty");
122+
throw new ProcessFailedError(errorCommand, options.cwd, null, "command is empty");
120123
}
121124
if (options.signal?.aborted) throw abortReason(options.signal);
122125

@@ -130,7 +133,7 @@ export async function* streamProcess(
130133
detached: !useShell,
131134
});
132135
} catch (error) {
133-
throw new ProcessFailedError(command, options.cwd, null, String(error));
136+
throw new ProcessFailedError(errorCommand, options.cwd, null, String(error));
134137
}
135138

136139
const events: ProcessEvent[] = [];
@@ -203,12 +206,12 @@ export async function* streamProcess(
203206

204207
if (options.signal?.aborted) throw abortReason(options.signal);
205208
if (spawnError) {
206-
throw new ProcessFailedError(command, options.cwd, null, String(spawnError));
209+
throw new ProcessFailedError(errorCommand, options.cwd, null, String(spawnError));
207210
}
208211
if (exitCode !== 0) {
209212
const signalMessage = exitSignal ? `terminated by ${exitSignal}` : "";
210213
throw new ProcessFailedError(
211-
command,
214+
errorCommand,
212215
options.cwd,
213216
exitCode,
214217
[...recentOutput, signalMessage].filter(Boolean).join("\n"),

0 commit comments

Comments
 (0)