From 9e087e877118f86e769bb310b5d38eaf8268d086 Mon Sep 17 00:00:00 2001 From: Eugene Eisenstein Date: Wed, 15 Jul 2026 11:47:49 -0400 Subject: [PATCH] feat(llm backend): enable combined tool calling + structured output in the LLM backend transport layer (#907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(llm): support combined tool calling and structured output across backends - OpenAI: parse() 500s on non-strict function tools; route tool-carrying structured requests through create() with an explicit json_schema response_format (mirrors the streaming path) - Anthropic: skip the '{' JSON prefill when tools are present so tool_use blocks stay reachable; make the schema instruction conditional and rely on parse + repair - Gemini: native response_schema + function calling is rejected before Gemini 3; with tools present, inject a schema instruction into the final turn instead and rely on parse + repair - All backends: tool-call turns carry no consumable content, so skip structured-output parsing on them Extracted from the dialectic structured-output branch (DEV-1652) so the transport layer can land independently. DEV-2035 Co-Authored-By: Claude Fable 5 * test(live_llm): exercise combined tools + structured output per provider Two-turn live flow per backend: a forced tool-call turn (structured parsing must be skipped) followed by a replay turn that must return a schema-conforming answer with tools still attached. Asserts the provider-specific request shaping: no parse() for OpenAI (500s on non-strict tools), no '{' prefill for Anthropic, no native response_schema for Gemini. Verified against live OpenAI (gpt-4.1, gpt-5, gpt-5.4, gpt-5.4-mini) and Gemini (gemini-2.5-flash). DEV-2035 Co-Authored-By: Claude Fable 5 * fix: some needed unrelated test failures * ci: add label-triggered live LLM test workflow Adding the run-live-llm label to a PR (or workflow_dispatch) runs tests/live_llm/ against real provider APIs — the only place the --live-llm suite runs in CI. Reuses the unified-tests environment and its Secrets Manager staging-dotenv resolution for provider keys; runs on ubuntu-latest (no Fly runner, no Docker — the suite only touches the LLM backends). Pins LIVE_LLM_ANTHROPIC_45_PLUS_MODELS=claude-sonnet-4-5 since the Anthropic family has no default models and would otherwise silently collect empty. Opt-in by design: live model behavior is variable, so this is a signal, not a required check. DEV-2035 Co-Authored-By: Claude Fable 5 * ci: run live LLM tests on main pushes touching the transport Mirrors unified-tests' push trigger, scoped to paths that can affect the live suite (src/llm/, config, the tests, deps, and the workflow itself) so provider API calls aren't spent on unrelated changes. DEV-2035 Co-Authored-By: Claude Fable 5 * ci: disable auth in live LLM test environment The staging dotenv sets AUTH_USE_AUTH=true without a usable JWT secret, and src/config.py validates the pair at import time — the same reason unified-tests overrides it. This suite never runs the API server. DEV-2035 Co-Authored-By: Claude Fable 5 * test(live_llm): fix gpt-5.4 reasoning_effort and gemini replay-turn flake - test_live_openai: gpt-5.4 dropped 'minimal' from the reasoning_effort vocabulary, so the gpt5 caching test 400'd — and the OpenAI backend's BadRequestError terminal swallowed it into an empty CompletionResult. Pick the effort per model generation. - test_live_tools_structured_output: use tool_choice='auto' on the replay turn, matching the production dialectic loop (which never forces 'none') — NONE mode is what provoked gemini-2.5-flash's empty candidates. Drop the temperature pin so retries actually resample, and treat a repeat tool call as a retryable attempt. Verified live: full suite green, gemini 4/4 consecutive passes. DEV-2035 Co-Authored-By: Claude Fable 5 * ci: fail live LLM run when no staging secret was loaded If the latest-tag fetch fails and no second tag exists, the fallback step is skipped rather than failed, and the job would proceed without provider keys — every test then skips via require_provider_key and the run goes green. Guard on both fetch outcomes so that path fails loudly. DEV-2035 Co-Authored-By: Claude Fable 5 * docs(live-llm-tests-GHA): remove extra comments * ci(CODEOWNERS): introduce CODEOWNERS and gate GHA heavy test runs behind being a CODEOWNER * ci(GHA-live-LLM-tests): consolidate common GHA steps * test(test_live_openai): fix reasoning level adjustment for gpt-5 * test(live-llm-tests): temporary removal of gate to test the workflow * test(live-llm-tests): revert removal of gate --------- Co-authored-by: Claude Fable 5 --- .github/CODEOWNERS | 19 ++ .../actions/load-staging-secrets/action.yml | 84 ++++++ .github/workflows/live-llm-tests.yml | 123 +++++++++ .github/workflows/manual-trigger-gate.yml | 81 ++++++ .github/workflows/unified-tests.yml | 110 +++----- src/llm/backends/anthropic.py | 24 +- src/llm/backends/gemini.py | 44 +++- src/llm/backends/openai.py | 42 ++- src/llm/structured_output.py | 16 ++ tests/live_llm/test_live_openai.py | 8 +- .../test_live_tools_structured_output.py | 246 ++++++++++++++++++ tests/llm/test_backends/test_anthropic.py | 204 +++++++++++++++ tests/llm/test_backends/test_gemini.py | 123 +++++++++ tests/llm/test_backends/test_openai.py | 120 +++++++++ tests/routes/test_messages.py | 2 + 15 files changed, 1150 insertions(+), 96 deletions(-) create mode 100644 .github/CODEOWNERS create mode 100644 .github/actions/load-staging-secrets/action.yml create mode 100644 .github/workflows/live-llm-tests.yml create mode 100644 .github/workflows/manual-trigger-gate.yml create mode 100644 tests/live_llm/test_live_tools_structured_output.py diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 00000000..039d4e6e --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,19 @@ +# Code owners for Honcho. +# +# Beyond review routing, this file is the allowlist for manually triggering +# the live-llm-tests and unified-tests workflows (via their PR labels and +# workflow_dispatch). The gate jobs grep every @username in this file — +# regardless of which path pattern it sits on — and read it from `main`, +# never from the PR branch, so additions only take effect once merged. +# +# The workflow gates only understand individual @usernames (no @org/team +# entries). + +# Reviewers auto-requested on changes under .github/ (workflows, this file, +# templates). +/.github/ @akattelu @eisene @Rajat-Ahuja1997 @VVoruganti + +# CI-trigger allowlist only: this path matches no real file, so these people +# are never auto-requested for review, but the workflow gates still pick +# them up. +/ci-trigger-allowlist @3un01a @adavyas @ajspig @courtlandleer @erosika @lowyelling @matthewlanders @vintrocode diff --git a/.github/actions/load-staging-secrets/action.yml b/.github/actions/load-staging-secrets/action.yml new file mode 100644 index 00000000..64665ec5 --- /dev/null +++ b/.github/actions/load-staging-secrets/action.yml @@ -0,0 +1,84 @@ +name: Load staging secrets +description: >- + Resolve staging secret ids from the two newest v git tags, + then load the newest fetchable secret's keys into the job environment from + AWS Secrets Manager. Falls back to the second-latest tag when the latest + tag's secret isn't published yet, and fails the job loudly when neither can + be fetched. Requires the repository to be checked out and AWS credentials to + be configured beforehand. + +inputs: + secret-prefix: + description: >- + Secret-name prefix combined with a resolved tag version to form the full + secret id. Masked so it stays out of public CI logs. + required: true + +runs: + using: composite + steps: + - name: Resolve secret ids from latest git tags + id: resolve-secret + shell: bash + env: + SECRET_PREFIX: ${{ inputs.secret-prefix }} + run: | + set -euo pipefail + : "${SECRET_PREFIX:?secret-prefix input is empty — is the STAGING_SECRET_PREFIX secret set for this environment?}" + # Keep the secret-name prefix out of public CI logs. + echo "::add-mask::${SECRET_PREFIX}" + + # Two newest v tags, highest first (tags are public). + versions="$(git ls-remote --tags origin 'v*' \ + | sed -n 's#.*refs/tags/v\([0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\)$#\1#p' \ + | sort -t. -k1,1nr -k2,2nr -k3,3nr -u)" + latest="$(printf '%s\n' "$versions" | sed -n '1p')" + second="$(printf '%s\n' "$versions" | sed -n '2p')" + if [ -z "${latest:-}" ]; then + echo "::error::No v git tags found to resolve a secret version" + exit 1 + fi + + latest_id="${SECRET_PREFIX}${latest}" + echo "::add-mask::${latest_id}" + echo "latest-id=${latest_id}" >> "$GITHUB_OUTPUT" + echo "Latest version: ${latest}" + if [ -n "${second:-}" ]; then + second_id="${SECRET_PREFIX}${second}" + echo "::add-mask::${second_id}" + echo "second-id=${second_id}" >> "$GITHUB_OUTPUT" + echo "Fallback version: ${second}" + fi + + # Fetch the latest tag's secret. continue-on-error so a not-yet-published + # latest falls through to the second-latest instead of failing the job. + - name: Fetch staging secret (latest) + id: fetch-latest + continue-on-error: true + uses: aws-actions/aws-secretsmanager-get-secrets@v2 + with: + secret-ids: | + ,${{ steps.resolve-secret.outputs.latest-id }} + parse-json-secrets: true + + # Runs only if the latest fetch failed; this one is NOT continue-on-error, + # so if the fallback also fails the job fails loudly. + - name: Fetch staging secret (fallback to second-latest) + id: fetch-fallback + if: steps.fetch-latest.outcome == 'failure' && steps.resolve-secret.outputs.second-id != '' + uses: aws-actions/aws-secretsmanager-get-secrets@v2 + with: + secret-ids: | + ,${{ steps.resolve-secret.outputs.second-id }} + parse-json-secrets: true + + # If the latest fetch failed and the fallback was skipped (no second tag), + # no secret keys were loaded — fail here instead of letting the job run + # without staging config (e.g. every live LLM test would silently skip via + # require_provider_key and the run would go green). + - name: Verify staging secrets were loaded + if: steps.fetch-latest.outcome != 'success' && steps.fetch-fallback.outcome != 'success' + shell: bash + run: | + echo "::error::No staging secret could be fetched (latest failed; fallback skipped or failed)" + exit 1 diff --git a/.github/workflows/live-llm-tests.yml b/.github/workflows/live-llm-tests.yml new file mode 100644 index 00000000..ee1b5b2b --- /dev/null +++ b/.github/workflows/live-llm-tests.yml @@ -0,0 +1,123 @@ +name: Live LLM Tests + +on: + # Runs on main pushes that can affect the LLM transport (narrower than + # unified-tests' src/** — live provider calls aren't worth burning on + # changes that can't reach the backends). + push: + branches: [main] + paths: + - 'src/llm/**' + - 'src/config.py' + - 'tests/live_llm/**' + - 'pyproject.toml' + - 'uv.lock' + - '.github/workflows/live-llm-tests.yml' + # Manual trigger for PRs: add the `run-live-llm` label to run the suite + # against the PR's merge commit. The label is purged as soon as the run + # starts so it can be re-added to trigger another run. + pull_request: + types: [labeled] + workflow_dispatch: + +# Cap spend: at most one active run per PR (per ref for push/dispatch). +# Re-triggering a PR run cancels the in-flight one instead of stacking live +# provider calls; pushes to main queue instead of cancelling so main CI +# results aren't lost. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: ${{ github.event_name != 'push' }} + +permissions: + contents: read + +jobs: + # Only code owners (.github/CODEOWNERS) may trigger the suite manually via + # the label or workflow_dispatch; the gate also purges the trigger label so + # it can be re-added to trigger another run. + gate: + name: Gate manual trigger + permissions: + contents: read + pull-requests: write + uses: ./.github/workflows/manual-trigger-gate.yml + with: + label: run-live-llm + allow-workflow-dispatch: true + + live-llm-tests: + name: Run Live LLM Tests + needs: gate + # always() lets this run on push events, where the gate's jobs are skipped. + # Manual triggers (label / workflow_dispatch) additionally require the + # gate's CODEOWNERS check to have passed. + if: >- + always() && + (github.event_name == 'push' || + ((github.event_name == 'workflow_dispatch' || + github.event.label.name == 'run-live-llm') && + needs.gate.outputs.authorized == 'true')) + runs-on: ubuntu-latest + timeout-minutes: 20 + environment: unified-tests + permissions: + id-token: write # Required for OIDC authentication with AWS + contents: read + env: + PYTHONUNBUFFERED: "1" + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + ref: ${{ github.sha }} + + - name: Configure AWS credentials + uses: aws-actions/configure-aws-credentials@v4 + with: + role-to-assume: ${{ vars.AWS_OIDC_ROLE_ARN }} + aws-region: us-east-1 + role-duration-seconds: 3600 + + # Resolves secret ids from the newest release tags, fetches the newest + # available staging secret into the job env, and fails if none loaded. + - name: Load staging secrets + uses: ./.github/actions/load-staging-secrets + with: + secret-prefix: ${{ secrets.STAGING_SECRET_PREFIX }} + + # Configure the test environment. Sentry/CloudEvents endpoints aren't + # reachable from CI. LIVE_LLM_ANTHROPIC_45_PLUS_MODELS must be set for + # the Anthropic tests to materialize — the claude_4_5_plus family has no + # default models, so with only the API key they'd silently collect as + # empty parameter sets. + - name: Configure test environment + run: | + { + # The staging dotenv carries AUTH_USE_AUTH=true without a usable + # JWT secret; src/config.py validates the pair at import time, so + # disable auth (this suite never runs the API server anyway). + echo "AUTH_USE_AUTH=false" + echo "SENTRY_ENABLED=false" + echo "TELEMETRY_ENABLED=false" + echo "LIVE_LLM_ANTHROPIC_45_PLUS_MODELS=claude-sonnet-4-5" + } >> "$GITHUB_ENV" + + - name: Install uv + uses: astral-sh/setup-uv@v2 + with: + enable-cache: true + cache-dependency-glob: "uv.lock" + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version-file: "pyproject.toml" + + - name: Install the project + run: uv sync --all-extras + + # -n 0 overrides the `-n auto` xdist default from pyproject: ~15 short + # tests gain nothing from parallelism, serial execution avoids bursting + # every provider at once, and flake diagnosis gets ordered output. + - name: Run live LLM tests + run: uv run --frozen pytest tests/live_llm/ --live-llm -n 0 -v diff --git a/.github/workflows/manual-trigger-gate.yml b/.github/workflows/manual-trigger-gate.yml new file mode 100644 index 00000000..6cd8b209 --- /dev/null +++ b/.github/workflows/manual-trigger-gate.yml @@ -0,0 +1,81 @@ +name: Manual Trigger Gate + +# Shared gate for workflows that can be triggered manually on PRs by adding a +# label (and optionally via workflow_dispatch): verifies the actor is a code +# owner and purges the trigger label so it can be re-added for another run. +# +# Callers must grant `pull-requests: write` on the calling job so the +# remove-label job can delete the label, and should gate downstream jobs on +# the `authorized` output rather than this workflow's conclusion. + +on: + workflow_call: + inputs: + label: + description: PR label that triggers the calling workflow + required: true + type: string + allow-workflow-dispatch: + description: Whether workflow_dispatch events may pass the gate + required: false + default: false + type: boolean + outputs: + authorized: + description: >- + 'true' when the manual trigger's actor passed the CODEOWNERS check. + Empty on events where the check did not run (e.g. push). + value: ${{ jobs.check-actor.outputs.authorized }} + +jobs: + # Only code owners (.github/CODEOWNERS) may trigger the calling workflow + # manually. + check-actor: + name: Verify actor is a code owner + if: >- + (inputs.allow-workflow-dispatch && github.event_name == 'workflow_dispatch') || + (github.event_name == 'pull_request' && github.event.label.name == inputs.label) + runs-on: ubuntu-latest + permissions: + contents: read + outputs: + authorized: ${{ steps.codeowners.outputs.authorized }} + steps: + - name: Check actor against CODEOWNERS on main + id: codeowners + env: + GH_TOKEN: ${{ github.token }} + ACTOR: ${{ github.actor }} + run: | + set -euo pipefail + # Usernames are case-insensitive on GitHub; compare lowercased. + owners="$(gh api -H "Accept: application/vnd.github.raw" \ + "repos/${{ github.repository }}/contents/.github/CODEOWNERS?ref=main" \ + | sed 's/#.*//' | grep -oE '@[A-Za-z0-9-]+' | tr -d '@' \ + | tr '[:upper:]' '[:lower:]' | sort -u)" + actor_lc="$(printf '%s' "$ACTOR" | tr '[:upper:]' '[:lower:]')" + if printf '%s\n' "$owners" | grep -qxF "$actor_lc"; then + echo "@${ACTOR} is a code owner; proceeding" + echo "authorized=true" >> "$GITHUB_OUTPUT" + else + echo "::error::@${ACTOR} is not listed in .github/CODEOWNERS on main — only code owners may trigger this workflow manually" + exit 1 + fi + + # Purge the trigger label first thing. Best-effort: failing to remove the + # label (e.g. read-only token on a fork PR) doesn't block the tests. + remove-label: + name: Remove trigger label + if: github.event_name == 'pull_request' && github.event.label.name == inputs.label + runs-on: ubuntu-latest + permissions: + pull-requests: write + steps: + - name: Remove trigger label + env: + GH_TOKEN: ${{ github.token }} + run: | + if ! gh api --method DELETE \ + "repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels/${{ inputs.label }}"; then + echo "::warning::Could not remove the ${{ inputs.label }} label (it may have been removed already)" + fi diff --git a/.github/workflows/unified-tests.yml b/.github/workflows/unified-tests.yml index 1ea2c539..4813a126 100644 --- a/.github/workflows/unified-tests.yml +++ b/.github/workflows/unified-tests.yml @@ -12,38 +12,44 @@ on: pull_request: types: [labeled] +# Cap spend: at most one active run per PR (per ref for push). Re-triggering +# a PR run cancels the in-flight one instead of stacking Fly machines; pushes +# to main queue instead of cancelling so main CI results aren't lost. The +# cleanup-machine job runs `if: always()`, which still executes on cancelled +# runs, so a cancelled run's Fly machine and runner are still torn down. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: ${{ github.event_name != 'push' }} + permissions: contents: read actions: read jobs: - # Purge the trigger label first thing. Best-effort: failing to remove the - # label (e.g. read-only token on a fork PR) doesn't block the tests. - remove-label: - name: Remove trigger label - if: github.event_name == 'pull_request' && github.event.label.name == 'run-unified-tests' - runs-on: ubuntu-latest + # Only code owners (.github/CODEOWNERS) may trigger the suite manually via + # the label; the gate also purges the trigger label so it can be re-added + # to trigger another run. + gate: + name: Gate manual trigger permissions: + contents: read pull-requests: write - steps: - - name: Remove run-unified-tests label - env: - GH_TOKEN: ${{ github.token }} - run: | - if ! gh api --method DELETE \ - "repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels/run-unified-tests"; then - echo "::warning::Could not remove the run-unified-tests label (it may have been removed already)" - fi + uses: ./.github/workflows/manual-trigger-gate.yml + with: + label: run-unified-tests start-runner: name: Start Fly Runner - needs: remove-label - # always() lets this run on push events, where remove-label is skipped. + needs: gate + # always() lets this run on push events, where the gate's jobs are skipped. # Label adds other than run-unified-tests trigger the workflow but skip - # every job here. + # every job here; the run-unified-tests label additionally requires the + # gate's CODEOWNERS check to have passed. if: >- always() && - (github.event_name == 'push' || github.event.label.name == 'run-unified-tests') + (github.event_name == 'push' || + (github.event.label.name == 'run-unified-tests' && + needs.gate.outputs.authorized == 'true')) uses: ./.github/workflows/start-fly-runner.yml secrets: inherit @@ -74,58 +80,12 @@ jobs: aws-region: us-east-1 role-duration-seconds: 43200 # 12 hours - - name: Resolve secret ids from latest git tags - id: resolve-secret - env: - SECRET_PREFIX: ${{ secrets.STAGING_SECRET_PREFIX }} - run: | - set -euo pipefail - : "${SECRET_PREFIX:?STAGING_SECRET_PREFIX secret is not set for this environment}" - # Keep the secret-name prefix out of public CI logs. - echo "::add-mask::${SECRET_PREFIX}" - - # Two newest v tags, highest first (tags are public). - versions="$(git ls-remote --tags origin 'v*' \ - | sed -n 's#.*refs/tags/v\([0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\)$#\1#p' \ - | sort -t. -k1,1nr -k2,2nr -k3,3nr -u)" - latest="$(printf '%s\n' "$versions" | sed -n '1p')" - second="$(printf '%s\n' "$versions" | sed -n '2p')" - if [ -z "${latest:-}" ]; then - echo "::error::No v git tags found to resolve a secret version" - exit 1 - fi - - latest_id="${SECRET_PREFIX}${latest}" - echo "::add-mask::${latest_id}" - echo "latest-id=${latest_id}" >> "$GITHUB_OUTPUT" - echo "Latest version: ${latest}" - if [ -n "${second:-}" ]; then - second_id="${SECRET_PREFIX}${second}" - echo "::add-mask::${second_id}" - echo "second-id=${second_id}" >> "$GITHUB_OUTPUT" - echo "Fallback version: ${second}" - fi - - # Fetch the latest tag's secret. continue-on-error so a not-yet-published - # latest falls through to the second-latest instead of failing the job. - - name: Fetch staging secret (latest) - id: fetch-latest - continue-on-error: true - uses: aws-actions/aws-secretsmanager-get-secrets@v2 + # Resolves secret ids from the newest release tags, fetches the newest + # available staging secret into the job env, and fails if none loaded. + - name: Load staging secrets + uses: ./.github/actions/load-staging-secrets with: - secret-ids: | - ,${{ steps.resolve-secret.outputs.latest-id }} - parse-json-secrets: true - - # Runs only if the latest fetch failed; this one is NOT continue-on-error, - # so if the fallback also fails the job fails loudly. - - name: Fetch staging secret (fallback to second-latest) - if: steps.fetch-latest.outcome == 'failure' && steps.resolve-secret.outputs.second-id != '' - uses: aws-actions/aws-secretsmanager-get-secrets@v2 - with: - secret-ids: | - ,${{ steps.resolve-secret.outputs.second-id }} - parse-json-secrets: true + secret-prefix: ${{ secrets.STAGING_SECRET_PREFIX }} # Layer test-specific overrides on top of the staging secret. The staging # dotenv tracks the deployed release and can drift from what main's config @@ -175,10 +135,12 @@ jobs: # been renamed or removed on main) must always be ignored by the app config. - name: Configure test environment run: | - echo "AUTH_USE_AUTH=false" >> "$GITHUB_ENV" - echo "SENTRY_ENABLED=false" >> "$GITHUB_ENV" - echo "TELEMETRY_ENABLED=false" >> "$GITHUB_ENV" - echo "REASONING_TRACES_FILE=unified-reasoning-traces.jsonl" >> "$GITHUB_ENV" + { + echo "AUTH_USE_AUTH=false" + echo "SENTRY_ENABLED=false" + echo "TELEMETRY_ENABLED=false" + echo "REASONING_TRACES_FILE=unified-reasoning-traces.jsonl" + } >> "$GITHUB_ENV" - name: Verify Docker is available run: docker info diff --git a/src/llm/backends/anthropic.py b/src/llm/backends/anthropic.py index 673f4d03..614a2e32 100644 --- a/src/llm/backends/anthropic.py +++ b/src/llm/backends/anthropic.py @@ -10,7 +10,7 @@ from pydantic import BaseModel, ValidationError from src.llm.backend import CompletionResult, StreamChunk, ToolCallResult from src.llm.request_builder import apply_sdk_passthroughs -from src.llm.structured_output import repair_response_model_json +from src.llm.structured_output import repair_response_model_json, schema_instruction class AnthropicBackend: @@ -74,26 +74,28 @@ class AnthropicBackend: # from ModelConfig.provider_params. Shallow merge with operator-wins. apply_sdk_passthroughs(params, extra_params) + # The '{' prefill forces a JSON-first response, which suppresses + # tool_use blocks — skip it when tools are available and rely on the + # conditional instruction + repair fallback instead. use_json_prefill = ( bool(response_format or self._json_mode(extra_params)) and not thinking_budget_tokens + and not tools and self._supports_assistant_prefill(model) ) if use_json_prefill and params["messages"]: if response_format and isinstance(response_format, type): - schema_json = json.dumps(response_format.model_json_schema(), indent=2) self._append_text_to_last_message( params["messages"], - f"\n\nRespond with valid JSON matching this schema:\n{schema_json}", + schema_instruction(response_format, tools_present=False), ) params["messages"].append({"role": "assistant", "content": "{"}) elif ( response_format and isinstance(response_format, type) and params["messages"] ): - schema_json = json.dumps(response_format.model_json_schema(), indent=2) self._append_text_to_last_message( params["messages"], - f"\n\nRespond with valid JSON matching this schema:\n{schema_json}", + schema_instruction(response_format, tools_present=bool(tools)), ) response = await self._client.messages.create(**params) @@ -155,26 +157,27 @@ class AnthropicBackend: # Operator escape hatch: forward Anthropic SDK passthrough kwargs # from ModelConfig.provider_params. Shallow merge with operator-wins. apply_sdk_passthroughs(params, extra_params) + # See complete(): no '{' prefill when tools are available, so + # tool_use blocks stay reachable on the streamed path too. use_json_prefill = ( bool(response_format or is_json_mode) and not thinking_budget_tokens + and not tools and self._supports_assistant_prefill(model) ) if use_json_prefill and params["messages"]: if response_format and isinstance(response_format, type): - schema_json = json.dumps(response_format.model_json_schema(), indent=2) self._append_text_to_last_message( params["messages"], - f"\n\nRespond with valid JSON matching this schema:\n{schema_json}", + schema_instruction(response_format, tools_present=False), ) params["messages"].append({"role": "assistant", "content": "{"}) elif ( response_format and isinstance(response_format, type) and params["messages"] ): - schema_json = json.dumps(response_format.model_json_schema(), indent=2) self._append_text_to_last_message( params["messages"], - f"\n\nRespond with valid JSON matching this schema:\n{schema_json}", + schema_instruction(response_format, tools_present=bool(tools)), ) if thinking_budget_tokens: params["thinking"] = { @@ -251,7 +254,8 @@ class AnthropicBackend: ) content: Any = text_content - if response_format is not None: + # Tool-call turns carry no consumable content + if response_format is not None and not tool_calls: raw_content = f"{{{text_content}" if prefilled_json else text_content try: if prefilled_json: diff --git a/src/llm/backends/gemini.py b/src/llm/backends/gemini.py index 5c70268a..c114196e 100644 --- a/src/llm/backends/gemini.py +++ b/src/llm/backends/gemini.py @@ -15,7 +15,7 @@ from src.llm.caching import ( gemini_cache_store, ) from src.llm.request_builder import coerce_passthrough_mapping -from src.llm.structured_output import repair_response_model_json +from src.llm.structured_output import repair_response_model_json, schema_instruction GEMINI_BLOCKED_FINISH_REASONS = { "SAFETY", @@ -31,6 +31,29 @@ class GeminiBackend: def __init__(self, client: Any) -> None: self._client: Any = client + @staticmethod + def _append_schema_instruction( + contents: list[dict[str, Any]] | str, + response_format: type[BaseModel], + ) -> list[dict[str, Any]] | str: + """Append the schema instruction to the final turn. + + Used when tools accompany a response_format: native response_schema + + function calling is a Gemini 3 preview feature and earlier models + reject the pairing, so instruct the model and rely on parse + repair. + Returns a new list — _convert_messages shallow-copies parts-style + messages, so in-place appends would leak into the caller's history + and accumulate across tool-loop iterations. + """ + instruction = schema_instruction(response_format, tools_present=True) + if isinstance(contents, str): + return contents + instruction + if not contents: + return contents + last = contents[-1] + parts: list[Any] = [*(last.get("parts") or []), {"text": instruction}] + return [*contents[:-1], {**last, "parts": parts}] + async def complete( self, *, @@ -61,6 +84,10 @@ class GeminiBackend: ) if system_instruction: config["system_instruction"] = system_instruction + if tools and isinstance(response_format, type): + # The final turn is never part of the cached prefix, so this is + # safe to do before cache attachment. + contents = self._append_schema_instruction(contents, response_format) cache_policy = ( extra_params.get("cache_policy") @@ -130,6 +157,10 @@ class GeminiBackend: ) if system_instruction: config["system_instruction"] = system_instruction + if tools and isinstance(response_format, type): + # The final turn is never part of the cached prefix, so this is + # safe to do before cache attachment. + contents = self._append_schema_instruction(contents, response_format) cache_policy = ( extra_params.get("cache_policy") @@ -228,7 +259,11 @@ class GeminiBackend: config["tools"] = self._convert_tools(tools) if tool_choice: config["tool_config"] = self._convert_tool_choice(tool_choice) - if response_format is not None: + # Native structured output combined with function calling is a + # Gemini 3 preview feature; earlier models reject the pairing. With + # tools present, callers inject a schema instruction instead (see + # _append_schema_instruction) and rely on parse + repair downstream. + if response_format is not None and not tools: config["response_mime_type"] = "application/json" config["response_schema"] = response_format elif extra_params and extra_params.get("json_mode") and not tools: @@ -335,7 +370,10 @@ class GeminiBackend: ) content: Any = "\n".join(text_parts) if text_parts else "" - if response_format is not None: + # Tool-call turns carry no consumable content — the tool loop ignores + # it — and parsing their (empty) text would raise through the repair + # fallback, failing the iteration. + if response_format is not None and not tool_calls: parsed_response = getattr(response, "parsed", None) if isinstance(parsed_response, response_format): content = parsed_response diff --git a/src/llm/backends/openai.py b/src/llm/backends/openai.py index 8c0ca8a4..6d13f1fc 100644 --- a/src/llm/backends/openai.py +++ b/src/llm/backends/openai.py @@ -172,6 +172,24 @@ class OpenAIBackend: response, response_format, model, empty_on_missing=True ) return self._normalize_response(response, content_override=content) + if tools: + # parse() refuses non-strict function tools, and our agent tool + # schemas are deliberately non-strict (see _convert_tools), so + # tool-loop iterations use create() with an explicit json_schema + # response_format — same server-side schema enforcement, no + # strict-tools requirement — mirroring the streaming path. + params["response_format"] = self._json_schema_response_format( + response_format + ) + response = await self._client.chat.completions.create(**params) + # Tool-call turns carry no consumable content — the tool loop + # ignores it — and parsing their empty text would raise. + if getattr(response.choices[0].message, "tool_calls", None): + return self._normalize_response(response) + content = self._parse_or_repair_structured_content( + response, response_format, model, empty_on_missing=False + ) + return self._normalize_response(response, content_override=content) params["response_format"] = response_format try: response = await self._client.chat.completions.parse(**params) @@ -274,13 +292,9 @@ class OpenAIBackend: else: # Streaming create() can't take a BaseModel like parse() does; # convert to a json_schema dict. - params["response_format"] = { - "type": "json_schema", - "json_schema": { - "name": response_format.__name__, - "schema": response_format.model_json_schema(), - }, - } + params["response_format"] = self._json_schema_response_format( + response_format + ) elif response_format is not None: params["response_format"] = response_format elif extra_params and extra_params.get("json_mode"): @@ -422,6 +436,20 @@ class OpenAIBackend: raw_response=response, ) + @staticmethod + def _json_schema_response_format( + response_format: type[BaseModel], + ) -> dict[str, Any]: + """Build the response_format param for create() calls that can't use + parse(): streaming, and requests carrying non-strict function tools.""" + return { + "type": "json_schema", + "json_schema": { + "name": response_format.__name__, + "schema": response_format.model_json_schema(), + }, + } + @staticmethod def _structured_output_mode(extra_params: dict[str, Any] | None) -> str | None: # Threaded in via extra_params (see build_config_extra_params). diff --git a/src/llm/structured_output.py b/src/llm/structured_output.py index 0bb346ff..37d6b982 100644 --- a/src/llm/structured_output.py +++ b/src/llm/structured_output.py @@ -12,6 +12,22 @@ class StructuredOutputError(ValueError): """Raised when structured output cannot be validated or repaired.""" +def schema_instruction(response_format: type[BaseModel], *, tools_present: bool) -> str: + """Structured-output instruction appended to the conversation for + providers without native (or tools-compatible) schema enforcement. + + When tools are in play the wording is conditional so the model remains + free to emit tool calls; validation then relies on parse + repair. + """ + schema_json = json.dumps(response_format.model_json_schema(), indent=2) + if tools_present: + return ( + "\n\nIf not responding with a tool call, respond with valid JSON " + f"matching this schema:\n{schema_json}" + ) + return f"\n\nRespond with valid JSON matching this schema:\n{schema_json}" + + def repair_response_model_json( raw_content: str, response_model: type[BaseModel], diff --git a/tests/live_llm/test_live_openai.py b/tests/live_llm/test_live_openai.py index e5617ff0..8b0544ec 100644 --- a/tests/live_llm/test_live_openai.py +++ b/tests/live_llm/test_live_openai.py @@ -93,7 +93,11 @@ async def test_live_openai_gpt5_reasoning_structured_output_and_prefix_caching( monkeypatch: pytest.MonkeyPatch, ) -> None: require_provider_key(model_spec) - backend, config = make_backend(model_spec, reasoning_effort="minimal") + # Only the original gpt-5 generation accepts 'minimal'; gpt-5.1+ replaced + # it with 'none'. 'low' is valid everywhere else, including future models. + is_base_gpt5 = model_spec.model == "gpt-5" or model_spec.model.startswith("gpt-5-") + reasoning_effort = "minimal" if is_base_gpt5 else "low" + backend, config = make_backend(model_spec, reasoning_effort=reasoning_effort) parse_calls = wrap_async_method( monkeypatch, backend._client.chat.completions, @@ -136,7 +140,7 @@ async def test_live_openai_gpt5_reasoning_structured_output_and_prefix_caching( assert second.cache_read_input_tokens > 0 assert parse_calls[0]["kwargs"]["response_format"] is StructuredLiveResponse - assert parse_calls[0]["kwargs"]["reasoning_effort"] == "minimal" + assert parse_calls[0]["kwargs"]["reasoning_effort"] == reasoning_effort assert "max_completion_tokens" in parse_calls[0]["kwargs"] assert "max_tokens" not in parse_calls[0]["kwargs"] diff --git a/tests/live_llm/test_live_tools_structured_output.py b/tests/live_llm/test_live_tools_structured_output.py new file mode 100644 index 00000000..33aac21b --- /dev/null +++ b/tests/live_llm/test_live_tools_structured_output.py @@ -0,0 +1,246 @@ +"""Live coverage for combining tool calling with structured output. + +Each provider needs a different workaround when a request carries both +function tools and a response_format (see src/llm/backends/): + +- OpenAI: parse() rejects non-strict function tools with a 500, so + tool-carrying structured requests must go through create() with an + explicit json_schema response_format. +- Anthropic: the '{' assistant prefill suppresses tool_use blocks, so it + must be skipped when tools are present (conditional instruction + + parse/repair instead). +- Gemini: native response_schema + function calling is rejected before + Gemini 3, so a schema instruction is injected into the final turn. + +The flow below drives both halves of the combination against real APIs: +a first turn that must produce a tool call (structured parsing skipped), +and a replay turn that must produce a schema-conforming final answer +while tools are still attached. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from pydantic import BaseModel, ValidationError + +from src.exceptions import LLMError +from src.llm.backend import CompletionResult +from src.llm.history_adapters import ( + AnthropicHistoryAdapter, + GeminiHistoryAdapter, + HistoryAdapter, + OpenAIHistoryAdapter, +) +from src.llm.request_builder import execute_completion +from src.llm.structured_output import StructuredOutputError + +from .conftest import ( + execute_local_tool, + favorite_prime_tools, + make_backend, + require_provider_key, + wrap_async_method, +) +from .model_matrix import LiveModelSpec, get_live_model_specs + +pytestmark = [pytest.mark.live_llm] + +_OPENAI_TOOL_SPECS = tuple( + spec + for spec in get_live_model_specs(provider="openai", feature="structured_output") + if spec.family in {"gpt_4_class", "gpt_5_class"} +) + + +class FavoritePrimeReport(BaseModel): + number: int + is_prime: bool + summary: str + + +_INITIAL_PROMPT = ( + "Before answering, call the get_favorite_prime tool exactly once. " + "Do not answer with plain text or JSON on this turn. " + "After you receive the tool result, answer with JSON where number is " + "the tool's number, is_prime says whether that number is prime, and " + "summary is one short sentence." +) + + +async def run_tool_then_structured_flow( + backend: Any, + config: Any, + adapter: HistoryAdapter, +) -> tuple[CompletionResult, CompletionResult]: + """First turn must tool-call (parsing skipped); replay turn must return + a schema-conforming answer with tools still attached.""" + initial_messages = [{"role": "user", "content": _INITIAL_PROMPT}] + tools = favorite_prime_tools() + + first = await execute_completion( + backend, + config, + messages=initial_messages, + max_tokens=4096, + tools=tools, + tool_choice="required", + response_format=FavoritePrimeReport, + ) + + assert first.tool_calls, "first turn should issue a tool call" + assert not isinstance( + first.content, FavoritePrimeReport + ), "tool-call turns carry no consumable content and must not be parsed" + + tool_call = first.tool_calls[0] + tool_result = execute_local_tool(tool_call.name, tool_call.input) + replay_messages = initial_messages + [ + adapter.format_assistant_tool_message(first), + *adapter.format_tool_results( + [ + { + "tool_id": tool_call.id, + "tool_name": tool_call.name, + "result": tool_result, + } + ] + ), + ] + + # tool_choice stays "auto" on the replay turn — the production tool loop + # (dialectic) never forces "none", and gemini-2.5-flash is prone to + # returning empty candidates under NONE mode. Retry the turn on empty / + # unparseable candidates (in production the executor's retry layer + # absorbs those; this calls the backend directly) and on the rare run + # where the model chooses to tool-call again instead of answering. + second: CompletionResult | None = None + last_error: Exception | None = None + for _ in range(3): + try: + candidate = await execute_completion( + backend, + config, + messages=replay_messages, + max_tokens=4096, + tools=tools, + tool_choice="auto", + response_format=FavoritePrimeReport, + ) + except (ValidationError, LLMError, StructuredOutputError) as exc: + last_error = exc + continue + if candidate.tool_calls: + last_error = AssertionError( + "model issued another tool call instead of answering" + ) + continue + second = candidate + break + if second is None: + raise AssertionError( + "structured replay turn failed on all attempts" + ) from last_error + + assert isinstance(second.content, FavoritePrimeReport) + assert second.content.number == 13 + assert second.content.is_prime is True + return first, second + + +@pytest.mark.asyncio +@pytest.mark.requires_anthropic +@pytest.mark.parametrize( + "model_spec", + get_live_model_specs(provider="anthropic", feature="structured_output"), + ids=lambda spec: spec.id, +) +async def test_live_anthropic_tools_with_structured_output( + model_spec: LiveModelSpec, + monkeypatch: pytest.MonkeyPatch, +) -> None: + require_provider_key(model_spec) + backend, config = make_backend(model_spec) + create_calls = wrap_async_method(monkeypatch, backend._client.messages, "create") + + await run_tool_then_structured_flow(backend, config, AnthropicHistoryAdapter()) + + assert len(create_calls) >= 2 + for call in create_calls: + messages = call["kwargs"]["messages"] + assert messages[-1] != { + "role": "assistant", + "content": "{", + }, "the '{' prefill would suppress tool_use blocks" + assert "If not responding with a tool call" in str( + messages[-1] + ), "schema instruction should use the conditional wording with tools" + + +@pytest.mark.asyncio +@pytest.mark.requires_openai +@pytest.mark.parametrize("model_spec", _OPENAI_TOOL_SPECS, ids=lambda spec: spec.id) +async def test_live_openai_tools_with_structured_output( + model_spec: LiveModelSpec, + monkeypatch: pytest.MonkeyPatch, +) -> None: + require_provider_key(model_spec) + # gpt-5.4 rejects function tools combined with any explicit + # reasoning_effort other than 'none' on /v1/chat/completions, so leave + # the parameter unset and let the server default apply. + backend, config = make_backend(model_spec) + parse_calls = wrap_async_method( + monkeypatch, backend._client.chat.completions, "parse" + ) + create_calls = wrap_async_method( + monkeypatch, backend._client.chat.completions, "create" + ) + + await run_tool_then_structured_flow(backend, config, OpenAIHistoryAdapter()) + + # favorite_prime_tools() is deliberately non-strict, the exact shape + # parse() refuses with a 500. + assert not parse_calls, "tool-carrying structured requests must avoid parse()" + assert len(create_calls) >= 2 + for call in create_calls: + response_format = call["kwargs"]["response_format"] + assert response_format["type"] == "json_schema" + assert response_format["json_schema"]["name"] == "FavoritePrimeReport" + + +@pytest.mark.asyncio +@pytest.mark.requires_gemini +@pytest.mark.parametrize( + "model_spec", + get_live_model_specs(provider="gemini", feature="structured_output"), + ids=lambda spec: spec.id, +) +async def test_live_gemini_tools_with_structured_output( + model_spec: LiveModelSpec, + monkeypatch: pytest.MonkeyPatch, +) -> None: + require_provider_key(model_spec) + # No temperature pin: gemini-2.5-flash occasionally returns an empty + # candidate on the replay turn, and at temperature=0 the retry re-sends + # a deterministic request — default sampling gives retries a real chance. + backend, config = make_backend(model_spec) + generate_calls = wrap_async_method( + monkeypatch, + backend._client.aio.models, + "generate_content", + ) + + await run_tool_then_structured_flow(backend, config, GeminiHistoryAdapter()) + + assert len(generate_calls) >= 2 + for call in generate_calls: + gen_config = call["kwargs"]["config"] + assert ( + "response_schema" not in gen_config + ), "native response_schema + function calling is rejected pre-Gemini 3" + assert "response_mime_type" not in gen_config + contents = call["kwargs"]["contents"] + assert "matching this schema" in str( + contents[-1] + ), "schema instruction should be injected into the final turn" diff --git a/tests/llm/test_backends/test_anthropic.py b/tests/llm/test_backends/test_anthropic.py index c7a4bb75..13255ec7 100644 --- a/tests/llm/test_backends/test_anthropic.py +++ b/tests/llm/test_backends/test_anthropic.py @@ -1,4 +1,5 @@ from types import SimpleNamespace +from typing import Any from unittest.mock import AsyncMock, Mock import pytest @@ -245,3 +246,206 @@ async def test_anthropic_backend_ignores_thinking_effort() -> None: call = await_args.kwargs assert "thinking" not in call assert "reasoning_effort" not in call + + +def _make_client(content_blocks: list[Any]) -> Mock: + client = Mock() + client.messages.create = AsyncMock( + return_value=SimpleNamespace( + content=content_blocks, + usage=SimpleNamespace( + input_tokens=10, + output_tokens=5, + cache_creation_input_tokens=0, + cache_read_input_tokens=0, + ), + stop_reason="end_turn", + ) + ) + return client + + +SEARCH_TOOL = { + "name": "search", + "description": "Search for information", + "input_schema": { + "type": "object", + "properties": {"query": {"type": "string"}}, + }, +} + + +@pytest.mark.asyncio +async def test_anthropic_backend_no_prefill_when_tools_present() -> None: + """With tools + response_format, the '{' prefill must be skipped and the + schema instruction must be conditional, so tool_use blocks stay reachable.""" + client = _make_client([TextBlock(type="text", text='{"answer":"ok"}')]) + + backend = AnthropicBackend(client) + result = await backend.complete( + # claude-3-5 supports prefill, so only the tools guard prevents it here + model="claude-3-5-sonnet-latest", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[SEARCH_TOOL], + response_format=StructuredResponse, + ) + + assert isinstance(result.content, StructuredResponse) + call = client.messages.create.await_args.kwargs + assert call["messages"][-1]["role"] == "user" # no assistant '{' prefill + assert ( + "If not responding with a tool call, respond with valid JSON" + in call["messages"][0]["content"] + ) + + +@pytest.mark.asyncio +async def test_anthropic_backend_prefill_unchanged_without_tools() -> None: + """Tool-less structured calls keep the prefill + unconditional wording.""" + client = _make_client([TextBlock(type="text", text='"answer":"ok"}')]) + + backend = AnthropicBackend(client) + result = await backend.complete( + model="claude-3-5-sonnet-latest", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + response_format=StructuredResponse, + ) + + assert isinstance(result.content, StructuredResponse) + call = client.messages.create.await_args.kwargs + assert call["messages"][-1] == {"role": "assistant", "content": "{"} + instruction = call["messages"][0]["content"] + assert "\n\nRespond with valid JSON matching this schema:" in instruction + assert "If not responding with a tool call" not in instruction + + +@pytest.mark.asyncio +async def test_anthropic_backend_repairs_malformed_structured_output( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Structured output that fails JSON parsing falls back to + repair_response_model_json, whose result becomes the response content.""" + # After the '{' prefill is prepended this is still invalid JSON. + client = _make_client([TextBlock(type="text", text='"answer": not-json')]) + + repaired = StructuredResponse(answer="fixed") + repair_calls: list[tuple[str, type[BaseModel], str]] = [] + + def _fake_repair( + raw: str, response_format: type[BaseModel], model_name: str + ) -> StructuredResponse: + repair_calls.append((raw, response_format, model_name)) + return repaired + + monkeypatch.setattr( + "src.llm.backends.anthropic.repair_response_model_json", _fake_repair + ) + + backend = AnthropicBackend(client) + result = await backend.complete( + model="claude-3-5-sonnet-latest", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + response_format=StructuredResponse, + ) + + assert result.content is repaired + assert repair_calls == [ + ('{"answer": not-json', StructuredResponse, "claude-3-5-sonnet-latest") + ] + + +@pytest.mark.asyncio +async def test_anthropic_backend_skips_parsing_on_tool_call_turns( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A tool-call response with response_format set must not attempt JSON + parsing — the repair fallback raises on the empty text of tool-call + turns, which would fail every intermediate tool iteration.""" + client = _make_client( + [ + ToolUseBlock( + type="tool_use", + id="tool_1", + name="search", + input={"query": "honcho"}, + ) + ] + ) + + def _fail_repair(*_args: object, **_kwargs: object) -> None: + raise AssertionError("repair must not run for tool-call turns") + + monkeypatch.setattr( + "src.llm.backends.anthropic.repair_response_model_json", _fail_repair + ) + + backend = AnthropicBackend(client) + result = await backend.complete( + model="claude-3-5-sonnet-latest", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[SEARCH_TOOL], + response_format=StructuredResponse, + ) + + assert result.content == "" # raw (empty) text, not a parsed model + assert result.tool_calls[0].name == "search" + + +@pytest.mark.asyncio +async def test_anthropic_backend_stream_no_prefill_when_tools_present() -> None: + """The streaming path applies the same tools guard.""" + + class _FakeStream: + def __init__(self) -> None: + self._chunks: list[SimpleNamespace] = [ + SimpleNamespace( + type="content_block_delta", + delta=SimpleNamespace(text='{"answer":"ok"}'), + ) + ] + + async def __aenter__(self): + return self + + async def __aexit__(self, *_args: object) -> bool: + return False + + def __aiter__(self): + return self + + async def __anext__(self): + if self._chunks: + return self._chunks.pop(0) + raise StopAsyncIteration + + async def get_final_message(self): + return SimpleNamespace( + stop_reason="end_turn", usage=SimpleNamespace(output_tokens=5) + ) + + client = Mock() + client.messages.stream = Mock(return_value=_FakeStream()) + + backend = AnthropicBackend(client) + chunks = [ + chunk + async for chunk in backend.stream( + model="claude-3-5-sonnet-latest", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[SEARCH_TOOL], + response_format=StructuredResponse, + ) + ] + + assert chunks[0].content == '{"answer":"ok"}' + call = client.messages.stream.call_args.kwargs + assert call["messages"][-1]["role"] == "user" # no assistant '{' prefill + assert ( + "If not responding with a tool call, respond with valid JSON" + in call["messages"][0]["content"] + ) diff --git a/tests/llm/test_backends/test_gemini.py b/tests/llm/test_backends/test_gemini.py index ca4fce97..1e3933b4 100644 --- a/tests/llm/test_backends/test_gemini.py +++ b/tests/llm/test_backends/test_gemini.py @@ -502,3 +502,126 @@ def test_gemini_convert_tools_sanitizes_parameters_schema() -> None: params = converted[0]["function_declarations"][0]["parameters"] assert "additionalProperties" not in params assert "additionalProperties" not in params["properties"]["observations"]["items"] + + +class _GeminiStructured(BaseModel): + answer: str + + +GEMINI_AGENT_TOOL = { + "name": "search", + "description": "Search for information", + "input_schema": { + "type": "object", + "properties": {"query": {"type": "string"}}, + }, +} + + +def _gemini_response( + parts: list[SimpleNamespace], parsed: object = None +) -> SimpleNamespace: + return SimpleNamespace( + candidates=[ + SimpleNamespace( + finish_reason=SimpleNamespace(name="STOP"), + content=SimpleNamespace(parts=parts), + ) + ], + usage_metadata=SimpleNamespace( + prompt_token_count=12, + candidates_token_count=6, + ), + parsed=parsed, + ) + + +@pytest.mark.asyncio +async def test_gemini_backend_structured_with_tools_skips_native_schema() -> None: + """Native response_schema + function calling is Gemini-3-preview-only, so + with tools present the schema must be delivered as an instruction on the + final turn instead, and the answer parsed from raw text.""" + client = Mock() + client.aio.models.generate_content = AsyncMock( + return_value=_gemini_response([SimpleNamespace(text='{"answer":"ok"}')]) + ) + + backend = GeminiBackend(client) + result = await backend.complete( + model="gemini-2.5-flash", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[GEMINI_AGENT_TOOL], + response_format=_GeminiStructured, + ) + + assert isinstance(result.content, _GeminiStructured) + assert result.content.answer == "ok" + await_args = client.aio.models.generate_content.await_args + if await_args is None: + raise AssertionError("Expected Gemini generate_content call") + call = await_args.kwargs + assert "response_schema" not in call["config"] + assert "response_mime_type" not in call["config"] + assert "tools" in call["config"] + last_part_text = call["contents"][-1]["parts"][-1]["text"] + assert "If not responding with a tool call" in last_part_text + + +@pytest.mark.asyncio +async def test_gemini_backend_structured_tool_call_turn_not_parsed() -> None: + """A tool-call turn under tools + response_format must not attempt JSON + parsing (its text is empty and the repair fallback raises on that).""" + client = Mock() + client.aio.models.generate_content = AsyncMock( + return_value=_gemini_response( + [ + SimpleNamespace( + function_call=SimpleNamespace( + name="search", args={"query": "honcho"} + ), + thought_signature=None, + ) + ] + ) + ) + + backend = GeminiBackend(client) + result = await backend.complete( + model="gemini-2.5-flash", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[GEMINI_AGENT_TOOL], + response_format=_GeminiStructured, + ) + + assert result.content == "" # raw (empty) text, not a parsed model + assert result.tool_calls[0].name == "search" + assert result.tool_calls[0].input == {"query": "honcho"} + + +@pytest.mark.asyncio +async def test_gemini_backend_structured_without_tools_uses_native_schema() -> None: + """Tool-less structured calls keep native response_schema enforcement.""" + client = Mock() + client.aio.models.generate_content = AsyncMock( + return_value=_gemini_response( + [SimpleNamespace(text='{"answer":"ok"}')], + parsed={"answer": "ok"}, + ) + ) + + backend = GeminiBackend(client) + result = await backend.complete( + model="gemini-2.5-flash", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + response_format=_GeminiStructured, + ) + + assert isinstance(result.content, _GeminiStructured) + call = client.aio.models.generate_content.await_args.kwargs # pyright: ignore + assert call["config"]["response_schema"] is _GeminiStructured + assert call["config"]["response_mime_type"] == "application/json" + # No instruction injected on the tool-less path. + assert call["contents"][-1]["parts"][-1]["text"] == "Hello" diff --git a/tests/llm/test_backends/test_openai.py b/tests/llm/test_backends/test_openai.py index 0ff6988f..89572f25 100644 --- a/tests/llm/test_backends/test_openai.py +++ b/tests/llm/test_backends/test_openai.py @@ -947,3 +947,123 @@ async def test_stream_structured_output_json_object_mode() -> None: assert system_messages assert "JSON" in system_messages[0]["content"] assert "json" in system_messages[0]["content"] + + +AGENT_TOOL = { + "name": "search", + "description": "Search for information", + "input_schema": { + "type": "object", + "properties": {"query": {"type": "string"}}, + }, +} + + +@pytest.mark.asyncio +async def test_openai_backend_structured_with_tools_uses_create_not_parse() -> None: + """With tools + response_format, complete() must route through create() + with an explicit json_schema response_format: parse() raises client-side + on non-strict function tools, and our agent tools are deliberately + non-strict (see _convert_tools).""" + client = Mock() + client.chat.completions.create = AsyncMock( + return_value=_structured_create_return('{"answer":"ok"}') + ) + client.chat.completions.parse = AsyncMock( + side_effect=AssertionError("parse() must not be called with tools") + ) + + backend = OpenAIBackend(client) + result = await backend.complete( + model="gpt-5.4-mini", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[AGENT_TOOL], + response_format=_StructuredResponse, + ) + + assert isinstance(result.content, _StructuredResponse) + assert result.content.answer == "ok" + client.chat.completions.parse.assert_not_awaited() + call = _await_kwargs(client.chat.completions.create) + assert call["response_format"]["type"] == "json_schema" + assert ( + call["response_format"]["json_schema"]["schema"] + == _StructuredResponse.model_json_schema() + ) + # Tools stay non-strict; strictness was the whole reason to avoid parse(). + assert "strict" not in call["tools"][0]["function"] + + +@pytest.mark.asyncio +async def test_openai_backend_structured_with_tools_skips_parsing_tool_call_turn() -> ( + None +): + """A tool-call turn under tools + response_format must not attempt JSON + parsing (its content is empty and _parse_or_repair raises on that).""" + client = Mock() + client.chat.completions.create = AsyncMock( + return_value=SimpleNamespace( + choices=[ + SimpleNamespace( + finish_reason="tool_calls", + message=SimpleNamespace( + content=None, + tool_calls=[ + SimpleNamespace( + id="tool_1", + function=SimpleNamespace( + name="search", + arguments='{"query": "honcho"}', + ), + ) + ], + reasoning_details=[], + refusal=None, + ), + ) + ], + usage=SimpleNamespace( + prompt_tokens=10, + completion_tokens=5, + prompt_tokens_details=None, + ), + ) + ) + + backend = OpenAIBackend(client) + result = await backend.complete( + model="gpt-5.4-mini", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + tools=[AGENT_TOOL], + response_format=_StructuredResponse, + ) + + assert result.content == "" # raw empty text, not a parsed model + assert result.tool_calls[0].name == "search" + assert result.tool_calls[0].input == {"query": "honcho"} + + +@pytest.mark.asyncio +async def test_openai_backend_structured_without_tools_still_uses_parse() -> None: + """Tool-less structured calls (deriver, final synthesis) keep parse().""" + parsed = _StructuredResponse(answer="ok") + client = Mock() + client.chat.completions.parse = AsyncMock( + return_value=_structured_create_return('{"answer":"ok"}', parsed=parsed) + ) + client.chat.completions.create = AsyncMock( + side_effect=AssertionError("create() must not be called without tools") + ) + + backend = OpenAIBackend(client) + result = await backend.complete( + model="gpt-5.4-mini", + messages=[{"role": "user", "content": "Hello"}], + max_tokens=100, + response_format=_StructuredResponse, + ) + + assert result.content is parsed + client.chat.completions.create.assert_not_awaited() diff --git a/tests/routes/test_messages.py b/tests/routes/test_messages.py index e70c0f18..a0186adc 100644 --- a/tests/routes/test_messages.py +++ b/tests/routes/test_messages.py @@ -63,6 +63,7 @@ async def test_create_message_schedules_immediate_embed( with ( patch("src.config.settings.EMBED_MESSAGES", True), + patch.object(settings.EMBEDDING, "MAX_PENDING_EMBED_TASKS", 50), patch( "src.reconciler.embed_now.embed_messages_now", new=AsyncMock() ) as mock_embed_now, @@ -147,6 +148,7 @@ async def test_file_upload_schedules_immediate_embed( with ( patch("src.config.settings.EMBED_MESSAGES", True), + patch.object(settings.EMBEDDING, "MAX_PENDING_EMBED_TASKS", 50), patch( "src.reconciler.embed_now.embed_messages_now", new=AsyncMock() ) as mock_embed_now,