Uh oh!
There was an error while loading. Please reload this page.
fix(deploy): scope TUI deploy to the picker-selected targets (#1267) - #1659
Conversation
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
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh 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
left a comment
There was a problem hiding this comment.
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:
- Land #1302 first (or fold its
awsTargetsfiltering into this PR) so the picker selection flows through preflight before this deploy-scope change goes in. - 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 }); |
There was a problem hiding this comment.
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) readsstackNames[0]andctx.awsTargets[0]to poll outputs and write deployed-state. For a[A, B]project where the user picksB, this pollsA's stack inA's region (which was never deployed) and writes state under targetAinstead ofB.- 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:
- Merge fix(deploy): respect TUI target selection in deploy flow #1302 first so
context.awsTargetsis already filtered to the selection by the time it reachesuseDeployFlow. - Fold fix(deploy): respect TUI target selection in deploy flow #1302's filtering into this PR.
- At minimum, in this hook resolve the active target(s) from
selectedTargets(falling back tocontext.awsTargets) and use that forpersistDeployedState, teardown, and transaction-search instead ofcontext.awsTargets[0]/stackNames[0].
There was a problem hiding this comment.
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 derivescurrentStackNameviatoStackName(projectName, activeTarget.name)(instead ofstackNames[0]) and pollsactiveTarget.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 fromactiveTargetinstead ofawsTargets[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')), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
commented
Jun 29, 2026
Reviewed this PR end-to-end. The existing review from
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 |
aidandaly24
commented
Jun 29, 2026
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 2. Diff path is now scoped. All three Two new regression tests lock both fixes in: persist polls the selected target's region+stack (not |
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
commented
Jun 29, 2026
Re-reviewed after the
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 LGTM to merge. |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…mpty selection Addresses review on aws#1267: - deploy summary now reports the picker-scoped stack set, not the full synthesized list (no longer claims it deployed unpicked targets) - warn when a multi-target pick is bookkept only for the first target - an empty picker selection now scopes to nothing (PATTERN_MUST_MATCH with no patterns -> toolkit NoStacksMatched) instead of falling back to ALL_STACKS
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Reviewed the PR end-to-end (all commits through d899da3d) after reading the prior review threads. Every serious concern I would have raised has already been flagged and addressed:
- Post-deploy bookkeeping on
awsTargets[0]/stackNames[0]— raised inline atuseDeployFlow.tsL810 in the first review, fixed ina37a9b5via theactiveTargetmemo threaded throughpersistDeployedState, the teardown branch (cleanupPaymentCredentialProviders+performStackTeardown), and transaction-search setup. - Unscoped diff paths — raised in the first review summary, fixed in
a37a9b5by passing{ stacks: deployStacks }to all threecdkToolkitWrapper.diff()call sites (pre-deploy L776, standalone diff-mode L1057, on-demandrequestDiff). - Persist path not exercised by the regression test — raised inline on the test file, closed in
a37a9b5via the hoistedgetStackOutputsSpy+persist polls the SELECTED target stack/region…test. - Success summary listing every synthesized stack instead of the scoped set — raised at L927, fixed in
d899da3d(deployStacks?.patterns ?? stackNames) with a regression test. - Empty picker selection silently falling back to ALL_STACKS — raised at L165, fixed in
d899da3dby distinguishingundefined(unscoped) from[](PATTERN_MUST_MATCH with no patterns → toolkit-lib throwsNoStacksMatched) with a regression test. - Multi-target pick leaves non-first targets without persist/transaction-search — raised at L183, addressed in
d899da3dwith a user-visible warning and tracked for full per-target bookkeeping under a follow-up issue.
Mocking surface in the new test file stays at real I/O boundaries (CDK toolkit wrapper, preflight hook, CloudFormation SDK, telemetry wrapper, filesystem-reading operations). Not excessive given useDeployFlow orchestrates half a dozen subsystems that would each need substantial fixtures to run for real; mocking at the hook boundary is the pragmatic choice here.
No new blockers or additional issues to flag from me. LGTM to merge.
Uh oh!
There was an error while loading. Please reload this page.
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
Related Issue
Closes#1267
Documentation PR
N/A — bug fix; no devguide change required.
Type of Change
Testing
How have you tested the change? (full validation in PR comments after bug bash)
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshots — N/A, nosrc/assets/changesChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.