Skip to content

Commit daa889e

Browse files
authored
fix(hide_comment): surface GraphQL node ID guidance in comment_id validation error (#40361)
1 parent 7be9b99 commit daa889e

5 files changed

Lines changed: 46 additions & 3 deletions

File tree

.github/workflows/ai-moderator.lock.yml

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

.github/workflows/smoke-codex.lock.yml

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

actions/setup/js/safe_output_type_validator.cjs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ function normalizeIssueClosingKeywordBackticks(content) {
6767
* @typedef {Object} FieldValidation
6868
* @property {boolean} [required] - Whether the field is required
6969
* @property {string} [type] - Expected type: 'string', 'number', 'boolean', 'array'
70+
* @property {string} [typeHint] - Overrides the type description in error messages (e.g. "GraphQL node ID string")
7071
* @property {boolean} [sanitize] - Whether to sanitize string content
7172
* @property {number} [maxLength] - Maximum length for strings
7273
* @property {number} [minLength] - Minimum length for strings
@@ -301,7 +302,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options)
301302

302303
// Handle required check for other fields
303304
if (validation.required && (value === undefined || value === null)) {
304-
const fieldType = validation.type || "string";
305+
const fieldType = validation.typeHint || validation.type || "string";
305306
return {
306307
isValid: false,
307308
error: `Line ${lineNum}: ${itemType} requires a '${fieldName}' field (${fieldType})`,
@@ -328,9 +329,10 @@ function validateField(value, fieldName, validation, itemType, lineNum, options)
328329
if (typeof value !== "string") {
329330
// For required fields, use "requires a" format for both missing and wrong type
330331
if (validation.required) {
332+
const fieldType = validation.typeHint || "string";
331333
return {
332334
isValid: false,
333-
error: `Line ${lineNum}: ${itemType} requires a '${fieldName}' field (string)`,
335+
error: `Line ${lineNum}: ${itemType} requires a '${fieldName}' field (${fieldType})`,
334336
};
335337
}
336338
return {

actions/setup/js/safe_output_type_validator.test.cjs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,18 @@ const SAMPLE_VALIDATION_CONFIG = {
164164
inputs: { type: "object" },
165165
},
166166
},
167+
hide_comment: {
168+
defaultMax: 5,
169+
fields: {
170+
comment_id: {
171+
required: true,
172+
type: "string",
173+
maxLength: 256,
174+
typeHint: "GraphQL node ID string (e.g. 'IC_kwDOABCD123456'); numeric REST comment IDs are accepted but may not resolve for all comment types (e.g. PR review comments)",
175+
},
176+
reason: { type: "string" },
177+
},
178+
},
167179
};
168180

169181
const ISSUE_CLOSING_KEYWORDS = ["fix", "fixes", "fixed", "close", "closes", "closed", "resolve", "resolves", "resolved"];
@@ -463,6 +475,32 @@ describe("safe_output_type_validator", () => {
463475
expect(result.error).toContain("pull_request_number");
464476
}
465477
});
478+
479+
it("should include typeHint in error when hide_comment comment_id is missing", async () => {
480+
const { validateItem } = await import("./safe_output_type_validator.cjs");
481+
482+
const result = validateItem({ type: "hide_comment" }, "hide_comment", 1);
483+
484+
expect(result.isValid).toBe(false);
485+
expect(result.error).toContain("GraphQL node ID");
486+
});
487+
488+
it("should include typeHint in error when hide_comment comment_id is a numeric REST id", async () => {
489+
const { validateItem } = await import("./safe_output_type_validator.cjs");
490+
491+
const result = validateItem({ type: "hide_comment", comment_id: 4748731349 }, "hide_comment", 1);
492+
493+
expect(result.isValid).toBe(false);
494+
expect(result.error).toContain("GraphQL node ID");
495+
});
496+
497+
it("should accept hide_comment with a GraphQL node ID comment_id", async () => {
498+
const { validateItem } = await import("./safe_output_type_validator.cjs");
499+
500+
const result = validateItem({ type: "hide_comment", comment_id: "IC_kwDOABCD123456" }, "hide_comment", 1);
501+
502+
expect(result.isValid).toBe(true);
503+
});
466504
});
467505

468506
describe("validatePositiveInteger", () => {

pkg/workflow/safe_outputs_validation_config.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ var safeOutputValidationLog = logger.New("workflow:safe_outputs_validation_confi
1616
type FieldValidation struct {
1717
Required bool `json:"required,omitempty"`
1818
Type string `json:"type,omitempty"`
19+
TypeHint string `json:"typeHint,omitempty"` // Overrides the type description in error messages (e.g. "GraphQL node ID string")
1920
Sanitize bool `json:"sanitize,omitempty"`
2021
MaxLength int `json:"maxLength,omitempty"`
2122
MinLength int `json:"minLength,omitempty"`
@@ -403,7 +404,7 @@ var ValidationConfig = map[string]TypeValidationConfig{
403404
"hide_comment": {
404405
DefaultMax: 5,
405406
Fields: map[string]FieldValidation{
406-
"comment_id": {Required: true, Type: "string", MaxLength: 256},
407+
"comment_id": {Required: true, Type: "string", MaxLength: 256, TypeHint: "GraphQL node ID string (e.g. 'IC_kwDOABCD123456'); numeric REST comment IDs are accepted but may not resolve for all comment types (e.g. PR review comments)"},
407408
"reason": {Type: "string", Enum: []string{"SPAM", "ABUSE", "OFF_TOPIC", "OUTDATED", "RESOLVED", "LOW_QUALITY"}},
408409
"repo": {Type: "string", MaxLength: 256}, // Optional: target repository in format "owner/repo"
409410
},

0 commit comments

Comments
 (0)