fix(vpc): correct create-harness VPC validation, dry-run persistence, and TUI review display (follow-up to #1671)

[aidandaly24]

Description

Follow-up to #1671. Three issues were found in the container-build VPC support after that PR merged; this PR corrects them.

1. agentcore create --network-mode VPC for a harness project (#1691)

validateCreateHarnessOptions validates the VPC options (--subnets, --security-groups, --vpc-id, and the build type derived from --container), but the two call sites in command.tsx (the dry-run path and the main path) were not passing those fields into it. As a result the validator saw them as undefined and returned --subnets is required when network mode is VPC even when the flags were supplied, so a VPC-mode harness could not be created through agentcore create, and the intended errors (missing --vpc-id, malformed --vpc-id, security-group limit) were never reached. The same VPC configuration works through the agent path of create and through add harness, so this path was simply unreachable.

Fix: forward container, subnets, securityGroups, and vpcId into validateCreateHarnessOptions at both call sites, bringing it to parity with add harness and the agent path.

The existing unit tests called validateCreateHarnessOptions directly with a fully-populated object, so they did not cover the wiring in command.tsx. Added integration tests that run the command through runCLI --dry-run to exercise that path.

2. deploy --dry-run writing to config files

The VPC-ID backfill on deploy resolves a missing networkConfig.vpcId from the subnets and writes it to agentcore.json / harness.json so the CDK synth subprocess (which reads config from disk) can use it. This write also happened under --dry-run and --diff, which are meant to preview without changing anything, so a preview left the config files modified.

Fix: backfillContainerVpcIds now takes a persist flag. In preview modes it still writes the resolved value to disk for synth, then returns a restore() callback that reverts the files to their original contents afterward, so the working tree is unchanged. The plan and diff paths call restore().

3. TUI review screen showing a stale VPC ID

In the interactive agentcore create wizard, selecting Container + VPC, entering a VPC ID, then going back and switching the build to CodeZip left the VPC ID set. The review screen then displayed a VPC ID line for a CodeZip agent, and the value could be written into agentcore.json.

Fix: guard the review-screen render and the schema-mapper write on buildType === 'Container', so a VPC ID only appears and persists for container builds.

Related Issue

Closes #1691 (issue #1 above). Issues #2 and #3 were found during follow-up testing of #1671 and do not have separate tracking issues.

Type of Change

• Bug fix

Testing

• I ran npm run test:unit — full unit suite passing (6120 tests)
• I ran npm run typecheck
• I ran npm run lint
• Added integration tests that run agentcore create through the CLI dispatch for the VPC harness path, plus tests for the dry-run restore behavior

Verified end-to-end on a build of this branch (rebased onto latest main):

• create --container ./Dockerfile --network-mode VPC --subnets ... --security-groups ... --vpc-id ... now succeeds and persists networkConfig.vpcId to harness.json (previously exited 1 with the misleading --subnets is required).
• Omitting --vpc-id now returns --vpc-id is required for Container builds with --network-mode VPC.
• A malformed --vpc-id returns Invalid VPC ID format.

New/updated tests:

• create.test.ts: harness VPC dispatch (--dry-run) — accepts a full set of VPC flags, returns the expected error when --vpc-id is missing, and rejects a malformed --vpc-id.
• backfill-vpc-id.test.ts: dry-run (persist=false) restore — writes the resolved VPC ID for synth and then reverts the file; confirms a real deploy leaves the value in place.

Checklist

• I have added tests that cover the fixes
• My changes generate no new warnings

[github-actions]

Package Tarball

aws-agentcore-0.22.0.tgz

How to install

gh release download pr-1682-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.22.0.tgz

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[agentcore-cli-automation]

Thanks for the follow-up. The three fixes are on-target and the new integration tests exercise the command.tsx → validator seam that the original unit tests bypassed — nice catch on that gap.

I do have one correctness concern with the BUG-2 fix that I'd like addressed before merge; details inline.

[agentcore-cli-automation]

backfill.restore() is only called on the two happy-path returns (plan at L485, diff at L504), but between the backfill call at L419 and those returns there are several ways the flow can bail out after agentcore.json/harness.json have already been rewritten to disk:

• synthesizeCdk at L427 can throw → caught by the outer catch at L982, no restore runs.
• Stack selection early-return at L436–L444 (still inside the same try), no restore.
• Bootstrap-required early-return at L456–L463, no restore.
• Deployability check early-return at L471–L479, no restore.
• In diff mode, runDiff at L501 can throw → outer catch, no restore.

So a --dry-run or --diff that fails for any reason after the backfill will leave the working tree dirty — which is exactly the "preview must not dirty the working tree" contract this PR's BUG-2 fix is trying to establish. The new dry-run (persist=false) restore test only covers the happy path.

A few ways to fix:

1. Register restore() for the finally block, e.g. capture it into a previewRestore: (() => Promise<void>) | undefined declared before the try and invoke it in the existing finally (L986–L991) when set. Cheapest change and covers every exit path.
2. Move the two current await backfill.restore() calls out of the happy branches entirely and rely solely on option 1.
3. Wrap the backfill + synth + preview span in its own try/finally that always calls restore() in preview mode.

Any of these will do; option 1 is the smallest diff. Please also add a regression test that forces a throw between backfillContainerVpcIds and the plan/diff short-circuit (e.g. by making synthesizeCdk reject) and asserts the on-disk files are still reverted.

[agentcore-cli-automation]

Nice targeted fix. The BUG-1 dispatch wiring is clearly right and the new integration tests exercise the seam the unit tests missed. The BUG-3 guard is a one-liner and the schema-mapper guard is consistent with it.

One concern on the BUG-2 fix — the preview-mode backfill.restore() calls are placed inline (not in a finally), so several failure paths between the backfill and the explicit restore will still leave agentcore.json / harness.json dirty on a --dry-run / --diff. That's the exact contract the fix is meant to uphold, so I think it's worth handling before merging. Details inline.

[agentcore-cli-automation]

BUG-2's contract is "a preview must not dirty the working tree" — but the restore is only called on the happy-path exits of the plan/diff blocks (L485, L504). Between the backfill at L419 and those calls there are several failure paths where the file stays dirty on a --dry-run / --diff:

• synthesizeCdk at L427 throws → caught by the outer catch at L982, restore() never runs.
• selectTargetStack fails at L436 → early return, no restore.
• Bootstrap check fails at L451-L464 → early return, no restore.
• Deployability check fails at L471-L479 → early return, no restore.
• In --diff mode, runDiff at L501 throws → outer catch, no restore.

Since restore() is a no-op when persist=true (the originals map is empty), it's safe to call unconditionally. A couple of options:

1. Move the restore into the finally block at L986 — hoist let backfill: BackfillVpcIdResult | null = null; above the try, and add if (backfill) await backfill.restore(); in finally. Cleanest and covers every failure path uniformly.
2. Wrap the preview flow in its own try/finally — keeps the scope narrower but is more invasive since synth/deployability/etc. are shared with the real deploy path.
3. At minimum, try { … } finally { await backfill.restore(); } around synth + the diff run when isPreview — narrower still but easy to get subtly wrong later.

Option 1 seems most robust and matches the pattern already used for restoreEnv / toolkitWrapper.dispose() in the same finally.

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	38.61%	14368 / 37208
🔵	Statements	37.92%	15314 / 40381
🔵	Functions	33.23%	2473 / 7441
🔵	Branches	32.36%	9570 / 29571

Generated in workflow #3984 for commit 295c184 by the Vitest Coverage Report Action

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[agentcore-cli-automation]

The follow-up commit (f8a1656) addresses the BUG-2 concern raised in both prior reviews: previewRestore is now hoisted before the outer try and invoked in the finally at L991-998, so every exit path (synth throw, stack-selection / bootstrap / deployability early returns, runDiff throw, or happy path) reverts the on-disk backfill. The new deploy.test.ts case at L62 also exercises this via a real deploy --dry-run without AWS creds — the run fails somewhere after the backfill and the test asserts agentcore.json is byte-for-byte unchanged, which is exactly the regression coverage the earlier review requested.

The other two fixes (create-harness VPC dispatch wiring in command.tsx, and the buildType === 'Container' guards in the review screen + schema-mapper) look correct, and the mocking in the new tests is confined to true I/O boundaries (EC2's resolveVpcIdFromSubnets, and ConfigIO — with real temp dirs for the on-disk assertions). No new serious issues from my pass.

[tejaskash]

Code review (high-effort, recall-biased). 8 findings; 5 posted inline below, 3 land in files this PR doesn't touch so they're summarized here.

Correctness

• [#2 — HarnessPrimitive.ts:354, not in diff] The CLI create/add-harness write path has no buildType === 'Container' guard on the vpcId write — the same stale-vpcId leak defect #3 fixes in schema-mapper.ts and GenerateWizardUI.tsx, left unfixed on this third path. agentcore create --name X --network-mode VPC --subnets ... --security-groups ... --vpc-id ... with no --container (a CodeZip harness) sets harnessBuildType=undefined, so validateVpcOptions never rejects the stray --vpc-id, and ...(options.vpcId && { vpcId: options.vpcId }) at HarnessPrimitive.ts:354 leaks a Container-only field into a CodeZip harness.json. Add the same guard here.

Altitude

• [#5 — useGenerateWizard.ts:148, not in diff] setBuildType clears dockerfile but not vpcId on a Container→CodeZip switch; this PR instead guards two read sites. That's symptom-patching stale state. setNetworkMode→PUBLIC already clears vpcId (line 269), proving the source-clear pattern exists. There are 4+ read sites of config.vpcId; clearing it in setBuildType when leaving Container (one line) makes all reads correct by default instead of requiring the buildType guard at every present and future consumer.
• [#7 — vpc-utils.ts:104, not in diff] validateVpcOptions gained an optional buildType? param, and the vpcId requirement fires only when 'Container' is passed. A caller that omits buildType gets zero vpcId validation and no error — the same forwarding-failure class this PR is fixing (59bbde0 forgot to forward fields). Make buildType required so the type system forces every caller to supply it.

Notable clears: snapshot() timing is correct (captures old on-disk bytes before the in-memory spec is written); the persist=true path is a genuine no-op; the redundant ec2:DescribeSubnets-per-resource calls are pre-existing (feature commit), not introduced here; no AGENTS.md rule violations in the new code.

[tejaskash]

Correctness — a throw inside this function after a partial write leaves the working tree dirty, defeating PATCH 2's "restore on every exit path" guarantee.

restore is only handed back on the successful return { backfilled, restore }. On deploy --dry-run with a Container+VPC runtime and a Container+VPC harness both missing vpcId (or two such harnesses): the runtime resolves and agentcore.json is snapshotted+rewritten with the vpcId, then the harness loop's resolveVpcIdFromSubnets (line 76) throws (subnets span multiple VPCs, DescribeSubnets throttled/denied). The function rejects before returning, so in actions.ts const backfill = await ... (423) throws, previewRestore = backfill.restore (426) never runs, and the finally-block restore is skipped. agentcore.json is left dirtied.

The added tests only exercise a single-resource fixture, so they never hit a post-write throw. Root-cause fix: own the rollback in the caller, or wrap the loops in try/finally here, so a partial write is always revertible — not just on a clean return.

[tejaskash]

Robustness — restore() is non-atomic. The writes run sequentially with no per-file try/catch; if the first writeFile rejects (file concurrently removed, transient EPERM), the remaining files are never reverted (partial restore) and the rejection propagates out of handleDeploy's finally, also skipping restoreEnv?.(). Make it best-effort/total: attempt every path and aggregate errors.

[tejaskash]

Simplification. The lazy getPath: () => string closure buys nothing: the first statement is already if (persist) return before getPath() is ever called, and the getters are trivial string returns. The comment "so a real deploy never calls the path getter" implies a cost that the guard already eliminates. Simplify to snapshot(path: string), called as await snapshot(configIO.getAgentConfigPath()).

[tejaskash]

Robustness — restore ordering in finally. previewRestore() runs after toolkitWrapper.dispose() (line 993). If dispose() throws — common after a synth/bootstrap failure on a creds-less preview, the exact path this PR targets — the finally block aborts before if (previewRestore) await previewRestore(), so agentcore.json stays dirty and restoreEnv?.() (999) never runs. Wrap dispose() in try/catch, or run previewRestore before dispose.

[tejaskash]

Simplification — this duplication is the shape that caused this PR's blocker. This 12-field options object is byte-identical to the telemetry-wrapped call site at line 223. The original bug (59bbde0) was forwarding VPC fields at one path but not the other; two verbatim copies guarantee the next field-add can be applied to one and forgotten on the other, reopening the same "validation sees undefined" blocker. Hoist one const harnessValidationInput = { name, projectName, ... } above the if (options.dryRun) branch and pass it to both validateCreateHarnessOptions calls.

[aidandaly24]

Thanks @tejaskash — all five addressed in 295c184b:

1. Partial-write rollback (correctness). The resolve/write loops are now wrapped so that if a later resource's resolveVpcIdFromSubnets throws after an earlier file was already rewritten in a preview, the written files are reverted before the error propagates — a failed dry-run/diff no longer leaves the tree dirty even though restore isn't handed back. Added a multi-resource regression test (runtime writes, harness resolve throws → agentcore.json asserted byte-for-byte original).
2. restore() is now best-effort/total. Each file is reverted in its own try/catch; failures are aggregated into one error at the end, so one bad write no longer skips the rest.
3. Ordering in finally. Each cleanup step (dispose(), previewRestore(), restoreEnv?.()) now runs in its own try/catch, so a throwing dispose() can't skip the rollback or env restore.
4. Simplified snapshot() to take a path directly (the if (persist) return guard already runs first); the backfill test mock now stubs the ConfigIO path getters to match the real interface.
5. De-duplicated the create-harness validation input into a single harnessValidationInput object shared by both call sites — removing the two-copies drift that caused the original blocker.

Full unit suite green (6145).

[agentcore-devx-automation]

Claude Security Review: no high-confidence findings. (run)

[agentcore-cli-automation]

LGTM. The three targeted fixes are on-point:

• BUG-1 (create-harness VPC dispatch): the shared harnessValidationInput object addresses both the immediate wiring gap and the drift-vector that caused it. Integration tests through runCLI --dry-run correctly cover the seam the direct-validator unit tests bypassed.
• BUG-2 (preview dry-run/diff persistence): the persist flag + restore() design, plus the hoisted previewRestore in handleDeploy's finally with per-step try/catch isolation, correctly enforces the "preview must not dirty the working tree" contract on every exit path (early returns, throws, dispose failures). Internal try/catch inside backfillContainerVpcIds also handles the mid-loop partial-write case that a caller-owned restore can't reach.
• BUG-3 (stale VPC ID in TUI review + schema-mapper): the buildType === 'Container' guards are consistent between the two read sites.

All the correctness concerns I would have raised are already flagged in existing comments — most notably the parallel guard needed at HarnessPrimitive.ts:354, the setBuildType source-clear for vpcId in useGenerateWizard.ts, and making buildType required on validateVpcOptions to prevent the exact same forwarding-failure class this PR is fixing. Those are all out-of-diff and can be addressed in a follow-up. No new blocking issues from my pass.

[tejaskash]

LGTM