Skip to content
Merged
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
16 changes: 14 additions & 2 deletions src/assets/templates/export-harness-python/mcp_client/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,18 @@
logger = logging.getLogger(__name__)

{{#if remoteMcpTools}}
{{#if (some remoteMcpTools "toolPatterns")}}
import fnmatch


def _allowed_tools(server, *patterns):
"""Load only the tools allowedTools selects, by the server's name or as <server>_<tool>."""
def allowed(tool, **_):
names = (tool.mcp_tool.name, f"{server}_{tool.mcp_tool.name}")
return any(fnmatch.fnmatchcase(name, pattern) for name in names for pattern in patterns)
return {"allowed": [allowed]}

{{/if}}
{{#if (some remoteMcpTools "headerCredentials")}}
from bedrock_agentcore.identity.auth import requires_api_key
{{/if}}
Expand All @@ -30,9 +42,9 @@ def transport():
headers = { {{#each headerCredentials}}{{safeJson headerKey}}: _get_{{pythonName}}_key(){{#unless @last}}, {{/unless}}{{/each}} }
return streamablehttp_client(url, headers=headers)

return MCPClient(transport)
return MCPClient(transport{{#if toolPatterns}}, tool_filters=_allowed_tools({{safeJson name}}, {{#each toolPatterns}}{{safeJson this}}{{#unless @last}}, {{/unless}}{{/each}}){{/if}})
{{else}}
return MCPClient(lambda: streamablehttp_client(url))
return MCPClient(lambda: streamablehttp_client(url){{#if toolPatterns}}, tool_filters=_allowed_tools({{safeJson name}}, {{#each toolPatterns}}{{safeJson this}}{{#unless @last}}, {{/unless}}{{/each}}){{/if}})
{{/if}}

{{/each}}
Expand Down
25 changes: 20 additions & 5 deletions src/assets/templates/export-harness-python/model/load.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{{#if (eq modelProvider "Bedrock")}}
{{#if bedrockMantle}}
{{#if modelAdditionalParams}}
import json
{{/if}}
import os

from aws_bedrock_token_generator import provide_token
Expand Down Expand Up @@ -34,7 +37,7 @@ def load_model():
{{/if}}
client_args = {"api_key": token, "base_url": base_url}

params = {}
params = {{#if modelAdditionalParams}}json.loads({{pyJsonStr modelAdditionalParams}}){{else}}{}{{/if}}
{{#if modelMaxTokens}}
{{#if (eq mantleApiFormat "chat_completions")}}
params["max_completion_tokens"] = {{modelMaxTokens}}
Expand All @@ -60,6 +63,9 @@ def load_model():
{{/if}}
{{/if}}
{{else}}
{{#if modelAdditionalParams}}
import json
{{/if}}
from strands.models.bedrock import BedrockModel


Expand All @@ -75,11 +81,17 @@ def load_model() -> BedrockModel:
{{/if}}
{{#if modelTopP}}
top_p={{modelTopP}},
{{/if}}
{{#if modelAdditionalParams}}
additional_args=json.loads({{pyJsonStr modelAdditionalParams}}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we ensure the explicit model settings take priority? Strands applies additional_args after max_tokens, temperature, and top_p; an inferenceConfig inside additionalParams therefore overrides those explicit settings.

for eg, additionalParams.inferenceConfig.maxTokens currently overrides max_tokens, even though the code intends the explicit value to win.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that additional_args wins on Bedrock Converse. The harness behaves the same way: it builds BedrockModel with the explicit settings plus additional_args, and Strands applies those last. So the export matches the harness here, and changing the precedence only in the export would make the two diverge. The comment claiming explicit settings win was wrong; it now says what the harness docs say: provider-specific parameters are passed through to the model provider unchanged. On OpenAI, Gemini, LiteLLM, and Mantle the explicit settings still take precedence, in both the harness and the export.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I see, makes sense to me!

{{/if}}
)
{{/if}}
{{/if}}
{{#if (eq modelProvider "OpenAI")}}
{{#if modelAdditionalParams}}
import json
{{/if}}
import os

{{#if (eq modelApiFormat "responses")}}
Expand Down Expand Up @@ -116,7 +128,7 @@ def _get_api_key() -> str:

def load_model():
"""Get authenticated OpenAI model client."""
params = {}
params = {{#if modelAdditionalParams}}json.loads({{pyJsonStr modelAdditionalParams}}){{else}}{}{{/if}}
{{#if modelMaxTokens}}
params["{{#if (eq modelApiFormat "responses")}}max_output_tokens{{else}}max_completion_tokens{{/if}}"] = {{modelMaxTokens}}
{{/if}}
Expand All @@ -133,6 +145,9 @@ def load_model():
)
{{/if}}
{{#if (eq modelProvider "Gemini")}}
{{#if modelAdditionalParams}}
import json
{{/if}}
import os

from strands.models.gemini import GeminiModel
Expand Down Expand Up @@ -165,7 +180,7 @@ def _get_api_key() -> str:

def load_model() -> GeminiModel:
"""Get authenticated Gemini model client."""
params = {}
params = {{#if modelAdditionalParams}}json.loads({{pyJsonStr modelAdditionalParams}}){{else}}{}{{/if}}
{{#if modelMaxTokens}}
params["max_output_tokens"] = {{modelMaxTokens}}
{{/if}}
Expand All @@ -186,7 +201,7 @@ def load_model() -> GeminiModel:
{{/if}}
{{#if (eq modelProvider "LiteLLM")}}
import os
{{#if litellmAdditionalParams}}
{{#if modelAdditionalParams}}
import json
{{/if}}

Expand Down Expand Up @@ -231,7 +246,7 @@ def load_model() -> LiteLLMModel:
{{#if litellmApiBase}}
client_args["api_base"] = {{safeJson litellmApiBase}}
{{/if}}
params = {{#if litellmAdditionalParams}}json.loads({{pyJsonStr litellmAdditionalParams}}){{else}}{}{{/if}}
params = {{#if modelAdditionalParams}}json.loads({{pyJsonStr modelAdditionalParams}}){{else}}{}{{/if}}
{{#if modelMaxTokens}}
params["max_tokens"] = {{modelMaxTokens}}
{{/if}}
Expand Down
39 changes: 39 additions & 0 deletions src/core/project/manager.export.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,45 @@ function exportInput(overrides: Partial<ExportHarnessInput> = {}): ExportHarness
}

describe("FsProjectManager.exportHarness rendered tree", () => {
test("loads only the MCP tools allowedTools selects", async () => {
const { manager: subject } = manager();
const project = await projectWithHarness(subject, {
allowedTools: ["@exa/web_*"],
tools: [
{
type: "remote_mcp",
name: "exa",
config: { remoteMcp: { url: "https://mcp.exa.ai/mcp" } },
},
],
});

const result = await drain(subject.exportHarness(project, exportInput()));

const client = await Bun.file(join(result.agentPath, "mcp_client", "client.py")).text();
expect(client).toContain('tool_filters=_allowed_tools("exa", "web_*")');
});

test("merges service model parameters under the explicit settings", async () => {
const { manager: subject } = manager();
const project = await projectWithHarness(subject);
const spec = HarnessSpecSchema.parse({
name: "remote",
model: { provider: "bedrock", modelId: "us.amazon.nova-lite-v1:0", temperature: 0.2 },
});

const result = await drain(
subject.exportHarness(project, {
prefetched: { spec, modelAdditionalParams: { top_k: 5 } },
targetAgentName: "remoteAgent",
}),
);

const loadModel = await Bun.file(join(result.agentPath, "model", "load.py")).text();
expect(loadModel).toContain("additional_args=json.loads(");
expect(loadModel).toContain("top_k");
});

test("renders invocation-scoped native Strands limits without a custom hook", async () => {
const { manager: subject } = manager();
const project = await projectWithHarness(subject, {
Expand Down
1 change: 1 addition & 0 deletions src/core/project/manager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@ export class FsProjectManager implements ProjectManager {
systemPrompt,
projectSpec,
sourceNotes: input.prefetched?.notes,
modelAdditionalParams: input.prefetched?.modelAdditionalParams,
});

yield { type: "step", message: `Rendering agent code at 'app/${targetAgentName}'` };
Expand Down
120 changes: 108 additions & 12 deletions src/core/project/templates/export.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,42 @@ describe("mapHarnessToExportPlan model mapping", () => {
expect(result.context.modelProvider).toBe("LiteLLM");
expect(result.context.strandsExtras).toBe("litellm");
expect(result.context.litellmApiBase).toBe("https://litellm.example");
expect(result.context.litellmAdditionalParams).toEqual({ max_retries: 2 });
expect(result.context.modelAdditionalParams).toEqual({ max_retries: 2 });
expect(result.context.modelMaxTokens).toBe("300");
expect(result.context.modelTemperature).toBe("0.1");
expect(result.context.modelTopP).toBe("0.7");
expect(result.notes).toEqual([]);
});

test("grants bedrock:ApplyGuardrail when the model parameters set a guardrail", () => {
const guarded = (guardrailIdentifier: string) =>
plan({
modelAdditionalParams: { guardrailConfig: { guardrailIdentifier, guardrailVersion: "1" } },
}).policyFiles["bedrock-guardrail-policy.json"];
expect(guarded("gr-123")).toEqual({
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
Action: "bedrock:ApplyGuardrail",
Resource: "arn:aws:bedrock:*:*:guardrail/gr-123",
},
],
});
const arn = "arn:aws:bedrock:us-east-1:111122223333:guardrail/gr-123";
expect(guarded(arn)).toMatchObject({ Statement: [{ Resource: arn }] });
expect(plan({}).policyFiles["bedrock-guardrail-policy.json"]).toBeUndefined();
});

test("threads service-only model parameters into the render context", () => {
const result = plan({
modelAdditionalParams: { performanceConfig: { latency: "optimized" } },
});
expect(result.context.modelAdditionalParams).toEqual({
performanceConfig: { latency: "optimized" },
});
});

test("warns when a keyless LiteLLM model is not Bedrock-backed", () => {
const result = plan({
spec: harness({ model: { provider: "lite_llm", modelId: "openai/gpt-4.1" } }),
Expand Down Expand Up @@ -353,7 +382,7 @@ describe("mapHarnessToExportPlan tools", () => {

const restricted = plan({
spec: harness({
allowedTools: ["@builtin/shell", "exa"],
allowedTools: ["@builtin/shell", "@exa"],
tools: [
{
type: "remote_mcp",
Expand Down Expand Up @@ -382,18 +411,85 @@ describe("mapHarnessToExportPlan tools", () => {
});
});

describe("mapHarnessToExportPlan allowedTools selection", () => {
test("a bare name or glob selects builtins only", () => {
const shellOnly = plan({ spec: harness({ allowedTools: ["shell"] }) });
expect(shellOnly.context.hasShell).toBe(true);
expect(shellOnly.context.hasFileOperations).toBe(false);

const fileGlob = plan({ spec: harness({ allowedTools: ["file_*"] }) });
expect(fileGlob.context.hasShell).toBe(false);
expect(fileGlob.context.hasFileOperations).toBe(true);
});

test("keeps an MCP server that @server or * allows and drops it otherwise", () => {
const exa = {
type: "remote_mcp",
name: "exa",
config: { remoteMcp: { url: "https://mcp.exa.ai/mcp" } },
};
const servers = (allowedTools: string[]) =>
(
plan({ spec: harness({ tools: [exa], allowedTools }) }).context.remoteMcpTools as
{ name: string }[] | undefined
)?.map((tool) => tool.name);
expect(servers(["*"])).toEqual(["exa"]);
expect(servers(["@exa"])).toEqual(["exa"]);
expect(servers(["@e*/search"])).toEqual(["exa"]);
// A bare pattern selects builtins only.
expect(servers(["exa"])).toBeUndefined();
expect(servers(["@other"])).toBeUndefined();
});

test("narrows an MCP server to the tools @server/tool selects", () => {
const exa = {
type: "remote_mcp",
name: "exa",
config: { remoteMcp: { url: "https://mcp.exa.ai/mcp" } },
};
const patterns = (allowedTools: string[]) =>
(
plan({ spec: harness({ tools: [exa], allowedTools }) }).context.remoteMcpTools as
{ toolPatterns?: string[] }[] | undefined
)?.map((tool) => tool.toolPatterns);
expect(patterns(["@exa/search", "@e*/web_*"])).toEqual([["search", "web_*"]]);
expect(patterns(["@exa", "@exa/search"])).toEqual([undefined]);
expect(patterns(["*"])).toEqual([undefined]);
});

test("selects an inline function by @name, not by its bare name", () => {
const inline = {
type: "inline_function",
name: "lookup",
config: { inlineFunction: { description: "d", inputSchema: { type: "object" } } },
};
const names = (allowedTools: string[]) =>
(
plan({ spec: harness({ tools: [inline], allowedTools }) }).context.inlineFunctionTools as
{ name: string }[] | undefined
)?.map((tool) => tool.name);
expect(names(["@lookup"])).toEqual(["lookup"]);
expect(names(["lookup"])).toBeUndefined();
});
});

describe("matchesAllowedTools", () => {
test.each([
["*", "anything", true],
["exa", "exa", true],
["e*", "exa", true],
["@builtin/shell", "builtin/shell", true],
["@builtin", "builtin/shell", true],
["@server/tool", "server_tool", true],
["exa", "other", false],
["@builtin/shell", "builtin/file_operations", false],
])("pattern %s vs %s -> %p", (pattern, name, expected) => {
expect(matchesAllowedTools(name, [pattern])).toBe(expected);
["*", "exa", "search", true],
// A bare pattern is a glob over builtin names only.
["shell", "builtin", "shell", true],
["file_*", "builtin", "file_operations", true],
["exa", "exa", "exa", false],
["@builtin", "builtin", "shell", true],
["@builtin/shell", "builtin", "file_operations", false],
// "@server" allows every tool of a server; "@server/tool" globs its tools.
["@exa", "exa", "search", true],
["@e*", "exa", "search", true],
["@exa/web_*", "exa", "web_search", true],
["@exa/web_*", "exa", "crawl", false],
["@exa", "builtin", "shell", false],
])("pattern %s vs %s/%s -> %p", (pattern, server, tool, expected) => {
expect(matchesAllowedTools(server, tool, [pattern])).toBe(expected);
});
});

Expand Down
Loading
Loading