Skip to content

fix(deploy): scope TUI deploy to the picker-selected targets (#1267)#1659

Draft
aidandaly24 wants to merge 4 commits into
aws:mainfrom
aidandaly24:fix/1267
Draft

fix(deploy): scope TUI deploy to the picker-selected targets (#1267)#1659
aidandaly24 wants to merge 4 commits into
aws:mainfrom
aidandaly24:fix/1267

Conversation

@aidandaly24

Copy link
Copy Markdown
Contributor

Description

#1267

In the TUI deploy flow, when aws-targets.json has more than one target the multi-select picker lets the user choose a subset, but the choice is dropped: the deploy screen header lists every target (cosmetic), and far worse, the deploy itself runs against ALL synthesized stacks (all targets), not the selected one(s). A user who picks a single target silently deploys real infrastructure to every configured account/region. Only affects multi-target projects; single-target (the default) is correct.

Fix

Thread the picker selection through preflight AND scope the deploy. (1) Display + context (what open PR #1302 does): add selectedTargets?: AwsDeploymentTarget[] to PreflightOptions/DeployFlowOptions; DeployScreen derives it from awsConfig.selectedTargetIndices over availableTargets and passes it down; useCdkPreflight filters preflightContext.awsTargets to the selected names after validateProject(). This fixes the header (DeployScreen.tsx:279) and makes awsTargets[0] reads (persist/teardown/transaction-search; checkBootstrapNeeded preflight.ts:307) reflect the user's choice. (2) ADDITIONALLY (NOT in PR #1302, and the load-bearing part): scope the actual deploy by passing a stacks selector into the TUI cdkToolkitWrapper.deploy() at useDeployFlow.ts:784, built from the selected target names mapped through toStackName(projectName, targetName) with StackSelectionStrategy.PATTERN_MUST_MATCH, e

This fix was produced by an automated triage+fix run and validated locally (build + unit suite + symptom reproduction). Opened as a draft for CI and human review; will be bug-bashed before it's marked ready.

Related Issue

Closes #1267

Documentation PR

N/A — bug fix; no devguide change required.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

How have you tested the change? (full validation in PR comments after bug bash)

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots — N/A, no src/assets/ changes

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

The TUI multi-select target picker records the user's choice in
selectedTargetIndices, but nothing downstream consumed it: useDeployFlow
called cdkToolkitWrapper.deploy() with no stacks selector, so toolkit-lib
defaulted to ALL_STACKS and deployed every synthesized stack (one per
configured target). A user who picked a single target silently deployed
infrastructure to every configured account/region.

Thread selectedTargets from DeployScreen through useDeployFlow and scope
the deploy with StackSelectionStrategy.PATTERN_MUST_MATCH over
toStackName(project, target), mirroring CLI mode. Also fix the header to
show the picked target(s) rather than the full configured list.

Refs aws#1267
@github-actions github-actions Bot added size/m PR size: M agentcore-harness-reviewing AgentCore Harness review in progress labels Jun 29, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

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

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Package Tarball

aws-agentcore-0.21.0.tgz

How to install

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

@agentcore-cli-automation agentcore-cli-automation left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix — the deploy scoping itself is correct and the regression test is well-targeted.

I have two concerns about completeness that I think should be resolved before merging.

1. Post-deploy bookkeeping still operates on the wrong target (see inline on useDeployFlow.ts). The PR's stated dependency on #1302 is load-bearing: the description acknowledges that filtering context.awsTargets (so that awsTargets[0] reads in persist/teardown/transaction-search reflect the user's choice) is what #1302 does, but #1302 is still open and not merged. As of this PR alone, the deploy goes to the right target while the post-deploy bookkeeping does not — arguably a worse state than today's bug for affected users (silent state corruption rather than over-deployment).

Options:

  1. Land #1302 first (or fold its awsTargets filtering into this PR) so the picker selection flows through preflight before this deploy-scope change goes in.
  2. Combine the two PRs into one end-to-end fix for #1267.

2. Diff path is still unscoped. The pre-deploy cdkToolkitWrapper.diff() (around line 747) and the standalone diff-mode call (around line 1010) are still called without stacks. When a multi-target project's user picks one target the diff still runs against ALL synthesized stacks — same class of bug as the deploy issue, just without provisioning real infra. The PR title says "deploy" so this may be deliberate, but worth either fixing here (pass { stacks: deployStacks } to both diff() calls) or explicitly calling out as out-of-scope and tracked separately.

See inline comments for specifics.

// Output goes to stdout via the switchable ioHost.
// deployStacks restricts the deploy to the picker's selected targets; undefined (single
// target or plan path) lets the assembly's lone stack deploy as before.
await cdkToolkitWrapper.deploy({ stacks: deployStacks });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deploy is now correctly scoped to the picked target's stack, but downstream code in this same effect still assumes a single-target world via awsTargets[0] / stackNames[0]:

  • persistDeployedState() (line 205-206) reads stackNames[0] and ctx.awsTargets[0] to poll outputs and write deployed-state. For a [A, B] project where the user picks B, this polls A's stack in A's region (which was never deployed) and writes state under target A instead of B.
  • The teardown branch (around line 399) uses context.awsTargets[0]?.name — runs teardown for the wrong target.
  • The transaction-search setup (around lines 417-418) uses context.awsTargets[0]?.region / .account — wires CloudWatch in the wrong account/region.

Without #1302 (which filters preflightContext.awsTargets to the selected names), these post-deploy paths silently operate on the wrong target. The deploy bug is fixed but a new state-corruption bug is introduced for multi-target users who pick a subset.

Options:

  1. Merge fix(deploy): respect TUI target selection in deploy flow #1302 first so context.awsTargets is already filtered to the selection by the time it reaches useDeployFlow.
  2. Fold fix(deploy): respect TUI target selection in deploy flow #1302's filtering into this PR.
  3. At minimum, in this hook resolve the active target(s) from selectedTargets (falling back to context.awsTargets) and use that for persistDeployedState, teardown, and transaction-search instead of context.awsTargets[0] / stackNames[0].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a37a9b5 by taking your Option 3 (resolve the active target in-hook, no dependency on the unmerged #1302).

Added a memoized activeTarget = selectedTargets?.[0] ?? context?.awsTargets[0] and routed the post-deploy bookkeeping through it:

  • persistDeployedState() now derives currentStackName via toStackName(projectName, activeTarget.name) (instead of stackNames[0]) and polls activeTarget.region — so a non-first pick polls the stack that was actually deployed and writes deployed-state under the right target name.
  • The teardown branch now resolves targetName/region from activeTarget instead of awsTargets[0].
  • Transaction-search setup now reads activeTarget.region/.account.

Fall-throughs are preserved: the plan path (no project name / no picker) and the no-selection case still fall back to stackNames[0] / context.awsTargets[0], matching prior behavior. The TUI persists a single target; when several are picked we bookkeep the first selected one (the same target whose outputs we poll). New regression test asserts the persist path polls B's region + B's stack when B is the non-first pick.

const actual = await vi.importActual<any>('../../../../cloudformation');
return {
...actual,
getStackOutputs: vi.fn().mockRejectedValue(new Error('test: skip persist')),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because getStackOutputs is mocked to reject, the test never exercises the post-deploy persistDeployedState path — which is exactly the path that's broken when the picked target isn't awsTargets[0] (see the inline comment on useDeployFlow.ts around line 810). The test asserts deploy() is called with the right scope but doesn't catch the end-to-end regression for multi-target picks. Adding coverage that the persist path resolves outputs from the selected target's region/stack (not awsTargets[0] / stackNames[0]) would make the gap in the previous comment self-enforcing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — closed the coverage gap in a37a9b5. The getStackOutputs mock is now a hoisted spy (so the test can keep a handle while still short-circuiting the long poll), and a new test persist polls the SELECTED target stack/region, not awsTargets[0]/stackNames[0] renders with selectedTargets=[B] on an [A, B] project and asserts the persist path calls getStackOutputs(B.region, toStackName(project, B.name)) — and never A's region/stack. Before the hook fix this asserted on A's values, so the regression is now self-enforcing. Also added scopes the pre-deploy diff to the selected target stack to cover the diff-scoping fix.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 29, 2026
@github-actions github-actions Bot added size/m PR size: M agentcore-harness-reviewing AgentCore Harness review in progress and removed size/m PR size: M labels Jun 29, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

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

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed this PR end-to-end. The existing review from agentcore-cli-automation already flags every serious issue I would have raised:

  1. Post-deploy bookkeeping (useDeployFlow.ts ~L290-291, L831, L838, L870-871)persistDeployedState, the teardown branch, and the transaction-search setup all still read context.awsTargets[0] / stackNames[0]. When a multi-target user picks a non-first target, the deploy now goes to the right stack but the post-deploy paths write deployed-state under the wrong target, polish outputs from a stack that wasn't deployed, and wire CloudWatch in the wrong account/region — a state-corruption regression. Already covered in the inline at L810 with concrete fix options.
  2. Diff paths unscoped (useDeployFlow.ts L747 pre-deploy diff, L1010 standalone diff-mode) — same class of bug as the original (multi-target user picks one → diff runs against all). Already raised in the review summary.
  3. Test gap on the persist pathgetStackOutputs is mocked to reject so the regression test never exercises the persist code that issue build(deps): bump diff and @aws-cdk/cloudformation-diff #1 lives in. Already raised inline on the test file at L60.

No additional issues to flag — deferring to the existing review and not duplicating its comments. Approving the deploy-scoping change in isolation is correct, but I agree with the existing reviewer's CHANGES_REQUESTED state: the post-deploy bookkeeping needs to be fixed in this PR (or #1302 landed first) before merging, otherwise this trades over-deployment for silent state corruption on the exact same multi-target/non-first-pick path.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 29, 2026
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Jun 29, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@aidandaly24

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Both concerns are addressed in a37a9b5 — kept the change surgical and avoided depending on the unmerged #1302.

1. Post-deploy bookkeeping now tracks the deployed target. Added a memoized activeTarget (selectedTargets?.[0] ?? context?.awsTargets[0]) and routed persistDeployedState (stack name now via toStackName(project, activeTarget.name), region via activeTarget.region), the teardown branch, and transaction-search through it instead of awsTargets[0] / stackNames[0]. The plan path and no-selection case still fall back to the prior values, so single-target and pre-synthesized flows are unchanged. This removes the state-corruption window for multi-target users who pick a non-first target.

2. Diff path is now scoped. All three cdkToolkitWrapper.diff() call sites — the pre-deploy diff, the standalone diff-mode run, and the on-demand requestDiff — now pass { stacks: deployStacks }, so the diff mirrors exactly what will deploy. deployStacks is undefined (→ full assembly) when nothing is picked / on the plan path, preserving existing behavior.

Two new regression tests lock both fixes in: persist polls the selected target's region+stack (not awsTargets[0]), and the pre-deploy diff is scoped to the selected target's stack. Full local suite: prettier --check, eslint src/, typecheck, build, and the 7-test deploy unit file all green.

@github-actions github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 29, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

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

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@agentcore-cli-automation

Copy link
Copy Markdown

Re-reviewed after the a37a9b5 fixup. Both prior concerns are properly resolved:

  1. Post-deploy bookkeepingactiveTarget = selectedTargets?.[0] ?? context?.awsTargets[0] is now threaded through persistDeployedState (stack name via toStackName(project, target.name), region via target.region), the teardown branch (cleanupPaymentCredentialProviders + performStackTeardown(targetName)), and the transaction-search setup. Fallbacks preserve single-target and pre-synthesized plan paths. Verified by grepping all remaining awsTargets[0] / stackNames[0] references — they're all in comments or fallback branches now.
  2. Diff scoping — all three cdkToolkitWrapper.diff() call sites (pre-deploy at L768, standalone diff-mode at L1034, on-demand at L284) now pass { stacks: deployStacks }. deployStacks correctly resolves to undefined when nothing is picked / on the plan path, so the assembly-wide fallback is preserved.
  3. Test coverage — the new persist polls the SELECTED target stack/region test (rendering with selectedTargets=[B] on an [A, B] project and asserting getStackOutputs is called with B's region + toStackName(project, B.name), never A's) closes the gap on the previously-untested persist path. The hoisted getStackOutputsSpy pattern is clean. The new scopes the pre-deploy diff to the selected target stack test exercises the isFirstDeploy=false branch.

Mocking surface stays at real I/O boundaries (preflight hook, telemetry wrapper, CloudFormation SDK, operations/deploy filesystem reads) — not excessive.

No new merge-blocking issues from me.

One latent follow-up (not a blocker, pre-existing, not introduced by this PR): when the user picks multiple targets in the multi-select (e.g. 2 of 3), the deploy correctly scopes to those 2 via PATTERN_MUST_MATCH, but activeTarget = selectedTargets[0] means persistDeployedState / teardown / transaction-search only run for the first picked target. The second deployed stack's outputs never make it into deployed-state.json. The code comment on the activeTarget memo ("if the user picked several, we bookkeep the first selected one") explicitly acknowledges this. This was true before the PR too — back then the deploy hit all configured targets but persist wrote awsTargets[0] only — so it isn't a regression. Worth tracking as a separate issue since the multi-pick path is now more reachable in practice (users can deliberately pick 2 targets and have both actually deploy).

LGTM to merge.

@agentcore-cli-automation agentcore-cli-automation left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The two prior CHANGES_REQUESTED issues (post-deploy bookkeeping operating on awsTargets[0] and the unscoped diff paths) are properly addressed in a37a9b5 via activeTarget + { stacks: deployStacks } on all three diff sites, and the new persist regression test closes the coverage gap on the post-deploy path. Details in the comment thread. One latent multi-pick persist gap noted as a follow-up — pre-existing, not a regression, not a blocker.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

All targets are shown even if only 1 was selected

2 participants