-
Notifications
You must be signed in to change notification settings - Fork 73
feat(project): wire dev handler #1966
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| # Environment variables for local development. | ||
| # `agentcore dev` loads this file into your agent's process. Values here | ||
| # override anything the CLI injects. This file is gitignored — keep secrets | ||
| # out of version control, but they are safe here. | ||
| # `agentcore project dev` loads this file into your agent's process. Values here | ||
| # override injected values except PORT, FASTMCP_PORT, and LOCAL_DEV, which the | ||
| # CLI owns. This file is gitignored — keep secrets out of version control. | ||
| # | ||
| # Example: | ||
| # MY_API_KEY=... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { afterEach, describe, expect, test } from "bun:test"; | ||
| import { createHash } from "node:crypto"; | ||
| import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; | ||
| import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from "node:fs/promises"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join, resolve } from "node:path"; | ||
| import { InputValidationError, InvalidEnvironmentError } from "../../errors"; | ||
|
|
@@ -11,7 +11,7 @@ import { | |
| type ProcessStreamer, | ||
| type StreamProcessOptions, | ||
| } from "../../io"; | ||
| import type { ProjectRuntime } from "../project/schema"; | ||
| import type { ProjectRuntime } from "../../projectSchemas/runtime"; | ||
| import { ContainerDevRunner } from "./container"; | ||
|
|
||
| type ProcessCall = { | ||
|
|
@@ -61,6 +61,8 @@ function harness( | |
| config: { | ||
| available?: (tool: string, probeArgs?: string[]) => Promise<boolean>; | ||
| stream?: StreamBehavior; | ||
| awsDirectory?: string; | ||
| processEnv?: NodeJS.ProcessEnv; | ||
| } = {}, | ||
| ) { | ||
| const calls: ProcessCall[] = []; | ||
|
|
@@ -77,6 +79,11 @@ function harness( | |
| (async (tool) => { | ||
| return tool === "docker"; | ||
| }), | ||
| awsDirectory: config.awsDirectory ?? join(tmpdir(), "agentcore-container-no-aws"), | ||
| processEnv: config.processEnv ?? { | ||
| AWS_ACCESS_KEY_ID: "test-access-key", | ||
| AWS_SECRET_ACCESS_KEY: "test-secret-key", | ||
| }, | ||
| }), | ||
| }; | ||
| } | ||
|
|
@@ -189,6 +196,10 @@ describe("ContainerDevRunner", () => { | |
| "-p", | ||
| `127.0.0.1:3000:${containerPort}`, | ||
| "-e", | ||
| "AWS_ACCESS_KEY_ID=test-access-key", | ||
| "-e", | ||
| "AWS_SECRET_ACCESS_KEY=test-secret-key", | ||
| "-e", | ||
| "API_KEY=super-secret", | ||
| "-e", | ||
| `PORT=${containerPort}`, | ||
|
|
@@ -202,6 +213,35 @@ describe("ContainerDevRunner", () => { | |
| expect(run.options.redactedCommand?.join(" ")).not.toContain("super-secret"); | ||
| }); | ||
|
|
||
| test("uses a shared AWS config and rejects missing credentials", async () => { | ||
| const projectRuntime = runtime(); | ||
| const root = await projectRoot(projectRuntime); | ||
| const awsDirectory = join(root, ".aws"); | ||
| await mkdir(awsDirectory); | ||
| await writeFile(join(awsDirectory, "config"), "[profile sandbox]\nregion=us-east-1\n"); | ||
| const { calls, runner } = harness({ | ||
| awsDirectory, | ||
| processEnv: { AWS_PROFILE: "sandbox", AWS_REGION: "us-east-1" }, | ||
| }); | ||
|
|
||
| await collect(runner.run(input(root, projectRuntime))); | ||
|
|
||
| const run = commandCall(calls, "run"); | ||
| expect(run.command).toContain(`${awsDirectory}:/aws-config:ro`); | ||
| expect(run.command).toContain("AWS_PROFILE=sandbox"); | ||
| expect(run.command).toContain("AWS_CONFIG_FILE=/aws-config/config"); | ||
| expect(run.options.redactedCommand?.join(" ")).not.toContain("sandbox"); | ||
|
|
||
| const missing = harness({ | ||
| awsDirectory: join(root, "missing-aws"), | ||
| processEnv: {}, | ||
| }); | ||
| await expect(collect(missing.runner.run(input(root, projectRuntime)))).rejects.toThrow( | ||
| "Unable to resolve AWS credentials for the container", | ||
| ); | ||
| expect(missing.calls).toHaveLength(0); | ||
| }); | ||
|
|
||
| test("preserves an existing build context .dockerignore", async () => { | ||
| const projectRuntime = runtime({ buildContextPath: "." }); | ||
| const root = await projectRoot(projectRuntime); | ||
|
|
@@ -409,6 +449,31 @@ describe("ContainerDevRunner", () => { | |
| expect(calls.map(({ command }) => command[1])).toEqual(["rm"]); | ||
| }); | ||
|
|
||
| test("rejects build contexts outside the project root, including symlinks", async () => { | ||
| const root = await mkdtemp(join(tmpdir(), "agentcore-container-")); | ||
| const outside = await mkdtemp(join(tmpdir(), "agentcore-container-outside-")); | ||
| tempDirectories.push(root, outside); | ||
| await symlink(outside, join(root, "linked"), process.platform === "win32" ? "junction" : "dir"); | ||
| const probes: string[] = []; | ||
|
|
||
| for (const buildContextPath of ["..", "linked"]) { | ||
| const { calls, runner } = harness({ | ||
| available: async (tool) => { | ||
| probes.push(tool); | ||
| return true; | ||
| }, | ||
| }); | ||
|
|
||
| await expect(collect(runner.run(input(root, runtime({ buildContextPath }))))).rejects.toThrow( | ||
| "container build context must be within the project root", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we validate the error types in these tests? |
||
| ); | ||
| expect(calls).toHaveLength(0); | ||
| } | ||
|
|
||
| expect(probes).toHaveLength(0); | ||
| await expect(readFile(join(outside, ".dockerignore"), "utf8")).rejects.toThrow(); | ||
| }); | ||
|
|
||
| test("rejects a build context that is not a directory", async () => { | ||
| const root = await mkdtemp(join(tmpdir(), "agentcore-container-")); | ||
| tempDirectories.push(root); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { createHash } from "node:crypto"; | ||
| import { existsSync, statSync, writeFileSync } from "node:fs"; | ||
| import { join, resolve } from "node:path"; | ||
| import { existsSync, realpathSync, statSync, writeFileSync } from "node:fs"; | ||
| import { homedir } from "node:os"; | ||
| import { isAbsolute, join, relative, resolve, sep } from "node:path"; | ||
| import { InputValidationError, InvalidEnvironmentError } from "../../errors"; | ||
| import type { DevEvent, DevRunner, DevServerInput } from "../../handlers/project/dev/types"; | ||
| import { | ||
|
|
@@ -10,8 +11,17 @@ import { | |
| type ProcessStreamer, | ||
| type StreamProcessOptions, | ||
| } from "../../io"; | ||
| import { DEV_PORTS } from "./port"; | ||
|
|
||
| const CONTAINER_TOOLS = ["docker", "podman", "finch"] as const; | ||
| const AWS_ENV_KEYS = [ | ||
| "AWS_ACCESS_KEY_ID", | ||
| "AWS_SECRET_ACCESS_KEY", | ||
| "AWS_SESSION_TOKEN", | ||
| "AWS_REGION", | ||
| "AWS_DEFAULT_REGION", | ||
| "AWS_PROFILE", | ||
| ] as const; | ||
| const CLEANUP_TIMEOUT_MS = 2_000; | ||
| const DOCKERFILE_NAME = "Dockerfile"; | ||
| const CONTAINER_RUNTIME_INSTALL_HINT = | ||
|
|
@@ -44,15 +54,21 @@ type ToolAvailable = typeof toolAvailable; | |
| type ContainerDevRunnerConfig = { | ||
| streamProcess?: ProcessStreamer; | ||
| toolAvailable?: ToolAvailable; | ||
| awsDirectory?: string; | ||
| processEnv?: NodeJS.ProcessEnv; | ||
| }; | ||
|
|
||
| export class ContainerDevRunner implements DevRunner { | ||
| private readonly streamProcess: ProcessStreamer; | ||
| private readonly toolAvailable: ToolAvailable; | ||
| private readonly awsDirectory: string; | ||
| private readonly processEnv: NodeJS.ProcessEnv; | ||
|
|
||
| constructor(config: ContainerDevRunnerConfig = {}) { | ||
| this.streamProcess = config.streamProcess ?? streamProcess; | ||
| this.toolAvailable = config.toolAvailable ?? toolAvailable; | ||
| this.awsDirectory = config.awsDirectory ?? join(homedir(), ".aws"); | ||
| this.processEnv = config.processEnv ?? process.env; | ||
| } | ||
|
|
||
| public async *run(input: DevServerInput): AsyncGenerator<DevEvent, void> { | ||
|
|
@@ -65,12 +81,36 @@ export class ContainerDevRunner implements DevRunner { | |
| throw new InputValidationError(`container build context directory not found: ${context}`); | ||
| } | ||
|
|
||
| const canonicalContext = realpathSync(context); | ||
| const relativeContext = relative(realpathSync(input.projectRoot), canonicalContext); | ||
| if ( | ||
| relativeContext === ".." || | ||
| relativeContext.startsWith(`..${sep}`) || | ||
| isAbsolute(relativeContext) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in what cases does |
||
| ) { | ||
| throw new InputValidationError( | ||
| `container build context must be within the project root: ${canonicalContext}`, | ||
| ); | ||
| } | ||
|
|
||
| const dockerfile = input.runtime.dockerfile ?? DOCKERFILE_NAME; | ||
| const dockerfilePath = join(context, dockerfile); | ||
| if (!isFile(dockerfilePath)) { | ||
| throw new InputValidationError(`container Dockerfile not found: ${dockerfilePath}`); | ||
| } | ||
|
|
||
| const hasAwsCredentials = Boolean( | ||
| (input.env?.AWS_ACCESS_KEY_ID ?? this.processEnv.AWS_ACCESS_KEY_ID) && | ||
| (input.env?.AWS_SECRET_ACCESS_KEY ?? this.processEnv.AWS_SECRET_ACCESS_KEY), | ||
| ); | ||
| const hasAwsConfig = existsSync(this.awsDirectory); | ||
| if (!hasAwsCredentials && !hasAwsConfig) { | ||
| throw new InputValidationError( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think it would be useful to distinguish the missing credentials case from the invalid inputs case in telemetry with a separate error type here? |
||
| "Unable to resolve AWS credentials for the container. Configure AWS credentials " + | ||
| "or set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY, then retry.", | ||
| ); | ||
| } | ||
|
|
||
| const tool = await this.resolveContainerTool(input.signal); | ||
| input.signal.throwIfAborted(); | ||
| if (input.runtime.buildContextPath) { | ||
|
|
@@ -116,15 +156,23 @@ export class ContainerDevRunner implements DevRunner { | |
| yield { type: "status", message: `Building image with ${tool}` }; | ||
| yield* this.streamProcess(buildCommand, buildOptions); | ||
|
|
||
| const containerPort = portForProtocol(input.runtime.protocol); | ||
| const forwardedEnv: Record<string, string> = { | ||
| ...input.env, | ||
| const containerPort = DEV_PORTS[input.runtime.protocol ?? "HTTP"]; | ||
| const forwardedEnv: Record<string, string> = {}; | ||
| for (const key of AWS_ENV_KEYS) { | ||
| if (this.processEnv[key]) forwardedEnv[key] = this.processEnv[key]; | ||
| } | ||
| Object.assign(forwardedEnv, input.env, { | ||
| PORT: String(containerPort), | ||
| LOCAL_DEV: "1", | ||
| }; | ||
| }); | ||
| if (input.runtime.protocol === "MCP") { | ||
| forwardedEnv.FASTMCP_PORT = String(containerPort); | ||
| } | ||
| const awsMount = hasAwsConfig ? ["-v", `${this.awsDirectory}:/aws-config:ro`] : []; | ||
| if (awsMount.length) { | ||
| forwardedEnv.AWS_CONFIG_FILE = "/aws-config/config"; | ||
| forwardedEnv.AWS_SHARED_CREDENTIALS_FILE = "/aws-config/credentials"; | ||
| } | ||
| const envFlags = Object.entries(forwardedEnv).flatMap(([key, value]) => [ | ||
| "-e", | ||
| `${key}=${value}`, | ||
|
|
@@ -141,6 +189,7 @@ export class ContainerDevRunner implements DevRunner { | |
| containerName, | ||
| "-p", | ||
| `127.0.0.1:${input.port}:${containerPort}`, | ||
| ...awsMount, | ||
| ...envFlags, | ||
| imageTag, | ||
| ]; | ||
|
|
@@ -158,6 +207,7 @@ export class ContainerDevRunner implements DevRunner { | |
| containerName, | ||
| "-p", | ||
| `127.0.0.1:${input.port}:${containerPort}`, | ||
| ...awsMount, | ||
| ...redactedEnvFlags, | ||
| imageTag, | ||
| ], | ||
|
|
@@ -204,12 +254,6 @@ export class ContainerDevRunner implements DevRunner { | |
| } | ||
| } | ||
|
|
||
| function portForProtocol(protocol: DevServerInput["runtime"]["protocol"]): number { | ||
| if (protocol === "MCP") return 8000; | ||
| if (protocol === "A2A") return 9000; | ||
| return 8080; | ||
| } | ||
|
|
||
| function isDirectory(path: string): boolean { | ||
| try { | ||
| return statSync(path).isDirectory(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import type { PortChecker } from "../../io"; | ||
| import { resolveDevPort } from "./port"; | ||
|
|
||
| const signal = new AbortController().signal; | ||
|
|
||
| describe("resolveDevPort", () => { | ||
| test.each([ | ||
| ["HTTP", 8080], | ||
| ["AGUI", 8080], | ||
| ["MCP", 8000], | ||
| ["A2A", 9000], | ||
| ] as const)("uses the %s default", async (protocol, port) => { | ||
| expect(await resolveDevPort(protocol, undefined, async () => true, signal)).toEqual({ | ||
| port, | ||
| requestedPort: port, | ||
| }); | ||
| }); | ||
|
|
||
| test("walks up from occupied defaults", async () => { | ||
| const checked: number[] = []; | ||
| const check: PortChecker = async (port) => { | ||
| checked.push(port); | ||
| return port === 8002; | ||
| }; | ||
|
|
||
| expect(await resolveDevPort("MCP", undefined, check, signal)).toEqual({ | ||
| port: 8002, | ||
| requestedPort: 8000, | ||
| }); | ||
| expect(checked).toEqual([8000, 8001, 8002]); | ||
| }); | ||
|
|
||
| test("accepts a free explicit port and rejects an occupied one", async () => { | ||
| expect(await resolveDevPort("A2A", 4567, async () => true, signal)).toEqual({ | ||
| port: 4567, | ||
| requestedPort: 4567, | ||
| }); | ||
| await expect(resolveDevPort("A2A", 4567, async () => false, signal)).rejects.toThrow( | ||
| "lsof -i :4567", | ||
| ); | ||
| }); | ||
|
|
||
| test("bounds the default search", async () => { | ||
| await expect(resolveDevPort("HTTP", undefined, async () => false, signal)).rejects.toThrow( | ||
| "No free port found in range 8080-8179", | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { InputValidationError } from "../../errors"; | ||
| import type { ProjectRuntime } from "../../projectSchemas/runtime"; | ||
| import type { PortChecker } from "../../io"; | ||
|
|
||
| const MAX_PORT_ATTEMPTS = 100; | ||
| export const DEV_PORTS = { HTTP: 8080, AGUI: 8080, MCP: 8000, A2A: 9000 } as const; | ||
|
|
||
| export type DevPort = { | ||
| port: number; | ||
| requestedPort: number; | ||
| }; | ||
|
|
||
| function portInUse(port: number, suffix = ""): InputValidationError { | ||
| return new InputValidationError( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be simpler to make this function its own error type? |
||
| `Port ${port} is already in use. Find the process with ` + | ||
| `'lsof -i :${port}' (macOS/Linux) or 'netstat -ano | findstr :${port}' (Windows), ` + | ||
| `then stop it${suffix}.`, | ||
| ); | ||
| } | ||
|
|
||
| export async function resolveDevPort( | ||
| protocol: ProjectRuntime["protocol"], | ||
| explicitPort: number | undefined, | ||
| checkPort: PortChecker, | ||
| signal: AbortSignal, | ||
| ): Promise<DevPort> { | ||
| const defaultPort = DEV_PORTS[protocol ?? "HTTP"]; | ||
| const requestedPort = explicitPort ?? defaultPort; | ||
|
|
||
| if (await checkPort(requestedPort, signal)) { | ||
| return { port: requestedPort, requestedPort }; | ||
| } | ||
|
|
||
| if (explicitPort !== undefined) { | ||
| throw portInUse(requestedPort, " or choose a different --port"); | ||
| } | ||
|
|
||
| for (let port = requestedPort + 1; port < requestedPort + MAX_PORT_ATTEMPTS; port++) { | ||
| if (await checkPort(port, signal)) return { port, requestedPort }; | ||
| } | ||
|
|
||
| throw new InputValidationError( | ||
| `No free port found in range ${requestedPort}-${requestedPort + MAX_PORT_ATTEMPTS - 1}.`, | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,7 +135,7 @@ export class EmbeddedAssetNotFoundError extends AgentCoreCLIError { | |
| } | ||
| } | ||
|
|
||
| export class RuntimeInvokeInterruptedError extends AgentCoreCLIError { | ||
| export class CommandInterruptedError extends AgentCoreCLIError { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is doing something similar to https://github.com/aws/agentcore-cli/pull/1986/changes (haven't fully reviewed that one yet) |
||
| readonly reported: boolean; | ||
|
|
||
| constructor(cause?: unknown, reported = false) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why exactly is this needed? is there a component bun is missing that we need here?