Conversation
移除 FileEditTool 和 FileWriteTool 中的 "read before edit" 校验， 允许直接编辑未读取过的文件。保留文件修改过期检测。 Co-Authored-By: Claude Opus 4.7 <[email protected]>
- 扩大模型名称检测范围，匹配所有 deepseek 模型（V4、R1 等） - 始终保留 thinking blocks 为 reasoning_content 回传给 API - 移除有 bug 的 turn boundary 剥离逻辑 Co-Authored-By: Claude Opus 4.7 <[email protected]>
Pro 和 Max/Team 订阅者的 Opus 默认 effort 之前被降级为 medium， 导致用户感知模型「变笨」。恢复为 high。 Co-Authored-By: Claude Opus 4.7 <[email protected]>
空闲超过 1 小时后 thinkingClearLatched 会被触发且永不重置， 导致每轮 API 调用都清除 thinking 历史。完整移除该 latch 机制， clearAllThinking 硬编码为 false。 Co-Authored-By: Claude Opus 4.7 <[email protected]>
删除「工具调用间文字 ≤25 词、最终回复 ≤100 词」的硬性限制。 ablation 测试显示该约束使整体智能下降 3%。 Co-Authored-By: Claude Opus 4.7 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the thinking-clear latch, always preserves Anthropic "thinking" as OpenAI Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.7 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/utils/effort.ts (2)
344-358:⚠️ Potential issue | 🟡 MinorStale comment contradicts the new defaults.
The comment still says "medium for Pro" and "Max/Team also get medium", but both branches now return
'high'. Please refresh the comment so the intent is not misleading for future maintainers.✏️ Proposed fix
- // Default effort on Opus 4.6 to medium for Pro. - // Max/Team also get medium when the tengu_grey_step2 config is enabled. + // Default effort on Opus 4.6/4.7 to high for Pro. + // Max/Team also get high when the tengu_grey_step2 config is enabled. if ( model.toLowerCase().includes('opus-4-7') || model.toLowerCase().includes('opus-4-6') ) { if (isProSubscriber()) { return 'high' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/effort.ts` around lines 344 - 358, Update the stale comment above the Opus model branch in effort determination (the block checking model.includes('opus-4-7') || model.includes('opus-4-6')) to reflect current behavior: Pro, Max and Team subscribers now get 'high' effort when conditions apply. Edit the comment near the logic that calls isProSubscriber(), getOpusDefaultEffortConfig(), isMaxSubscriber(), and isTeamSubscriber() so it no longer states "medium" and clearly documents that these subscribers receive 'high' by default under the current config.
297-302:⚠️ Potential issue | 🟡 MinorComment on line 353 is outdated; update the fallback dialog or the effort logic.
The comment at line 353 states the defaults are "medium," but the code returns
'high'for Pro subscribers and Max/Team when the config is enabled (lines 358, 363). The fallback dialog text at line 299 also says "We recommend medium effort," creating an inconsistency. When GrowthBook cannot serve thetengu_grey_step2config, the fallback is used and will enable the code path that returns'high'while displaying a dialog recommending'medium'. Either align the fallback dialog text to match the actual effort default, update the code comment, or reconsider the fallback'senabled: truestate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/effort.ts` around lines 297 - 302, The OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT fallback is inconsistent with the effort selection logic: OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT (enabled, dialogTitle, dialogDescription) says "medium" while the effort resolver returns 'high' for Pro/Max/Team when the config is enabled; update the fallback to be consistent by either changing OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT.dialogDescription to recommend "high" effort, set enabled: false so the fallback won't trigger the high-effort path, or adjust the effort-selection logic to return 'medium' when OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT is used; locate and modify the OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT constant and the effort-resolving function (the method that returns 'high' for Pro/Max/Team) so both the dialog text and the returned default effort match.src/services/api/openai/requestBody.ts (1)
11-22:⚠️ Potential issue | 🟡 MinorStale JSDoc — update "Enabled when" rule to match the broadened detection.
The docstring still advertises
deepseek-reasonerORDeepSeek-V3.2only, but the implementation now matches any model name containingdeepseek. Either update the docs to reflect that, or (preferably, per the comment below) restore a narrow allowlist and update docs to enumerate the supported SKUs.✏️ Minimal doc update if keeping the broad match
- * 2. Model name contains "deepseek-reasoner" OR "DeepSeek-V3.2" (auto-detect, case-insensitive) + * 2. Model name contains "deepseek" (auto-detect, case-insensitive)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/requestBody.ts` around lines 11 - 22, The JSDoc above the thinking-mode detection is stale: it says enabled for "deepseek-reasoner" or "DeepSeek-V3.2" but the implementation now enables thinking for any model name containing "deepseek"; update the docstring to reflect the broadened rule (e.g., "model name contains 'deepseek' (case-insensitive)") or alternatively tighten the implementation back to an explicit allowlist and then enumerate those SKUs in the JSDoc; locate the thinking-mode detection logic and the JSDoc immediately above it (the "Detect whether DeepSeek-style thinking mode should be enabled." block) and make the doc and code agree.packages/@ant/model-provider/src/shared/openaiConvertMessages.ts (1)
208-217:⚠️ Potential issue | 🔴 Critical
reasoning_contentmust not be unconditionally preserved across turn boundaries—this will cause DeepSeek 400 errors.According to DeepSeek's official API docs,
reasoning_contentmust only be carried forward within the same tool-call loop. For new user turns (after a fresh message),reasoning_contentfrom prior turns must be stripped—without tools, the API returns 400 if included; even with tools, it should only persist within the same reasoning loop, not across distinct user messages.The current code at lines 208–217 unconditionally preserves all thinking blocks into
reasoningPartswithout checking turn boundaries. The inline comment claiming to "always preserve reasoning_content" contradicts the API contract. This will cause production failures when a new user turn begins after a previous tool-call loop concludes.Restore turn-boundary logic: only preserve
reasoning_contentfrom the trailing contiguous assistant-message run (the current tool-call loop up to the next non-tool_resultmessage). Earlier assistant messages and prior turns must drop it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/shared/openaiConvertMessages.ts around lines 208 - 217, The code currently always pushes assistant "thinking" blocks into reasoningParts (block.type === 'thinking'), which violates DeepSeek's rule to only carry reasoning_content within the same tool-call loop; change the logic so you only preserve thinkingText when the block is part of the trailing contiguous assistant-message run for the current tool-call loop (i.e., the most recent assistant messages that are followed only by tool_result blocks and no new user message), and skip pushing earlier assistant thinking blocks from prior turns; reference the existing symbols reasoningParts, block (type 'thinking'), thinkingText and the notion of tool_result to implement the conditional check that drops reasoning_content across turn boundaries.packages/builtin-tools/src/tools/FileWriteTool/FileWriteTool.ts (1)
198-214:⚠️ Potential issue | 🟠 MajorRelaxed validation opens a silent data-loss path for partial reads.
The validation at lines 198-214 only checks staleness (mtime) when
readTimestampexists; it does not guard against partial reads. The call-site check (line 81) does not compensate for one critical case: a partial read followed by a full-file overwrite with no external modification.Trace for a file previously read with
offset/limitset and no external change:
validateInput:readTimestampexists,lastWriteTime <= readTimestamp.timestamp→ returns{ result: true }.call(line 81):lastReadexists and timestamp unchanged → condition false → staleness block is skipped entirely. The write proceeds.Consequence: the model can overwrite an entire file based on a partial view (e.g., read lines 1–100 of a 1000-line file, write a "replacement" that silently drops lines 101–1000).
FileEditToolincludes an equivalentisFullReadguard in validation and rejects partial reads;FileWriteToolremoved this without preserving it in the call-site path.Secondary concerns:
- For unread existing files, the call-site throws only after
mkdir(dir)andfileHistoryTrackEdithave run, leaving stray directories and backups on every rejected write.- Error message degrades from "Read it first before writing" to the generic
FILE_UNEXPECTEDLY_MODIFIED_ERROR, less actionable for recovery.- Diverges from
NotebookEditTool, which enforces strict read-before-edit (errorCode 9/10).Re-introduce a partial-view guard in
validateInputto reject writes whenreadTimestamp.offset !== undefined || readTimestamp.limit !== undefined || readTimestamp.isPartialView, with a clear message directing the model to re-read the full file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/builtin-tools/src/tools/FileWriteTool/FileWriteTool.ts` around lines 198 - 214, The validateInput logic in FileWriteTool must reject partial reads: in validateInput (the function that inspects toolUseContext.readFileState.get(fullFilePath)) re-introduce a guard that returns an error when readTimestamp.offset !== undefined || readTimestamp.limit !== undefined || readTimestamp.isPartialView (with a clear message asking the model to re-read the full file), and keep the existing mtime staleness check; also move or add the unread-existing-file/staleness check so it runs before side-effectful operations in call (the write entry point that currently mkdir(dir) and fileHistoryTrackEdit) to avoid creating directories/backups on rejected writes and preserve a descriptive error code/message rather than the generic FILE_UNEXPECTEDLY_MODIFIED_ERROR.
🧹 Nitpick comments (1)
packages/@ant/model-provider/src/shared/openaiConvertMessages.ts (1)
16-20: Stale interface doc —enableThinkingis now ignored.The
_optionsparameter is accepted for compatibility but never read, soConvertMessagesOptions.enableThinkinghas no effect anymore. Please update the JSDoc so consumers aren't misled into thinking this flag still toggles behavior, or drop the field entirely if nothing downstream reads it.✏️ Proposed doc update
export interface ConvertMessagesOptions { - /** When true, preserve thinking blocks as reasoning_content on assistant messages - * (required for DeepSeek thinking mode with tool calls). */ - enableThinking?: boolean + /** + * `@deprecated` No longer consulted. Thinking blocks are always converted + * to `reasoning_content`. Kept for backward compatibility of callers. + */ + enableThinking?: boolean }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/shared/openaiConvertMessages.ts around lines 16 - 20, Update the stale JSDoc or API shape for ConvertMessagesOptions: either remove the unused enableThinking field from the interface or change its JSDoc to explicitly state it is ignored for compatibility because the function accepts an _options parameter but never reads it; locate the ConvertMessagesOptions declaration and the converter that accepts _options to ensure the comment matches reality (reference symbols: ConvertMessagesOptions, enableThinking, and the _options parameter) and keep the public contract consistent with actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/teach-me/SKILL.md:
- Around line 306-328: The template hardcodes Chinese headers ("{Topic} 核心笔记"
and "实战参数 / Cheat Sheet"), conflicting with rule 7; update the SKILL.md template
to be language-agnostic by replacing those Chinese strings with neutral/English
placeholders (e.g., "{Topic} Notes" and "Cheat Sheet (if applicable)") or
clearly mark the examples as "Chinese example" so the template content matches
the session language; modify the header lines shown in the diff (the lines
containing "{Topic} 核心笔记" and "实战参数 / Cheat Sheet") accordingly.
In `@src/services/api/claude.ts`:
- Line 1671: The hardcoded clearAllThinking: false removes the previous
idle-time guard that switched clear_thinking_20251015 between { type:
'thinking_turns', value: 1 } and 'all'; restore the prior behavior by making
clearAllThinking conditional again: check getLastApiCompletionTimestamp()
against CACHE_TTL_1HOUR_MS (or re-use the existing latch logic) and set
clearAllThinking = true only if the last completion is within the TTL, otherwise
keep the thinking_turns = 1 behavior; update references in the Claude request
construction (clearAllThinking) so the decision mirrors the original strategy
instead of always forcing false and causing long-idle sessions to resend full
thinking history.
In `@src/services/api/openai/requestBody.ts`:
- Around line 28-30: The auto-detect currently uses
modelLower.includes('deepseek') which is too broad; change it to only match the
supported thinking-mode DeepSeek models by checking model (modelLower) against
the explicit allowed names/prefixes (e.g., 'deepseek-reasoner', 'deepseek-v3.2',
and the v4 family like 'deepseek-v4', 'deepseek-v4-flash', 'deepseek-v4-pro')
instead of any substring; update the logic in requestBody.ts that computes
modelLower to use an allowlist (array of exact names or prefixes) and return
true only when modelLower equals or startsWith one of those entries so
coder/legacy models like 'deepseek-coder' or 'deepseek-chat' do not trigger
thinking:true.
---
Outside diff comments:
In `@packages/`@ant/model-provider/src/shared/openaiConvertMessages.ts:
- Around line 208-217: The code currently always pushes assistant "thinking"
blocks into reasoningParts (block.type === 'thinking'), which violates
DeepSeek's rule to only carry reasoning_content within the same tool-call loop;
change the logic so you only preserve thinkingText when the block is part of the
trailing contiguous assistant-message run for the current tool-call loop (i.e.,
the most recent assistant messages that are followed only by tool_result blocks
and no new user message), and skip pushing earlier assistant thinking blocks
from prior turns; reference the existing symbols reasoningParts, block (type
'thinking'), thinkingText and the notion of tool_result to implement the
conditional check that drops reasoning_content across turn boundaries.
In `@packages/builtin-tools/src/tools/FileWriteTool/FileWriteTool.ts`:
- Around line 198-214: The validateInput logic in FileWriteTool must reject
partial reads: in validateInput (the function that inspects
toolUseContext.readFileState.get(fullFilePath)) re-introduce a guard that
returns an error when readTimestamp.offset !== undefined || readTimestamp.limit
!== undefined || readTimestamp.isPartialView (with a clear message asking the
model to re-read the full file), and keep the existing mtime staleness check;
also move or add the unread-existing-file/staleness check so it runs before
side-effectful operations in call (the write entry point that currently
mkdir(dir) and fileHistoryTrackEdit) to avoid creating directories/backups on
rejected writes and preserve a descriptive error code/message rather than the
generic FILE_UNEXPECTEDLY_MODIFIED_ERROR.
In `@src/services/api/openai/requestBody.ts`:
- Around line 11-22: The JSDoc above the thinking-mode detection is stale: it
says enabled for "deepseek-reasoner" or "DeepSeek-V3.2" but the implementation
now enables thinking for any model name containing "deepseek"; update the
docstring to reflect the broadened rule (e.g., "model name contains 'deepseek'
(case-insensitive)") or alternatively tighten the implementation back to an
explicit allowlist and then enumerate those SKUs in the JSDoc; locate the
thinking-mode detection logic and the JSDoc immediately above it (the "Detect
whether DeepSeek-style thinking mode should be enabled." block) and make the doc
and code agree.
In `@src/utils/effort.ts`:
- Around line 344-358: Update the stale comment above the Opus model branch in
effort determination (the block checking model.includes('opus-4-7') ||
model.includes('opus-4-6')) to reflect current behavior: Pro, Max and Team
subscribers now get 'high' effort when conditions apply. Edit the comment near
the logic that calls isProSubscriber(), getOpusDefaultEffortConfig(),
isMaxSubscriber(), and isTeamSubscriber() so it no longer states "medium" and
clearly documents that these subscribers receive 'high' by default under the
current config.
- Around line 297-302: The OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT fallback is
inconsistent with the effort selection logic: OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT
(enabled, dialogTitle, dialogDescription) says "medium" while the effort
resolver returns 'high' for Pro/Max/Team when the config is enabled; update the
fallback to be consistent by either changing
OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT.dialogDescription to recommend "high" effort,
set enabled: false so the fallback won't trigger the high-effort path, or adjust
the effort-selection logic to return 'medium' when
OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT is used; locate and modify the
OPUS_DEFAULT_EFFORT_CONFIG_DEFAULT constant and the effort-resolving function
(the method that returns 'high' for Pro/Max/Team) so both the dialog text and
the returned default effort match.
---
Nitpick comments:
In `@packages/`@ant/model-provider/src/shared/openaiConvertMessages.ts:
- Around line 16-20: Update the stale JSDoc or API shape for
ConvertMessagesOptions: either remove the unused enableThinking field from the
interface or change its JSDoc to explicitly state it is ignored for
compatibility because the function accepts an _options parameter but never reads
it; locate the ConvertMessagesOptions declaration and the converter that accepts
_options to ensure the comment matches reality (reference symbols:
ConvertMessagesOptions, enableThinking, and the _options parameter) and keep the
public contract consistent with actual behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 898a8d36-6099-4480-a347-1c88756065e9
📒 Files selected for processing (13)
.claude/skills/teach-me/SKILL.mdpackages/@ant/model-provider/src/shared/__tests__/openaiConvertMessages.test.tspackages/@ant/model-provider/src/shared/openaiConvertMessages.tspackages/builtin-tools/src/tools/FileEditTool/FileEditTool.tspackages/builtin-tools/src/tools/FileEditTool/UI.tsxpackages/builtin-tools/src/tools/FileWriteTool/FileWriteTool.tssrc/bootstrap/state.tssrc/constants/prompts.tssrc/services/api/claude.tssrc/services/api/openai/__tests__/thinking.test.tssrc/services/api/openai/requestBody.tssrc/services/api/src/bootstrap/state.tssrc/utils/effort.ts
💤 Files with no reviewable changes (5)
- src/services/api/src/bootstrap/state.ts
- src/constants/prompts.ts
- packages/builtin-tools/src/tools/FileEditTool/FileEditTool.ts
- packages/builtin-tools/src/tools/FileEditTool/UI.tsx
- src/bootstrap/state.ts
| ```markdown | ||
| # {Topic} 核心笔记 | ||
|
|
||
| ## 1. {Section Name} | ||
| {Key concept, mechanism, or principle} | ||
| * **One-line summary**: {what it does / why it matters} | ||
| * **Detail**: {brief explanation, 2-4 sentences max} | ||
| * **Example** (if applicable): {code snippet, command, or concrete scenario} | ||
|
|
||
| --- | ||
|
|
||
| ## 2. {Section Name} | ||
| ... | ||
|
|
||
| --- | ||
|
|
||
| ## n. 实战参数 / Cheat Sheet (if applicable) | ||
| {Practical commands, config, or quick-reference table} | ||
|
|
||
| | Parameter / Concept | What it does | Tuning tip | | ||
| |---------------------|-------------|------------| | ||
| | ... | ... | ... | | ||
| ``` |
There was a problem hiding this comment.
Template hardcodes Chinese, conflicting with rule 7.
The notes template at lines 307 and 322 hardcodes Chinese text (核心笔记, 实战参数), but rule 7 (line 338) states that "Language matches the session." This inconsistency could confuse implementers and lead to English sessions generating Chinese headers.
Consider making the template language-agnostic or marking it clearly as an example for Chinese sessions.
📝 Proposed fix to make template language-agnostic
### Notes Structure
```markdown
-# {Topic} 核心笔记
+# {Topic} Notes
## 1. {Section Name}
{Key concept, mechanism, or principle}
* **One-line summary**: {what it does / why it matters}
* **Detail**: {brief explanation, 2-4 sentences max}
* **Example** (if applicable): {code snippet, command, or concrete scenario}
---
## 2. {Section Name}
...
---
-## n. 实战参数 / Cheat Sheet (if applicable)
+## n. Cheat Sheet (if applicable)
{Practical commands, config, or quick-reference table}
| Parameter / Concept | What it does | Tuning tip |
|---------------------|-------------|------------|
| ... | ... | ... |
Alternatively, if you want to keep the Chinese example, clearly mark it:
```diff
### Notes Structure
+**Example template (Chinese session):**
+
```markdown
# {Topic} 核心笔记
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/teach-me/SKILL.md around lines 306 - 328, The template
hardcodes Chinese headers ("{Topic} 核心笔记" and "实战参数 / Cheat Sheet"), conflicting
with rule 7; update the SKILL.md template to be language-agnostic by replacing
those Chinese strings with neutral/English placeholders (e.g., "{Topic} Notes"
and "Cheat Sheet (if applicable)") or clearly mark the examples as "Chinese
example" so the template content matches the session language; modify the header
lines shown in the diff (the lines containing "{Topic} 核心笔记" and "实战参数 / Cheat
Sheet") accordingly.
| hasThinking, | ||
| isRedactThinkingActive: betasParams.includes(REDACT_THINKING_BETA_HEADER), | ||
| clearAllThinking: thinkingClearLatched, | ||
| clearAllThinking: false, |
There was a problem hiding this comment.
Hardcoding clearAllThinking: false removes the >1h idle token-bloat guard.
Per src/services/compact/apiMicrocompact.ts (snippet 1), clearAllThinking toggled the clear_thinking_20251015 strategy between { type: 'thinking_turns', value: 1 } and 'all'. With this always false, every request now keeps 'all' previous thinking turns, including after a long idle where the server-side prompt cache (TTL ~5m / 1h) has already been evicted. That's a guaranteed cache miss that previously trimmed thinking to 1 turn and now re-sends the full thinking history uncached — a measurable token/$$ regression on resumed sessions.
If the latch logic itself was buggy, consider replacing it with a direct check against getLastApiCompletionTimestamp() + CACHE_TTL_1HOUR_MS (both still imported here) rather than removing the behavior entirely, or confirm with the model-launch DRI that the cost impact is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api/claude.ts` at line 1671, The hardcoded clearAllThinking:
false removes the previous idle-time guard that switched clear_thinking_20251015
between { type: 'thinking_turns', value: 1 } and 'all'; restore the prior
behavior by making clearAllThinking conditional again: check
getLastApiCompletionTimestamp() against CACHE_TTL_1HOUR_MS (or re-use the
existing latch logic) and set clearAllThinking = true only if the last
completion is within the TTL, otherwise keep the thinking_turns = 1 behavior;
update references in the Claude request construction (clearAllThinking) so the
decision mirrors the original strategy instead of always forcing false and
causing long-idle sessions to resend full thinking history.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests