graphql-java · Copilot · Jan 24, 2026 · Jan 24, 2026 · Jan 24, 2026 · Jan 25, 2026
diff --git a/.claude/commands/jspecify-annotate.md b/.claude/commands/jspecify-annotate.md
@@ -2,7 +2,21 @@ The task is to annotate public API classes (marked with `@PublicAPI`) with JSpec
 
 Note that JSpecify is already used in this repository so it's already imported.
 
-If you see a builder static class, you can label it `@NullUnmarked` and not need to do anymore for this static class in terms of annotations.
+**IMPORTANT: Builder classes MUST be annotated with `@NullUnmarked`.** When you encounter a `public static final class Builder` or `public static class Builder` inside a `@NullMarked` class, you MUST:
+1. Add `@NullUnmarked` directly above the Builder class declaration
+2. Remove ALL `@Nullable` annotations from the Builder's fields (e.g., `private @Nullable SourceLocation sourceLocation;` → `private SourceLocation sourceLocation;`)
+3. Remove ALL `@Nullable` annotations from the Builder's setter parameters (e.g., `public Builder sourceLocation(@Nullable SourceLocation sourceLocation)` → `public Builder sourceLocation(SourceLocation sourceLocation)`)
+4. Add `import org.jspecify.annotations.NullUnmarked;` if not already present
+
+No further nullability annotations are needed inside the Builder after applying `@NullUnmarked`.
+
+## Batch Size and Prioritization
+
+Annotate approximately 10 classes per batch for optimal context management. Start with interface/simple classes first, then tackle complex classes with builders. This helps identify patterns early.
+
+## Exploration Phase
+
+Before annotating, use `grep` to search for how each class is instantiated (e.g., `grep -r "new Comment"`) to understand which parameters can be null. Check constructor calls, method returns, and field assignments to inform your nullability decisions.
 
 Analyze this Java class and add JSpecify annotations based on:
 1. Set the class to be `@NullMarked`
@@ -12,14 +26,73 @@ Analyze this Java class and add JSpecify annotations based on:
 5. Method implementations that return null or check for null
 6. GraphQL specification details (see details below)
 
+## Pattern Examples
+
+Here are concrete examples of common annotation patterns:
+
+**Interface:**
+```java
+@PublicApi
+@NullMarked
+public interface MyInterface {
+    // Methods inherit @NullMarked context
+}
+```
+
+**Class with nullable field:**
+```java
+@PublicApi
+@NullMarked
+public class Comment {
+    private final String content;
+    private final @Nullable SourceLocation sourceLocation;
+
+    public Comment(String content, @Nullable SourceLocation sourceLocation) {
+        this.content = content;
+        this.sourceLocation = sourceLocation;
+    }
+
+    public @Nullable SourceLocation getSourceLocation() {
+        return sourceLocation;
+    }
+}
+```
+
+**Class with nullable return type:**
+```java
+@PublicApi
+@NullMarked
+public class Container {
+    public @Nullable Node getChildOrNull(String key) {
+        // May return null
+        return children.get(key);
+    }
+}
+```
+
+**Builder with @NullUnmarked:**
+```java
+@PublicApi
+@NullMarked
+public class MyClass {
+    @NullUnmarked
+    public static class Builder {
+        // No further annotations needed in builder
+    }
+}
+```
+
 ## GraphQL Specification Compliance
 This is a GraphQL implementation. When determining nullability, consult the GraphQL specification (https://spec.graphql.org/draft/) for the relevant concept. Key principles:
 
 The spec defines which elements are required (non-null) vs optional (nullable). Look for keywords like "MUST" to indicate when an element is required, and conditional words such as "IF" to indicate when an element is optional.
 
 If a class implements or represents a GraphQL specification concept, prioritize the spec's nullability requirements over what IntelliJ inferred.
 
-## How to validate
+## Validation Strategy
+
+Run `./gradlew compileJava` after every 3-5 classes annotated, not just at the end. This catches issues early and makes debugging easier.
+
 Finally, please check all this works by running the NullAway compile check.
 
 If you find NullAway errors, try and make the smallest possible change to fix them. If you must, you can use assertNotNull. Make sure to include a message as well.

diff --git a/.claude/fix-jspecify-test-failures-plan.md b/.claude/fix-jspecify-test-failures-plan.md
@@ -0,0 +1,126 @@
+# Fix 23 Test Failures from JSpecify Annotation Changes
+
+The JSpecify PR added `assertNotNull()` calls in several Builder `build()` methods for non-nullable fields. Many Groovy tests construct incomplete objects (e.g., `Field.newField().build()` without setting `name`), which now crash at runtime.
+
+## Root Cause
+
+Builder `build()` methods in the following classes now call `assertNotNull()` on fields that tests leave null:
+
+| Source class | `assertNotNull` fields in `build()` | Test impact |
+|---|---|---|
+| `Field.Builder` | `name` | 12 occurrences of `Field.newField().build()` across 3 test files |
+| `FieldDefinition.Builder` | `name`, `type` | 2 occurrences in `SchemaPrinterTest` |
+| `EnumTypeDefinition.Builder` | `name` | 1 occurrence in `SchemaPrinterTest` |
+| `EnumValueDefinition.Builder` | `name` | 1 occurrence in `SchemaPrinterTest` |
+| `FragmentDefinition.Builder` | `name`, `typeCondition`, `selectionSet` | 2 in `TraversalContextTest`, 1 in `FragmentsOnCompositeTypeTest` |
+
+## Proposed Changes
+
+### Test Fixes — Supply Required Fields
+
+The fix is to update test code to supply the required fields that builders now enforce. These are minimal changes — adding `.name("dummy")` or similar placeholder values.
+
+---
+
+### ExecutionStrategyTest
+
+#### [MODIFY] [ExecutionStrategyTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/execution/ExecutionStrategyTest.groovy)
+
+Replace all `Field.newField().build()` with `Field.newField().name("dummy").build()` at lines: 143, 145, 773, 774, 797, 798, 821, 822, 945, 946, 969, 970.
+
+---
+
+### QueryTraverserTest
+
+#### [MODIFY] [QueryTraverserTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/analysis/QueryTraverserTest.groovy)
+
+Replace `Field.newField().build()` with `Field.newField().name("dummy").build()` at lines: 1528-1536, 1547.
+
+---
+
+### NodeParentTreeTest
+
+#### [MODIFY] [NodeParentTreeTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/language/NodeParentTreeTest.groovy)
+
+Replace `FieldDefinition.newFieldDefinition().name("field").build()` — this should already work since `name` is set, but `type` is not. Need to add `.type(new TypeName("String"))`.
+
+At line 10: `FieldDefinition.newFieldDefinition().name("field").build()` → add `.type(new TypeName("String"))`.
+
+---
+
+### NodeTraverserTest
+
+#### [MODIFY] [NodeTraverserTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/language/NodeTraverserTest.groovy)
+
+Replace `Field.newField().build()` with `Field.newField().name("dummy").build()` at line 143.
+
+---
+
+### DataFetchingEnvironmentImplTest
+
+#### [MODIFY] [DataFetchingEnvironmentImplTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/schema/DataFetchingEnvironmentImplTest.groovy)
+
+- Line 29: `FragmentDefinition.newFragmentDefinition().name("frag").typeCondition(new TypeName("t")).build()` — needs `.selectionSet(SelectionSet.newSelectionSet().build())` added since `selectionSet` is now `assertNotNull`.
+- Line 32: `new OperationDefinition("q")` — this constructor was NOT changed (no diff), so it should still work. But the class may depend on other annotated classes. Need to check if `OperationDefinition` has a single-arg convenience constructor.
+- Line 150: `new Argument("arg1", new StringValue("argVal"))` and `new Field("someField", [argument])` — `Field` convenience constructors still work since they pass `name` directly to the main constructor, which no longer null-checks `name` (the `assertNotNull` is only in the Builder).
+
+> [!IMPORTANT]
+> The `Field` convenience constructors (e.g., `new Field("someField")`) should still work because they call the main constructor directly without `assertNotNull`. The failures in `DataFetchingEnvironmentImplTest` likely stem from `FragmentDefinition.Builder.build()` crashing on the `frag` field (line 29), which fails before the test even starts since it's a class-level field.
+
+---
+
+### SchemaPrinterTest
+
+#### [MODIFY] [SchemaPrinterTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy)
+
+Lines 2795, 2799: `FieldDefinition.newFieldDefinition().comments(...).build()` — add `.name("placeholder").type(new TypeName("String"))`.
+
+Line 2757-2758: `newEnumTypeDefinition().comments(...).build()` — add `.name("placeholder")`.
+
+Line 2761: `EnumValueDefinition.newEnumValueDefinition().comments(...).build()` — add `.name("placeholder")`.
+
+Line 2744: `DirectiveDefinition.newDirectiveDefinition().comments(...).build()` — `DirectiveDefinition.Builder` does NOT have `assertNotNull(name)`, so this should be fine. But verify.
+
+---
+
+### TraversalContextTest
+
+#### [MODIFY] [TraversalContextTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/validation/TraversalContextTest.groovy)
+
+Lines 131, 165-168: `FragmentDefinition.newFragmentDefinition().name(...).typeCondition(...).build()` — add `.selectionSet(SelectionSet.newSelectionSet().build())` since `selectionSet` is now `assertNotNull`.
+
+---
+
+### FragmentsOnCompositeTypeTest
+
+#### [MODIFY] [FragmentsOnCompositeTypeTest.groovy](file:///Users/dondonz/Development/graphql-java/src/test/groovy/graphql/validation/rules/FragmentsOnCompositeTypeTest.groovy)
+
+Line 71: `FragmentDefinition.newFragmentDefinition().name("fragment").typeCondition(...).build()` — add `.selectionSet(SelectionSet.newSelectionSet().build())`.
+
+---
+
+## Verification Plan
+
+### Automated Tests
+
+Run each failing test class individually:
+
+```bash
+cd /Users/dondonz/Development/graphql-java
+
+# Need to work around jar task failure from branch name with '/'
+# Option 1: Run just the test compilation and execution
+./gradlew test --tests "graphql.execution.ExecutionStrategyTest" \
+  --tests "graphql.analysis.QueryTraverserTest" \
+  --tests "graphql.language.NodeParentTreeTest" \
+  --tests "graphql.language.NodeTraverserTest" \
+  --tests "graphql.schema.DataFetchingEnvironmentImplTest" \
+  --tests "graphql.schema.idl.SchemaPrinterTest" \
+  --tests "graphql.validation.TraversalContextTest" \
+  --tests "graphql.validation.rules.FragmentsOnCompositeTypeTest"
+```
+
+> [!WARNING]
+> The `:jar` task is currently failing due to the branch name `copilot/add-jspecify-annotations-again` containing `/`, which creates an invalid path for the jar artifact. This may need to be fixed before tests can be run. Alternative: compile test code only with `./gradlew compileTestGroovy`.
+
+All 23 originally failing tests should pass after the fixes.
diff --git a/src/main/java/graphql/language/Comment.java b/src/main/java/graphql/language/Comment.java
@@ -1,18 +1,21 @@
 package graphql.language;
 
 import graphql.PublicApi;
+import org.jspecify.annotations.NullMarked;
+import org.jspecify.annotations.Nullable;
 
 import java.io.Serializable;
 
 /**
  * A single-line comment. These are comments that start with a {@code #} in source documents.
  */
 @PublicApi
+@NullMarked
 public class Comment implements Serializable {
     public final String content;
-    public final SourceLocation sourceLocation;
+    public final @Nullable SourceLocation sourceLocation;
 
-    public Comment(String content, SourceLocation sourceLocation) {
+    public Comment(String content, @Nullable SourceLocation sourceLocation) {
         this.content = content;
         this.sourceLocation = sourceLocation;
     }
@@ -21,7 +24,7 @@ public String getContent() {
         return content;
     }
 
-    public SourceLocation getSourceLocation() {
+    public @Nullable SourceLocation getSourceLocation() {
         return sourceLocation;
     }
 }
diff --git a/src/main/java/graphql/language/Definition.java b/src/main/java/graphql/language/Definition.java
@@ -2,8 +2,10 @@
 
 
 import graphql.PublicApi;
+import org.jspecify.annotations.NullMarked;
 
 @PublicApi
+@NullMarked
 public interface Definition<T extends Definition> extends Node<T> {
 
 }
diff --git a/src/main/java/graphql/language/DirectiveLocation.java b/src/main/java/graphql/language/DirectiveLocation.java
@@ -1,11 +1,13 @@
 package graphql.language;
 
-
 import com.google.common.collect.ImmutableList;
 import graphql.Internal;
 import graphql.PublicApi;
 import graphql.util.TraversalControl;
 import graphql.util.TraverserContext;
+import org.jspecify.annotations.NullMarked;
+import org.jspecify.annotations.NullUnmarked;
+import org.jspecify.annotations.Nullable;
 
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -20,11 +22,13 @@
 import static graphql.language.NodeUtil.assertNewChildrenAreEmpty;
 
 @PublicApi
+@NullMarked
 public class DirectiveLocation extends AbstractNode<DirectiveLocation> implements NamedNode<DirectiveLocation> {
     private final String name;
 
     @Internal
-    protected DirectiveLocation(String name, SourceLocation sourceLocation, List<Comment> comments, IgnoredChars ignoredChars, Map<String, String> additionalData) {
+    protected DirectiveLocation(String name, @Nullable SourceLocation sourceLocation, List<Comment> comments,
+            IgnoredChars ignoredChars, Map<String, String> additionalData) {
         super(sourceLocation, comments, ignoredChars, additionalData);
         this.name = name;
     }
@@ -60,7 +64,7 @@ public DirectiveLocation withNewChildren(NodeChildrenContainer newChildren) {
     }
 
     @Override
-    public boolean isEqualTo(Node o) {
+    public boolean isEqualTo(@Nullable Node o) {
         if (this == o) {
             return true;
         }
@@ -100,6 +104,7 @@ public DirectiveLocation transform(Consumer<Builder> builderConsumer) {
         return builder.build();
     }
 
+    @NullUnmarked
     public static final class Builder implements NodeBuilder {
         private SourceLocation sourceLocation;
         private ImmutableList<Comment> comments = emptyList();
@@ -148,7 +153,7 @@ public Builder additionalData(String key, String value) {
         }
 
         public DirectiveLocation build() {
-            return new DirectiveLocation(name, sourceLocation, comments, ignoredChars, additionalData);
+            return new DirectiveLocation(assertNotNull(name), sourceLocation, comments, ignoredChars, additionalData);
         }
     }
 }
diff --git a/src/main/java/graphql/language/DirectivesContainer.java b/src/main/java/graphql/language/DirectivesContainer.java
@@ -2,6 +2,7 @@
 
 
 import graphql.PublicApi;
+import org.jspecify.annotations.NullMarked;
 
 import java.util.List;
 import java.util.Map;
@@ -15,6 +16,7 @@
  * @see DirectiveDefinition#isRepeatable()
  */
 @PublicApi
+@NullMarked
 public interface DirectivesContainer<T extends DirectivesContainer> extends Node<T> {
 
     /**