feat: add tiered tool-repetition detection (soft warning + hard stop)#772
feat: add tiered tool-repetition detection (soft warning + hard stop)#772p12tic wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a configurable ChangesTool repetition soft limit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/task/__tests__/Task.spec.ts (1)
419-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name overstates what is verified. The test claims to verify the hard stop is derived from
consecutiveMistakeLimit, but it only asserts the two input fields and thattoolRepetitionDetectoris defined — it never checks that the detector's hard limit actually equals 7. SincehardStopLimitis private, consider verifying behavior instead (e.g., constructing the detector and exercisingcheck()until the hard block fires at the 7th identical call), or relax the test name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.spec.ts` around lines 419 - 432, The test name is stronger than what the `Task` spec actually verifies: it says the tool repetition hard stop is derived from `consecutiveMistakeLimit`, but only checks the constructor inputs and that `toolRepetitionDetector` exists. Update the `Task.spec.ts` case around the `Task` constructor to either assert the detector behavior through repeated `check()` calls until the 7th identical repetition triggers the block, or rename/relax the test so it only reflects the fields being verified. Use `Task` and `toolRepetitionDetector` as the key symbols when adjusting the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/ApiOptions.tsx`:
- Around line 763-770: The soft threshold in ToolRepetitionLimitControl can
still be saved at or above the hard limit, which makes the soft-block path
unreachable in ToolRepetitionDetector.check(). Update the ApiOptions setting
flow (the softValue/onSoftChange wiring around setApiConfigurationField and the
consecutiveMistakeLimit-related controls) to clamp or reject
toolRepetitionSoftLimit so it always stays below the hard stop, and add a local
Vitest regression test under webview-ui/src/**/__tests__ covering the
invalid-combination behavior and the saved-value validation/constraint.
---
Nitpick comments:
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 419-432: The test name is stronger than what the `Task` spec
actually verifies: it says the tool repetition hard stop is derived from
`consecutiveMistakeLimit`, but only checks the constructor inputs and that
`toolRepetitionDetector` exists. Update the `Task.spec.ts` case around the
`Task` constructor to either assert the detector behavior through repeated
`check()` calls until the 7th identical repetition triggers the block, or
rename/relax the test so it only reflects the fields being verified. Use `Task`
and `toolRepetitionDetector` as the key symbols when adjusting the assertion.
🪄 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 Plus
Run ID: fdea1a9d-ff92-4776-9d48-b1d9d27e929d
📒 Files selected for processing (52)
apps/vscode-e2e/src/types/global.d.tspackages/types/src/provider-settings.tspackages/types/src/task.tssrc/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-images.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/config/ProviderSettingsManager.tssrc/core/config/__tests__/ProviderSettingsManager.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/tools/ToolRepetitionDetector.tssrc/core/tools/__tests__/ToolRepetitionDetector.spec.tssrc/core/webview/ClineProvider.tssrc/i18n/locales/ca/tools.jsonsrc/i18n/locales/de/tools.jsonsrc/i18n/locales/en/tools.jsonsrc/i18n/locales/es/tools.jsonsrc/i18n/locales/fr/tools.jsonsrc/i18n/locales/hi/tools.jsonsrc/i18n/locales/id/tools.jsonsrc/i18n/locales/it/tools.jsonsrc/i18n/locales/ja/tools.jsonsrc/i18n/locales/ko/tools.jsonsrc/i18n/locales/nl/tools.jsonsrc/i18n/locales/pl/tools.jsonsrc/i18n/locales/pt-BR/tools.jsonsrc/i18n/locales/ru/tools.jsonsrc/i18n/locales/tr/tools.jsonsrc/i18n/locales/vi/tools.jsonsrc/i18n/locales/zh-CN/tools.jsonsrc/i18n/locales/zh-TW/tools.jsonwebview-ui/src/components/settings/ApiOptions.tsxwebview-ui/src/components/settings/ToolRepetitionLimitControl.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
2a2e2c6 to
5f7162d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/components/settings/__tests__/ToolRepetitionLimitControl.spec.tsx (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign this test with the component’s declared prop contract.
ToolRepetitionLimitControlcurrently typessoftValueasnumber, sosoftValue={undefined as unknown as number}only works by sidestepping TypeScript. Either make the prop optional if this fallback is a supported runtime path, or move this coverage to the parent that can actually omit/default the value. As written, the test passes for a state typed callers cannot express.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/__tests__/ToolRepetitionLimitControl.spec.tsx` around lines 47 - 54, The test is forcing an impossible prop state by passing softValue as undefined even though ToolRepetitionLimitControl declares it as a number. Update the coverage to match the component contract: either make softValue optional in ToolRepetitionLimitControl if undefined is a supported input, or move this defaulting assertion to the parent component that actually omits the value and supplies the fallback. Use the ToolRepetitionLimitControl prop type and the existing softValue/default behavior in the test suite to locate the right place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.ts`:
- Around line 207-228: The allow-path test in presentAssistantMessage is only
checking that toolRepetitionDetector.check was called and that no hard-block
prompt happened, so it can still pass if tool dispatch never occurs. Update this
spec to assert a downstream side effect from the normal execution path after
action: "allow", using presentAssistantMessage and the mocked tool execution
path (for example, verify the tool runner or resulting message/state change was
triggered) so the test proves the tool actually continued normally.
---
Nitpick comments:
In
`@webview-ui/src/components/settings/__tests__/ToolRepetitionLimitControl.spec.tsx`:
- Around line 47-54: The test is forcing an impossible prop state by passing
softValue as undefined even though ToolRepetitionLimitControl declares it as a
number. Update the coverage to match the component contract: either make
softValue optional in ToolRepetitionLimitControl if undefined is a supported
input, or move this defaulting assertion to the parent component that actually
omits the value and supplies the fallback. Use the ToolRepetitionLimitControl
prop type and the existing softValue/default behavior in the test suite to
locate the right place.
🪄 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 Plus
Run ID: 20da49a2-5201-4a7d-ab32-18730952fc5b
📒 Files selected for processing (53)
packages/types/src/provider-settings.tspackages/types/src/task.tssrc/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-images.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/config/ProviderSettingsManager.tssrc/core/config/__tests__/ProviderSettingsManager.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/tools/ToolRepetitionDetector.tssrc/core/tools/__tests__/ToolRepetitionDetector.spec.tssrc/core/webview/ClineProvider.tssrc/i18n/locales/ca/tools.jsonsrc/i18n/locales/de/tools.jsonsrc/i18n/locales/en/tools.jsonsrc/i18n/locales/es/tools.jsonsrc/i18n/locales/fr/tools.jsonsrc/i18n/locales/hi/tools.jsonsrc/i18n/locales/id/tools.jsonsrc/i18n/locales/it/tools.jsonsrc/i18n/locales/ja/tools.jsonsrc/i18n/locales/ko/tools.jsonsrc/i18n/locales/nl/tools.jsonsrc/i18n/locales/pl/tools.jsonsrc/i18n/locales/pt-BR/tools.jsonsrc/i18n/locales/ru/tools.jsonsrc/i18n/locales/tr/tools.jsonsrc/i18n/locales/vi/tools.jsonsrc/i18n/locales/zh-CN/tools.jsonsrc/i18n/locales/zh-TW/tools.jsonwebview-ui/src/components/settings/ApiOptions.tsxwebview-ui/src/components/settings/ToolRepetitionLimitControl.tsxwebview-ui/src/components/settings/__tests__/ToolRepetitionLimitControl.spec.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
✅ Files skipped from review due to trivial changes (31)
- src/i18n/locales/hi/tools.json
- src/i18n/locales/ca/tools.json
- src/i18n/locales/en/tools.json
- src/i18n/locales/zh-CN/tools.json
- webview-ui/src/i18n/locales/nl/settings.json
- src/i18n/locales/ja/tools.json
- src/i18n/locales/zh-TW/tools.json
- webview-ui/src/i18n/locales/pl/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- src/i18n/locales/vi/tools.json
- src/i18n/locales/pl/tools.json
- src/i18n/locales/nl/tools.json
- src/i18n/locales/es/tools.json
- src/i18n/locales/tr/tools.json
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/es/settings.json
- src/i18n/locales/de/tools.json
- src/core/assistant-message/tests/presentAssistantMessage-unknown-tool.spec.ts
- webview-ui/src/i18n/locales/en/settings.json
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- src/i18n/locales/pt-BR/tools.json
- src/i18n/locales/ru/tools.json
- webview-ui/src/i18n/locales/fr/settings.json
- webview-ui/src/i18n/locales/de/settings.json
- webview-ui/src/i18n/locales/pt-BR/settings.json
- src/i18n/locales/it/tools.json
- src/i18n/locales/ko/tools.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/ko/settings.json
- src/i18n/locales/id/tools.json
🚧 Files skipped from review as they are similar to previous changes (20)
- src/core/assistant-message/tests/presentAssistantMessage-images.spec.ts
- src/core/assistant-message/tests/presentAssistantMessage-custom-tool.spec.ts
- packages/types/src/task.ts
- src/core/assistant-message/presentAssistantMessage.ts
- src/i18n/locales/fr/tools.json
- webview-ui/src/components/settings/ApiOptions.tsx
- webview-ui/src/i18n/locales/id/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- src/core/webview/ClineProvider.ts
- webview-ui/src/components/settings/ToolRepetitionLimitControl.tsx
- src/core/config/tests/ProviderSettingsManager.spec.ts
- webview-ui/src/i18n/locales/ca/settings.json
- src/core/task/tests/Task.spec.ts
- src/core/task/Task.ts
- src/core/config/ProviderSettingsManager.ts
- webview-ui/src/i18n/locales/ru/settings.json
- packages/types/src/provider-settings.ts
- webview-ui/src/i18n/locales/hi/settings.json
- src/core/tools/tests/ToolRepetitionDetector.spec.ts
- src/core/tools/ToolRepetitionDetector.ts
5f7162d to
75ae6ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/toolRepetitionLimits.ts`:
- Around line 25-30: The clamping logic in clampToolRepetitionSoftLimit can
return a negative value when hardLimit is a fractional positive number,
violating the helper’s non-negative contract. Update the Math.min branch so the
returned soft limit is still bounded below by zero while remaining strictly
below the hard stop, and keep the existing early return for hardLimit <= 0
intact.
🪄 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 Plus
Run ID: 6c3221e9-f84c-4e37-b0dd-b8373dbdd026
📒 Files selected for processing (55)
packages/types/src/provider-settings.tspackages/types/src/task.tssrc/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-images.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-tool-repetition.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/config/ProviderSettingsManager.tssrc/core/config/__tests__/ProviderSettingsManager.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/tools/ToolRepetitionDetector.tssrc/core/tools/__tests__/ToolRepetitionDetector.spec.tssrc/core/webview/ClineProvider.tssrc/i18n/locales/ca/tools.jsonsrc/i18n/locales/de/tools.jsonsrc/i18n/locales/en/tools.jsonsrc/i18n/locales/es/tools.jsonsrc/i18n/locales/fr/tools.jsonsrc/i18n/locales/hi/tools.jsonsrc/i18n/locales/id/tools.jsonsrc/i18n/locales/it/tools.jsonsrc/i18n/locales/ja/tools.jsonsrc/i18n/locales/ko/tools.jsonsrc/i18n/locales/nl/tools.jsonsrc/i18n/locales/pl/tools.jsonsrc/i18n/locales/pt-BR/tools.jsonsrc/i18n/locales/ru/tools.jsonsrc/i18n/locales/tr/tools.jsonsrc/i18n/locales/vi/tools.jsonsrc/i18n/locales/zh-CN/tools.jsonsrc/i18n/locales/zh-TW/tools.jsonwebview-ui/src/components/settings/ApiOptions.tsxwebview-ui/src/components/settings/ToolRepetitionLimitControl.tsxwebview-ui/src/components/settings/__tests__/ToolRepetitionLimitControl.spec.tsxwebview-ui/src/components/settings/__tests__/toolRepetitionLimits.spec.tswebview-ui/src/components/settings/toolRepetitionLimits.tswebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
✅ Files skipped from review due to trivial changes (31)
- src/i18n/locales/pl/tools.json
- src/i18n/locales/ja/tools.json
- src/i18n/locales/ru/tools.json
- src/core/assistant-message/tests/presentAssistantMessage-unknown-tool.spec.ts
- webview-ui/src/i18n/locales/nl/settings.json
- src/i18n/locales/en/tools.json
- src/i18n/locales/id/tools.json
- src/i18n/locales/ko/tools.json
- src/i18n/locales/it/tools.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- src/i18n/locales/tr/tools.json
- webview-ui/src/i18n/locales/ca/settings.json
- webview-ui/src/i18n/locales/de/settings.json
- src/i18n/locales/hi/tools.json
- webview-ui/src/i18n/locales/ko/settings.json
- webview-ui/src/i18n/locales/fr/settings.json
- src/i18n/locales/fr/tools.json
- src/i18n/locales/ca/tools.json
- src/i18n/locales/pt-BR/tools.json
- src/i18n/locales/zh-TW/tools.json
- webview-ui/src/i18n/locales/id/settings.json
- src/i18n/locales/es/tools.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- src/i18n/locales/de/tools.json
- webview-ui/src/i18n/locales/pt-BR/settings.json
- src/i18n/locales/vi/tools.json
- webview-ui/src/i18n/locales/es/settings.json
- src/i18n/locales/zh-CN/tools.json
- webview-ui/src/i18n/locales/pl/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- webview-ui/src/i18n/locales/en/settings.json
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/types/src/task.ts
- src/i18n/locales/nl/tools.json
- webview-ui/src/components/settings/ToolRepetitionLimitControl.tsx
- src/core/assistant-message/tests/presentAssistantMessage-images.spec.ts
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/components/settings/tests/ToolRepetitionLimitControl.spec.tsx
- src/core/assistant-message/presentAssistantMessage.ts
- webview-ui/src/i18n/locales/tr/settings.json
- src/core/assistant-message/tests/presentAssistantMessage-custom-tool.spec.ts
- webview-ui/src/components/settings/ApiOptions.tsx
- src/core/assistant-message/tests/presentAssistantMessage-tool-repetition.spec.ts
- webview-ui/src/i18n/locales/ru/settings.json
- src/core/webview/ClineProvider.ts
- src/core/task/tests/Task.spec.ts
- packages/types/src/provider-settings.ts
- src/core/config/tests/ProviderSettingsManager.spec.ts
- src/core/task/Task.ts
- src/core/config/ProviderSettingsManager.ts
- src/core/tools/ToolRepetitionDetector.ts
- src/core/tools/tests/ToolRepetitionDetector.spec.ts
- webview-ui/src/i18n/locales/hi/settings.json
4950296 to
8bbfa84
Compare
Replace the single-threshold ToolRepetitionDetector with a two-tier system: - Soft block (default 2nd identical call): the tool is not executed and the model receives an error asking it to justify the repeat. The user is not involved and the counter keeps incrementing toward the hard limit. - Hard stop: execution stops and the user is asked for guidance (previous behavior).
8bbfa84 to
1b37dec
Compare
|
@coderabbitai Please review |
|
✅ Action performedReview finished.
|
Related GitHub Issue
Closes: #771
Description
Replace the single-threshold ToolRepetitionDetector with a two-tier system:
Test Procedure
Tell a model to call the same command multiple times. See how it can't execute 3rd command and starts to using different variations of it.
Pre-Submission Checklist
Screenshots / Videos
None.
Documentation Updates
None.
Additional Notes
None.
Summary by CodeRabbit
New Features
0disabling soft warnings).Bug Fixes