Conversation
PiperOrigin-RevId: 985463163
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request rewrites the experimental Gemini API CLI from a Bun/Node.js-based project to a Go-based CLI using Cobra and the Speakeasy SDK. The review feedback identifies two critical issues: first, the required flag validation in guardRequiredFlags fails to detect missing slice or array flags because their default value is represented as "[]" rather than ""; second, a type mismatch compile error exists in internal/cli/custom/tts.go where *int64 pointer fields are assigned directly to int variables without explicit casting.
| c.LocalFlags().VisitAll(func(f *pflag.Flag) { | ||
| required := len(f.Annotations[flagutil.AnnotationRequired]) > 0 | ||
| blank := f.Value.Type() == "string" && strings.TrimSpace(f.Value.String()) == "" | ||
| if required && (blank || (!f.Changed && f.DefValue == "")) { | ||
| missing = append(missing, "--"+f.Name) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The current required flag validation in guardRequiredFlags only checks if a flag is of type string and is blank, or if it is not changed and has an empty default value (f.DefValue == ""). However, for required slice or array flags (like stringSlice or stringArray), the default value in Cobra is represented as "[]" rather than "". As a result, required slice/array flags that are omitted by the user are not detected as missing, which can lead to invalid API requests.
To fix this, we should check if the flag is unset (not changed) and has an empty string or empty slice representation as its value.
| c.LocalFlags().VisitAll(func(f *pflag.Flag) { | |
| required := len(f.Annotations[flagutil.AnnotationRequired]) > 0 | |
| blank := f.Value.Type() == "string" && strings.TrimSpace(f.Value.String()) == "" | |
| if required && (blank || (!f.Changed && f.DefValue == "")) { | |
| missing = append(missing, "--"+f.Name) | |
| } | |
| }) | |
| c.LocalFlags().VisitAll(func(f *pflag.Flag) { | |
| required := len(f.Annotations[flagutil.AnnotationRequired]) > 0 | |
| if !required { | |
| return | |
| } | |
| isDefault := !f.Changed | |
| isEmpty := f.Value.String() == "" || f.Value.String() == "[]" | |
| if (isDefault && isEmpty) || (f.Value.Type() == "string" && strings.TrimSpace(f.Value.String()) == "") { | |
| missing = append(missing, "--"+f.Name) | |
| } | |
| }) |
| blockMIME, blockChannels, blockRate := "", 0, 0 | ||
| if ac.MimeType != nil { | ||
| blockMIME = string(*ac.MimeType) | ||
| } | ||
| if ac.Channels != nil { | ||
| blockChannels = *ac.Channels | ||
| } | ||
| if ac.SampleRate != nil { | ||
| blockRate = *ac.SampleRate | ||
| } |
There was a problem hiding this comment.
In Speakeasy generated Go SDKs, integer fields in schemas (like channels and sample_rate) are typically represented as *int64 pointers. Since blockChannels and blockRate are inferred as int (from the 0 literal), directly assigning *ac.Channels or *ac.SampleRate to them will cause a type mismatch compile error.
To prevent this, we should explicitly cast the dereferenced values to int.
| blockMIME, blockChannels, blockRate := "", 0, 0 | |
| if ac.MimeType != nil { | |
| blockMIME = string(*ac.MimeType) | |
| } | |
| if ac.Channels != nil { | |
| blockChannels = *ac.Channels | |
| } | |
| if ac.SampleRate != nil { | |
| blockRate = *ac.SampleRate | |
| } | |
| blockMIME, blockChannels, blockRate := "", 0, 0 | |
| if ac.MimeType != nil { | |
| blockMIME = string(*ac.MimeType) | |
| } | |
| if ac.Channels != nil { | |
| blockChannels = int(*ac.Channels) | |
| } | |
| if ac.SampleRate != nil { | |
| blockRate = int(*ac.SampleRate) | |
| } |
|
What is this e mail? I dont understand what is it??
בתאריך יום ב׳, 21 בספט׳ 2026, 23:12, מאת gemini-code-assist[bot] <
***@***.***>:
… ***@***.***[bot]* commented on this pull request.
Code Review
This pull request rewrites the experimental Gemini API CLI from a
Bun/Node.js-based project to a Go-based CLI using Cobra and the Speakeasy
SDK. The review feedback identifies two critical issues: first, the
required flag validation in guardRequiredFlags fails to detect missing
slice or array flags because their default value is represented as "[]"
rather than ""; second, a type mismatch compile error exists in
internal/cli/custom/tts.go where *int64 pointer fields are assigned
directly to int variables without explicit casting.
------------------------------
In internal/cli/custom/register.go
<#22 (comment)>
:
> + c.LocalFlags().VisitAll(func(f *pflag.Flag) {
+ required := len(f.Annotations[flagutil.AnnotationRequired]) > 0
+ blank := f.Value.Type() == "string" && strings.TrimSpace(f.Value.String()) == ""
+ if required && (blank || (!f.Changed && f.DefValue == "")) {
+ missing = append(missing, "--"+f.Name)
+ }
+ })
[image: high]
<https://camo.githubusercontent.com/7559374fd248a2a146dfe7112beda558979c4f6d02dbe7c5161fd893fe834423/68747470733a2f2f7777772e677374617469632e636f6d2f636f64657265766965776167656e742f686967682d7072696f726974792e737667>
The current required flag validation in guardRequiredFlags only checks if
a flag is of type string and is blank, or if it is not changed and has an
empty default value (f.DefValue == ""). However, for required slice or
array flags (like stringSlice or stringArray), the default value in Cobra
is represented as "[]" rather than "". As a result, required slice/array
flags that are omitted by the user are not detected as missing, which can
lead to invalid API requests.
To fix this, we should check if the flag is unset (not changed) and has an
empty string or empty slice representation as its value.
⬇️ Suggested change
- c.LocalFlags().VisitAll(func(f *pflag.Flag) {
- required := len(f.Annotations[flagutil.AnnotationRequired]) > 0
- blank := f.Value.Type() == "string" && strings.TrimSpace(f.Value.String()) == ""
- if required && (blank || (!f.Changed && f.DefValue == "")) {
- missing = append(missing, "--"+f.Name)
- }
- })
+ c.LocalFlags().VisitAll(func(f *pflag.Flag) {
+ required := len(f.Annotations[flagutil.AnnotationRequired]) > 0
+ if !required {
+ return
+ }
+ isDefault := !f.Changed
+ isEmpty := f.Value.String() == "" || f.Value.String() == "[]"
+ if (isDefault && isEmpty) || (f.Value.Type() == "string" && strings.TrimSpace(f.Value.String()) == "") {
+ missing = append(missing, "--"+f.Name)
+ }
+ })
------------------------------
In internal/cli/custom/tts.go
<#22 (comment)>
:
> + blockMIME, blockChannels, blockRate := "", 0, 0
+ if ac.MimeType != nil {
+ blockMIME = string(*ac.MimeType)
+ }
+ if ac.Channels != nil {
+ blockChannels = *ac.Channels
+ }
+ if ac.SampleRate != nil {
+ blockRate = *ac.SampleRate
+ }
[image: high]
<https://camo.githubusercontent.com/7559374fd248a2a146dfe7112beda558979c4f6d02dbe7c5161fd893fe834423/68747470733a2f2f7777772e677374617469632e636f6d2f636f64657265766965776167656e742f686967682d7072696f726974792e737667>
In Speakeasy generated Go SDKs, integer fields in schemas (like channels
and sample_rate) are typically represented as *int64 pointers. Since
blockChannels and blockRate are inferred as int (from the 0 literal),
directly assigning *ac.Channels or *ac.SampleRate to them will cause a
type mismatch compile error.
To prevent this, we should explicitly cast the dereferenced values to int.
⬇️ Suggested change
- blockMIME, blockChannels, blockRate := "", 0, 0
- if ac.MimeType != nil {
- blockMIME = string(*ac.MimeType)
- }
- if ac.Channels != nil {
- blockChannels = *ac.Channels
- }
- if ac.SampleRate != nil {
- blockRate = *ac.SampleRate
- }
+ blockMIME, blockChannels, blockRate := "", 0, 0
+ if ac.MimeType != nil {
+ blockMIME = string(*ac.MimeType)
+ }
+ if ac.Channels != nil {
+ blockChannels = int(*ac.Channels)
+ }
+ if ac.SampleRate != nil {
+ blockRate = int(*ac.SampleRate)
+ }
—
Reply to this email directly, view it on GitHub
<#22?email_source=notifications&email_token=CMWAUNULOKO443VAESKBH6T5QGDTRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMRXGEZTMMBUG442M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#pullrequestreview-5271360479>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CMWAUNVSTMGQBR3IVMB3SYD5QGDTRAVCNFSNUABGKJSXA33TNF2G64TZHMYTEMRUG4YTKMBRGM5US43TOVSTWNJVGMYTSMJWGA4TTILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CMWAUNVLABYXYCVEIIQM4HD5QGDTRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMRXGEZTMMBUG442M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/CMWAUNR47D7ZY2PVINVGO6D5QGDTRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMRXGEZTMMBUG442M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Hello BenJomy, I apologize for the confusion. The email you received is an automated notification from the Gemini Code Assist bot regarding the pull request you are currently reviewing. It summarizes the changes in the pull request and provides specific code review feedback to help improve the code quality and ensure it meets project standards. You can safely ignore the email if you have already reviewed the pull request or if you do not need to take any action on it. |
2 participants
No public description