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.

Found 1 test failure on Blacksmith runners:

Failure

@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

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

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

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

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