Skip to content

Commit da10bba

Browse files
david-waltermiredavid-waltermire
committed
docs: add workflow for identifying and commenting in unresolved PR conversations
1 parent 67dd30e commit da10bba

File tree

12 files changed

+271
-145
lines changed

12 files changed

+271
-145
lines changed

‎.claude/skills/pr-feedback.md‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,80 @@ Address Feedback → Push → Wait for CI/Bot Review
267267
| Resolve thread | GraphQL `resolveReviewThread` mutation |
268268
| Verify resolved | GraphQL query checking `isResolved` |
269269

270+
## Identifying and Commenting in Unresolved Conversations
271+
272+
After addressing feedback, identify unresolved threads that need inline replies:
273+
274+
### Step 1: Find Unresolved Threads with Comment IDs
275+
276+
```bash
277+
gh api graphql -f query='
278+
query {
279+
repository(owner: "<OWNER>", name: "<REPO>") {
280+
pullRequest(number: <PR_NUMBER>) {
281+
reviewThreads(first: 50) {
282+
nodes {
283+
id
284+
isResolved
285+
path
286+
comments(first: 1) {
287+
nodes {
288+
fullDatabaseId
289+
author {
290+
login
291+
}
292+
body
293+
}
294+
}
295+
}
296+
}
297+
}
298+
}
299+
}'
300+
```
301+
302+
### Step 2: Filter for Unresolved Threads
303+
304+
Look for threads where `isResolved: false`. Each has:
305+
- `fullDatabaseId` - The numeric ID needed for inline replies (use this instead of deprecated `databaseId`)
306+
- `path` - The file path for context
307+
- `body` - Preview of what the comment is about
308+
- `author.login` - Who made the comment (e.g., `coderabbitai`)
309+
310+
### Step 3: Post Inline Replies
311+
312+
For each unresolved thread, use the `fullDatabaseId` to reply inline:
313+
314+
```bash
315+
gh api repos/<OWNER>/<REPO>/pulls/<PR_NUMBER>/comments \
316+
-X POST \
317+
-F in_reply_to=<FULL_DATABASE_ID> \
318+
-f body="@<author_login> Addressed in commit <hash>. <explanation>"
319+
```
320+
321+
**Example workflow:**
322+
```bash
323+
# 1. Get unresolved threads with reliable jq extraction
324+
gh api graphql -f query='...' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false) | {id, fullDatabaseId: .comments.nodes[0].fullDatabaseId, path, author: .comments.nodes[0].author.login}'
325+
326+
# 2. For each unresolved thread, note the fullDatabaseId from the output
327+
328+
# 3. Post inline reply
329+
gh api repos/owner/repo/pulls/123/comments \
330+
-X POST \
331+
-F in_reply_to=2653785779 \
332+
-f body="@coderabbitai Addressed in commit abc123. Fixed the documentation."
333+
```
334+
335+
### When to Use This Workflow
336+
337+
- After pushing fixes for review feedback
338+
- When the user asks "there are open conversations that haven't been commented in"
339+
- Before requesting a re-review to ensure all feedback has responses
340+
- When CodeRabbit or other bots don't auto-resolve threads
341+
342+
---
343+
270344
## Common Pitfalls
271345

272346
1. **Using wrong reply API** - The `/pulls/comments/{id}/replies` endpoint returns 404. Use `POST /pulls/{id}/comments` with `-F in_reply_to=<comment_id>` instead.
@@ -278,3 +352,5 @@ Address Feedback → Push → Wait for CI/Bot Review
278352
4. **Missing feedback** - Check both `comments` (general) and `reviews` (code review) in the PR data.
279353

280354
5. **Incorrect automated feedback** - Bots like CodeRabbit can make mistakes. Verify technical claims against official docs or schema introspection before implementing.
355+
356+
6. **Not getting databaseId for inline replies** - The GraphQL thread `id` (node ID) is for resolving threads. The `databaseId` from the first comment is needed for inline replies via REST API.

‎cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CallingContext.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,9 @@ private static CharSequence getExtraArguments(@NonNull ICommand targetCommand) {
543543

544544
/**
545545
* Output the help text to the console.
546+
*
547+
* @throws UncheckedIOException
548+
* if an error occurs while writing help output
546549
*/
547550
public void showHelp() {
548551
PrintStream out = cliProcessor.getOutputStream();

‎core/src/test/java/gov/nist/secauto/metaschema/core/model/constraint/DefaultConstraintValidatorThreadSafetyTest.java‎

Lines changed: 81 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -90,47 +90,50 @@ void testConcurrentIndexHasKeyAccumulation() throws Exception {
9090
int threadCount = 10;
9191
int keysPerThread = 50;
9292
ExecutorService executor = Executors.newFixedThreadPool(threadCount);
93-
CountDownLatch startLatch = new CountDownLatch(1);
94-
CountDownLatch doneLatch = new CountDownLatch(threadCount);
95-
ConcurrentLinkedQueue<String> errorMessages = new ConcurrentLinkedQueue<>();
96-
97-
// All threads will add key references to the same index name
98-
String indexName = "test-index";
99-
100-
for (int t = 0; t < threadCount; t++) {
101-
final int threadId = t;
102-
executor.submit(() -> {
103-
try {
104-
startLatch.await(); // Wait for all threads to be ready
105-
for (int i = 0; i < keysPerThread; i++) {
106-
IIndexHasKeyConstraint constraint = createMockIndexHasKeyConstraint(indexName);
107-
IDefinitionNodeItem<?, ?> node = createMockDefinitionNodeItem("/thread" + threadId + "/item" + i);
108-
ISequence<? extends INodeItem> targets = createMockSequence();
109-
110-
// This method adds to indexNameToKeyRefMap
111-
invokeValidateIndexHasKey(validator, constraint, node, targets);
112-
}
113-
} catch (Exception e) {
114-
StringBuilder sb = new StringBuilder();
115-
sb.append("Thread ").append(threadId).append(" error: ")
116-
.append(e.getClass().getName()).append(": ").append(e.getMessage());
117-
if (e.getCause() != null) {
118-
sb.append(" Caused by: ").append(e.getCause().getClass().getName())
119-
.append(": ").append(e.getCause().getMessage());
93+
try {
94+
CountDownLatch startLatch = new CountDownLatch(1);
95+
CountDownLatch doneLatch = new CountDownLatch(threadCount);
96+
ConcurrentLinkedQueue<String> errorMessages = new ConcurrentLinkedQueue<>();
97+
98+
// All threads will add key references to the same index name
99+
String indexName = "test-index";
100+
101+
for (int t = 0; t < threadCount; t++) {
102+
final int threadId = t;
103+
executor.submit(() -> {
104+
try {
105+
startLatch.await(); // Wait for all threads to be ready
106+
for (int i = 0; i < keysPerThread; i++) {
107+
IIndexHasKeyConstraint constraint = createMockIndexHasKeyConstraint(indexName);
108+
IDefinitionNodeItem<?, ?> node = createMockDefinitionNodeItem("/thread" + threadId + "/item" + i);
109+
ISequence<? extends INodeItem> targets = createMockSequence();
110+
111+
// This method adds to indexNameToKeyRefMap
112+
invokeValidateIndexHasKey(validator, constraint, node, targets);
113+
}
114+
} catch (Exception e) {
115+
StringBuilder sb = new StringBuilder();
116+
sb.append("Thread ").append(threadId).append(" error: ")
117+
.append(e.getClass().getName()).append(": ").append(e.getMessage());
118+
if (e.getCause() != null) {
119+
sb.append(" Caused by: ").append(e.getCause().getClass().getName())
120+
.append(": ").append(e.getCause().getMessage());
121+
}
122+
errorMessages.add(sb.toString());
123+
} finally {
124+
doneLatch.countDown();
120125
}
121-
errorMessages.add(sb.toString());
122-
} finally {
123-
doneLatch.countDown();
124-
}
125-
});
126-
}
126+
});
127+
}
127128

128-
startLatch.countDown(); // Start all threads simultaneously
129-
assertTrue(doneLatch.await(30, TimeUnit.SECONDS), "Threads should complete within timeout");
130-
executor.shutdown();
129+
startLatch.countDown(); // Start all threads simultaneously
130+
assertTrue(doneLatch.await(30, TimeUnit.SECONDS), "Threads should complete within timeout");
131131

132-
assertEquals(0, errorMessages.size(),
133-
"No errors should occur during concurrent access. Errors: " + errorMessages);
132+
assertEquals(0, errorMessages.size(),
133+
"No errors should occur during concurrent access. Errors: " + errorMessages);
134+
} finally {
135+
executor.shutdown();
136+
}
134137
}
135138
}
136139

@@ -146,47 +149,50 @@ void testConcurrentAllowedValuesTracking() throws Exception {
146149
int threadCount = 8;
147150
int itemsPerThread = 25;
148151
ExecutorService executor = Executors.newFixedThreadPool(threadCount);
149-
CountDownLatch startLatch = new CountDownLatch(1);
150-
CountDownLatch doneLatch = new CountDownLatch(threadCount);
151-
ConcurrentLinkedQueue<String> errorMessages = new ConcurrentLinkedQueue<>();
152-
153-
for (int t = 0; t < threadCount; t++) {
154-
final int threadId = t;
155-
executor.submit(() -> {
156-
try {
157-
startLatch.await();
158-
for (int i = 0; i < itemsPerThread; i++) {
159-
INodeItem targetItem = createMockNodeItemWithValue("/thread" + threadId + "/value" + i);
160-
IAllowedValuesConstraint constraint = createMockAllowedValuesConstraint();
161-
IDefinitionNodeItem<?, ?> node = createMockDefinitionNodeItem("/thread" + threadId + "/node" + i);
162-
163-
try {
164-
validator.updateValueStatus(targetItem, constraint, node);
165-
} catch (@SuppressWarnings("unused") ConstraintValidationException ex) {
166-
// Expected for some constraint combinations
152+
try {
153+
CountDownLatch startLatch = new CountDownLatch(1);
154+
CountDownLatch doneLatch = new CountDownLatch(threadCount);
155+
ConcurrentLinkedQueue<String> errorMessages = new ConcurrentLinkedQueue<>();
156+
157+
for (int t = 0; t < threadCount; t++) {
158+
final int threadId = t;
159+
executor.submit(() -> {
160+
try {
161+
startLatch.await();
162+
for (int i = 0; i < itemsPerThread; i++) {
163+
INodeItem targetItem = createMockNodeItemWithValue("/thread" + threadId + "/value" + i);
164+
IAllowedValuesConstraint constraint = createMockAllowedValuesConstraint();
165+
IDefinitionNodeItem<?, ?> node = createMockDefinitionNodeItem("/thread" + threadId + "/node" + i);
166+
167+
try {
168+
validator.updateValueStatus(targetItem, constraint, node);
169+
} catch (@SuppressWarnings("unused") ConstraintValidationException ex) {
170+
// Expected for some constraint combinations
171+
}
167172
}
173+
} catch (Exception e) {
174+
StringBuilder sb = new StringBuilder();
175+
sb.append("Thread ").append(threadId).append(" error: ")
176+
.append(e.getClass().getName()).append(": ").append(e.getMessage());
177+
if (e.getCause() != null) {
178+
sb.append(" Caused by: ").append(e.getCause().getClass().getName())
179+
.append(": ").append(e.getCause().getMessage());
180+
}
181+
errorMessages.add(sb.toString());
182+
} finally {
183+
doneLatch.countDown();
168184
}
169-
} catch (Exception e) {
170-
StringBuilder sb = new StringBuilder();
171-
sb.append("Thread ").append(threadId).append(" error: ")
172-
.append(e.getClass().getName()).append(": ").append(e.getMessage());
173-
if (e.getCause() != null) {
174-
sb.append(" Caused by: ").append(e.getCause().getClass().getName())
175-
.append(": ").append(e.getCause().getMessage());
176-
}
177-
errorMessages.add(sb.toString());
178-
} finally {
179-
doneLatch.countDown();
180-
}
181-
});
182-
}
185+
});
186+
}
183187

184-
startLatch.countDown();
185-
assertTrue(doneLatch.await(30, TimeUnit.SECONDS), "Threads should complete within timeout");
186-
executor.shutdown();
188+
startLatch.countDown();
189+
assertTrue(doneLatch.await(30, TimeUnit.SECONDS), "Threads should complete within timeout");
187190

188-
assertEquals(0, errorMessages.size(),
189-
"No errors should occur during concurrent access. Errors: " + errorMessages);
191+
assertEquals(0, errorMessages.size(),
192+
"No errors should occur during concurrent access. Errors: " + errorMessages);
193+
} finally {
194+
executor.shutdown();
195+
}
190196
}
191197
}
192198

‎core/src/test/java/gov/nist/secauto/metaschema/core/model/validation/SchemaContentValidatorTest.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -297,13 +297,14 @@ void testValidationResultSeverityLevels() {
297297
*/
298298
@NonNull
299299
private static XmlSchemaContentValidator createXmlValidator() throws IOException {
300-
try (InputStream schemaIs = SchemaContentValidatorTest.class
301-
.getResourceAsStream("/schema-validation/simple-test.xsd")) {
302-
assertNotNull(schemaIs, "Schema resource should be found on classpath");
303-
304-
StreamSource schemaSource = new StreamSource(schemaIs);
305-
return new XmlSchemaContentValidator(ObjectUtils.notNull(List.of(schemaSource)));
306-
}
300+
// Don't use try-with-resources - the XmlSchemaContentValidator constructor
301+
// takes @Owning ownership of the stream and closes it in toSchema()
302+
InputStream schemaIs = SchemaContentValidatorTest.class
303+
.getResourceAsStream("/schema-validation/simple-test.xsd");
304+
assertNotNull(schemaIs, "Schema resource should be found on classpath");
305+
306+
StreamSource schemaSource = new StreamSource(schemaIs);
307+
return new XmlSchemaContentValidator(ObjectUtils.notNull(List.of(schemaSource)));
307308
}
308309

309310
/**

‎databind/src/main/java/gov/nist/secauto/metaschema/databind/codegen/typeinfo/AbstractPropertyTypeInfo.java‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -304,14 +304,8 @@ private static boolean isMethodDeclaredInClass(
304304
return true;
305305
}
306306
}
307-
} catch (ClassNotFoundException ex) {
308-
// Class not on classpath; @Override annotation will be omitted
309-
if (LOGGER.isWarnEnabled()) {
310-
LOGGER.warn("Class '{}' not found on classpath during code generation. "
311-
+ "The @Override annotation for method '{}' will be omitted. "
312-
+ "Ensure the class is available as a dependency of the code generation plugin.",
313-
className.reflectionName(), methodName);
314-
}
307+
} catch (@SuppressWarnings("unused") ClassNotFoundException ex) {
308+
logClassNotFound(className.reflectionName(), methodName);
315309
}
316310
return false;
317311
}
@@ -345,18 +339,30 @@ && isTypeMatch(ObjectUtils.notNull(method.getParameterTypes()[0]), parameterType
345339
return true;
346340
}
347341
}
348-
} catch (ClassNotFoundException ex) {
349-
// Class not on classpath; @Override annotation will be omitted
350-
if (LOGGER.isWarnEnabled()) {
351-
LOGGER.warn("Class '{}' not found on classpath during code generation. "
352-
+ "The @Override annotation for method '{}' will be omitted. "
353-
+ "Ensure the class is available as a dependency of the code generation plugin.",
354-
className.reflectionName(), methodName);
355-
}
342+
} catch (@SuppressWarnings("unused") ClassNotFoundException ex) {
343+
logClassNotFound(className.reflectionName(), methodName);
356344
}
357345
return false;
358346
}
359347

348+
/**
349+
* Logs a warning when a class cannot be found on the classpath during code
350+
* generation.
351+
*
352+
* @param className
353+
* the fully qualified name of the class that was not found
354+
* @param methodName
355+
* the method name being checked for override
356+
*/
357+
private static void logClassNotFound(@NonNull String className, @NonNull String methodName) {
358+
if (LOGGER.isWarnEnabled()) {
359+
LOGGER.warn("Class '{}' not found on classpath during code generation. "
360+
+ "The @Override annotation for method '{}' will be omitted. "
361+
+ "Ensure the class is available as a dependency of the code generation plugin.",
362+
className, methodName);
363+
}
364+
}
365+
360366
/**
361367
* Checks if a Class matches a JavaPoet TypeName.
362368
* <p>

‎databind/src/main/java/gov/nist/secauto/metaschema/databind/codegen/typeinfo/DefaultMetaschemaClassFactory.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -780,7 +780,7 @@ private static boolean extendsIBoundObject(@NonNull List<ClassName> superinterfa
780780
if (IBoundObject.class.isAssignableFrom(clazz)) {
781781
return true;
782782
}
783-
} catch (ClassNotFoundException ex) {
783+
} catch (@SuppressWarnings("unused") ClassNotFoundException ex) {
784784
// Class not on classpath; assume it doesn't extend IBoundObject
785785
if (LOGGER.isWarnEnabled()) {
786786
LOGGER.warn("Superinterface '{}' not found on classpath during code generation. "

‎databind/src/main/java/gov/nist/secauto/metaschema/databind/metapath/function/Model.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.List;
2323

2424
import edu.umd.cs.findbugs.annotations.NonNull;
25+
import edu.umd.cs.findbugs.annotations.Nullable;
2526

2627
/**
2728
* Implementation of the model() Metapath function.
@@ -106,6 +107,7 @@ public static ISequence<?> execute(
106107
* the definition node item to get the model for
107108
* @return the source node item, or {@code null} if not available
108109
*/
110+
@Nullable
109111
public static INodeItem getModel(@NonNull IDefinitionNodeItem<?, ?> definitionNodeItem) {
110112
INamedInstance instance = definitionNodeItem.getInstance();
111113

‎databind/src/main/java/gov/nist/secauto/metaschema/databind/model/metaschema/impl/InstanceModelGroupedAssemblyReference.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,13 @@ public class InstanceModelGroupedAssemblyReference
5757
* the assembly instance for the underlying bound class
5858
* @param position
5959
* the zero-based position of this instance relative to its bound
60-
* siblings
60+
* siblings; must be a valid index within the model items
6161
* @param definition
6262
* the global assembly definition being referenced
6363
* @param parent
6464
* the parent choice group instance containing this reference
65+
* @throws IndexOutOfBoundsException
66+
* if the position is invalid when the source node item is accessed
6567
*/
6668
public InstanceModelGroupedAssemblyReference(
6769
@NonNull AssemblyModel.ChoiceGroup.Assembly binding,

0 commit comments

Comments
 (0)