-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add Vertex Claude Sonnet 4 1M context support #7848
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
feat: add Vertex Claude Sonnet 4 1M context support #7848
Conversation
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.
The key 'vertex1MContextLabel' uses an inconsistent language: the string "1M context window (Preview) aktivieren" mixes English with German. Consider changing it to fully German, for example "1M Kontextfenster (Vorschau) aktivieren", to maintain consistency with the rest of the file.
| "vertex1MContextLabel": "1M context window (Preview) aktivieren", | |
| "vertex1MContextLabel": "1M Kontextfenster (Vorschau) aktivieren", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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.
Addressed in updated PR
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.
Typographical suggestion: The key value uses “內容視窗” but for consistency with other keys, it might be intended to be "上下文視窗".
| "vertex1MContextLabel": "啟用 1M 內容視窗(預覽)", | |
| "vertex1MContextLabel": "啟用 1M 上下文視窗(預覽)", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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.
Addressed in updated PR
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.
Typographical suggestion: Consider changing “內容視窗” to "上下文視窗" in the description for consistency with similar keys.
| "vertex1MContextDescription": "為 Claude Sonnet 4 將內容視窗擴展至 100 萬個 token。此為預覽功能,可能不適用於所有使用者。", | |
| "vertex1MContextDescription": "為 Claude Sonnet 4 將上下文視窗擴展至 100 萬個 token。此為預覽功能,可能不適用於所有使用者。", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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.
Addressed in updated PR
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.
Thank you for your contribution! I've reviewed the changes for adding Vertex Claude Sonnet 4 1M context support and found several issues that need attention.
.gitignore
Outdated
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.
These changes appear unrelated to the 1M context feature. Could you remove these local development entries from this PR? They should be in a separate PR or kept in your local .git/info/exclude file.
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.
Unrelated local development ignore entries have been reverted. Only project-relevant ignore patterns remain.
.husky/pre-commit
Outdated
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.
Is this intentional? Adding test execution to the pre-commit hook is unrelated to the 1M context feature and will affect all developers. This change should be discussed separately and not included in this feature PR.
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.
The pre-commit test execution has been reverted and is no longer included, this was intended for a local change.
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.
Could we improve the error handling here? Currently, errors are silently ignored which could mask configuration issues. Consider:
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.
Error handling for the credential file load is now clear and visible, logging configuration issues with a standard message consistent with project conventions.
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.
Is setting vertex1MContext: false here intentional? This seems inconsistent with how other provider-specific fields are handled. Consider removing this default initialization since it's provider-specific.
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.
The default config no longer sets vertex1MContext: false. This provider-specific key is now initialized only via migration logic where relevant, restoring consistency with project patterns.
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.
The Catalan translations are still in English. Should these be translated to Catalan for consistency?
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.
All new Catalan translation keys are now fully and accurately translated; no English remains in user-facing content.
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.
The diff shows '...[2558 characters omitted]...' which suggests test content may be truncated or missing. Could you verify that all test implementations are complete?
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.
The test file is byte-for-byte identical to upstream/main except for a minimal, appended set of new tests for 1M context support. There is no structural truncation, no "rest of file unchanged" lines, and no block reordering. The original "characters omitted" was due to Roo repeatedly failling flle-edits and trying to restore the entire file.
3f5c1b0 to
9ae1a0e
Compare
|
Hey @victoraranda Thank you for your contribution, I still see some unrelated changes around the |
9ae1a0e to
0263ca0
Compare
…, and test support
0263ca0 to
29781ca
Compare
… in streaming and UI (PR RooCodeInc#7848)
|
Stale. Please open new PR if still interested. |
This PR introduces support for the Claude Sonnet 4 model on Vertex AI with a 1M token context window.
Key updates: