Skip to content

Conversation

@maxbrunet
Copy link
Contributor

@maxbrunet maxbrunet commented Nov 4, 2025

Description

[ What changed? Feel free to be brief. ]

  • Add support for reasoning in Cohere provider
  • Add reasoning/vision models
  • Add translate models (I was a bit hesitant, but I thought working on i18n of an app could be a good use case, and there is no particular extra work to support it)
  • Add basic tests for Cohere provider

AI Code Review

  • Team members only: AI review runs automatically when PR is opened or marked ready for review
  • Team members can also trigger a review by commenting @continue-review

Checklist

  • I've read the contributing guide
  • The relevant docs, if any, have been updated or created
  • The relevant tests, if any, have been updated or created

Screen recording or screenshot

[ When applicable, please include a short screen recording or screenshot - this makes it much easier for us as contributors to review and understand your changes. See this PR as a good example. ]

Tests

[ What tests were added or updated to ensure the changes work as expected? ]

  • Update toolSupport tests for vision model
  • Add basic tests for Cohere provider

Summary by cubic

Add Cohere Reasoning, Vision, and Translate model support across the provider, UI, and docs. Enables thinking traces, image input, and new models with correct tool-calling behavior and tests.

  • New Features
    • Reasoning: handle thinking content (stream/non-stream), configurable token_budget, and autodetect for command-a-reasoning models.
    • Vision: accept image_url inputs; disable tool use for command-a-vision models.
    • Translate: add command-a-translate model.
    • Message shaping: merge thinking with assistant messages and support tool_calls correctly.
    • Ecosystem updates: VS Code schema, GUI model picker, llm-info metadata, and docs capability matrix.
    • Tests: add comprehensive Cohere provider tests and tool support tests.

Written for commit b7f0f42. Summary will update automatically on new commits.

@maxbrunet maxbrunet requested a review from a team as a code owner November 4, 2025 23:28
@maxbrunet maxbrunet requested review from RomneyDa and removed request for a team November 4, 2025 23:28
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Nov 4, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 9 files

Prompt for AI agents (all 1 issues)

Understand the root cause of the following 1 issues and fix them.


<file name="core/llm/llms/Cohere.ts">

<violation number="1" location="core/llm/llms/Cohere.ts:83">
Wrapping assistant content in a `{type: &quot;text&quot;}` block assigns arrays of `MessagePart`s to the `text` field, corrupting structured assistant replies when they are already multi-part.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

} else {
msg.content.push({
type: "text",
text: m.content,
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Nov 4, 2025

Choose a reason for hiding this comment

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

Wrapping assistant content in a {type: "text"} block assigns arrays of MessageParts to the text field, corrupting structured assistant replies when they are already multi-part.

Prompt for AI agents
Address the following comment on core/llm/llms/Cohere.ts at line 83:

<comment>Wrapping assistant content in a `{type: &quot;text&quot;}` block assigns arrays of `MessagePart`s to the `text` field, corrupting structured assistant replies when they are already multi-part.</comment>

<file context>
@@ -48,36 +47,44 @@ class Cohere extends BaseLLM {
+          } else {
+            msg.content.push({
+              type: &quot;text&quot;,
+              text: m.content,
             });
-            lastToolPlan = undefined;
</file context>

✅ Addressed in 0579434

Copy link
Collaborator

Choose a reason for hiding this comment

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

@maxbrunet could you check this feedback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that was a real problem, the provider never creates blocks from response, but addressed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback! I've saved this as a new learning to improve future reviews.

Copy link
Collaborator

@RomneyDa RomneyDa left a comment

Choose a reason for hiding this comment

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

@maxbrunet appreciate this contribution and thanks for the tests.
Pretty much looks good to me, could you check on the comments briefly including the cubic one? Then should be good to merge

{
model: "command-a-translate-08-2025",
displayName: "Command A Translate 08-2025",
contextLength: 8000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably 8192, not critical

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is documented as 8k and I wanted to be consistent with the others (128k, 256k). In all cases, there is always a little bit of wiggle room for the system prompt and for a minimal completion response

switch (value.type) {
// https://docs.cohere.com/v2/docs/streaming#content-delta
case "content-delta":
if (value.delta.message.content.thinking) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@maxbrunet just to double check can content deltas also have text that would be lost here? or is it thinking OR text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

} else {
msg.content.push({
type: "text",
text: m.content,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@maxbrunet could you check this feedback?

Copy link
Contributor Author

@maxbrunet maxbrunet left a comment

Choose a reason for hiding this comment

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

Thank you @RomneyDa for the review!

} else {
msg.content.push({
type: "text",
text: m.content,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that was a real problem, the provider never creates blocks from response, but addressed it

switch (value.type) {
// https://docs.cohere.com/v2/docs/streaming#content-delta
case "content-delta":
if (value.delta.message.content.thinking) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

{
model: "command-a-translate-08-2025",
displayName: "Command A Translate 08-2025",
contextLength: 8000,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is documented as 8k and I wanted to be consistent with the others (128k, 256k). In all cases, there is always a little bit of wiggle room for the system prompt and for a minimal completion response

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 1 file (reviewed changes from recent commits).

1 issue found across 1 file

Prompt for AI agents (all 1 issues)

Understand the root cause of the following 1 issues and fix them.


<file name="core/llm/llms/Cohere.ts">

<violation number="1" location="core/llm/llms/Cohere.ts:87">
Overwriting `msg.content` here discards any previously merged thinking segment when we reuse the prior message, so the reasoning trace is lost. Append the new parts instead of replacing the array to keep earlier content intact.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants