-
Notifications
You must be signed in to change notification settings - Fork 862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AI Chat: Remove Claude Instant model in favor of Claude Haiku and Claude Sonnet #23799
Conversation
5587978
to
23d19f3
Compare
// Unretained is ok as credential manager is owned by this class, | ||
// and it owns the mojo binding that is used to make async call in | ||
// |GetPremiumStatus|. | ||
base::Unretained(this))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:
- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
Should we also address brave/brave-browser#38071 and brave/brave-browser#38195 in this PR? Should be a minimal addition. |
00fcd8a
to
0781484
Compare
0781484
to
de61f08
Compare
The server-side doesn't seem ready for that, so I'd prefer to keep them separate. Also the QA work for this is larger, so would like to allow them to get started verifying instead of waiting. |
Verification
|
example | example |
---|---|
Upgrades - PASSED
Case 1: Non-Premium
user (Claude 3 Haiku
) - PASSED
Steps:
- installed
1.66.118
- launched Brave using
--enable-features=AIChat:conversation_api\true --env-leo=staging --env-ai-chat.bsg=dev --env-ai-chat-premium.bsg=dev
- opened
brave://settings/leo-assistant
- changed
Default model for new conversations
fromMixtral
toClaude Instant
- loaded
https://en.wikipedia.org/wiki/Mozilla_Thunderbird
- clicked on
Show Sidebar
- clicked on
Leo
- clicked on
Summarize this page
- NOTE: experienced a variant of Google doc summary is not rendered with Mixtral model when not opted in yet brave-browser#36086 😭
- quit Brave
- upgraded to
1.68.64
, keeping profile intact, and renaming toBrave-Browser-Nightly
- launched Brave
- opened
brave://settings/leo-assistant
- confirmed
Claude 3 Haiku
is now shown asDefault model for new conversations
- clicked
Summarize this page
Confirmed Claude 3 Haiku
successfully summarized the Wikipedia page and supplanted Claude Instant
, which is removed
example | example | example | example |
---|---|---|---|
Case 2: Premium
user (Claude 3 Sonnet
) - PASSED
Steps:
- installed
1.66.118
- launched Brave using
--enable-features=AIChat:conversation_api\true --env-leo=staging --env-ai-chat.bsg=dev --env-ai-chat-premium.bsg=dev
- purchased and loaded
Leo
Premium
credentials fromaccount.bravesoftware.com
using[email protected]
- set
Default model for new conversations
toClaude Instant
inbrave://settings/leo-assistant
- loaded
https://www.reuters.com/world/us/white-house-race-plunges-into-uncharted-territory-trump-awaits-july-sentencing-2024-05-31/
- clicked on
Summarize this page
- clicked
I understand
- NOTE: experienced a variant of Google doc summary is not rendered with Mixtral model when not opted in yet brave-browser#36086 😭
- quit Brave
- renamed
Brave-Browser
profile toBrave-Browser-Nightly
- installed
1.68.64
- launched Brave using
--enable-logging=stderr --v=2 --enable-features=AIChat:conversation_api\true --env-leo=staging --env-ai-chat.bsg=dev --env-ai-chat-premium.bsg=dev
- opened
brave://settings/leo-assistant
- confirmed
Claude Sonnet
was the chosen default - returned to
https://www.reuters.com/world/us/white-house-race-plunges-into-uncharted-territory-trump-awaits-july-sentencing-2024-05-31/
- clicked on
Summarize this page
Confirmed new default of Premium
Claude 3 Sonnet
replaced Claude Instant
, and provided an accurate summarization
example | example | example | example | example |
---|---|---|---|---|
Also removes version (date) from model name, since server uses latest configured.
Resolves brave/brave-browser#37988
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: