test: cover api conversation edge cases#68
Conversation
📝 WalkthroughWalkthroughThis PR extracts two independent concerns: API conversation message preparation and pending edit operation management. The first refactoring moves reasoning/thinking block injection and tool_result validation into a dedicated ChangesAPI Conversation Message Preparation
Pending Edit Operation Store
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/core/task/__tests__/apiConversationHistory.spec.ts (1)
103-121: 💤 Low valueConsider asserting the conversion is skipped when validation applies.
This test already verifies that
validateAndFixToolResultIdsrewriteswrong-id→tool-1. To make the boundary between the two user-message branches fully explicit, you could additionally assert that the block is still atool_result(i.e., not converted to atext"Tool result:\n…" block). The currenttoEqualdoes this implicitly, but a short comment or extraexpect(result.content[0].type).toBe("tool_result")would document intent and guard against future regressions where the branch order changes.🤖 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__/apiConversationHistory.spec.ts` around lines 103 - 121, Add an explicit assertion that the user block remains a tool_result after validation to document and guard the branch boundary in prepareApiConversationMessage; after calling prepareApiConversationMessage (used to exercise validateAndFixToolResultIds) assert result.content[0].type === "tool_result" (or equivalently expect(...).not.toBe("text")) so the test ensures the block was corrected but not converted into a text "Tool result" block.src/core/task/apiConversationHistory.ts (1)
58-62: 💤 Low valueLoss of type safety via
anycast.
messageWithTs: any(and theanyparameters inprependContentBlock/appendContentBlock) discards allAnthropic.MessageParam/ApiMessagetype checking for the assistant flow. ReturningApiMessagefrom ananyvalue also bypasses the declared return type. Consider typing this asApiMessage & Record<string, unknown>(or a small union of permitted shapes) so the assignments toreasoning_details,id, and content blocks are checked.🤖 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/apiConversationHistory.ts` around lines 58 - 62, Replace the loose any usage with a properly constrained type so TypeScript enforces expected message shape: change messageWithTs’s type from any to something like ApiMessage & Record<string, unknown> (or a small union of permitted shapes) and update the signatures of prependContentBlock and appendContentBlock to accept Anthropic.MessageParam | ApiMessage (or the same constrained type) instead of any; ensure assignments to reasoning_details, id, and added content blocks are compatible with the new type so the function returns an ApiMessage without bypassing type checks (refer to messageWithTs, prependContentBlock, appendContentBlock, reasoning_details, id, ApiMessage, and Anthropic.MessageParam).src/core/webview/PendingEditOperationStore.ts (1)
11-12: 💤 Low valueConsider consolidating identical type aliases.
PendingEditOperationInputandPendingEditOperationVieware structurally identical. While they serve different semantic purposes (input tosetvs output fromget), using the same type alias for both would reduce duplication.♻️ Consolidate type aliases
-export type PendingEditOperationInput = Omit<PendingEditOperation, "timeoutId" | "createdAt"> -export type PendingEditOperationView = Omit<PendingEditOperation, "timeoutId" | "createdAt"> +export type PendingEditOperationData = Omit<PendingEditOperation, "timeoutId" | "createdAt">Then update the method signatures:
set(operationId: string, editData: PendingEditOperationData): void get(operationId: string): PendingEditOperationData | undefined🤖 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/webview/PendingEditOperationStore.ts` around lines 11 - 12, The two identical type aliases PendingEditOperationInput and PendingEditOperationView should be consolidated into a single alias (e.g., PendingEditOperationData) and then update all uses: replace PendingEditOperationInput and PendingEditOperationView with the new PendingEditOperationData, and adjust the store method signatures so set(operationId: string, editData: PendingEditOperationData): void and get(operationId: string): PendingEditOperationData | undefined; update any references inside PendingEditOperationStore (methods set and get and related callers) to use the single consolidated alias.
🤖 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.
Nitpick comments:
In `@src/core/task/__tests__/apiConversationHistory.spec.ts`:
- Around line 103-121: Add an explicit assertion that the user block remains a
tool_result after validation to document and guard the branch boundary in
prepareApiConversationMessage; after calling prepareApiConversationMessage (used
to exercise validateAndFixToolResultIds) assert result.content[0].type ===
"tool_result" (or equivalently expect(...).not.toBe("text")) so the test ensures
the block was corrected but not converted into a text "Tool result" block.
In `@src/core/task/apiConversationHistory.ts`:
- Around line 58-62: Replace the loose any usage with a properly constrained
type so TypeScript enforces expected message shape: change messageWithTs’s type
from any to something like ApiMessage & Record<string, unknown> (or a small
union of permitted shapes) and update the signatures of prependContentBlock and
appendContentBlock to accept Anthropic.MessageParam | ApiMessage (or the same
constrained type) instead of any; ensure assignments to reasoning_details, id,
and added content blocks are compatible with the new type so the function
returns an ApiMessage without bypassing type checks (refer to messageWithTs,
prependContentBlock, appendContentBlock, reasoning_details, id, ApiMessage, and
Anthropic.MessageParam).
In `@src/core/webview/PendingEditOperationStore.ts`:
- Around line 11-12: The two identical type aliases PendingEditOperationInput
and PendingEditOperationView should be consolidated into a single alias (e.g.,
PendingEditOperationData) and then update all uses: replace
PendingEditOperationInput and PendingEditOperationView with the new
PendingEditOperationData, and adjust the store method signatures so
set(operationId: string, editData: PendingEditOperationData): void and
get(operationId: string): PendingEditOperationData | undefined; update any
references inside PendingEditOperationStore (methods set and get and related
callers) to use the single consolidated alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76891c15-78df-40bd-80cf-8c6741dcfb24
📒 Files selected for processing (6)
src/core/task/Task.tssrc/core/task/__tests__/apiConversationHistory.spec.tssrc/core/task/apiConversationHistory.tssrc/core/webview/ClineProvider.tssrc/core/webview/PendingEditOperationStore.tssrc/core/webview/__tests__/PendingEditOperationStore.spec.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Adds the two
prepareApiConversationMessageedge-case tests requested during review of #27:getThoughtSignaturefalls back to a generic reasoning blocktool_resultcontent is converted to text when the last effective history message is not an assistantValidation:
corepack pnpm --dir src exec vitest run core/task/__tests__/apiConversationHistory.spec.tscorepack pnpm --dir src check-typesNote: this branch is based on
doctarock:issue-8-monolith-refactor, so it contains the PR #27 stack plus these additional tests.Summary by CodeRabbit
Release Notes
Refactor
Tests