Agent: re-enable bindings generation CI check #5851
Conversation
Fixes CODY-3809 Previously, we didn't automatically generate Kotlin bindings when making changes to the agent protocol. This was problematic because it meant that JetBrains, Visual Studio, and Eclipse teams need to manually write the bindings to stay updated with the latest Cody. Now, we re-enable the CI check. In addition, the codegen tool also reports much more helpful errors now to increase the odds that a developer can work around codegen issues in the future when making changes to protocol types. We have made a mistake in the past of leaking too many internal types into the protocol. This is problematic because internal types tend to lean heavily on advanced TypeScript typing techniques that are complicated to translate to other languages. Instead, we should start using more actively `Protocol*` named types, which mirror internal types but don't rely on advanced type machinery. This is a bit extra work on the agent side, but increases the odds we can keep the bindings generation enabled in CI. Internal types are regulary updated with features we don't need to care about in other IDE clients so this technique should also lower the burden on VSC changes to constantly run `pnpm generate-agent-kotlin-bindings`.
| @@ -70,7 +69,10 @@ export type ClientRequests = { | |||
|
|
|||
| // history is Map of {endpoint}-{username} to chat transcripts by date | |||
| 'chat/import': [ | |||
| { history: Record<string, Record<string, SerializedChatTranscript>>; merge: boolean }, | |||
| { | |||
There was a problem hiding this comment.
I have the biome extension installed on my computer, but for some reason I was getting tons of formatting changes. Probably due to a different version or something 😢
| // anyways. As a rule of thumb, we should try to avoid leaking internal types | ||
| // that are constantly making tiny changes that are irrelevant for the other | ||
| // clients anyways. | ||
| export type ProtocolAuthStatus = ProtocolAuthenticatedAuthStatus | ProtocolUnauthenticatedAuthStatus |
There was a problem hiding this comment.
The docstring here is important documentation. We need to start using these kinds of types more actively to keep these bindings alive. The other alternative is to move entirely to something like TypeSpec/Protobuf so 100% of the types in the protocol don't come anywhere from internal code.
| this.queueClassLikeType(memberType, member, 'parameter') | ||
| } catch (error) { | ||
| const stack = error instanceof Error ? '\n' + error.stack : '' | ||
| const errorMessage = `error handling member: ${member.symbol}. To fix this problem, you may want to ignore it from code generation by adding the symbol name to the "ignoredProperties" in the Formatter.ts file.\n${error}${stack}` |
There was a problem hiding this comment.
These new error messages make it 100x easier to resolve some common errors. I didn't spend super much time on formulating the message so any feedback is appreciated 🙏🏻 The short explanation is that we can often fix errors by ignoring properties with weird types because they're not relevant for the protocol anyways.
| // throw new TypeError(`type has no properties: ${this.debug(type)}`) | ||
| this.reporter.error('', `type has no properties: ${this.debug(type)}`) | ||
| return [] | ||
| throw new TypeError(`type has no properties: ${this.debug(type)}`) |
There was a problem hiding this comment.
This change was OK because we now report much more helpful error messages
| @@ -8,31 +8,23 @@ import com.google.gson.JsonDeserializer; | |||
| import com.google.gson.JsonElement; | |||
| import java.lang.reflect.Type; | |||
|
|
|||
| sealed class AuthStatus { | |||
| sealed class ProtocolAuthStatus { | |||
There was a problem hiding this comment.
@pkukielka FYI this overwrites your manual changes. This type now uses a new status: string field to discriminate between cases.
|
Closing this as superseded by #5854 |
Fixes CODY-3809
Previously, we didn't automatically generate Kotlin bindings when making changes to the agent protocol. This was problematic because it meant that JetBrains, Visual Studio, and Eclipse teams need to manually write the bindings to stay updated with the latest Cody.
Now, we re-enable the CI check. In addition, the codegen tool also reports much more helpful errors now to increase the odds that a developer can work around codegen issues in the future when making changes to protocol types.
We have made a mistake in the past of leaking too many internal types into the protocol. This is problematic because internal types tend to lean heavily on advanced TypeScript typing techniques that are complicated to translate to other languages. Instead, we should start using more actively
Protocol*named types, which mirror internal types but don't rely on advanced type machinery. This is a bit extra work on the agent side, but increases the odds we can keep the bindings generation enabled in CI. Internal types are regulary updated with features we don't need to care about in other IDE clients so this technique should also lower the burden on VSC changes to constantly runpnpm generate-agent-kotlin-bindings.Test plan
Green CI.
Changelog