diff --git a/.github/workflows/release-pipeline.yml b/.github/workflows/release-pipeline.yml index b5e0a0a55de..7d2e8d8fdb2 100644 --- a/.github/workflows/release-pipeline.yml +++ b/.github/workflows/release-pipeline.yml @@ -315,6 +315,7 @@ jobs: path: | tmp/newman-report*.json tmp/newman-cli*.log + tmp/provider-harness-report.html tmp/harness-failures.md tmp/harness-token-parity.md tmp/stream-cancel-report.json @@ -341,6 +342,9 @@ jobs: chmod +x ./.github/workflows/scripts/configure-r2.sh ./.github/workflows/scripts/upload-test-reports-to-r2.sh ./.github/workflows/scripts/configure-r2.sh || { echo "::warning::R2 not configured; skipping report publish"; exit 0; } mkdir -p tmp/provider-harness-report + # index.html so the R2 key serves as a browsable page, matching the + # cli-harness reports the changelog links alongside it. + cp tmp/provider-harness-report.html tmp/provider-harness-report/index.html 2>/dev/null || true cp tmp/harness-failures.md tmp/harness-token-parity.md tmp/provider-harness-report/ 2>/dev/null || true ./.github/workflows/scripts/upload-test-reports-to-r2.sh \ "${{ needs.detect-changes.outputs.transport-version }}" \ @@ -486,14 +490,14 @@ jobs: include: ${{ fromJSON(needs.resolve-cli-versions.outputs.matrix) }} name: test-cli-harness (${{ matrix.label }}) runs-on: ubuntu-latest - # The four run_cases invocations in test-cli-harness.sh each pass - # TIMEOUT=25m, so the Go suites alone can take 100m on top of the CLI + # The five run_cases invocations in test-cli-harness.sh each pass + # TIMEOUT=25m, so the Go suites alone can take 125m on top of the CLI # installs, the UI build and the gateway build. A job timeout cancels the # run, and the report upload is `if: !cancelled()`, so a cancel loses the # harness artifacts too. Raised twice: 90 -> 120 when the opencode suite was # added and the core scenarios went multi-turn, 120 -> 150 for the - # opencode-responses suite. - timeout-minutes: 150 + # opencode-responses suite, 150 -> 180 for opencode-anthropic. + timeout-minutes: 180 permissions: contents: read steps: @@ -615,6 +619,7 @@ jobs: tmp/cli-harness-codex.log tmp/cli-harness-opencode.log tmp/cli-harness-opencode-responses.log + tmp/cli-harness-opencode-anthropic.log # Per-attempt snapshots, written when a suite needed retries. The # reports above are overwritten in place by each rerun, so this is # the only record of what the first attempt failed on. diff --git a/.github/workflows/scripts/detect-all-changes.sh b/.github/workflows/scripts/detect-all-changes.sh index d71306bf51b..0227d942ff7 100755 --- a/.github/workflows/scripts/detect-all-changes.sh +++ b/.github/workflows/scripts/detect-all-changes.sh @@ -436,6 +436,25 @@ echo " Bifrost HTTP: $BIFROST_HTTP_NEEDS_RELEASE (v$TRANSPORT_VERSION)" echo " Docker: $DOCKER_NEEDS_RELEASE (v$TRANSPORT_VERSION)" # Set outputs (only when running in GitHub Actions) +# Version strings are read straight out of the repo's version files and are then +# interpolated by callers into shell commands (the R2 upload steps, the changelog +# push, the docker manifest). A value carrying a quote, a space or a newline would +# break out of the generated command, and GITHUB_OUTPUT is line-oriented so a +# newline would additionally forge an extra output line. Nothing downstream can +# defend against that once it is written, so it is rejected here, at the single +# point where these values enter the workflow. +for _pair in "core:$CORE_VERSION" "framework:$FRAMEWORK_VERSION" "transport:$TRANSPORT_VERSION"; do + _name="${_pair%%:*}" + _value="${_pair#*:}" + case "$_value" in + "") continue ;; + *[!0-9A-Za-z.+_-]*) + echo "โŒ refusing to emit $_name version with unexpected characters: $_value" >&2 + exit 1 + ;; + esac +done + if [ -n "${GITHUB_OUTPUT:-}" ]; then { echo "core-needs-release=$CORE_NEEDS_RELEASE" diff --git a/.github/workflows/scripts/push-mintlify-changelog.sh b/.github/workflows/scripts/push-mintlify-changelog.sh index 3b103c6d717..6575e1873ac 100755 --- a/.github/workflows/scripts/push-mintlify-changelog.sh +++ b/.github/workflows/scripts/push-mintlify-changelog.sh @@ -136,7 +136,7 @@ $CLI_HARNESS_INTRO | Suite | What it covers | Report | | --- | --- | --- | -| Provider harness | Every provider ร— modality through a live gateway | [failure breakdown]($REPORTS_BASE/provider-harness/harness-failures.md) | +| Provider harness | Every provider ร— modality through a live gateway | [requests]($REPORTS_BASE/provider-harness/index.html) ยท [failure breakdown]($REPORTS_BASE/provider-harness/harness-failures.md) | $CLI_HARNESS_ROWS The CLI harness reports include the full conversation for every scenario - each @@ -201,18 +201,18 @@ if ! grep -q "\"$route\"" docs/docs.json; then node -e " const fs = require('fs'); const docs = JSON.parse(fs.readFileSync('docs/docs.json', 'utf8')); - + // Semantic version comparison function // Extracts version from route/filename and compares in descending order (newest first) function compareVersionsDesc(a, b) { // Extract route string from string or object const routeA = typeof a === 'string' ? a : ''; const routeB = typeof b === 'string' ? b : ''; - + // Extract version from route (e.g., 'changelogs/v1.3.34' -> 'v1.3.34') const versionA = routeA.split('/').pop() || ''; const versionB = routeB.split('/').pop() || ''; - + // Remove 'v' prefix and split into parts const partsA = versionA.replace(/^v/, '').split(/[.-]/).map(p => { const num = parseInt(p, 10); @@ -222,7 +222,7 @@ if ! grep -q "\"$route\"" docs/docs.json; then const num = parseInt(p, 10); return isNaN(num) ? p : num; }); - + // Compare each part (major, minor, patch, pre-release, etc.) const maxLength = Math.max(partsA.length, partsB.length); for (let i = 0; i < maxLength; i++) { @@ -233,10 +233,10 @@ if ! grep -q "\"$route\"" docs/docs.json; then if (partsB[i] === undefined && partsA[i] !== undefined) { return 1; // B (release) comes first in descending order } - + const partA = partsA[i]; const partB = partsB[i]; - + // If both are numbers, compare numerically if (typeof partA === 'number' && typeof partB === 'number') { if (partA !== partB) { @@ -248,7 +248,7 @@ if ! grep -q "\"$route\"" docs/docs.json; then const strB = String(partB); const matchA = strA.match(/^([a-zA-Z]+)(\\d+)$/); const matchB = strB.match(/^([a-zA-Z]+)(\\d+)$/); - + if (matchA && matchB && matchA[1] === matchB[1]) { // Same prefix, compare numbers numerically const numA = parseInt(matchA[2], 10); @@ -261,45 +261,45 @@ if ! grep -q "\"$route\"" docs/docs.json; then } } } - + return 0; // Equal } - + // Sort a pages array by semver (descending) function sortPagesBySemver(pages) { return pages.slice().sort(compareVersionsDesc); } - + // Get current month/year const releaseDate = new Date('$CURRENT_DATE'); const currentDate = new Date(); const releaseMonthYear = releaseDate.toLocaleDateString('en-US', { year: 'numeric', month: 'long' }); const currentMonthYear = currentDate.toLocaleDateString('en-US', { year: 'numeric', month: 'long' }); - + // Find the Changelogs tab const changelogsTab = docs.navigation.tabs.find(tab => tab.tab === 'Changelogs'); if (!changelogsTab) { console.error('Changelogs tab not found'); process.exit(1); } - + // Find the Open Source menu item const openSourceItem = changelogsTab.menu?.find(item => item.item === 'Open Source'); if (!openSourceItem) { console.error('Open Source menu item not found in Changelogs tab'); process.exit(1); } - + // Get all top-level entries and existing groups const topLevelEntries = openSourceItem.pages.filter(p => typeof p === 'string'); const existingGroups = openSourceItem.pages.filter(p => typeof p === 'object'); - + // Check if we need to group existing top-level entries if (topLevelEntries.length > 0) { // Get the month of the first top-level entry (they should all be from same month) const firstEntryPath = topLevelEntries[0].replace('changelogs/', '') + '.mdx'; const firstEntryFile = 'docs/changelogs/' + firstEntryPath; - + let topLevelMonth = null; try { const content = fs.readFileSync(firstEntryFile, 'utf8'); @@ -311,22 +311,22 @@ if ! grep -q "\"$route\"" docs/docs.json; then } catch (e) { console.log(\`Warning: Could not read entry file \${firstEntryFile}: \${e.message}\`); } - + // Only group if the month has changed if (topLevelMonth && topLevelMonth !== releaseMonthYear) { console.log(\`๐Ÿ“ฆ Month changed from \${topLevelMonth} to \${releaseMonthYear}\`); console.log(\`๐Ÿ“ฆ Grouping \${topLevelEntries.length} top-level entries into \${topLevelMonth} group...\`); - + // Create a group for all existing top-level entries const previousMonthGroup = { group: topLevelMonth, pages: sortPagesBySemver(topLevelEntries) }; - + // Add this group at the top of existing groups existingGroups.unshift(previousMonthGroup); console.log(\`โœ… Created \${topLevelMonth} group with \${topLevelEntries.length} entries (sorted)\`); - + // Clear top-level entries (they're now in the group) openSourceItem.pages = existingGroups; } else { @@ -335,30 +335,30 @@ if ! grep -q "\"$route\"" docs/docs.json; then openSourceItem.pages = [...topLevelEntries, ...existingGroups]; } } - + const newRoute = '$route'; - + // Add the new changelog at the top level openSourceItem.pages.unshift(newRoute); console.log(\`โœ… Added \${newRoute} to top level\`); - + // Sort the top-level pages array by semver const topLevelPages = openSourceItem.pages.filter(p => typeof p === 'string'); const groupPages = openSourceItem.pages.filter(p => typeof p === 'object'); - + if (topLevelPages.length > 0) { const sortedTopLevel = sortPagesBySemver(topLevelPages); openSourceItem.pages = [...sortedTopLevel, ...groupPages]; console.log(\`โœ… Sorted \${topLevelPages.length} top-level pages by semver\`); } - + // Sort each group's pages by semver for (const group of groupPages) { if (group.pages && Array.isArray(group.pages)) { group.pages = sortPagesBySemver(group.pages); } } - + fs.writeFileSync('docs/docs.json', JSON.stringify(docs, null, 2) + '\n'); console.log('โœ… Updated docs.json'); " diff --git a/.github/workflows/scripts/test-cli-harness.sh b/.github/workflows/scripts/test-cli-harness.sh index d50039653e5..33e04bb9e8e 100755 --- a/.github/workflows/scripts/test-cli-harness.sh +++ b/.github/workflows/scripts/test-cli-harness.sh @@ -103,6 +103,22 @@ OPENCODE_CASES="${OPENCODE_CASES:-TestCLIs/opencode/(openai|azure)/gpt-5\.5/(sim # suite that costs Bedrock quota. OPENCODE_RESPONSES_CASES="${OPENCODE_RESPONSES_CASES:-TestCLIs/opencode-responses/bedrock/global\.anthropic\.claude-sonnet-5/(simple-chat|reasoning-replay)}" +# opencode-anthropic covers the THIRD wire format: /anthropic/v1/messages. +# +# It is the only client here that replays reasoning, and that is a protocol +# constraint rather than a client preference -- the Anthropic API requires an +# assistant turn's thinking blocks to be sent back verbatim when that turn +# contains tool_use. The Responses SDK is free to rebuild history as prose and +# does exactly that, which is why no amount of turns or tools makes the +# opencode-responses suite exercise reasoning replay. +# +# reasoning-tool-replay is therefore the case that matters: it is the only cell +# in the whole harness that reproduces the reported Bedrock 400 +# ("messages.2 ... reasoningContent.reasoningText.text ... Member must not be +# null"). simple-chat is its control, and running both providers gives the +# Anthropic wire coverage on the native path as well as the Bedrock one. +OPENCODE_ANTHROPIC_CASES="${OPENCODE_ANTHROPIC_CASES:-TestCLIs/opencode-anthropic/(anthropic|bedrock)/(claude-sonnet-5|global\.anthropic\.claude-sonnet-5)/(simple-chat|reasoning-tool-replay)}" + # Retries per suite, re-running ONLY the cells that failed. Mirrors test-core's # RERUN_FAILED policy (see test-provider-harness.sh): this suite is a required # release gate, so a flaky cell must not sink a release on its own, while a real @@ -350,12 +366,14 @@ CLAUDE_RC=0 CODEX_RC=0 OPENCODE_RC=0 OPENCODE_RESPONSES_RC=0 -# All four run even if an earlier one fails, so one broken CLI does not mask +OPENCODE_ANTHROPIC_RC=0 +# All five run even if an earlier one fails, so one broken CLI does not mask # the others. run_cases "claude" "$CLAUDE_CASES" || CLAUDE_RC=$? run_cases "codex" "$CODEX_CASES" || CODEX_RC=$? run_cases "opencode" "$OPENCODE_CASES" || OPENCODE_RC=$? run_cases "opencode-responses" "$OPENCODE_RESPONSES_CASES" || OPENCODE_RESPONSES_RC=$? +run_cases "opencode-anthropic" "$OPENCODE_ANTHROPIC_CASES" || OPENCODE_ANTHROPIC_RC=$? # Renders tests/e2e/clis/reports/index.html from the reports/*.json just written. # Free and instant - no test re-execution. @@ -379,6 +397,7 @@ if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then echo "| codex | \`$CODEX_VERSION\` | \`$CODEX_CASES\` | $(attempts_for codex) | $([ "$CODEX_RC" -eq 0 ] && echo "โœ… pass" || echo "โŒ fail") |" echo "| opencode | \`$OPENCODE_VERSION\` | \`$OPENCODE_CASES\` | $(attempts_for opencode) | $([ "$OPENCODE_RC" -eq 0 ] && echo "โœ… pass" || echo "โŒ fail") |" echo "| opencode-responses | \`$OPENCODE_VERSION\` | \`$OPENCODE_RESPONSES_CASES\` | $(attempts_for opencode-responses) | $([ "$OPENCODE_RESPONSES_RC" -eq 0 ] && echo "โœ… pass" || echo "โŒ fail") |" + echo "| opencode-anthropic | \`$OPENCODE_VERSION\` | \`$OPENCODE_ANTHROPIC_CASES\` | $(attempts_for opencode-anthropic) | $([ "$OPENCODE_ANTHROPIC_RC" -eq 0 ] && echo "โœ… pass" || echo "โŒ fail") |" echo "" echo "Attempts > 1 means failed cells were re-run; only cells with status \`fail\` are retried." echo "Full per-cell results are in the \`cli-harness-reports${CLI_VERSION_LABEL:+-$CLI_VERSION_LABEL}\` artifact (\`index.html\`)," @@ -391,8 +410,8 @@ if [ ! -d "$REPORTS_DIR" ]; then echo "โš ๏ธ No reports directory at $REPORTS_DIR - the harness may not have run any cells" fi -if [ "$CLAUDE_RC" -ne 0 ] || [ "$CODEX_RC" -ne 0 ] || [ "$OPENCODE_RC" -ne 0 ] || [ "$OPENCODE_RESPONSES_RC" -ne 0 ]; then - echo "โŒ CLI harness failed (claude=$CLAUDE_RC, codex=$CODEX_RC, opencode=$OPENCODE_RC, opencode-responses=$OPENCODE_RESPONSES_RC)" +if [ "$CLAUDE_RC" -ne 0 ] || [ "$CODEX_RC" -ne 0 ] || [ "$OPENCODE_RC" -ne 0 ] || [ "$OPENCODE_RESPONSES_RC" -ne 0 ] || [ "$OPENCODE_ANTHROPIC_RC" -ne 0 ]; then + echo "โŒ CLI harness failed (claude=$CLAUDE_RC, codex=$CODEX_RC, opencode=$OPENCODE_RC, opencode-responses=$OPENCODE_RESPONSES_RC, opencode-anthropic=$OPENCODE_ANTHROPIC_RC)" exit 1 fi diff --git a/.github/workflows/scripts/test-provider-harness.sh b/.github/workflows/scripts/test-provider-harness.sh index 4e62cd398ef..5398dc46a89 100755 --- a/.github/workflows/scripts/test-provider-harness.sh +++ b/.github/workflows/scripts/test-provider-harness.sh @@ -179,6 +179,48 @@ while [ "$HARNESS_EXIT" -ne 0 ] && [ "$attempt" -le "$RERUN_ATTEMPTS" ]; do attempt=$((attempt + 1)) done +# Render the browsable HTML report. +# +# newman's own htmlextra reporter only emits one in sequential mode, and this +# runs PARALLEL=1 (one fork per provider, no way to merge N HTML documents) - so +# without this, the provider harness ships no HTML at all while the CLI harness +# does. harness-viewer.mjs already renders exactly this JSON for local use, so +# --static reuses its markup rather than growing a second renderer that can +# drift from it. +# +# Best-effort: a rendering failure must not fail a passing test run. +echo "" +echo "๐Ÿ“Š Rendering provider harness HTML report..." +if node "$REPO_ROOT/tests/e2e/api/runners/harness-viewer.mjs" \ + --report "$REPO_ROOT/tmp/newman-report.json" \ + --failures-md "$REPO_ROOT/tmp/harness-failures.md" \ + --token-parity-md "$REPO_ROOT/tmp/harness-token-parity.md" \ + --static "$REPO_ROOT/tmp/provider-harness-report.html"; then + : +else + echo "โš ๏ธ HTML report rendering failed; the JSON and markdown artifacts are still intact" + # Rendering is best effort, but the LINK to it is not: the release changelog + # points at provider-harness/index.html unconditionally, and the upload step + # publishes whatever is in this directory. Without a file here that link 404s + # in public release notes. A stub that points at the markdown siblings keeps + # the notes honest about what happened and still reaches the real data. + cat > "$REPO_ROOT/tmp/provider-harness-report.html" <<'FALLBACK_HTML' + + +Bifrost Provider Harness Report + +

Provider harness report unavailable

+

The harness ran and its results were collected, but rendering this HTML view +failed. The underlying data is unaffected and published alongside this page:

+ +

The full newman-report.json is attached to the release workflow run as an artifact.

+FALLBACK_HTML +fi + if [ -n "${GITHUB_STEP_SUMMARY:-}" ] && [ -f "$REPO_ROOT/tmp/harness-failures.md" ]; then { echo "## Provider harness failure breakdown" diff --git a/core/providers/anthropic/reasoningreplay_test.go b/core/providers/anthropic/reasoningreplay_test.go new file mode 100644 index 00000000000..597b76c17d6 --- /dev/null +++ b/core/providers/anthropic/reasoningreplay_test.go @@ -0,0 +1,131 @@ +package anthropic + +import ( + "testing" + + "github.com/maximhq/bifrost/core/schemas" + "github.com/stretchr/testify/require" +) + +// convertBifrostReasoningToAnthropicThinking's INNER branches are already +// hardened against this defect class -- they gate on len(Summary) > 0 rather +// than nil, and the summary and encrypted arms are deliberately independent ifs +// so a reasoning item carrying both survives whole. The comments there spell +// that reasoning out. +// +// The outer `else if` defeated it. `msg.Content != nil && msg.Content.ContentBlocks +// != nil` is true for a NON-NIL BUT EMPTY slice, so such a message took the +// content-block branch, emitted nothing, and never reached the hardened code +// below. Same shape as the Bedrock and Cohere defects: data dropped silently, +// no error, the model simply loses its prior reasoning. +func TestConvertBifrostReasoningToAnthropicThinkingFallsThrough(t *testing.T) { + const encrypted = "EqQBCgIYAhIM...fixture" + ctx := &schemas.BifrostContext{} + + tests := []struct { + name string + msg *schemas.ResponsesMessage + wantThinking []string // Thinking texts expected, in order + wantRedacted []string // redacted_thinking Data expected + wantTotalCount int + }{ + { + // The regression: empty-but-non-nil content blocks must not shadow + // the encrypted payload sitting in ResponsesReasoning. + name: "empty content blocks does not shadow encrypted content", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{}}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(encrypted), + }, + }, + wantRedacted: []string{encrypted}, + wantTotalCount: 1, + }, + { + // Content blocks present but none of them reasoning: the summary is + // the only reasoning available and must not be lost. + name: "content blocks without reasoning falls back to summary", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + {Type: schemas.ResponsesOutputMessageContentTypeText, Text: schemas.Ptr("not reasoning")}, + }}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "visible reasoning"}}, + }, + }, + wantThinking: []string{"visible reasoning"}, + wantTotalCount: 1, + }, + { + // Already correct today: the inner independent-ifs keep both halves. + name: "summary and encrypted content both survive", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "visible reasoning"}}, + EncryptedContent: schemas.Ptr(encrypted), + }, + }, + wantThinking: []string{"visible reasoning"}, + wantRedacted: []string{encrypted}, + wantTotalCount: 2, + }, + { + // Already correct today: real content blocks take precedence and the + // fallback must NOT also fire, or the reasoning would be duplicated. + name: "content blocks with reasoning do not also emit the fallback", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + {Type: schemas.ResponsesOutputMessageContentTypeReasoning, Text: schemas.Ptr("step by step")}, + }}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "should not appear"}}, + EncryptedContent: schemas.Ptr(encrypted), + }, + }, + wantThinking: []string{"step by step"}, + wantTotalCount: 1, + }, + { + name: "no reasoning at all", + msg: &schemas.ResponsesMessage{Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning)}, + wantTotalCount: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + blocks := convertBifrostReasoningToAnthropicThinking(ctx, tc.msg, schemas.OpenAI, "gpt-5.5") + require.Len(t, blocks, tc.wantTotalCount) + + var thinking, redacted []string + for i, block := range blocks { + switch block.Type { + case AnthropicContentBlockTypeThinking: + require.NotNil(t, block.Thinking, "block %d: thinking block with nil text", i) + // The Agent SDK requires the signature field to be present on + // every thinking block, even when there is nothing to embed. + require.NotNil(t, block.Signature, "block %d: thinking block with nil signature", i) + thinking = append(thinking, *block.Thinking) + case AnthropicContentBlockTypeRedactedThinking: + require.NotNil(t, block.Data, "block %d: redacted block with nil data", i) + redacted = append(redacted, *block.Data) + } + } + require.Equal(t, tc.wantThinking, nilIfEmpty(thinking), "thinking blocks") + require.Equal(t, tc.wantRedacted, nilIfEmpty(redacted), "redacted_thinking blocks") + }) + } +} + +func nilIfEmpty(s []string) []string { + if len(s) == 0 { + return nil + } + return s +} diff --git a/core/providers/anthropic/responses.go b/core/providers/anthropic/responses.go index b1d3d44fc7a..3c5a75f06c8 100644 --- a/core/providers/anthropic/responses.go +++ b/core/providers/anthropic/responses.go @@ -6212,7 +6212,13 @@ func convertBifrostReasoningToAnthropicThinking(ctx *schemas.BifrostContext, msg var thinkingBlocks []AnthropicContentBlock embedID := providerUtils.ShouldEmbedReasoningItemID(ctx, sourceProvider, model) - if msg.Content != nil && msg.Content.ContentBlocks != nil { + // Track whether the content blocks actually yielded reasoning rather than + // branching on Content being non-nil. `ContentBlocks != nil` is true for an + // EMPTY but non-nil slice -- and for one holding only non-reasoning blocks -- + // so an else-if here shadowed the carefully hardened fallback below and + // dropped the replayed reasoning without a word. + emittedFromContentBlocks := false + if msg.Content != nil { for _, block := range msg.Content.ContentBlocks { if block.Type == schemas.ResponsesOutputMessageContentTypeReasoning && block.Text != nil { // signature is required by the Agent SDK; converted (non-Anthropic) reasoning @@ -6231,9 +6237,11 @@ func convertBifrostReasoningToAnthropicThinking(ctx *schemas.BifrostContext, msg Signature: signature, } thinkingBlocks = append(thinkingBlocks, thinkingBlock) + emittedFromContentBlocks = true } } - } else if msg.ResponsesReasoning != nil { + } + if !emittedFromContentBlocks && msg.ResponsesReasoning != nil { // Redacted-only reasoning items carry an EMPTY (non-nil) summary list next // to encrypted_content, in both the streaming and non-streaming converters, // so gate on the list having entries rather than on nil: a nil-check sends diff --git a/core/providers/bedrock/reasoning_replay_test.go b/core/providers/bedrock/reasoning_replay_test.go index 0f3b13902f2..72f90acaba9 100644 --- a/core/providers/bedrock/reasoning_replay_test.go +++ b/core/providers/bedrock/reasoning_replay_test.go @@ -1,17 +1,217 @@ package bedrock import ( + "context" "testing" + "github.com/bytedance/sonic" "github.com/maximhq/bifrost/core/schemas" "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" ) -// TestConvertBifrostReasoningToBedrockReasoningEncryptedContent verifies that -// encrypted reasoning replay data survives cross-provider translation into the -// Bedrock reasoning signature field. Responses reasoning items use an empty, -// non-nil Summary alongside EncryptedContent, so the converter must not treat -// the empty slice as visible reasoning and silently drop the replay payload. +// Bedrock Converse requires reasoningContent.reasoningText.text on every reasoning +// block. Replaying reasoning history to Anthropic-on-Bedrock over the Responses +// path returns, once per prior assistant turn: +// +// N validation errors detected: Value at 'messages.2.member.content.1.member. +// reasoningContent.reasoningText.text' failed to satisfy constraint: +// Member must not be null +// +// The tests below pin the invariant that prevents it. They are deliberately +// written against the serialised wire form as well as the struct, because +// BedrockReasoningContentText.Text is `*string json:"text,omitempty"` -- a nil +// Text does not serialise as `"text":null`, it vanishes from the request +// entirely, which is what made this defect invisible to every struct-level check. + +// reasoningTextInvariant asserts the one rule that matters: no block leaves this +// converter without a Text. Run over every case's output, not just the ones the +// case author thought to check. +func reasoningTextInvariant(t *testing.T, blocks []BedrockContentBlock) { + t.Helper() + for i, block := range blocks { + require.NotNil(t, block.ReasoningContent, "block %d: nil ReasoningContent", i) + require.NotNil(t, block.ReasoningContent.ReasoningText, "block %d: nil ReasoningText", i) + require.NotNil(t, block.ReasoningContent.ReasoningText.Text, + "block %d: nil Text -- Bedrock rejects this with \"reasoningContent.reasoningText.text ... Member must not be null\"", i) + } +} + +func reasoningTextBlock(text string, signature *string) schemas.ResponsesMessageContentBlock { + block := schemas.ResponsesMessageContentBlock{ + Type: schemas.ResponsesOutputMessageContentTypeReasoning, + Text: &text, + } + block.Signature = signature + return block +} + +func TestConvertBifrostReasoningToBedrockReasoning(t *testing.T) { + signature := "EqQBCgIYAhIM...fixture" + + tests := []struct { + name string + msg *schemas.ResponsesMessage + wantBlocks int + wantTexts []string + wantSignatures []*string + }{ + { + // The defect. This is exactly what the STREAMING ingress path emits + // (responses.go, output_item.added: Summary is a non-nil empty slice and + // EncryptedContent carries the Bedrock reasoning signature), so a client + // replaying what Bifrost itself streamed lands here. + name: "streaming shape: empty summary plus encrypted content", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: &signature, + }, + }, + wantBlocks: 1, + wantTexts: []string{""}, + wantSignatures: []*string{&signature}, + }, + { + // Non-nil but empty ContentBlocks must not shadow the encrypted payload. + // The `else if` on Content made this drop the item entirely; the sibling + // invoke path guards it with emittedFromContentBlocks (invoke.go). + name: "empty content blocks slice does not shadow encrypted content", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{}}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: &signature, + }, + }, + wantBlocks: 1, + wantTexts: []string{""}, + wantSignatures: []*string{&signature}, + }, + { + // Content blocks present but none of them reasoning: the summary is the + // only reasoning data available and must not be lost. + name: "content blocks without a reasoning block falls back to summary", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + {Type: schemas.ResponsesOutputMessageContentTypeText, Text: schemas.Ptr("not reasoning")}, + }}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "thinking about it"}}, + }, + }, + wantBlocks: 1, + wantTexts: []string{"thinking about it"}, + wantSignatures: []*string{nil}, + }, + { + // Branch A baseline -- the NON-streaming ingress shape. Bedrock returns + // reasoning here as content blocks with a per-block signature, which is + // why replaying a non-streamed turn has always worked. + name: "content block with text and signature", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + reasoningTextBlock("step by step", &signature), + }}, + }, + wantBlocks: 1, + wantTexts: []string{"step by step"}, + wantSignatures: []*string{&signature}, + }, + { + name: "content block with empty text keeps its signature", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + reasoningTextBlock("", &signature), + }}, + }, + wantBlocks: 1, + wantTexts: []string{""}, + wantSignatures: []*string{&signature}, + }, + { + name: "summary only", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "first"}, {Text: "second"}}, + }, + }, + wantBlocks: 2, + wantTexts: []string{"first", "second"}, + wantSignatures: []*string{nil, nil}, + }, + { + // An empty signature is not a signature. Echoing signature:"" back 400s + // with "This model doesn't support the ... signature field" (see + // reasoningSignatureForBedrock), so emitting nothing is correct. + name: "empty encrypted content string emits nothing", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(""), + }, + }, + wantBlocks: 0, + }, + { + name: "no reasoning and no content", + msg: &schemas.ResponsesMessage{Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning)}, + wantBlocks: 0, + }, + { + name: "multiple content blocks preserve order", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + reasoningTextBlock("one", nil), + reasoningTextBlock("two", &signature), + }}, + }, + wantBlocks: 2, + wantTexts: []string{"one", "two"}, + wantSignatures: []*string{nil, &signature}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + blocks := convertBifrostReasoningToBedrockReasoning(tc.msg) + + require.Len(t, blocks, tc.wantBlocks) + reasoningTextInvariant(t, blocks) + + for i := range tc.wantTexts { + require.Equal(t, tc.wantTexts[i], *blocks[i].ReasoningContent.ReasoningText.Text, + "block %d text", i) + } + for i := range tc.wantSignatures { + got := blocks[i].ReasoningContent.ReasoningText.Signature + if tc.wantSignatures[i] == nil { + require.Nil(t, got, "block %d expected no signature", i) + continue + } + require.NotNil(t, got, "block %d expected a signature", i) + require.Equal(t, *tc.wantSignatures[i], *got, "block %d signature", i) + } + }) + } +} + +// TestConvertBifrostReasoningToBedrockReasoningEncryptedContent keeps the original +// test's intent -- encrypted replay data must survive translation into the Bedrock +// signature field -- and corrects the shape it pinned. +// +// It previously asserted Text was nil. That is the state Bedrock rejects: `text` is +// omitempty, so a nil pointer means the key is absent from the serialised request, +// and Converse answers "reasoningContent.reasoningText.text ... Member must not be +// null". Preserving the signature was right; dropping the text was not. func TestConvertBifrostReasoningToBedrockReasoningEncryptedContent(t *testing.T) { encryptedContent := "EqQBCgIYAhIM...fixture" message := &schemas.ResponsesMessage{ @@ -26,6 +226,99 @@ func TestConvertBifrostReasoningToBedrockReasoningEncryptedContent(t *testing.T) require.Len(t, blocks, 1) require.NotNil(t, blocks[0].ReasoningContent) require.NotNil(t, blocks[0].ReasoningContent.ReasoningText) - require.Nil(t, blocks[0].ReasoningContent.ReasoningText.Text) + require.NotNil(t, blocks[0].ReasoningContent.ReasoningText.Text) require.Equal(t, encryptedContent, *blocks[0].ReasoningContent.ReasoningText.Signature) } + +// TestConvertBifrostReasoningToBedrockReasoningTextAlwaysSerialized is the +// assertion that maps one-to-one onto the upstream error message. +// +// A struct-level `Text != nil` check is not sufficient evidence here: omitempty is +// the mechanism that made this defect invisible. Only marshalling proves the key +// reaches the wire. +func TestConvertBifrostReasoningToBedrockReasoningTextAlwaysSerialized(t *testing.T) { + signature := "EqQBCgIYAhIM...fixture" + blocks := convertBifrostReasoningToBedrockReasoning(&schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: &signature, + }, + }) + require.Len(t, blocks, 1) + + raw, err := sonic.Marshal(blocks[0]) + require.NoError(t, err) + + text := gjson.GetBytes(raw, "reasoningContent.reasoningText.text") + require.True(t, text.Exists(), + "reasoningContent.reasoningText.text missing from the serialised block; Bedrock rejects this. got: %s", raw) + require.Equal(t, gjson.String, text.Type, "text must serialise as a string, got: %s", raw) + require.True(t, gjson.GetBytes(raw, "reasoningContent.reasoningText.signature").Exists(), + "signature must survive alongside the text, got: %s", raw) +} + +// TestStreamingReasoningReplaySerialisesForBedrock drives the full request +// conversion, not just the reasoning helper, so it fails the same way a live +// request does. +// +// The input is the assistant turn a STREAMING client replays: Bifrost's streaming +// ingress emits a reasoning item with an empty summary and the signature in +// encrypted_content, and a faithful client sends that straight back on the next +// turn. Every prior assistant turn contributes one block, which is why the live +// error count scales 1:1 with conversation depth -- so this asserts across three +// replayed turns rather than one. +func TestStreamingReasoningReplaySerialisesForBedrock(t *testing.T) { + signature := "EqQBCgIYAhIM...fixture" + + var input []schemas.ResponsesMessage + input = append(input, schemas.ResponsesMessage{ + Role: schemas.Ptr(schemas.ResponsesInputMessageRoleUser), + Content: &schemas.ResponsesMessageContent{ContentStr: schemas.Ptr("Start.")}, + }) + for i := 0; i < 3; i++ { + input = append(input, + schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Role: schemas.Ptr(schemas.ResponsesInputMessageRoleAssistant), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: &signature, + }, + }, + schemas.ResponsesMessage{ + Role: schemas.Ptr(schemas.ResponsesInputMessageRoleAssistant), + Content: &schemas.ResponsesMessageContent{ContentStr: schemas.Ptr("Answer.")}, + }, + schemas.ResponsesMessage{ + Role: schemas.Ptr(schemas.ResponsesInputMessageRoleUser), + Content: &schemas.ResponsesMessageContent{ContentStr: schemas.Ptr("Continue.")}, + }, + ) + } + + messages, _, err := ConvertBifrostMessagesToBedrockMessages(context.Background(), input, false) + require.NoError(t, err) + + raw, err := sonic.Marshal(messages) + require.NoError(t, err) + + reasoningBlocks := gjson.GetBytes(raw, `#.content.#(reasoningContent)#`) + require.True(t, reasoningBlocks.Exists(), "no reasoning survived the conversion: %s", raw) + + var seen int + gjson.GetBytes(raw, "#.content").ForEach(func(_, content gjson.Result) bool { + content.ForEach(func(_, block gjson.Result) bool { + reasoning := block.Get("reasoningContent.reasoningText") + if !reasoning.Exists() { + return true + } + seen++ + require.True(t, reasoning.Get("text").Exists(), + "replayed reasoning block %d serialised without a text key -- this is the live 400: %s", seen, reasoning.Raw) + return true + }) + return true + }) + require.Equal(t, 3, seen, "expected one reasoning block per replayed assistant turn, got %d: %s", seen, raw) +} diff --git a/core/providers/bedrock/reasoningreplayaudit_test.go b/core/providers/bedrock/reasoningreplayaudit_test.go new file mode 100644 index 00000000000..59bdfb64c7c --- /dev/null +++ b/core/providers/bedrock/reasoningreplayaudit_test.go @@ -0,0 +1,160 @@ +package bedrock + +import ( + "context" + "testing" + + "github.com/bytedance/sonic" + "github.com/maximhq/bifrost/core/schemas" + "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" +) + +// bedrockReasoningBlocksForChatMessage runs the real chat-completions converter +// and returns the reasoning blocks it produced. +func bedrockReasoningBlocksForChatMessage(t *testing.T, msg schemas.ChatMessage) []BedrockContentBlock { + t.Helper() + converted, err := convertMessage(context.Background(), msg) + require.NoError(t, err) + var blocks []BedrockContentBlock + for _, block := range converted.Content { + if block.ReasoningContent != nil { + blocks = append(blocks, block) + } + } + return blocks +} + +// invokeThinkingBlocksFor runs the real invoke converter and returns the +// thinking blocks it produced. +func invokeThinkingBlocksFor(t *testing.T, item *schemas.ResponsesMessage) []BedrockInvokeMessagesContentBlock { + t.Helper() + resp := &schemas.BifrostResponsesResponse{Output: []schemas.ResponsesMessage{*item}} + converted := toBedrockInvokeAnthropicResponse(resp, "global.anthropic.claude-sonnet-5") + require.NotNil(t, converted) + return converted.Content +} + +// An audit for the defect fixed in convertBifrostReasoningToBedrockReasoning +// found the same shape on Bedrock's OTHER two request paths. Same struct, same +// omitempty, same upstream rejection -- only the entry point differs, so fixing +// the Responses converter alone left two thirds of the surface broken: +// +// Responses convertBifrostReasoningToBedrockReasoning (fixed) +// ChatCompletion convertMessage -> utils.go (this file) +// Invoke toBedrockInvokeAnthropicResponse (this file) +// +// Bedrock requires the thinking text on every reasoning block. Because the +// fields are omitempty, omitting it does not send an explicit null -- the key +// vanishes and the request is rejected before a token is read. + +// TestChatReasoningDetailsAlwaysSerialiseText covers the chat-completions path. +// +// The trigger is Bifrost's own streaming ingress: on a signature delta it emits +// a ChatReasoningDetails with Signature set and Text nil (see chat.go's +// reasoningContentDelta handling). A client replaying that assistant turn sends +// it straight back, and utils.go copies detail.Text through unguarded -- so a +// nil Text becomes a reasoningText block with no text key at all. +func TestChatReasoningDetailsAlwaysSerialiseText(t *testing.T) { + signature := "EqQBCgIYAhIM...fixture" + + for _, tc := range []struct { + name string + detail schemas.ChatReasoningDetails + }{ + { + // Exactly what the streaming path emits for a signature-only delta. + name: "signature with nil text", + detail: schemas.ChatReasoningDetails{ + Index: 0, Type: schemas.BifrostReasoningDetailsTypeText, Signature: &signature, + }, + }, + { + name: "signature with empty text", + detail: schemas.ChatReasoningDetails{ + Index: 0, Type: schemas.BifrostReasoningDetailsTypeText, + Text: schemas.Ptr(""), Signature: &signature, + }, + }, + { + name: "text and signature", + detail: schemas.ChatReasoningDetails{ + Index: 0, Type: schemas.BifrostReasoningDetailsTypeText, + Text: schemas.Ptr("step by step"), Signature: &signature, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + msg := schemas.ChatMessage{ + Role: schemas.ChatMessageRoleAssistant, + ChatAssistantMessage: &schemas.ChatAssistantMessage{ + ReasoningDetails: []schemas.ChatReasoningDetails{tc.detail}, + }, + } + + blocks := bedrockReasoningBlocksForChatMessage(t, msg) + require.NotEmpty(t, blocks, "reasoning detail produced no Bedrock block at all") + + for i, block := range blocks { + if block.ReasoningContent == nil { + continue + } + require.NotNil(t, block.ReasoningContent.ReasoningText, "block %d", i) + require.NotNil(t, block.ReasoningContent.ReasoningText.Text, + "block %d: nil Text -- Bedrock rejects this with \"reasoningContent.reasoningText.text ... Member must not be null\"", i) + + raw, err := sonic.Marshal(block) + require.NoError(t, err) + require.True(t, gjson.GetBytes(raw, "reasoningContent.reasoningText.text").Exists(), + "text key missing from the serialised block: %s", raw) + } + }) + } +} + +// TestInvokeThinkingAlwaysSerialised covers the invoke path. +// +// BedrockInvokeMessagesContentBlock.Thinking is `string json:"thinking,omitempty"`, +// so an empty string is indistinguishable from an absent one on the wire. The +// converter deliberately allows an empty text with a real signature in order to +// preserve the replay token -- which produces {"type":"thinking","signature":...} +// with no thinking key. +// +// Bifrost's own re-ingest proves the shape is unusable: invoke.go's decoder +// returns nil when Thinking is absent, silently dropping the whole block. +func TestInvokeThinkingAlwaysSerialised(t *testing.T) { + signature := "EqQBCgIYAhIM...fixture" + + // Both fields set, which is how a reasoning item actually arrives: the + // converter gates the whole branch on ResponsesReasoning != nil and then + // reads the content blocks. + block := schemas.ResponsesMessageContentBlock{ + Type: schemas.ResponsesOutputMessageContentTypeReasoning, + Text: schemas.Ptr(""), + } + block.Signature = &signature + + blocks := invokeThinkingBlocksFor(t, &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{block}}, + ResponsesReasoning: &schemas.ResponsesReasoning{Summary: []schemas.ResponsesReasoningSummary{}}, + }) + + require.NotEmpty(t, blocks, "signature-bearing reasoning produced no thinking block") + inspected := 0 + for i, block := range blocks { + if block.Type != "thinking" { + continue + } + inspected++ + raw, err := sonic.Marshal(block) + require.NoError(t, err) + require.True(t, gjson.GetBytes(raw, "thinking").Exists(), + "block %d: thinking key missing, so re-ingest drops the block entirely: %s", i, raw) + } + // NotEmpty above only proves SOME block came back. If the converter stops + // emitting thinking blocks but still emits something else, every iteration + // takes the continue and the assertion never runs - so the test would report + // success for precisely the regression it exists to catch. + require.NotZero(t, inspected, "no thinking block was inspected, so nothing above was actually asserted") +} diff --git a/core/providers/bedrock/responses.go b/core/providers/bedrock/responses.go index 5288e23a9c2..48d89d5a9eb 100644 --- a/core/providers/bedrock/responses.go +++ b/core/providers/bedrock/responses.go @@ -4577,46 +4577,91 @@ func convertSingleBedrockMessageToBifrostMessages(ctx *schemas.BifrostContext, m return outputMessages } -// convertBifrostReasoningToBedrockReasoning converts a Bifrost reasoning message to Bedrock reasoning blocks +// convertBifrostReasoningToBedrockReasoning converts a Bifrost reasoning message to Bedrock reasoning blocks. +// +// Every block this emits MUST carry a non-nil Text. BedrockReasoningContentText.Text +// is `*string json:"text,omitempty"`, so a nil pointer does not serialise as +// `"text":null` -- the key disappears from the request entirely, and Bedrock Converse +// answers: +// +// N validation errors detected: Value at 'messages.2.member.content.1.member. +// reasoningContent.reasoningText.text' failed to satisfy constraint: Member must not be null +// +// once per replayed assistant turn. See reasoning_replay_test.go, which pins the +// invariant at both the struct and the serialised-wire level. func convertBifrostReasoningToBedrockReasoning(msg *schemas.ResponsesMessage) []BedrockContentBlock { var reasoningBlocks []BedrockContentBlock - if msg.Content != nil && msg.Content.ContentBlocks != nil { + // Track whether the content blocks actually produced reasoning rather than + // branching on Content being non-nil. A non-nil but empty ContentBlocks -- or + // one holding only non-reasoning blocks -- must fall through to + // ResponsesReasoning instead of shadowing it, otherwise the replayed reasoning + // is silently dropped. Mirrors toBedrockInvokeAnthropicResponse in invoke.go, + // which already guards this on the invoke path. + emittedFromContentBlocks := false + if msg.Content != nil { for _, block := range msg.Content.ContentBlocks { if block.Type == schemas.ResponsesOutputMessageContentTypeReasoning && block.Text != nil { - reasoningBlock := BedrockContentBlock{ + reasoningBlocks = append(reasoningBlocks, BedrockContentBlock{ ReasoningContent: &BedrockReasoningContent{ ReasoningText: &BedrockReasoningContentText{ Text: block.Text, Signature: reasoningSignatureForBedrock(block.Signature), }, }, - } - reasoningBlocks = append(reasoningBlocks, reasoningBlock) + }) + emittedFromContentBlocks = true } } - } else if msg.ResponsesReasoning != nil { - if len(msg.ResponsesReasoning.Summary) > 0 { - for _, reasoningContent := range msg.ResponsesReasoning.Summary { - reasoningBlock := BedrockContentBlock{ - ReasoningContent: &BedrockReasoningContent{ - ReasoningText: &BedrockReasoningContentText{ - Text: &reasoningContent.Text, - }, - }, - } - reasoningBlocks = append(reasoningBlocks, reasoningBlock) - } - } else if msg.ResponsesReasoning.EncryptedContent != nil && *msg.ResponsesReasoning.EncryptedContent != "" { - reasoningBlock := BedrockContentBlock{ + } + if emittedFromContentBlocks || msg.ResponsesReasoning == nil { + return reasoningBlocks + } + + // Routed through the helper rather than read directly, so the empty-string + // guard matches every other signature site (see reasoningSignatureForBedrock). + signature := reasoningSignatureForBedrock(msg.ResponsesReasoning.EncryptedContent) + + if len(msg.ResponsesReasoning.Summary) > 0 { + for i, reasoningContent := range msg.ResponsesReasoning.Summary { + text := reasoningContent.Text + block := BedrockContentBlock{ ReasoningContent: &BedrockReasoningContent{ - ReasoningText: &BedrockReasoningContentText{ - Signature: msg.ResponsesReasoning.EncryptedContent, - }, + ReasoningText: &BedrockReasoningContentText{Text: &text}, }, } - reasoningBlocks = append(reasoningBlocks, reasoningBlock) - } + // The signature goes on the first block only. Bedrock verifies it + // against that block's text, so repeating one signature across several + // summary entries would present it as signing text it never signed -- + // and dropping it entirely, as this branch used to, loses the replay + // token the next turn needs. + if i == 0 { + block.ReasoningContent.ReasoningText.Signature = signature + } + reasoningBlocks = append(reasoningBlocks, block) + } + return reasoningBlocks + } + + if signature != nil { + // A signature with no accompanying thinking text. This is what the + // streaming ingress path emits (an empty summary plus the signature in + // encrypted_content), so it is what a client faithfully replaying a + // streamed turn sends back. + // + // An empty Text is the only text available here -- the client never + // received the prose to replay -- but an ABSENT one is not an option: it + // is the difference between a request Bedrock evaluates and one it + // rejects outright before reading a token. + emptyText := "" + reasoningBlocks = append(reasoningBlocks, BedrockContentBlock{ + ReasoningContent: &BedrockReasoningContent{ + ReasoningText: &BedrockReasoningContentText{ + Text: &emptyText, + Signature: signature, + }, + }, + }) } return reasoningBlocks diff --git a/core/providers/bedrock/types.go b/core/providers/bedrock/types.go index f5548a22597..8827f7e259e 100644 --- a/core/providers/bedrock/types.go +++ b/core/providers/bedrock/types.go @@ -692,6 +692,10 @@ type BedrockInvokeMessagesResponse struct { } // BedrockInvokeMessagesContentBlock represents a content block in an Anthropic Messages response. +// +// One struct serves every block type, so each field carries omitempty to keep a +// text block from advertising empty tool fields and vice versa. See MarshalJSON +// for the one case where that default is wrong. type BedrockInvokeMessagesContentBlock struct { Type string `json:"type"` Text string `json:"text,omitempty"` @@ -702,6 +706,28 @@ type BedrockInvokeMessagesContentBlock struct { Signature string `json:"signature,omitempty"` } +// MarshalJSON forces the thinking key to be present on thinking blocks. +// +// A thinking block whose text is empty is a real state: a client replaying a +// streamed assistant turn has the reasoning signature but not the prose it +// signs. omitempty then deletes the key entirely, producing +// {"type":"thinking","signature":"..."} -- which Bifrost's own re-ingest treats +// as malformed and drops on the floor (invoke.go's decoder returns nil when +// thinking is absent), silently losing the replay token the next turn needs. +// +// omitempty stays on the field so text and tool_use blocks are unaffected; only +// thinking blocks are special-cased here. +func (b BedrockInvokeMessagesContentBlock) MarshalJSON() ([]byte, error) { + type alias BedrockInvokeMessagesContentBlock + if b.Type != "thinking" || b.Thinking != "" { + return sonic.Marshal(alias(b)) + } + return sonic.Marshal(struct { + alias + Thinking string `json:"thinking"` + }{alias: alias(b), Thinking: b.Thinking}) +} + // BedrockInvokeMessagesUsage represents token usage in an Anthropic Messages response. type BedrockInvokeMessagesUsage struct { InputTokens int `json:"input_tokens"` diff --git a/core/providers/bedrock/utils.go b/core/providers/bedrock/utils.go index 54c3c0d54bf..1ba5e6f321b 100644 --- a/core/providers/bedrock/utils.go +++ b/core/providers/bedrock/utils.go @@ -923,10 +923,26 @@ func convertMessage(ctx context.Context, msg schemas.ChatMessage) (BedrockMessag if msg.ChatAssistantMessage != nil && len(msg.ChatAssistantMessage.ReasoningDetails) > 0 { for _, detail := range msg.ChatAssistantMessage.ReasoningDetails { if detail.Type == schemas.BifrostReasoningDetailsTypeText { + // Text must never reach Bedrock as nil. It is + // `*string json:"text,omitempty"`, so a nil pointer drops the key + // from the request rather than sending an explicit null, and + // Converse rejects that with "reasoningContent.reasoningText.text + // ... Member must not be null". + // + // This is reachable from Bifrost's own output: the streaming + // ingress emits a reasoning detail carrying only a Signature on a + // signature delta, and a client replaying that assistant turn + // sends it straight back. Same defect as the Responses converter + // (convertBifrostReasoningToBedrockReasoning), different entry + // point. + text := detail.Text + if text == nil { + text = schemas.Ptr("") + } contentBlocks = append(contentBlocks, BedrockContentBlock{ ReasoningContent: &BedrockReasoningContent{ ReasoningText: &BedrockReasoningContentText{ - Text: detail.Text, + Text: text, Signature: reasoningSignatureForBedrock(detail.Signature), }, }, diff --git a/core/providers/cohere/reasoningreplay_test.go b/core/providers/cohere/reasoningreplay_test.go new file mode 100644 index 00000000000..a344fd3de01 --- /dev/null +++ b/core/providers/cohere/reasoningreplay_test.go @@ -0,0 +1,306 @@ +package cohere + +import ( + "strings" + "testing" + + "github.com/maximhq/bifrost/core/schemas" + "github.com/stretchr/testify/require" +) + +// Replayed reasoning must survive translation into Cohere thinking blocks. +// +// Two independent bugs made it vanish instead, both found by auditing the same +// defect class as the Bedrock reasoning-replay 400: +// +// 1. `if Summary != nil` is true for an EMPTY but non-nil slice, so the loop ran +// zero times and the encrypted-content fallback below it was unreachable -- +// dead code. Every construction site of schemas.ResponsesReasoning in this +// codebase sets `Summary: []schemas.ResponsesReasoningSummary{}`, and the +// field has no omitempty, so the empty-non-nil state survives a JSON round +// trip. That branch had never once executed. +// +// 2. The outer `else if` on Content meant a non-nil but empty ContentBlocks -- +// or one holding only non-reasoning blocks -- emitted nothing and never fell +// through to ResponsesReasoning. +// +// Both drop data silently: no error, no warning, the model simply loses its +// prior reasoning. Cohere has no encrypted-reasoning field, so the replay lands +// in a marked thinking block; losing it outright is strictly worse. +func TestConvertBifrostReasoningToCohereThinking(t *testing.T) { + const encrypted = "EqQBCgIYAhIM...fixture" + + tests := []struct { + name string + msg *schemas.ResponsesMessage + wantCount int + wantAll []string // substrings that must appear across the emitted blocks + }{ + { + // Bug 1. The shape every provider's ingress produces. + name: "empty summary plus encrypted content", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(encrypted), + }, + }, + wantCount: 1, + wantAll: []string{encrypted}, + }, + { + // Bug 2. Non-nil but empty content blocks must not shadow the fallback. + name: "empty content blocks does not shadow encrypted content", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{}}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(encrypted), + }, + }, + wantCount: 1, + wantAll: []string{encrypted}, + }, + { + // Bug 2 again, via content blocks that carry no reasoning. + name: "content blocks without reasoning falls back to summary", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + {Type: schemas.ResponsesOutputMessageContentTypeText, Text: schemas.Ptr("not reasoning")}, + }}, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "thinking about it"}}, + }, + }, + wantCount: 1, + wantAll: []string{"thinking about it"}, + }, + { + // A summary and an encrypted payload are independent facts; keeping + // only one of them loses replay data the next turn needs. + name: "summary and encrypted content both survive", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "visible reasoning"}}, + EncryptedContent: schemas.Ptr(encrypted), + }, + }, + wantCount: 2, + wantAll: []string{"visible reasoning", encrypted}, + }, + { + name: "content blocks with reasoning", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ContentBlocks: []schemas.ResponsesMessageContentBlock{ + {Type: schemas.ResponsesOutputMessageContentTypeReasoning, Text: schemas.Ptr("step by step")}, + }}, + }, + wantCount: 1, + wantAll: []string{"step by step"}, + }, + { + name: "summary only", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{{Text: "first"}, {Text: "second"}}, + }, + }, + wantCount: 2, + wantAll: []string{"first", "second"}, + }, + { + name: "empty encrypted content emits nothing", + msg: &schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(""), + }, + }, + wantCount: 0, + }, + { + name: "no reasoning at all", + msg: &schemas.ResponsesMessage{Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning)}, + wantCount: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + blocks := convertBifrostReasoningToCohereThinking(tc.msg) + require.Len(t, blocks, tc.wantCount) + + var combined strings.Builder + for i, block := range blocks { + require.Equal(t, CohereContentBlockTypeThinking, block.Type, "block %d", i) + require.NotNil(t, block.Thinking, "block %d: nil Thinking", i) + combined.WriteString(*block.Thinking) + combined.WriteString("\n") + } + for _, want := range tc.wantAll { + require.Contains(t, combined.String(), want, + "replayed reasoning was dropped: %q missing from the emitted thinking blocks", want) + } + }) + } +} + +// The encrypted replay token has to survive a FULL round trip, not just egress. +// +// Cohere has no encrypted-reasoning field, so egress smuggles the token out +// inside a marked thinking block. That is only half a round trip: on the way +// back in, every thinking block was mapped to ordinary reasoning text, so the +// marker reached clients as visible prose and was replayed as if the model had +// literally thought "[ENCRYPTED_REASONING: ...]". The token that was supposed +// to be restored to EncryptedContent was, from the provider's point of view, +// gone - and the replay it exists to enable could never happen. +func TestEncryptedReasoningSurvivesCohereRoundTrip(t *testing.T) { + const token = "ErcBCkYIBRgCKkDd2xLm...opaque" + + out := convertBifrostReasoningToCohereThinking(&schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(token), + }, + }) + require.NotEmpty(t, out, "egress dropped the encrypted reasoning entirely") + + // Feed exactly what egress produced back through ingress, which is what a + // client replaying the turn actually does. + back := convertSingleCohereMessageToBifrostMessages(&CohereMessage{ + Role: "assistant", + Content: &CohereMessageContent{BlocksContent: out}, + }, true) + require.NotEmpty(t, back, "ingress produced no messages") + + var reasoning *schemas.ResponsesMessage + for i := range back { + if back[i].Type != nil && *back[i].Type == schemas.ResponsesMessageTypeReasoning { + reasoning = &back[i] + break + } + } + require.NotNil(t, reasoning, "no reasoning message came back") + require.NotNil(t, reasoning.ResponsesReasoning, "reasoning message carries no ResponsesReasoning") + require.NotNil(t, reasoning.ResponsesReasoning.EncryptedContent, + "the replay token was not restored to EncryptedContent, so the next turn cannot replay it") + require.Equal(t, token, *reasoning.ResponsesReasoning.EncryptedContent) + + // And it must not ALSO surface as visible reasoning text: the marker is a + // transport detail, and leaking it to clients means replaying it as content. + if reasoning.Content != nil { + for _, b := range reasoning.Content.ContentBlocks { + if b.Text != nil { + require.NotContains(t, *b.Text, "ENCRYPTED_REASONING", + "the transport marker leaked into visible reasoning text") + } + } + } +} + +// The combined shape - content-block reasoning AND an encrypted token on the +// same message - is not hypothetical: it is exactly what this file's own ingress +// path now produces, since restoring EncryptedContent leaves the visible +// reasoning blocks in place. Returning early once the content blocks yielded +// reasoning drops the token on the very next turn, which is the turn the token +// exists for. The token-only round-trip test above stayed green throughout, +// because it never exercised both at once. +func TestEncryptedTokenSurvivesAlongsideContentBlockReasoning(t *testing.T) { + const token = "ErcBCkYIBRgCKkDd2xLm...combined" + + blocks := convertBifrostReasoningToCohereThinking(&schemas.ResponsesMessage{ + Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), + Content: &schemas.ResponsesMessageContent{ + ContentBlocks: []schemas.ResponsesMessageContentBlock{{ + Type: schemas.ResponsesOutputMessageContentTypeReasoning, + Text: schemas.Ptr("Thinking it through."), + }}, + }, + ResponsesReasoning: &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: schemas.Ptr(token), + }, + }) + + var sawText, sawMarker bool + for _, b := range blocks { + if b.Thinking == nil { + continue + } + if *b.Thinking == "Thinking it through." { + sawText = true + } + if enc, ok := parseEncryptedReasoning(*b.Thinking); ok && enc == token { + sawMarker = true + } + } + + require.True(t, sawText, "visible reasoning text was dropped") + require.True(t, sawMarker, + "the encrypted replay token was dropped because the content blocks already yielded reasoning") +} + +// The full replay path, not just one half of it. +// +// The combined-shape test above starts from a hand-built message and only proves egress emits +// both parts. What actually happens on turn two of a replay is: a Cohere response is INGESTED +// into a Bifrost message, and that message is then converted back out. Only the round trip shows +// whether the two halves agree - an ingress that restores the token into a field egress skips, or +// an egress that drops what ingress carefully preserved, both look correct in isolation. +func TestMixedReasoningSurvivesIngressThenEgress(t *testing.T) { + const token = "ErcBCkYIBRgCKkDd2xLm...mixed" + const visible = "Working through the constraints." + + // What Cohere sends back when the previous turn carried both: ordinary thinking plus the + // marked block Bifrost wrote on the way out. + marker := formatEncryptedReasoning(token) + visibleText := visible + incoming := []CohereContentBlock{ + {Type: CohereContentBlockTypeThinking, Thinking: &visibleText}, + {Type: CohereContentBlockTypeThinking, Thinking: &marker}, + } + + back := convertSingleCohereMessageToBifrostMessages(&CohereMessage{ + Role: "assistant", + Content: &CohereMessageContent{BlocksContent: incoming}, + }, true) + + var reasoning *schemas.ResponsesMessage + for i := range back { + if back[i].Type != nil && *back[i].Type == schemas.ResponsesMessageTypeReasoning { + reasoning = &back[i] + break + } + } + require.NotNil(t, reasoning, "ingress produced no reasoning message") + require.NotNil(t, reasoning.ResponsesReasoning, "ingress dropped ResponsesReasoning") + require.NotNil(t, reasoning.ResponsesReasoning.EncryptedContent, "ingress did not restore the replay token") + require.Equal(t, token, *reasoning.ResponsesReasoning.EncryptedContent) + + // Now send that exact message back out, which is what the next turn does. + out := convertBifrostReasoningToCohereThinking(reasoning) + + var sawVisible, sawMarker bool + for _, b := range out { + if b.Thinking == nil { + continue + } + if *b.Thinking == visible { + sawVisible = true + } + if enc, ok := parseEncryptedReasoning(*b.Thinking); ok && enc == token { + sawMarker = true + } + } + require.True(t, sawVisible, "the visible reasoning text was lost across the round trip") + require.True(t, sawMarker, + "the replay token was lost across the round trip - ingress restored it but egress did not send it back") +} diff --git a/core/providers/cohere/responses.go b/core/providers/cohere/responses.go index bd09878de58..1719ec5f5c1 100644 --- a/core/providers/cohere/responses.go +++ b/core/providers/cohere/responses.go @@ -1660,40 +1660,101 @@ func convertBifrostMessageToCohereMessage(msg *schemas.ResponsesMessage) *Cohere func convertBifrostReasoningToCohereThinking(msg *schemas.ResponsesMessage) []CohereContentBlock { var thinkingBlocks []CohereContentBlock - if msg.Content != nil && msg.Content.ContentBlocks != nil { + // Track whether the content blocks actually yielded reasoning rather than + // branching on Content being non-nil. A non-nil but EMPTY ContentBlocks -- or + // one carrying only non-reasoning blocks -- must fall through to + // ResponsesReasoning instead of shadowing it, otherwise the replayed + // reasoning is dropped with no error and no warning. + emittedFromContentBlocks := false + if msg.Content != nil { for _, block := range msg.Content.ContentBlocks { if block.Type == schemas.ResponsesOutputMessageContentTypeReasoning && block.Text != nil { - thinkingBlock := CohereContentBlock{ + thinkingBlocks = append(thinkingBlocks, CohereContentBlock{ Type: CohereContentBlockTypeThinking, Thinking: block.Text, - } - thinkingBlocks = append(thinkingBlocks, thinkingBlock) + }) + emittedFromContentBlocks = true } } - } else if msg.ResponsesReasoning != nil { - if msg.ResponsesReasoning.Summary != nil { - for _, reasoningContent := range msg.ResponsesReasoning.Summary { - thinkingBlock := CohereContentBlock{ - Type: CohereContentBlockTypeThinking, - Thinking: &reasoningContent.Text, - } - thinkingBlocks = append(thinkingBlocks, thinkingBlock) - } - } else if msg.ResponsesReasoning.EncryptedContent != nil { - // Cohere doesn't have a direct equivalent to encrypted content, - // so we'll store it as a regular thinking block with a special marker - encryptedText := fmt.Sprintf("[ENCRYPTED_REASONING: %s]", *msg.ResponsesReasoning.EncryptedContent) - thinkingBlock := CohereContentBlock{ + } + if msg.ResponsesReasoning == nil { + return thinkingBlocks + } + + // Not `emittedFromContentBlocks || ...`: the content blocks and the encrypted + // token are independent facts, and a message can carry both. This file's own + // ingress path produces exactly that shape - restoring EncryptedContent + // leaves the visible reasoning blocks in place - so returning early here + // dropped the token on the next turn, which is the turn it exists for. + // + // Only the SUMMARY is skipped when the content blocks already spoke, since + // those two carry the same visible text and emitting both would duplicate it. + + // len(Summary), not Summary != nil. Every construction site of + // schemas.ResponsesReasoning in this codebase sets an empty-but-non-nil + // slice, and the field has no omitempty so that state survives a JSON round + // trip -- so the nil check was true for essentially every real message, the + // loop ran zero times, and the encrypted branch below was unreachable. + if !emittedFromContentBlocks { + for _, reasoningContent := range msg.ResponsesReasoning.Summary { + text := reasoningContent.Text + thinkingBlocks = append(thinkingBlocks, CohereContentBlock{ Type: CohereContentBlockTypeThinking, - Thinking: &encryptedText, - } - thinkingBlocks = append(thinkingBlocks, thinkingBlock) + Thinking: &text, + }) } } + // Emitted IN ADDITION to any summary, not instead of it: the visible summary + // and the encrypted replay token are independent facts, and the next turn + // needs whichever the upstream will accept. Cohere has no encrypted-reasoning + // field, so it travels as a marked thinking block -- imperfect, but strictly + // better than silently losing it. + if enc := msg.ResponsesReasoning.EncryptedContent; enc != nil && *enc != "" { + encryptedText := formatEncryptedReasoning(*enc) + thinkingBlocks = append(thinkingBlocks, CohereContentBlock{ + Type: CohereContentBlockTypeThinking, + Thinking: &encryptedText, + }) + } + return thinkingBlocks } +// Cohere has no encrypted-reasoning field, so the replay token travels as a +// marked thinking block. The marker is a transport detail of THIS provider +// pairing: both ends of it live here, and neither end is meaningful without the +// other. Egress-only was the original bug - the token was written out and never +// read back, so it reached clients as visible reasoning prose and was replayed +// as if the model had thought the marker itself. +const ( + encryptedReasoningPrefix = "[ENCRYPTED_REASONING: " + encryptedReasoningSuffix = "]" +) + +func formatEncryptedReasoning(enc string) string { + return encryptedReasoningPrefix + enc + encryptedReasoningSuffix +} + +// parseEncryptedReasoning recovers a replay token from a thinking block, and +// reports whether the block was a marker at all. +// +// Deliberately exact: only a block that is ENTIRELY the marker counts. A model +// that merely mentions the phrase mid-sentence is writing prose, and treating +// that as a replay token would both swallow real reasoning text and hand the +// provider a token it never issued. +func parseEncryptedReasoning(thinking string) (string, bool) { + if !strings.HasPrefix(thinking, encryptedReasoningPrefix) || + !strings.HasSuffix(thinking, encryptedReasoningSuffix) { + return "", false + } + enc := thinking[len(encryptedReasoningPrefix) : len(thinking)-len(encryptedReasoningSuffix)] + if enc == "" { + return "", false + } + return enc, true +} + // convertBifrostFunctionCallToCohereMessage converts a Bifrost function call to Cohere message func convertBifrostFunctionCallToCohereMessage(msg *schemas.ResponsesMessage) *CohereMessage { assistantMsg := CohereMessage{ @@ -1770,6 +1831,8 @@ func convertBifrostFunctionCallOutputToCohereMessage(msg *schemas.ResponsesMessa func convertSingleCohereMessageToBifrostMessages(cohereMsg *CohereMessage, isOutputMessage bool) []schemas.ResponsesMessage { var outputMessages []schemas.ResponsesMessage var reasoningContentBlocks []schemas.ResponsesMessageContentBlock + // Replay token recovered from a marked thinking block, if this message carried one. + var encryptedReasoning string // Handle text content first if cohereMsg.Content != nil { @@ -1791,6 +1854,17 @@ func convertSingleCohereMessageToBifrostMessages(cohereMsg *CohereMessage, isOut // Convert content blocks and separate reasoning blocks for _, block := range cohereMsg.Content.BlocksContent { if block.Type == CohereContentBlockTypeThinking { + // A marker block is the encrypted replay token this + // provider smuggles through the thinking channel, not + // something the model thought. It goes back to + // EncryptedContent, where the next turn's egress looks for + // it, and is kept out of the visible reasoning text. + if block.Thinking != nil { + if enc, ok := parseEncryptedReasoning(*block.Thinking); ok { + encryptedReasoning = enc + continue + } + } // Collect reasoning blocks to create a single reasoning message reasoningContentBlocks = append(reasoningContentBlocks, schemas.ResponsesMessageContentBlock{ Type: schemas.ResponsesOutputMessageContentTypeReasoning, @@ -1836,14 +1910,23 @@ func convertSingleCohereMessageToBifrostMessages(cohereMsg *CohereMessage, isOut } } - // Handle reasoning blocks - prepend reasoning message if we collected any - if len(reasoningContentBlocks) > 0 { + // Handle reasoning blocks - prepend reasoning message if we collected any. + // + // The encrypted token counts on its own: a turn whose only reasoning was the + // replay marker has no visible blocks left after it is extracted, and + // gating solely on the block count would drop the token entirely - the very + // bug this restores. + if len(reasoningContentBlocks) > 0 || encryptedReasoning != "" { + reasoning := &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + } + if encryptedReasoning != "" { + reasoning.EncryptedContent = new(encryptedReasoning) + } reasoningMessage := schemas.ResponsesMessage{ - ID: new("rs_" + fmt.Sprintf("%d", time.Now().UnixNano())), - Type: new(schemas.ResponsesMessageTypeReasoning), - ResponsesReasoning: &schemas.ResponsesReasoning{ - Summary: []schemas.ResponsesReasoningSummary{}, - }, + ID: new("rs_" + fmt.Sprintf("%d", time.Now().UnixNano())), + Type: new(schemas.ResponsesMessageTypeReasoning), + ResponsesReasoning: reasoning, Content: &schemas.ResponsesMessageContent{ ContentBlocks: reasoningContentBlocks, }, diff --git a/core/providers/gemini/reasoningreplay_test.go b/core/providers/gemini/reasoningreplay_test.go new file mode 100644 index 00000000000..694c0913b1b --- /dev/null +++ b/core/providers/gemini/reasoningreplay_test.go @@ -0,0 +1,106 @@ +package gemini + +import ( + "encoding/base64" + "testing" + + "github.com/maximhq/bifrost/core/schemas" + "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" +) + +// Gemini's streaming and non-streaming egress disagreed about whether +// encrypted_content is already base64. +// +// encrypted_content is a base64 STRING; Part.ThoughtSignature is a []byte that +// Part.MarshalJSON base64-encodes on the way out. The non-streaming converters +// decoded first, so the wire carried base64(signature). The streaming converter +// assigned []byte(encryptedContent) straight through, so the wire carried +// base64(base64(signature)) -- a value Gemini cannot verify, for the same +// conversation, depending only on whether the client streamed. +// +// Both paths now go through thoughtSignatureFromEncryptedContent, so they cannot +// drift apart again. +func TestThoughtSignatureFromEncryptedContent(t *testing.T) { + raw := []byte{0x01, 0x02, 0xff, 0xfe, 0x7f} + encoded := base64.StdEncoding.EncodeToString(raw) + + t.Run("decodes to the original bytes", func(t *testing.T) { + require.Equal(t, raw, thoughtSignatureFromEncryptedContent(&encoded)) + }) + + t.Run("serialises to single-encoded base64", func(t *testing.T) { + part := &Part{ThoughtSignature: thoughtSignatureFromEncryptedContent(&encoded)} + data, err := part.MarshalJSON() + require.NoError(t, err) + + onWire := gjson.GetBytes(data, "thoughtSignature").String() + require.Equal(t, encoded, onWire, + "thoughtSignature must round-trip to the value the client sent, not a re-encoding of it") + + // The specific corruption this guards against: passing the base64 string + // through as bytes yields base64(base64(sig)), which is strictly longer + // and decodes to the base64 TEXT rather than the signature. + doubled := base64.StdEncoding.EncodeToString([]byte(encoded)) + require.NotEqual(t, doubled, onWire, "signature was double-encoded") + }) + + t.Run("rejects unusable values rather than corrupting them", func(t *testing.T) { + require.Nil(t, thoughtSignatureFromEncryptedContent(nil)) + require.Nil(t, thoughtSignatureFromEncryptedContent(ptr(""))) + // Not valid base64: dropping it beats shipping a signature Gemini will + // reject, and beats a partial decode. + require.Nil(t, thoughtSignatureFromEncryptedContent(ptr("not!valid!base64!"))) + }) +} + +func ptr(s string) *string { return &s } + +// Gemini 3 carries thoughtSignature on the thought part itself, and requires it +// back on replay -- there is a dedicated finish reason for its absence +// (FinishReasonMissingThoughtSignature). The ingress converter read only +// part.Text, so the signature never reached the client and could never be +// replayed; a signature-only thought part produced no message at all. +func TestThoughtPartIngressPreservesSignature(t *testing.T) { + raw := []byte{0x0a, 0x0b, 0x0c, 0xff} + encoded := base64.StdEncoding.EncodeToString(raw) + + t.Run("signature survives alongside text", func(t *testing.T) { + msgs := responsesMessagesForThoughtPart(t, &Part{ + Thought: true, Text: "step by step", ThoughtSignature: raw, + }) + require.Len(t, msgs, 1) + require.NotNil(t, msgs[0].ResponsesReasoning, "signature dropped: no reasoning payload") + require.NotNil(t, msgs[0].ResponsesReasoning.EncryptedContent) + require.Equal(t, encoded, *msgs[0].ResponsesReasoning.EncryptedContent) + // Round trip: what egress decodes must equal what Gemini sent. + require.Equal(t, raw, thoughtSignatureFromEncryptedContent(msgs[0].ResponsesReasoning.EncryptedContent)) + }) + + t.Run("signature-only thought part is not dropped", func(t *testing.T) { + msgs := responsesMessagesForThoughtPart(t, &Part{Thought: true, ThoughtSignature: raw}) + require.Len(t, msgs, 1, "a signature-only thought part must still produce a message") + require.NotNil(t, msgs[0].ResponsesReasoning) + require.Equal(t, encoded, *msgs[0].ResponsesReasoning.EncryptedContent) + }) + + t.Run("thought part with neither text nor signature emits nothing", func(t *testing.T) { + require.Empty(t, responsesMessagesForThoughtPart(t, &Part{Thought: true})) + }) +} + +// responsesMessagesForThoughtPart runs the real ingress converter over a single +// candidate holding one part, and returns the reasoning messages it produced. +func responsesMessagesForThoughtPart(t *testing.T, part *Part) []schemas.ResponsesMessage { + t.Helper() + out := convertGeminiCandidatesToResponsesOutput([]*Candidate{ + {Content: &Content{Parts: []*Part{part}}}, + }) + var reasoning []schemas.ResponsesMessage + for _, msg := range out { + if msg.Type != nil && *msg.Type == schemas.ResponsesMessageTypeReasoning { + reasoning = append(reasoning, msg) + } + } + return reasoning +} diff --git a/core/providers/gemini/responses.go b/core/providers/gemini/responses.go index 41e7f34053e..150ade4064e 100644 --- a/core/providers/gemini/responses.go +++ b/core/providers/gemini/responses.go @@ -15,6 +15,30 @@ import ( "github.com/maximhq/bifrost/core/schemas" ) +// thoughtSignatureFromEncryptedContent converts a Responses-API +// encrypted_content value into the raw bytes Part.ThoughtSignature expects. +// +// The decode is required, not cosmetic. encrypted_content is already a base64 +// STRING, while ThoughtSignature is a []byte that Part.MarshalJSON base64s on +// the way out -- so assigning []byte(encryptedContent) directly ships +// base64(base64(signature)) and Gemini cannot verify it. The non-streaming +// converters always decoded first; the streaming one did not, which meant the +// two paths silently disagreed about the encoding for the same conversation. +// +// Returns nil when there is nothing usable, so callers can skip the part +// entirely rather than emit an empty signature: a malformed value is dropped +// rather than corrupted onwards. +func thoughtSignatureFromEncryptedContent(encryptedContent *string) []byte { + if encryptedContent == nil || *encryptedContent == "" { + return nil + } + decoded, err := base64.StdEncoding.DecodeString(*encryptedContent) + if err != nil || len(decoded) == 0 { + return nil + } + return decoded +} + func (request *GeminiGenerationRequest) ToBifrostResponsesRequest(ctx *schemas.BifrostContext) *schemas.BifrostResponsesRequest { if request == nil { return nil @@ -522,8 +546,8 @@ func ToGeminiResponsesResponse(bifrostResp *schemas.BifrostResponsesResponse) *G } } if msg.ResponsesReasoning.EncryptedContent != nil { - decodedSig, err := base64.StdEncoding.DecodeString(*msg.ResponsesReasoning.EncryptedContent) - if err == nil { + decodedSig := thoughtSignatureFromEncryptedContent(msg.ResponsesReasoning.EncryptedContent) + if decodedSig != nil { currentParts = append(currentParts, &Part{ ThoughtSignature: decodedSig, }) @@ -849,10 +873,10 @@ func ToGeminiResponsesStreamResponse(bifrostResp *schemas.BifrostResponsesStream // Already handled via deltas, skip return nil case schemas.ResponsesStreamResponseTypeOutputItemAdded: - if bifrostResp.Item != nil && bifrostResp.Item.ResponsesReasoning != nil && bifrostResp.Item.EncryptedContent != nil { - candidate.Content.Parts = append(candidate.Content.Parts, &Part{ - ThoughtSignature: []byte(*bifrostResp.Item.ResponsesReasoning.EncryptedContent), - }) + if bifrostResp.Item != nil && bifrostResp.Item.ResponsesReasoning != nil { + if sig := thoughtSignatureFromEncryptedContent(bifrostResp.Item.ResponsesReasoning.EncryptedContent); sig != nil { + candidate.Content.Parts = append(candidate.Content.Parts, &Part{ThoughtSignature: sig}) + } } // Track function call metadata for later use in FunctionCallArgumentsDone if bifrostResp.Item != nil && bifrostResp.Item.Type != nil && @@ -2662,20 +2686,43 @@ func convertGeminiCandidatesToResponsesOutput(candidates []*Candidate) []schemas // Handle different types of parts switch { case part.Thought: - // Thinking/reasoning message - if part.Text != "" { + // Thinking/reasoning message. + // + // The signature has to come across with the text. Gemini 3 puts + // thoughtSignature on the thought part itself, and requires it back + // on replay -- there is a dedicated finish reason for its absence + // (FinishReasonMissingThoughtSignature). Reading only part.Text + // dropped it on the floor, so a client could never send it back. + // + // Emitted even when the text is empty: a signature-only thought + // part is a real Gemini shape, and skipping it loses the one field + // the next turn actually needs. + if part.Text != "" || len(part.ThoughtSignature) > 0 { + text := part.Text msg := schemas.ResponsesMessage{ Role: schemas.Ptr(schemas.ResponsesInputMessageRoleAssistant), Content: &schemas.ResponsesMessageContent{ ContentBlocks: []schemas.ResponsesMessageContentBlock{ { Type: schemas.ResponsesOutputMessageContentTypeReasoning, - Text: &part.Text, + Text: &text, }, }, }, Type: schemas.Ptr(schemas.ResponsesMessageTypeReasoning), } + if len(part.ThoughtSignature) > 0 { + // Stored base64-encoded, which is the form + // encrypted_content carries on the wire and the form + // thoughtSignatureFromEncryptedContent decodes on the way + // back out -- so the round trip is symmetric by construction. + encoded := base64.StdEncoding.EncodeToString(part.ThoughtSignature) + msg.ResponsesReasoning = &schemas.ResponsesReasoning{ + Summary: []schemas.ResponsesReasoningSummary{}, + EncryptedContent: &encoded, + } + msg.Content.ContentBlocks[0].Signature = &encoded + } messages = append(messages, msg) } diff --git a/tests/e2e/api/collections/provider-harness.json b/tests/e2e/api/collections/provider-harness.json index 195e7a12c0b..99c126b8938 100644 --- a/tests/e2e/api/collections/provider-harness.json +++ b/tests/e2e/api/collections/provider-harness.json @@ -48638,6 +48638,243 @@ "response": [] } ] + }, + { + "name": "42. Cohere Encrypted Reasoning Round-Trip (PR #5982)", + "description": "Regression coverage for the Cohere half of PR #5982. Cohere's API has no encrypted-reasoning field, so a replay token travels out inside a thinking block marked '[ENCRYPTED_REASONING: ...]'. That was only half a round trip: egress wrote the marker, and nothing on ingress read it back, so every thinking block - marker included - was mapped to ordinary reasoning text. The token never reached ResponsesReasoning.EncryptedContent, and the marker itself leaked to clients as visible model reasoning, then got replayed as if it were content. Case 1 pins that reasoning comes back as a reasoning item and captures the turn; case 2 replays it and asserts both that the replay is accepted and that the transport marker never appears in the response.", + "item": [ + { + "name": "cohere/command-a-reasoning /v1/responses returns reasoning + captures replay input - PR #5982", + "event": [ + { + "listen": "test", + "script": { + "type": "text/javascript", + "exec": [ + "if ([401, 403, 429, 500, 502, 503, 504].indexOf(pm.response.code) !== -1) { return; }", + "pm.test('Cohere reasoning returned as a reasoning output item', function () {", + " pm.expect(pm.response.code, 'request failed: ' + pm.response.text()).to.be.below(400);", + " var j = pm.response.json();", + " pm.expect(j.output, 'expected output array').to.be.an('array').that.is.not.empty;", + " var reasoning = j.output.filter(function (o) { return o.type === 'reasoning'; });", + " pm.expect(reasoning.length, 'expected at least one reasoning output item').to.be.above(0);", + " // The replay row checks this too, but only that row - a capture response", + " // that leaked the marker would slip through whenever the replay response", + " // happened to carry no reasoning item.", + " pm.expect(pm.response.text(), 'the transport marker leaked into the capture response')", + " .to.not.include('ENCRYPTED_REASONING');", + "});", + "// Capture the exact turn to replay: the original user message plus the", + "// assistant output echoed verbatim. Echoing verbatim is the point - it is", + "// what carries any encrypted_content back out to the provider.", + "try {", + " var body = pm.response.json();", + " if (body.output && body.output.length) {", + " var input = [{ type: 'message', role: 'user', content: \"Work out 17 * 23 step by step, then give just the number.\" }]", + " .concat(body.output);", + " pm.collectionVariables.set('cohereReasoningReplayInput5982', JSON.stringify(input));", + " }", + "} catch (e) {}" + ] + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\"model\": \"cohere/command-a-reasoning-08-2025\", \"input\": \"Work out 17 * 23 step by step, then give just the number.\", \"reasoning\": {\"effort\": \"medium\"}, \"max_output_tokens\": 2048}" + }, + "url": { + "raw": "{{baseUrl}}/v1/responses", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "v1", + "responses" + ] + } + } + }, + { + "name": "cohere/command-a-reasoning /v1/responses replay preserves encrypted reasoning - PR #5982", + "event": [ + { + "listen": "test", + "script": { + "type": "text/javascript", + "exec": [ + "// Skip when the capture step did not run: the body placeholder stays\n// unresolved, so there is nothing meaningful to replay.", + "if (pm.request.body && pm.request.body.raw && pm.request.body.raw.indexOf('{{') !== -1) { return; }", + "if ([401, 403, 429, 500, 502, 503, 504].indexOf(pm.response.code) !== -1) { return; }", + "pm.test('Replayed Cohere reasoning is accepted', function () {", + " pm.expect(pm.response.code, 'replay request failed: ' + pm.response.text()).to.be.below(400);", + " var j = pm.response.json();", + " pm.expect(j.output, 'expected an output array').to.be.an('array').that.is.not.empty;", + "});", + "// The regression this folder exists for. Cohere has no encrypted-reasoning", + "// field, so Bifrost smuggles the replay token out inside a marked thinking", + "// block. Pre-fix nothing decoded that marker on the way back in, so it", + "// surfaced to the client as ordinary reasoning TEXT - the model appearing to", + "// have literally thought '[ENCRYPTED_REASONING: ...]', and the token that was", + "// meant to land in encrypted_content lost.", + "pm.test('Transport marker never surfaces as visible reasoning text', function () {", + " pm.expect(pm.response.text(), 'the [ENCRYPTED_REASONING: ...] transport marker leaked to the client')", + " .to.not.include('ENCRYPTED_REASONING');", + "});" + ] + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\"model\": \"cohere/command-a-reasoning-08-2025\", \"input\": {{cohereReasoningReplayInput5982}}, \"reasoning\": {\"effort\": \"medium\"}, \"max_output_tokens\": 2048}" + }, + "url": { + "raw": "{{baseUrl}}/v1/responses", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "v1", + "responses" + ] + } + } + } + ] + }, + { + "name": "43. Gemini Reasoning Signature Replay (PR #5982)", + "description": "Multi-turn reasoning replay for Gemini through /v1/responses. Gemini returns a thought signature on reasoning that accompanies a function call, and requires it back unmodified when the tool result is sent - a dropped or rewritten signature is rejected on the continuation, so a converter that keeps only part of a reasoning item fails here and nowhere else. The harness had 71 reasoning rows and not one that replayed reasoning back on a second turn, which is the only shape that exercises this. Case 1 captures a reasoning + function_call turn; case 2 replays it with the tool result.", + "item": [ + { + "name": "gemini/gemini-2.5-flash /v1/responses reasoning + tool call captures replay input - PR #5982", + "event": [ + { + "listen": "test", + "script": { + "type": "text/javascript", + "exec": [ + "if ([401, 403, 429, 500, 502, 503, 504].indexOf(pm.response.code) !== -1) { return; }", + "pm.test('Gemini returns reasoning alongside the tool call', function () {", + " pm.expect(pm.response.code, 'request failed: ' + pm.response.text()).to.be.below(400);", + " var j = pm.response.json();", + " pm.expect(j.output, 'expected output array').to.be.an('array').that.is.not.empty;", + " // Asserted, not merely hoped for: the replay row skips when the capture", + " // stored nothing, so without this a run where Gemini did not call the", + " // tool passes green having tested no signature preservation at all.", + " var kinds = j.output.map(function (o) { return o.type; });", + " pm.expect(kinds, 'no function_call came back, so the replay row would silently skip').to.include('function_call');", + " pm.expect(kinds, 'no reasoning item came back, so there is no thought signature to preserve').to.include('reasoning');", + "});", + "// Only capture when a function call came back: the replay case is about a", + "// reasoning item sitting next to a function_call, which is the shape whose", + "// thought signature has to survive. Without one there is nothing to pin, so", + "// the replay skips rather than false-failing.", + "try {", + " var body = pm.response.json();", + " var fc = (body.output || []).filter(function (o) { return o.type === 'function_call'; })[0];", + " if (fc && fc.call_id) {", + " var input = [{ type: 'message', role: 'user', content: \"What time is it in Tokyo? Use the get_time tool, thinking it through first.\" }]", + " .concat(body.output)", + " .concat([{ type: 'function_call_output', call_id: fc.call_id, output: '{\"time\": \"21:00 JST\"}' }]);", + " pm.collectionVariables.set('geminiReasoningReplayInput5982', JSON.stringify(input));", + " var reasoning = (body.output || []).filter(function (o) { return o.type === 'reasoning'; });", + " pm.collectionVariables.set('geminiReasoningCount5982', String(reasoning.length));", + " }", + "} catch (e) {}" + ] + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\"model\": \"gemini/gemini-2.5-flash\", \"input\": \"What time is it in Tokyo? Use the get_time tool, thinking it through first.\", \"tools\": [{\"type\": \"function\", \"name\": \"get_time\", \"description\": \"Get the current time\", \"parameters\": {\"type\": \"object\", \"properties\": {\"timezone\": {\"type\": \"string\"}}, \"required\": []}}], \"tool_choice\": {\"type\": \"function\", \"name\": \"get_time\"}, \"reasoning\": {\"effort\": \"low\"}, \"max_output_tokens\": 2048}" + }, + "url": { + "raw": "{{baseUrl}}/v1/responses", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "v1", + "responses" + ] + } + } + }, + { + "name": "gemini/gemini-2.5-flash /v1/responses replay preserves thought signature - PR #5982", + "event": [ + { + "listen": "test", + "script": { + "type": "text/javascript", + "exec": [ + "// Skip when the capture step did not run: the body placeholder stays\n// unresolved, so there is nothing meaningful to replay.", + "if (pm.request.body && pm.request.body.raw && pm.request.body.raw.indexOf('{{') !== -1) { return; }", + "if ([401, 403, 429, 500, 502, 503, 504].indexOf(pm.response.code) !== -1) { return; }", + "pm.test('Replayed Gemini reasoning + tool call is accepted', function () {", + " var body = pm.response.text() || '';", + " pm.expect(body, 'Gemini rejected the replayed turn - the thought signature did not survive the round trip')", + " .to.not.include('thought_signature');", + " pm.expect(pm.response.code, 'replay request failed: ' + body).to.be.below(400);", + " var j = pm.response.json();", + " pm.expect(j.output, 'expected an output array').to.be.an('array').that.is.not.empty;", + "});" + ] + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\"model\": \"gemini/gemini-2.5-flash\", \"input\": {{geminiReasoningReplayInput5982}}, \"tools\": [{\"type\": \"function\", \"name\": \"get_time\", \"description\": \"Get the current time\", \"parameters\": {\"type\": \"object\", \"properties\": {\"timezone\": {\"type\": \"string\"}}, \"required\": []}}], \"reasoning\": {\"effort\": \"low\"}, \"max_output_tokens\": 2048}" + }, + "url": { + "raw": "{{baseUrl}}/v1/responses", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "v1", + "responses" + ] + } + } + } + ] } ] } \ No newline at end of file diff --git a/tests/e2e/api/runners/harness-viewer.mjs b/tests/e2e/api/runners/harness-viewer.mjs index 54d0a6d2582..f45384538d6 100644 --- a/tests/e2e/api/runners/harness-viewer.mjs +++ b/tests/e2e/api/runners/harness-viewer.mjs @@ -8,8 +8,9 @@ // Usage: // node harness-viewer.mjs --report tmp/newman-report.json [--port 8090] -import { readFileSync, existsSync } from "node:fs"; +import { readFileSync, existsSync, writeFileSync } from "node:fs"; import { readReport } from "./lib/read-report.mjs"; +import { redactItemsForPublic } from "./lib/redact-report.mjs"; import { createServer } from "node:http"; import { URL } from "node:url"; @@ -263,8 +264,14 @@ const filterInput = document.getElementById('filter'); const onlyFailed = document.getElementById('only-failed'); async function load() { - const r = await fetch('/api/report'); - items = await r.json(); + // Static mode inlines the report instead of serving it, so the same page + // works as a CI artifact opened from disk with no server behind it. + if (window.__STATIC_REPORT__) { + items = window.__STATIC_REPORT__; + } else { + const r = await fetch('/api/report'); + items = await r.json(); + } document.getElementById('meta').textContent = items.length + ' requests'; renderSummary(); render(); @@ -324,11 +331,18 @@ function render() { '' + '
' + '

Assertions

' + (assertions || '(none)') + '
' + - '
' + - '' + - '' + - '' + - '
' + + // Resend proxies through this process, so it cannot work in static + // mode. Copy curl is pure client-side and stays useful offline. + (window.__STATIC_REPORT__ + ? '
' + + '' + + 'Resend needs the live viewer: make run-provider-harness-test' + + '
' + : '
' + + '' + + '' + + '' + + '
') + '
' + '

Request Body

' + escape(prettyBody(i.reqBody)) + '
' + '

Response Body

' + escape(prettyBody(i.respBody)) + '
' + @@ -420,6 +434,32 @@ load(); `; const items = summarize(); + +// Static mode: write the same page to a file with the report inlined, and do +// not start a server. +// +// This exists because CI has no HTML report at all for the provider harness. +// The htmlextra reporter only emits one in sequential mode (PARALLEL=0), and CI +// runs PARALLEL=1 -- one newman fork per provider, with no way to merge N HTML +// documents. The merged JSON, however, is exactly what this viewer already +// renders, so reusing it costs nothing and keeps a single source of truth for +// the markup: fixing the viewer fixes the artifact, and they cannot drift. +if (args.static) { + // Redact before serializing, and ONLY here. The live viewer below serves the + // full items to whoever ran the harness on their own machine; this file is + // uploaded to R2 and linked from the public changelog, so every byte inlined + // into it is world-readable - and provider requests carry Authorization and + // API-key headers, signed URLs, and error bodies that quote keys back. + const inlined = + ``; + // Injected before the page's own script so load() sees it, and with < escaped + // so a response body containing "" cannot break out of the tag. + const html = VIEWER_HTML.replace("
", `
${inlined}`); + writeFileSync(args.static, html); + console.log(`Static provider harness report: ${args.static} (${items.length} requests)`); + process.exit(0); +} + const allowedTargets = new Set( items.map((i) => `${String(i.method || "GET").toUpperCase()} ${i.url}`) ); diff --git a/tests/e2e/api/runners/lib/redact-report.mjs b/tests/e2e/api/runners/lib/redact-report.mjs new file mode 100644 index 00000000000..2ebab31099a --- /dev/null +++ b/tests/e2e/api/runners/lib/redact-report.mjs @@ -0,0 +1,172 @@ +// Redaction for the PUBLISHED provider-harness report. +// +// The live viewer serves this data to whoever ran the harness, on their own +// machine, from their own keys - full fidelity there is the point. The --static +// page is a different audience entirely: it is uploaded to R2 and linked from +// the public changelog, so anything inlined into it is world-readable and stays +// that way in caches and mirrors long after any key is rotated. +// +// So this is applied on the static path only. The shape is preserved - the +// report still shows WHICH headers were sent and how big a body was - because a +// report that hides what was exercised is not worth publishing either. + +// Header names whose value is a credential. Matched case-insensitively against +// the whole name, since these are exact header names rather than prefixes. +const SECRET_HEADERS = new Set([ + "authorization", + "proxy-authorization", + "cookie", + "set-cookie", + "x-api-key", + "api-key", + "apikey", + "x-goog-api-key", + "x-goog-iam-authorization-token", + "x-amz-security-token", + "x-amz-content-sha256", + "x-auth-token", + "x-access-token", + "openai-api-key", + "anthropic-api-key", + "x-bifrost-api-key", +]); + +// Query parameters that carry credentials in the URL itself. Matched +// case-insensitively. +const SECRET_PARAMS = new Set([ + "key", + "api_key", + "apikey", + "access_token", + "token", + "signature", + "sig", + "awsaccesskeyid", + "x-amz-signature", + "x-amz-credential", + "x-amz-security-token", + "x-goog-api-key", +]); + +export const REDACTED = "[REDACTED]"; + +// Credential-shaped runs that appear INSIDE bodies. Providers echo keys back in +// error messages ("Incorrect API key provided: sk-..."), and a signed URL in a +// response body is just as live as one in a request. +// +// Ordered most specific first; each is applied globally. +const BODY_SECRET_PATTERNS = [ + /\bBearer\s+[A-Za-z0-9._~+/=-]{8,}/gi, // Authorization values quoted into errors + /\b(?:sk|rk|pk)-[A-Za-z0-9._-]{12,}/g, // OpenAI/Anthropic-style keys + /\bAKIA[0-9A-Z]{16}\b/g, // AWS access key id + /\bASIA[0-9A-Z]{16}\b/g, // AWS temporary access key id + /\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}/g, // JWT + /\bAIza[0-9A-Za-z._-]{20,}/g, // Google API key + /\bxox[abposr]-[A-Za-z0-9-]{8,}/g, // Slack-style token +]; + +const redactHeaders = (headers) => + (headers || []).map((h) => + SECRET_HEADERS.has(String(h.key || "").toLowerCase()) ? { ...h, value: REDACTED } : h + ); + +// redactUrl blanks credential-bearing query parameters while leaving the path +// and every other parameter legible, so the report still says which endpoint +// was called. +// decodeParamName tolerates a malformed escape rather than throwing: a name that +// cannot be decoded is compared as-is, which is the pre-existing behaviour. +const decodeParamName = (name) => { + try { + return decodeURIComponent(name.replace(/\+/g, " ")); + } catch { + return name; + } +}; + +// redactUserinfo blanks a `user:password@` credential embedded in the authority. +// +// This is not reachable from the query-parameter path at all - such a URL often +// has no query string - and it is a live credential just the same. +const redactUserinfo = (raw) => + // The WHOLE userinfo goes, username included. Keeping the username "because it is only a name" + // was the bug: token-in-URL auth is normally written with the credential in the username + // position and no password at all (https://sk-proj-...@host/), so preserving it published the + // key while redacting an empty password beside it. + raw.replace(/^([a-zA-Z][a-zA-Z0-9+.-]*:\/\/)[^/?#@]*@/, `$1${REDACTED}@`); + +// redactParamList blanks credential-bearing entries in an `a=1&b=2` list, +// leaving every other entry legible. Shared by the query string and the +// fragment, which use the same encoding. +const redactParamList = (list) => + list + .split("&") + .map((pair) => { + const eq = pair.indexOf("="); + if (eq === -1) return pair; + const name = pair.slice(0, eq); + // Decode before matching: `api%5Fkey` is the same parameter as `api_key`, + // and comparing the raw name published its value. + return SECRET_PARAMS.has(decodeParamName(name).toLowerCase()) ? `${name}=${REDACTED}` : pair; + }) + .join("&"); + +export function redactUrl(url) { + const raw = redactUserinfo(String(url || "")); + + // The fragment comes off FIRST, before the query is located. Splitting on "?" + // alone leaves `a=1#access_token=...` as a single pair named "a", which + // matches nothing in SECRET_PARAMS and publishes the token - and a URL whose + // credential lives in the fragment (the implicit-flow OAuth response puts it + // there by design) often has no query string to split on at all. + const h = raw.indexOf("#"); + const beforeFragment = h === -1 ? raw : raw.slice(0, h); + const fragment = h === -1 ? "" : raw.slice(h + 1); + + const q = beforeFragment.indexOf("?"); + const base = + q === -1 + ? beforeFragment + : `${beforeFragment.slice(0, q)}?${redactParamList(beforeFragment.slice(q + 1))}`; + + return fragment ? `${base}#${redactParamList(fragment)}` : base; +} + +// URLs embedded in a body. Bounded by whitespace and the delimiters a URL cannot +// contain unescaped, so a link inside JSON ("...") or HTML () ends where +// the payload says it ends rather than swallowing the rest of the document. +// +// Both spellings of the separators are accepted: `\/` is a legal JSON escape for `/` +// and many serializers emit it, so the same signed URL reaches us as +// https:\/\/host\/path just as often as the literal form. Matching only the literal +// one left the escaped variant unredacted, which is the harder case to notice. +const URL_IN_BODY = /https?:(?:\\?\/){2}(?:\\\/|[^\s"'<>\\)\]}])+/g; + +export function redactBody(body) { + let out = String(body || ""); + // A signed URL is a live credential wherever it appears, and providers hand them + // back inside response payloads - a presigned S3 link in a body is exactly as usable + // as one in the request line. BODY_SECRET_PATTERNS cannot cover this: it matches + // credential SHAPES, and an HMAC signature is just hex. So run the same + // SECRET_PARAMS logic the request URL gets, rather than enumerating more shapes. + // redactUrl parses a real URL, so an escaped one is unescaped before redaction and + // re-escaped afterwards - stripping the escaping here would leave the body invalid + // JSON, and a report that will not parse is no more publishable than one that leaks. + out = out.replace(URL_IN_BODY, (url) => { + if (!url.includes("\\/")) return redactUrl(url); + return redactUrl(url.split("\\/").join("/")).split("/").join("\\/"); + }); + for (const re of BODY_SECRET_PATTERNS) out = out.replace(re, REDACTED); + return out; +} + +// redactItemsForPublic returns a copy safe to inline into a world-readable page. +export function redactItemsForPublic(items) { + return (items || []).map((item) => ({ + ...item, + url: redactUrl(item.url), + reqHeaders: redactHeaders(item.reqHeaders), + respHeaders: redactHeaders(item.respHeaders), + reqBody: redactBody(item.reqBody), + respBody: redactBody(item.respBody), + })); +} diff --git a/tests/e2e/api/runners/lib/redact-report.test.mjs b/tests/e2e/api/runners/lib/redact-report.test.mjs new file mode 100644 index 00000000000..9a71218ba6a --- /dev/null +++ b/tests/e2e/api/runners/lib/redact-report.test.mjs @@ -0,0 +1,182 @@ +// Unit tests for published-report redaction. Run directly: `node redact-report.test.mjs`. +import assert from "node:assert"; +import { redactItemsForPublic, REDACTED } from "./redact-report.mjs"; + +let passed = 0; +function test(name, fn) { + fn(); + passed++; + console.log(` ok - ${name}`); +} + +// Every one of these is a live credential shape that the provider harness +// genuinely puts on the wire. +const SECRETS = [ + "sk-proj-abcdefghijklmnop1234", + "AKIAIOSFODNN7EXAMPLE", + "AIzaSyA1234567890abcdefghijklmnopqrstu", + "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NSJ9.dBjftJeZ4CVPmB92K27uhbUJU1p1r3wa", +]; + +const item = () => ({ + idx: 0, + name: "openai/gpt-4o-mini", + folder: "8. Criss-Cross", + method: "POST", + url: `https://gw/genai/v1beta/models/x:generateContent?key=${SECRETS[2]}&alt=sse`, + reqHeaders: [ + { key: "Authorization", value: `Bearer ${SECRETS[0]}`, disabled: false }, + { key: "x-api-key", value: SECRETS[0], disabled: false }, + { key: "Content-Type", value: "application/json", disabled: false }, + ], + reqBody: `{"model":"gpt-4o-mini","auth":"Bearer ${SECRETS[0]}","aws":"${SECRETS[1]}"}`, + respHeaders: [ + { key: "Set-Cookie", value: "session=abc123; HttpOnly" }, + { key: "Content-Type", value: "application/json" }, + ], + respBody: `{"error":{"message":"Incorrect API key provided: ${SECRETS[0]}","trace":"${SECRETS[3]}"}}`, + assertions: [{ name: "status is 200", passed: true, error: null }], + failed: false, +}); + +// The whole point: whatever else changes, no credential may survive into the +// string that gets inlined into a world-readable page. +test("no credential survives serialization of the public report", () => { + const serialized = JSON.stringify(redactItemsForPublic([item()])); + for (const secret of SECRETS) { + assert.ok(!serialized.includes(secret), `published report still contains ${secret.slice(0, 12)}...`); + } + assert.ok(!serialized.includes("session=abc123"), "published report still contains a session cookie"); +}); + +// A report that hides what ran is not worth publishing, so redaction has to be +// surgical rather than wholesale. +test("the report stays legible after redaction", () => { + const [out] = redactItemsForPublic([item()]); + assert.strictEqual(out.name, "openai/gpt-4o-mini"); + assert.ok(out.url.startsWith("https://gw/genai/v1beta/models/x:generateContent"), out.url); + assert.ok(out.url.includes("alt=sse"), "non-secret query parameters must survive"); + assert.ok(out.url.includes(`key=${REDACTED}`), out.url); + // Header NAMES stay, so the report still shows what was sent. + assert.deepStrictEqual( + out.reqHeaders.map((h) => h.key), + ["Authorization", "x-api-key", "Content-Type"] + ); + assert.strictEqual(out.reqHeaders[2].value, "application/json"); + assert.strictEqual(out.reqHeaders[0].value, REDACTED); + assert.ok(out.respBody.includes("Incorrect API key provided"), "error text must survive redaction"); + assert.deepStrictEqual(out.assertions, [{ name: "status is 200", passed: true, error: null }]); +}); + +test("the original items are not mutated", () => { + const original = item(); + redactItemsForPublic([original]); + assert.strictEqual(original.reqHeaders[0].value, `Bearer ${SECRETS[0]}`); +}); + +test("missing fields do not throw", () => { + const [out] = redactItemsForPublic([{ idx: 0, name: "x" }]); + assert.strictEqual(out.url, ""); + assert.deepStrictEqual(out.reqHeaders, []); + assert.strictEqual(out.reqBody, ""); +}); + +// A percent-encoded parameter name is the same parameter. Comparing the raw +// name means `api%5Fkey=...` sails past the SECRET_PARAMS check and its value is +// published verbatim. +test("percent-encoded parameter names are still redacted", () => { + const [out] = redactItemsForPublic([ + { idx: 0, name: "x", url: `https://gw/v1/chat?api%5Fkey=${SECRETS[0]}&alt=sse` }, + ]); + assert.ok(!out.url.includes(SECRETS[0]), `encoded param name leaked its value: ${out.url}`); + assert.ok(out.url.includes("alt=sse"), "unrelated parameters must survive"); +}); + +// Credentials also travel in URL userinfo, which has no query string at all - so +// the whole query-parameter path never sees them. +test("URL userinfo credentials are redacted", () => { + const [out] = redactItemsForPublic([ + { idx: 0, name: "x", url: "https://svcuser:s3cr3t-token@gateway.example/v1/chat" }, + ]); + assert.ok(!out.url.includes("s3cr3t-token"), `userinfo credential leaked: ${out.url}`); + assert.ok(out.url.includes("gateway.example"), "the host must stay legible"); +}); + +// The credential is just as often the USERNAME with no password at all - that is +// how token-in-URL auth is normally written. Preserving the username "because it +// is only a name" publishes the key. +test("a credential in the username position is redacted", () => { + const [out] = redactItemsForPublic([ + { idx: 0, name: "x", url: "https://sk-proj-abcdefghijklmnop1234@gateway.example/v1/chat" }, + ]); + assert.ok(!out.url.includes("sk-proj-abcdefghijklmnop1234"), `username credential leaked: ${out.url}`); + assert.ok(out.url.includes("gateway.example"), "the host must stay legible"); +}); + +// A signed URL is just as live inside a body as it is in the request line - +// providers return presigned S3/GCS links in response payloads routinely. The +// signature value is deliberately chosen NOT to match any BODY_SECRET_PATTERNS +// entry, so this passes only if the URL itself is redacted rather than caught by a +// credential-shape regex. +test("a signed URL embedded in a body has its credentials redacted", () => { + const signature = "9f8e7d6c5b4a39281706abcdef01234567"; + const body = JSON.stringify({ + message: "download ready", + url: `https://bucket.s3.amazonaws.com/report.json?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Signature=${signature}&response-content-type=application%2Fjson`, + }); + const [out] = redactItemsForPublic([{ idx: 0, name: "x", respBody: body }]); + assert.ok(!out.respBody.includes(signature), `signed URL leaked from a body: ${out.respBody}`); + assert.ok(out.respBody.includes("bucket.s3.amazonaws.com"), "the host must stay legible"); + assert.ok( + out.respBody.includes("X-Amz-Algorithm=AWS4-HMAC-SHA256"), + "non-credential parameters must survive" + ); +}); + +// `\/` is a legal JSON escape for `/`, and plenty of serializers emit it, so the same +// signed URL can arrive in a body spelled https:\/\/... A matcher that only knows the +// literal form skips it entirely - and a hex signature matches none of the +// credential-shape patterns, so nothing else catches it either. +test("a slash-escaped signed URL in a body is redacted", () => { + const signature = "0011223344556677889900aabbccddeeff"; + const body = `{"url":"https:\\/\\/bucket.s3.amazonaws.com\\/report.json?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Signature=${signature}"}`; + const [out] = redactItemsForPublic([{ idx: 0, name: "x", respBody: body }]); + assert.ok(!out.respBody.includes(signature), `slash-escaped signed URL leaked: ${out.respBody}`); + assert.ok(out.respBody.includes("bucket.s3.amazonaws.com"), "the host must stay legible"); + assert.ok(out.respBody.includes("X-Amz-Algorithm=AWS4-HMAC-SHA256"), "non-credentials must survive"); + // The body has to stay parseable - redaction must not strip the JSON escaping. + assert.doesNotThrow(() => JSON.parse(out.respBody), "redacted body must remain valid JSON"); +}); + +// Redacting URLs inside bodies must not damage ordinary prose or plain links. +test("a body without credentials is left intact", () => { + const body = JSON.stringify({ docs: "https://docs.example.com/guide?page=2", note: "no secrets here" }); + const [out] = redactItemsForPublic([{ idx: 0, name: "x", reqBody: body }]); + assert.strictEqual(out.reqBody, body, "a credential-free body must be unchanged"); +}); + +// A fragment carries credentials too - the implicit-flow OAuth response puts the +// token there by design - and a URL whose credential lives in the fragment often +// has no query string at all, so the query-parameter path never runs. +test("a credential in the fragment is redacted when there is no query string", () => { + const [out] = redactItemsForPublic([ + { idx: 0, name: "x", url: "https://gateway.example/callback#access_token=s3cr3t-token&state=xyz" }, + ]); + assert.ok(!out.url.includes("s3cr3t-token"), `fragment credential leaked: ${out.url}`); + assert.ok(out.url.includes(`access_token=${REDACTED}`), out.url); + assert.ok(out.url.includes("state=xyz"), "non-credential fragment parameters must survive"); + assert.ok(out.url.startsWith("https://gateway.example/callback"), "the endpoint must stay legible"); +}); + +// The query string does not shield the fragment: splitting on "?" alone leaves +// "a=1#access_token=..." as a single pair named "a", which matches nothing in +// SECRET_PARAMS and publishes the token untouched. +test("a credential in the fragment is redacted when a query string is present", () => { + const [out] = redactItemsForPublic([ + { idx: 0, name: "x", url: "https://gateway.example/callback?alt=sse#access_token=s3cr3t-token" }, + ]); + assert.ok(!out.url.includes("s3cr3t-token"), `fragment credential leaked past the query: ${out.url}`); + assert.ok(out.url.includes("alt=sse"), "unrelated query parameters must survive"); +}); + +console.log(`\n${passed} passed`); diff --git a/tests/e2e/clis/assertion_output_test.go b/tests/e2e/clis/assertion_output_test.go index 619b1ebc7b5..eac5bb9de24 100644 --- a/tests/e2e/clis/assertion_output_test.go +++ b/tests/e2e/clis/assertion_output_test.go @@ -211,6 +211,38 @@ func TestRateLimitDelayParsesEscapedCLIError(t *testing.T) { } } +// A throttle has to be recognised whichever side of the phrase the failure word +// lands on. "API Error: rate limit" is standard CLI wording and puts the error +// marker first, where the reverse-order branch was only looking for +// exceeded/hit/reached/throttled - so the cell failed outright instead of +// retrying a transient throttle. +// +// The negative cases are the reason the pattern demands a failure word at all: +// a model that has read this repo will happily quote "rate-limit" out of the +// architecture docs, and that must not be mistaken for being rate limited. +func TestRateLimitDelayDetectsErrorFirstWording(t *testing.T) { + for _, throttled := range []string{ + "API Error: rate limit", + "API Error: rate limit reached for this model", + "Error: rate_limit_error", + "Rate limit exceeded. Please wait 12 seconds before retrying.", + "429 Too Many Requests", + } { + if _, ok := rateLimitDelay(throttled, nil); !ok { + t.Errorf("genuine throttle not detected: %q", throttled) + } + } + + for _, prose := range []string{ + "PreLLMHook Pipeline (auth, rate-limit, cache check - registration order)", + "The gateway supports rate limiting per virtual key.", + } { + if _, ok := rateLimitDelay(prose, nil); ok { + t.Errorf("model prose about rate limits mistaken for a throttle: %q", prose) + } + } +} + func TestSoftPassCandidateRequiresNonErrorAssistantText(t *testing.T) { okTranscript := `{"type":"text","part":{"type":"text","text":"Waves crash on grey stone, tide pulls secrets."}}` if !isSoftPassCandidate(okTranscript, "opencode") { @@ -227,3 +259,36 @@ func TestSoftPassCandidateRequiresNonErrorAssistantText(t *testing.T) { t.Fatal("expected empty assistant text to be ineligible for soft pass") } } + +// The regression: a model that READ Bifrost's own docs and quoted them made a +// cell wait 3 x 60s and then fail. Discussing rate limits is not being rate +// limited. +func TestRateLimitSignalRequiresFailureContext(t *testing.T) { + quoted := `{"type":"assistant","message":{"content":[{"type":"text","text":` + + `"The pipeline is: TransportPreHook -> PreLLMHook Pipeline (auth, rate-limit, cache check - registration order) -> Provider Queue"}]}}` + + if _, ok := rateLimitDelay(quoted, nil); ok { + t.Error("assistant prose merely mentioning rate-limit must not trigger a retry") + } + if pattern, _, ok := detectError(quoted, nil); ok { + t.Errorf("assistant prose merely mentioning rate-limit must not be an error marker (matched %q)", pattern) + } + + // Genuine throttling must still be caught, on both the plain-stdout channel + // (CLIs print this without a non-zero exit) and the structured one. + for _, genuine := range []string{ + "API Error: 429 rate limit exceeded. Please wait 30 seconds before retrying.", + `{"type":"error","message":"rate_limit_error: too many requests, try again in 5 seconds"}`, + "Rate limit reached for this model. Please wait 12 seconds before retrying.", + } { + if _, ok := rateLimitDelay(genuine, nil); !ok { + t.Errorf("genuine rate limit not detected: %q", genuine) + } + } + + // And the wait is still parsed out of it. + wait, ok := rateLimitDelay("API Error: 429 rate limit exceeded. Please wait 30 seconds before retrying.", nil) + if !ok || wait.Seconds() != 31 { + t.Errorf("expected a 31s wait (30 + buffer), got %s ok=%v", wait, ok) + } +} diff --git a/tests/e2e/clis/clis_test.go b/tests/e2e/clis/clis_test.go index a973fd99889..031e5cd5376 100644 --- a/tests/e2e/clis/clis_test.go +++ b/tests/e2e/clis/clis_test.go @@ -102,6 +102,12 @@ func TestCLIs(t *testing.T) { t.Fatalf("list providers: %v", err) } + // Deliberately after the health check and provider discovery: an + // unreachable gateway skips out above without destroying the previous run's + // report. Deliberately before the CLI loop: every cell calls t.Parallel() + // before runCell, so parked subtests cannot write until this returns. + maybeClearReportsDir(t) + scenarios := allScenarios() modelFilter := os.Getenv("MODEL") // optional substring/exact filter on model ID @@ -403,7 +409,28 @@ func cellBudget(sc scenario) time.Duration { const maxRateLimitRetries = 3 var rateLimitWaitRE = regexp.MustCompile(`(?i)(?:please\s+wait|try\s+again\s+in|retry\s+after)\s+(\d+)\s*(?:seconds?|secs?|s)\b`) -var rateLimitSignalRE = regexp.MustCompile(`(?i)\brate[_ -]?limit\b|\b429\b|too many requests`) +// rateLimitSignalRE requires a failure word alongside the phrase, not the phrase +// alone. +// +// The bare form matched the model's own writing. A cell burned three 60-second +// retries and then failed outright because the assistant had read Bifrost's +// architecture docs and quoted back "PreLLMHook Pipeline (auth, rate-limit, +// cache check - registration order)". Discussing rate limits is not being rate +// limited, and any scenario where a model reads this repo can quote any marker, +// so widening ErrorIgnore per scenario would be endless whack-a-mole. +// +// 429 and "too many requests" stay unqualified: they are unambiguous on their +// own, and both are what a CLI prints when it really is throttled. +var rateLimitSignalRE = regexp.MustCompile( + `(?i)\brate[ _-]?limits?\b[^\n]{0,40}\b(?:exceed|reach|hit|error|throttl|retry|wait)` + + // "error" belongs in the reverse-order branch too: "API Error: rate + // limit" is standard CLI wording, and it puts the failure word ahead of + // the phrase where the forward branch cannot see it. Without it a real + // throttle skipped the retry path and failed the cell outright. + `|(?i)\b(?:exceeded|hit|reached|throttled|error)\b[^\n]{0,40}\brate[ _-]?limit` + + `|\b429\b` + + `|(?i)too many requests` + + `|(?i)rate_limit_error`) func runSingleTurnWithRetry(ctx context.Context, t *testing.T, cli CLI, modelRef string, turn Turn, env []string, mirror io.Writer, effort string) (string, error) { t.Helper() @@ -646,7 +673,7 @@ func containsFold(haystack, needle string) bool { func assertionOutput(cliID, raw string) string { switch cliID { - case "codex", "opencode", "opencode-responses": + case "codex", "opencode", "opencode-responses", "opencode-anthropic": text, sawJSON := extractJSONAssistantText(raw) if sawJSON { return text @@ -718,6 +745,77 @@ type cellResult struct { // subdirectory; see .github/workflows/scripts/test-cli-harness.sh. const reportsDirEnv = "BIFROST_E2E_CLIS_REPORTS_DIR" +// keepReportsEnv preserves the reports directory across a run. See +// maybeClearReportsDir. +const keepReportsEnv = "BIFROST_E2E_CLIS_KEEP_REPORTS" + +var clearReportsOnce sync.Once + +// maybeClearReportsDir empties the reports directory once per run, so +// index.html shows THIS run rather than every cell ever executed locally +// (the aggregate had grown past 300 stale summaries, with no timestamp to tell +// runs apart). +// +// Two cases must NOT clear, and both are load-bearing: +// +// - BIFROST_E2E_CLIS_REPORTS_DIR set. CI owns the directory lifecycle there: +// test-cli-harness.sh rm -rf's each per-label dir up front, and then its +// retry loop re-invokes `go test` against that SAME dir to re-run only the +// failed cells. Clearing on that second invocation would delete the passing +// cells' summaries, breaking count_failed_cells, the aggregate report and +// the "cells produced: 0" guard. +// - BIFROST_E2E_CLIS_KEEP_REPORTS set, for accumulating across several +// filtered local runs on purpose. +// +// TestRenderReport / `make cli-harness-report` never reach this: they re-render +// what is already on disk, which is the whole point of them. +func maybeClearReportsDir(t *testing.T) { + t.Helper() + if os.Getenv(reportsDirEnv) != "" || os.Getenv(keepReportsEnv) != "" { + return + } + clearReportsOnce.Do(func() { + reportMu.Lock() + defer reportMu.Unlock() + if err := clearReportsDir(reportsDir()); err != nil { + t.Logf("could not clear reports dir: %v", err) + } + }) +} + +// clearReportsDir removes previous results from dir, recursively so stale +// per-label subdirectories go too (loadReportResults walks the same tree). +// +// Only the harness's own artefacts are deleted, rather than os.RemoveAll on the +// directory: anything else a developer has parked in reports/ survives, and a +// caller who points reportsDir somewhere unwise cannot lose unrelated data. A +// missing directory is success, not an error. +func clearReportsDir(dir string) error { + if _, err := os.Stat(dir); os.IsNotExist(err) { + return nil + } + return filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + // Surface traversal failures rather than skipping them. A directory + // that cannot be read still holds the previous run's reports, and + // swallowing the error here would let maybeClearReportsDir report a + // successful clear - after which the run renders its report over stale + // cells from an earlier invocation. + if err != nil { + return err + } + if d.IsDir() { + return nil + } + name := d.Name() + if filepath.Ext(name) == ".json" || strings.HasSuffix(name, ".transcript.log") || name == "index.html" { + if rmErr := os.Remove(path); rmErr != nil && !os.IsNotExist(rmErr) { + return rmErr + } + } + return nil + }) +} + // reportsDir is where this invocation writes per-cell summaries. It defaults to // "reports" so every local run and the standalone renderer behave as before. func reportsDir() string { diff --git a/tests/e2e/clis/errordetect_test.go b/tests/e2e/clis/errordetect_test.go index 010e2869870..3164e1fc021 100644 --- a/tests/e2e/clis/errordetect_test.go +++ b/tests/e2e/clis/errordetect_test.go @@ -18,7 +18,11 @@ var errorPatterns = []*regexp.Regexp{ // Plain-prose error markers that real CLIs print on failure. regexp.MustCompile(`(?i)API Error`), regexp.MustCompile(`(?i)Request failed`), - regexp.MustCompile(`(?i)\brate[_ -]?limit\b`), + // Requires a failure word near the phrase. A bare `\brate[_ -]?limit\b` + // matched the model's own prose: a cell failed because it read Bifrost's + // architecture docs and quoted "PreLLMHook Pipeline (auth, rate-limit, cache + // check)". Naming the concept is not hitting it. + rateLimitSignalRE, regexp.MustCompile(`\b(?:401|403|429|5\d{2})\b\s+(?i)(?:error|status)`), regexp.MustCompile(`(?i)authentication failed`), regexp.MustCompile(`(?i)invalid[_ ]api[_ ]key`), diff --git a/tests/e2e/clis/matrix_test.go b/tests/e2e/clis/matrix_test.go index 0dea4dfe03f..354baca3bf0 100644 --- a/tests/e2e/clis/matrix_test.go +++ b/tests/e2e/clis/matrix_test.go @@ -89,6 +89,15 @@ func isBedrockNovaModel(modelID string) bool { return strings.Contains(modelID, "amazon.nova") } +// isAnthropicFamilyModel reports whether a model can be served over the +// Anthropic Messages wire, regardless of which cloud fronts it. Every catalog in +// this file spells Claude models with a "claude" substring -- native +// (claude-opus-5), Bedrock (global.anthropic.claude-opus-5), Azure and Vertex +// (claude-opus-5, claude-haiku-4-5@20251001) -- so one check covers all four. +func isAnthropicFamilyModel(modelID string) bool { + return strings.Contains(modelID, "claude") +} + // Provider describes a Bifrost-configured provider and its model catalog. // // Models lists the top chat-capable model IDs we want the harness to exercise. @@ -241,6 +250,43 @@ var clis = map[string]CLI{ PreLaunch: opencodeResponsesPreLaunch, MultiTurnDriver: opencodeResumeDriver, }, + // Third wire format for the same binary: the Anthropic Messages API, via + // @ai-sdk/anthropic against Bifrost's /anthropic path, with a Bedrock model. + // + // This is the shape reported from the field ("Anthropic native" in OpenCode's + // own status line, custom endpoint on /anthropic), and it is the only one of + // the three that replays reasoning at all. That is a protocol constraint, not + // a client preference: the Anthropic API REQUIRES an assistant turn's thinking + // blocks to be sent back verbatim when that turn contains tool_use. The + // OpenAI-Responses SDK is free to rebuild history as plain prose and, as the + // gateway logs confirm, does exactly that - which is why opencode-responses + // passes even with tool calls, and why this variant exists. + // + // Anthropic wire in, Bedrock model out, is precisely the conversion that + // fails: Bifrost lowers the replayed thinking block into a Responses + // reasoning item, then into a Bedrock reasoning block with no text. + "opencode-anthropic": { + ID: "opencode-anthropic", + Binary: "opencode", + InstallPkg: "opencode-ai", + BasePath: "/anthropic", + BaseURLEnv: "ANTHROPIC_BASE_URL", + APIKeyEnv: "ANTHROPIC_API_KEY", + ExtraEnv: map[string]string{ + "OPENCODE_DISABLE_AUTOUPDATE": "1", + }, + SingleTurnArgs: func(model, prompt string, extra []string) []string { + args := []string{"run", "--format", "json"} + if model != "" { + args = append(args, "--model", opencodeModelRef(model)) + } + args = append(args, extra...) + args = append(args, prompt) + return args + }, + PreLaunch: opencodeAnthropicPreLaunch, + MultiTurnDriver: opencodeResumeDriver, + }, } // Anthropic โ€” sourced from https://platform.claude.com/docs/en/about-claude/models/overview @@ -514,6 +560,19 @@ func opencodeResponsesPreLaunch(baseURL, apiKey, model string) ([]string, func() ) } +// opencodeAnthropicPreLaunch wires OpenCode to Bifrost over /anthropic/v1/messages. +// +// baseURL gets "/v1" appended for the same reason the Responses variant does: +// @ai-sdk/anthropic posts to /messages, and Anthropic's own default +// baseURL ends in /v1 - so without it the request lands on /anthropic/messages +// instead of Bifrost's canonical /anthropic/v1/messages route. +func opencodeAnthropicPreLaunch(baseURL, apiKey, model string) ([]string, func(), error) { + return opencodeWriteConfig( + strings.TrimSuffix(strings.TrimSpace(baseURL), "/")+"/v1", + apiKey, model, "@ai-sdk/anthropic", + ) +} + func opencodeWriteConfig(baseURL, apiKey, model, npmPkg string) ([]string, func(), error) { model = strings.TrimSpace(model) if model == "" { diff --git a/tests/e2e/clis/reasoningreplay_test.go b/tests/e2e/clis/reasoningreplay_test.go index 220d2d3e845..a95037481ce 100644 --- a/tests/e2e/clis/reasoningreplay_test.go +++ b/tests/e2e/clis/reasoningreplay_test.go @@ -32,27 +32,57 @@ func TestEnvNamesOmitsValues(t *testing.T) { // way that silently stops happening, leaving a scenario that runs, passes, and // verifies nothing. -// opencode-responses is the only cell on Bifrost's /v1/responses path for -// Anthropic-on-Bedrock. If this gate widens, it duplicates coverage; if it -// narrows to nothing, the defect class goes untested with no failing test to -// say so. -func TestOpenCodeResponsesIsScopedToBedrockAnthropic(t *testing.T) { +// Bifrost converts requests separately per wire format, so a wire format with +// no cell behind it ships unexercised - which is how the Bedrock reasoning-replay +// defect reached production while this harness was green. These gates are what +// guarantee all three wires are reachable; if one narrows to nothing, the defect +// class it covers goes untested with no failing test to say so. +func TestOpenCodeVariantsCoverBothEndpoints(t *testing.T) { anthropicOnBedrock := ModelInfo{ID: "global.anthropic.claude-opus-5", AdaptiveThinking: true} novaOnBedrock := ModelInfo{ID: "us.amazon.nova-2-lite-v1:0"} + claudeNative := ModelInfo{ID: "claude-sonnet-5", AdaptiveThinking: true} + claudeOnVertex := ModelInfo{ID: "claude-haiku-4-5@20251001", ExtendedThinking: true} gpt := ModelInfo{ID: "gpt-5.5", AdaptiveThinking: true} - if !supportsCLIProviderModel("opencode-responses", "bedrock", anthropicOnBedrock) { - t.Error("opencode-responses must run against Anthropic-on-Bedrock - that is the reported defect's path") - } - if supportsCLIProviderModel("opencode-responses", "bedrock", novaOnBedrock) { - t.Error("opencode-responses should not run against non-Anthropic Bedrock models") + // /openai/v1/responses -- unrestricted, mirroring the chat/completions + // variant, so the Responses conversion is covered on every provider rather + // than only the one a defect happened to be reported on. + for _, provider := range []string{"openai", "azure", "bedrock", "anthropic"} { + model := gpt + if provider == "bedrock" { + model = anthropicOnBedrock + } + if !supportsCLIProviderModel("opencode-responses", provider, model) { + t.Errorf("opencode-responses must reach %s to cover the /openai responses wire there", provider) + } } - for _, provider := range []string{"openai", "anthropic", "azure", "gemini", "vertex"} { - if supportsCLIProviderModel("opencode-responses", provider, gpt) { - t.Errorf("opencode-responses should be scoped to bedrock, but %s was allowed", provider) + + // /anthropic/v1/messages -- gated on the model family, not the provider, + // because the Anthropic wire can only carry Claude models but every cloud + // fronting Claude should be covered. + for _, tc := range []struct { + provider string + model ModelInfo + }{ + {"anthropic", claudeNative}, + {"bedrock", anthropicOnBedrock}, + {"azure", claudeNative}, + {"vertex", claudeOnVertex}, + } { + if !supportsCLIProviderModel("opencode-anthropic", tc.provider, tc.model) { + t.Errorf("opencode-anthropic must reach %s/%s to cover the /anthropic wire there", tc.provider, tc.model.ID) } } - // The chat/completions variant must be untouched by that scoping. + // Non-Claude models cannot be served over the Anthropic Messages wire. + if supportsCLIProviderModel("opencode-anthropic", "bedrock", novaOnBedrock) { + t.Error("opencode-anthropic must not run against a non-Anthropic model") + } + if supportsCLIProviderModel("opencode-anthropic", "openai", gpt) { + t.Error("opencode-anthropic must not run against a GPT model") + } + + // /openai/v1/chat/completions -- the default variant is untouched by any of + // the above scoping. if !supportsCLIProviderModel("opencode", "openai", gpt) { t.Error("the chat/completions opencode variant must still run against openai") } diff --git a/tests/e2e/clis/scenarios_test.go b/tests/e2e/clis/scenarios_test.go index 1978d08356e..18423eafe48 100644 --- a/tests/e2e/clis/scenarios_test.go +++ b/tests/e2e/clis/scenarios_test.go @@ -42,8 +42,12 @@ func supportsCLIProviderModel(cliID, providerID string, model ModelInfo) bool { // cells rather than the whole matrix keeps it from duplicating coverage the // chat/completions variant already provides, at five models rather than // every provider's catalogue. - if cliID == "opencode-responses" { - return providerID == "bedrock" && isBedrockAnthropicModel(model.ID) + // The Anthropic Messages wire can only carry Anthropic-family models, whoever + // serves them -- native, Bedrock, Azure or Vertex. Gating on the model family + // rather than on the provider is what gives /anthropic coverage across every + // cloud that fronts Claude, instead of only the one a defect was reported on. + if cliID == "opencode-anthropic" { + return isAnthropicFamilyModel(model.ID) } return true } @@ -73,6 +77,7 @@ func allScenarios() []scenario { conversationRefinementScenario(), conversationRoleStabilityScenario(), reasoningReplayScenario(), + reasoningToolReplayScenario(), subagentDelegationScenario(), imageQAScenario(), pdfQAScenario(), @@ -615,7 +620,72 @@ func validateReplayRecall(output string) error { return nil } -// 10 subagent-delegation โ€” the CLI's own subagent-delegation tool (Claude +// 10 reasoning-tool-replay โ€” a thinking model that also calls tools, then has +// to answer from the resulting history. +// +// This is the shape reported from the field, and it is deliberately distinct +// from reasoning-replay above. The difference is tool calls, and it turns out to +// be the whole difference: +// +// - reasoning-replay is five turns of pure arithmetic. It passes. +// - This is the same models over the same wire path, but each assistant turn +// carries reasoning AND a tool call. Reported failing at `messages.2` โ€” the +// FIRST assistant turn, from a one-word prompt. +// +// Why tools change it: a client can paraphrase a plain assistant answer, but a +// turn containing a tool call has to be replayed verbatim to keep the +// tool_use/tool_result pairing intact. That verbatim replay is what puts the +// reasoning item back on the wire, and on the Bedrock Responses path a reasoning +// item that arrives with an empty summary and only encrypted_content converts to +// a block with no `text` โ€” which Bedrock rejects. +// +// It also explains the reported error's index pattern (14, 16, then 24, then 30, +// 32, ...): the gaps are the assistant turns that made no tool call, so carried +// no replayable reasoning. A scenario without tools reproduces nothing, which is +// exactly what reasoning-replay demonstrated. +// +// Turn 2 is the one under test: it forbids re-reading, so the model can only +// answer from a history that includes turn 1's reasoning + tool call. +func reasoningToolReplayScenario() scenario { + fixture, _ := filepath.Abs("fixtures/sample.txt") + return scenario{ + ID: "reasoning-tool-replay", + ModelKind: "reasoning", + Supports: supportsReasoningReplay, + ErrorIgnore: []string{"FILE_FIXTURE_73129", "TOOLREPLAY"}, + Turns: []Turn{ + { + Send: "Think carefully about this before answering, then use your file tool to read " + + fixture + ". Report the hidden verification token it contains.", + AssertText: []string{"FILE_FIXTURE_73129"}, + AssertNotText: []string{ + "don't have access to file", "cannot read files", "unable to read", + }, + Timeout: 180 * time.Second, + }, + { + // The replay turn. No re-read allowed, so the answer must come from + // the turn-1 history - which the client can only send back by + // replaying the assistant turn (reasoning + tool call) verbatim. + Send: "Without reading the file again, which capital city did that file name, and what " + + "did it say the square root of 144 is?", + AssertTextFold: []string{"Paris"}, + AssertText: []string{"12"}, + Timeout: 180 * time.Second, + }, + { + // A second tool call on top of an already-replayed history, so the + // reasoning items accumulate the way they do in a real session. + Send: "Now use your shell tool to run `printf TOOLREPLAY` and reply with its exact " + + "output followed by the verification token from the first step.", + AssertText: []string{"TOOLREPLAY", "FILE_FIXTURE_73129"}, + Timeout: 180 * time.Second, + }, + }, + } +} + +// 11 subagent-delegation โ€” the CLI's own subagent-delegation tool (Claude // Code's Agent/Task tool, or Codex's spawn_agent collab-tool) spawns a // nested subagent that makes its own separate LLM call. Proves that traffic // shape (a completion whose tool call triggers an additional completion with @@ -679,7 +749,7 @@ func supportsSubagentDelegation(cliID, providerID string, model ModelInfo) bool return ok && model.ID == prov.chatModel().ID } -// 11 image-qa โ€” the model receives a genuine image attachment (not just a +// 12 image-qa โ€” the model receives a genuine image attachment (not just a // file path) and must answer questions requiring real visual/OCR // understanding: a token rendered as text, a fact written in the image, and // the color of a drawn shape. Claude has no CLI-level image-attach flag -- @@ -725,7 +795,7 @@ func supportsImageQA(cliID, providerID string, m ModelInfo) bool { return supportsCLIProviderModel(cliID, providerID, m) } -// 12 pdf-qa โ€” the model reads a real PDF document (not a .txt file) and +// 13 pdf-qa โ€” the model reads a real PDF document (not a .txt file) and // must answer questions from its content, exercising genuine document // parsing rather than plain-text file reading. Claude only: PDF attachment // has no CLI flag on either CLI, and Claude's Read tool natively handles diff --git a/tests/e2e/clis/waitdelayunix_test.go b/tests/e2e/clis/waitdelayunix_test.go index 4ebb4bfb4a0..3c9e78024f5 100644 --- a/tests/e2e/clis/waitdelayunix_test.go +++ b/tests/e2e/clis/waitdelayunix_test.go @@ -9,7 +9,9 @@ package clis import ( "bytes" "errors" + "os" "os/exec" + "path/filepath" "strings" "syscall" "testing" @@ -171,6 +173,35 @@ func TestLiveCommandIsSignalled(t *testing.T) { untrackCmd(cmd) } +// A cleanup that could not read the tree must say so. Swallowing the traversal +// error leaves the previous run's reports/*.json in place while the caller logs +// success, and the next run then renders a report over stale cells - the one +// outcome clearing the directory exists to prevent. +// +// Unix-only: it needs a directory the process genuinely cannot read, and +// chmod 000 does not stop root, so this is skipped when running as root. +func TestClearReportsDirReportsTraversalFailure(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root: chmod 000 does not deny traversal") + } + dir := t.TempDir() + blocked := filepath.Join(dir, "blocked") + if err := os.Mkdir(blocked, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(blocked, "cell.json"), []byte("{}"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + if err := os.Chmod(blocked, 0o000); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(blocked, 0o755) }) + + if err := clearReportsDir(dir); err == nil { + t.Error("an unreadable reports directory was reported as successfully cleared") + } +} + // An ordinary failure carries no ErrWaitDelay and must pass through untouched. func TestWaitErrPassesThroughOrdinaryFailures(t *testing.T) { cmd := exec.CommandContext(t.Context(), "sh", "-c", "exit 4")