Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 70 additions & 1 deletion src/cli/aws/__tests__/target-region.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { applyTargetRegionToEnv, withTargetRegion } from '../target-region.js';
import type { AwsDeploymentTarget } from '../../../schema';
import { applyTargetRegionToEnv, runWithTargetRegion, withTargetRegion } from '../target-region.js';
import { afterEach, beforeEach, describe, expect, it } from 'vitest';

describe('target-region', () => {
Expand Down Expand Up @@ -96,4 +97,72 @@ describe('target-region', () => {
expect(result).toBe(42);
});
});

describe('runWithTargetRegion', () => {
const buildTarget = (region: string): AwsDeploymentTarget => ({
name: 'default',
account: '123456789012',
region: region as AwsDeploymentTarget['region'],
});

it('applies the resolved target region inside the callback and restores afterwards', async () => {
let seenRegion: string | undefined;
const target = buildTarget('ap-southeast-2');

const result = await runWithTargetRegion(
() => Promise.resolve(target),
resolved => {
seenRegion = process.env.AWS_REGION;
expect(resolved).toBe(target);
return Promise.resolve('ok');
}
);

expect(seenRegion).toBe('ap-southeast-2');
expect(result).toBe('ok');
expect(process.env.AWS_REGION).toBeUndefined();
expect(process.env.AWS_DEFAULT_REGION).toBeUndefined();
});

it('skips the env override when no target is resolved', async () => {
let seenRegion: string | undefined;

await runWithTargetRegion(
() => Promise.resolve(undefined),
resolved => {
seenRegion = process.env.AWS_REGION;
expect(resolved).toBeUndefined();
return Promise.resolve();
}
);

expect(seenRegion).toBeUndefined();
expect(process.env.AWS_REGION).toBeUndefined();
});

it('restores env vars even when the callback throws', async () => {
process.env.AWS_REGION = 'us-east-1';

await expect(
runWithTargetRegion(
() => Promise.resolve(buildTarget('sa-east-1')),
() => Promise.reject(new Error('boom'))
)
).rejects.toThrow('boom');

expect(process.env.AWS_REGION).toBe('us-east-1');
});

it('propagates errors from the target resolver without mutating env', async () => {
await expect(
runWithTargetRegion(
() => Promise.reject(new Error('cannot resolve')),
() => Promise.resolve('unreached')
)
).rejects.toThrow('cannot resolve');

expect(process.env.AWS_REGION).toBeUndefined();
expect(process.env.AWS_DEFAULT_REGION).toBeUndefined();
});
});
});
2 changes: 1 addition & 1 deletion src/cli/aws/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ export { detectAwsContext, type AwsContext } from './aws-context';
export { detectAccount, getCredentialProvider } from './account';
export { getPartition, arnPrefix, dnsSuffix, serviceEndpoint, consoleDomain } from './partition';
export { detectRegion, type RegionDetectionResult } from './region';
export { applyTargetRegionToEnv, withTargetRegion } from './target-region';
export { applyTargetRegionToEnv, withTargetRegion, runWithTargetRegion } from './target-region';
export {
invokeBedrockSync,
invokeClaude,
Expand Down
53 changes: 53 additions & 0 deletions src/cli/aws/target-region.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,32 @@
* Without this override, a user with a non-default region in aws-targets.json
* but no AWS_DEFAULT_REGION set would see resources created in the SDK's default
* region — see https://github.com/aws/agentcore-cli/issues/924.
*
* --------------------------------------------------------------------------
* Policy for new CLI entry points
* --------------------------------------------------------------------------
* Any new CLI command handler (anything in `src/cli/commands/*` or
* `src/cli/operations/*` that may be invoked from a CLI / TUI entry point) that
* could end up constructing AWS SDK clients without an explicit `region` option
* MUST wrap its body so that `AWS_REGION` / `AWS_DEFAULT_REGION` reflect the
* resolved deployment target's region for the duration of the call.
*
* Use one of:
* - `withTargetRegion(region, fn)` when work fits inside a single
* callback (preferred — cannot leak).
* - `runWithTargetRegion(getTarget, fn)` when target resolution and the
* work itself live in the same scope.
* - `applyTargetRegionToEnv(region)` for handlers whose control flow spans
* helpers that can't easily be wrapped
* in a callback. The returned restore
* function MUST be invoked from a
* `finally` block.
*
* Commands that already pass `region: targetConfig.region` explicitly to every
* SDK client they construct don't strictly need this, but adding the env
* override is cheap and prevents regressions when new helpers are added later.
*/
import type { AwsDeploymentTarget } from '../../schema';

type RestoreEnv = () => void;

Expand Down Expand Up @@ -53,3 +78,31 @@ export async function withTargetRegion<T>(region: string, fn: () => Promise<T>):
restore();
}
}

/**
* Resolve a deployment target via `getTarget`, apply its region to the
* environment, and run `fn(target)` with that override in effect. Restores the
* prior environment on return — including when target resolution itself throws,
* when no target is found (the override is simply skipped in that case), or
* when `fn` throws.
*
* Use this in command handlers where the target resolution and the AWS-SDK
* work both live in the same lexical scope. For handlers that span many
* helpers across `try`/`catch`/`finally` blocks, prefer the lower-level
* `applyTargetRegionToEnv` + manual `finally` instead.
*/
export async function runWithTargetRegion<T>(
getTarget: () => Promise<AwsDeploymentTarget | undefined>,
fn: (target: AwsDeploymentTarget | undefined) => Promise<T>
): Promise<T> {
const target = await getTarget();
if (!target?.region) {
return fn(target);
}
const restore = applyTargetRegionToEnv(target.region);
try {
return await fn(target);
} finally {
restore();
}
}
40 changes: 32 additions & 8 deletions src/cli/commands/abtest/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* from the data plane API, including evaluation scores/metrics.
*/
import { ConfigIO } from '../../../lib';
import { applyTargetRegionToEnv } from '../../aws';
import { getABTest, listABTests } from '../../aws/agentcore-ab-tests';
import type { GetABTestResult } from '../../aws/agentcore-ab-tests';
import { dnsSuffix } from '../../aws/partition';
Expand All @@ -15,16 +16,39 @@ import type { Command } from '@commander-js/extra-typings';
// Helpers
// ============================================================================

/**
* Resolve the region for AB-test API calls, preferring (in order):
* 1. an explicit `--region` flag
* 2. the first target in `aws-targets.json`
* 3. AWS_DEFAULT_REGION / AWS_REGION env vars
* 4. `us-east-1`
*
* As a side effect, also promotes the resolved region onto AWS_REGION /
* AWS_DEFAULT_REGION so any downstream SDK client constructed without an
* explicit `region` option behaves consistently. See
* https://github.com/aws/agentcore-cli/issues/924.
*/
async function getRegion(cliRegion?: string): Promise<string> {
if (cliRegion) return cliRegion;
try {
const configIO = new ConfigIO();
const targets = await configIO.resolveAWSDeploymentTargets();
if (targets.length > 0) return targets[0]!.region;
} catch {
// Fall through to env vars
let region: string;
if (cliRegion) {
region = cliRegion;
} else {
let targetRegion: string | undefined;
try {
const configIO = new ConfigIO();
const targets = await configIO.resolveAWSDeploymentTargets();
if (targets.length > 0) targetRegion = targets[0]!.region;
} catch {
// Fall through to env vars
}
region = targetRegion ?? process.env.AWS_DEFAULT_REGION ?? process.env.AWS_REGION ?? 'us-east-1';
}
return process.env.AWS_DEFAULT_REGION ?? process.env.AWS_REGION ?? 'us-east-1';
// Intentionally not restored — the abtest command runs to completion and
// exits the process; restoring would require threading a teardown through
// every caller. The override is safe because it only sets env vars to the
// same region we'd otherwise pass explicitly to every SDK client below.
applyTargetRegionToEnv(region);
return region;
}

async function resolveABTestId(
Expand Down
Loading
Loading