Skip to content

Commit add35a6

Browse files
committed
Apply Copilot review comments.
modified: scripts/codegen/java.ts ### 18:24 Prompt Consider these Copilot review comments, all about `java.ts` - Lines 167 - 178 > The generator passes required=true when generating container element/value types (List<T>, Map<String, V>). This can produce illegal Java generic types like List<long> / Map<String, boolean> when the schema item type is integer/boolean/number, causing generated code not to compile. Fix by ensuring container element/value types are always boxed (e.g., call schemaTypeToJava(..., false, ...) for items and additionalProperties value schemas, or add a dedicated “box primitives in generics” step). ### 18:29 Prompt Consider these Copilot review comments, all about `java.ts` - Lines 193 - 201 > The generator passes required=true when generating container element/value types (List<T>, Map<String, V>). This can produce illegal Java generic types like List<long> / Map<String, boolean> when the schema item type is integer/boolean/number, causing generated code not to compile. Fix by ensuring container element/value types are always boxed (e.g., call schemaTypeToJava(..., false, ...) for items and additionalProperties value schemas, or add a dedicated “box primitives in generics” step). ### 18:31 Prompt Consider these Copilot review comments, all about `java.ts` - Lines 1258 - 1261 > RpcMapper.INSTANCE is a plain new ObjectMapper() with no modules. Generated RPC DTOs can include OffsetDateTime (your type mapping emits it for format: date-time), and ObjectMapper.valueToTree(...) used by session wrappers can fail without JavaTimeModule registered. Fix by configuring this shared mapper consistently (e.g., register com.fasterxml.jackson.datatype.jsr310.JavaTimeModule, and align any other ObjectMapper features needed by the SDK). ### 18:34 Prompt Consider these Copilot review comments, all about `java.ts` - Lines 1202 - 1207 > This comment is likely incorrect/misleading: Java lambdas can generally target a functional interface whose single abstract method is generic (the compiler can still infer the type parameter from the call site). Consider removing this claim or rephrasing it to a neutral recommendation (e.g., “method reference is typical/clear”) so consumers aren’t discouraged from valid usage. Copilot suggests: ```typescript lines.push(` * (e.g., a {@code JsonRpcClient} instance). A method reference is typically the clearest`); lines.push(` * way to adapt a generic {@code invoke} method to this interface:`); lines.push(` * <pre>{@code`); lines.push(` * RpcCaller caller = jsonRpcClient::invoke;`); lines.push(` * }</pre>`); ``` What can we do about this? ### 18:36 Prompt Consider these Copilot review comments, all about `java.ts` - Lines 848 - 854 > For session-scoped RPC methods with additional params, the generated *Params records still include a sessionId field (because they’re generated directly from the schema), but the wrapper then overwrites sessionId via _p.put("sessionId", this.sessionId). This duplication is confusing for API consumers and makes the param records look “callable” with arbitrary session IDs when they are not. Consider adjusting generation so sessionId is omitted from session-scoped params records (and only injected by SessionRpc), or documenting clearly in the generated Javadoc that any provided sessionId is ignored/overridden. Copilot suggests: ```typescript * Return the wrapper-visible parameter property names for a method. * For session-scoped wrappers, sessionId is injected by SessionRpc and is not * considered a user-supplied parameter. */ function wrapperParamPropertyNames(method: RpcMethodNode, isSession: boolean): string[] { if (!method.params || typeof method.params !== "object") return []; const props = method.params.properties ?? {}; return Object.keys(props).filter((k) => !(isSession && k === "sessionId")); } /** * Return the params class name if the method has wrapper-visible properties * (i.e. user-supplied parameters after filtering out injected sessionId for * session-scoped wrappers). */ function wrapperParamsClassName(method: RpcMethodNode, isSession: boolean): string | null { const userProps = wrapperParamPropertyNames(method, isSession); ``` What can we do about this? ### 18:40 Prompt Consider these Copilot review comments, all about `java.ts` - Lines 903 - 915 > For session-scoped RPC methods with additional params, the generated *Params records still include a sessionId field (because they’re generated directly from the schema), but the wrapper then overwrites sessionId via _p.put("sessionId", this.sessionId). This duplication is confusing for API consumers and makes the param records look “callable” with arbitrary session IDs when they are not. Consider adjusting generation so sessionId is omitted from session-scoped params records (and only injected by SessionRpc), or documenting clearly in the generated Javadoc that any provided sessionId is ignored/overridden. What can we do about this? This seems the same as the previous comment. No? modified: src/generated/java/com/github/copilot/sdk/generated/rpc/RpcCaller.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/RpcMapper.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionAgentApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionCommandsApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionExtensionsApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionFleetApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionHistoryApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionMcpApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionModeApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionModelApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionPermissionsApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionPlanApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionRpc.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionShellApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionSkillsApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionToolsApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionUiApi.java modified: src/generated/java/com/github/copilot/sdk/generated/rpc/SessionWorkspaceApi.java - Regenerated. Signed-off-by: Ed Burns <edburns@microsoft.com>
1 parent 0beb1d8 commit add35a6

19 files changed

+81
-10
lines changed

scripts/codegen/java.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,8 @@ function schemaTypeToJava(
167167
if (schema.type === "array") {
168168
const items = schema.items as JSONSchema7 | undefined;
169169
if (items) {
170-
const itemResult = schemaTypeToJava(items, true, context, propName + "Item", nestedTypes);
170+
// Always pass required=false so primitives are boxed (List<Long>, not List<long>)
171+
const itemResult = schemaTypeToJava(items, false, context, propName + "Item", nestedTypes);
171172
imports.add("java.util.List");
172173
for (const imp of itemResult.imports) imports.add(imp);
173174
return { javaType: `List<${itemResult.javaType}>`, imports };
@@ -194,7 +195,8 @@ function schemaTypeToJava(
194195
const valueSchema = typeof schema.additionalProperties === "object"
195196
? schema.additionalProperties as JSONSchema7
196197
: { type: "object" } as JSONSchema7;
197-
const valueResult = schemaTypeToJava(valueSchema, true, context, propName + "Value", nestedTypes);
198+
// Always pass required=false so primitives are boxed (Map<String, Long>, not Map<String, long>)
199+
const valueResult = schemaTypeToJava(valueSchema, false, context, propName + "Value", nestedTypes);
198200
imports.add("java.util.Map");
199201
for (const imp of valueResult.imports) imports.add(imp);
200202
return { javaType: `Map<String, ${valueResult.javaType}>`, imports };
@@ -885,6 +887,11 @@ function generateApiMethod(
885887
?? `Invokes {@code ${method.rpcMethod}}.`;
886888
lines.push(` /**`);
887889
lines.push(` * ${description}`);
890+
if (isSession && hasExtraParams && hasSessionId) {
891+
lines.push(` * <p>`);
892+
lines.push(` * Note: the {@code sessionId} field in the params record is overridden`);
893+
lines.push(` * by the session-scoped wrapper; any value provided is ignored.`);
894+
}
888895
if (method.stability === "experimental") {
889896
lines.push(` *`);
890897
lines.push(` * @apiNote This method is experimental and may change in a future version.`);
@@ -1199,12 +1206,11 @@ async function generateRpcCallerInterface(packageName: string, packageDir: strin
11991206
lines.push(` * Interface for invoking JSON-RPC methods with typed responses.`);
12001207
lines.push(` * <p>`);
12011208
lines.push(` * Implementations delegate to the underlying transport layer`);
1202-
lines.push(` * (e.g., a {@code JsonRpcClient} instance). Use a method reference:`);
1209+
lines.push(` * (e.g., a {@code JsonRpcClient} instance). A method reference is typically the clearest`);
1210+
lines.push(` * way to adapt a generic {@code invoke} method to this interface:`);
12031211
lines.push(` * <pre>{@code`);
12041212
lines.push(` * RpcCaller caller = jsonRpcClient::invoke;`);
12051213
lines.push(` * }</pre>`);
1206-
lines.push(` * Note: because the {@code invoke} method has a type parameter, this interface cannot`);
1207-
lines.push(` * be implemented using a lambda expression — use a method reference or anonymous class.`);
12081214
lines.push(` *`);
12091215
lines.push(` * @since 1.0.0`);
12101216
lines.push(` */`);
@@ -1256,7 +1262,7 @@ async function generateRpcMapperClass(packageName: string, packageDir: string):
12561262
lines.push(`final class RpcMapper {`);
12571263
lines.push(``);
12581264
lines.push(` static final com.fasterxml.jackson.databind.ObjectMapper INSTANCE =`);
1259-
lines.push(` new com.fasterxml.jackson.databind.ObjectMapper();`);
1265+
lines.push(` com.github.copilot.sdk.JsonRpcClient.getObjectMapper();`);
12601266
lines.push(``);
12611267
lines.push(` private RpcMapper() {}`);
12621268
lines.push(`}`);

src/generated/java/com/github/copilot/sdk/generated/rpc/RpcCaller.java

Lines changed: 2 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/RpcMapper.java

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionAgentApi.java

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionCommandsApi.java

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionExtensionsApi.java

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionFleetApi.java

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionHistoryApi.java

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionMcpApi.java

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/rpc/SessionModeApi.java

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)