diff --git a/1682-java-tool-ergonomics-prompts-remove-before-merge/20260622-prompts.md b/1682-java-tool-ergonomics-prompts-remove-before-merge/20260622-prompts.md index 39109bee2..5f453e841 100644 --- a/1682-java-tool-ergonomics-prompts-remove-before-merge/20260622-prompts.md +++ b/1682-java-tool-ergonomics-prompts-remove-before-merge/20260622-prompts.md @@ -737,3 +737,274 @@ For Java's `NONE("")`: if Jackson serializes it, it would emit `"defer": ""` — For all the created issues please find the right place to add the following requirement: ✅✅ YOU MUST run `mvn spotless:apply` before every commit. + +and add it. + +### 19:13 Response + + +Updated todo list + +Ran terminal command: gh issue view 1758 --repo github/copilot-sdk --json body -q .body | tail -20 + +I'll add the requirement to the Constraints section of each issue. The best placement is at the top of Constraints since it's a workflow mandate: + +Ran terminal command: cd /Users/edburns/workareas/copilot-sdk && for ISSUE in 1758 1759 1760 1761 1762; do + BODY=$(gh issue view "$ISSUE" --repo github/copilot-sdk --json body -q .body) + UPDATED=$(echo "$BODY" | sed 's/## Constraints/## Constraints\n\n- ✅✅ YOU MUST run `mvn spotless:apply` before every commit./') + echo "$UPDATED" | gh issue edit "$ISSUE" --repo github/copilot-sdk --body-file - + echo "Updated issue #$ISSUE" +done + +Done. All 5 issues (#1758–#1762) now have `✅✅ YOU MUST run mvn spotless:apply before every commit.` as the first bullet in their Constraints section. + +Created 1 todo + +### 19:13 Prompt + + +Ugh, I did a git problem. + +``` +edburns@Edwards-MacBook-Pro copilot-sdk % git push upstream +git push upstream +To github.com:github/copilot-sdk.git + ! [rejected] edburns/1682-java-tool-ergonomics -> edburns/1682-java-tool-ergonomics (non-fast-forward) +error: failed to push some refs to 'github.com:github/copilot-sdk.git' +hint: Updates were rejected because the tip of your current branch is behind +hint: its remote counterpart. If you want to integrate the remote changes, +hint: use 'git pull' before pushing again. +``` + +Please fix it and push. Make sure not to overwrite anything. + +### 19:16 Response + +Updated todo list + +Ran terminal command: git log --oneline -5 + +Ran terminal command: git log --oneline -5 upstream/edburns/1682-java-tool-ergonomics + +Ran terminal command: git fetch upstream edburns/1682-java-tool-ergonomics && git log --oneline upstream/edburns/1682-java-tool-ergonomics..HEAD + +Ran terminal command: git log --oneline HEAD..upstream/edburns/1682-java-tool-ergonomics + +Ran terminal command: git merge-base HEAD upstream/edburns/1682-java-tool-ergonomics + +Ran terminal command: git log --oneline d15cfcbe -1 + +Ran terminal command: echo "Local tree (Resolve 3.8):" && git cat-file -p e9c978bb | grep tree && echo "Remote tree (Resolve 3.8):" && git cat-file -p 622e3ae0 | grep tree + +Ran terminal command: git diff upstream/edburns/1682-java-tool-ergonomics..HEAD --stat + +Ran terminal command: git diff HEAD..upstream/edburns/1682-java-tool-ergonomics --stat + +The remote is strictly a subset of local — it just has deletions (all the same files local added). The local branch has everything the remote has plus more. A safe rebase will work: + +Ran terminal command: git rebase upstream/edburns/1682-java-tool-ergonomics + +Ran terminal command: git stash && git rebase upstream/edburns/1682-java-tool-ergonomics + +Ran terminal command: git stash pop && git log --oneline -5 + +Rebase succeeded — local is now 4 commits ahead of remote with no divergence. Now pushing: + +Ran terminal command: git push upstream edburns/1682-java-tool-ergonomics + +Fixed. The rebase detected that your local commits (with different hashes but same content) were duplicates of what was already on the remote, skipped them, and replayed only the unique commits on top. Push succeeded as a fast-forward. + +Created 3 todos + +### 19:16 Prompt + +Did you push it to upstream? + +### 19:17 Response + + +Updated todo list + + +Yes. The output confirms it pushed to `upstream`: + +``` +To github.com:github/copilot-sdk.git + 622e3ae0..170d5aaa edburns/1682-java-tool-ergonomics -> edburns/1682-java-tool-ergonomics +``` + +### 19:17 Prompt + + + +I don't see the commits at https://github.com/github/copilot-sdk/pull/1763/commits . + +Why? + +### 19:18 Response + +Updated todo list + +Ran terminal command: gh pr view 1763 --repo github/copilot-sdk --json headRefName,baseRefName,headRepository + +PR #1763 is tracking branch `copilot/edburns1682-java-tool-ergonomics`, but you pushed to `edburns/1682-java-tool-ergonomics`. They're different branches. + +You either need to: +1. Push to the PR's branch: `git push upstream edburns/1682-java-tool-ergonomics:copilot/edburns1682-java-tool-ergonomics` +2. Or update the PR to point to your branch instead. + +Which would you prefer? + +### 19:18 Prompt + +We should always and only be pushing to the PR's branch. + +Updated todo list + +Ran terminal command: git push upstream edburns/1682-java-tool-ergonomics:copilot/edburns1682-java-tool-ergonomics + +Ran terminal command: git fetch upstream copilot/edburns1682-java-tool-ergonomics && git log --oneline upstream/copilot/edburns1682-java-tool-ergonomics -5 + +Ran terminal command: git log --oneline HEAD -5 + +Ran terminal command: git diff upstream/copilot/edburns1682-java-tool-ergonomics..HEAD --stat | tail -5 + +Ran terminal command: git diff HEAD..upstream/copilot/edburns1682-java-tool-ergonomics --stat + +Local is a strict superset — the reverse diff only shows deletions (content local has that the PR branch doesn't). Nothing would be lost. This needs a force push since the histories diverged. + +Analyzing your answers... + +Ran terminal command: git push upstream edburns/1682-java-tool-ergonomics:copilot/edburns1682-java-tool-ergonomics --force-with-lease + +Done. PR #1763 now shows the commits at `https://github.com/github/copilot-sdk/pull/1763/commits`. + +Created 2 todos + +### 19:24 Prompt + +Now there are ton of files in the **Files changed** tab that are not at all related to this PR: + +https://github.com/github/copilot-sdk/pull/1763 + +Please fix that so only the files relevant to our work are in there. + +Updated todo list + +Ran terminal command: git log --oneline upstream/main..HEAD + +Ran terminal command: git diff upstream/main..HEAD --stat | tail -40 + +Ran terminal command: git show 5c360d56 --stat + +Ran terminal command: git fetch upstream main && git log --oneline upstream/main -3 + +The branch has a duplicate of commit `a3dbdd19` (already on main) with a different hash (`5c360d56`). Rebasing onto current `upstream/main` will drop it: + +Ran terminal command: git stash && git rebase upstream/main + +Git skipped the duplicate. Let me verify and push: + +Ran terminal command: git diff upstream/main..HEAD --stat && echo "---" && git log --oneline upstream/main..HEAD + +Only Java and prompt files now — no dotnet/go/nodejs/python/rust files. Pushing: + +Ran terminal command: git push upstream edburns/1682-java-tool-ergonomics:copilot/edburns1682-java-tool-ergonomics --force-with-lease && git stash pop + +Done. The PR's "Files changed" tab should now show only the 19 relevant files — all Java source, prompts, and plan files. The unrelated dotnet/go/nodejs/python/rust changes from the duplicate commit are gone. + +Created 2 todos + +### 19:29 Prompt + +Now we come back to the most important thing to fix, that `NONE` field in `java/src/main/java/com/github/copilot/rpc/ToolDefer.java`. + +Read this comment from a reviewer: https://github.com/github/copilot-sdk/pull/1763#discussion_r3455952418 + +And recall what we elicited earlier: + +> Let's take .NET. What is the behavior of the tool in the "absence" case? And what does that serialize as? + +> There's the answer: + +> **In .NET, when `Defer` is `null`, the key is simply omitted from the JSON.** +> +> The serializer uses `DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull`, so any `null`-valued property (like `Defer`, `OverridesBuiltInTool`, `SkipPermission`) is not written to the wire at all. The CLI receives a `ToolDefinition` JSON object with no `defer` key — meaning "no preference, use server default." +> +> So the wire behavior for "absence" is: **the `defer` field does not appear in the JSON-RPC message**. +> +> --- +> +> For Java's `NONE("")`: if Jackson serializes it, it would emit `"defer": ""` — which is **not the same** as omitting the key. The Java code must treat `NONE` as "do not serialize this field" (i.e., pass `null` to `ToolDefinition.create()` for the defer parameter). The `@JsonValue` of `""` is a landmine if it ever leaks to the wire. + +It seems to me at this point the best thing is to update the Javadoc so that the future work to implement the annotation processor doesn't cause the `defer` to leak onto the wire. + +Is there any other better way at this point? + +### 19:31 Response + +Updated todo list + +Read [](file:///Users/edburns/workareas/copilot-sdk/java/src/main/java/com/github/copilot/rpc/ToolDefer.java#1-1), lines 1 to 70 + +Read [](file:///Users/edburns/workareas/copilot-sdk/java/src/main/java/com/github/copilot/rpc/ToolDefer.java#70-70), lines 70 to 76 + + + +Fetched https://github.com/github/copilot-sdk/pull/1763#discussion_r3455952418 + +There's a better option than Javadoc alone: make `NONE` serialize as `null` instead of `""`. This provides a safety net at the Jackson level: + +```java +@JsonValue +public String getValue() { + return this == NONE ? null : value; +} +``` + +With this change, even if `ToolDefer.NONE` accidentally reaches a `ToolDefinition`, Jackson serializes it as `null`, and `@JsonInclude(NON_NULL)` (already on `ToolDefinition`) omits it from the wire. The bug becomes impossible regardless of what the processor does. + +The internal `""` string is still needed because Java annotations require compile-time constants (`null` can't be used as an annotation element default), but the `@JsonValue` doesn't have to expose it. + +**Tradeoff:** + +| Approach | Prevents wire leak? | Relies on downstream code? | +|----------|---------------------|---------------------------| +| Javadoc only | No — still serializes as `""` if leaked | Yes — processor must map to `null` | +| `@JsonValue` returns `null` for NONE | Yes — impossible to emit `""` | No — defense in depth | + +I'd recommend doing **both**: the `@JsonValue` fix for safety, plus a Javadoc update explaining `NONE` is an annotation-only sentinel. Want me to make those changes? + +Created 1 todo + +### 19:31 Prompt + +Yes. Please do that and commit it. But do not push it. + +Updated todo list + +Read [](file:///Users/edburns/workareas/copilot-sdk/java/src/main/java/com/github/copilot/rpc/ToolDefer.java#22-22), lines 22 to 55 + +Replacing 14 lines with 28 lines in [](file:///Users/edburns/workareas/copilot-sdk/java/src/main/java/com/github/copilot/rpc/ToolDefer.java) + + +``` + +``` + + +Ran terminal command: cd /Users/edburns/workareas/copilot-sdk/java && export JAVA_HOME="/Library/Java/JavaVirtualMachines/microsoft-25.jdk/Contents/Home" && export M2_HOME=${HOME}/Downloads/apache-maven-3.9.8 && export PATH=${M2_HOME}/bin:${JAVA_HOME}/bin:${PATH} && mvn spotless:apply -q + +Ran terminal command: cd /Users/edburns/workareas/copilot-sdk && git add java/src/main/java/com/github/copilot/rpc/ToolDefer.java && git commit -m "fix(java): make ToolDefer.NONE serialize as null to prevent wire leak + +NONE is an annotation-only sentinel for @CopilotTool(defer=...) defaults. +Its @JsonValue now returns null so @JsonInclude(NON_NULL) omits it from +the JSON-RPC payload, matching the nullable/optional semantics used by +all other SDKs (.NET CopilotToolDefer?, Node defer?, Go omitempty, +Python | None, Rust Option)." + +Committed as `b8ad8264`. Spotless check passed. Not pushed. + +Created 1 todo + +Made changes. diff --git a/java/src/main/java/com/github/copilot/rpc/ToolDefer.java b/java/src/main/java/com/github/copilot/rpc/ToolDefer.java index 5b9e115c7..ba888ca97 100644 --- a/java/src/main/java/com/github/copilot/rpc/ToolDefer.java +++ b/java/src/main/java/com/github/copilot/rpc/ToolDefer.java @@ -27,13 +27,18 @@ public enum ToolDefer { *

* This constant must not be passed to {@link ToolDefinition} factory * methods. The annotation processor and {@code ToolDefinition.fromObject()} - * must map {@code NONE} to {@code null} so the {@code defer} field is omitted - * from the JSON-RPC wire payload (matching the nullable/optional semantics used - * by all other SDKs). + * must map {@code NONE} to a {@code null} field reference so that + * {@code @JsonInclude(NON_NULL)} on {@link ToolDefinition} omits the + * {@code defer} key from the JSON-RPC wire payload entirely (matching the + * nullable/optional semantics used by all other SDKs). *

- * As a safety net, {@link #getValue()} returns {@code null} for this constant, - * so {@code @JsonInclude(NON_NULL)} will omit it even if it accidentally - * reaches serialization. + * As a secondary safety net, {@link #getValue()} returns {@code null} for this + * constant. Note that this alone does not cause field omission: if a + * non-null {@code NONE} reference reaches a {@link ToolDefinition} field, + * Jackson's {@code @JsonInclude(NON_NULL)} will still emit the field (as + * {@code "defer": null}) because the field reference itself is not null. The + * primary protection is mapping {@code NONE} to a null field reference before + * constructing the {@link ToolDefinition}. */ NONE(""), @@ -52,8 +57,10 @@ public enum ToolDefer { /** * Returns the JSON value for this deferral mode. *

- * Returns {@code null} for {@link #NONE} so that {@code @JsonInclude(NON_NULL)} - * omits it from the wire payload. + * Returns {@code null} for {@link #NONE} to avoid emitting an empty string + * ({@code "defer": ""}) if this sentinel accidentally reaches serialization. + * With {@code null}, the worst-case leak becomes {@code "defer": null} rather + * than an invalid empty string. * * @return the string value used in JSON serialization, or {@code null} for * {@link #NONE} diff --git a/java/src/main/java/com/github/copilot/tool/CopilotTool.java b/java/src/main/java/com/github/copilot/tool/CopilotTool.java index 92347259f..9cde49b20 100644 --- a/java/src/main/java/com/github/copilot/tool/CopilotTool.java +++ b/java/src/main/java/com/github/copilot/tool/CopilotTool.java @@ -27,7 +27,7 @@ * } * * - * @since 1.0.0 + * @since 1.0.2 */ @Documented @Retention(RetentionPolicy.RUNTIME) diff --git a/java/src/main/java/com/github/copilot/tool/Param.java b/java/src/main/java/com/github/copilot/tool/Param.java index 56d513d84..75f359dd8 100644 --- a/java/src/main/java/com/github/copilot/tool/Param.java +++ b/java/src/main/java/com/github/copilot/tool/Param.java @@ -25,7 +25,7 @@ * } * * - * @since 1.0.0 + * @since 1.0.2 */ @Documented @Retention(RetentionPolicy.RUNTIME) diff --git a/java/src/test/java/com/github/copilot/tool/CopilotToolAnnotationTest.java b/java/src/test/java/com/github/copilot/tool/CopilotToolAnnotationTest.java index 56b64b19f..9052c6b1c 100644 --- a/java/src/test/java/com/github/copilot/tool/CopilotToolAnnotationTest.java +++ b/java/src/test/java/com/github/copilot/tool/CopilotToolAnnotationTest.java @@ -6,12 +6,14 @@ import static org.junit.jupiter.api.Assertions.*; +import java.io.InputStream; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import java.lang.reflect.Method; import java.lang.reflect.Parameter; +import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; import org.junit.jupiter.api.Test; @@ -41,12 +43,7 @@ void copilotToolTargetsMethod() { } @Test - void copilotToolIsAnnotatedWithCopilotExperimental() { - // @CopilotExperimental has CLASS retention so it is not visible via - // reflection at runtime. However, we can confirm: - // 1. The annotation type targets TYPE (which includes @interface declarations). - // 2. Compilation succeeded with @CopilotExperimental on @CopilotTool - // (the CopilotExperimentalProcessor would reject usage otherwise). + void copilotExperimentalTargetsTypeForAnnotationDeclarations() { Target expTarget = CopilotExperimental.class.getAnnotation(Target.class); assertNotNull(expTarget); boolean includesType = false; @@ -59,6 +56,16 @@ void copilotToolIsAnnotatedWithCopilotExperimental() { assertTrue(includesType, "@CopilotExperimental must target TYPE to be applicable to annotation declarations"); } + @Test + void copilotToolDeclaresCopilotExperimentalInClassFile() throws Exception { + String classFileResourcePath = "/" + CopilotTool.class.getName().replace('.', '/') + ".class"; + try (InputStream classFile = CopilotTool.class.getResourceAsStream(classFileResourcePath)) { + assertNotNull(classFile, "CopilotTool class file must be readable as a resource"); + String classFileText = new String(classFile.readAllBytes(), StandardCharsets.ISO_8859_1); + assertTrue(classFileText.contains("com/github/copilot/CopilotExperimental")); + } + } + @Test void copilotToolDefaultValues() throws Exception { Method nameMethod = CopilotTool.class.getDeclaredMethod("name");