spotless

Author: edburns

1682-java-tool-ergonomics-prompts-remove-before-merge/20260622-prompts.md

@@ -431,3 +431,309 @@ Ran terminal command:  export JAVA_HOME="/Library/Java/JavaVirtualMachines/micro
 
 Made changes.
 
+### 17:46 Prompt
+
+Read `copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md` (the plan) to get the context. 
+
+✅✅Pay very close attention to each **Resolution:** to the questions in `Phase 3 — Ignorance reduction: questions to answer before writing code`.
+
+In this session, we are creating issues of type **Task** as children of Epic https://github.com/github/copilot-sdk/issues/1682 . These issues correspond to the sub-sections in `## Phase 4 — Implementation (the build order)`.
+
+I want you to use `gh` to create issues as sub-issues of 1682.
+
+- I will dispatch these issues by assigning them to an agent. Leave them unassigned for now.
+
+- The description you put in the issues you create is the specification for the work the agent assigned to the issue must do. Therefore, you need to be very specific and targeted when writing the description.
+
+- Set the type to **Task**.
+
+- Each issue will be worked on and resolved in serial. You can assume:
+
+   - All issues will operate in this topic branch of `upstream`: `edburns/1682-java-tool-ergonomics`. 
+
+   - The assignee agent has access to the topic branch and, most importantly, to the prompts in the directory `1682-java-tool-ergonomics-prompts-remove-before-merge`, which includes the plan. Therefore, you can instruct the agent to access the plan by reference, you don't need to reference it by value directly in the issue.
+   
+- The sub-sections of `## Phase 4 — Implementation (the build order)` have essential details for each corresponding **Task**.
+
+- Instruct the agent to read the entire plan before working.
+
+- Instruct the agent to carefully re-read the relevant sections of the of the plan. ❌❌You can't just say, "read the relevant sections of the plan". ✅✅In the description of the issue, list exactly what sections of the plan apply to this particular issue.
+   
+- Though the sub-sections of `## Phase 4 — Implementation (the build order)` have essential details for each corresponding **Task**, I want you to carefully consider if there additional gating tests you can add so that as the agent proceeds from one task to the next, the likelihood of rework is reduced.
+
+### 18:16 Response
+
+Updated todo list
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md#1-1), lines 1 to 100
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md#101-101), lines 101 to 200
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md#201-201), lines 201 to 300
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md#301-301), lines 301 to 400
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md#401-401), lines 401 to 500
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/1682-java-tool-ergonomics-prompts-remove-before-merge/dd-3018003-ignorance-reduction-for-implementation-plan.md#501-501), lines 501 to 505
+
+Now I have the full plan. Let me create the 5 issues (4.1–4.5, skipping 4.6 which is deferred) as sub-issues of #1682. I'll write detailed descriptions with gating tests. 
+
+Ran terminal command:  gh issue create --help 2>&1 | head -40
+
+`gh issue create` supports `--parent` and `--type` directly. I'll create each issue with a body file. 
+
+Created [](file:///tmp/issue-4.1-body.md)
+
+Created [](file:///tmp/issue-4.2-body.md)
+
+Created [](file:///tmp/issue-4.3-body.md)
+
+Created 5 todos
+
+Created [](file:///tmp/issue-4.4-body.md)
+
+Created [](file:///tmp/issue-4.5-body.md)
+
+Now I'll create the issues using `gh`: 
+
+Ran terminal command:  cd /Users/edburns/workareas/copilot-sdk && gh issue create \
+  --repo github/copilot-sdk \
+  --title "[Java] @CopilotTool ergonomics 4.1: Create @CopilotTool and @Param annotations" \
+  --body-file /tmp/issue-4.1-body.md \
+  --type "Task" \
+  --parent 1682
+
+Ran terminal command:  cd /Users/edburns/workareas/copilot-sdk && gh issue create \
+  --repo github/copilot-sdk \
+  --title "[Java] @CopilotTool ergonomics 4.2: Schema generation utility (TypeMirror to JSON Schema)" \
+  --body-file /tmp/issue-4.2-body.md \
+  --type "Task" \
+  --parent 1682
+
+Ran terminal command:  cd /Users/edburns/workareas/copilot-sdk && gh issue create \
+  --repo github/copilot-sdk \
+  --title "[Java] @CopilotTool ergonomics 4.3: Annotation processor (CopilotToolProcessor)" \
+  --body-file /tmp/issue-4.3-body.md \
+  --type "Task" \
+  --parent 1682
+
+Ran terminal command:  cd /Users/edburns/workareas/copilot-sdk && gh issue create \
+  --repo github/copilot-sdk \
+  --title "[Java] @CopilotTool ergonomics 4.4: ToolDefinition.fromObject(Object) registration API" \
+  --body-file /tmp/issue-4.4-body.md \
+  --type "Task" \
+  --parent 1682
+
+Ran terminal command:  cd /Users/edburns/workareas/copilot-sdk && gh issue create \
+  --repo github/copilot-sdk \
+  --title "[Java] @CopilotTool ergonomics 4.5: E2E integration test with replay proxy" \
+  --body-file /tmp/issue-4.5-body.md \
+  --type "Task" \
+  --parent 1682
+
+All 5 issues created as sub-issues of #1682:
+
+| Phase | Issue | Title |
+|-------|-------|-------|
+| 4.1 | [#1758](https://github.com/github/copilot-sdk/issues/1758) | Create `@CopilotTool` and `@Param` annotations |
+| 4.2 | [#1759](https://github.com/github/copilot-sdk/issues/1759) | Schema generation utility (TypeMirror → JSON Schema) |
+| 4.3 | [#1760](https://github.com/github/copilot-sdk/issues/1760) | Annotation processor (`CopilotToolProcessor`) |
+| 4.4 | [#1761](https://github.com/github/copilot-sdk/issues/1761) | `ToolDefinition.fromObject(Object)` registration API |
+| 4.5 | [#1762](https://github.com/github/copilot-sdk/issues/1762) | E2E integration test with replay proxy |
+
+Each issue includes:
+- Explicit list of plan sections to re-read (by section number and title)
+- Instruction to read the full plan before starting
+- Detailed gating tests beyond what the plan specifies (e.g., reflection-based annotation attribute verification in 4.1, handler invocation tests in 4.4)
+- Clear prerequisites (serial dependency chain)
+- Branch and constraint information
+
+Updated todo list
+
+Made changes.
+
+### 18:37 Prompt
+
+Consider this text from the plan
+
+--------
+
+**Question:** What attributes does `@CopilotTool` need?
+
+Based on ADR-005 and the C#/langchain4j comparisons:
+
+```java
+@Documented
+@Retention(RetentionPolicy.SOURCE)   // only needed at compile time for processor
+@Target(ElementType.METHOD)
+@CopilotExperimental
+public @interface CopilotTool {
+    /** Tool description (sent to the model). */
+    String value();
+
+    /** Tool name. Defaults to method name converted to snake_case. */
+    String name() default "";
+
+    /** Whether this tool overrides a built-in tool. */
+    boolean overridesBuiltInTool() default false;
+
+    /** Whether to skip permission checks. */
+    boolean skipPermission() default false;
+}
+```
+
+
+------
+
+I'm concerned about relying on snake_case here. That's a Python thing. Java uses camelCase. 
+
+What would be the changes to the overall design if we said camelCase instead of snake_case?
+
+### 18:40 Prompt
+
+Updated todo list
+
+Let me check what naming convention the existing SDK and wire protocol actually use for tool names.  
+
+Searched for regex `tool.*name` (`**/test/snapshots/tools/*.yaml`), no results
+
+Searched for regex `snake_case|snakeCase|toSnakeCase|to_snake` (`**/java/**`), 9 results
+
+Searched for regex `DefineTool|define_tool|defineTool` (`**/nodejs/src/**`), 4 results
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/test/snapshots/tools/low_level_tool_definition.yaml)
+
+Searched for regex `DefineTool|tool.*name` (`**/go/*.go`), 10 results
+
+Searched for regex `set_current_phase|search_items|tool.*name` (`**/java/src/test/**/*LowLevel*"`), no results
+
+Searched for regex `set_current_phase|search_items` (`**/java/src/test/**"`), no results
+
+Searched for regex `tool.*name.*snake|tool.*name.*camel` (`**/nodejs/test/**`), no results
+
+Searched for regex `set_current_phase|search_items` (`**/java/src/test/**`), 2 results
+
+Searched for regex `defineTool.*name` (`**/nodejs/samples/**`), no results
+
+Searched for regex `Name:` (`**/go/samples/**`), 1 result
+
+Searched for regex `defineTool` (`**/nodejs/examples/**`), 2 results
+
+Searched for regex `DefineTool|tool.*name` (`**/dotnet/samples/**`), 2 results
+
+Here's the analysis:
+
+## The wire protocol uses `snake_case` — it's not just a Python thing
+
+Looking at the evidence:
+
+1. **Replay snapshot** (low_level_tool_definition.yaml): `set_current_phase`, `search_items`
+2. **Node.js examples**: `defineTool("lookup_fact", ...)`
+3. **Go samples**: `Name: "manual_resume_status"`
+4. **.NET samples**: `"manual_resume_status"`
+5. **Java existing test**: `ToolDefinition.create("set_current_phase", ...)`
+
+Every SDK across all languages uses `snake_case` for tool names on the wire. This is the **Copilot CLI / model-facing convention** — it's how the LLM identifies and calls tools in function-calling. It's not language-internal naming; it's a wire protocol identifier.
+
+## Impact of switching to `camelCase`
+
+If you changed the default conversion to camelCase (or just identity — since Java methods are already camelCase):
+
+| Aspect | Impact |
+|--------|--------|
+| **Wire compatibility** | Tool names like `setCurrentPhase` instead of `set_current_phase`. The model still works (it's just a string), but diverges from every other SDK's convention. |
+| **Cross-SDK consistency** | A tool that's `set_current_phase` in Node/Go/.NET/Python/Rust would be `setCurrentPhase` in Java — confusing for multi-SDK teams. |
+| **Snapshot reuse** | Can't share the same test snapshots across SDKs (model expects specific tool names in responses). |
+| **Code change** | Actually simplifies the processor — no case conversion logic needed (identity transform). |
+| **Override tools** | Built-in Copilot tools use `snake_case` (like `grep`). Overriding them already requires explicit `name="grep"`, so no impact there. |
+
+## Recommendation
+
+Keep `snake_case` as the **default** wire name derivation. The rationale is protocol-level consistency, not Java style:
+
+- The Java **method** stays camelCase (`setCurrentPhase`) — idiomatic Java.
+- The **tool name** sent to the model is `set_current_phase` — idiomatic function-calling protocol.
+- Users can always override with `@CopilotTool(name="whateverTheyWant")`.
+
+This is the same pattern Jackson uses: Java fields are camelCase, but `@JsonProperty` / `PropertyNamingStrategies` controls the wire format. The wire convention is independent of the source language.
+
+If you still prefer dropping the conversion, the only code changes would be:
+1. Remove the snake_case converter from the processor (4.3).
+2. Update the test in 4.3 that verifies `setCurrentPhase` → `set_current_phase`.
+3. Update the E2E snapshot/test (4.5) tool names.
+
+### 18:41 Prompt
+
+Look at what the agent changed in commit 8b899e15254b2b539ae585c85d1e7c39ef864194 to `java/src/main/java/com/github/copilot/rpc/ToolDefer.java`. It added NONE.
+
+Is there a precedent for this NONE thing in the other languages?
+
+### 18:43 Response
+
+Updated todo list
+
+Ran terminal command:  git show 8b899e15254b2b539ae585c85d1e7c39ef864194 -- java/src/main/java/com/github/copilot/rpc/ToolDefer.java
+
+Searched for regex `ToolDefer|toolDefer|tool_defer|defer` (`**/nodejs/src/**`), 20 results
+
+Searched for regex `ToolDefer|Defer` (`**/go/**/*.go`), 20 results
+
+Searched for regex `ToolDefer|Defer` (`**/dotnet/src/**`), 20 results
+
+Searched for regex `CopilotToolDefer` (`**/dotnet/src/**`), 8 results
+
+Searched for regex `defer` (`**/nodejs/src/types.ts`), 4 results
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/dotnet/src/CopilotTool.cs#155-155), lines 155 to 170
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/java/src/main/java/com/github/copilot/rpc/ToolDefer.java#1-1), lines 1 to 10
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/java/src/main/java/com/github/copilot/rpc/ToolDefer.java#20-20), lines 20 to 50
+
+**No, there is no precedent for `NONE` in the other languages.**
+
+- **.NET**: `CopilotToolDefer` has only `Auto` and `Never`. The field is `CopilotToolDefer?` (nullable) — absence is expressed as `null`.
+- **Node.js**: `defer?: "auto" | "never"` — absence is expressed as `undefined` (the field is optional).
+- **Go**: Would use a pointer or zero value.
+
+Every other SDK uses **nullability/optionality** to represent "no defer preference." The agent invented `NONE("")` to solve a Java annotation limitation: annotation attributes can't be `null`, so a sentinel value is needed for the default.
+
+This is a reasonable Java-specific workaround (annotations require compile-time constants), but it introduces a concern: `NONE("")` will serialize as `""` via `@JsonValue` if it's ever accidentally included in a JSON-RPC message. The processor/`fromObject()` code must treat `NONE` as equivalent to `null` (i.e., don't pass it to `ToolDefinition.create()`). You'll want to verify that gate exists in task 4.3/4.4.
+
+### 18:45 Prompt
+
+Let's take .NET. What is the behavior of the tool in the "absence" case? And what does that serialize as?
+
+### 18:47 Response
+
+
+Updated todo list
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/dotnet/src/CopilotTool.cs#140-140), lines 140 to 160
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/dotnet/src/Client.cs#2415-2415), lines 2415 to 2450
+
+Now let me see how `ToolDefinition` serializes when `Defer` is null: 
+
+Searched for regex `ToolDefinition.*serial|WriteJson|JsonSerializ|toolDefinition.*json|RegisterTools|registerTools` (`**/dotnet/src/Client.cs`), 20 results
+
+Read [](file:///Users/edburns/workareas/copilot-sdk/dotnet/src/Client.cs#2117-2117), lines 2117 to 2130
+
+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.
+
+### 19:11 Prompt
+
+For all the created issues please find the right place to add the following requirement:
+
+✅✅ YOU MUST run `mvn spotless:apply` before every commit.

java/src/main/java/com/github/copilot/rpc/ToolDefer.java

@@ -22,8 +22,8 @@
 public enum ToolDefer {
 
     /**
-     * No deferral preference set. Used as the default for annotation-based
-     * tool definitions where the user has not explicitly chosen a mode.
+     * No deferral preference set. Used as the default for annotation-based tool
+     * definitions where the user has not explicitly chosen a mode.
      */
     NONE(""),
 

java/src/main/java/com/github/copilot/tool/CopilotTool.java

@@ -22,9 +22,8 @@
  *
  * <pre>
  * &#64;CopilotTool("Get weather for a location")
- * public CompletableFuture&lt;String&gt; getWeather(
- *         &#64;Param(value = "City name", required = true) String location) {
- *     return CompletableFuture.completedFuture("Sunny in " + location);
+ * public CompletableFuture&lt;String&gt; getWeather(&#64;Param(value = "City name", required = true) String location) {
+ * 	return CompletableFuture.completedFuture("Sunny in " + location);
  * }
  * </pre>
  *

java/src/main/java/com/github/copilot/tool/Param.java

@@ -19,10 +19,9 @@
  *
  * <pre>
  * &#64;CopilotTool("Search for issues")
- * public CompletableFuture&lt;String&gt; searchIssues(
- *         &#64;Param(value = "Search query", required = true) String query,
- *         &#64;Param(value = "Max results", required = false, defaultValue = "10") int limit) {
- *     // ...
+ * public CompletableFuture&lt;String&gt; searchIssues(&#64;Param(value = "Search query", required = true) String query,
+ * 		&#64;Param(value = "Max results", required = false, defaultValue = "10") int limit) {
+ * 	// ...
  * }
  * </pre>
  *

java/src/test/java/com/github/copilot/tool/CopilotToolAnnotationTest.java

@@ -37,7 +37,7 @@ void copilotToolHasRuntimeRetention() {
     void copilotToolTargetsMethod() {
         Target target = CopilotTool.class.getAnnotation(Target.class);
         assertNotNull(target);
-        assertArrayEquals(new ElementType[] {ElementType.METHOD}, target.value());
+        assertArrayEquals(new ElementType[]{ElementType.METHOD}, target.value());
     }
 
     @Test
@@ -46,7 +46,7 @@ void copilotToolIsAnnotatedWithCopilotExperimental() {
         // 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).
+        // (the CopilotExperimentalProcessor would reject usage otherwise).
         Target expTarget = CopilotExperimental.class.getAnnotation(Target.class);
         assertNotNull(expTarget);
         boolean includesType = false;
@@ -56,8 +56,7 @@ void copilotToolIsAnnotatedWithCopilotExperimental() {
                 break;
             }
         }
-        assertTrue(includesType,
-                "@CopilotExperimental must target TYPE to be applicable to annotation declarations");
+        assertTrue(includesType, "@CopilotExperimental must target TYPE to be applicable to annotation declarations");
     }
 
     @Test
@@ -88,7 +87,7 @@ void paramHasRuntimeRetention() {
     void paramTargetsParameter() {
         Target target = Param.class.getAnnotation(Target.class);
         assertNotNull(target);
-        assertArrayEquals(new ElementType[] {ElementType.PARAMETER}, target.value());
+        assertArrayEquals(new ElementType[]{ElementType.PARAMETER}, target.value());
     }
 
     @Test
@@ -112,8 +111,7 @@ void paramDefaultValues() throws Exception {
     static class SampleToolHolder {
 
         @CopilotTool(value = "Get weather for a location", name = "get_weather", defer = ToolDefer.AUTO)
-        public CompletableFuture<String> getWeather(
-                @Param(value = "City name", required = true) String location,
+        public CompletableFuture<String> getWeather(@Param(value = "City name", required = true) String location,
                 @Param(value = "Temperature unit", required = false, defaultValue = "celsius") String unit) {
             return CompletableFuture.completedFuture("Sunny in " + location);
         }