Review skill: enumerate all node shapes of a matched construct

Address review feedback: a guard can miss a construct that lives in a
different node shape entirely, not only a different name. arrayJoin is
the case: the function form is ASTFunction/FunctionNode, but the
ARRAY JOIN clause is ASTArrayJoin/ArrayJoinNode, so banning only the
function still leaves the clause open.

The trigger now tells the reviewer to first enumerate every AST and
query-tree representation of the construct, then apply the alias, UDF,
and alternate-spelling checks to each name-based carrier.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

.claude/skills/review/SKILL.md

@@ -121,7 +121,7 @@ Run these only when the trigger appears. They are small expansion passes, not a
 
 - **After first serious invariant failure:** fan out once through the same invariant in foreground paths, background paths, DDL/mutating entrypoints, lifecycle transitions, and sibling engines/settings. Group related issues when they share a cause, but do not omit distinct user-impacting paths.
 - **New setting/flag/option:** grep consumers that share the settings class or configuration surface. Each relevant engine/mode/API must implement it, reject it, or make an explicit harmless no-op contract.
-- **Name-based matching of a SQL construct:** fires when the diff inspects a query to allow, reject, rewrite, or special-case a construct by matching its name or node kind (for example a specific function, keyword, or operator). This applies to both representations: the raw AST (`ASTFunction`) and the analyzer query tree (`FunctionNode::getFunctionName`). A user can reach the same semantics through forms the surface name does not reveal, so the matcher misses cases unless it accounts for all of them. Verify the guard handles: (a) **name aliases**: resolve to the canonical name (case-insensitive and registered aliases) instead of comparing a literal string; (b) **user-defined expansions**: SQL UDF bodies, and any macro, lambda, or view inlining, that expand to the construct, so descend into them with cycle protection; (c) **alternate spellings**: equivalent operators or syntaxes for the same construct. The two representations normalize different things, so confirm which forms the chosen one already resolves and which the guard must still handle. On the raw AST nothing is resolved. On the query tree the analyzer may already inline UDFs and turn operators into functions, but the stored name can still be an alias. Also confirm the guard runs at (or is duplicated at) the point where these expansions have already happened. A check placed before inlining that is not re-checked after inlining is a gap.
+- **Matching a SQL construct in a query:** fires when the diff inspects a query to allow, reject, rewrite, or special-case a construct (for example a specific function, keyword, or operator). First enumerate every AST and query-tree representation of the construct's semantics, not just the one the diff happens to match. The same semantics often live in more than one node shape. `arrayJoin` is the motivating example: the expression form is an `ASTFunction` / `FunctionNode`, but the `ARRAY JOIN` clause is a separate node (`ASTArrayJoin` / `ArrayJoinNode`), so a guard that bans only the function still misses the clause. A guard that covers only one carrier is a gap even if its name matching is perfect. Then, for each name-based carrier, a user can reach the same semantics through forms the surface name does not reveal, so verify the guard handles: (a) **name aliases**: resolve to the canonical name (case-insensitive and registered aliases) instead of comparing a literal string; (b) **user-defined expansions**: SQL UDF bodies, and any macro, lambda, or view inlining, that expand to the construct, so descend into them with cycle protection; (c) **alternate spellings**: equivalent operators or syntaxes for the same construct. The AST and the query tree normalize different things, so confirm which forms the chosen one already resolves and which the guard must still handle. On the raw AST nothing is resolved. On the query tree the analyzer may already inline UDFs and turn operators into functions, but the stored name can still be an alias. Also confirm the guard runs at (or is duplicated at) the point where these expansions have already happened. A check placed before inlining that is not re-checked after inlining is a gap.
 - **Build flag that drops a global side-effect or weakens numeric semantics:** fires when the diff changes compiler/build flags that remove a documented side-effect or relax FP behavior across many TUs (`-fno-math-errno`, `-ffast-math`, `-ffp-contract=fast`, `-fassociative-math`, `-fno-signed-zeros`, strict-aliasing relaxation, and similar). Read `references.md#build-flags` and follow that procedure before reviewing.
 - **Native protocol or native format change:** fires when the diff touches the native TCP protocol or the `Native` format wire layout (see the file/symbol list in `references.md#spec-sync`). Read that section and verify the matching spec update.
 - **New or clarified invariant:** when a PR introduces, tightens, or makes explicit an invariant for a value, type, state, operation, permission, lifecycle, setting, or error condition, verify it across the existing system, not only in the changed code. State the invariant independently of the implementation, then trace pre-existing carriers and paths that can create, copy, transform, store, cache, serialize, expose, or act on the affected data or state. This includes sibling fields, old members, wrapper types, subclasses, alternate constructors, default values, legacy configuration/data, generated values, copied metadata, parallel code paths, and equivalent implementations in other engines or modes. Each path must either preserve the invariant by construction, enforce it before use, reject unsupported cases, or document a deliberate exception. Tests should prove the invariant at the boundary where a violation would matter, not only at the helper or code path touched by the patch.