fix(bedrock): reject tool with empty description before calling Converse

[simon0191]

Summary

• Bedrock's Converse API rejects any tool whose description is the empty string with an opaque ValidationException (Member must have length greater than or equal to 1), failing the entire call. The mapper used to pass empty descriptions through unchanged, so callers got a server-side error that pointed at tool indices (toolConfig.tools.12.member.toolSpec.description) rather than tool names — hard to map back to source for an agent that mixes MCP-derived and subagent-as-tool entries.
• mapToolConfig now validates upfront and returns one error listing every offender by name, e.g. bedrock: every tool must have a non-empty description; missing on: subagent_a, subagent_b. Errors bubble up wrapped in llm.ErrRequestMapping.
• Scoped to the Bedrock provider deliberately. OpenAI and Anthropic accept empty tool descriptions, so a check at agent.New() would crashloop agents that work fine on those providers.

Context

Hit in production by an AI agent using subagents whose Subagent.description was empty (proto schema marks it OPTIONAL with no min_len). On the first Bedrock call the whole request failed; the cloudv2 side will land a separate proto-level fix to make description non-empty for new data, but this client-side preflight gives every existing caller a useful error today.

Test plan

• go test ./providers/bedrock/... — full provider suite passes
• golangci-lint run ./providers/bedrock/... — clean
• New TestRequestMapper_EmptyToolDescription asserts: error wraps llm.ErrRequestMapping, both empty-description tools are named in the message, valid tools are not flagged

🤖 Generated with Claude Code

[claude]

Code Review

Summary: This PR adds upfront validation in the Bedrock provider's mapToolConfig to reject tools with empty descriptions before hitting Bedrock's Converse API — which otherwise returns an opaque ValidationException at tool index rather than tool name. Good, targeted fix.

---

What works well

• Correct scoping. Bedrock-only validation is the right call; OpenAI and Anthropic accept empty descriptions, so hoisting this to agent.New() would break cross-provider agents. The PR description explains this clearly.
• Batch error reporting. Collecting all offenders into one error (vs. failing on the first) is a meaningful UX improvement for agents with many tools.
• Test is solid. Covers the primary error path, errors.Is sentinel check, offenders named in the message, and valid tools not flagged. Uses t.Parallel() per project convention.
• errors.Is chain is correct. mapToolConfig's raw error is wrapped by buildConverseParams at line 104 (%w: tool mapping failed: %w), so ErrorIs(err, llm.ErrRequestMapping) works as expected through ToConverseInput.

---

Issues / suggestions

1. Whitespace-only descriptions are also rejected by Bedrock

A description of " " will pass this check but Bedrock will still reject it (same ValidationException). Consider:

if strings.TrimSpace(tool.Description) == "" {
    missingDesc = append(missingDesc, tool.Name)
}

This costs nothing and closes the gap between the validation and the actual Bedrock constraint.

2. Fragile negative assertion in the test

assert.NotContains(t, err.Error(), "summarize,")

The trailing comma means this assertion passes if "summarize" appears last in the list (no trailing comma). The intent — that valid tools are not named — is right, but the assertion is accidentally wrong in that edge case. Simpler and clearer:

assert.NotContains(t, err.Error(), "search")
assert.NotContains(t, err.Error(), "summarize")

These are unambiguous as long as no offending tool shares a prefix with a valid tool name (they don't here).

3. Comment verbosity (minor)

The 5-line block comment in mapToolConfig is on the verbose side for a project that uses sparse commenting. The core "why" (// Bedrock rejects empty tool descriptions with an opaque ValidationException) in one line is sufficient — the rest is derivable from the code itself.

4. mapToolConfig relies on caller for ErrRequestMapping wrapping (low risk, worth noting)

The sentinel is only attached by buildConverseParams, not by mapToolConfig itself. Today there's one call site, so it's fine. If mapToolConfig grows another caller that forgets to wrap, the sentinel silently disappears. Not a blocker, but something to keep in mind if the function is reused.

---

Verdict

Approve with the whitespace-only description fix strongly recommended (it closes a real gap between the client check and Bedrock's actual constraint) and the test assertion tightened. The core logic and structure are correct.

[blacksmith-sh]

Found 1 test failure on Blacksmith runners:

Failure

Test	View Logs
github.com/redpanda-data/ai-sdk-go/providers/openai/TestOpenAICachedTokens	View Logs

[weeco]

@claude im wondering whether we should do the check at tool registration time. Would it make sense to require this for all tools across providers? Breaking changes / behaviors are still acceptable

[claude]

Should empty-description validation move to tool registration?

• Read relevant source files to understand tool registration architecture
• Analyze tradeoffs of Bedrock-scoped vs. universal validation
• Post recommendation

---

Short answer: yes, move it to tool.Registry.Register() — that's the right architectural seam, and since breaking changes are acceptable, there's no reason not to.

Here's the full analysis:

How tools flow today

tool.Registry.Register()          ← where we'd add the check
        ↓
tool.Registry.List() → []llm.ToolDefinition
        ↓
provider's mapToolConfig()        ← where the check lives now
        ↓
Bedrock / OpenAI / Anthropic

Register() already validates Name != "" (ErrToolNameEmpty). Description is the natural next field to gate on — a tool without a description is arguably an incomplete definition regardless of provider, since the LLM literally can't know what the tool does.

Why registration time is better

	Current (Bedrock mapToolConfig)	Proposed (tool.Registry.Register)
Failure point	First LLM call at runtime	Program startup / configuration
Error owner	Provider-specific, error wrapped in llm.ErrRequestMapping	Universal, surfaced as ErrToolDescriptionEmpty
Providers covered	Bedrock only	All
DRY	Bedrock-specific copy	Single check, Bedrock mapToolConfig simplifies
MCP tools	Caught only on Bedrock	Caught universally at registration
agenttool.New()	Caught only on Bedrock	Caught when the parent agent is configured

The fail-fast argument is strong: if a tool has no description, it's a configuration error, not a runtime error. Catching it at Register() gives the developer a clear error at startup rather than buried in a live request chain.

What to change

1. tool/errors.go — add ErrToolDescriptionEmpty
2. tool/registry.go:Register() — after the name check, add:

   if strings.TrimSpace(definition.Description) == "" {
       return fmt.Errorf("%w: %q", ErrToolDescriptionEmpty, definition.Name)
   }

   Use TrimSpace to catch whitespace-only strings (consistent with what the earlier review suggested for the Bedrock check).
3. providers/bedrock/request_mapper.go:mapToolConfig() — remove the empty-description validation block entirely (it becomes dead code once Register() guards it).
4. providers/bedrock/provider_test.go — remove/replace TestRequestMapper_EmptyToolDescription with a test in tool/registry_test.go that covers the universal case.
5. agent/llmagent/config.go:validate() — optionally add a check that description != "" when tools are registered via agenttool, though the registry gate will catch this anyway.

One consideration: agenttool

agenttool.New(a) pulls description from a.Info().Description, which is empty if WithDescription() is not called. Today llmagent.New doesn't require a description. If we add the registry guard, any caller doing registry.Register(agenttool.New(agent)) without having called WithDescription(...) will get ErrToolDescriptionEmpty. That's the desired breaking change — it forces callers to be explicit about what the subagent does.

Verdict

This is a clean improvement. The check belongs in one place (Register()), not in each provider that happens to enforce it. Given that breaking changes are on the table, this is the right call. The Bedrock-specific code in this PR can then be removed, and the fix becomes smaller and more principled.

View job run

[birdayz]

hah i had exactly same thought @weeco . agree, it should be done at registration time.

[birdayz]

i digged a bit deeper, and maybe it's the wrong call to reject empty desc always..

  ┌─────────────────────┬─────────────────────────────────────────────────────────────────────────────┐
  │         SDK         │                           Validates description?                            │
  ├─────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
  │ adk-go              │ No — Tool.Description() string interface, value is passed through unchecked │
  ├─────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
  │ adk-python          │ No — BaseTool accepts description: str with no runtime check                │
  ├─────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
  │ openai-agents-js    │ No — typed as string in StrictToolOptions, no runtime check                 │
  ├─────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
  │ ai-sdk-go (this PR) │ Bedrock-only at mapping time                                                │
  └─────────────────────┴─────────────────────────────────────────────────────────────────────────────┘
  
    Nobody else validates this — anywhere. Not at registration, not at model-call, not provider-specifically. Type systems make it string,
   but empty string passes.

IMHO would not be right call to reject at registration time, the runtime fix is cleaner. LGTM

[weeco]

@simon0191 I was bothered by this solution and did some research. I noticed:

1. Bedrock / AWS itself claims the tool description is NOT required, but when provided it can't be empty, see: https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ToolSpecification.html
2. This leads me to believe that this is the problem - we should omit it instead of setting an empty aws.String()

Have you tried this and see if that works?

[birdayz]

good catch!