From c4a300198928dc6261e2761008daeb14d2b9f6d6 Mon Sep 17 00:00:00 2001 From: agentcore-bot Date: Wed, 6 May 2026 20:50:39 +0000 Subject: [PATCH 1/4] fix(#1128): resolve Dockerfile paths against invocation cwd The BYO/Container 'agentcore create' wizard previously resolved user-entered Dockerfile paths against the project root (where agentcore.json lives), causing 'not found' errors when users specified a path relative to the directory they ran the command from. Two related fixes: 1. AddAgentScreen PathInput basePath now uses getWorkingDirectory() instead of /, so completion + validation match the user's invocation directory. This mirrors the pattern used by 'agentcore policy add --source' and CodePathScreen. 2. The wizard no longer strips the user-entered path via basename() at submit time. The full path is preserved so that the downstream copy logic in useAddAgent can resolve it. basename() is now applied at the persistence/render boundary instead. 3. useAddAgent now resolves Dockerfile sources against getWorkingDirectory() (not projectRoot) and broadens the path detection to include absolute paths. The same submit-time basename() bug is also fixed in GenerateWizardUI so the 'agentcore generate --dockerfile ' flow actually copies the file as intended. Fixes #1128 --- src/cli/tui/screens/agent/AddAgentScreen.tsx | 13 ++++++++----- src/cli/tui/screens/agent/useAddAgent.ts | 16 ++++++++++------ .../tui/screens/generate/GenerateWizardUI.tsx | 6 ++++-- 3 files changed, 22 insertions(+), 13 deletions(-) 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/useAddAgent.ts b/src/cli/tui/screens/agent/useAddAgent.ts index e2fabe140..5e9b04b41 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'; // ───────────────────────────────────────────────────────────────────────────── @@ -261,8 +261,10 @@ async function handleCreatePath( 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 (generateConfig.dockerfile && (isAbsolute(generateConfig.dockerfile) || generateConfig.dockerfile.includes('/'))) { + // Resolve relative paths against the user's invocation directory (not the project + // root), mirroring the `agentcore policy add --source ` behavior. + const sourcePath = resolve(getWorkingDirectory(), generateConfig.dockerfile); if (!existsSync(sourcePath)) { return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; } @@ -371,8 +373,10 @@ 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 (dockerfileName && (isAbsolute(dockerfileName) || dockerfileName.includes('/'))) { + // Resolve relative paths against the user's invocation directory (not the project + // root), mirroring the `agentcore policy add --source ` behavior. + const sourcePath = resolve(getWorkingDirectory(), dockerfileName); if (!existsSync(sourcePath)) { return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; } diff --git a/src/cli/tui/screens/generate/GenerateWizardUI.tsx b/src/cli/tui/screens/generate/GenerateWizardUI.tsx index 9c6c79599..30f051ba8 100644 --- a/src/cli/tui/screens/generate/GenerateWizardUI.tsx +++ b/src/cli/tui/screens/generate/GenerateWizardUI.tsx @@ -36,7 +36,6 @@ import { } from './types'; import type { useGenerateWizard } from './useGenerateWizard'; import { Box, Text, useInput } from 'ink'; -import { basename } from 'path'; // Helper to get provider display name and env var name from ModelProvider function getProviderInfo(provider: ModelProvider): { name: string; envVarName: string } { @@ -234,7 +233,10 @@ export function GenerateWizardUI({ allowEmpty emptyHelpText="Press Enter to use the default Dockerfile" onSubmit={value => { - 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} /> From 0c875f1adc8979db3639e2e3fa2a0e1802c0ad3c Mon Sep 17 00:00:00 2001 From: agentcore-bot Date: Wed, 6 May 2026 20:54:04 +0000 Subject: [PATCH 2/4] test(#1128): extract resolveDockerfileSource helper + unit tests Extract the path-resolution logic for user-provided Dockerfile paths into a small pure helper so it can be unit-tested without mocking ConfigIO, telemetry, credential primitives, etc. Tests cover the cases that motivated issue #1128: - bare filename -> no copy (existing in place) - relative path -> resolved against the invocation cwd, not projectRoot - absolute path -> used verbatim - parent-relative paths - empty/undefined input --- .../__tests__/resolveDockerfileSource.test.ts | 64 +++++++++++++++++ src/cli/tui/screens/agent/useAddAgent.ts | 68 ++++++++++++++----- 2 files changed, 115 insertions(+), 17 deletions(-) create mode 100644 src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts 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..1660c309c --- /dev/null +++ b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts @@ -0,0 +1,64 @@ +import { resolveDockerfileSource } from '../useAddAgent'; +import { resolve } from 'path'; +import { describe, expect, it } from 'vitest'; + +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'); + }); + + it('uses getWorkingDirectory() by default when no cwd is provided', () => { + // The default parameter should call getWorkingDirectory(), which falls back + // to process.cwd() when INIT_CWD is unset. We assert the resolved path + // matches resolution against process.cwd(). + const result = resolveDockerfileSource('./local.Dockerfile'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve(process.env.INIT_CWD ?? process.cwd(), './local.Dockerfile')); + }); +}); diff --git a/src/cli/tui/screens/agent/useAddAgent.ts b/src/cli/tui/screens/agent/useAddAgent.ts index 5e9b04b41..de49c781d 100644 --- a/src/cli/tui/screens/agent/useAddAgent.ts +++ b/src/cli/tui/screens/agent/useAddAgent.ts @@ -63,6 +63,45 @@ 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. + if (!isAbsolute(value) && !value.includes('/')) { + return { shouldCopy: false }; + } + const sourcePath = resolve(cwd, value); + return { + shouldCopy: true, + sourcePath, + filename: basename(sourcePath), + }; +} + /** * Maps AddAgentConfig (from BYO wizard) to v2 AgentEnvSpec for schema persistence. */ @@ -261,16 +300,13 @@ async function handleCreatePath( await renderer.render({ outputDir: projectRoot }); // If dockerfile is a path (contains /), copy it into the agent directory (overwriting template default) - if (generateConfig.dockerfile && (isAbsolute(generateConfig.dockerfile) || generateConfig.dockerfile.includes('/'))) { - // Resolve relative paths against the user's invocation directory (not the project - // root), mirroring the `agentcore policy add --source ` behavior. - const sourcePath = resolve(getWorkingDirectory(), generateConfig.dockerfile); - if (!existsSync(sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; + const resolvedDockerfile = resolveDockerfileSource(generateConfig.dockerfile); + if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { + if (!existsSync(resolvedDockerfile.sourcePath)) { + return { ok: false, error: `Dockerfile not found at ${resolvedDockerfile.sourcePath}` }; } - const filename = basename(sourcePath); - copyFileSync(sourcePath, join(agentPath, filename)); - generateConfig.dockerfile = filename; + copyFileSync(resolvedDockerfile.sourcePath, join(agentPath, resolvedDockerfile.filename)); + generateConfig.dockerfile = resolvedDockerfile.filename; } // Write agent to project config @@ -373,15 +409,13 @@ 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 && (isAbsolute(dockerfileName) || dockerfileName.includes('/'))) { - // Resolve relative paths against the user's invocation directory (not the project - // root), mirroring the `agentcore policy add --source ` behavior. - const sourcePath = resolve(getWorkingDirectory(), dockerfileName); - if (!existsSync(sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${sourcePath}` }; + const resolvedDockerfile = resolveDockerfileSource(dockerfileName); + if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { + if (!existsSync(resolvedDockerfile.sourcePath)) { + return { ok: false, error: `Dockerfile not found at ${resolvedDockerfile.sourcePath}` }; } - dockerfileName = basename(sourcePath); - copyFileSync(sourcePath, join(codeDir, dockerfileName)); + dockerfileName = resolvedDockerfile.filename; + copyFileSync(resolvedDockerfile.sourcePath, join(codeDir, dockerfileName)); } const project = await configIO.readProjectSpec(); From aae77dfacde1f156b1aec55de242595582005939 Mon Sep 17 00:00:00 2001 From: agentcore-bot Date: Wed, 6 May 2026 21:01:35 +0000 Subject: [PATCH 3/4] fix: address review findings round 1 [MEDIUM] GenerateWizardUI: pass basePath={getWorkingDirectory()} to the Dockerfile PathInput so completion/validation/downstream resolution all agree on the invocation cwd (previously the PathInput defaulted to process.cwd() while resolveDockerfileSource resolved against INIT_CWD, causing 'not found' errors under npm/bun script wrappers). [LOW] useAddAgent.resolveDockerfileSource: detect both forward- and back-slash separators when classifying a value as a path vs bare filename, so Windows users entering 'subdir\Dockerfile' aren't silently treated as referencing a non-existent bare file. [LOW] useAddAgent.handleCreatePath: resolve+normalize generateConfig .dockerfile to its basename BEFORE calling mapGenerateConfigToRenderConfig so render templates that interpolate {{dockerfile}} never see a host path string. The actual file copy still happens after render so the template default is overwritten correctly. [LOW] useAddAgent.handleByoPath: when the user supplies a bare filename (no copy path), surface a clear error if that file is not present in the code directory, instead of letting it fail later at deploy time with a less helpful message. [LOW] types.ts (agent + generate): expand the JSDoc on the dockerfile field to document its dual lifecycle (full path during the wizard, basename after handle*Path normalization). [LOW] resolveDockerfileSource.test.ts: - Replace tautological default-cwd test with vi.stubEnv('INIT_CWD', '/sentinel/...') so the test actually exercises getWorkingDirectory. - Add a second case stubbing INIT_CWD to undefined to cover the process.cwd() fallback. - Add Windows-style separator tests covering 'subdir\\Dockerfile' and '.\\sub\\My.Dockerfile' to lock in cross-platform behavior. --- .../__tests__/resolveDockerfileSource.test.ts | 52 ++++++++++++++++--- src/cli/tui/screens/agent/types.ts | 14 ++++- src/cli/tui/screens/agent/useAddAgent.ts | 37 ++++++++++--- .../tui/screens/generate/GenerateWizardUI.tsx | 2 + src/cli/tui/screens/generate/types.ts | 14 ++++- 5 files changed, 101 insertions(+), 18 deletions(-) diff --git a/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts index 1660c309c..abddb99b0 100644 --- a/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts +++ b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts @@ -1,8 +1,12 @@ import { resolveDockerfileSource } from '../useAddAgent'; import { resolve } from 'path'; -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; describe('resolveDockerfileSource', () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + it('returns shouldCopy=false when value is undefined', () => { expect(resolveDockerfileSource(undefined, '/some/cwd')).toEqual({ shouldCopy: false }); }); @@ -53,12 +57,44 @@ describe('resolveDockerfileSource', () => { expect(result.filename).toBe('MyDocker.file'); }); - it('uses getWorkingDirectory() by default when no cwd is provided', () => { - // The default parameter should call getWorkingDirectory(), which falls back - // to process.cwd() when INIT_CWD is unset. We assert the resolved path - // matches resolution against process.cwd(). - const result = resolveDockerfileSource('./local.Dockerfile'); - expect(result.shouldCopy).toBe(true); - expect(result.sourcePath).toBe(resolve(process.env.INIT_CWD ?? process.cwd(), './local.Dockerfile')); + describe('cross-platform path-separator detection', () => { + it('treats backslash-containing relative paths as paths, not bare filenames', () => { + // 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('treats dot-prefixed backslash paths as paths', () => { + const result = resolveDockerfileSource('.\\sub\\My.Dockerfile', '/cwd'); + expect(result.shouldCopy).toBe(true); + }); + + it('still treats 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); + }); + }); + + describe('default cwd parameter (getWorkingDirectory)', () => { + it('uses INIT_CWD when set (npm/bun script invocation case)', () => { + // Stub INIT_CWD to a known sentinel and verify the helper actually + // routes through getWorkingDirectory() rather than process.cwd(). + vi.stubEnv('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(). vi.stubEnv with undefined deletes the variable. + vi.stubEnv('INIT_CWD', undefined as unknown as string); + const result = resolveDockerfileSource('./local.Dockerfile'); + expect(result.shouldCopy).toBe(true); + expect(result.sourcePath).toBe(resolve(process.cwd(), './local.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 de49c781d..6e6b306ed 100644 --- a/src/cli/tui/screens/agent/useAddAgent.ts +++ b/src/cli/tui/screens/agent/useAddAgent.ts @@ -90,8 +90,10 @@ export function resolveDockerfileSource( } // 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. - if (!isAbsolute(value) && !value.includes('/')) { + // 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); @@ -294,19 +296,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 resolvedDockerfile = resolveDockerfileSource(generateConfig.dockerfile); + if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { + if (!existsSync(resolvedDockerfile.sourcePath)) { + return { ok: false, error: `Dockerfile not found at ${resolvedDockerfile.sourcePath}` }; + } + generateConfig.dockerfile = resolvedDockerfile.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) - const resolvedDockerfile = resolveDockerfileSource(generateConfig.dockerfile); + // If a user-supplied Dockerfile was resolved above, copy it into the agent + // directory (overwriting the template default written by the renderer). if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { - if (!existsSync(resolvedDockerfile.sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${resolvedDockerfile.sourcePath}` }; - } copyFileSync(resolvedDockerfile.sourcePath, join(agentPath, resolvedDockerfile.filename)); - generateConfig.dockerfile = resolvedDockerfile.filename; } // Write agent to project config @@ -416,6 +427,16 @@ async function handleByoPath( } dockerfileName = resolvedDockerfile.filename; copyFileSync(resolvedDockerfile.sourcePath, join(codeDir, dockerfileName)); + } else if (dockerfileName) { + // Bare-filename case: the user referenced a Dockerfile expected to already + // exist in their codeLocation. Surface a clear error here rather than + // letting it fail at deploy/build time with a less helpful message. + if (!existsSync(join(codeDir, dockerfileName))) { + return { + ok: false, + error: `Dockerfile "${dockerfileName}" not found in code location "${config.codeLocation}". Provide a path to a Dockerfile to copy in, or place the file at ${join(codeDir, dockerfileName)}.`, + }; + } } const project = await configIO.readProjectSpec(); diff --git a/src/cli/tui/screens/generate/GenerateWizardUI.tsx b/src/cli/tui/screens/generate/GenerateWizardUI.tsx index 30f051ba8..07ce2bf9d 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, @@ -229,6 +230,7 @@ export function GenerateWizardUI({ {isDockerfileStep && ( Date: Wed, 6 May 2026 21:10:03 +0000 Subject: [PATCH 4/4] fix: address review findings round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [MEDIUM] resolveDockerfileSource.test.ts (Windows separators): Tightened the cross-platform-detection describe block to clarify it locks in *classification* (shouldCopy) only — basename extraction on Windows is the responsibility of node's platform-default `path` module. Added a new test that uses path.win32 directly to document the expected end-to-end basename behavior on Windows hosts, so a future regression in win32 path handling would be caught. [MEDIUM] handleByoPath bare-filename strict check: Reverted the new `else if (dockerfileName) -> existsSync(codeDir/...)` branch added in round 1. While well-intentioned, it broke the legitimate workflow where a user adds the agent config first and produces / places the Dockerfile in a separate step. The deploy/build path will still surface a clear error if the file is missing at that time. Added an explanatory NOTE comment so future contributors don't re-introduce the check. [MEDIUM] handleByoPath / handleCreatePath error-path test coverage: Extracted a new `validateDockerfileSource(value, cwd?, fileExists?)` helper that bundles `resolveDockerfileSource` + the existsSync gate used by both call sites, with an injectable `fileExists` predicate for testing without fs mocks. Both handle*Path functions now delegate to the helper, eliminating duplicated logic. Added 7 new unit tests covering: success path, missing-source error path (asserts the exact user-visible 'Dockerfile not found at ' message), bare-filename short-circuit (verifies fileExists is NOT called), absolute-path success+failure, and a regression guard ensuring the error message references the cwd-resolved path (issue #1128). [LOW] resolveDockerfileSource.test.ts (INIT_CWD handling): - Replaced the `vi.stubEnv('INIT_CWD', undefined as unknown as string)` pattern with explicit beforeEach/afterEach save+restore via `delete process.env.INIT_CWD`, which doesn't depend on the vitest deletion-via-undefined contract. - Added a new test documenting that INIT_CWD='' (empty string) is honored by getWorkingDirectory()'s `??` semantics rather than falling through to process.cwd(). [LOW] GenerateWizardUI summary: Now displays `basename(config.dockerfile)` instead of the raw value (which post-fix may be a long absolute or relative host path). Mirrors the equivalent normalization already in AddAgentScreen.tsx for UX consistency. Re-imported `basename` from 'path'. --- .../__tests__/resolveDockerfileSource.test.ts | 159 ++++++++++++++++-- src/cli/tui/screens/agent/useAddAgent.ts | 93 +++++++--- .../tui/screens/generate/GenerateWizardUI.tsx | 3 +- 3 files changed, 215 insertions(+), 40 deletions(-) diff --git a/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts index abddb99b0..136f71512 100644 --- a/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts +++ b/src/cli/tui/screens/agent/__tests__/resolveDockerfileSource.test.ts @@ -1,12 +1,10 @@ -import { resolveDockerfileSource } from '../useAddAgent'; -import { resolve } from 'path'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { resolveDockerfileSource, validateDockerfileSource } from '../useAddAgent'; +import { resolve, win32 } from 'path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -describe('resolveDockerfileSource', () => { - afterEach(() => { - vi.unstubAllEnvs(); - }); +const { resolve: win32Resolve, basename: win32Basename } = win32; +describe('resolveDockerfileSource', () => { it('returns shouldCopy=false when value is undefined', () => { expect(resolveDockerfileSource(undefined, '/some/cwd')).toEqual({ shouldCopy: false }); }); @@ -58,7 +56,14 @@ describe('resolveDockerfileSource', () => { }); describe('cross-platform path-separator detection', () => { - it('treats backslash-containing relative paths as paths, not bare filenames', () => { + // 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). @@ -66,23 +71,56 @@ describe('resolveDockerfileSource', () => { expect(result.shouldCopy).toBe(true); }); - it('treats dot-prefixed backslash paths as paths', () => { + 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 treats a bare filename with no separators as not-a-path', () => { + 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)', () => { - // Stub INIT_CWD to a known sentinel and verify the helper actually + // Set INIT_CWD to a known sentinel and verify the helper actually // routes through getWorkingDirectory() rather than process.cwd(). - vi.stubEnv('INIT_CWD', '/sentinel/init/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')); @@ -90,11 +128,104 @@ describe('resolveDockerfileSource', () => { it('falls back to process.cwd() when INIT_CWD is unset', () => { // When INIT_CWD is unset, getWorkingDirectory() falls back to - // process.cwd(). vi.stubEnv with undefined deletes the variable. - vi.stubEnv('INIT_CWD', undefined as unknown as string); + // 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/useAddAgent.ts b/src/cli/tui/screens/agent/useAddAgent.ts index 6e6b306ed..eb136719a 100644 --- a/src/cli/tui/screens/agent/useAddAgent.ts +++ b/src/cli/tui/screens/agent/useAddAgent.ts @@ -104,6 +104,54 @@ export function resolveDockerfileSource( }; } +/** + * 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. */ @@ -301,12 +349,12 @@ async function handleCreatePath( // `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 resolvedDockerfile = resolveDockerfileSource(generateConfig.dockerfile); - if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { - if (!existsSync(resolvedDockerfile.sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${resolvedDockerfile.sourcePath}` }; - } - generateConfig.dockerfile = resolvedDockerfile.filename; + 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 @@ -316,8 +364,8 @@ async function handleCreatePath( // If a user-supplied Dockerfile was resolved above, copy it into the agent // directory (overwriting the template default written by the renderer). - if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { - copyFileSync(resolvedDockerfile.sourcePath, join(agentPath, resolvedDockerfile.filename)); + if (validatedDockerfile.shouldCopy) { + copyFileSync(validatedDockerfile.sourcePath, join(agentPath, validatedDockerfile.filename)); } // Write agent to project config @@ -420,24 +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; - const resolvedDockerfile = resolveDockerfileSource(dockerfileName); - if (resolvedDockerfile.shouldCopy && resolvedDockerfile.sourcePath && resolvedDockerfile.filename) { - if (!existsSync(resolvedDockerfile.sourcePath)) { - return { ok: false, error: `Dockerfile not found at ${resolvedDockerfile.sourcePath}` }; - } - dockerfileName = resolvedDockerfile.filename; - copyFileSync(resolvedDockerfile.sourcePath, join(codeDir, dockerfileName)); - } else if (dockerfileName) { - // Bare-filename case: the user referenced a Dockerfile expected to already - // exist in their codeLocation. Surface a clear error here rather than - // letting it fail at deploy/build time with a less helpful message. - if (!existsSync(join(codeDir, dockerfileName))) { - return { - ok: false, - error: `Dockerfile "${dockerfileName}" not found in code location "${config.codeLocation}". Provide a path to a Dockerfile to copy in, or place the file at ${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 07ce2bf9d..7d13adb08 100644 --- a/src/cli/tui/screens/generate/GenerateWizardUI.tsx +++ b/src/cli/tui/screens/generate/GenerateWizardUI.tsx @@ -37,6 +37,7 @@ import { } from './types'; import type { useGenerateWizard } from './useGenerateWizard'; import { Box, Text, useInput } from 'ink'; +import { basename } from 'path'; // Helper to get provider display name and env var name from ModelProvider function getProviderInfo(provider: ModelProvider): { name: string; envVarName: string } { @@ -485,7 +486,7 @@ function ConfirmView({ config, credentialProjectName }: { config: GenerateConfig {config.buildType === 'Container' && config.dockerfile && ( Dockerfile: - {config.dockerfile} + {basename(config.dockerfile)} )}