fix(hide_comment): surface GraphQL node ID guidance in comment_id validation error

[Copilot]

When hide_comment receives a numeric REST comment ID (e.g. 4748731349), the collection-time error "hide_comment requires a 'comment_id' field (string)" misdirects agents toward a simple int→string coercion that still fails downstream. The field prefers a GraphQL node ID (IC_kwDOABCD123456), though numeric REST IDs are accepted at runtime and resolved where possible.

Changes

• FieldValidation struct / JSDoc typedef — adds optional typeHint field (typeHint string, serialized as typeHint in JSON) that overrides the type label in required-field and wrong-type error messages.
• safe_outputs_validation_config.go — sets TypeHint on hide_comment.comment_id:

  "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)"
  

• safe_output_type_validator.cjs — both error paths (value === null/undefined and typeof value !== "string" for required fields) now use typeHint when present.

Before:

hide_comment requires a 'comment_id' field (string)

After:

hide_comment requires a 'comment_id' field (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))

Tests added for missing, numeric, and valid GraphQL node ID comment_id values. Assertions check for the key "GraphQL node ID" substring rather than exact prose, so they remain stable under copy changes.

[Copilot]

Pull request overview

This PR improves the safe outputs validation experience for hide_comment by allowing the validation config to provide a more specific, user-facing “type label” for required-field / wrong-type errors (e.g., clarifying that comment_id should be a GraphQL node ID).

Changes:

• Adds an optional typeHint field to the Go FieldValidation config (serialized as typeHint in JSON).
• Updates the JS safe-output type validator to prefer typeHint when constructing required-field and required-wrong-type error messages.
• Adds JS tests covering missing, numeric, and valid hide_comment.comment_id inputs.

Show a summary per file

File	Description
pkg/workflow/safe_outputs_validation_config.go	Adds TypeHint to FieldValidation and sets a hint for hide_comment.comment_id.
actions/setup/js/safe_output_type_validator.cjs	Uses typeHint when formatting required-field and required-wrong-type errors.
actions/setup/js/safe_output_type_validator.test.cjs	Extends the fixture config and adds tests asserting typeHint appears in errors.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 3/3 changed files
• Comments generated: 4

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #40361 does not have the 'implementation' label (has_implementation_label=false) and has only 2 new lines of code in business logic directories (well under the 100-line threshold).

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

🧪 Test quality analysis by Test Quality Sentinel · 40.7 AIC · ⌖ 12.2 AIC · ⊞ 8.3K · ◷

[github-actions]

✅ Test Quality Sentinel: 80/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%).

[github-actions]

Review: feature is partially implemented

The targeted fix (improving the validation error) is correct and solves the stated problem. One medium-priority gap in the new feature's design worth addressing before expanding its use.

Findings summary

Medium — incomplete typeHint coverage (see inline comment)
typeHint is only honoured in the missing-value path (all types) and the wrong-type path for type === "string". The array, boolean, and number wrong-type branches ignore it. The JSDoc promises "Overrides the type description in error messages" — a promise that holds today because only a string field uses typeHint, but will silently fail for future non-string fields without any warning.

Noted (already raised by existing reviewer) — misleading hint wording
The hint says "not a numeric REST comment ID" but the runtime handler (hide_comment.cjs) accepts stringified numeric IDs (e.g., "4748731349") and resolves them via REST. Consider softening to "GraphQL node ID (e.g., 'IC_kwDOABCD123456') preferred; numeric REST IDs should be provided as strings and require owner/repo context at runtime."

🔎 Code quality review by PR Code Quality Reviewer · 75.2 AIC · ⌖ 7.2 AIC · ⊞ 5.1K

[github-actions]

typeHint is silently ignored for wrong-type errors on array, boolean, and number fields — the feature is incompletely implemented and will mislead future callers.

💡 Details and suggested fix

typeHint is applied in exactly two places:

1. Missing-value path (line 305): validation.typeHint || validation.type || "string" — covers all types ✅
2. Wrong-type path for string (line 332, this line): validation.typeHint || "string" — covers only string ❌

The array, boolean, and number wrong-type branches all hardcode their type labels:

// type === "array" (~line 428)
error: `...requires a '${fieldName}' field (array)`  // typeHint ignored

// type === "boolean" (~line 467)
error: `...'${fieldName}' must be a boolean`           // typeHint ignored

// type === "number" (~line 477)
error: `...'${fieldName}' must be a number`            // typeHint ignored

The JSDoc says "Overrides the type description in error messages" (plural). Anyone who adds typeHint to a type: "number" or type: "array" required field in the Go config will get a silent no-op — the error will still show the hardcoded label.

Fix — apply typeHint in the array required wrong-type branch (and mirror for boolean/number):

if (!Array.isArray(value)) {
  if (validation.required) {
    const fieldType = validation.typeHint || "array";
    return {
      isValid: false,
      error: `Line ${lineNum}: ${itemType} requires a '${fieldName}' field (${fieldType})`,
    };
  }
}

Or centralise the "requires a field" error construction into a shared helper so typeHint is picked up once instead of having to be patched in four separate branches.

[github-actions]

Skills-Based Review 🧠

Applied /diagnose and /tdd — commenting with a few targeted observations. No blocking issues; the core fix is correct and well-tested.

📋 Key Themes & Highlights

Key Themes

• Fallback asymmetry (/diagnose): The wrong-type branch at line 332 uses typeHint || "string", skipping validation.type, while the missing-value branch uses typeHint || validation.type || "string". Functionally identical today (the guard ensures type === "string" already), but a latent trap for future non-string fields with typeHint.
• Incomplete abstraction (/diagnose): typeHint is only wired for the type === "string" and missing-value paths. The type === "array" wrong-type path (lines 423–434, unchanged) still hardcodes "array" with no typeHint support. Worth noting if this feature is extended.
• Test-config drift (/tdd): SAMPLE_VALIDATION_CONFIG in the test file duplicates the production Go struct value. No contract test ties them together — a change to TypeHint in Go silently decouples from the JS tests.
• Partial root-cause fix (/diagnose): The error now correctly steers agents away from JS-number REST IDs. String-coerced REST IDs ("4748731349") still pass silently; a pattern constraint or a known-limitation test would round this out.

Positive Highlights

• ✅ Backward-compatible typeHint field — clean extension of existing struct without overloading type
• ✅ Three targeted tests cover the three meaningful cases (missing, wrong-type, correct value)
• ✅ Very low blast radius — 6 changed logic lines across 2 files
• ✅ Good PR description with before/after error messages

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 71.8 AIC · ⌖ 9.99 AIC · ⊞ 6.9K

[github-actions]

[/diagnose] The fallback chain here diverges from the established pattern in the missing-value branch above.

Line 305 uses validation.typeHint || validation.type || "string", but this line uses validation.typeHint || "string", skipping validation.type. Within the if (validation.type === "string") guard the result is identical today, but the inconsistency will silently produce the wrong fallback label if a future field sets typeHint on a non-string typed field (e.g. a number field) — the wrong-type error would still say "string" rather than "number".

💡 Suggested fix

// wrong-type branch — keep the same fallback chain as the missing-value branch
const fieldType = validation.typeHint || validation.type || "string";

This is also consistent with the validation.typeHint || validation.type pattern, and makes the intent obvious without relying on the outer guard as an implicit constraint.

[github-actions]

[/tdd] The typeHint value here must stay in sync with the Go struct in safe_outputs_validation_config.go, but nothing enforces that contract.

If TypeHint is removed, renamed, or changed in the Go config, these tests pass unaffected because they load from SAMPLE_VALIDATION_CONFIG (a hand-maintained fixture), not from the compiled JSON emitted by the Go build.

💡 Suggested contract test

In compiler_safe_outputs_config_test.go, the existing "hide_comment config" test case only checks expectedKeys: ["hide_comment"]. Adding a strings.Contains assertion on the marshalled JSON is sufficient to lock in the value and catch regressions:

// assert the typeHint is emitted in the serialised validation config
require.Contains(t, marshalledJSON, "GraphQL node ID string")

[github-actions]

[/diagnose] The happy-path test validates "IC_kwDOABCD123456" is accepted, but a stringified numeric REST ID like "4748731349" would also pass — it's a valid JS string.

The PR correctly addresses the JS-number case (agents returning 4748731349). Agents that stringify their IDs before passing them ("4748731349") are a plausible second failure mode that remains silently uncaught.

💡 Follow-up options

Option A — add a pattern constraint (enforces format, prevents silent acceptance):

// in safe_outputs_validation_config.go
"comment_id": {Required: true, Type: "string", MaxLength: 256,
    Pattern: `^[A-Za-z0-9_]+$`,
    TypeHint: "GraphQL node ID string (e.g. 'IC_kwDOABCD123456'), not a numeric REST comment ID"},

Option B — add a test case documenting current behaviour (cheaper, makes the gap explicit):

it("accepts stringified numeric REST id (known limitation — no pattern check)", async () => {
  const { validateItem } = await import("./safe_output_type_validator.cjs");
  const result = validateItem({ type: "hide_comment", comment_id: "4748731349" }, "hide_comment", 1);
  // Documents that string-coerced REST IDs still slip through
  expect(result.isValid).toBe(true);
});

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Done. Three changes made in response to the reviewer feedback:

1. Softened TypeHint in safe_outputs_validation_config.go — now reads: "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)", which accurately reflects the handler's actual behaviour.
2. Updated test fixture to match the new text.
3. Loosened test assertions — both error-path tests now assert only toContain("GraphQL node ID") instead of exact prose, making them resilient to copy changes.
4. Regenerated lock files via make recompile — the two affected .lock.yml files now embed the updated hint.

All local checks pass (make agent-report-progress, make test-js).