From 87dd8cca3aa29525adf934c94140e056a5531002 Mon Sep 17 00:00:00 2001 From: Adnan Hajar Date: Wed, 11 Mar 2026 17:51:16 -0400 Subject: [PATCH] =?UTF-8?q?fix(hyperion):=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20tenant=20ID=20derivation,=20atomic=20pairing,=20too?= =?UTF-8?q?l=20key=20mapping?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Derive tenant ID from session key via extractTenantId() instead of agent name, preventing cross-tenant collisions when agents share names (P1) - Consume pairing codes atomically with DynamoDB conditional delete to prevent double-redemption race conditions (P1) - Map tool API keys to provider-specific paths (brave_search → search.apiKey, others → search..apiKey) instead of overwriting a single key (P1) - Pass gateway base config (ctx.config) into Hyperion runtime so tenant configs inherit global settings like ACP backend selection (P1) - Add endpointOverride to AgentCore config that applies after SSM loading, so runtime ARNs are still discovered when testing with a local endpoint (P2) - Fix env var restoration in credentials test to delete undefined keys instead of setting them to string "undefined" - Update tests for all changes; all 99 tests pass Co-Authored-By: Claude Opus 4.6 --- extensions/agentcore/index.ts | 2 +- extensions/agentcore/src/config.test.ts | 37 +++++++++++++++++++++++++ extensions/agentcore/src/config.ts | 3 ++ extensions/agentcore/src/runtime.ts | 6 ++-- extensions/hyperion/src/service.ts | 1 + extensions/nova/src/credentials.test.ts | 13 +++++---- src/hyperion/dynamodb-client.ts | 33 ++++++++++++++++++++++ src/hyperion/pairing-store.test.ts | 32 ++++++--------------- src/hyperion/pairing-store.ts | 8 ++---- src/hyperion/tenant-config-loader.ts | 20 ++++++++----- 10 files changed, 111 insertions(+), 44 deletions(-) diff --git a/extensions/agentcore/index.ts b/extensions/agentcore/index.ts index 5441a522b65ae..d59e35d5920f5 100644 --- a/extensions/agentcore/index.ts +++ b/extensions/agentcore/index.ts @@ -35,7 +35,7 @@ const plugin = { configSource: { ssmPrefix, region, - localOverride: pluginConfig.endpoint ? { endpoint: pluginConfig.endpoint } : undefined, + endpointOverride: pluginConfig.endpoint, }, }), ); diff --git a/extensions/agentcore/src/config.test.ts b/extensions/agentcore/src/config.test.ts index cb0a467b405cc..ac54a8b02a370 100644 --- a/extensions/agentcore/src/config.test.ts +++ b/extensions/agentcore/src/config.test.ts @@ -180,6 +180,43 @@ describe("loadAgentCoreConfig", () => { }); }); + // ── endpointOverride (loads from SSM, applies endpoint after) ─────── + + describe("endpointOverride", () => { + it("applies endpoint override without skipping SSM", async () => { + mockSsmSend.mockImplementation((cmd: any) => { + const name = cmd.input?.Name; + if (name?.endsWith("/runtime-arns")) { + return { + Parameter: { + Value: '["arn:aws:agentcore:us-west-2:123:runtime/real"]', + }, + }; + } + return { Parameter: { Value: null } }; + }); + + const config = await loadAgentCoreConfig({ + ssmPrefix: "/hyperion/beta/agentcore", + endpointOverride: "https://localhost:9999", + }); + + expect(config.runtimeArns).toEqual(["arn:aws:agentcore:us-west-2:123:runtime/real"]); + expect(config.endpoint).toBe("https://localhost:9999"); + expect(mockSsmSend).toHaveBeenCalled(); + }); + + it("does not set endpoint when endpointOverride is undefined", async () => { + mockSsmSend.mockResolvedValue({ Parameter: { Value: null } }); + + const config = await loadAgentCoreConfig({ + ssmPrefix: "/hyperion/beta/agentcore", + }); + + expect(config.endpoint).toBeUndefined(); + }); + }); + // ── SSM error handling ────────────────────────────────────────────── describe("SSM error handling", () => { diff --git a/extensions/agentcore/src/config.ts b/extensions/agentcore/src/config.ts index 159e150bc77da..9dfa4788425ec 100644 --- a/extensions/agentcore/src/config.ts +++ b/extensions/agentcore/src/config.ts @@ -12,6 +12,8 @@ export type AgentCoreConfigSource = { region?: string; /** Override for local development (skip SSM, use provided config). */ localOverride?: Partial; + /** AgentCore endpoint override — applied after SSM loading (does NOT skip SSM). */ + endpointOverride?: string; }; /** @@ -73,6 +75,7 @@ export async function loadAgentCoreConfig( runtimeArns, memoryNamespacePrefix, defaultModel, + ...(source.endpointOverride ? { endpoint: source.endpointOverride } : {}), }; } diff --git a/extensions/agentcore/src/runtime.ts b/extensions/agentcore/src/runtime.ts index 8ecfeea69d10f..71836190bdb47 100644 --- a/extensions/agentcore/src/runtime.ts +++ b/extensions/agentcore/src/runtime.ts @@ -111,8 +111,10 @@ export class AgentCoreRuntime implements AcpRuntime { const runtimeArn = pickRuntimeArn(this.config.runtimeArns); - // For Hyperion, the OC agentId IS the tenant user_id. - const tenantId = agent; + // [claude-infra] Derive tenant user_id from the Hyperion session key + // (format: "tenant_{userId}:{agentId}:{rest}"), not from `agent` which + // may be a shared logical name like "main" across different tenants. + const tenantId = extractTenantId(sessionKey) ?? agent; // [claude-infra] Multi-instance: extract agent instance ID from session key. const agentId = extractAgentId(sessionKey); diff --git a/extensions/hyperion/src/service.ts b/extensions/hyperion/src/service.ts index ade87eab3d4d2..05e95f92f8d09 100644 --- a/extensions/hyperion/src/service.ts +++ b/extensions/hyperion/src/service.ts @@ -64,6 +64,7 @@ export function createHyperionPluginService( dynamoConfig, docClient, kmsClient, + defaultConfig: ctx.config, }); setHyperionRuntime(runtime); diff --git a/extensions/nova/src/credentials.test.ts b/extensions/nova/src/credentials.test.ts index c2252a30959e3..070fb19987034 100644 --- a/extensions/nova/src/credentials.test.ts +++ b/extensions/nova/src/credentials.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import type { NovaConfig } from "./types.js"; import { resolveNovaCredentials } from "./credentials.js"; +import type { NovaConfig } from "./types.js"; describe("resolveNovaCredentials", () => { const savedEnv: Record = {}; @@ -17,10 +17,13 @@ describe("resolveNovaCredentials", () => { }); afterEach(() => { - process.env.NOVA_BASE_URL = savedEnv.NOVA_BASE_URL; - process.env.NOVA_API_KEY = savedEnv.NOVA_API_KEY; - process.env.NOVA_USER_ID = savedEnv.NOVA_USER_ID; - process.env.NOVA_DEVICE_ID = savedEnv.NOVA_DEVICE_ID; + for (const [key, val] of Object.entries(savedEnv)) { + if (val === undefined) { + delete process.env[key]; + } else { + process.env[key] = val; + } + } }); it("resolves credentials from config", () => { diff --git a/src/hyperion/dynamodb-client.ts b/src/hyperion/dynamodb-client.ts index 1786063f1f035..8014c25b9a5c4 100644 --- a/src/hyperion/dynamodb-client.ts +++ b/src/hyperion/dynamodb-client.ts @@ -175,6 +175,39 @@ export class HyperionDynamoDBClient { ); } + /** + * Atomically consume a pairing code: deletes it only if it still exists. + * Returns the deleted item, or null if it was already consumed. + * Uses ConditionExpression to prevent double-redemption races. + */ + async consumePairingCode(code: string): Promise { + const { DeleteCommand } = await import("@aws-sdk/lib-dynamodb"); + try { + const result = await this.docClient.send( + new DeleteCommand({ + TableName: this.config.pairingCodesTableName, + Key: { code }, + ConditionExpression: "attribute_exists(code)", + ReturnValues: "ALL_OLD", + }), + ); + const item = (result as { Attributes?: PairingCode }).Attributes; + if (!item) { + return null; + } + // DynamoDB TTL is eventually consistent — check expiry explicitly. + if (item.expires_at <= Math.floor(Date.now() / 1000)) { + return null; + } + return item; + } catch (err) { + if ((err as { name?: string }).name === "ConditionalCheckFailedException") { + return null; + } + throw err; + } + } + // -- User Credentials -- [claude-infra] composite key: user_id + agent_id async getUserCredentials( diff --git a/src/hyperion/pairing-store.test.ts b/src/hyperion/pairing-store.test.ts index a6c53fbbe2ca9..311407201c330 100644 --- a/src/hyperion/pairing-store.test.ts +++ b/src/hyperion/pairing-store.test.ts @@ -7,12 +7,14 @@ function createMockDbClient() { return { putPairingCode: vi.fn(), getPairingCode: vi.fn(), + consumePairingCode: vi.fn(), deletePairingCode: vi.fn(), putChannelLink: vi.fn(), deleteChannelLink: vi.fn(), } as unknown as HyperionDynamoDBClient & { putPairingCode: ReturnType; getPairingCode: ReturnType; + consumePairingCode: ReturnType; deletePairingCode: ReturnType; putChannelLink: ReturnType; deleteChannelLink: ReturnType; @@ -105,9 +107,8 @@ describe("HyperionPairingStore", () => { }; it("creates ChannelLink with correct data, inherits agent_id from pairing code", async () => { - dbClient.getPairingCode.mockResolvedValueOnce(basePairingCode); + dbClient.consumePairingCode.mockResolvedValueOnce(basePairingCode); dbClient.putChannelLink.mockResolvedValueOnce(undefined); - dbClient.deletePairingCode.mockResolvedValueOnce(undefined); const link = await store.redeemPairingCode({ code: "ABCD1234", @@ -129,9 +130,8 @@ describe("HyperionPairingStore", () => { }); it("normalizes code to uppercase", async () => { - dbClient.getPairingCode.mockResolvedValueOnce(basePairingCode); + dbClient.consumePairingCode.mockResolvedValueOnce(basePairingCode); dbClient.putChannelLink.mockResolvedValueOnce(undefined); - dbClient.deletePairingCode.mockResolvedValueOnce(undefined); await store.redeemPairingCode({ code: " abcd1234 ", @@ -139,7 +139,7 @@ describe("HyperionPairingStore", () => { platformUserId: "tg-user-99", }); - expect(dbClient.getPairingCode).toHaveBeenCalledWith("ABCD1234"); + expect(dbClient.consumePairingCode).toHaveBeenCalledWith("ABCD1234"); }); it("returns null for empty code", async () => { @@ -153,8 +153,8 @@ describe("HyperionPairingStore", () => { expect(dbClient.getPairingCode).not.toHaveBeenCalled(); }); - it("returns null if pairing code not found", async () => { - dbClient.getPairingCode.mockResolvedValueOnce(null); + it("returns null if pairing code not found (already consumed)", async () => { + dbClient.consumePairingCode.mockResolvedValueOnce(null); const link = await store.redeemPairingCode({ code: "NONEXIST", @@ -167,7 +167,7 @@ describe("HyperionPairingStore", () => { }); it("returns null if platform doesn't match", async () => { - dbClient.getPairingCode.mockResolvedValueOnce(basePairingCode); + dbClient.consumePairingCode.mockResolvedValueOnce(basePairingCode); const link = await store.redeemPairingCode({ code: "ABCD1234", @@ -178,22 +178,6 @@ describe("HyperionPairingStore", () => { expect(link).toBeNull(); expect(dbClient.putChannelLink).not.toHaveBeenCalled(); }); - - it("deletes consumed code (best effort)", async () => { - dbClient.getPairingCode.mockResolvedValueOnce(basePairingCode); - dbClient.putChannelLink.mockResolvedValueOnce(undefined); - dbClient.deletePairingCode.mockRejectedValueOnce(new Error("Delete failed")); - - const link = await store.redeemPairingCode({ - code: "ABCD1234", - platform: "telegram", - platformUserId: "tg-user-99", - }); - - // Link should still be returned even though delete failed - expect(link).not.toBeNull(); - expect(dbClient.deletePairingCode).toHaveBeenCalledWith("ABCD1234"); - }); }); describe("validatePairingCode", () => { diff --git a/src/hyperion/pairing-store.ts b/src/hyperion/pairing-store.ts index f8bc4b6ba8a65..957ca2c707076 100644 --- a/src/hyperion/pairing-store.ts +++ b/src/hyperion/pairing-store.ts @@ -94,8 +94,9 @@ export class HyperionPairingStore { return null; } - // Fetch and validate the pairing code. - const pairingCode = await this.dbClient.getPairingCode(normalizedCode); + // Atomically consume the pairing code — conditional delete ensures only one + // concurrent redeem succeeds, preventing double-bind race conditions. + const pairingCode = await this.dbClient.consumePairingCode(normalizedCode); if (!pairingCode) { return null; } @@ -118,9 +119,6 @@ export class HyperionPairingStore { await this.dbClient.putChannelLink(channelLink); - // Delete the consumed code (best-effort — TTL will clean up regardless). - await this.dbClient.deletePairingCode(normalizedCode).catch(() => {}); - return channelLink; } diff --git a/src/hyperion/tenant-config-loader.ts b/src/hyperion/tenant-config-loader.ts index 9ed5648e97cde..8875a6af9282d 100644 --- a/src/hyperion/tenant-config-loader.ts +++ b/src/hyperion/tenant-config-loader.ts @@ -215,23 +215,29 @@ export class TenantConfigLoader { } // Inject per-user tool API keys from encrypted credential store. + // Each search provider resolves its key from a provider-specific path: + // brave_search → tools.web.search.apiKey (default/brave) + // gemini/grok/kimi/perplexity → tools.web.search..apiKey if (credentials?.tool_keys) { - const web = config.tools?.web ?? {}; - const search = web.search ?? {}; + const web = { ...config.tools?.web }; + const search = { ...web.search } as Record; for (const [toolName, apiKey] of Object.entries(credentials.tool_keys)) { - if ( - toolName === "brave_search" || + if (toolName === "brave_search") { + search.apiKey = apiKey; + } else if ( toolName === "gemini" || toolName === "grok" || toolName === "kimi" || toolName === "perplexity" ) { - config.tools = { - ...config.tools, - web: { ...web, search: { ...search, apiKey } }, + search[toolName] = { + ...(search[toolName] as Record | undefined), + apiKey, }; } } + web.search = search; + config.tools = { ...config.tools, web }; } // Apply tenant-level skill permissions.