Skip to content

Commit ca1a7ee

Browse files
author
agentcore-bot
committed
fix: propagate aws-targets.json region to all CLI entry points (#924)
Make the resolved deployment target's region authoritative on the process environment for every CLI command that may construct AWS SDK clients (or invoke CDK toolkit-lib internals) without an explicit `region` option. Previously this override existed only in `deploy`, `teardown`, and the TUI preflight hook. The `import` command mutated AWS_REGION / AWS_DEFAULT_REGION directly without a finally restore (leaking env state to subsequent in-process work) and `abtest`'s region resolver could silently fall back to us-east-1 even when aws-targets.json was present. Changes - target-region: add `runWithTargetRegion` convenience wrapper and a policy comment documenting that new entry points MUST apply the override after resolving their target. - import/actions: replace bare process.env mutation with `applyTargetRegionToEnv`; re-apply with the resolved target region (overrides the YAML-based initial region) and restore in a top-level finally block on both success and error paths. - abtest/command: have `getRegion()` also promote the resolved region onto AWS_REGION/AWS_DEFAULT_REGION so any helper constructing an SDK client without an explicit region behaves consistently. - Add focused region-override tests for handleDeploy and handleImport that exercise both the happy path and the error path, and extend the target-region unit tests to cover `runWithTargetRegion`. Refs #924
1 parent 9f231d0 commit ca1a7ee

7 files changed

Lines changed: 590 additions & 12 deletions

File tree

src/cli/aws/__tests__/target-region.test.ts

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { applyTargetRegionToEnv, withTargetRegion } from '../target-region.js';
1+
import type { AwsDeploymentTarget } from '../../../schema';
2+
import { applyTargetRegionToEnv, runWithTargetRegion, withTargetRegion } from '../target-region.js';
23
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
34

45
describe('target-region', () => {
@@ -96,4 +97,72 @@ describe('target-region', () => {
9697
expect(result).toBe(42);
9798
});
9899
});
100+
101+
describe('runWithTargetRegion', () => {
102+
const buildTarget = (region: string): AwsDeploymentTarget => ({
103+
name: 'default',
104+
account: '123456789012',
105+
region: region as AwsDeploymentTarget['region'],
106+
});
107+
108+
it('applies the resolved target region inside the callback and restores afterwards', async () => {
109+
let seenRegion: string | undefined;
110+
const target = buildTarget('ap-southeast-2');
111+
112+
const result = await runWithTargetRegion(
113+
() => Promise.resolve(target),
114+
resolved => {
115+
seenRegion = process.env.AWS_REGION;
116+
expect(resolved).toBe(target);
117+
return Promise.resolve('ok');
118+
}
119+
);
120+
121+
expect(seenRegion).toBe('ap-southeast-2');
122+
expect(result).toBe('ok');
123+
expect(process.env.AWS_REGION).toBeUndefined();
124+
expect(process.env.AWS_DEFAULT_REGION).toBeUndefined();
125+
});
126+
127+
it('skips the env override when no target is resolved', async () => {
128+
let seenRegion: string | undefined;
129+
130+
await runWithTargetRegion(
131+
() => Promise.resolve(undefined),
132+
resolved => {
133+
seenRegion = process.env.AWS_REGION;
134+
expect(resolved).toBeUndefined();
135+
return Promise.resolve();
136+
}
137+
);
138+
139+
expect(seenRegion).toBeUndefined();
140+
expect(process.env.AWS_REGION).toBeUndefined();
141+
});
142+
143+
it('restores env vars even when the callback throws', async () => {
144+
process.env.AWS_REGION = 'us-east-1';
145+
146+
await expect(
147+
runWithTargetRegion(
148+
() => Promise.resolve(buildTarget('sa-east-1')),
149+
() => Promise.reject(new Error('boom'))
150+
)
151+
).rejects.toThrow('boom');
152+
153+
expect(process.env.AWS_REGION).toBe('us-east-1');
154+
});
155+
156+
it('propagates errors from the target resolver without mutating env', async () => {
157+
await expect(
158+
runWithTargetRegion(
159+
() => Promise.reject(new Error('cannot resolve')),
160+
() => Promise.resolve('unreached')
161+
)
162+
).rejects.toThrow('cannot resolve');
163+
164+
expect(process.env.AWS_REGION).toBeUndefined();
165+
expect(process.env.AWS_DEFAULT_REGION).toBeUndefined();
166+
});
167+
});
99168
});

src/cli/aws/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ export { detectAwsContext, type AwsContext } from './aws-context';
22
export { detectAccount, getCredentialProvider } from './account';
33
export { getPartition, arnPrefix, dnsSuffix, serviceEndpoint, consoleDomain } from './partition';
44
export { detectRegion, type RegionDetectionResult } from './region';
5-
export { applyTargetRegionToEnv, withTargetRegion } from './target-region';
5+
export { applyTargetRegionToEnv, withTargetRegion, runWithTargetRegion } from './target-region';
66
export {
77
invokeBedrockSync,
88
invokeClaude,

src/cli/aws/target-region.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,32 @@
1010
* Without this override, a user with a non-default region in aws-targets.json
1111
* but no AWS_DEFAULT_REGION set would see resources created in the SDK's default
1212
* region — see https://github.com/aws/agentcore-cli/issues/924.
13+
*
14+
* --------------------------------------------------------------------------
15+
* Policy for new CLI entry points
16+
* --------------------------------------------------------------------------
17+
* Any new CLI command handler (anything in `src/cli/commands/*` or
18+
* `src/cli/operations/*` that may be invoked from a CLI / TUI entry point) that
19+
* could end up constructing AWS SDK clients without an explicit `region` option
20+
* MUST wrap its body so that `AWS_REGION` / `AWS_DEFAULT_REGION` reflect the
21+
* resolved deployment target's region for the duration of the call.
22+
*
23+
* Use one of:
24+
* - `withTargetRegion(region, fn)` when work fits inside a single
25+
* callback (preferred — cannot leak).
26+
* - `runWithTargetRegion(getTarget, fn)` when target resolution and the
27+
* work itself live in the same scope.
28+
* - `applyTargetRegionToEnv(region)` for handlers whose control flow spans
29+
* helpers that can't easily be wrapped
30+
* in a callback. The returned restore
31+
* function MUST be invoked from a
32+
* `finally` block.
33+
*
34+
* Commands that already pass `region: targetConfig.region` explicitly to every
35+
* SDK client they construct don't strictly need this, but adding the env
36+
* override is cheap and prevents regressions when new helpers are added later.
1337
*/
38+
import type { AwsDeploymentTarget } from '../../schema';
1439

1540
type RestoreEnv = () => void;
1641

@@ -53,3 +78,31 @@ export async function withTargetRegion<T>(region: string, fn: () => Promise<T>):
5378
restore();
5479
}
5580
}
81+
82+
/**
83+
* Resolve a deployment target via `getTarget`, apply its region to the
84+
* environment, and run `fn(target)` with that override in effect. Restores the
85+
* prior environment on return — including when target resolution itself throws,
86+
* when no target is found (the override is simply skipped in that case), or
87+
* when `fn` throws.
88+
*
89+
* Use this in command handlers where the target resolution and the AWS-SDK
90+
* work both live in the same lexical scope. For handlers that span many
91+
* helpers across `try`/`catch`/`finally` blocks, prefer the lower-level
92+
* `applyTargetRegionToEnv` + manual `finally` instead.
93+
*/
94+
export async function runWithTargetRegion<T>(
95+
getTarget: () => Promise<AwsDeploymentTarget | undefined>,
96+
fn: (target: AwsDeploymentTarget | undefined) => Promise<T>
97+
): Promise<T> {
98+
const target = await getTarget();
99+
if (!target?.region) {
100+
return fn(target);
101+
}
102+
const restore = applyTargetRegionToEnv(target.region);
103+
try {
104+
return await fn(target);
105+
} finally {
106+
restore();
107+
}
108+
}

src/cli/commands/abtest/command.ts

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
* from the data plane API, including evaluation scores/metrics.
66
*/
77
import { ConfigIO } from '../../../lib';
8+
import { applyTargetRegionToEnv } from '../../aws';
89
import { getABTest, listABTests } from '../../aws/agentcore-ab-tests';
910
import type { GetABTestResult } from '../../aws/agentcore-ab-tests';
1011
import { dnsSuffix } from '../../aws/partition';
@@ -15,16 +16,39 @@ import type { Command } from '@commander-js/extra-typings';
1516
// Helpers
1617
// ============================================================================
1718

19+
/**
20+
* Resolve the region for AB-test API calls, preferring (in order):
21+
* 1. an explicit `--region` flag
22+
* 2. the first target in `aws-targets.json`
23+
* 3. AWS_DEFAULT_REGION / AWS_REGION env vars
24+
* 4. `us-east-1`
25+
*
26+
* As a side effect, also promotes the resolved region onto AWS_REGION /
27+
* AWS_DEFAULT_REGION so any downstream SDK client constructed without an
28+
* explicit `region` option behaves consistently. See
29+
* https://github.com/aws/agentcore-cli/issues/924.
30+
*/
1831
async function getRegion(cliRegion?: string): Promise<string> {
19-
if (cliRegion) return cliRegion;
20-
try {
21-
const configIO = new ConfigIO();
22-
const targets = await configIO.resolveAWSDeploymentTargets();
23-
if (targets.length > 0) return targets[0]!.region;
24-
} catch {
25-
// Fall through to env vars
32+
let region: string;
33+
if (cliRegion) {
34+
region = cliRegion;
35+
} else {
36+
let targetRegion: string | undefined;
37+
try {
38+
const configIO = new ConfigIO();
39+
const targets = await configIO.resolveAWSDeploymentTargets();
40+
if (targets.length > 0) targetRegion = targets[0]!.region;
41+
} catch {
42+
// Fall through to env vars
43+
}
44+
region = targetRegion ?? process.env.AWS_DEFAULT_REGION ?? process.env.AWS_REGION ?? 'us-east-1';
2645
}
27-
return process.env.AWS_DEFAULT_REGION ?? process.env.AWS_REGION ?? 'us-east-1';
46+
// Intentionally not restored — the abtest command runs to completion and
47+
// exits the process; restoring would require threading a teardown through
48+
// every caller. The override is safe because it only sets env vars to the
49+
// same region we'd otherwise pass explicitly to every SDK client below.
50+
applyTargetRegionToEnv(region);
51+
return region;
2852
}
2953

3054
async function resolveABTestId(

0 commit comments

Comments
 (0)