Skip to content

fix: thinking enabled + tool_use first block#577

Merged
ding113 merged 2 commits intodevfrom
fix/thinking-disable-on-tooluse-first
Jan 10, 2026
Merged

fix: thinking enabled + tool_use first block#577
ding113 merged 2 commits intodevfrom
fix/thinking-disable-on-tooluse-first

Conversation

@ding113
Copy link
Owner

@ding113 ding113 commented Jan 10, 2026

问题:当 /v1/messages 请求顶层 thinking 启用,但最后一条 assistant message 的 content 以 tool_use 开头(缺少 thinking/redacted_thinking 前缀)时,部分 Claude 兼容渠道会返回 400:Expected thinking/redacted_thinking but found tool_use。

修复:

  • thinking signature rectifier 新增触发器识别该错误,并在同供应商重试前删除本次请求的顶层 thinking 字段(不伪造 thinking block)。
  • 默认 error rule 增加兜底规则,将该类错误转换为可操作的友好提示(回传上一轮 thinking/redacted_thinking 或关闭 thinking)。

验证:

  • bun run lint
  • bun run typecheck
  • bun run test
  • bun run test:coverage
  • bun run build
  • bun run test:e2e(以 next start 方式在 13500 端口运行)

Greptile Overview

Greptile Summary

Overview

This PR fixes a specific error scenario where Claude-compatible channels return a 400 error when thinking is enabled at the top level but the last assistant message's content starts with tool_use instead of a thinking/redacted_thinking block.

Key Changes

Core Fix (thinking-signature-rectifier.ts)

  • Added new trigger type assistant_message_must_start_with_thinking to detect the specific error pattern
  • Enhanced error detection regex to match: "Expected `thinking` or `redacted_thinking`, but found `tool_use`"
  • Implemented fallback logic that removes the top-level thinking field when:
    1. Thinking is enabled
    2. Last assistant message contains tool_use blocks
    3. First block is NOT thinking/redacted_thinking
  • Logic properly handles edge cases: null/empty content, no assistant messages, multiple assistants

Error Rule (error-rules.ts)

  • Added default error rule (priority 69) for "must start with a thinking block" pattern
  • Provides actionable Chinese guidance: suggests either passing through the previous thinking block with signature/data OR disabling thinking

Runtime Fix (instrumentation.ts)

  • Declared runtime = "nodejs" to ensure Node.js capabilities are available
  • Converted closeRedis import to dynamic import in shutdown handler to prevent Edge runtime conflicts

Cascading Fix Behavior
The rectification is designed to handle multiple related issues in one pass:

  1. First removes invalid thinking blocks and signature fields
  2. Then checks if tool_use is now the first block and removes top-level thinking if needed
    This cascading behavior is intentional and well-tested (see test "移除 thinking block 后若 tool_use 置顶且 thinking 仍启用,应同时关闭 thinking 再重试")

Test Coverage

  • ✅ Unit tests for trigger detection with actual error message
  • ✅ Unit tests for thinking field removal logic
  • ✅ Integration tests in ProxyForwarder with retry behavior validation
  • ✅ Tests for cascading fix (invalid signature → thinking removed → top-level thinking disabled)
  • ✅ Tests for no-op scenarios (no applicable content to rectify)
  • ✅ Error rule validation tests
  • ✅ New emit-event tests for runtime-specific behavior

Architecture

The fix maintains the existing retry-on-same-provider pattern without polluting the circuit breaker or triggering provider switches. The rectification is applied in-place to the request message before retry, and audit trails are properly recorded in specialSettings.

Confidence Score: 5/5

  • This PR is safe to merge with high confidence - it addresses a specific well-defined error scenario with comprehensive test coverage and proper error handling
  • Score of 5 reflects excellent code quality: (1) Thorough analysis reveals sound logic that properly handles all edge cases (null/empty content, no assistant messages, multiple assistants); (2) Comprehensive test coverage including unit tests, integration tests, and cascading fix scenarios; (3) Proper type safety with union type updates; (4) Well-documented code with clear Chinese comments explaining the fix rationale; (5) Follows existing patterns for retry-on-same-provider without polluting circuit breaker; (6) Runtime fix prevents Edge compatibility issues; (7) Provides actionable error messages to users
  • No files require special attention - all changes are well-implemented with appropriate test coverage

Important Files Changed

File Analysis

Filename Score Overview
src/app/v1/_lib/proxy/thinking-signature-rectifier.ts 5/5 Core logic for detecting and rectifying thinking signature errors; adds new trigger type and fallback mechanism to remove top-level thinking field when tool_use blocks lack thinking prefix
src/instrumentation.ts 5/5 Properly declares Node.js runtime and converts closeRedis to dynamic import to prevent Edge runtime compatibility issues
src/repository/error-rules.ts 5/5 Adds new default error rule for thinking/tool_use conflict with clear actionable guidance in Chinese; properly configured with priority 69
tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts 5/5 Integration tests added for new error scenario and cascading fix; thoroughly validates retry behavior and thinking field removal

Sequence Diagram

sequenceDiagram
    participant Client
    participant ProxyForwarder
    participant ThinkingRectifier
    participant AnthropicProvider
    participant ErrorRules
    
    Client->>ProxyForwarder: POST /v1/messages<br/>{thinking: enabled, messages: [assistant with tool_use]}
    ProxyForwarder->>AnthropicProvider: Forward request
    AnthropicProvider-->>ProxyForwarder: 400: Expected thinking/redacted_thinking,<br/>but found tool_use
    
    ProxyForwarder->>ThinkingRectifier: detectThinkingSignatureRectifierTrigger(errorMsg)
    ThinkingRectifier-->>ProxyForwarder: "assistant_message_must_start_with_thinking"
    
    ProxyForwarder->>ProxyForwarder: Check enableThinkingSignatureRectifier
    
    ProxyForwarder->>ThinkingRectifier: rectifyAnthropicRequestMessage(message)
    
    Note over ThinkingRectifier: Step 1: Remove thinking/redacted_thinking blocks<br/>and signature fields from message content
    
    Note over ThinkingRectifier: Step 2: Check if last assistant has tool_use<br/>and first block is not thinking
    
    ThinkingRectifier->>ThinkingRectifier: Delete top-level thinking field
    ThinkingRectifier-->>ProxyForwarder: {applied: true, removedThinkingBlocks: X}
    
    ProxyForwarder->>ProxyForwarder: Add specialSettings audit trail
    ProxyForwarder->>ProxyForwarder: Update message_request_details
    
    ProxyForwarder->>AnthropicProvider: Retry with rectified request<br/>(no top-level thinking)
    AnthropicProvider-->>ProxyForwarder: 200 OK {content: [...]}
    ProxyForwarder-->>Client: Success response
    
    alt If retry also fails
        AnthropicProvider-->>ProxyForwarder: Still 400
        ProxyForwarder->>ErrorRules: Match error pattern
        ErrorRules-->>Client: Override with friendly error message
    end
Loading

- Extend thinking signature rectifier to detect tool_use-first error and drop top-level thinking on retry
- Add default error rule guidance for thinking/tool_use mismatch
- Add unit tests for rectifier, forwarder retry path, and default rule sync
@coderabbitai
Copy link

coderabbitai bot commented Jan 10, 2026

📝 Walkthrough

Walkthrough

新增并扩展了 thinking signature rectifier 的检测与修复路径,新增 error-rules 条目以识别“必须以 thinking 块开头”类错误,更新相关类型与运行时导出,并添加/调整多处单元测试覆盖这些行为。

Changes

Cohort / File(s) 变更摘要
Thinking Signature Rectifier 核心
src/app/v1/_lib/proxy/thinking-signature-rectifier.ts, src/types/special-settings.ts
新增触发器值 "assistant_message_must_start_with_thinking";detect 增加早期检测路径识别缺失 thinking 前缀的错误;rectify 在特定重试场景下移除顶级 thinking 字段并标记已应用。类型定义同步扩展。
Thinking Signature Rectifier 测试
src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts, tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts
新增/扩展测试:检测当 assistant 首块为 tool_use 时触发新 trigger;验证在 thinking 启用但 continuation 缺少 thinking 前缀时的修复行为及重试流程。
Error Rules 配置
src/repository/error-rules.ts
在 DEFAULT_ERROR_RULES 中新增两条与 thinking/tool_use 相关的默认规则(包含模式 "must start with a thinking block" 与正则匹配),用于返回 thinking_error 类型的覆盖响应。注意这些规则在数组中出现了重复插入。
Error Rules 测试
tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
新增单元测试,验证通过 syncDefaultErrorRules 插入含 "must start with a thinking block" 的默认规则及其 Claude 风格的 overrideResponse 结构。
运行时与资源清理
src/instrumentation.ts
新增并导出 runtime = "nodejs";将 closeRedis 的静态顶层导入改为在 shutdown 处理器内的动态导入(lazy import)并保留错误处理。
Emit / Session 测试调整
tests/unit/lib/emit-event.test.ts, tests/unit/lib/session-manager-helpers.test.ts
新增 emit-event 的完整测试套件(区分 nodejs/edge 行为);session-manager 测试重构为通过专用过滤器断言 header 解析警告。

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 分钟

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed 标题明确总结了主要改动:修复当thinking启用但最后一条assistant message以tool_use开头时出现的问题,与PR的核心修复完全相关。
Description check ✅ Passed 描述详细说明了问题、修复方案、涉及的文件以及验证步骤,与整个变更集密切相关,信息充分。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/thinking-disable-on-tooluse-first

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

此拉取请求旨在增强系统对 Claude API 特定错误模式的鲁棒性。它通过引入智能的请求整流逻辑和用户友好的错误提示,解决了在启用 thinking 功能时,如果 assistant 消息的工具使用块未正确前置 thinking 块而导致的 400 错误。这提高了用户体验,并减少了因 API 行为差异而导致的请求失败。

Highlights

  • 修复 Claude 400 错误: 解决了当顶层 thinking 启用,但最后一条 assistant 消息的 content 以 tool_use 开头(缺少 thinking/redacted_thinking 前缀)时,部分 Claude 兼容渠道返回 400 错误的问题。
  • 新增整流触发器: 在 thinking-signature-rectifier 中新增了一个触发器,用于识别 'thinking 启用但 assistant 首块为 tool_use' 这种特定错误模式。
  • 请求重试机制: 当检测到上述错误时,系统会在同供应商重试前删除本次请求的顶层 thinking 字段,以避免再次触发 400 错误。
  • 默认错误规则: 增加了一条默认错误规则,将此类错误转换为用户友好的提示,建议用户回传上一轮 thinking/redacted_thinking 或关闭 thinking。
  • 运行时配置优化: 在 src/instrumentation.ts 中明确指定了 nodejs 运行时,并优化了 closeRedis 的导入方式,以确保在服务器关闭时正确清理资源。
  • 新增单元测试: 为 emit-event 和新的默认错误规则添加了单元测试,确保功能按预期工作,并覆盖了不同运行时环境下的行为。
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions github-actions bot added bug Something isn't working area:Error Rule labels Jan 10, 2026
@github-actions
Copy link
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

这个 PR 修复了一个在启用 thinkingtool_use 时与 Claude 兼容渠道交互的边缘 case。修复方案很全面,包括了两个方面:

  1. 在代理层通过 thinking-signature-rectifier 增加了对错误的识别和自动修复逻辑,即在重试前移除顶层的 thinking 字段,这能有效避免连续的 400 错误。
  2. error-rules 中增加了一条新的默认规则,当上游返回特定错误时,能给用户一个清晰、可操作的错误提示。

代码变更逻辑清晰,并且附带了非常详尽的单元测试和集成测试,覆盖了新增长的逻辑和相关的边缘场景。特别是 proxy-forwarder-thinking-signature-rectifier.test.ts 中对组合场景的测试,确保了整流器在不同情况下的行为符合预期。

整体来看,这是一个高质量的修复,不仅解决了问题,还提升了代码的可维护性和健壮性。

Comment on lines 157 to 176
if (lastAssistantContent && lastAssistantContent.length > 0) {
const hasToolUse = lastAssistantContent.some((block) => {
if (!block || typeof block !== "object") return false;
return (block as Record<string, unknown>).type === "tool_use";
});

const firstBlock = lastAssistantContent[0];
const firstBlockType =
firstBlock && typeof firstBlock === "object"
? (firstBlock as Record<string, unknown>).type
: null;

const missingThinkingPrefix =
firstBlockType !== "thinking" && firstBlockType !== "redacted_thinking";

if (hasToolUse && missingThinkingPrefix && "thinking" in message) {
delete (message as any).thinking;
applied = true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

为了提高代码的可读性和效率,可以稍微重构这部分逻辑。

当前的实现会无条件地遍历 lastAssistantContent 来检查 hasToolUse,即使 missingThinkingPrefixfalse。我们可以调整一下顺序,仅在确实缺少 thinking 前缀时,才去检查是否存在 tool_use

此外,if 条件中的 "thinking" in message 检查是多余的,因为代码块已经处于 if (thinkingEnabled) 的保护之下。

建议修改如下,使逻辑更清晰,并避免不必要的操作。

    if (lastAssistantContent && lastAssistantContent.length > 0) {
      const firstBlock = lastAssistantContent[0];
      const firstBlockType =
        firstBlock && typeof firstBlock === "object"
          ? (firstBlock as Record<string, unknown>).type
          : null;

      const missingThinkingPrefix =
        firstBlockType !== "thinking" && firstBlockType !== "redacted_thinking";

      // 仅在缺少 thinking 前缀时才需要进一步检查
      if (missingThinkingPrefix) {
        const hasToolUse = lastAssistantContent.some((block) => {
          if (!block || typeof block !== "object") return false;
          return (block as Record<string, unknown>).type === "tool_use";
        });

        if (hasToolUse) {
          delete (message as any).thinking;
          applied = true;
        }
      }
    }

@github-actions github-actions bot added the size/M Medium PR (< 500 lines) label Jan 10, 2026
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @src/repository/error-rules.ts:
- Around line 660-680: The rule object with pattern "must start with a thinking
block" embeds a hard-coded Chinese string in overrideResponse.message; replace
that literal with an i18n key (or a translations object) and supply localized
strings for zh-CN, en, ja, ko, de, then update any consumer (e.g.,
edit-rule-dialog.tsx) to render the translated text by looking up that key/value
instead of using the hard-coded message; specifically modify the rule entry's
overrideResponse to reference a messageKey (or messages map) and add the
five-language translations so the UI can display the proper localized message.
🧹 Nitpick comments (3)
tests/unit/lib/emit-event.test.ts (1)

38-64: 测试覆盖了 Node.js runtime 下的预期行为。

测试验证了:

  • emitErrorRulesUpdated 触发本地事件并广播缓存失效
  • emitSensitiveWordsUpdated 仅触发本地事件(无缓存失效)
  • emitRequestFiltersUpdated 触发本地事件并广播缓存失效

建议在每个测试的动态导入前添加 vi.resetModules() 以确保模块状态隔离,避免跨测试污染。

♻️ 可选优化:确保模块隔离
  test("emitErrorRulesUpdated:Node.js runtime 下应触发本地事件并广播缓存失效", async () => {
+   vi.resetModules();
    const { emitErrorRulesUpdated } = await import("@/lib/emit-event");
    await emitErrorRulesUpdated();
tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts (1)

208-260: 测试用例覆盖了“tool_use 置顶 + thinking 启用”重试路径,建议降低对上游报错文案的耦合度。

Line 227 的错误字符串过长且过度依赖完整文案;这里仅需包含能命中检测逻辑的关键片段(例如 “must start with a thinking block” 或 “Expected thinking ... found tool_use”)即可,避免未来上游文案微调导致用例脆弱、也提升可读性。

建议改法(缩短并聚焦触发关键词)
-      throw new ProxyError(
-        "messages.69.content.0.type: Expected `thinking` or `redacted_thinking`, but found `tool_use`. When `thinking` is enabled, a final `assistant` message must start with a thinking block (preceeding the lastmost set of `tool_use` and `tool_result` blocks). To avoid this requirement, disable `thinking`.",
+      throw new ProxyError(
+        "Expected `thinking` or `redacted_thinking`, but found `tool_use`; must start with a thinking block",
         400,
         {
           body: "",
           providerId: 1,
           providerName: "anthropic-1",
         }
       );
src/app/v1/_lib/proxy/thinking-signature-rectifier.ts (1)

18-41: 检测规则建议再“抗变体”一点(但当前实现已可用)。

目前同时覆盖关键短语与 “Expected thinking/redacted_thinking ... found tool_use” 正则,方向正确;若线上渠道存在更多变体(例如无 backtick、或 tool_use 前后有额外字段路径),建议后续补充 1-2 个最小化样例到单测中以锁定行为。

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to Reviews > Disable Cache setting

📥 Commits

Reviewing files that changed from the base of the PR and between be460ab and 4b7d60a.

📒 Files selected for processing (9)
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
  • src/instrumentation.ts
  • src/repository/error-rules.ts
  • src/types/special-settings.ts
  • tests/unit/lib/emit-event.test.ts
  • tests/unit/lib/session-manager-helpers.test.ts
  • tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts
  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

No emoji characters in any code, comments, or string literals

Files:

  • src/types/special-settings.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
  • tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts
  • src/repository/error-rules.ts
  • src/instrumentation.ts
  • tests/unit/lib/emit-event.test.ts
  • tests/unit/lib/session-manager-helpers.test.ts
  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
**/*.{ts,tsx,jsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,jsx,js}: All user-facing strings must use i18n (5 languages supported: zh-CN, en, ja, ko, de). Never hardcode display text
Use path alias @/ to map to ./src/
Use Biome for code formatting with configuration: double quotes, trailing commas, 2-space indent, 100 character line width
Prefer named exports over default exports
Use next-intl for internationalization
Use Next.js 16 App Router with Hono for API routes

Files:

  • src/types/special-settings.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
  • tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts
  • src/repository/error-rules.ts
  • src/instrumentation.ts
  • tests/unit/lib/emit-event.test.ts
  • tests/unit/lib/session-manager-helpers.test.ts
  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Place unit tests in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts

Files:

  • src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts
  • tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts
  • tests/unit/lib/emit-event.test.ts
  • tests/unit/lib/session-manager-helpers.test.ts
  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
🧠 Learnings (3)
📚 Learning: 2026-01-05T03:01:39.354Z
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 539
File: src/types/user.ts:158-170
Timestamp: 2026-01-05T03:01:39.354Z
Learning: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.

Applied to files:

  • src/types/special-settings.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
  • src/repository/error-rules.ts
  • src/instrumentation.ts
📚 Learning: 2026-01-10T06:19:58.167Z
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 573
File: src/actions/model-prices.ts:275-335
Timestamp: 2026-01-10T06:19:58.167Z
Learning: In the `ding113/claude-code-hub` repository, Server Actions (files under `src/actions/*.ts`) currently return hardcoded Chinese error messages directly. This is a codebase-wide architectural decision that applies to all action files (e.g., model-prices.ts, users.ts, system-config.ts). Changing this pattern requires a coordinated i18n refactor across all Server Actions, which should be handled as a separate unified task rather than piecemeal changes in individual PRs.

Applied to files:

  • src/repository/error-rules.ts
📚 Learning: 2026-01-07T17:05:36.362Z
Learnt from: CR
Repo: ding113/claude-code-hub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T17:05:36.362Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Place unit tests in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts

Applied to files:

  • tests/unit/lib/emit-event.test.ts
  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
🧬 Code graph analysis (4)
src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts (1)
src/app/v1/_lib/proxy/thinking-signature-rectifier.ts (2)
  • detectThinkingSignatureRectifierTrigger (18-59)
  • rectifyAnthropicRequestMessage (70-185)
src/instrumentation.ts (10)
src/app/api/actions/[...route]/route.ts (1)
  • runtime (45-45)
src/app/v1/[...route]/route.ts (1)
  • runtime (16-16)
src/app/api/leaderboard/route.ts (1)
  • runtime (31-31)
src/app/api/version/route.ts (1)
  • runtime (7-7)
src/app/api/proxy-status/route.ts (1)
  • runtime (7-7)
src/app/api/admin/database/export/route.ts (1)
  • runtime (7-7)
src/app/api/admin/database/import/route.ts (1)
  • runtime (13-13)
src/app/api/auth/login/route.ts (1)
  • runtime (6-6)
src/app/api/admin/log-level/route.ts (1)
  • runtime (5-5)
src/app/api/admin/log-cleanup/manual/route.ts (1)
  • runtime (8-8)
tests/unit/lib/session-manager-helpers.test.ts (1)
src/lib/session-manager.ts (1)
  • parseHeaderRecord (2010-2010)
tests/unit/repository/error-rules-default-thinking-tooluse.test.ts (1)
src/repository/error-rules.ts (1)
  • syncDefaultErrorRules (816-896)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: 🌐 API Tests
  • GitHub Check: pr-review
  • GitHub Check: pr-description
  • GitHub Check: Docker Build Test
  • GitHub Check: check-codex-status
  • GitHub Check: dev-build-deploy
🔇 Additional comments (12)
src/instrumentation.ts (2)

6-8: LGTM!

runtime = "nodejs" 导出与代码库中其他路由文件保持一致(如 src/app/v1/[...route]/route.tssrc/app/api/actions/[...route]/route.ts 等)。这确保了 instrumentation hook 在正确的运行时环境中执行。


82-89: LGTM!

closeRedis 改为懒加载动态导入是合理的优化。这与文件中其他 shutdown handler 内的动态导入模式一致(如 stopMessageRequestWriteBuffer),避免了在 shutdown 阶段可能出现的模块解析问题。

tests/unit/lib/session-manager-helpers.test.ts (2)

4-8: LGTM! 测试重构提升了可维护性。

引入 PARSE_HEADER_RECORD_WARN_MESSAGE 常量和 getParseHeaderRecordWarnCalls() 辅助函数是良好的实践:

  • 避免魔法字符串,确保一致性
  • 过滤器模式精确匹配目标警告,减少其他 warn 调用的干扰
  • 提高测试的可读性和可维护性

67-72: 断言逻辑清晰。

使用 calls[0] ?? [] 进行安全解构是良好的防御性编程,避免空数组时的 undefined 错误。

tests/unit/repository/error-rules-default-thinking-tooluse.test.ts (2)

68-90: LGTM! 测试覆盖了新规则的关键属性。

测试设计合理:

  • 验证 pattern、matchType、category 等核心字段
  • 检查 overrideResponse 包含可操作的用户提示(tool_resultsignature关闭
  • Mock 设置模拟了空数据库场景,正确测试插入逻辑

测试文件位置符合编码规范(tests/unit/)。


21-53: Mock 设置完整且符合 syncDefaultErrorRules 的事务流程。

通过 capturedInsertedRules.push(rule) 捕获插入的规则,便于后续断言验证。returning: vi.fn(async () => [{ id: 1 }]) 正确模拟了成功插入的响应。

tests/unit/lib/emit-event.test.ts (1)

66-80: Edge runtime 测试验证了静默跳过行为。

测试确保在 Edge runtime 下所有 emit 函数都不触发任何事件或广播,符合预期的运行时条件逻辑。

src/app/v1/_lib/proxy/thinking-signature-rectifier.test.ts (2)

24-29: LGTM! 新增的触发器检测测试。

测试用例使用了真实的 Claude API 错误消息格式,验证了 detectThinkingSignatureRectifierTrigger 能正确识别 "assistant_message_must_start_with_thinking" 场景。


95-124: LGTM! 关键场景测试覆盖。

测试验证了 PR 目标中描述的核心场景:

  • thinking 已启用
  • 最后一条 assistant 消息以 tool_use 开头(缺少 thinking/redacted_thinking 前缀)
  • 整流器应删除顶层 thinking 字段

这确保了在重试前正确处理请求,避免继续触发 400 错误。

src/types/special-settings.ts (1)

103-106: LGTM! 类型扩展正确。

新增的 "assistant_message_must_start_with_thinking" 触发器类型与以下实现保持一致:

  • detectThinkingSignatureRectifierTrigger 函数的返回值
  • 相关测试用例的断言
  • 新增的默认错误规则

类型安全得到保证,命名风格与现有触发器一致。

tests/unit/proxy/proxy-forwarder-thinking-signature-rectifier.test.ts (1)

262-316: “先移除 thinking block,再因 tool_use 置顶而关闭顶层 thinking” 的链式兜底验证到位。

重试时同时断言 blocks 清理结果(Line 293-295)和 bodyMsg.thinking 被删除(Line 295)很关键;整体用例设计符合该 PR 的修复目标。

src/app/v1/_lib/proxy/thinking-signature-rectifier.ts (1)

1-5: 新增 trigger 类型扩展合理。

ThinkingSignatureRectifierTrigger 增加 "assistant_message_must_start_with_thinking" 能更精确区分该类 400 场景。

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

No significant issues identified in this PR. The implementation correctly addresses the specific error scenario where Claude-compatible channels return a 400 error when thinking is enabled but the last assistant message's content starts with tool_use instead of thinking/redacted_thinking.

PR Size: M

  • Lines changed: 431
  • Files changed: 9

Review Coverage

  • Logic and correctness - Clean
  • Security (OWASP Top 10) - Clean
  • Error handling - Clean
  • Type safety - Clean
  • Documentation accuracy - Clean
  • Test coverage - Adequate
  • Code clarity - Good

Notable Observations

Core Changes Validated:

  1. thinking-signature-rectifier.ts - New trigger type assistant_message_must_start_with_thinking properly detects the error pattern and the rectification logic correctly removes the top-level thinking field when tool_use is the first block
  2. instrumentation.ts - Runtime declaration and dynamic import for closeRedis properly addresses Edge runtime compatibility
  3. error-rules.ts - New default error rule provides actionable guidance with appropriate priority
  4. special-settings.ts - Type union correctly extended

Test Coverage:

  • Unit tests cover trigger detection with actual error messages
  • Integration tests validate retry behavior and cascading fix scenarios
  • New emit-event tests properly validate runtime-specific behavior

Automated review by Claude AI


afterEach(() => {
process.env.NEXT_RUNTIME = prevRuntime;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] [TEST-BRITTLE] process.env.NEXT_RUNTIME restore can leak 'undefined' across the suite

Why this is a problem: Node coerces process.env assignments to strings. If prevRuntime is undefined, process.env.NEXT_RUNTIME = prevRuntime sets the literal string "undefined", leaving the var truthy and not "nodejs". That can change runtime-guarded behavior (e.g. process.env.NEXT_RUNTIME && process.env.NEXT_RUNTIME !== "nodejs") and cause cross-test contamination/flakes.

Suggested fix:

afterEach(() => {
  if (prevRuntime === undefined) {
    delete process.env.NEXT_RUNTIME;
  } else {
    process.env.NEXT_RUNTIME = prevRuntime;
  }
});

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

PR adds a new thinking-signature-rectifier trigger for Anthropic 400s when tool_use precedes thinking, plus a default error rule and related tests. One high-priority test isolation issue can leak NEXT_RUNTIME across the test suite and cause flaky behavior.

PR Size: M

  • Lines changed: 431
  • Files changed: 9

Issues Found

Category Critical High Medium Low
Logic/Bugs 0 0 0 0
Security 0 0 0 0
Error Handling 0 0 0 0
Types 0 0 0 0
Comments/Docs 0 0 0 0
Tests 0 1 0 0
Simplification 0 0 0 0

Critical Issues (Must Fix)

None.

High Priority Issues (Should Fix)

  • [TEST-BRITTLE] process.env.NEXT_RUNTIME restore can leak 'undefined' across the suite (tests/unit/lib/emit-event.test.ts:35, confidence 90)

Review Coverage

  • Logic and correctness
  • Security (OWASP Top 10)
  • Error handling
  • Type safety
  • Documentation accuracy
  • Test coverage
  • Code clarity

Automated review by Codex AI

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Identified PR #577 and applied label size/M.
  • Posted 1 inline review comment: tests/unit/lib/emit-event.test.ts:35[High] [TEST-BRITTLE] restoring process.env.NEXT_RUNTIME can leave the literal string "undefined" and leak env state across tests; suggested using delete process.env.NEXT_RUNTIME when the previous value is undefined. (Discussion: https://github.com/ding113/claude-code-hub/pull/577#discussion_r2678499549)
  • Submitted the required summary review (Review: https://github.com/ding113/claude-code-hub/pull/577#pullrequestreview-3646663258).

@github-actions
Copy link
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/thinking-signature-rectifier.ts (1)

135-180: 逻辑正确,建议考虑在结果中追踪 thinking 字段的移除

兜底逻辑处理了各种边界情况(空 content、无 assistant 消息、多条 assistant 消息),实现符合预期。

不过当前 ThinkingSignatureRectifierResult 只追踪了 removedThinkingBlocksremovedRedactedThinkingBlocksremovedSignatureFields,未记录顶层 thinking 字段被移除的情况。如果审计或调试时需要区分「移除了 thinking block」和「禁用了顶层 thinking」,可考虑在返回结构中新增一个字段(如 disabledTopLevelThinking: boolean)。

♻️ 可选:扩展返回结构
 export type ThinkingSignatureRectifierResult = {
   applied: boolean;
   removedThinkingBlocks: number;
   removedRedactedThinkingBlocks: number;
   removedSignatureFields: number;
+  disabledTopLevelThinking: boolean;
 };

并在函数中追踪:

+  let disabledTopLevelThinking = false;
   // ...
         if (hasToolUse) {
           delete (message as any).thinking;
           applied = true;
+          disabledTopLevelThinking = true;
         }
   // ...
   return {
     applied,
     removedThinkingBlocks,
     removedRedactedThinkingBlocks,
     removedSignatureFields,
+    disabledTopLevelThinking,
   };
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to Reviews > Disable Cache setting

📥 Commits

Reviewing files that changed from the base of the PR and between 4b7d60a and 25fa185.

📒 Files selected for processing (3)
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
  • src/repository/error-rules.ts
  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/repository/error-rules.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

No emoji characters in any code, comments, or string literals

Files:

  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
**/*.{ts,tsx,jsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,jsx,js}: All user-facing strings must use i18n (5 languages supported: zh-CN, en, ja, ko, de). Never hardcode display text
Use path alias @/ to map to ./src/
Use Biome for code formatting with configuration: double quotes, trailing commas, 2-space indent, 100 character line width
Prefer named exports over default exports
Use next-intl for internationalization
Use Next.js 16 App Router with Hono for API routes

Files:

  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Place unit tests in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts

Files:

  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 573
File: src/actions/model-prices.ts:275-335
Timestamp: 2026-01-10T06:20:04.478Z
Learning: In the `ding113/claude-code-hub` repository, Server Actions (files under `src/actions/*.ts`) currently return hardcoded Chinese error messages directly. This is a codebase-wide architectural decision that applies to all action files (e.g., model-prices.ts, users.ts, system-config.ts). Changing this pattern requires a coordinated i18n refactor across all Server Actions, which should be handled as a separate unified task rather than piecemeal changes in individual PRs.
📚 Learning: 2026-01-07T17:05:37.171Z
Learnt from: CR
Repo: ding113/claude-code-hub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T17:05:37.171Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Place unit tests in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts

Applied to files:

  • tests/unit/repository/error-rules-default-thinking-tooluse.test.ts
📚 Learning: 2026-01-05T03:01:39.354Z
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 539
File: src/types/user.ts:158-170
Timestamp: 2026-01-05T03:01:39.354Z
Learning: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.

Applied to files:

  • src/app/v1/_lib/proxy/thinking-signature-rectifier.ts
🧬 Code graph analysis (1)
tests/unit/repository/error-rules-default-thinking-tooluse.test.ts (1)
src/repository/error-rules.ts (1)
  • syncDefaultErrorRules (837-917)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: 🌐 API Tests
  • GitHub Check: Docker Build Test
  • GitHub Check: dev-build-deploy
🔇 Additional comments (3)
src/app/v1/_lib/proxy/thinking-signature-rectifier.ts (2)

1-4: LGTM!

新增的触发类型 assistant_message_must_start_with_thinking 命名清晰,与现有类型风格一致。


25-40: LGTM!

检测逻辑设计合理:

  • 正则兼容带/不带反引号的多种错误消息变体
  • 检测顺序正确(更具体的条件优先匹配)
  • 注释清晰解释了触发场景和修复策略
tests/unit/repository/error-rules-default-thinking-tooluse.test.ts (1)

1-75: 测试设置结构合理。

Mock 策略完善,通过 capturedInsertedRules 捕获插入的规则对象,环境变量隔离避免了与全局测试设置的冲突,所有 mock 返回值结构正确。

Comment on lines +93 to +98
// 覆写响应需为 Claude 错误格式,且包含清晰的自助修复建议
expect(rule.overrideResponse?.type).toBe("error");
expect(rule.overrideResponse?.error?.type).toBe("thinking_error");
expect(rule.overrideResponse?.error?.message).toContain("tool_result");
expect(rule.overrideResponse?.error?.message).toContain("signature");
expect(rule.overrideResponse?.error?.message).toContain("关闭");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's check if the test file exists and examine the context around lines 93-98
wc -l tests/unit/repository/error-rules-default-thinking-tooluse.test.ts

Repository: ding113/claude-code-hub

Length of output: 137


🏁 Script executed:

# Let's look at the specific test case and surrounding context
sed -n '70,105p' tests/unit/repository/error-rules-default-thinking-tooluse.test.ts

Repository: ding113/claude-code-hub

Length of output: 1350


🏁 Script executed:

# Now let's check the source error-rules.ts file for the pattern "must start with a thinking block"
rg -A 15 -B 2 'must start with a thinking block' src/repository/

Repository: ding113/claude-code-hub

Length of output: 1247


错误消息应使用国际化,作为整体 i18n 重构计划的一部分。

错误规则的 overrideResponse 消息包含硬编码的中文(如"关闭"),违反了编码规范要求"所有面向用户的字符串必须使用 i18n"。虽然当前代码库中存在硬编码中文错误消息的模式,但这应作为统一的国际化重构任务处理,而不是分散在各个 PR 中单独修改。建议:

  • 将此问题标记为技术债务,跟踪于统一的 i18n 重构计划中
  • 该重构应覆盖 src/repository/ 中的所有用户面向错误消息,包含所有 5 种语言支持(zh-CN, en, ja, ko, de)

@ding113 ding113 merged commit c5cb60c into dev Jan 10, 2026
11 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in Claude Code Hub Roadmap Jan 10, 2026
NieiR added a commit to NieiR/claude-code-hub that referenced this pull request Jan 10, 2026
- PR ding113#580: TOML cloud price table + billing fail-open
- PR ding113#578: make drizzle migrations idempotent
- PR ding113#577: fix thinking enabled + tool_use first block
- PR ding113#573: add manual model price management

Conflict resolutions:
- drizzle migrations: use upstream idempotent version
- i18n messages: accept upstream additions
- price-sync.ts: removed (replaced by cloud-price-table)
- model-prices.ts: use upstream refactored version
@coderabbitai coderabbitai bot mentioned this pull request Jan 22, 2026
7 tasks
@ding113 ding113 deleted the fix/thinking-disable-on-tooluse-first branch January 27, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Error Rule bug Something isn't working size/M Medium PR (< 500 lines)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant

Comments