JS-1450 chore: update Claude skills with guidance from PR review feedback (#6584)

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Author: 3 people

.claude/skills/helpers/SKILL.md

@@ -24,10 +24,23 @@ Helper functions are located in `packages/jsts/src/rules/helpers/`. These provid
 
 ## Usage Guidance
 
-When implementing a fix:
-1. Review the `packages/jsts/src/rules/helpers/` directory for existing utilities
-2. Use existing helpers instead of writing custom logic where possible
-3. This ensures consistency and reduces code duplication
+**Before writing any new function**, search `packages/jsts/src/rules/helpers/` for an existing equivalent. Key files to check:
+- `ast.ts` — generic AST utilities (child enumeration, node classification, ancestor walking)
+- `vue.ts` — Vue-specific helpers
+- `react.ts` — React/JSX-specific helpers
+
+Reviewers will reject re-implementations of existing helpers.
+
+**Anti-pattern:** Iterating `Object.keys(node)` to enumerate child nodes. Use the existing `childrenOf` helper instead:
+```typescript
+// ❌ Do not do this
+for (const key of Object.keys(node)) { ... }
+
+// ✅ Use this
+for (const child of childrenOf(node)) { ... }
+```
+
+If you write a **new** utility function that only operates on `estree`/`TSESTree` nodes with no rule-specific domain logic, place it in `helpers/ast.ts` (or `helpers/vue.ts`, `helpers/react.ts` for domain-specific utilities) — not inside the individual rule file. Functions buried in a rule file are invisible to future implementers.
 
 ## Finding Helpers
 
@@ -37,3 +50,21 @@ ls packages/jsts/src/rules/helpers/
 ```
 
 Read helper implementations to understand their usage and parameters.
+
+## Helper Contracts
+
+### childrenOf
+
+`childrenOf(node)` always returns `estree.Node[]`. Do not write custom type guard predicates or `Array.isArray()` checks on its return values — they are dead code:
+
+```typescript
+// ❌ Redundant — childrenOf already guarantees estree.Node[]
+for (const child of childrenOf(node)) {
+  if (typeof child === 'object' && 'type' in child) { ... }
+}
+
+// ✅ Iterate directly
+for (const child of childrenOf(node)) {
+  // child is already estree.Node
+}
+```

.claude/skills/rule-implementation/SKILL.md

@@ -131,3 +131,275 @@ After modifying configuration or metadata files, regenerate the derived files:
 
 - `npm run generate-meta` - Regenerate rule metadata after changes to `meta.ts` or `config.ts`
 - `npm run generate-java-rule-classes` - Regenerate Java rule classes (required after `config.ts` changes)
+
+---
+
+## AST Traversal Strategy
+
+### Prefer ESLint selector-based node collection over recursive walks
+
+When implementing a rule, use ESLint's selector-based dispatch instead of walking the AST manually:
+
+```typescript
+// ❌ Recursive walk — expensive and brittle
+create(context) {
+  return {
+    FunctionDeclaration(node) {
+      const returns = collectReturns(node.body); // recursive walk inside
+    }
+  };
+}
+
+// ✅ Selector + side-table — ESLint dispatches once per node
+create(context) {
+  const returnsByFunction = new Map();
+  return {
+    ReturnStatement(node) {
+      const fn = getEnclosingFunction(node);
+      if (fn) returnsByFunction.set(fn, [...(returnsByFunction.get(fn) ?? []), node]);
+    },
+    'FunctionDeclaration:exit'(node) {
+      const returns = returnsByFunction.get(node) ?? [];
+      // analyse returns
+    }
+  };
+}
+```
+
+Selector-based approaches let ESLint dispatch to your handler exactly once per matching node at no extra traversal cost, and automatically handle all node types including future additions to the language.
+
+### When recursive traversal is unavoidable, use visitorKeys — not switch
+
+If you must traverse a subtree recursively inside a helper (e.g. `containsAssignment`, `nodeHasReturn`), enumerate child properties generically using ESLint's `visitorKeys` instead of a manual `switch`:
+
+```typescript
+// ❌ Manual switch — silently drops unlisted node types
+function containsReturn(node: estree.Node): boolean {
+  switch (node.type) {
+    case 'ReturnStatement': return true;
+    case 'BlockStatement': return node.body.some(containsReturn);
+    default: return false; // silently misses loops, try/catch, switch bodies...
+  }
+}
+
+// ✅ Generic traversal via visitorKeys
+import { visitorKeys } from '@typescript-eslint/visitor-keys';
+function containsReturn(node: estree.Node): boolean {
+  if (node.type === 'ReturnStatement') return true;
+  const keys = visitorKeys[node.type] ?? [];
+  return keys.some(key => {
+    const child = (node as Record<string, unknown>)[key];
+    if (Array.isArray(child)) return child.some(c => c && containsReturn(c as estree.Node));
+    if (child && typeof child === 'object') return containsReturn(child as estree.Node);
+    return false;
+  });
+}
+```
+
+---
+
+## TypeScript Type Safety
+
+### Unsafe type assertions must be guarded or explained
+
+Do not cast AST nodes silently. A cast is safe only if it holds for every node type the upstream rule can report on.
+
+```typescript
+// ❌ Silent cast — crashes if upstream rule ever reports a non-Identifier node
+const identifier = tsNode as TSESTree.Identifier;
+
+// ✅ Type guard
+if (tsNode.type !== 'Identifier') return;
+const identifier = tsNode; // narrowed
+
+// ✅ Explanatory comment when guard is impractical
+// Safe: prefer-single-call only reports Identifier property nodes; computed access is excluded.
+const identifier = tsNode as TSESTree.Identifier;
+```
+
+### Enumerate all upstream report sites before casting reportDescriptor.node
+
+When writing a decorator, inspect **every** `context.report()` call in the upstream rule to determine all possible node types. A cast that only covers the common case will crash on edge cases.
+
+```typescript
+// ❌ Assumes only one node type is ever reported
+const node = descriptor.node as TSESTree.TSCallSignatureDeclaration;
+
+// ✅ After auditing all context.report() calls in the upstream source
+const node = descriptor.node as TSESTree.TSCallSignatureDeclaration | TSESTree.TSConstructSignatureDeclaration;
+```
+
+This audit is the natural follow-on to Step 1 ("Find All Report Calls") in the FP tracing workflow above.
+
+### Use specific node types in function signatures
+
+Declare parameters with the most specific `estree`/`TSESTree` type the function actually requires. Using the base `estree.Node` hides the contract and disables type-checker assistance:
+
+```typescript
+// ❌ Too broad
+function isInDirectionalContext(node: estree.Node): boolean { ... }
+
+// ✅ Specific
+function isInDirectionalContext(node: estree.CallExpression): boolean { ... }
+```
+
+---
+
+## TypeScript Compiler API
+
+### Use mutual assignability to test type equivalence
+
+One-directional `isTypeAssignableTo(A, B)` only proves that `A` is a structural subtype of `B`. It returns `true` even when `B` is much wider, causing wrong-trigger suppressions.
+
+```typescript
+// ❌ One-directional — false matches when B has extra optional fields
+if (checker.isTypeAssignableTo(typeA, typeB)) { suppress(); }
+
+// ✅ Mutual — true only when both types are structurally equivalent
+if (checker.isTypeAssignableTo(typeA, typeB) && checker.isTypeAssignableTo(typeB, typeA)) {
+  suppress();
+}
+```
+
+### Use bitwise operators for TypeFlags comparisons
+
+`ts.TypeFlags` values are bitmasks. A type can have multiple flags set simultaneously. Using `===` against a single flag value fails whenever any other flag is also set:
+
+```typescript
+// ❌ Fails when multiple flags are set (common with union types)
+if (type.flags === ts.TypeFlags.Any) { ... }
+
+// ✅ Bitwise mask — true whenever the flag is set, regardless of other flags
+if (type.flags & ts.TypeFlags.Any) { ... }
+if (type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { ... }
+```
+
+---
+
+## FP Remediation Quality
+
+### Do not suppress reports that the rule is correct to raise
+
+Not every complaint is a genuine false positive. Two cases to watch for:
+
+**strictNullChecks disabled:** Do not add an exemption that suppresses non-null assertion (`!`) reports when `strictNullChecks` is off. Without `strictNullChecks`, `!` is a no-op — but suppressing the report hides future migration debt. Let the rule fire and guide developers to write proper null guards.
+
+**Rule goal conflicts:** Before implementing an exception, re-read the upstream rule's documentation. If the exception would allow exactly the pattern the rule was designed to catch, the report is not a false positive. Do not proceed with the fix; flag it for human review instead.
+
+### Cover the full API family when adding an exception
+
+When adding a suppression exception for one method, identify all semantically equivalent siblings in the same API family and include them in the same guard:
+
+```typescript
+// ❌ Only covers Object.keys — Object.getOwnPropertyNames users still get FPs
+if (isCallingMethod(node, 1, 'keys')) { suppress(); }
+
+// ✅ Covers the full family
+if (isCallingMethod(node, 1, 'keys', 'getOwnPropertyNames', 'getOwnPropertySymbols')) {
+  suppress();
+}
+```
+
+Add test cases and update rule documentation for each covered variant.
+
+---
+
+## Decorator Design
+
+### When all identification strategies fail, preserve the report
+
+When a decorator uses multiple strategies to identify the owning component, and all strategies fail, preserve the original report. Do not fall back to `context.sourceCode.ast` (the entire file AST) as a catch-all scope:
+
+```typescript
+// ❌ File-scope fallback — suppresses reports the decorator cannot correctly attribute
+const scope = findOwner(node) ?? context.sourceCode.ast;
+if (isSuppressible(node, scope)) return;
+
+// ✅ No owner found → preserve the report
+const owner = findOwner(node);
+if (!owner) {
+  context.report(reportDescriptor); // original report is more likely correct
+  return;
+}
+```
+
+### Scope suppression decisions to the component, not the file
+
+Never cache a suppression decision at the file level. If one component matches, a file-level cache incorrectly suppresses reports for other components in the same file:
+
+```typescript
+// ❌ File-level cache — contaminates all components in the file
+let fileHasSuppressibleProps = false;
+return {
+  'Program:exit'() {
+    if (fileHasSuppressibleProps) return; // suppresses everything in file
+  }
+};
+
+// ✅ Per-report decision scoped to the component
+(context, descriptor) => {
+  const owner = findOwner(descriptor.node);
+  if (owner && isSuppressible(descriptor.node, owner)) return;
+  context.report(descriptor);
+}
+```
+
+### Resolve spread expressions before suppressing
+
+Do not suppress a report simply because a JSX spread attribute `{...expr}` is present. Attempt to resolve `expr` statically first:
+
+```typescript
+// ❌ Suppresses on mere presence of spread — may produce false negatives
+if (node.openingElement.attributes.some(a => a.type === 'JSXSpreadAttribute')) return;
+
+// ✅ Resolve the spread value first
+const spreadAttr = node.openingElement.attributes.find(a => a.type === 'JSXSpreadAttribute');
+if (spreadAttr) {
+  const value = getValueOfExpression(context, spreadAttr.argument, 'ObjectExpression');
+  if (!value) return; // unresolvable — suppress conservatively
+  if (hasRelevantProperties(value)) return; // spread carries the relevant content
+  // else: spread resolves to irrelevant object — do not suppress
+}
+```
+
+### Document multi-strategy decorator helpers with JSDoc and inline comments
+
+When a decorator helper uses multiple identification strategies, add JSDoc describing the strategy sequence and inline comments at each branch:
+
+```typescript
+/**
+ * Finds the React component that owns the reported props interface.
+ * Strategy A: match by component name in the same file.
+ * Strategy B: match by structural subtyping against known prop shapes.
+ * Strategy C: match by mutual type assignability.
+ * Returns null if no owner is found; the caller must preserve the report in that case.
+ */
+function findOwner(node: TSESTree.Node): ComponentNode | null {
+  // Strategy A: direct name match
+  const byName = findByName(node);
+  if (byName) return byName;
+
+  // Strategy B: structural subtype check (faster, less precise)
+  const byShape = findByShape(node);
+  if (byShape) return byShape;
+
+  // Strategy C: mutual assignability (slower, most precise)
+  return findByMutualAssignability(node);
+}
+```
+
+---
+
+## Code Style
+
+### Use optional chaining for optional array length checks
+
+```typescript
+// ❌ Redundant double guard
+if (arr && arr.length > 0) { ... }
+if (declaration?.typeParameters && declaration.typeParameters.length > 0) { ... }
+
+// ✅ Optional chaining short-circuits on undefined; undefined > 0 is false
+if (arr?.length > 0) { ... }
+if (declaration?.typeParameters?.length > 0) { ... }
+```