fix(deploy): scope TUI deploy to the picker-selected targets (#1267)#1659
fix(deploy): scope TUI deploy to the picker-selected targets (#1267)#1659aidandaly24 wants to merge 4 commits into
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) |
|
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 |
|
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) |
|
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.
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
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.