From 6d836d2d5e99d86d6cfa80911d839c94bee9f0c4 Mon Sep 17 00:00:00 2001 From: Aidan Daly Date: Thu, 2 Jul 2026 18:15:23 +0000 Subject: [PATCH 1/3] fix(vpc): fix 3 bugbash-found defects in the create-harness VPC + backfill paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full bug-bash (14 isolated scenarios, adversarially verified) found three defects in the review-fix changes — one a blocker regression introduced by the prior fix commit (59bbde0a): - BLOCKER: `agentcore create --network-mode VPC` (harness path) always failed with a misleading "--subnets is required" even when supplied. Root cause: 59bbde0a added a validateVpcOptions call inside validateCreateHarnessOptions, but the two call sites in command.tsx (dry-run + main) never forwarded container/subnets/ securityGroups/vpcId into it, so those were always undefined. The entire VPC create-harness path was dead and the intended friendly errors unreachable. Fix: forward the four fields at both call sites. - MEDIUM: `deploy --dry-run` persisted the backfilled vpcId to agentcore.json/ harness.json, dirtying the working tree — violating the "preview without deploying" contract. Fix: backfillContainerVpcIds takes a `persist` flag; in preview mode it still writes (the CDK synth subprocess reads from disk) but returns restore() which reverts the files after synth. Plan + diff modes call it. - LOW (TUI): a stale "VPC ID" line rendered on the CodeZip review screen after a Container→CodeZip back-nav, and the stale vpcId could leak into agentcore.json. Fix: guard the review render and the schema-mapper write on buildType==='Container'. The blocker slipped past unit tests because they called validateCreateHarnessOptions directly with a full options object, bypassing the broken command.tsx caller. Added integration tests that drive the real CLI dispatch via runCLI --dry-run (would have caught it), plus dry-run restore tests for the backfill. Full unit suite green (6120). --- .../commands/create/__tests__/create.test.ts | 93 +++++++++++++++++++ src/cli/commands/create/command.tsx | 8 ++ src/cli/commands/deploy/actions.ts | 18 ++-- .../agent/generate/schema-mapper.ts | 4 +- .../deploy/__tests__/backfill-vpc-id.test.ts | 61 ++++++++++++ src/cli/operations/deploy/backfill-vpc-id.ts | 50 ++++++++-- .../tui/screens/generate/GenerateWizardUI.tsx | 2 +- src/lib/schemas/io/config-io.ts | 14 +++ 8 files changed, 233 insertions(+), 17 deletions(-) diff --git a/src/cli/commands/create/__tests__/create.test.ts b/src/cli/commands/create/__tests__/create.test.ts index 964178541..4797ea8b9 100644 --- a/src/cli/commands/create/__tests__/create.test.ts +++ b/src/cli/commands/create/__tests__/create.test.ts @@ -333,4 +333,97 @@ describe('create command', () => { expect(result.stderr).not.toContain('--name is required'); }); }); + + // Regression coverage for the create-harness VPC dispatch: the unit tests on + // validateCreateHarnessOptions call the validator directly with a full options object, so they + // could not catch command.tsx failing to forward container/subnets/securityGroups/vpcId into it. + // These drive the real CLI dispatch (via runCLI --dry-run, no AWS) so the wiring is exercised. + describe('harness VPC dispatch (--dry-run)', () => { + const SUBNET = 'subnet-05169b775866f2440'; + const SG = 'sg-0390682a9d9f7dd8d'; + const VPC = 'vpc-07086549ccf106a5d'; + + it('accepts a Container+VPC dockerfile harness when all VPC flags incl --vpc-id are supplied', async () => { + const name = `HVpcOk${Date.now().toString().slice(-6)}`; + const result = await runCLI( + [ + 'create', + '--name', + name, + '--container', + './Dockerfile', + '--network-mode', + 'VPC', + '--subnets', + SUBNET, + '--security-groups', + SG, + '--vpc-id', + VPC, + '--model-provider', + 'bedrock', + '--dry-run', + ], + testDir + ); + // Must NOT fail with the bogus "--subnets is required" (the regression); dry-run reaches synth-info. + expect(result.exitCode, `stderr: ${result.stderr}, stdout: ${result.stdout}`).toBe(0); + expect(result.stderr).not.toContain('--subnets is required'); + }); + + it('rejects a Container+VPC harness missing --vpc-id with the friendly error', async () => { + const name = `HVpcNoId${Date.now().toString().slice(-6)}`; + const result = await runCLI( + [ + 'create', + '--name', + name, + '--container', + './Dockerfile', + '--network-mode', + 'VPC', + '--subnets', + SUBNET, + '--security-groups', + SG, + '--model-provider', + 'bedrock', + '--dry-run', + ], + testDir + ); + expect(result.exitCode).toBe(1); + const out = `${result.stderr}${result.stdout}`; + expect(out).toContain('--vpc-id is required'); + // It must NOT be the misleading "--subnets is required" message. + expect(out).not.toContain('--subnets is required'); + }); + + it('rejects a malformed --vpc-id on the harness path', async () => { + const name = `HVpcBad${Date.now().toString().slice(-6)}`; + const result = await runCLI( + [ + 'create', + '--name', + name, + '--container', + './Dockerfile', + '--network-mode', + 'VPC', + '--subnets', + SUBNET, + '--security-groups', + SG, + '--vpc-id', + 'vpc-XYZ', + '--model-provider', + 'bedrock', + '--dry-run', + ], + testDir + ); + expect(result.exitCode).toBe(1); + expect(`${result.stderr}${result.stdout}`).toContain('Invalid VPC ID format'); + }); + }); }); diff --git a/src/cli/commands/create/command.tsx b/src/cli/commands/create/command.tsx index 67998b5a1..a8b5ed4af 100644 --- a/src/cli/commands/create/command.tsx +++ b/src/cli/commands/create/command.tsx @@ -169,7 +169,11 @@ async function handleCreateHarnessCLI(options: CreateOptions): Promise { modelProvider: options.modelProvider, modelId: options.modelId, apiKeyArn: options.apiKeyArn, + container: options.container, networkMode: options.networkMode, + subnets: options.subnets, + securityGroups: options.securityGroups, + vpcId: options.vpcId, efsAccessPointArn: options.efsAccessPointArn, efsMountPath: options.efsMountPath, s3AccessPointArn: options.s3AccessPointArn, @@ -216,7 +220,11 @@ async function handleCreateHarnessCLI(options: CreateOptions): Promise { modelProvider: options.modelProvider, modelId: options.modelId, apiKeyArn: options.apiKeyArn, + container: options.container, networkMode: options.networkMode, + subnets: options.subnets, + securityGroups: options.securityGroups, + vpcId: options.vpcId, efsAccessPointArn: options.efsAccessPointArn, efsMountPath: options.efsMountPath, s3AccessPointArn: options.s3AccessPointArn, diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 3f0d02ac8..6bba7e9bf 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -411,10 +411,12 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0) { logger.log(`Resolved networkConfig.vpcId for Container+VPC build(s): ${backfill.backfilled.join(', ')}`, 'info'); } @@ -477,8 +479,10 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0 && { diff --git a/src/cli/operations/deploy/__tests__/backfill-vpc-id.test.ts b/src/cli/operations/deploy/__tests__/backfill-vpc-id.test.ts index 0b068bd14..f059f9a44 100644 --- a/src/cli/operations/deploy/__tests__/backfill-vpc-id.test.ts +++ b/src/cli/operations/deploy/__tests__/backfill-vpc-id.test.ts @@ -1,6 +1,10 @@ import type { ConfigIO } from '../../../../lib'; import type { AgentCoreProjectSpec, HarnessSpec } from '../../../../schema'; import { backfillContainerVpcIds } from '../backfill-vpc-id'; +import { randomUUID } from 'node:crypto'; +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { beforeEach, describe, expect, it, vi } from 'vitest'; const { mockResolveVpcId } = vi.hoisted(() => ({ mockResolveVpcId: vi.fn() })); @@ -147,4 +151,61 @@ describe('backfillContainerVpcIds', () => { expect(harnessSpecs.h2.networkConfig?.vpcId).toBe('vpc-0123456789abcdef0'); expect(result.backfilled).toEqual(['h2']); }); + + // BUG-2 regression: in preview mode (persist=false) the resolved vpcId is written to disk so the + // synth subprocess can read it, but restore() must revert the file so a --dry-run/--diff preview + // leaves the working tree untouched. + describe('dry-run (persist=false) restore', () => { + it('writes vpcId for synth then restore() reverts agentcore.json to its original bytes', async () => { + const dir = mkdtempSync(join(tmpdir(), `backfill-${randomUUID()}-`)); + const agentPath = join(dir, 'agentcore.json'); + const original = JSON.stringify({ name: 'proj', runtimes: [{ name: 'a' }] }, null, 2); + writeFileSync(agentPath, original, 'utf-8'); + + const runtime = containerVpcRuntime(); + const spec = { name: 'proj', runtimes: [runtime], harnesses: [] } as unknown as AgentCoreProjectSpec; + // ConfigIO stub whose writeProjectSpec writes the real file at agentPath (what CDK synth reads). + const configIO = { + getAgentConfigPath: () => agentPath, + getHarnessConfigPath: (n: string) => join(dir, `${n}.json`), + writeProjectSpec: (s: AgentCoreProjectSpec) => { + writeFileSync(agentPath, JSON.stringify(s, null, 2), 'utf-8'); + return Promise.resolve(); + }, + readHarnessSpec: () => Promise.reject(new Error('no harness')), + } as unknown as ConfigIO; + + const result = await backfillContainerVpcIds(configIO, spec, 'us-east-1', false); + + // While synth would run, the resolved vpcId is on disk... + expect(readFileSync(agentPath, 'utf-8')).toContain('vpc-0123456789abcdef0'); + expect(result.backfilled).toEqual(['agent1']); + + // ...but after restore() the file is byte-for-byte what it was before the preview. + await result.restore(); + expect(readFileSync(agentPath, 'utf-8')).toBe(original); + }); + + it('persist=true leaves the value on disk (no restore) — restore() is a no-op', async () => { + const dir = mkdtempSync(join(tmpdir(), `backfill-${randomUUID()}-`)); + const agentPath = join(dir, 'agentcore.json'); + writeFileSync(agentPath, JSON.stringify({ name: 'proj', runtimes: [{ name: 'a' }] }, null, 2), 'utf-8'); + + const runtime = containerVpcRuntime(); + const spec = { name: 'proj', runtimes: [runtime], harnesses: [] } as unknown as AgentCoreProjectSpec; + const configIO = { + getAgentConfigPath: () => agentPath, + getHarnessConfigPath: (n: string) => join(dir, `${n}.json`), + writeProjectSpec: (s: AgentCoreProjectSpec) => { + writeFileSync(agentPath, JSON.stringify(s, null, 2), 'utf-8'); + return Promise.resolve(); + }, + readHarnessSpec: () => Promise.reject(new Error('no harness')), + } as unknown as ConfigIO; + + const result = await backfillContainerVpcIds(configIO, spec, 'us-east-1', true); + await result.restore(); // no-op on a real deploy + expect(readFileSync(agentPath, 'utf-8')).toContain('vpc-0123456789abcdef0'); + }); + }); }); diff --git a/src/cli/operations/deploy/backfill-vpc-id.ts b/src/cli/operations/deploy/backfill-vpc-id.ts index a85195237..f2126079f 100644 --- a/src/cli/operations/deploy/backfill-vpc-id.ts +++ b/src/cli/operations/deploy/backfill-vpc-id.ts @@ -2,31 +2,55 @@ import type { ConfigIO } from '../../../lib'; import type { AgentCoreProjectSpec } from '../../../schema'; import { isContainerBuild } from '../../../schema/constants'; import { resolveVpcIdFromSubnets } from '../../commands/shared/vpc-utils'; +import { readFile, writeFile } from 'fs/promises'; export interface BackfillVpcIdResult { - /** Names of runtimes/harnesses whose vpcId was resolved and persisted this run. */ + /** Names of runtimes/harnesses whose vpcId was resolved this run. */ backfilled: string[]; + /** + * Restore the config files this call rewrote to their original on-disk bytes. Populated only when + * `persist` is false (dry-run/plan): the value is still written to disk so the CDK synth subprocess + * can read it, then reverted so a preview leaves the working tree untouched. No-op on a real deploy. + */ + restore: () => Promise; } /** - * Backfill `networkConfig.vpcId` for any Container+VPC runtime or harness that is missing it, then - * persist the change to disk so CDK synth (a separate process that re-reads agentcore.json / - * harness.json) sees the resolved value. + * Backfill `networkConfig.vpcId` for any Container+VPC runtime or harness that is missing it. * * vpcId became required for Container+VPC builds after this feature shipped, but agentcore.json * files written before then have only subnets + security groups. Rather than hard-fail those * configs on read, deploy resolves the VPC from the first subnet via ec2:DescribeSubnets — a subnet - * uniquely determines its VPC — and writes it back once. Fresh creates already collect --vpc-id at - * the CLI layer, so this is a no-op for them. + * uniquely determines its VPC. Fresh creates already collect --vpc-id at the CLI layer, so this is a + * no-op for them. * - * @returns the names that were backfilled (empty if none needed it). + * CDK synth is a separate process that re-reads agentcore.json / harness.json from disk, so the + * resolved value must be written there. On a real deploy (`persist: true`) the write is permanent. + * On a preview (`persist: false` — dry-run/plan) the write is temporary: the returned `restore()` + * reverts the files to their original bytes after synth, honoring the "preview without deploying" + * contract (the working tree is left untouched). + * + * @returns the names that were backfilled and a `restore()` callback (a no-op when persist is true). */ export async function backfillContainerVpcIds( configIO: ConfigIO, projectSpec: AgentCoreProjectSpec, - region: string + region: string, + persist = true ): Promise { const backfilled: string[] = []; + // path -> original bytes, captured before the first rewrite of each file (preview mode only, so + // restore() can revert them). Never touched on a real deploy (persist=true). + const originals = new Map(); + + // Snapshot a file's current bytes before we rewrite it — but only in preview mode. Computes the + // path lazily so a real deploy never calls the path getter (keeps the persist=true path minimal). + const snapshot = async (getPath: () => string): Promise => { + if (persist) return; + const path = getPath(); + if (originals.has(path)) return; + originals.set(path, await readFile(path, 'utf-8')); + }; // Runtimes live inline in agentcore.json; mutate the spec and write it back once at the end if any // changed. @@ -40,6 +64,7 @@ export async function backfillContainerVpcIds( } } if (runtimesChanged) { + await snapshot(() => configIO.getAgentConfigPath()); await configIO.writeProjectSpec(projectSpec); } @@ -49,10 +74,17 @@ export async function backfillContainerVpcIds( const nc = harness.networkConfig; if (isContainerBuild(harness) && harness.networkMode === 'VPC' && nc && nc.subnets.length > 0 && !nc.vpcId) { nc.vpcId = await resolveVpcIdFromSubnets(nc.subnets, region); + await snapshot(() => configIO.getHarnessConfigPath(entry.name)); await configIO.writeHarnessSpec(entry.name, harness); backfilled.push(entry.name); } } - return { backfilled }; + const restore = async (): Promise => { + for (const [path, content] of originals) { + await writeFile(path, content, 'utf-8'); + } + }; + + return { backfilled, restore }; } diff --git a/src/cli/tui/screens/generate/GenerateWizardUI.tsx b/src/cli/tui/screens/generate/GenerateWizardUI.tsx index c6b190343..bdd8708c3 100644 --- a/src/cli/tui/screens/generate/GenerateWizardUI.tsx +++ b/src/cli/tui/screens/generate/GenerateWizardUI.tsx @@ -690,7 +690,7 @@ function ConfirmView({ config, credentialProjectName }: { config: GenerateConfig {config.securityGroups.join(', ')} )} - {config.networkMode === 'VPC' && config.vpcId && ( + {config.networkMode === 'VPC' && config.buildType === 'Container' && config.vpcId && ( VPC ID: {config.vpcId} diff --git a/src/lib/schemas/io/config-io.ts b/src/lib/schemas/io/config-io.ts index 9fd6bf8c6..8b59d8d69 100644 --- a/src/lib/schemas/io/config-io.ts +++ b/src/lib/schemas/io/config-io.ts @@ -105,6 +105,20 @@ export class ConfigIO { return this.pathResolver.getBaseDir(); } + /** + * Absolute path to the project config file (agentcore.json). + */ + getAgentConfigPath(): string { + return this.pathResolver.getAgentConfigPath(); + } + + /** + * Absolute path to a harness's config file (app//harness.json). + */ + getHarnessConfigPath(harnessName: string): string { + return this.pathResolver.getHarnessConfigPath(harnessName); + } + /** * Read and validate the project configuration. */ From f8a1656774af881ed27a8920b095dd2b7d2bc55e Mon Sep 17 00:00:00 2001 From: Aidan Daly Date: Mon, 6 Jul 2026 16:00:57 +0000 Subject: [PATCH 2/3] fix(deploy): restore dry-run/diff vpcId backfill on all exit paths, not just happy path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review on #1682: the preview-mode backfill restore() was only called on the plan/diff happy-path returns. If any step between the backfill and those returns bailed out — synthesizeCdk throwing, the stack-selection / bootstrap / deployability early-returns, or runDiff throwing — the resolved vpcId written to disk for synth was left behind, dirtying the working tree on a failed --dry-run / --diff. Move the restore into the deploy handler's finally block (hoisted previewRestore callback, mirroring the existing restoreEnv / toolkitWrapper.dispose() pattern) so it runs on every exit path — success, early return, or throw. restore() is a no-op on a real deploy. Test: a --dry-run on a pre-existing vpcId-less Container+VPC config (which fails at synth/bootstrap without creds) now leaves agentcore.json byte-for-byte unchanged. --- .../commands/deploy/__tests__/deploy.test.ts | 84 ++++++++++++++++++- src/cli/commands/deploy/actions.ts | 21 +++-- 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/src/cli/commands/deploy/__tests__/deploy.test.ts b/src/cli/commands/deploy/__tests__/deploy.test.ts index 0df7905e2..965e4e0da 100644 --- a/src/cli/commands/deploy/__tests__/deploy.test.ts +++ b/src/cli/commands/deploy/__tests__/deploy.test.ts @@ -2,7 +2,7 @@ import { runCLI } from '../../../../test-utils/index.js'; import { runDeploy, runDiff, selectTargetStack } from '../actions.js'; import { StackSelectionStrategy } from '@aws-cdk/toolkit-lib'; import { randomUUID } from 'node:crypto'; -import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { mkdir, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -59,6 +59,88 @@ describe('deploy without agents', () => { }); }); +// Regression for the preview-mode vpcId backfill: `deploy --dry-run` on a pre-existing Container+VPC +// config missing vpcId resolves + writes it to disk for synth, but must revert on EVERY exit path — +// including when synth/bootstrap fails (no real creds here) — so a preview never dirties the tree. +describe('deploy --dry-run preview does not dirty a vpcId-less Container+VPC config', () => { + let testDir: string; + let projectDir: string; + let agentConfigPath: string; + + beforeAll(async () => { + testDir = join(tmpdir(), `agentcore-deploy-preview-${randomUUID()}`); + await mkdir(testDir, { recursive: true }); + + const projectName = 'PreviewVpcProj'; + const dockerfile = join(testDir, 'Dockerfile'); + await writeFile(dockerfile, 'FROM public.ecr.aws/lambda/python:3.12\n'); + + // Container + VPC agent, created WITH a vpcId... + const create = await runCLI( + [ + 'create', + '--project-name', + projectName, + '--name', + 'pvagent', + '--build', + 'Container', + '--network-mode', + 'VPC', + '--subnets', + 'subnet-0123456789abcdef0', + '--security-groups', + 'sg-0123456789abcdef0', + '--vpc-id', + 'vpc-0123456789abcdef0', + '--language', + 'Python', + '--framework', + 'Strands', + '--model-provider', + 'Bedrock', + '--memory', + 'none', + ], + testDir + ); + if (create.exitCode !== 0) { + throw new Error(`Failed to create project: ${create.stdout} ${create.stderr}`); + } + projectDir = join(testDir, projectName); + agentConfigPath = join(projectDir, 'agentcore', 'agentcore.json'); + + await writeFile( + join(projectDir, 'agentcore', 'aws-targets.json'), + JSON.stringify([{ name: 'default', account: '123456789012', region: 'us-east-1' }]) + ); + + // ...then strip the vpcId to simulate a config written before the field existed. + const spec = JSON.parse(await readFile(agentConfigPath, 'utf-8')); + for (const rt of spec.runtimes ?? []) { + if (rt.networkConfig) delete rt.networkConfig.vpcId; + } + await writeFile(agentConfigPath, JSON.stringify(spec, null, 2)); + }); + + afterAll(async () => { + await rm(testDir, { recursive: true, force: true }); + }); + + it('leaves agentcore.json unchanged after a --dry-run (even when the preview fails without creds)', async () => { + const before = await readFile(agentConfigPath, 'utf-8'); + expect(before).not.toContain('vpcId'); + + // No real AWS creds/bootstrap in CI, so the dry-run will fail somewhere after the backfill (at + // DescribeSubnets or synth). Either way the finally-block restore must revert the file. + await runCLI(['deploy', '--dry-run', '--json'], projectDir); + + const after = await readFile(agentConfigPath, 'utf-8'); + expect(after, 'preview must not persist the backfilled vpcId').toBe(before); + expect(after).not.toContain('vpcId'); + }); +}); + describe('selectTargetStack', () => { // Multi-target projects synth one stack per target in aws-targets.json. The deploy flow // must persist/describe the stack for the *deployed* target — not blindly stackNames[0]. diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 6bba7e9bf..78d50dbc8 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -160,6 +160,10 @@ export async function runDeploy(toolkitWrapper: CdkToolkitWrapper, stackName: st export async function handleDeploy(options: ValidatedDeployOptions): Promise { let toolkitWrapper = null; let restoreEnv: (() => void) | null = null; + // In preview modes (--dry-run / --diff) the vpcId backfill writes to agentcore.json/harness.json so + // the synth subprocess can read it; this reverts those writes on EVERY exit path (success, early + // return, or throw) so a preview never leaves the working tree dirty. No-op on a real deploy. + let previewRestore: (() => Promise) | null = null; const logger = new ExecLogger({ command: 'deploy' }); const { onProgress } = options; let currentStepName = ''; @@ -417,6 +421,9 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0) { logger.log(`Resolved networkConfig.vpcId for Container+VPC build(s): ${backfill.backfilled.join(', ')}`, 'info'); } @@ -479,10 +486,9 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise Date: Mon, 6 Jul 2026 18:43:38 +0000 Subject: [PATCH 3/3] fix(deploy): make preview vpcId rollback total and drift-proof (review follow-up) Addresses five review comments on #1682: - Roll back partial writes on a mid-run failure: if a later resource's resolveVpcIdFromSubnets throws after an earlier file was already rewritten in a preview, the function now reverts the written files before propagating, so a failed dry-run/diff never leaves the working tree dirty (previously `restore` was only handed back on the success path, so the caller's finally couldn't run). - Make restore() best-effort and total: each file is reverted independently and any failures are aggregated, so one bad write no longer skips the rest. - Run each deploy `finally` cleanup step in its own try/catch so a throwing toolkitWrapper.dispose() (common after a creds-less preview) can't skip the vpcId rollback or the env restore. - Simplify snapshot() to take a path directly (the persist guard already runs first); stub the ConfigIO path getters in the backfill test mock to match the real interface. - Hoist the duplicated create-harness validation input to a single object shared by both call sites, removing the drift that caused the original blocker. Adds a multi-resource partial-write rollback regression test. Full unit suite green (6145). --- src/cli/commands/create/command.tsx | 60 +++++------- src/cli/commands/deploy/actions.ts | 15 ++- .../deploy/__tests__/backfill-vpc-id.test.ts | 48 +++++++++ src/cli/operations/deploy/backfill-vpc-id.ts | 98 ++++++++++++------- 4 files changed, 144 insertions(+), 77 deletions(-) diff --git a/src/cli/commands/create/command.tsx b/src/cli/commands/create/command.tsx index a8b5ed4af..2250bf505 100644 --- a/src/cli/commands/create/command.tsx +++ b/src/cli/commands/create/command.tsx @@ -160,27 +160,29 @@ async function handleCreateHarnessCLI(options: CreateOptions): Promise { const name = options.name ?? options.projectName; const projectName = options.projectName ?? name; + // Single source for the harness validation input, shared by the dry-run and telemetry-wrapped + // paths below. Keeping ONE object prevents the two call sites from drifting — forwarding a field + // on one path but not the other is exactly what caused the create-harness VPC validation blocker. + const harnessValidationInput = { + name, + projectName, + modelProvider: options.modelProvider, + modelId: options.modelId, + apiKeyArn: options.apiKeyArn, + container: options.container, + networkMode: options.networkMode, + subnets: options.subnets, + securityGroups: options.securityGroups, + vpcId: options.vpcId, + efsAccessPointArn: options.efsAccessPointArn, + efsMountPath: options.efsMountPath, + s3AccessPointArn: options.s3AccessPointArn, + s3MountPath: options.s3MountPath, + }; + // Handle dry-run mode (no telemetry for dry-run) if (options.dryRun) { - const validation = validateCreateHarnessOptions( - { - name, - projectName, - modelProvider: options.modelProvider, - modelId: options.modelId, - apiKeyArn: options.apiKeyArn, - container: options.container, - networkMode: options.networkMode, - subnets: options.subnets, - securityGroups: options.securityGroups, - vpcId: options.vpcId, - efsAccessPointArn: options.efsAccessPointArn, - efsMountPath: options.efsMountPath, - s3AccessPointArn: options.s3AccessPointArn, - s3MountPath: options.s3MountPath, - }, - cwd - ); + const validation = validateCreateHarnessOptions(harnessValidationInput, cwd); if (!validation.valid) { if (options.json) { console.log(JSON.stringify({ success: false, error: validation.error })); @@ -213,25 +215,7 @@ async function handleCreateHarnessCLI(options: CreateOptions): Promise { network_mode: standardize(NetworkModeEnum, options.networkMode ?? 'public'), }, async () => { - const validation = validateCreateHarnessOptions( - { - name, - projectName, - modelProvider: options.modelProvider, - modelId: options.modelId, - apiKeyArn: options.apiKeyArn, - container: options.container, - networkMode: options.networkMode, - subnets: options.subnets, - securityGroups: options.securityGroups, - vpcId: options.vpcId, - efsAccessPointArn: options.efsAccessPointArn, - efsMountPath: options.efsMountPath, - s3AccessPointArn: options.s3AccessPointArn, - s3MountPath: options.s3MountPath, - }, - cwd - ); + const validation = validateCreateHarnessOptions(harnessValidationInput, cwd); if (!validation.valid) { return { success: false as const, error: new ValidationError(validation.error!) }; } diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 78d50dbc8..6aeaf852a 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -989,12 +989,23 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise = {}): { writeProjectSpec, writeHarnessSpec, readHarnessSpec: (name: string) => Promise.resolve(harnessSpecs[name]), + // Path getters are part of the real ConfigIO interface; snapshot() calls them on the preview + // (persist=false) path. Stub them so the mock matches the interface even for persist=true tests. + getAgentConfigPath: () => '/tmp/mock-agentcore.json', + getHarnessConfigPath: (name: string) => `/tmp/mock-${name}.json`, ...overrides, } as unknown as ConfigIO; return { configIO, writeProjectSpec, writeHarnessSpec, harnessSpecs }; @@ -207,5 +211,49 @@ describe('backfillContainerVpcIds', () => { await result.restore(); // no-op on a real deploy expect(readFileSync(agentPath, 'utf-8')).toContain('vpc-0123456789abcdef0'); }); + + it('rolls back an already-written file when a LATER resource resolve throws (multi-resource preview)', async () => { + // Runtime resolves + writes agentcore.json, THEN the harness resolve throws (e.g. DescribeSubnets + // denied / subnets span VPCs). The function rejects before returning `restore`, so the caller + // can't revert — the rollback must happen internally so the preview never leaves the tree dirty. + const dir = mkdtempSync(join(tmpdir(), `backfill-${randomUUID()}-`)); + const agentPath = join(dir, 'agentcore.json'); + const original = JSON.stringify({ name: 'proj', runtimes: [{ name: 'a' }] }, null, 2); + writeFileSync(agentPath, original, 'utf-8'); + + const runtime = containerVpcRuntime(); + const harness = { + name: 'h1', + dockerfile: 'Dockerfile', + networkMode: 'VPC', + networkConfig: { subnets: ['subnet-0000000000000000b'], securityGroups: ['sg-0000000000000000b'] }, + } as unknown as HarnessSpec; + const spec = { + name: 'proj', + runtimes: [runtime], + harnesses: [{ name: 'h1' }], + } as unknown as AgentCoreProjectSpec; + + const configIO = { + getAgentConfigPath: () => agentPath, + getHarnessConfigPath: (n: string) => join(dir, `${n}.json`), + writeProjectSpec: (s: AgentCoreProjectSpec) => { + writeFileSync(agentPath, JSON.stringify(s, null, 2), 'utf-8'); + return Promise.resolve(); + }, + readHarnessSpec: () => Promise.resolve(harness), + writeHarnessSpec: () => Promise.resolve(), + } as unknown as ConfigIO; + + // First subnet (runtime) resolves; second (harness) throws. + mockResolveVpcId + .mockResolvedValueOnce('vpc-0123456789abcdef0') + .mockRejectedValueOnce(new Error('Subnets span multiple VPCs')); + + await expect(backfillContainerVpcIds(configIO, spec, 'us-east-1', false)).rejects.toThrow(/span multiple VPCs/); + + // The runtime write must have been rolled back — agentcore.json is byte-for-byte the original. + expect(readFileSync(agentPath, 'utf-8')).toBe(original); + }); }); }); diff --git a/src/cli/operations/deploy/backfill-vpc-id.ts b/src/cli/operations/deploy/backfill-vpc-id.ts index f2126079f..d881f47ef 100644 --- a/src/cli/operations/deploy/backfill-vpc-id.ts +++ b/src/cli/operations/deploy/backfill-vpc-id.ts @@ -11,6 +11,9 @@ export interface BackfillVpcIdResult { * Restore the config files this call rewrote to their original on-disk bytes. Populated only when * `persist` is false (dry-run/plan): the value is still written to disk so the CDK synth subprocess * can read it, then reverted so a preview leaves the working tree untouched. No-op on a real deploy. + * + * Best-effort and total: it attempts every rewritten file even if one revert fails, then throws an + * aggregate error if any did. Safe to call more than once. */ restore: () => Promise; } @@ -27,8 +30,12 @@ export interface BackfillVpcIdResult { * CDK synth is a separate process that re-reads agentcore.json / harness.json from disk, so the * resolved value must be written there. On a real deploy (`persist: true`) the write is permanent. * On a preview (`persist: false` — dry-run/plan) the write is temporary: the returned `restore()` - * reverts the files to their original bytes after synth, honoring the "preview without deploying" - * contract (the working tree is left untouched). + * reverts the files after synth, honoring the "preview without deploying" contract. + * + * A preview write is ALWAYS revertible, even on a mid-run failure: if resolution throws after one or + * more files were already rewritten (e.g. a second Container+VPC resource whose subnets span VPCs or + * whose DescribeSubnets is throttled), the partial writes are rolled back before the error + * propagates — so a failed preview never leaves the working tree dirty. * * @returns the names that were backfilled and a `restore()` callback (a no-op when persist is true). */ @@ -40,51 +47,68 @@ export async function backfillContainerVpcIds( ): Promise { const backfilled: string[] = []; // path -> original bytes, captured before the first rewrite of each file (preview mode only, so - // restore() can revert them). Never touched on a real deploy (persist=true). + // restore() can revert them). Never populated on a real deploy (persist=true). const originals = new Map(); - // Snapshot a file's current bytes before we rewrite it — but only in preview mode. Computes the - // path lazily so a real deploy never calls the path getter (keeps the persist=true path minimal). - const snapshot = async (getPath: () => string): Promise => { - if (persist) return; - const path = getPath(); - if (originals.has(path)) return; + // Snapshot a file's current bytes before we rewrite it — preview mode only, once per path. + const snapshot = async (path: string): Promise => { + if (persist || originals.has(path)) return; originals.set(path, await readFile(path, 'utf-8')); }; - // Runtimes live inline in agentcore.json; mutate the spec and write it back once at the end if any - // changed. - let runtimesChanged = false; - for (const runtime of projectSpec.runtimes ?? []) { - const nc = runtime.networkConfig; - if (isContainerBuild(runtime) && runtime.networkMode === 'VPC' && nc && nc.subnets.length > 0 && !nc.vpcId) { - nc.vpcId = await resolveVpcIdFromSubnets(nc.subnets, region); - runtimesChanged = true; - backfilled.push(runtime.name); + // Revert every rewritten file to its snapshot. Best-effort/total: attempts each path even if a + // prior one fails, then throws an aggregate so callers (e.g. a deploy `finally`) still run their + // other cleanup. No-op on a real deploy (nothing snapshotted). + const restore = async (): Promise => { + const failures: string[] = []; + for (const [path, content] of originals) { + try { + await writeFile(path, content, 'utf-8'); + } catch (err) { + failures.push(`${path}: ${(err as Error).message ?? String(err)}`); + } } - } - if (runtimesChanged) { - await snapshot(() => configIO.getAgentConfigPath()); - await configIO.writeProjectSpec(projectSpec); - } + if (failures.length > 0) { + throw new Error(`Failed to restore ${failures.length} config file(s) after preview: ${failures.join('; ')}`); + } + }; - // Harnesses live in their own harness.json files; resolve + persist each independently. - for (const entry of projectSpec.harnesses ?? []) { - const harness = await configIO.readHarnessSpec(entry.name); - const nc = harness.networkConfig; - if (isContainerBuild(harness) && harness.networkMode === 'VPC' && nc && nc.subnets.length > 0 && !nc.vpcId) { - nc.vpcId = await resolveVpcIdFromSubnets(nc.subnets, region); - await snapshot(() => configIO.getHarnessConfigPath(entry.name)); - await configIO.writeHarnessSpec(entry.name, harness); - backfilled.push(entry.name); + try { + // Runtimes live inline in agentcore.json; mutate the spec and write it back once at the end if + // any changed. + let runtimesChanged = false; + for (const runtime of projectSpec.runtimes ?? []) { + const nc = runtime.networkConfig; + if (isContainerBuild(runtime) && runtime.networkMode === 'VPC' && nc && nc.subnets.length > 0 && !nc.vpcId) { + nc.vpcId = await resolveVpcIdFromSubnets(nc.subnets, region); + runtimesChanged = true; + backfilled.push(runtime.name); + } + } + if (runtimesChanged) { + await snapshot(configIO.getAgentConfigPath()); + await configIO.writeProjectSpec(projectSpec); } - } - const restore = async (): Promise => { - for (const [path, content] of originals) { - await writeFile(path, content, 'utf-8'); + // Harnesses live in their own harness.json files; resolve + persist each independently. + for (const entry of projectSpec.harnesses ?? []) { + const harness = await configIO.readHarnessSpec(entry.name); + const nc = harness.networkConfig; + if (isContainerBuild(harness) && harness.networkMode === 'VPC' && nc && nc.subnets.length > 0 && !nc.vpcId) { + nc.vpcId = await resolveVpcIdFromSubnets(nc.subnets, region); + await snapshot(configIO.getHarnessConfigPath(entry.name)); + await configIO.writeHarnessSpec(entry.name, harness); + backfilled.push(entry.name); + } } - }; + } catch (err) { + // A resolve/write failed partway through a preview — roll back any files we already rewrote so + // the working tree is clean, then propagate. (No-op on a real deploy: nothing was snapshotted.) + await restore().catch(() => { + /* preserve the original error over a secondary restore failure */ + }); + throw err; + } return { backfilled, restore }; }