Merge pull request #4 from carmelosantana/chore-fix-400

carmelosantana · web-flow · commit 61fa764f3569 · 2026-02-15T12:32:37.000-05:00
Enhance provider error reporting and Anthropic message handling
diff --git a/src/Agent/AbstractAgent.php b/src/Agent/AbstractAgent.php
@@ -23,6 +23,7 @@
 use CarmeloSantana\PHPAgents\Tool\DoneTool;
 use CarmeloSantana\PHPAgents\Tool\ToolResult;
 use SplObserver;
+use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;
 
 abstract class AbstractAgent implements AgentInterface
 {
@@ -105,10 +106,32 @@ public function run(MessageInterface $input, ?Conversation $history = null): Out
                     $allTools,
                 );
             } catch (\Throwable $e) {
-                $this->notify('agent.error', $e->getMessage());
+                $errorMessage = $e->getMessage();
+
+                // Extract API response body for HTTP client errors (4xx/5xx)
+                // Symfony's ClientException only includes the status line, not the
+                // API error body, for non-RFC-7807 APIs like Anthropic and OpenAI.
+                if ($e instanceof ClientExceptionInterface) {
+                    try {
+                        $body = $e->getResponse()->getContent(false);
+                        $decoded = json_decode($body, true);
+
+                        // Anthropic: {"type":"error","error":{"message":"..."}}
+                        // OpenAI:    {"error":{"message":"..."}}
+                        $apiMessage = $decoded['error']['message']
+                            ?? $decoded['message']
+                            ?? $body;
+
+                        $errorMessage .= "\n\nAPI response: " . $apiMessage;
+                    } catch (\Throwable) {
+                        // If we can't read the body, fall through with original message
+                    }
+                }
+
+                $this->notify('agent.error', $errorMessage);
 
                 return new Output(
-                    content: 'Provider error: ' . $e->getMessage(),
+                    content: 'Provider error: ' . $errorMessage,
                     toolResults: $allToolResults,
                     usage: $totalUsage,
                     iterations: $i + 1,
diff --git a/src/Provider/AnthropicProvider.php b/src/Provider/AnthropicProvider.php
@@ -18,12 +18,13 @@ final class AnthropicProvider extends AbstractProvider
 
     public function __construct(
         string $model = 'claude-sonnet-4-20250514',
+        string $baseUrl = 'https://api.anthropic.com/v1',
         string $apiKey = '',
         ?HttpClientInterface $httpClient = null,
     ) {
         parent::__construct(
             model: $model,
-            baseUrl: 'https://api.anthropic.com/v1',
+            baseUrl: $baseUrl,
             apiKey: $apiKey,
             httpClient: $httpClient,
         );
@@ -148,7 +149,7 @@ protected function formatMessages(array $messages): array
     private function extractSystemAndMessages(array $messages): array
     {
         $systemPrompt = '';
-        $formattedMessages = [];
+        $formatted = [];
 
         foreach ($messages as $message) {
             if ($message->role() === Role::System) {
@@ -157,10 +158,23 @@ private function extractSystemAndMessages(array $messages): array
                 continue;
             }
 
-            $formattedMessages[] = $this->formatAnthropicMessage($message);
+            $formatted[] = $this->formatAnthropicMessage($message);
         }
 
-        return [$systemPrompt, $formattedMessages];
+        // Merge consecutive same-role messages (required by Anthropic).
+        // Consecutive tool_result user messages must be combined into a single
+        // user message with multiple content blocks.
+        $merged = [];
+        foreach ($formatted as $msg) {
+            $last = end($merged);
+            if ($last !== false && $last['role'] === $msg['role'] && is_array($last['content']) && is_array($msg['content'])) {
+                $merged[array_key_last($merged)]['content'] = array_merge($last['content'], $msg['content']);
+            } else {
+                $merged[] = $msg;
+            }
+        }
+
+        return [$systemPrompt, $merged];
     }
 
     /**
@@ -175,19 +189,50 @@ private function formatAnthropicMessage(MessageInterface $message): array
             default => 'user',
         };
 
+        // Tool result messages → user message with tool_result content block
         if ($message->role() === Role::Tool) {
+            $toolCallId = $message->toolCallId();
+
+            // Anthropic requires a non-null tool_use_id. Generate a fallback
+            // for replayed conversations where the ID was not persisted.
+            if ($toolCallId === null || $toolCallId === '') {
+                $toolCallId = 'toolu_' . bin2hex(random_bytes(12));
+            }
+
             return [
                 'role' => 'user',
                 'content' => [
                     [
                         'type' => 'tool_result',
-                        'tool_use_id' => $message->toolCallId(),
+                        'tool_use_id' => $toolCallId,
                         'content' => $message->content(),
                     ],
                 ],
             ];
         }
 
+        // Assistant messages with tool calls → content blocks with tool_use
+        if ($message->role() === Role::Assistant && !empty($message->toolCalls())) {
+            $content = [];
+            $text = $message->content();
+            if (is_string($text) && $text !== '') {
+                $content[] = ['type' => 'text', 'text' => $text];
+            }
+            foreach ($message->toolCalls() as $toolCall) {
+                $content[] = [
+                    'type' => 'tool_use',
+                    'id' => $toolCall->id,
+                    'name' => $toolCall->name,
+                    'input' => !empty($toolCall->arguments) ? $toolCall->arguments : (object) [],
+                ];
+            }
+
+            return [
+                'role' => 'assistant',
+                'content' => $content,
+            ];
+        }
+
         return [
             'role' => $role,
             'content' => $message->content(),
diff --git a/src/Provider/ProviderFactory.php b/src/Provider/ProviderFactory.php
@@ -9,12 +9,41 @@
 
 final class ProviderFactory
 {
+    /**
+     * Conventional environment variable names for provider API keys.
+     *
+     * Checked via getenv() before falling back to openclaw.json config values.
+     * Coqui's CredentialResolver calls putenv() at boot, so workspace .env
+     * entries are automatically available here.
+     */
+    private const ENV_KEY_MAP = [
+        'openai' => 'OPENAI_API_KEY',
+        'anthropic' => 'ANTHROPIC_API_KEY',
+        'openrouter' => 'OPENROUTER_API_KEY',
+    ];
+
+    public function __construct(
+        private readonly ?ConfigInterface $config = null,
+    ) {}
+
+    /**
+     * Create a provider from a model string using injected config.
+     *
+     * Preferred over the static method when you have a factory instance,
+     * since the config is already bound and doesn't need to be passed each time.
+     */
+    public function create(string $modelString): ProviderInterface
+    {
+        return self::fromModelString($modelString, $this->config);
+    }
+
     /**
      * Create a provider from an OpenClaw-style model string.
      *
      * @param string $modelString e.g., "ollama/llama3.2:latest"
      * @param ConfigInterface|null $config OpenClaw config for baseUrl/apiKey lookups
      */
+
     public static function fromModelString(
         string $modelString,
         ?ConfigInterface $config = null,
@@ -23,7 +52,7 @@ public static function fromModelString(
 
         $providerConfig = $config?->getProviderConfig($providerName) ?? [];
         $baseUrl = self::resolveBaseUrl($providerName, $providerConfig);
-        $apiKey = $providerConfig['apiKey'] ?? '';
+        $apiKey = self::resolveApiKey($providerName, $providerConfig);
 
         return match ($providerName) {
             'ollama' => new OllamaProvider(
@@ -32,12 +61,13 @@ public static function fromModelString(
             ),
             'anthropic' => new AnthropicProvider(
                 model: $model,
-                apiKey: is_string($apiKey) ? $apiKey : '',
+                baseUrl: $baseUrl,
+                apiKey: $apiKey,
             ),
             default => new OpenAICompatibleProvider(
                 model: $model,
                 baseUrl: $baseUrl,
-                apiKey: is_string($apiKey) ? $apiKey : '',
+                apiKey: $apiKey,
             ),
         };
     }
@@ -92,4 +122,28 @@ private static function defaultBaseUrl(string $provider): string
             default => '',
         };
     }
+
+    /**
+     * Resolve API key with environment variable override.
+     *
+     * Priority: getenv(PROVIDER_API_KEY) > config apiKey > empty string.
+     * This allows .env files to override hardcoded config values, and
+     * enables Coqui's CredentialTool to manage provider keys at runtime.
+     *
+     * @param array<string, mixed> $providerConfig
+     */
+    private static function resolveApiKey(string $provider, array $providerConfig): string
+    {
+        // Check environment variable first (highest priority)
+        $envVar = self::ENV_KEY_MAP[$provider] ?? strtoupper($provider) . '_API_KEY';
+        $envValue = getenv($envVar);
+        if ($envValue !== false && $envValue !== '') {
+            return $envValue;
+        }
+
+        // Fall back to config value
+        $configKey = $providerConfig['apiKey'] ?? '';
+
+        return is_string($configKey) ? $configKey : '';
+    }
 }
diff --git a/src/Tool/Tool.php b/src/Tool/Tool.php
@@ -18,7 +18,6 @@ public function __construct(
         private readonly string $description,
         private readonly array $parameters,
         private readonly Closure $callback,
-        private readonly int $maxTries = 3,
     ) {}
 
     public function name(): string
@@ -36,11 +35,6 @@ public function parameters(): array
         return $this->parameters;
     }
 
-    public function maxTries(): int
-    {
-        return $this->maxTries;
-    }
-
     public function execute(array $input): ToolResult
     {
         try {
@@ -69,16 +63,21 @@ public function toFunctionSchema(): array
             }
         }
 
+        $schema = [
+            'type' => 'object',
+            'properties' => empty($properties) ? new \stdClass() : $properties,
+        ];
+
+        if (!empty($required)) {
+            $schema['required'] = $required;
+        }
+
         return [
             'type' => 'function',
             'function' => [
                 'name' => $this->name,
                 'description' => $this->description,
-                'parameters' => [
-                    'type' => 'object',
-                    'properties' => $properties,
-                    'required' => $required,
-                ],
+                'parameters' => $schema,
             ],
         ];
     }
diff --git a/tests/Unit/Tool/ToolTest.php b/tests/Unit/Tool/ToolTest.php
@@ -82,13 +82,14 @@
     expect($schema['function']['parameters']['required'])->toBe(['query']);
 });
 
-test('tool default maxTries is 3', function () {
+test('toFunctionSchema omits required when no required parameters', function () {
     $tool = new Tool(
         name: 'test',
         description: 'test',
         parameters: [],
         callback: fn(array $input) => '',
     );
 
-    expect($tool->maxTries())->toBe(3);
+    $schema = $tool->toFunctionSchema();
+    expect($schema['function']['parameters'])->not->toHaveKey('required');
 });
