diff --git a/src/cli/tui/screens/agent/AddAgentScreen.tsx b/src/cli/tui/screens/agent/AddAgentScreen.tsx index c8961c065..9dab7531e 100644 --- a/src/cli/tui/screens/agent/AddAgentScreen.tsx +++ b/src/cli/tui/screens/agent/AddAgentScreen.tsx @@ -1,4 +1,4 @@ -import { APP_DIR, ConfigIO } from '../../../../lib'; +import { APP_DIR, ConfigIO, getWorkingDirectory } from '../../../../lib'; import type { ModelProvider, NetworkMode, RuntimeAuthorizerType, SDKFramework } from '../../../../schema'; import { AgentNameSchema, DEFAULT_MODEL_IDS, LIFECYCLE_TIMEOUT_MAX, LIFECYCLE_TIMEOUT_MIN } from '../../../../schema'; import { listBedrockAgentAliases, listBedrockAgents } from '../../../aws/bedrock-import'; @@ -45,7 +45,7 @@ import { } from './types'; import { Box, Text, useInput } from 'ink'; import Spinner from 'ink-spinner'; -import { basename, resolve } from 'path'; +import { basename } from 'path'; import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; // Helper to get provider display name and env var name from ModelProvider @@ -1086,12 +1086,15 @@ export function AddAgentScreen({ existingAgentNames, onComplete, onExit }: AddAg {byoStep === 'dockerfile' && ( { - setByoConfig(c => ({ ...c, dockerfile: value ? basename(value) : '' })); + // Preserve the full path so downstream copy logic in useAddAgent can + // resolve it against the invocation cwd. The persisted spec only + // stores the basename (applied at serialization time). + setByoConfig(c => ({ ...c, dockerfile: value ?? '' })); goToNextByoStep('dockerfile'); }} onCancel={handleByoBack} @@ -1295,7 +1298,7 @@ export function AddAgentScreen({ existingAgentNames, onComplete, onExit }: AddAg value: BUILD_TYPE_OPTIONS.find(o => o.id === byoConfig.buildType)?.title ?? byoConfig.buildType, }, ...(byoConfig.buildType === 'Container' && byoConfig.dockerfile - ? [{ label: 'Dockerfile', value: byoConfig.dockerfile }] + ? [{ label: 'Dockerfile', value: basename(byoConfig.dockerfile) }] : []), { label: 'Model Provider', diff --git a/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts new file mode 100644 index 000000000..136f71512 --- /dev/null +++ b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts @@ -0,0 +1,231 @@ +import { resolveDockerfileSource, validateDockerfileSource } from '../useAddAgent'; +import { resolve, win32 } from 'path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { resolve: win32Resolve, basename: win32Basename } = win32; + +describe('resolveDockerfileSource', () => { + it('returns shouldCopy=false when value is undefined', () => { + expect(resolveDockerfileSource(undefined, '/some/cwd')).toEqual({ shouldCopy: false }); + }); + + it('returns shouldCopy=false when value is empty string', () => { + expect(resolveDockerfileSource('', '/some/cwd')).toEqual({ shouldCopy: false }); + }); + + it('returns shouldCopy=false for a bare filename (no path separators)', () => { + // A bare filename refers to the file already in place; no copy needed. + expect(resolveDockerfileSource('Dockerfile', '/some/cwd')).toEqual({ shouldCopy: false }); + expect(resolveDockerfileSource('my.Dockerfile', '/some/cwd')).toEqual({ shouldCopy: false }); + }); + + it('resolves a relative path against the provided cwd (not the project root)', () => { + // This is the bug from issue #1128: previously the path resolved against + // /, now it resolves against the invocation cwd. + const result = resolveDockerfileSource('./my.Dockerfile', '/home/user/cwd'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve('/home/user/cwd', './my.Dockerfile')); + expect(result.filename).toBe('my.Dockerfile'); + }); + + it('resolves a nested relative path against cwd', () => { + const result = resolveDockerfileSource('subdir/Custom.Dockerfile', '/home/user'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve('/home/user', 'subdir/Custom.Dockerfile')); + expect(result.filename).toBe('Custom.Dockerfile'); + }); + + it('handles parent-relative paths against cwd', () => { + const result = resolveDockerfileSource('../sibling/Dockerfile', '/home/user/project'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve('/home/user/project', '../sibling/Dockerfile')); + expect(result.filename).toBe('Dockerfile'); + }); + + it('returns absolute paths verbatim', () => { + const result = resolveDockerfileSource('/absolute/path/to/Dockerfile.prod', '/some/cwd'); + expect(result.shouldCopy).toBe(true); + // resolve() is a no-op on absolute paths. + expect(result.sourcePath).toBe('/absolute/path/to/Dockerfile.prod'); + expect(result.filename).toBe('Dockerfile.prod'); + }); + + it('strips directory components, returning only the basename for filename', () => { + const result = resolveDockerfileSource('a/b/c/MyDocker.file', '/cwd'); + expect(result.filename).toBe('MyDocker.file'); + }); + + describe('cross-platform path-separator detection', () => { + // These tests lock in the *classification* contract (i.e. `shouldCopy`) + // for inputs containing backslashes, which is the value-add of detecting + // both separators. Note: on a POSIX test runner the platform-default + // `path` module does not interpret '\\' as a separator, so + // `basename('subdir\\Dockerfile')` returns the literal string verbatim. + // Genuine end-to-end Windows path resolution is exercised below using + // `path.win32` directly. + it('classifies a backslash-containing relative path as a path-to-copy', () => { + // On Windows, users may enter paths like 'subdir\\Dockerfile'. + // Without backslash detection, this would be misclassified as a bare + // filename and silently skip the copy step (data-loss bug). + const result = resolveDockerfileSource('subdir\\Dockerfile', '/cwd'); + expect(result.shouldCopy).toBe(true); + }); + + it('classifies a dot-prefixed backslash path as a path-to-copy', () => { + const result = resolveDockerfileSource('.\\sub\\My.Dockerfile', '/cwd'); + expect(result.shouldCopy).toBe(true); + }); + + it('still classifies a bare filename with no separators as not-a-path', () => { + // Sanity check: backslash detection must not over-trigger on plain + // filenames containing dots. + expect(resolveDockerfileSource('Dockerfile.dev', '/cwd').shouldCopy).toBe(false); + }); + + it('extracts the correct basename from a Windows-style path under win32 semantics', () => { + // This documents the expected behavior on a real Windows host. We + // simulate it by computing the expected values via `path.win32` + // directly: on Windows, node's default `path` IS `path.win32`, so + // resolve/basename understand backslashes. + const input = 'subdir\\Custom.Dockerfile'; + const expectedSourcePath = win32Resolve('C:\\cwd', input); + const expectedFilename = win32Basename(expectedSourcePath); + expect(expectedFilename).toBe('Custom.Dockerfile'); + // The actual helper uses platform-default path resolution, so this + // assertion is also platform-dependent. We assert on classification + // only here — basename correctness on Windows is provided by node's + // `path` module itself, which we trust. + expect(resolveDockerfileSource(input, 'C:\\cwd').shouldCopy).toBe(true); + }); + }); + + describe('default cwd parameter (getWorkingDirectory)', () => { + // Save and restore INIT_CWD manually rather than relying on the + // `vi.stubEnv(name, undefined)` deletion contract, which is a vitest + // implementation detail that has shifted across versions. + let savedInitCwd: string | undefined; + + beforeEach(() => { + savedInitCwd = process.env.INIT_CWD; + }); + + afterEach(() => { + if (savedInitCwd === undefined) { + delete process.env.INIT_CWD; + } else { + process.env.INIT_CWD = savedInitCwd; + } + }); + + it('uses INIT_CWD when set (npm/bun script invocation case)', () => { + // Set INIT_CWD to a known sentinel and verify the helper actually + // routes through getWorkingDirectory() rather than process.cwd(). + process.env.INIT_CWD = '/sentinel/init/cwd'; + const result = resolveDockerfileSource('./local.Dockerfile'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve('/sentinel/init/cwd', './local.Dockerfile')); + }); + + it('falls back to process.cwd() when INIT_CWD is unset', () => { + // When INIT_CWD is unset, getWorkingDirectory() falls back to + // process.cwd(). + delete process.env.INIT_CWD; + const result = resolveDockerfileSource('./local.Dockerfile'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve(process.cwd(), './local.Dockerfile')); + }); + + it('treats INIT_CWD="" (empty string) as set, not as unset (?? semantics)', () => { + // getWorkingDirectory() uses the nullish coalescing operator (??), + // which does NOT treat empty string as nullish. This test documents + // that contract: INIT_CWD='' is honored as the working directory. + // The resulting resolve('', './local.Dockerfile') is equivalent to + // resolving against process.cwd() (since resolve('', x) is + // process.cwd() + x), so we verify the value goes through the + // INIT_CWD branch by asserting it matches resolve('', ...) (which + // is process.cwd() + ...) — this is observationally identical to + // the unset case but documents intent. + process.env.INIT_CWD = ''; + const result = resolveDockerfileSource('./local.Dockerfile'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve('', './local.Dockerfile')); + }); + }); +}); + +describe('validateDockerfileSource', () => { + it('returns ok+shouldCopy=false when value is undefined', () => { + const result = validateDockerfileSource(undefined, '/cwd', () => true); + expect(result).toEqual({ ok: true, shouldCopy: false }); + }); + + it('returns ok+shouldCopy=false for a bare filename (does not call fileExists)', () => { + // Bare filenames refer to a file already in place (or to be placed + // later); they bypass the existence check entirely. + const fileExists = vi.fn(() => false); + const result = validateDockerfileSource('Dockerfile', '/cwd', fileExists); + expect(result).toEqual({ ok: true, shouldCopy: false }); + expect(fileExists).not.toHaveBeenCalled(); + }); + + it('returns the resolved source+filename when the path exists', () => { + const fileExists = vi.fn(() => true); + const result = validateDockerfileSource('./my.Dockerfile', '/home/user/cwd', fileExists); + expect(result).toEqual({ + ok: true, + shouldCopy: true, + sourcePath: resolve('/home/user/cwd', './my.Dockerfile'), + filename: 'my.Dockerfile', + }); + expect(fileExists).toHaveBeenCalledWith(resolve('/home/user/cwd', './my.Dockerfile')); + }); + + it('returns ok=false with a "Dockerfile not found at " error when missing', () => { + // This is the error path consumed by handleCreatePath / handleByoPath. + // Asserting the exact message protects against accidental changes that + // would alter user-visible CLI output. + const fileExists = vi.fn(() => false); + const result = validateDockerfileSource('./missing.Dockerfile', '/home/user/cwd', fileExists); + expect(result).toEqual({ + ok: false, + error: `Dockerfile not found at ${resolve('/home/user/cwd', './missing.Dockerfile')}`, + }); + }); + + it('reports the resolved (cwd-relative) sourcePath in the error, not the user-typed input', () => { + // Regression for issue #1128: the error message must reference the + // path resolved against the invocation cwd, so users can see exactly + // where the CLI looked for their file. + const result = validateDockerfileSource('myFile', '/expected/cwd', () => false); + // 'myFile' is a bare filename → shouldCopy=false, no error, no + // existsSync call. Confirm we do not produce a misleading error. + expect(result).toEqual({ ok: true, shouldCopy: false }); + + const result2 = validateDockerfileSource('./myFile', '/expected/cwd', () => false); + expect(result2).toEqual({ + ok: false, + error: `Dockerfile not found at ${resolve('/expected/cwd', './myFile')}`, + }); + expect((result2 as { ok: false; error: string }).error).toContain('/expected/cwd'); + }); + + it('handles absolute paths that exist', () => { + const fileExists = vi.fn(() => true); + const result = validateDockerfileSource('/abs/path/Dockerfile.prod', '/cwd', fileExists); + expect(result).toEqual({ + ok: true, + shouldCopy: true, + sourcePath: '/abs/path/Dockerfile.prod', + filename: 'Dockerfile.prod', + }); + expect(fileExists).toHaveBeenCalledWith('/abs/path/Dockerfile.prod'); + }); + + it('handles absolute paths that do not exist', () => { + const result = validateDockerfileSource('/abs/missing/Dockerfile', '/cwd', () => false); + expect(result).toEqual({ + ok: false, + error: 'Dockerfile not found at /abs/missing/Dockerfile', + }); + }); +}); diff --git a/src/cli/tui/screens/agent/types.ts b/src/cli/tui/screens/agent/types.ts index c708bcac0..ff291f78b 100644 --- a/src/cli/tui/screens/agent/types.ts +++ b/src/cli/tui/screens/agent/types.ts @@ -71,7 +71,19 @@ export interface AddAgentConfig { entrypoint: string; language: TargetLanguage; buildType: BuildType; - /** Path to custom Dockerfile (copied into code directory at setup) or filename already in code directory. */ + /** + * Custom Dockerfile reference. Has two distinct lifecycle states: + * - **During the wizard:** may be a relative or absolute filesystem path + * entered by the user (resolved against the invocation cwd via + * `resolveDockerfileSource`). + * - **After `handleCreatePath` / `handleByoPath`:** normalized to a bare + * filename (basename) and persisted to the agent spec. The actual file + * has been copied into the agent's code directory by that point. + * + * If the user enters a bare filename instead of a path, no copy is performed + * and the value is treated as a reference to a file already in the code + * directory. + */ dockerfile?: string; /** Protocol (HTTP, MCP, A2A, AGUI). Defaults to HTTP. */ protocol: ProtocolMode; diff --git a/src/cli/tui/screens/agent/useAddAgent.ts b/src/cli/tui/screens/agent/useAddAgent.ts index e2fabe140..eb136719a 100644 --- a/src/cli/tui/screens/agent/useAddAgent.ts +++ b/src/cli/tui/screens/agent/useAddAgent.ts @@ -1,4 +1,4 @@ -import { APP_DIR, ConfigIO, NoProjectError, findConfigRoot, setEnvVar } from '../../../../lib'; +import { APP_DIR, ConfigIO, NoProjectError, findConfigRoot, getWorkingDirectory, setEnvVar } from '../../../../lib'; import type { AgentEnvSpec, DirectoryPath, FilePath } from '../../../../schema'; import { type PythonSetupResult, setupPythonProject } from '../../../operations'; import { createConfigBundleForAgent } from '../../../operations/agent/config-bundle-defaults'; @@ -29,7 +29,7 @@ import { createRenderer } from '../../../templates'; import type { GenerateConfig } from '../generate/types'; import type { AddAgentConfig } from './types'; import { copyFileSync, existsSync, mkdirSync } from 'fs'; -import { basename, dirname, join, resolve } from 'path'; +import { basename, dirname, isAbsolute, join, resolve } from 'path'; import { useCallback, useState } from 'react'; // ───────────────────────────────────────────────────────────────────────────── @@ -63,6 +63,95 @@ export type AddAgentOutcome = AddAgentCreateResult | AddAgentByoResult | AddAgen // Config Mappers // ───────────────────────────────────────────────────────────────────────────── +/** + * Result of resolving a user-entered Dockerfile path. + * + * - When `value` is empty/undefined or a bare filename (no path separators), + * we return `{ shouldCopy: false }` and the caller should leave the existing + * dockerfile untouched (template default or already-in-place file). + * - When `value` is a relative or absolute path, we resolve it against the + * user's invocation directory (`getWorkingDirectory()`), mirroring the + * `agentcore policy add --source ` pattern. The returned `sourcePath` + * is the absolute path to copy from, and `filename` is the basename to + * persist into the agent spec and copy into the agent code directory. + */ +export interface ResolvedDockerfile { + shouldCopy: boolean; + sourcePath?: string; + filename?: string; +} + +export function resolveDockerfileSource( + value: string | undefined, + cwd: string = getWorkingDirectory() +): ResolvedDockerfile { + if (!value) { + return { shouldCopy: false }; + } + // Only treat as a path-to-copy when it contains a path separator or is + // absolute. A bare filename (e.g. "Dockerfile") refers to the file already + // in place and should not trigger a copy. Detect both forward and + // backslash separators so users on Windows entering paths like + // 'subdir\\Dockerfile' aren't silently misclassified as bare filenames. + if (!isAbsolute(value) && !/[/\\]/.test(value)) { + return { shouldCopy: false }; + } + const sourcePath = resolve(cwd, value); + return { + shouldCopy: true, + sourcePath, + filename: basename(sourcePath), + }; +} + +/** + * Result of validating that a user-supplied Dockerfile path actually exists + * on disk, ready to be consumed by handleCreatePath / handleByoPath. + * + * - `{ shouldCopy: false }` — caller should leave the dockerfile alone. + * - `{ shouldCopy: true, sourcePath, filename }` — caller should copy + * `sourcePath` into the destination directory, then persist `filename` + * into the agent spec. + * - `{ ok: false, error }` — surface the error to the user. + */ +export type ValidatedDockerfile = + | { ok: true; shouldCopy: false } + | { ok: true; shouldCopy: true; sourcePath: string; filename: string } + | { ok: false; error: string }; + +/** + * Resolve a user-entered dockerfile reference and verify its existence on + * disk. Used by both handleCreatePath and handleByoPath; extracted to a + * pure(-ish, fs-only) function so the not-found error path is unit-testable + * without spinning up the full React hook. + * + * @param value Raw user input (path or bare filename) + * @param cwd Directory to resolve relative paths against (defaults to + * the user's invocation cwd via getWorkingDirectory()) + * @param fileExists Injectable existence predicate, defaults to fs.existsSync. + * Accepting it as a parameter keeps this function trivial + * to test without mocking the fs module. + */ +export function validateDockerfileSource( + value: string | undefined, + cwd: string = getWorkingDirectory(), + fileExists: (path: string) => boolean = existsSync +): ValidatedDockerfile { + const resolved = resolveDockerfileSource(value, cwd); + if (!resolved.shouldCopy) { + return { ok: true, shouldCopy: false }; + } + // Type-narrow: when shouldCopy is true the helper always populates these. + const { sourcePath, filename } = resolved; + if (!sourcePath || !filename) { + return { ok: true, shouldCopy: false }; + } + if (!fileExists(sourcePath)) { + return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; + } + return { ok: true, shouldCopy: true, sourcePath, filename }; +} + /** * Maps AddAgentConfig (from BYO wizard) to v2 AgentEnvSpec for schema persistence. */ @@ -255,20 +344,28 @@ async function handleCreatePath( ]; } + // Resolve the user-entered dockerfile path early (before rendering) so the + // render config sees only the basename. This keeps templates that reference + // `dockerfile` from accidentally interpolating an absolute or relative + // host path. The actual file copy happens after the renderer writes its + // template default, so we capture the source path here and apply it below. + const validatedDockerfile = validateDockerfileSource(generateConfig.dockerfile); + if (!validatedDockerfile.ok) { + return { ok: false, error: validatedDockerfile.error }; + } + if (validatedDockerfile.shouldCopy) { + generateConfig.dockerfile = validatedDockerfile.filename; + } + // Generate agent files with correct identity provider const renderConfig = await mapGenerateConfigToRenderConfig(generateConfig, identityProviders); const renderer = createRenderer(renderConfig); await renderer.render({ outputDir: projectRoot }); - // If dockerfile is a path (contains /), copy it into the agent directory (overwriting template default) - if (generateConfig.dockerfile?.includes('/')) { - const sourcePath = resolve(projectRoot, generateConfig.dockerfile); - if (!existsSync(sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; - } - const filename = basename(sourcePath); - copyFileSync(sourcePath, join(agentPath, filename)); - generateConfig.dockerfile = filename; + // If a user-supplied Dockerfile was resolved above, copy it into the agent + // directory (overwriting the template default written by the renderer). + if (validatedDockerfile.shouldCopy) { + copyFileSync(validatedDockerfile.sourcePath, join(agentPath, validatedDockerfile.filename)); } // Write agent to project config @@ -371,14 +468,19 @@ async function handleByoPath( // If dockerfile is a path (contains /), copy it into the code directory and use the filename let dockerfileName = config.dockerfile; - if (dockerfileName?.includes('/')) { - const sourcePath = resolve(projectRoot, dockerfileName); - if (!existsSync(sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; - } - dockerfileName = basename(sourcePath); - copyFileSync(sourcePath, join(codeDir, dockerfileName)); + const validatedDockerfile = validateDockerfileSource(dockerfileName); + if (!validatedDockerfile.ok) { + return { ok: false, error: validatedDockerfile.error }; + } + if (validatedDockerfile.shouldCopy) { + dockerfileName = validatedDockerfile.filename; + copyFileSync(validatedDockerfile.sourcePath, join(codeDir, dockerfileName)); } + // NOTE: a bare-filename dockerfile (no path separator) is intentionally NOT + // pre-validated against codeDir here. Users may legitimately add the agent + // config first and produce/place the Dockerfile in a separate step. The + // deploy/build path will surface a clear error if the file is still missing + // at that time. const project = await configIO.readProjectSpec(); const agent = mapByoConfigToAgent({ ...config, dockerfile: dockerfileName }); diff --git a/src/cli/tui/screens/generate/GenerateWizardUI.tsx b/src/cli/tui/screens/generate/GenerateWizardUI.tsx index 9c6c79599..7d13adb08 100644 --- a/src/cli/tui/screens/generate/GenerateWizardUI.tsx +++ b/src/cli/tui/screens/generate/GenerateWizardUI.tsx @@ -1,3 +1,4 @@ +import { getWorkingDirectory } from '../../../../lib'; import type { ModelProvider, NetworkMode, RuntimeAuthorizerType } from '../../../../schema'; import { DEFAULT_MODEL_IDS, @@ -230,11 +231,15 @@ export function GenerateWizardUI({ {isDockerfileStep && ( { - wizard.setDockerfile(value ? basename(value) : undefined); + // Preserve the full path so downstream copy logic in useAddAgent can + // resolve it against the invocation cwd. The persisted spec stores + // only the basename (applied at copy/serialization time). + wizard.setDockerfile(value || undefined); }} onCancel={onBack} /> @@ -481,7 +486,7 @@ function ConfirmView({ config, credentialProjectName }: { config: GenerateConfig {config.buildType === 'Container' && config.dockerfile && ( Dockerfile: - {config.dockerfile} + {basename(config.dockerfile)} )} diff --git a/src/cli/tui/screens/generate/types.ts b/src/cli/tui/screens/generate/types.ts index 1b764ea80..1669a4957 100644 --- a/src/cli/tui/screens/generate/types.ts +++ b/src/cli/tui/screens/generate/types.ts @@ -40,7 +40,19 @@ export type { BuildType, ModelProvider, ProtocolMode, SDKFramework, TargetLangua export interface GenerateConfig { projectName: string; buildType: BuildType; - /** Path to custom Dockerfile (copied into code directory at setup) or filename already in code directory. */ + /** + * Custom Dockerfile reference. Has two distinct lifecycle states: + * - **During the wizard:** may be a relative or absolute filesystem path + * entered by the user (resolved against the invocation cwd via + * `resolveDockerfileSource`). + * - **After `handleCreatePath` mutates this config:** normalized to a bare + * filename (basename) and persisted to the agent spec. The actual file + * has been copied into the agent's code directory by that point. + * + * If the user enters a bare filename instead of a path, no copy is performed + * and the value is treated as a reference to a file already in the code + * directory. + */ dockerfile?: string; protocol: ProtocolMode; sdk: SDKFramework;