-
Notifications
You must be signed in to change notification settings - Fork 52
fix(pause-online-insights): reject mismatched --name and --arn #1601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,11 +68,34 @@ function parseOnlineEvalConfigArn( | |
| } | ||
|
|
||
| /** | ||
| * Resolve config ID and region from either a project config name or an ARN. | ||
| * Resolve config ID and region from a project config name, an ARN, or both. | ||
| * | ||
| * When both are provided, the named config is looked up and its configId is | ||
| * cross-checked against the ARN. A mismatch means the user passed a name and | ||
| * ARN that point to different configs — we reject rather than silently | ||
| * preferring one (the ARN previously won, so the name was a no-op). | ||
| */ | ||
| async function resolveConfig( | ||
| options: OnlineEvalActionOptions | ||
| ): Promise<{ success: true; configId: string; region: string } | { success: false; error: string }> { | ||
| if (options.arn && options.name) { | ||
| const arnResolution = parseOnlineEvalConfigArn(options.arn, options.region); | ||
| if (!arnResolution.success) return arnResolution; | ||
|
|
||
| const nameResolution = await resolveOnlineEvalConfig(options.name); | ||
| if (!nameResolution.success) return nameResolution; | ||
|
|
||
| if (nameResolution.configId !== arnResolution.configId) { | ||
| return { | ||
| success: false, | ||
| error: | ||
| `--arn and config name "${options.name}" refer to different configs ` + | ||
| `(name resolves to "${nameResolution.configId}", ARN resolves to "${arnResolution.configId}"). ` + | ||
| `Pass only one, or pass matching values.`, | ||
| }; | ||
| } | ||
| return arnResolution; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once the configIds match, this returns Consider also comparing |
||
| } | ||
| if (options.arn) { | ||
| return parseOnlineEvalConfigArn(options.arn, options.region); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When both
--arnand a config name are passed, this branch now callsresolveOnlineEvalConfig(options.name)→loadDeployedProjectConfig()unconditionally. But the command layer insrc/cli/commands/pause/command.tsxdeliberately skipsrequireProject()whenever--arnis present (lines 38–40 / 129–131):So a user running this outside a project directory with
agentcore pause online-insights some-name --arn arn:...will now hit aConfigNotFoundErrorfromreadProjectSpec()instead of either succeeding (pre-PR behavior) or getting a clear validation message. The throw falls through to the outertry/catchincommand.tsxand surfaces as a genericError: agentcore.json not found ..., which is confusing UX.A few ways to fix:
command.tsx, always callrequireProject()when anameargument is provided, even alongside--arn. That makes the contract explicit: "if you pass a name, you must be in a project."resolveConfighere, catch theConfigNotFoundError(or check for project presence first) and return a clearer error like"Cannot cross-check config name "X" against --arn: not inside a project. Pass only --arn, or run from a project directory."Option (1) seems cleanest and matches user intent.