…r drag

## Codex P2 #1: User row click hijacks group badge navigation

Clicking a group badge inside `UserGroupsCell` bubbled up to the new
`<TableRow onClick>` handler, which opened the user detail sheet *and*
navigated to the group — producing two competing route pushes.

Added `e.stopPropagation()` in the badge's onClick so badge clicks only
navigate to the group, as intended. Matches the pattern already used on
Delete/Restore buttons in the same rows.

## Codex P2 #2: Stale async selection race in WorkloadIdentitiesPage

`handleUserSelected` awaits `getOrFetchWorkloadIdentity` before calling
`setEditingWI`. If a user clicks two different rows in quick succession
on a cold cache, the slower first request can resolve *after* the second
and overwrite `editingWI` with the wrong identity.

Added a sequence-counter guard (`selectSeqRef`): each click bumps the
counter, and only the response whose captured `seq` still matches the
latest counter is allowed to update state. Stale responses are dropped.

## Environment reorder drag-and-drop was broken

Reported by the user — dragging environments in the Reorder sheet did
nothing. Two root causes:

1. **`onDragStart` never initialized `dataTransfer`.** HTML5 DnD requires
   `dataTransfer.setData()` to start a drag in Firefox, and
   `effectAllowed` + `dropEffect` for the drop cursor in Chrome. The old
   handler only called `setDragIndex(index)` on a closure that didn't
   receive the DragEvent — so dataTransfer was never touched. In some
   browsers this silently aborted the drag.

2. **Mid-drag list mutation.** The old `onDragOver` spliced `list` and
   called `setList(newList)` on *every* dragover event. Each `setList`
   re-renders the whole sheet Popup, and Chrome's drag session can abort
   when the source element re-mounts or its identity changes during the
   drag.

Rewrote with standard HTML5 DnD pattern:
- `onDragStart` sets `effectAllowed = "move"` and
  `dataTransfer.setData("text/plain", String(index))`.
- `onDragOver` calls `preventDefault()` (to mark the element as a valid
  drop target) and updates a separate `dragOverIndex` state purely for
  visual feedback (border highlight). The list itself is not mutated.
- `onDrop` reads the source index from `dataTransfer`, splices once,
  calls `setList` once.
- `onDragEnd` clears both `dragIndex` and `dragOverIndex`.

Also added a visible drop-target highlight (`border-accent bg-accent/5`)
on the hovered row so the user sees where the drop will land.

Type-check, lint, all checks pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…resource-list UX (#19962)

* feat(frontend): add Sheet primitive, migrate Custom Approval Rule to right-side drawer

The frontend has no Sheet/Drawer primitive in shadcn `ui/`, so every existing
"drawer" (RoleDrawer, EditEnvironmentDrawer) is hand-rolled with
`fixed inset-0 z-50` + a manual backdrop, manual escape handling, and an
inlined one-off width. That's the same missing-primitive pattern we just
fixed for DropdownMenu — different call sites, same drift (different widths,
different backdrop tokens, different escape hooks).

Meanwhile, multiple complex resource-edit forms are still centered dialogs
where a side drawer fits far better — Custom Approval Rule is the clearest
example, with presets + title + description + a CEL expression builder + a
draggable approval-nodes table all crammed into a modal that was already
sized `lg:max-w-[75vw] 2xl:max-w-[55vw]` just to fit the content.

This PR builds the missing primitive and uses Custom Approval Rule as the
reference implementation that proves it works end-to-end.

- Add `frontend/src/react/components/ui/sheet.tsx` wrapping Base UI's Dialog
  primitive (same `@base-ui/react/dialog` powering `dialog.tsx`) with a
  right-side layout. Focus trap, Escape, outside-click, ARIA, and portal all
  come free from Base UI — no more hand-rolled hooks.
- Codify three width tiers via `cva` on `SheetContent`:
    narrow   (w-[24rem], 384px) — single-field pickers, short forms
    standard (w-[44rem], 704px) — 3-6 field forms, permission transfer
    wide     (w-[52rem], 832px) — CEL builders, nested tables, multi-tab
  New consumers must pick a tier — no more ad-hoc widths.
- Match the overlay-stacking rule in `frontend/AGENTS.md`: backdrop and popup
  both at `z-50`, matching Dialog/Select/Tooltip/DropdownMenu so later-mounted
  portals (e.g. a Select opened *inside* a Sheet) correctly render on top.
- Slide-in transition keyed off Base UI's `data-starting-style` /
  `data-ending-style` data attributes.
- Export `Sheet`, `SheetTrigger`, `SheetContent`, `SheetHeader`, `SheetBody`,
  `SheetFooter`, `SheetTitle`, `SheetDescription`, `SheetClose`, `SheetOverlay`.

Migration:
- Rename `RuleEditDialog.tsx` → `RuleEditSheet.tsx` and component
  `RuleEditDialog` → `RuleEditSheet`.
- Replace Dialog/DialogContent/DialogTitle with
  Sheet/SheetContent width="wide"/SheetHeader/SheetBody/SheetFooter/SheetTitle.
- Drop the hand-rolled `flex flex-col overflow-hidden lg:max-w-[75vw]` and
  header/body/footer div chrome — that's now the primitive's job.
- Update the single importer in `RulesSection.tsx`.

All checks green: type-check, ESLint + Biome, React i18n, 1031 unit tests,
dev server compiles and serves HTTP 200.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(frontend): migrate standalone hand-rolled drawers to Sheet primitive

Replaces the hand-rolled `fixed inset-0 z-50` drawer markup in 7 standalone
files with the shadcn Sheet primitive, renaming each `*Drawer` to `*Sheet`
to match the shadcn naming convention. Also extends the Sheet primitive with
a built-in close button (top-right corner, absolute positioned) so consumers
no longer need to render their own X close button.

Migrated files (all renamed *Drawer → *Sheet):
- components/EditEnvironmentDrawer.tsx (narrow) — single environment picker
- components/CreateWorkloadIdentityDrawer.tsx (standard) — conditional provider fields
- components/database/LabelEditorDrawer.tsx (narrow) — bulk database labels editor
- components/database/TransferProjectDrawer.tsx (standard) — project picker w/ search
- components/database/CreateDatabaseDrawer.tsx (standard) — engine-conditional form
- pages/settings/shared/AADSyncDrawer.tsx (standard) — SCIM config display
- pages/project/export-center/DataExportPrepDrawer.tsx (wide) — multi-step export wizard

Importers updated: ProjectDatabasesPage, DatabasesPage (settings),
ProjectDataExportPage, ProjectGitOpsPage, WorkloadIdentitiesPage,
GroupsPage, UsersPage, ProjectDatabaseDetailPage (+ its test mock).

Also: Sheet primitive now includes a `<Dialog.Close>` in SheetContent that
renders an absolute-positioned X button in the top-right corner. SheetHeader
now reserves right-padding (`pr-12`) to avoid overlap with the close button.

Type-check passes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(frontend): migrate page-local drawers to Sheet primitive + fix edit-mode regression

Migrates the inline drawer components in 6 page files (EnvironmentsPage
CreateDrawer/ReorderDrawer, UsersPage CreateUserDrawer, ServiceAccountsPage
CreateServiceAccountDrawer, GroupsPage CreateGroupDrawer, LandingPage
ConfigDrawer, CreateWorkloadIdentitySheet standalone) from hand-rolled
`fixed inset-y-0 right-0` markup to the Sheet primitive. All renamed
locally (*Drawer → *Sheet) to match the shadcn naming convention.

**Fixes a regression introduced by the Sheet migration pattern**:

The original `{showX && <Drawer .../>}` pattern caused the drawer to
*remount* on every open, which re-ran every `useState(user?.title ?? "")`
initializer against the latest `user` prop — so editing an existing row
showed its populated fields. The new `<Sheet open={showX}/>` pattern keeps
the Sheet mounted across opens so those initializers only ran *once*
(during first mount, when `user` was undefined). Result: clicking View /
Edit on an existing user / group / service account / workload identity
opened the Sheet with *empty* fields instead of populated ones.

Fix: add an explicit `useEffect(() => { if (open) reset from props }, [open, entity])`
to each affected Sheet so state is repopulated every time the Sheet is
opened — whether in create mode (resets to defaults) or edit mode (copies
from the passed entity). Applied to:

- UsersPage.tsx / CreateUserSheet
- ServiceAccountsPage.tsx / CreateServiceAccountSheet
- GroupsPage.tsx / CreateGroupSheet
- CreateWorkloadIdentitySheet.tsx (latent bug — existing useEffect ran
  only on mount)
- EnvironmentsPage.tsx / CreateSheet (reset defaults on reopen)
- EnvironmentsPage.tsx / ReorderSheet (reset list from props on reopen)

Call sites also updated from `{showX && <Drawer/>}` to `<Sheet open={showX}/>`
for consistency, which is what revealed the regression in the first place.

Reported by the user after clicking "View" on a user row in Users page —
screenshot confirmed empty fields. Same pattern applied to Service Accounts.

Type-check passes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(frontend): migrate ProjectsPage/InstancesPage/RolesPage drawers to Sheet primitive

- ProjectsPage CreateProjectDrawer → CreateProjectSheet (standard width)
- InstancesPage inline EditEnvironmentDrawer → EditEnvironmentSheet (narrow)
- RolesPage RoleDrawer → RoleSheet (standard width)

All use the Sheet primitive's width tiers instead of hand-rolled `fixed inset-y-0 right-0` panels, and remove the custom X close button (Sheet provides one in the top-right automatically). The `{open && ...}` conditional renders are replaced with `<Sheet open={open}/>`.

The RoleSheet previously hid itself via `if (!role) return null`; now it uses `open={detailRole !== undefined}` passed from the parent. The existing `useEffect([role])` that syncs the incoming role prop to local state continues to handle edit-mode population.

Type-check passes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(frontend): reliably populate edit sheets via key-remount + add dirty tracking

The earlier `useEffect([open, entity])` reset pattern was fragile:

1. **Workload Identity** has two chained `useEffect` blocks that sync fields
   ↔ subject pattern bidirectionally via ref guards. The reset effect set
   owner/repo/branch, but the sync effects then re-ran with stale refs and
   sometimes clobbered them — leaving Organization / Repository / Branch
   empty in the screenshot the user sent.

2. For **User / Service Account / Group**, the reset effect worked in
   principle but was noisy (lots of setXxx calls inside a useEffect) and
   brittle to add dirty tracking on top of (no clean baseline to compare
   against).

Switch all four edit sheets to the **outer wrapper + inner form + key**
pattern:

    <Sheet open={open}>
      <SheetContent>
        {open && <InnerForm key={entity?.name ?? 'new'} {...props} />}
      </SheetContent>
    </Sheet>

- Outer `CreateXxxSheet` is a thin wrapper that only renders the Sheet.
- Inner `XxxForm` is mounted fresh every time the Sheet opens, because
  `{open && ...}` unmounts it on close and the `key` forces a remount when
  the entity changes.
- useState initializers in InnerForm read directly from the entity prop —
  they always see the latest values because the component is freshly
  mounted.
- No reset effect needed. No ref-guarded sync loops. No stale state bleed
  between opens.

Also adds **dirty tracking** to the Update button: in edit mode, the button
is disabled unless at least one field differs from its initial value. In
create mode, the button is always allowed (validation still applies).

Captures initial values at mount and computes `isDirty` via `useMemo`.
`allowConfirm = isFormValid && isDirty`.

Applied to:
- CreateWorkloadIdentitySheet — owner/repo/branch now populate from
  parseWorkloadIdentitySubjectPattern reliably; Update button disabled
  until something changes.
- CreateUserSheet — title/email/phone/password/roles all populate; Update
  disabled until field edit or password set.
- CreateServiceAccountSheet — title populates; Update disabled until
  title edit.
- CreateGroupSheet — title/description/members populate; existing
  `hasChanged` kept; Update disabled until change.

Reported by the user after testing Workload Identity edit and Service
Account edit on the branch. Type-check passes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(frontend): populate Workload Identity edit fields + migrate RequestRoleDialog to Sheet

## Workload Identity edit: Organization / Repository / Branch were empty

Root cause: `WorkloadIdentitiesPage.handleUserSelected` called the
synchronous `workloadIdentityStore.getWorkloadIdentity(user.email)`, which
returns a **stub** WorkloadIdentity (just name + email + title + state)
when the cache is empty. The list-fetch path doesn't populate the cache,
so every "Edit" click on a row returned a stub — no `workloadIdentityConfig`,
no `subjectPattern`, so `parseWorkloadIdentitySubjectPattern` returned
undefined and the inner form's `initialOwner/initialRepo/initialBranch`
all defaulted to "".

Downstream, the dirty-tracking logic correctly identified the form as
"not dirty" (state matched initial values), which is *why* the Update
button was also grayed out — the state was self-consistent but based on
an empty baseline.

Fix: swap `getWorkloadIdentity` for `getOrFetchWorkloadIdentity` and make
`handleUserSelected` async, so the full WorkloadIdentity (with config) is
loaded before `setEditingWI` is called. `ProjectGitOpsPage` already had a
prior fetch-into-cache workaround in its display path (see line 148), but
the edit flow in `WorkloadIdentitiesPage` was missing the equivalent fetch.

## RequestRoleDialog → RequestRoleSheet

Original motivator for this whole thread — the user's first screenshot was
the Custom Approval Rule dialog, then the same argument was extended to
other multi-field edit forms including RequestRole. `RequestRoleDialog`
was a `<Dialog>` containing 7+ conditionally-rendered sections (role
picker, reason textarea, database scope with ALL/SELECT/EXPRESSION mode +
CEL builder, environment picker, expiration picker, issue labels). Too
complex for a centered modal — converted to `<Sheet width="standard">`
using the same outer-wrapper + inner-form + key pattern as the other
edit sheets, to guarantee fresh state per open.

The Vue Dialog imports are now unused in `MembersPage.tsx` and were removed.

Type-check passes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(frontend): row-click → detail sheet + update AGENTS.md + CreateRevisionSheet

## Row-click opens detail (standardize across resource list pages)

Replaces the inconsistent mix of "Edit" (pencil) and "View" (eye) icon
buttons in the Operations column with a single pattern:

- The whole **row is clickable** to open the detail Sheet
- Row gets `cursor-pointer hover:bg-control-bg` as the affordance
- Operations column holds only **destructive/secondary actions** (Delete,
  Restore) — each with `e.stopPropagation()` so they don't also trigger
  row-open
- The "primary accent" color (`text-accent`) on the entity title is
  dropped in favor of `text-main` — the row hover state is the
  clickability signal, not a blue title

Whether the Sheet lets you edit or only view is a function of the user's
permissions (the form already disables inputs / hides Save when the user
lacks update permission). The entry point no longer promises "view" or
"edit" — both labels were half-truths on pages where a single row might
be editable for some users and read-only for others.

Applied to:
- `WorkloadIdentitiesPage` — removed Pencil edit button
- `UsersPage` — removed Eye/Pencil icon (whichever was shown per account
  type), kept Delete
- `GroupsPage` — removed Pencil edit button from GroupRow, kept Delete;
  the expand/collapse chevron button gets `e.stopPropagation()` so it
  toggles without also opening detail
- `ServiceAccountsPage` — removed Pencil edit button, kept Delete

## frontend/AGENTS.md: new Dialog vs Sheet guideline

Added a new section under `## shadcn Component Guidelines → Rules`
covering:

- When to use `<Sheet>` vs `<Dialog>` vs `<AlertDialog>` (creating/editing
  → Sheet; confirmation/prompt/read-only result → Dialog)
- Sheet width tiers (narrow / standard / wide) with concrete examples of
  when to pick each
- The outer-wrapper + inner-form + `key` pattern for edit sheets, with a
  code example, so future edit sheets populate reliably on every open
- The dirty-tracking rule (`allowConfirm = isFormValid && isDirty`) for
  edit sheets
- The fetch-full-entity-before-open rule to avoid the stub WI bug

## CreateRevisionDialog → CreateRevisionSheet

Complete the wrong-Dialog audit by migrating the last flagged offender.
Currently orphaned (not yet imported by DatabaseRevisionPanel which still
uses the Vue version), but in-place and ready for when that panel migrates.

Lint, type-check, 1031 unit tests all green.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(frontend): address review feedback — a11y, close animation, sheet primitive polish

Follow-up fixes from the self-review on #19962:

## Row-click keyboard accessibility (blocking a11y regression)

The new row-click → detail Sheet pattern in WorkloadIdentities / Users /
Groups / ServiceAccounts had no keyboard support — mouse users could click
the row to open the sheet but keyboard users had no way in. The old
Pencil/Eye `<Button>` handled this for free; the refactor dropped it.

Added to each clickable row:
- `tabIndex={0}` when the row is openable
- `role="button"` so screen readers announce it as actionable
- `aria-label` set to the entity's display title
- `onKeyDown` handling Enter and Space (with `preventDefault` on Space to
  avoid page scroll)
- `focus-visible:bg-control-bg` so the focused row has a visible outline

## Blank sheet during close animation (cosmetic bug)

The `{open && <InnerForm ... />}` guard inside `SheetContent` unmounted
the inner form the instant `open` flipped to false — but Base UI's Dialog
Portal keeps the Popup in the DOM for ~200ms during the close transition,
so users saw a blank sheet sliding off-screen.

Replaced with a **stable-entity ref** pattern in all five edit sheet
wrappers (CreateWorkloadIdentitySheet, CreateUserSheet,
CreateServiceAccountSheet, CreateGroupSheet, plus RequestRoleSheet for
consistency):

```tsx
const openEntityRef = useRef(user);
if (open) {
  openEntityRef.current = user;
}
const stableUser = openEntityRef.current;

return (
  <Sheet open={open} ...>
    <SheetContent>
      <UserForm key={stableUser?.name ?? "new"} user={stableUser} ... />
    </SheetContent>
  </Sheet>
);
```

The ref freezes the last-open entity while `open === false`, so the form
continues to render the previous entity's data through the close
animation. Base UI's Dialog.Portal unmounts the Popup after the animation
completes, at which point the form unmounts with it; a subsequent open
remounts it fresh (with useState initializers reading the then-current
prop).

The `{open && ...}` guard is gone — Base UI's natural
mount/unmount-around-animation handles lifecycle correctly.

## SheetHeader / SheetContent layout fragility

The previous design had the close button absolute-positioned at
`top-4 right-4` of `SheetContent`, with `SheetHeader` padding-compensating
via `pr-12` to avoid visual overlap. Two problems:

1. Callers that override `SheetHeader`'s `className` could clobber the
   `pr-12` and cause the close button to overlap the header content.
2. The close button's position and the header's padding had to stay in
   sync in two separate places.

Refactored so the close button lives **inside** `SheetHeader` as a flex
child, with the title/description in a `flex-col` on the left and the
button flush right. `SheetContent` no longer renders the button itself.
This means consumers without a `SheetHeader` don't get a close button —
but all current consumers use one, and the pattern is the same one
shadcn's sheet.tsx ships with.

## Orphan file deletion

`CreateRevisionSheet.tsx` was noted in the PR description as "not yet
wired up" — the Vue version is still in use via DatabaseRevisionPanel.
Carrying dead code is noise. Deleted the file and removed the 5 now-unused
i18n keys (`database.revision.import-revision`,
`database.revision.invalid-version-format`,
`database.revision.type-declarative`, `database.revision.type-versioned`,
`database.revision.version-already-exists`) from all 5 locale files. When
DatabaseRevisionPanel migrates to React, the component can be re-added.

## AGENTS.md guideline correction

The code sample in the new Dialog-vs-Sheet guideline used the old
`{open && <Form />}` pattern. Updated to show the stable-entity ref
pattern that the actual code now uses, with comments explaining the
close-animation reasoning. Future contributors reading the doc get the
right pattern on first try.

## Minor: comment on initialRoles mount-only capture

The `initialRoles` useMemo in `UsersPage.tsx` has an empty deps array
intentionally — we want the baseline frozen at mount so dirty tracking
compares against a stable reference. Added a comment explaining the
rationale so a future reader doesn't "fix" it by adding `userMapToRoles`
to deps (which would clobber the baseline on async store updates).

Type-check, lint, and all 1031 unit tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(frontend): address Codex review comments + fix environment reorder drag

## Codex P2 #1: User row click hijacks group badge navigation

Clicking a group badge inside `UserGroupsCell` bubbled up to the new
`<TableRow onClick>` handler, which opened the user detail sheet *and*
navigated to the group — producing two competing route pushes.

Added `e.stopPropagation()` in the badge's onClick so badge clicks only
navigate to the group, as intended. Matches the pattern already used on
Delete/Restore buttons in the same rows.

## Codex P2 #2: Stale async selection race in WorkloadIdentitiesPage

`handleUserSelected` awaits `getOrFetchWorkloadIdentity` before calling
`setEditingWI`. If a user clicks two different rows in quick succession
on a cold cache, the slower first request can resolve *after* the second
and overwrite `editingWI` with the wrong identity.

Added a sequence-counter guard (`selectSeqRef`): each click bumps the
counter, and only the response whose captured `seq` still matches the
latest counter is allowed to update state. Stale responses are dropped.

## Environment reorder drag-and-drop was broken

Reported by the user — dragging environments in the Reorder sheet did
nothing. Two root causes:

1. **`onDragStart` never initialized `dataTransfer`.** HTML5 DnD requires
   `dataTransfer.setData()` to start a drag in Firefox, and
   `effectAllowed` + `dropEffect` for the drop cursor in Chrome. The old
   handler only called `setDragIndex(index)` on a closure that didn't
   receive the DragEvent — so dataTransfer was never touched. In some
   browsers this silently aborted the drag.

2. **Mid-drag list mutation.** The old `onDragOver` spliced `list` and
   called `setList(newList)` on *every* dragover event. Each `setList`
   re-renders the whole sheet Popup, and Chrome's drag session can abort
   when the source element re-mounts or its identity changes during the
   drag.

Rewrote with standard HTML5 DnD pattern:
- `onDragStart` sets `effectAllowed = "move"` and
  `dataTransfer.setData("text/plain", String(index))`.
- `onDragOver` calls `preventDefault()` (to mark the element as a valid
  drop target) and updates a separate `dragOverIndex` state purely for
  visual feedback (border highlight). The list itself is not mutated.
- `onDrop` reads the source index from `dataTransfer`, splices once,
  calls `setList` once.
- `onDragEnd` clears both `dragIndex` and `dragOverIndex`.

Also added a visible drop-target highlight (`border-accent bg-accent/5`)
on the hovered row so the user sees where the drop will land.

Type-check, lint, all checks pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(frontend): port Environment reorder Sheet to @dnd-kit

My previous HTML5 DnD rewrite still didn't work inside the Sheet. Root
cause: Base UI's Dialog.Popup uses a focus trap + CSS transform
animation (`data-starting-style: translate-x-full`), and native HTML5
drag-and-drop fights with both — the transform on the popup element
messes up drag-image coordinates in Chrome, and the focus trap can
intercept certain pointer events. This is a known limitation of raw
HTML5 DnD inside modal dialog primitives.

The project already depends on `@dnd-kit/core`, `@dnd-kit/sortable`,
and `@dnd-kit/utilities` (used by `CustomApproval/RulesSection.tsx`'s
rule reorder). dnd-kit uses pointer-event-based drag tracking (not
native HTML5 DnD) and works reliably inside Base UI Dialog popups.

Rewrote `ReorderSheet` + `SortableEnvironmentRow` to match the existing
pattern in `RulesSection.tsx`:

- `DndContext` with `PointerSensor` (4px activation distance to avoid
  click-vs-drag ambiguity on the grip handle) and `KeyboardSensor`
  (accessible reorder via Tab + Space + Arrow keys)
- `SortableContext` with `verticalListSortingStrategy` wrapping the
  rows
- `useSortable({ id: env.id })` per row, with the grip handle getting
  the `attributes + listeners` (not the whole row — that would conflict
  with click-to-something-else if we add other row interactions later)
- `arrayMove` on drop, matching the local-state update pattern used by
  RulesSection
- `touch-none` class on the grip handle so touch devices don't scroll
  while dragging

Bonus: `KeyboardSensor` + `sortableKeyboardCoordinates` gives us
accessible reorder for free — tab to a grip handle, press Space to
pick up, arrow keys to move, Space to drop.

Type-check + check + 1031 tests all pass.

Reported by the user after the HTML5 DnD fix still didn't work in
practice.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(frontend): stop environment reorder from resetting after each drag

The previous attempt used @dnd-kit correctly but the visual reorder kept
snapping back because the outer ReorderSheet's reset `useEffect` was
firing on every parent render and resetting `list` from the incoming
`environments` prop.

Root cause: `EnvironmentsPage` passes `environments={environmentList}`
where `environmentList = useVueState(() => [...store.environmentList])`.
`useVueState`'s getter returns a new array reference on every render
(via the spread), which means the `environments` prop is a new
reference on every parent render. The reset effect
`useEffect(() => { ... setList([...environments]) }, [open, environments])`
therefore fired after every drop (triggered by any unrelated parent
re-render), clobbering the user's reorder back to the original order.

Fix: split into outer wrapper + inner form (matching the pattern used
by the other edit sheets in this PR). The outer wrapper:

1. Tracks an `openCount` that bumps on each closed→open transition.
2. Snapshots `environments` at the moment of the open transition into
   a ref, so the starting order is captured once.
3. Keys the inner component by `openCount`, forcing a clean remount
   with a fresh `list` state whenever a new open happens. No reset
   `useEffect` needed — `useState(() => [...environments])` in the
   inner component does the initialization correctly.

The inner component:

- Captures the initial order into `initialOrderRef` at mount so
  `orderChanged` compares against the stable baseline, not the live
  `environments` prop (which keeps churning references).
- Uses functional `setList((prev) => arrayMove(prev, ...))` so the
  drag-end handler always operates on the latest state, even if
  multiple drops fire in quick succession.

This matches the stable-entity ref pattern used by
`CreateWorkloadIdentitySheet` / `CreateUserSheet` / etc. — same class
of bug (useVueState spreads → new reference every render → naive
reset effects clobber local state), same class of fix.

Reported by the user: "after I dragged the environment to the spot I
want to (swapping environments), there is no change after I released
my mouse."

Type-check, lint, all checks green.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

… tab locator

- approval-blocker: treat APPROVED as terminal in poll loop (auto-approval
  policy can skip PENDING entirely)
- byt9160: use exact-match getByText("#2") instead of broken POM specTab
  to avoid matching "Issue #..." text elsewhere on page

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

PR review caught three issues. All addressed before merge.

#3 (ship-blocker) — Line-tracking regression. The original bridge called
bare ParseTiDB and skipped the SetOriginTextPosition + per-column line
work that the canonical pre-flip path (ParseTiDBForSyntaxCheck) applies
at tidb.go:64-75. 49 of 51 tidb advisors call node.OriginTextPosition()
for advice line numbers; without the fix, post-flip un-migrated
advisors silently regress to line 1 for every statement.

Fix: extract applyTiDBLineTracking(node, baseLine, originalText) helper
from ParseTiDBForSyntaxCheck. Both the canonical path and the bridge
call it, so consumers reading OriginTextPosition() see consistent
values across both paths. Helper returns the 1-based actualStartLine
for *AST.StartPosition use.

Regression test: TestAsPingCapASTLineTrackingMatchesCanonical parses a
two-statement input via both paths and asserts the second statement's
node.OriginTextPosition() matches (both must report 2, not 1).

#1 — Godoc overpromise. Original wording claimed "review continuity
from both directions" but the bridge only handles one direction —
omni-success → pingcap-fallback. The opposite (omni rejects at
dispatcher level so no *OmniAST exists) is the dispatcher-fallback
contract, not the bridge's job. Godoc now narrows the claim and
forward-references plan §1.5.0 invariant #5 (the dispatcher-fallback
invariant being added in a follow-up plan amendment).

#2 — Logging asymmetry. Pilot side soft-fails with slog.Debug + log.
Bridge silently returned (nil, false). Now logs symmetrically on each
failure mode (parse error, zero nodes, line-tracking error). Negative
cache means a single Debug per OmniAST instance, not per call.

#4 — Strict ParseTiDB note. Godoc now explains why the bridge uses
strict ParseTiDB (no error-recovery) instead of mysql's lenient ANTLR
parse: pingcap has no equivalent error-recovery mode, and the bridge
operates on omni-validated SQL where strict parsing is correct.

#5 — Multi-statement assumption made explicit. nodes[0] used; nodes[1:]
logged as a contract violation instead of silently dropped.

Validation:
  go test -count=1 ./backend/plugin/parser/tidb/...   # green incl. new regression test
  go test -count=1 ./backend/plugin/advisor/tidb/...  # green
  go test -count=1 ./backend/plugin/advisor/mysql/... # green (cross-engine regression)
  go build ...                                        # green
  golangci-lint run ...                               # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Codex P1 catch on PR #20179.

Without this fix, post-§1.5.N+1 dispatcher flip:
  ParsedStatement.AST = *OmniAST → un-migrated advisor calls
  getTiDBNodes → tidbparser.GetTiDBAST(stmt.AST) →
  AsPingCapAST fails (pingcap rejects what omni accepted) →
  GetTiDBAST returns (nil, false) →
  getTiDBNodes returns 'AST type mismatch: expected TiDB parser result' →
  advisor.Check propagates as 'Rule check failed' →
  every un-migrated advisor breaks for that review

That breaks the migration continuity the bridge is meant to preserve.
Phase 1.5 invariant #2 says soft-fail (skip statement, review
continues), not hard-fail.

Fix: when GetTiDBAST returns false AND stmt.AST is a
PingCapASTProvider (i.e. an *OmniAST that just routed through the
bridge and missed), skip the statement instead of returning an error.
The bridge has already logged the parse failure at debug level.

True engine-mismatch (stmt.AST is some unrelated type, neither
*tidb.AST nor a PingCapASTProvider) is still surfaced as an error —
that's a programmer error in registration, not a soft-fail case.

Regression test:
  TestGetTiDBNodesSoftFailsOnBridgeMiss feeds a context with two
  ParsedStatements: one *OmniAST whose AsPingCapAST will fail
  ('SELECT FROM WHERE;') bracketed by one that succeeds ('SELECT 1').
  Expects no error and len(got) == 1 (the successful one).

Sanity-checked: GetTiDBAST returns false on a pingcap-invalid OmniAST,
confirming the pre-fix path would have errored.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green incl. new test
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…0179)

* feat(tidb): add AsPingCapAST bridge on OmniAST (Phase 1.5 §1.5.2)

Mirror of mysql/OmniAST.AsANTLRAST for the Phase 1.5 advisor migration.
Required prerequisite for the eventual dispatcher flip (§1.5.N+1):
once the registered ParseStatementsFunc is flipped to omni and
*OmniAST values reach the ~50 un-migrated tidb advisors at any point
in the migration, those advisors keep working because GetTiDBAST
falls through to AsPingCapAST() automatically.

Implementation mirrors mysql/omni.go:39-55:
  - Lazy parse: native pingcap parse runs once on first AsPingCapAST
    call per OmniAST instance.
  - Cached result: pingcapParsed bool flag distinguishes "not yet
    parsed" from "parsed and got nil"; cache hits skip the work.
  - Negative caching: pingcap parse failure also short-circuits future
    calls so unparseable-by-pingcap SQL doesn't re-parse per advisor.

backend/plugin/parser/tidb/ast.go:
  - Adds PingCapASTProvider interface (analog of base.AntlrASTProvider).
  - GetTiDBAST extended to check for the provider, mirroring
    base.GetANTLRAST's pattern.

backend/plugin/parser/tidb/omni.go:
  - OmniAST gains pingcapAST + pingcapParsed unexported fields.
  - AsPingCapAST method implements the provider contract.
  - Godoc updated: removes the "neither fallback" rationale (correct
    for PR #20107's scope but inverted by the migration plan), points
    at plan §1.5.0 invariant #4 + bridge persistence rule for context.

Tests:
  - TestAsPingCapASTReturnsNativeAST: bridge contract for valid SQL.
  - TestAsPingCapASTCachesAcrossCalls: a.Same() pointer equality on
    repeat calls proves cache hit, not fresh re-parse.
  - TestAsPingCapASTCachesNegativeResult: parse failure cached too.
  - TestGetTiDBASTFallsBackToProvider: end-to-end bridge through
    GetTiDBAST — the path un-migrated advisors will use.

Bridge persistence: the bridge stays in the codebase indefinitely
post-Phase-1.5. The one Class III advisor (dml_dry_run, blocked on
omni's NonTransactionalDMLStmt grammar — Phase 2 §Tier 4g) requires
this fallback. Bridge cleanup is gated on dml_dry_run unblocking, not
on Phase 1.5 completion.

Validation:
  go test -count=1 ./backend/plugin/parser/tidb/...    # green (incl. 4 new bridge tests)
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green (no cross-engine regression)
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tidb): bridge line-tracking + symmetric logging + scoped godoc

PR review caught three issues. All addressed before merge.

#3 (ship-blocker) — Line-tracking regression. The original bridge called
bare ParseTiDB and skipped the SetOriginTextPosition + per-column line
work that the canonical pre-flip path (ParseTiDBForSyntaxCheck) applies
at tidb.go:64-75. 49 of 51 tidb advisors call node.OriginTextPosition()
for advice line numbers; without the fix, post-flip un-migrated
advisors silently regress to line 1 for every statement.

Fix: extract applyTiDBLineTracking(node, baseLine, originalText) helper
from ParseTiDBForSyntaxCheck. Both the canonical path and the bridge
call it, so consumers reading OriginTextPosition() see consistent
values across both paths. Helper returns the 1-based actualStartLine
for *AST.StartPosition use.

Regression test: TestAsPingCapASTLineTrackingMatchesCanonical parses a
two-statement input via both paths and asserts the second statement's
node.OriginTextPosition() matches (both must report 2, not 1).

#1 — Godoc overpromise. Original wording claimed "review continuity
from both directions" but the bridge only handles one direction —
omni-success → pingcap-fallback. The opposite (omni rejects at
dispatcher level so no *OmniAST exists) is the dispatcher-fallback
contract, not the bridge's job. Godoc now narrows the claim and
forward-references plan §1.5.0 invariant #5 (the dispatcher-fallback
invariant being added in a follow-up plan amendment).

#2 — Logging asymmetry. Pilot side soft-fails with slog.Debug + log.
Bridge silently returned (nil, false). Now logs symmetrically on each
failure mode (parse error, zero nodes, line-tracking error). Negative
cache means a single Debug per OmniAST instance, not per call.

#4 — Strict ParseTiDB note. Godoc now explains why the bridge uses
strict ParseTiDB (no error-recovery) instead of mysql's lenient ANTLR
parse: pingcap has no equivalent error-recovery mode, and the bridge
operates on omni-validated SQL where strict parsing is correct.

#5 — Multi-statement assumption made explicit. nodes[0] used; nodes[1:]
logged as a contract violation instead of silently dropped.

Validation:
  go test -count=1 ./backend/plugin/parser/tidb/...   # green incl. new regression test
  go test -count=1 ./backend/plugin/advisor/tidb/...  # green
  go test -count=1 ./backend/plugin/advisor/mysql/... # green (cross-engine regression)
  go build ...                                        # green
  golangci-lint run ...                               # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tidb): correct godoc reference to plan invariant #8

Numbering drift caught in review: the godoc cited "§1.5.0 invariant #5"
but the landed plan invariant is #8 (the dispatcher-fallback contract).
The §5 numbering came from an earlier draft of the plan amendment;
final landed numbering is #8 because three additional invariants (#5
no-mutation, #6 recipe-default, #7 shape-audit) had already accreted.

One-line fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): soft-fail getTiDBNodes on bridge miss

Codex P1 catch on PR #20179.

Without this fix, post-§1.5.N+1 dispatcher flip:
  ParsedStatement.AST = *OmniAST → un-migrated advisor calls
  getTiDBNodes → tidbparser.GetTiDBAST(stmt.AST) →
  AsPingCapAST fails (pingcap rejects what omni accepted) →
  GetTiDBAST returns (nil, false) →
  getTiDBNodes returns 'AST type mismatch: expected TiDB parser result' →
  advisor.Check propagates as 'Rule check failed' →
  every un-migrated advisor breaks for that review

That breaks the migration continuity the bridge is meant to preserve.
Phase 1.5 invariant #2 says soft-fail (skip statement, review
continues), not hard-fail.

Fix: when GetTiDBAST returns false AND stmt.AST is a
PingCapASTProvider (i.e. an *OmniAST that just routed through the
bridge and missed), skip the statement instead of returning an error.
The bridge has already logged the parse failure at debug level.

True engine-mismatch (stmt.AST is some unrelated type, neither
*tidb.AST nor a PingCapASTProvider) is still surfaced as an error —
that's a programmer error in registration, not a soft-fail case.

Regression test:
  TestGetTiDBNodesSoftFailsOnBridgeMiss feeds a context with two
  ParsedStatements: one *OmniAST whose AsPingCapAST will fail
  ('SELECT FROM WHERE;') bracketed by one that succeeds ('SELECT 1').
  Expects no error and len(got) == 1 (the successful one).

Sanity-checked: GetTiDBAST returns false on a pingcap-invalid OmniAST,
confirming the pre-fix path would have errored.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green incl. new test
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Per code review on PR #20198, six issues addressed:

#1 Acknowledge silent bug fix in migration_compatibility.go.
   The original pingcap-typed code at lines 104-106 of the pre-
   migration file checked ConstraintUniq + ConstraintUniqKey but
   missed ConstraintUniqIndex, so `ALTER TABLE t ADD UNIQUE INDEX
   idx (col)` previously emitted no compatibility advice. omni
   unifies all three under ConstrUnique, so the migration silently
   started flagging the missed case.
   Side-effect: ADD UNIQUE INDEX now correctly flags as
   compatibility-affecting (was a missed case in original code).
   Documented inline at the ConstrUnique branch.

#2 Remove dead cmd.NewName fallback in advisor_table_require_pk.go's
   ATChangeColumn case. Per omni contract (parsenodes.go:224),
   NewName is for RENAME ops, NOT ATChangeColumn — for ChangeColumn
   the new name is in cmd.Column.Name. The fallback either never
   fired (dead) or fired on malformed input (wrong). Now reads
   cmd.Column.Name only, with comment documenting the contract.

#3 Move addColumnTargets to utils.go. Was defined in
   advisor_table_require_pk.go but consumed by advisor_index_pk_type.go
   — same cross-file coupling pattern that the pre-batch checklist is
   designed to catch (and that we caught for tableNewColumn). Now
   shared from utils.go alongside other omni-typed helpers.

#4 Acknowledge graceful-skip behavior change in constraintPKColumns.
   The original pingcap-typed convertConstraintToKeySlice did
   `key.Column.Name.String()` without nil-checking key.Column. For
   `PRIMARY KEY ((expr))` syntax pingcap left key.Column nil and
   populated key.Expr instead, so the original would panic with a
   nil-pointer dereference on functional PKs. The migrated version
   returns an empty slice (graceful skip) for expression PKs.
   Documented inline.

#5 Narrow formatColumnType godoc. The previous comment claimed full
   pingcap tp.String() reproduction but the helper only handles
   types with Length/Scale; ENUM/SET (value lists), DECIMAL with
   implicit Scale, BIT, etc. would diverge. Updated godoc to state
   the contract is "fixture stability for INDEX_PK_TYPE_LIMIT" not
   full pingcap parity, with a note to extend when new fixture
   cases appear.

#6 Consolidate locStart into checkStmt arms. Previous structure
   had two switches covering the same node types — adding a node
   type to one without the other would silently degrade line
   numbers to default (0 → line 1). Per peer recommendation, line
   capture moves into the same arm as the code classification, so
   the trap is structurally impossible. Slight verbosity for
   guaranteed sync.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…overage

Round-1 review feedback on PR #20199.

#1 Line-number parity for no_select_all. The pre-migration pingcap
advisor set checker.line once per top-level statement, so every
wildcard hit (including nested-subquery hits) reported the OUTER
statement's line. The omni Recipe B port emitted per-match — using
the inner SelectStmt's Loc — which was an arguably-better behavioral
change but unacknowledged.

Reverted to outer-statement line for migration discipline: capture
ostmt.AbsoluteLine(0) once per statement and reuse for all advices.
Improving advice line precision is a follow-up decision, not a
side-effect of parser migration.

#2 UNION fixture coverage. omni's Walk recurses into SelectStmt.Left
and .Right (verified in walk_generated.go:756-762), so Recipe B
correctly visits UNION arm subqueries. Added three fixture cases to
pin the contract:

  - SELECT * FROM t1 UNION SELECT a FROM t2  → flagged (left arm)
  - SELECT a FROM t1 UNION SELECT * FROM t2  → flagged (right arm)
  - SELECT a FROM t1 UNION SELECT b FROM t2  → no advice (control)

#2 (cont) insertUsesLimit comment. Added documentation that only
the outermost LIMIT is checked — a LIMIT inside a parenthesized
UNION arm lives on Left.Limit, not on the outer .Limit, and was
not flagged by the pre-migration advisor either. Pingcap parity
preserved, behavior intentionally not extended.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…INTEGER fixture

Codex round-2 review on PR #20198. Two findings:

#1 INTEGER alias (NOT VALID).
Codex claimed CREATE TABLE t(id INTEGER PRIMARY KEY) would render the
type as "integer" (lowercase) and false-positive the rule. Empirically
omni's parser already normalizes INTEGER → DataType.Name = "INT" at
parse time, so formatColumnType correctly returns "INT" and the
column passes the INT/BIGINT check. Verified by adding two fixture
cases (INTEGER inline and table-level PK); both produce no advice.
No code change for this finding — the fixture pins the contract so
any future omni grammar change that breaks the alias surfaces.

#2 Empty cmd.Name on DROP PRIMARY KEY (DEFENSIVE FIX).
Codex claimed omni AST can represent DROP PRIMARY KEY with cmd.Name
== "" and that the mysql omni rule already guards this. Empirically
current omni grammar (both mysql and tidb) produces cmd.Name ==
"PRIMARY" for ALTER TABLE t DROP PRIMARY KEY — the regression Codex
described doesn't reproduce.

However, mysql/rule_table_require_pk.go has a defensive guard
strings.EqualFold(cmd.Name, primaryKeyName) || cmd.Name == "" with
comment "cmd.Name == '' when it's DROP PRIMARY KEY (no explicit
constraint name)". Either an older omni version produced empty name
or the guard is pure future-proofing. Adopting the same defensive
posture for tidb maintains cross-engine parity at no real cost.
Existing fixture covers DROP PRIMARY KEY (cmd.Name == "PRIMARY"
case) so non-empty path is already pinned; the empty-name path is
unreachable today but the guard is in place if grammar ever changes.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…AST (Phase 1.5 §1.5.3 batch 3) (#20199)

* refactor(advisor/tidb): migrate 4 STATEMENT_DISALLOW_* rules to omni AST (Phase 1.5 §1.5.3 batch 3)

First Class IIa batch — advisors that consume both AST nodes AND the
statement's source text for advice content. Four rules migrated:

  - advisor_stmt_disallow_commit.go    (STATEMENT_DISALLOW_COMMIT)
  - advisor_stmt_no_select_all.go      (STATEMENT_SELECT_NO_SELECT_ALL)
  - advisor_stmt_disallow_order_by.go  (STATEMENT_DISALLOW_ORDER_BY)
  - advisor_stmt_disallow_limit.go     (STATEMENT_DISALLOW_LIMIT)

Three use Recipe A (top-level type switch). One — no_select_all —
uses Recipe B (omniast.Walk) because the wildcard can appear in
nested subqueries; the existing fixture exercises this with
'SELECT a FROM (SELECT * FROM t1 JOIN t2) t' and a top-level switch
silently skips the inner SELECT.

Migration progress: 14 of 51 advisors on omni AST.

New shared helper:
  - OmniStmt.TrimmedText() in utils.go — returns Text with surrounding
    whitespace removed, for embedding into advice content. Multi-
    statement input passes leading newlines through Statement.Text;
    pingcap's node.Text() returned the trimmed form.

Notable mappings:
  pingcap UpdateStmt/DeleteStmt   omni
    Order != nil          →       len(OrderBy) > 0
    Limit != nil          →       Limit != nil
  pingcap InsertStmt              omni
    Select interface (SelectStmt | SetOprStmt) → SelectStmt with SetOp field;
                                    omni unifies both forms, single branch covers them.
  pingcap SelectStmt              omni
    Fields.Fields[].WildCard →    *StarExpr in TargetList[]
  pingcap CommitStmt              omni
    *ast.CommitStmt        →      *ast.CommitStmt (same)

Pre-batch checklist:
  1. Coupling scan: clean — useLimit was local to disallow_limit.go;
     inlined as insertUsesLimit during migration.
  2. Shape audit: pingcap *ast.SetOprStmt collapses to omni's
     *SelectStmt with SetOp field (similar to migration_compatibility's
     ConstrUnique unification). Fixture exercises the inner-subquery
     wildcard case which forced Recipe B for no_select_all.
  3. Yaml fixture coverage: TestTiDBRules green for all 4 rules.
  4. Pingcap-import quarantine: clean for migrated files.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): no_select_all line-number parity + UNION fixture coverage

Round-1 review feedback on PR #20199.

#1 Line-number parity for no_select_all. The pre-migration pingcap
advisor set checker.line once per top-level statement, so every
wildcard hit (including nested-subquery hits) reported the OUTER
statement's line. The omni Recipe B port emitted per-match — using
the inner SelectStmt's Loc — which was an arguably-better behavioral
change but unacknowledged.

Reverted to outer-statement line for migration discipline: capture
ostmt.AbsoluteLine(0) once per statement and reuse for all advices.
Improving advice line precision is a follow-up decision, not a
side-effect of parser migration.

#2 UNION fixture coverage. omni's Walk recurses into SelectStmt.Left
and .Right (verified in walk_generated.go:756-762), so Recipe B
correctly visits UNION arm subqueries. Added three fixture cases to
pin the contract:

  - SELECT * FROM t1 UNION SELECT a FROM t2  → flagged (left arm)
  - SELECT a FROM t1 UNION SELECT * FROM t2  → flagged (right arm)
  - SELECT a FROM t1 UNION SELECT b FROM t2  → no advice (control)

#2 (cont) insertUsesLimit comment. Added documentation that only
the outermost LIMIT is checked — a LIMIT inside a parenthesized
UNION arm lives on Left.Limit, not on the outer .Limit, and was
not flagged by the pre-migration advisor either. Pingcap parity
preserved, behavior intentionally not extended.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): batch-3 line-number parity for leading-comment inputs

Codex P2: 4 findings of line-number regression for inputs with leading
trivia (whitespace + comments). Empirically verified:

  Input: "\n-- cmt\nSTMT;"
    pingcap (main):    line 2
    omni (this PR):    line 1   ← regression

Codex's claimed direction was reversed for 3 of the 4 cases (claimed
line 3, actual line 1) but the regression itself is real for all 4.

Root cause: pingcap's lexer strips leading whitespace but keeps
comments in node.Text(), so its OriginTextPosition points at the line
of the first comment OR keyword. Omni's Loc.Start is 0 (start of
segment) for top-level statements; AbsoluteLine(0) gives BaseLine+1.

Fix: new OmniStmt.FirstTokenLine() helper finds the line of the first
non-whitespace character in s.Text. Matches pingcap behavior for the
common cases:

  | Input                            | pingcap | FirstTokenLine |
  |----------------------------------|---------|----------------|
  | "COMMIT;"                        | 1       | 1              |
  | "\n-- cmt\nCOMMIT;"              | 2       | 2              |
  | "\n/* block */\nCOMMIT;"         | 2       | 2              |

One edge case diverges: "   \n  COMMIT;" (leading whitespace, no
comments) — pingcap reports 1, FirstTokenLine reports 2. pingcap's
lexer preserves whitespace-only leading text in node.Text() so its
leadingNewlinesStripped is 0 here. Real SQL rarely has this shape;
both reported lines reasonably point at the statement.

All 4 batch-3 advisors (commit, no_select_all, disallow_order_by,
disallow_limit) updated to use FirstTokenLine() instead of
AbsoluteLine(0) or AbsoluteLine(n.Loc.Start).

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Empirical line-parity test across 6 input shapes: 5/6 match pingcap;
the 1 mismatch is the contrived leading-whitespace-only case
documented above.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): no_select_all detects qualified wildcards (t.*)

Codex P2: targetListHasStar only matched *ast.StarExpr, missing the
qualified-wildcard form. Empirically verified omni's TargetList shape:

  SELECT *      → *ast.StarExpr
  SELECT t.*    → *ast.ColumnRef with Star: true (NOT a StarExpr)

Pre-migration pingcap path used field.WildCard != nil, which was set
for both bare and qualified wildcards. omni splits them into two node
types; targetListHasStar must check both.

Fix: extend the type switch to also match *ast.ColumnRef with
Star == true.

Sanity-checked by reverting: TestTiDBRules fails on the new
"SELECT t.* FROM t" fixture case; restored fix passes.

Fixture additions:
  - SELECT t.* FROM t          → flagged (qualified)
  - SELECT a, t.* FROM t       → flagged (mixed with column)
  - SELECT a, t.b FROM t       → no advice (control: qualified non-star)

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… AST (Phase 1.5 §1.5.3 batch 4) (#20204)

* refactor(advisor/tidb): migrate naming index/UK/FK convention to omni AST (Phase 1.5 §1.5.3 batch 4)

Migrate 3 advisors to omni AST via Recipe A (top-level type-switch):
- advisor_naming_index_convention.go
- advisor_naming_unique_key_convention.go
- advisor_naming_foreign_key_convention.go

Pre-batch coupling scan promoted 2 cross-file helpers to utils.go:
- getTemplateRegexp (function)
- indexMetaData (struct)

Both are AST-agnostic Go (no pingcap types in their signatures), so the
move is clean. No Linear cleanup ticket needed (unlike canNull/BYT-9362
or tableNewColumn/BYT-9395, which stayed pingcap-typed for un-migrated
consumers).

Also added OmniStmt.TrimmedText() and OmniStmt.FirstTokenLine() helpers
to utils.go. Same helpers also defined on PR #20199 (batch 3); whichever
batch merges second drops the duplicate definition (trivial conflict).

NEW shape divergence caught and pinned:

omni splits ALTER TABLE ADD INDEX (ATAddIndex) vs ADD CONSTRAINT name
INDEX (ATAddConstraint) where pingcap collapsed both into
AlterTableAddConstraint. Same split applies to UNIQUE: ADD UNIQUE INDEX
goes to ATAddIndex, ADD CONSTRAINT name UNIQUE goes to ATAddConstraint.
Switch arms now cover both, mirroring mysql omni rule.

Fixture pins for the discovered divergence:
- naming_index_idx.yaml: ALTER TABLE ADD CONSTRAINT name INDEX
  (pass + violation pair).
- naming_index_uk.yaml: ALTER TABLE ADD UNIQUE INDEX, ALTER TABLE ADD
  UNIQUE KEY (4 cases: 2 forms x pass+violation each). These also pin
  the existing ConstrUnique unification (cumulative table item #2).

Behavior preserved (NOT aligned with mysql divergence):

CREATE FULLTEXT/SPATIAL INDEX statements continue to be checked by the
index naming convention advisor (matching pingcap behavior). The mysql
omni rule excludes them; aligning tidb with mysql on this is a separate
intentional behavior change deferred from this AST migration.

Migration progress: 17 of 51 advisors on omni AST.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): extract runNamingConventionRule helper to dedup naming trio

Sonar flagged 21.9% duplication across the 3 new naming-convention
advisors (33.6% / 32.2% / 11.0% per file). The duplication was real:
~140 LOC of structural overlap across the trio in the Check() preamble
(payload validation, template-token check, maxLength default) and the
advice-emission loop (regex match + length overflow advices).

Extracted runNamingConventionRule(checkCtx, cfg, collect) in utils.go.
Each advisor's Check() drops to ~17 lines: a config struct (mismatch
code, type noun, internal-error title) plus a closure that maps statement
nodes to []*indexMetaData. Collector functions (collect{Index,UK,FK}*)
unchanged — those carry the rule-specific logic that genuinely differs.

Net LOC: +93 helper / -167 dedup = -102 LOC. Sonar duplication on the
trio should drop to ~0%.

Mysql analogs left similar duplication inline; that precedent pre-dates
Sonar duplication gating on new code. Since the trio is new code, the
gate applies; the cumulative cost — naming_table, naming_column,
naming_auto_increment_column are queued in the handoff doc and would
each replicate the same ~50 LOC scaffold without the helper — justifies
the extraction now.

Verification:
- TestTiDBRules green — advice content is byte-identical to the
  pre-extraction inline form (helper builds the same "<noun> in table
  ..." and "<noun> `<name>` in table ..." strings via fmt.Sprintf with
  cfg.typeNoun substitution). Existing fixtures are the safety net.
- go test ./backend/plugin/advisor/tidb/ ./backend/plugin/advisor/mysql/
  green (cross-engine non-regression).
- golangci-lint 0 issues.
- Server build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): guard nil constraints in collectFKCreateTable

Codex P2 round-1 catch on PR #20204. The line argument
ostmt.AbsoluteLine(constraint.Loc.Start) was evaluated BEFORE
buildFKMetaData could nil-check, so a nil entry in n.Constraints would
panic with a nil pointer dereference instead of being skipped. The
helper's internal `if constraint == nil` was vestigial because of Go's
argument-evaluation order.

Sibling collectIndexCreateTable / collectUKCreateTable already
nil-checked at loop top; collectFKCreateTable was the outlier.
collectFKAlterTable was safe by accident — it precomputes stmtLine
outside the loop.

Fix: nil-check at loop top, mirroring siblings exactly.

Added TestCollectFKCreateTableNilConstraintGuard with a synthetic AST
containing a nil constraint slot. Omni's parser doesn't produce nils
from valid SQL so no YAML fixture can reach this branch — the unit test
pins the contract for future refactors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin internal-error advice path + clarifying comments

Code review pushback on PR #20204 — the runNamingConventionRule
extraction's byte-identity claim only stood for the regex-mismatch and
length-overflow paths the YAML fixtures exercise. The internal-error
advice path (fires when getTemplateRegexp's regexp.Compile fails on the
post-substitution template) had no test coverage; an accidental swap of
cfg.internalErrorTitle vs cfg.typeNoun, or a regression in Status/Code
fields, would go unnoticed.

Added TestRunNamingConventionRuleEmitsInternalErrorAdvice — uses an
unbalanced-paren format that passes payload-token validation but fails
regexp.Compile after no-op substitution. Asserts the advice's Title
comes from cfg.internalErrorTitle (not cfg.typeNoun or rule.Type),
Code is code.Internal, Status mirrors the rule level, and Content shape
matches the pre-extraction inline form.

Also added clarifying comments per the same review:
- runNamingConventionRule docstring: indexMetaData coupling tradeoff
  (4-field LCD shared by all callers; future divergence may require
  widening or replacing the closure shape).
- omniIndexColumns docstring: silent skip of expression-based parts
  (functional indexes contribute no name to column-list substitution;
  matches pingcap behavior).
- Index/UK Check methods: explain the closure-capture pattern for
  originalMetadata (keeps helper signature uniform across rules that
  don't need metadata).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…1.5.3 batch 2) (#20198)

* refactor(advisor/tidb): migrate 5 more rules to omni AST (Phase 1.5 §1.5.3 batch 2)

Class I batch following the recipe established in PR #20140 (pilot) and
extended in PR #20195 (batch 1).

Migrated:
  - advisor_database_drop_empty_db.go (DATABASE_DROP_EMPTY_DATABASE)
  - advisor_table_no_fk.go            (TABLE_NO_FOREIGN_KEY)
  - advisor_migration_compatibility.go (SCHEMA_BACKWARD_COMPATIBILITY)
  - advisor_table_require_pk.go       (TABLE_REQUIRE_PK)
  - advisor_index_pk_type.go          (INDEX_PK_TYPE_LIMIT)

Pre-batch checklist:
  1. Coupling scan: caught tableNewColumn / columnNameToColumnDef shared
     across 3 files (advisor_index_pk_type.go defines, +2 consumers).
     Moved pingcap-typed versions to utils.go for un-migrated consumers
     (advisor_index_primary_key_type_allowlist.go,
      advisor_index_type_no_blob.go); migrated advisor uses omni-typed
     siblings (tableNewOmniColumn / omniColumnNameToColumnDef).
     Cleanup tracked: BYT-9395.
  2. Shape audit: compared each against analogous mysql/rule_*.go omni
     rule. migration_compatibility surfaced one divergence — pingcap had
     three separate ConstraintUniq/UniqKey/UniqIndex types that all
     produce the same advice; omni unifies them under ConstrUnique.
     Code consolidates the three branches into one. PRIMARY KEY drop is
     also unified — pingcap had AlterTableDropPrimaryKey; omni routes
     through ATDropConstraint with cmd.Name == "PRIMARY".
  3. Yaml fixture coverage: TestTiDBRules green for all 5 rules.
  4. Pingcap-import quarantine: clean for the 5 migrated files.

Notable mappings used:
  ast.AlterTableDropPrimaryKey → ATDropConstraint with cmd.Name == "PRIMARY"
  ast.AlterTableDropIndex      → ATDropIndex (cmd.Name)
  ast.AlterTableAlterCheck     → ATAlterCheckEnforced
  ast.ConstraintUniq/UniqKey   → ConstrUnique (single)
  spec.OldColumnName.Name      → cmd.Name (in ATChangeColumn / ATDropColumn)
  node.ReferTable              → n.Like
  node.KeyType == IndexKeyTypeUnique → n.Unique (CreateIndexStmt)
  column.Tp (FieldType)        → column.TypeName (DataType: Name+Length+Scale)

advisor_index_pk_type's formatColumnType helper reproduces pingcap's
mixed casing convention (INT/BIGINT uppercase; everything else lowercase
with optional length/scale) so YAML fixture content like "varchar(5)"
remains stable.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green (TestTiDBRules covers all 5 rules)
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green (cross-engine)
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Migration progress: 10 of 51 advisors on omni AST.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): address review feedback round 1

Per code review on PR #20198, six issues addressed:

#1 Acknowledge silent bug fix in migration_compatibility.go.
   The original pingcap-typed code at lines 104-106 of the pre-
   migration file checked ConstraintUniq + ConstraintUniqKey but
   missed ConstraintUniqIndex, so `ALTER TABLE t ADD UNIQUE INDEX
   idx (col)` previously emitted no compatibility advice. omni
   unifies all three under ConstrUnique, so the migration silently
   started flagging the missed case.
   Side-effect: ADD UNIQUE INDEX now correctly flags as
   compatibility-affecting (was a missed case in original code).
   Documented inline at the ConstrUnique branch.

#2 Remove dead cmd.NewName fallback in advisor_table_require_pk.go's
   ATChangeColumn case. Per omni contract (parsenodes.go:224),
   NewName is for RENAME ops, NOT ATChangeColumn — for ChangeColumn
   the new name is in cmd.Column.Name. The fallback either never
   fired (dead) or fired on malformed input (wrong). Now reads
   cmd.Column.Name only, with comment documenting the contract.

#3 Move addColumnTargets to utils.go. Was defined in
   advisor_table_require_pk.go but consumed by advisor_index_pk_type.go
   — same cross-file coupling pattern that the pre-batch checklist is
   designed to catch (and that we caught for tableNewColumn). Now
   shared from utils.go alongside other omni-typed helpers.

#4 Acknowledge graceful-skip behavior change in constraintPKColumns.
   The original pingcap-typed convertConstraintToKeySlice did
   `key.Column.Name.String()` without nil-checking key.Column. For
   `PRIMARY KEY ((expr))` syntax pingcap left key.Column nil and
   populated key.Expr instead, so the original would panic with a
   nil-pointer dereference on functional PKs. The migrated version
   returns an empty slice (graceful skip) for expression PKs.
   Documented inline.

#5 Narrow formatColumnType godoc. The previous comment claimed full
   pingcap tp.String() reproduction but the helper only handles
   types with Length/Scale; ENUM/SET (value lists), DECIMAL with
   implicit Scale, BIT, etc. would diverge. Updated godoc to state
   the contract is "fixture stability for INDEX_PK_TYPE_LIMIT" not
   full pingcap parity, with a note to extend when new fixture
   cases appear.

#6 Consolidate locStart into checkStmt arms. Previous structure
   had two switches covering the same node types — adding a node
   type to one without the other would silently degrade line
   numbers to default (0 → line 1). Per peer recommendation, line
   capture moves into the same arm as the code classification, so
   the trap is structurally impossible. Slight verbosity for
   guaranteed sync.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): trim comments to code-comment scope

Round-1 review feedback added narrative comments documenting silent
behavior changes (UniqIndex bug fix, expression-PK panic-fix), the
fixture-stability contract on formatColumnType, and the locStart
synchronization rationale. That content belongs in the PR description
and commit messages, not in code comments — future readers don't need
the historical "what pingcap used to do" framing, just what the code
does and why.

Trimmed scope to what a future maintainer needs at read time:
contracts (what fields mean, mutual exclusivity, what each branch is
checking), gotchas (why both ATDropConstraint and ATDropIndex are
handled for PRIMARY), and TODO trackers (Linear tickets for the
moved-but-still-pingcap helpers).

Removed:
  - Bug-fix narratives ("the original pingcap code at lines X-Y missed Z")
  - Migration-history framing ("Moved here from <file>...", "post-flip...")
  - Plan cross-references and "see plans/..." pointers
  - Multi-paragraph contract docs that just restate the code

Kept:
  - Field-shape contracts (cmd.Name OLD vs cmd.Column.Name NEW; omni's
    Columns/Column mutual exclusivity)
  - Non-obvious branch semantics (DROP INDEX `PRIMARY` aliases DROP
    PRIMARY KEY)
  - Linear cleanup tickets on un-migrated-helper types

Net: -95 lines of comments across 4 files. No behavior change.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): ATAlterCheckEnforced reads cmd.NewName, not cmd.Constraint

Codex P1 catch on PR #20198. The migrated migration_compatibility
advisor stopped flagging ALTER TABLE ... ALTER CHECK chk ENFORCED as
a backward-compat issue.

omni populates ATAlterCheckEnforced with cmd.NewName ("ENFORCED" /
"NOT ENFORCED") and leaves cmd.Constraint nil — verified against
omni/tidb/parser/alter_table.go. The previous check
`cmd.Constraint != nil && !cmd.Constraint.NotEnforced` always
evaluated false, so the case never emitted advice. Pre-migration
pingcap path emitted code 110 when spec.Constraint.Enforced was true.

Fix: check cmd.NewName == "ENFORCED" instead. Pingcap parity:
ENFORCED transitions get flagged, NOT ENFORCED transitions don't.

Fixture additions pin the regression:
  - ALTER TABLE t ALTER CHECK chk ENFORCED → code 110 advice
  - ALTER TABLE t ALTER CHECK chk NOT ENFORCED → no advice

Sanity-checked by reverting the fix: TestTiDBRules fails on the
ENFORCED case with the expected diff (got nil advice, want code 110)
exactly as Codex described.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): defensive empty-name guard for ATDropConstraint + INTEGER fixture

Codex round-2 review on PR #20198. Two findings:

#1 INTEGER alias (NOT VALID).
Codex claimed CREATE TABLE t(id INTEGER PRIMARY KEY) would render the
type as "integer" (lowercase) and false-positive the rule. Empirically
omni's parser already normalizes INTEGER → DataType.Name = "INT" at
parse time, so formatColumnType correctly returns "INT" and the
column passes the INT/BIGINT check. Verified by adding two fixture
cases (INTEGER inline and table-level PK); both produce no advice.
No code change for this finding — the fixture pins the contract so
any future omni grammar change that breaks the alias surfaces.

#2 Empty cmd.Name on DROP PRIMARY KEY (DEFENSIVE FIX).
Codex claimed omni AST can represent DROP PRIMARY KEY with cmd.Name
== "" and that the mysql omni rule already guards this. Empirically
current omni grammar (both mysql and tidb) produces cmd.Name ==
"PRIMARY" for ALTER TABLE t DROP PRIMARY KEY — the regression Codex
described doesn't reproduce.

However, mysql/rule_table_require_pk.go has a defensive guard
strings.EqualFold(cmd.Name, primaryKeyName) || cmd.Name == "" with
comment "cmd.Name == '' when it's DROP PRIMARY KEY (no explicit
constraint name)". Either an older omni version produced empty name
or the guard is pure future-proofing. Adopting the same defensive
posture for tidb maintains cross-engine parity at no real cost.
Existing fixture covers DROP PRIMARY KEY (cmd.Name == "PRIMARY"
case) so non-empty path is already pinned; the empty-name path is
unreachable today but the guard is in place if grammar ever changes.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): pin ENFORCED case-normalization + SonarCloud cleanup

Codex round-3 review on PR #20198 flagged `cmd.NewName == "ENFORCED"` as
case-sensitive. Empirically false: omni's tidb lexer lowercases identifiers
before keyword lookup (omni/tidb/parser/lexer.go:2267), and the ALTER CHECK
arms assign canonical literal strings to cmd.NewName ("ENFORCED" /
"NOT ENFORCED"; alter_table.go:762-765, 785-788). The advisor matches
across all user-typed cases.

Added 4 fixture cases to schema_backward_compatibility.yaml pinning the
contract:
- "ALTER CHECK chk enforced" (lowercase) -> flagged
- "ALTER CHECK chk Enforced" (mixed) -> flagged
- "ALTER CHECK chk not enforced" (lowercase) -> not flagged
- "ALTER CHECK chk Not Enforced" (mixed) -> not flagged

If omni ever stops normalizing, the lowercase/mixed cases will start
regressing -- empirical pin per Phase 1.5 invariant #9.

SonarCloud cleanup (8 findings):
- S1192 critical: extracted "cannot find the type of \`%s\`.\`%s\`" to
  errCannotFindColumnTypeFmt const in advisor_index_pk_type.go (was
  duplicated 3x).
- S8209 minor (x7): grouped consecutive same-type Go params in
  advisor_index_pk_type.go (set/get/delete/getPKColumnType),
  advisor_table_require_pk.go (dropColumn/changeColumn), and utils.go
  (set/get/delete on tableNewColumn).

CodeQL Analyze (go) FAILURE on the previous commit was an infra-side
runner shutdown ("The runner has received a shutdown signal"), not a
real code finding. This push triggers a fresh run.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Codex flagged that even after the previous reset-after-validation
restructuring, BatchSkipTasks still let an empty request.Tasks slip
past every validation step:

  - the task-parse loop is empty (no per-task validation fires)
  - environmentSet is empty, so canUserRunEnvironmentTasks is never
    called (no permission check fires)
  - store.BatchSkipTasks is effectively a no-op for an empty ID slice
  - and yet the new ResetPlanWebhookDelivery / completion-check signal
    still ran — mutating the dedup ledger and re-arming notifications
    for a request that did literally nothing

Add an early empty-input guard at the top of BatchSkipTasks, mirroring
the pattern that already exists in BatchRunTasks (line 714) and
BatchCancelTaskRuns (line 970). All three batch endpoints now reject
empty input before any state inspection or mutation.

Adds a regression subtest BatchSkipTasks_RejectsEmptyTasks that asserts
InvalidArgument on empty Tasks. Without the guard this test would have
returned success and silently mutated the dedup ledger.

Verified by 2 consecutive TestWebhookIntegration runs (163s/146s,
20/20 PASS).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…mni AST (Phase 1.5 §1.5.3 batch 6) (#20217)

* refactor(advisor/tidb): migrate charset/collation/comment family to omni AST (Phase 1.5 §1.5.3 batch 6)

Migrate 4 advisors to omni AST via Recipe A (top-level type-switch):
- advisor_charset_allowlist.go
- advisor_collation_allowlist.go
- advisor_table_comment_convention.go
- advisor_column_comment_convention.go

Recipe choices verified empirically (per Vincent's pre-flight gate after
batch 5's Recipe-A miss on EXPLAIN-wrapped DML):
- All 4 pingcap Visitors return (in, false) which would suggest Recipe B
  by the heuristic, BUT empirically `EXPLAIN CREATE TABLE` and any other
  EXPLAIN-DDL form fails to parse in both pingcap and omni grammars.
  The matched stmt types (CreateDatabase, CreateTable, AlterDatabase,
  AlterTable) cannot nest themselves, and no wrapper exists that embeds
  them. Recipe A is safe for all 4.

NEW cumulative shape divergence #14 (proposed for plan invariant #7
table): TiDB option representation simplification.

  Pingcap: TableOption{Tp: enum, StrValue}, DatabaseOption{Tp, Value},
  ColumnDef.Tp.GetCharset() (method on FieldType), column.Options[Tp ==
  ColumnOptionComment].Expr (needs restoreNode).

  Omni: TableOption{Name: "KEYWORD", Value}, DatabaseOption{Name, Value},
  ColumnDef.TypeName.Charset (direct string), ColumnDef.TypeName.Collate
  (direct string), ColumnDef.Comment (direct string), ColumnConstraint{
  Type: ColConstrComment} marker for "clause was written" signal.

  Critical wrinkle empirically caught: omni does NOT canonicalize keyword
  shorthand. `CHARSET ascii` -> Name="CHARSET"; `CHARACTER SET ascii` ->
  Name="CHARACTER SET". Pingcap canonicalized at the enum level (both
  produced TableOptionCharset). Mechanical migration that compared only
  against "CHARACTER SET" would silently miss the shorthand form. Fixed
  by accepting a names-set in the helpers (see omniOptionNamesCharset).

AST-shape accessors added to utils.go (NOT rule-logic helper extraction
which Vincent told me to resist this batch — these are tiny shape-aware
field-readers, same category as addColumnTargets and omniIndexColumns):
- omniDatabaseOption / omniTableOption / omniTableOptionPresent
- omniColumnCharset / omniColumnCollate / omniColumnHasComment
- omniOptionNameMatches
- omniOptionNamesCharset / omniOptionNamesCollate / omniOptionNamesComment

Pre-batch coupling miss caught + recovered: getColumnCharset (pingcap-
typed) was used cross-file by un-migrated advisor_column_disallow_set_
charset.go. Lost when I rewrote charset_allowlist.go. Moved to utils.go
with cleanup comment per BYT-9362/9395 pattern. Linear ticket TBD.

Internal-error advice path removed from column_comment_convention: pingcap
needed restoreNode on column.Options[].Expr (fallible); omni reads
column.Comment directly (infallible). One less code path to maintain.

Fixture pins added to system_charset_allowlist.yaml:
- `CREATE TABLE t(a int) CHARACTER SET ascii` (long-form keyword pin)
- `CREATE TABLE t(a varchar(255) CHARSET ascii) CHARSET utf8mb4`
  (column-level scan runs when table-level is in allowlist)

Soft-fail accepted (per invariant #2): `ALTER TABLE t CHARACTER SET
utf8mb4 COMMENT 'x'` (combined form) is an omni grammar gap; advisor
emits no advice, review continues. Individual forms (CHARSET-only,
COMMENT-only, COLLATE-only) parse correctly.

Migration progress: 24 of 51 advisors on omni AST.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): preserve single-advice-per-stmt cardinality on ALTER TABLE

Codex P2 catch on PR #20217 (charset and collation, same regression).
Verified empirically by running both pingcap and omni paths on:

  ALTER TABLE t CHARSET ascii, MODIFY COLUMN c VARCHAR(10) CHARSET latin1

Pingcap's Visitor pattern: set `code` and `disabledCharset` inside the
spec loop without break, single append after the loop. Multiple
violations overwrite — last violation wins. Result: 1 advice with
charset="latin1" on the test input.

My batch-6 migration emitted advice inside the per-command loop, so the
same input produced 2 advices (one per violating command). Cardinality
regression — externally observable for fixtures and downstream tooling.

Fix: track a `lastViolation` local in the AlterTableStmt branch, set it
inside the loop, emit ONCE after the loop (matching pingcap's
"set-overwrite-then-append" pattern). Same fix applied to charset and
collation allowlists.

Pinned with new fixtures in system_charset_allowlist.yaml and
system_collation_allowlist.yaml — multi-violation ALTER TABLE inputs
that expect exactly ONE advice with the LAST violation's value. Without
these fixtures, a future refactor that emits per-command would silently
regress; existing fixtures only had single-violation ALTER TABLE inputs.

Per Phase 1.5 invariant #9: real find → fix + fixture pin.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): handle ATConvertCharset in charset/collation allowlists

Codex P2 catch (round 2) on PR #20217 — same regression class as
round 1 (charset and collation, identical structural pattern). Verified
empirically by parsing `ALTER TABLE t CONVERT TO CHARACTER SET ascii`
through both pingcap and omni:

Pingcap: parses as Spec.Tp=AlterTableOption (Tp=1) with the charset
in spec.Options as Tp=TableOptionCharset/StrValue="ascii". The
pingcap-typed advisor's `case ast.AlterTableOption:` matches both
"CHARSET ascii" AND "CONVERT TO CHARACTER SET ascii" forms — same
spec type, same option carrier.

Omni: SPLITS the two forms — `CHARSET ascii` is ATTableOption (Type=16)
with cmd.Option populated; `CONVERT TO CHARACTER SET ascii` is
ATConvertCharset (Type=17) with cmd.Name=charset and cmd.NewName=
collation (if COLLATE clause specified). cmd.Option is nil for the
CONVERT form. Mechanical migration matching only ATTableOption silently
misses the CONVERT form.

Fix: add `case ast.ATConvertCharset:` arm to both advisors:
- charset_allowlist: read cmd.Name (the charset)
- collation_allowlist: read cmd.NewName (the collation; "" if no COLLATE
  clause)

Both flow through the existing `lastViolation` accumulator from round 1,
preserving single-advice cardinality.

Pinned with new fixtures:
- system_charset_allowlist.yaml: `ALTER TABLE t CONVERT TO CHARACTER SET
  ascii` -> triggers
- system_collation_allowlist.yaml: `ALTER TABLE t CONVERT TO CHARACTER
  SET utf8mb4 COLLATE latin1_bin` -> triggers (collation-only check;
  charset utf8mb4 is in allowlist)

This is the same shape as cumulative #15 — pingcap collapsed two
syntactic forms at the enum level (both → AlterTableOption), omni
preserves the syntactic distinction. Mechanical case-arm migration
loses coverage on the form omni split off. Pattern: when migrating an
advisor that handled `case ast.AlterTableX:`, audit omni's split for
that enum value — there may be multiple omni cases that pingcap unified.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin DEFAULT-prefix normalization + omniColumnHasComment empty-vs-absent + Linear ticket BYT-9412

Peer review on PR #20217 identified two gaps and the BYT-9412 placeholder
to file. Bundle:

1. Linear ticket filed: BYT-9412
   "[omni/tidb] Delete getColumnCharset from advisor/tidb/utils.go after
    advisor_column_disallow_set_charset.go migrates"
   Mirrors BYT-9362 (canNull) / BYT-9395 (tableNewColumn) pattern.
   Parented to BYT-9141. utils.go comment updated to point at the
   ticket URL.

2. DEFAULT-prefix normalization fixture pin (system_charset_allowlist.yaml):
   - "CREATE TABLE t(a int) DEFAULT CHARSET ascii"
   - "CREATE TABLE t(a int) DEFAULT CHARACTER SET ascii"
   Both produce identical advisor output to the bare "CHARSET ascii" /
   "CHARACTER SET ascii" forms — locks in the claim that omni's parser
   consumes DEFAULT without affecting the TableOption shape, so the
   advisor's name-set ({CHARACTER SET, CHARSET}) catches all three
   syntactic variants identically.

   (The peer-review concern that "the original `if option.Default { return
   '' }` guard was deliberate" turned out to be empirically false —
   pingcap sets Default=false on DEFAULT-prefixed forms, so the guard
   never fired for these inputs. Both pingcap and omni flag identically.
   No silent behavior change. Pin proves the parity.)

3. TestOmniColumnHasComment_PresentAbsentDistinction (utils_test.go):
   pins the structural-marker contract distinguishing "no COMMENT clause"
   from `COMMENT ''` (deliberately empty). Both cases produce
   ColumnDef.Comment == "", so the value alone is ambiguous; the signal
   lives in Constraints[ColConstrComment]. If a future omni parser
   change drops the marker for the empty-explicit form, the
   column-comment-required check would silently treat
   deliberately-empty comments as missing. This test catches that.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Codex flagged that even after the previous reset-after-validation
restructuring, BatchSkipTasks still let an empty request.Tasks slip
past every validation step:

  - the task-parse loop is empty (no per-task validation fires)
  - environmentSet is empty, so canUserRunEnvironmentTasks is never
    called (no permission check fires)
  - store.BatchSkipTasks is effectively a no-op for an empty ID slice
  - and yet the new ResetPlanWebhookDelivery / completion-check signal
    still ran — mutating the dedup ledger and re-arming notifications
    for a request that did literally nothing

Add an early empty-input guard at the top of BatchSkipTasks, mirroring
the pattern that already exists in BatchRunTasks (line 714) and
BatchCancelTaskRuns (line 970). All three batch endpoints now reject
empty input before any state inspection or mutation.

Adds a regression subtest BatchSkipTasks_RejectsEmptyTasks that asserts
InvalidArgument on empty Tasks. Without the guard this test would have
returned success and silently mutated the dedup ledger.

Verified by 2 consecutive TestWebhookIntegration runs (163s/146s,
20/20 PASS).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec): BYT-9367 Oracle splitter trailing `;` leak design

Captures root cause (single-token SEMI lookahead skips hidden-channel
tokens), corrected design intent (channel-aware skip), fix code, test plan
(4 new YAML cases covering every cell of the design gap), audit
confirming bug is one-engine, and risk/rollback notes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec): correct PlSqlLexer.g4 line refs in BYT-9367 design

Whitespace SPACES rule is at line 2638; the 2621-2624 range only covers
the three comment rules. Spec now cites both ranges.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(plan): BYT-9367 Oracle splitter trailing ';' leak implementation

Six-task plan: verify starting state, capture buggy YAML snapshot to
demonstrate the leak, apply the channel-aware loop fix at split.go:108-113,
re-record YAML and verify text invariants, lint+build, commit. Three new
test fixtures (BYT-9367 exact, leading inline comment, multi-newline) plus
existing fixtures for cells (a), (e), (f), (g) of the design gap.

Spec: docs/superpowers/specs/2026-05-07-oracle-splitter-trailing-semicolon-leak-design.md

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec,plan): expand BYT-9367 to cover Trino + Range bookkeeping

Review surfaced two real issues missed in the original draft:

1. byteOffsetStart bookkeeping: when the channel-aware loop consumes
   tokens, the next statement's Range.Start (and Start.Line/Column)
   stayed at byteOffsetEnd of the previous statement, off by N bytes
   (where N = bytes consumed by the loop). Fix adds a 3-line update
   to advance byteOffsetStart past consumed tokens. Three existing
   plsql fixtures re-record with 1-byte/1-column shifts.

2. Trino splitter has the same bug class at trino/split.go:88-91.
   Single-token SEMI lookahead misses whitespace before ';'. Added
   to scope; same loop fix without bookkeeping (trino's text/range
   model handles it naturally via statement[byteOffsetStart:rangeEnd]).

Spec restructured: §4 design gap split into Invariant A (Text) and
Invariant B (Range alignment). §5 has separate plsql (5.1) and
trino (5.2) fixes. §6 test plan covers cells (b,c,d,f,h) for plsql
and 3 Trino fixtures. §7 audit updated. §8 risk lists which
existing fixtures re-record.

Plan restructured: Task 1 verifies both buggy locations and audits
trino's tokenizer fallback. Task 2 adds all 8 new fixtures with
result: [] placeholders. Task 3 applies both fixes. Task 4 records
once and verifies text invariants + expected existing-fixture shifts.

Pre-fix snapshot ceremony dropped (review feedback: risks accidental
commit, marginal value). allTokens hoist removed from spec but kept
in plsql code (used twice, worth the local).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec,plan): BYT-9367 v3 — drop Trino, fix cell (f), tighten bookkeeping

Adversarial review caught two real issues in v2:

1. Trino does NOT have the bug. TrinoParser.g4:38-40 says
   `singleStatement: statement SEMICOLON_` — the `;` is REQUIRED in the
   parse tree. `singleStmt.GetStop()` is the `;` itself, and rangeEnd =
   endToken.GetStop()+1 already extends past it. The 1-token lookahead
   at trino/split.go:89 handles a stray double-`;` (empty statement
   absorption), not the BYT-9367 leak shape. v3 removes Trino from
   scope entirely and updates §7 audit row to explain why.

2. v2's cell (f) input `BEGIN NULL; END\n/` is grammatically invalid:
   anonymous_block requires `END SEMICOLON` (PlSqlParser.g4:5386). v3
   replaces with `BEGIN NULL; END;\nSELECT 1 FROM dual` — anonymous
   block (needSemicolon, stop=`;`) followed by SELECT with only `\n`
   between, exercising the bail-on-default-channel branch with a
   non-`/` token (a SELECT keyword).

Other fixes:
- §6.2 missed 4th re-record fixture: `SELECT statements separated by
  forward slash` (line 120). Stmt 1 is non-needSemicolon SELECT, so the
  bookkeeping fires and shifts stmt 2's range/start.column by 1.
- Bookkeeping uses stop-index arithmetic (`allTokens[prevStop].GetStop()
  - allTokens[loopStart].GetStop()`) instead of materializing the
  consumed text via GetTextFromTokens. Equivalent and cheaper.
- §5.3 (v1→v2 changelog note) dropped — redundant with §4.
- Plan tokenizer-audit step removed (no longer needed without Trino).

Net code change: ~10 lines (loop + 2-line bookkeeping). Five new YAML
fixtures, four existing fixtures re-record with 1-byte/1-column shifts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(spec,plan): BYT-9367 v4 — fix UTF-8 bookkeeping bug, correct fixture count

Adversarial review caught two real issues in v3:

1. BLOCKER: stop-index arithmetic for bookkeeping is rune-based, not
   byte-based. ANTLR's InputStream uses []rune internally (bytebase/antlr
   v4@v4.0.0/input_stream.go:14), and token Start/Stop are rune indices.
   Go's len(string) is byte length. For ASCII the two coincide, but
   multi-byte UTF-8 inside a hidden token (e.g., a comment with non-ASCII
   characters) would diverge — the bookkeeping would advance byteOffsetStart
   by rune count instead of byte count, drifting Range and line/column.
   Reverted to len(GetTextFromTokens(...)) which is unconditionally
   byte-correct (matches the byte-offset arithmetic at line 82).

2. v3 said "14 existing fixtures (4 change, anonymous-block-with-/ at lines
   55,79,150,172 region unchanged)" but actually there are 15 fixtures and
   line 172 is ALTER TABLE (not an anonymous-block-with-/). v4 spells out
   the full set: 15 fixtures, 4 change, 11 unchanged, with explicit per-
   fixture rationale.

Other v4 cleanups:

- Cell (d) "if 3 unit_statements" hedge was impossible per grammar
  (sql_script: ((unit_statement) SEMICOLON? | SEMICOLON)*), so a bare
  `;` matches the top-level alternative, not a unit_statement, and the
  splitter only iterates IUnit_statementContext children. Cell (d) always
  produces exactly 2 unit_statements; the hedge is removed.
- Spec §4 Invariant B condensed (~10 lines cut) — the §6.2 table now
  carries the concrete numbers.
- Spec §5 Properties list trimmed; fixture-by-fixture behavior moved to
  §6 where it belongs.
- Plan Task 1 Step 1 (sed-check the lookahead block) dropped — the Edit
  tool's exact-match contract subsumes it.
- Plan re-record table now lists exact byte numbers per codex's trace.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(plsql): trailing ';' leak when whitespace/comment separates stop and ';'

BYT-9367. The plsql splitter's "advance past trailing ';'" lookahead
inspected only the token immediately after the parse-tree stop, so any
hidden-channel token (whitespace, comment) between the stop and the ';'
prevented consumption. The ';' then leaked into the next statement's
text. For Oracle this surfaced as ORA-00900 at position 1.

Replace the single-token lookahead with a channel-aware loop that skips
hidden-channel tokens until it finds ';' (consume) or a default-channel
non-';' token (bail). After the loop, advance byteOffsetStart by the byte
length of any consumed tokens so the next statement's Range.Start aligns
with where its leadingContent actually begins in source. This also
quietly fixes a pre-existing 1-byte off-by-one in the immediate-';' case
(visible in 4 existing fixtures).

Tests:
- 5 new YAML fixtures (cells b/c/d/f/h of the design gap).
- 4 existing fixtures re-recorded with 1-byte/1-column Range/Start.Column
  shifts (text unchanged): multiple SELECT statements, multiple statements
  with newlines, SELECT statements separated by forward slash, position
  semantic: multi-statement with leading whitespace.

Trino was investigated and ruled out — TrinoParser.g4 requires ';' in
the singleStatement parse tree, so the leak shape can't occur there.

Spec: docs/superpowers/specs/2026-05-07-oracle-splitter-trailing-semicolon-leak-design.md

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(plsql): mirror BYT-9367 trailing ';' walk in ParsePLSQL AST path

The BYT-9367 fix in SplitSQL (commit ce6470b) cleaned up Statement.Text
for the leak shape. But ParsePLSQL — used by ParseStatements to attach
per-statement ASTs — has its own per-statement loop with the same
"prevStopTokenIndex = stopToken.GetTokenIndex()" pattern and was NOT
fixed, so the reparse substring for the next statement still absorbed
the trailing ';'. The reparsed sub-AST then had a leading bare SEMICOLON
child, which getStatementType()'s "return type of first child" walk
classified as STATEMENT_TYPE_UNSPECIFIED. Statement.Text was clean but
the attached AST was misclassified for any BYT-9367 input.

Apply the same channel-aware loop in ParsePLSQL: after stopIdx, walk
hidden-channel tokens looking for ';' (consume) or default-channel
non-';' (bail). No byteOffsetStart bookkeeping is needed in this
function — only prevStopTokenIndex matters for the next iteration's
leadingContent computation.

The audit gap: spec §7 covered splitters across engines but did not
explicitly scan for "duplicate of the splitter loop in a separate
parse path". Both SplitSQL and ParsePLSQL had the bug; only one was
fixed in the previous commit.

Tests:
- New byt9367_repro_test.go: TestBYT9367_ParseStatementsClassification
  covers cells b/c/d (whitespace-, comment-, multi-newline-before-';')
  via base.ParseStatements + GetStatementTypes round-trip. Without the
  fix, statement #2 (or #1 for cell c) classified as UNSPECIFIED.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(plsql): extract consumeTrailingSemicolon helper, broaden BYT-9367 test

Two convergent review findings from round 2:
- The channel-aware loop was duplicated verbatim between SplitSQL
  (split.go) and ParsePLSQL (plsql.go). The duplication is exactly the
  failure shape that caused BYT-9367's secondary leak — one path was
  fixed, the parallel one was forgotten.
- The regression test (TestBYT9367_ParseStatementsClassification)
  asserted on the deduplicated GetStatementTypes output, which has
  non-deterministic order and only covered cells b/c/d.

Extract consumeTrailingSemicolon(allTokens, stopIdx) into split.go and
call it from both SplitSQL and ParsePLSQL. Drop the TODO(zp) "consolidate
with split logic in ParsePLSQL?" — done. SplitSQL still owns the
byte-offset bookkeeping (no analogue in ParsePLSQL).

Strengthen the regression test:
- Switch to per-AST classification via getStatementType() so a single
  misclassified AST is detected even when other ASTs share its type.
  Order-deterministic.
- Add cell f (no separator, exercises bail-on-default-channel via the
  AST path) and cell h (trailing ';' at EOF, exercises loop bound).

Tests: all 20 splitter subtests pass; all 5 BYT-9367 classification
subtests pass; lint clean; build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(plsql): migrate BYT-9367 classification regression to YAML, trim call-site comments

Round-3 review findings:
- byt9367_repro_test.go's cell f assertion `[UNSPECIFIED, UNSPECIFIED]`
  did not distinguish bug-vs-fix because getStatementType doesn't
  classify SELECT either; the AST shape is invariant under the bail
  branch since stmtText is computed from startToken/stopToken regardless
  of prevStopTokenIndex.
- Cell h's single-statement input has no second-stmt classification
  bug to manifest.
- Call-site comments duplicated the helper's godoc.
- File naming (byt9367_repro_test.go) was an outlier vs the package's
  domain-named test files.

Move cells (b)(c)(d) — the cells where ParsePLSQL's reparse text
actually changes between bug and fix states — into
test_statement_type.yaml. The shared TestGetStatementType runner
asserts on the deduplicated GetStatementTypes set; for these inputs
the bug produces [INSERT, UNSPECIFIED] (catches), the fix produces
[INSERT] (passes). Verified empirically by reverting plsql.go's
helper call and observing the YAML test failure.

Drop byt9367_repro_test.go entirely. Cell f / h smoke coverage is
already in test_split.yaml at the SplitSQL layer (where the bail
branch and EOF behavior ARE distinguishing).

Trim call-site block comments — the helper godoc owns the rationale.

Tests: TestGetStatementType, TestPLSQLSplitSQL all pass; lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: retrigger CodeQL Analyze (go) after runner cancellation

The CodeQL Go scan on commit 8d6ae7f terminated with 'The runner has
received a shutdown signal' — runner preemption, no code issue. Pushing
an empty commit to retrigger; no functional change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(lint): replace deprecated reflect.Ptr with reflect.Pointer

Recent main bumps to golangci-lint 2.12.2 (#20238) + Go 1.26.3 (#20241)
made govet's "inline deprecated constant" check fire on reflect.Ptr
(deprecated alias for reflect.Pointer added in Go 1.18). Main itself
has not re-run CI under the bumped versions yet, so this surfaced
first on PRs landing on top of the bumps. Two pre-existing call sites
needed updating.

No functional change — reflect.Ptr and reflect.Pointer are the same
constant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…DECIMAL(N)/ZEROFILL type rendering (Codex round-2 on batch 11)

Two Codex P2 findings + one Sonar duplication signal addressed:

## Codex P2 #1: zero-scale DECIMAL rendering

Empirical reproduction:
  pingcap: DECIMAL(10) → Tp.String() = "decimal(10,0)"   (explicit 0 scale)
  pingcap: DECIMAL(10,0) → Tp.String() = "decimal(10,0)" (canonicalized)
  omni (current): both → DataType{Length=10, Scale=0}
  omni-built (pre-fix): "decimal(10)"   (Scale=0 → no comma form)

Catalog stores "decimal(10,0)" from INFORMATION_SCHEMA — false-positive
on no-op MODIFY when user writes DECIMAL(10) or DECIMAL(10,0).

Fix: omniBuildColumnTypeString now renders explicit scale when
Length > 0 AND the type is decimal-family (decimal/numeric/fixed/
float/double/real). New helper isDecimalFamilyTypeName mirrors the
MySQL canonicalization convention.

## Codex P2 #2: ZEROFILL rendering

Empirical reproduction:
  pingcap: INT ZEROFILL → Tp.String() = "int(11) UNSIGNED ZEROFILL"
  pingcap: INT UNSIGNED ZEROFILL → same
  omni (current): DataType{Unsigned: false-or-true, Zerofill: true}
  omni-built (pre-fix): "int" / "int unsigned"   (zerofill dropped)

Fix: append " zerofill" when Zerofill is set; treat Zerofill as
implying Unsigned in the rendering (pingcap convention; MySQL storage
requires UNSIGNED whenever ZEROFILL is set).

Caveat documented in code: MySQL canonicalizes ZEROFILL display widths
to maximums (int 11 → 10 etc.); the normalizeColumnType map doesn't
cover zerofill cases, so display-width canonicalization differences
may still surface as false-positives on rare ZEROFILL no-op modifies.
Out of scope to fix (ZEROFILL is deprecated in MySQL 8.0.17+).

## Sonar duplication: extract firstAlterCommandMatching

PR #20302 was flagged at 18.8% duplicated lines on new code (59.1% on
advisor_column_disallow_changing.go, 54.5% on advisor_column_disallow_changing_order.go).
The flagged duplication was the same shape across both single-shot
advisors:

  for _, ostmt := range stmts {
      alter, ok := ostmt.Node.(*ast.AlterTableStmt)
      if !ok { continue }
      for _, cmd := range alter.Commands {
          if cmd == nil { continue }
          if <matcher> { append advice; break }
      }
  }

Extracted firstAlterCommandMatching(n, matcher) int in utils.go —
single matcher callback, returns first matching index or -1. Each
single-shot advisor's body collapses to one matcher closure + one
conditional advice emit. Same shape as batch 8's collectIndexFamily*
and batch 10's collectColumnViolations helpers.

advisor_column_disallow_changing_type is structurally different
(catalog lookup, no break) — does NOT use this helper.

## No fixture pins for the rendering fixes

Same constraint as cumulative #21's original framing: the mock test
catalog (MockMySQLDatabase.tech_book + orders) has no decimal(N,0) or
ZEROFILL columns; adding them would have cross-engine impact. The
fixes are locked conceptually via the cumulative #21 plan-doc entry +
the empirical verification recorded in this commit message.

Net: net-zero LOC (helper offsets extraction); duplication eliminated;
two latent false-positive cases fixed; cumulative #21 strengthened.
TestTiDBRules + cross-engine + lint + build all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… (Codex round-4 on batch 11)

Codex P2 round-4 correctly flagged: omniBuildColumnTypeString dropped
EnumValues entirely. Empirical reproduction:

  Pingcap ENUM('x','y','z')  → Tp.String() = "enum('x','y','z')"
  Pingcap SET('a','b','c')   → Tp.String() = "set('a','b','c')"
  Omni                       → DataType{Name="ENUM", EnumValues=[x y z]}
  Omni-built (pre-fix)       → "enum"  (values dropped entirely)

Catalog stores the literal value list via MySQL info_schema's
COLUMN_TYPE column (verified per backend/plugin/db/tidb/sync.go).

Pre-fix false-positive:
  catalog: "enum('a','b')"
  user:    ALTER TABLE t MODIFY status ENUM('a','b')   (no-op)
  omni-built: "enum"
  comparison: "enum('a','b')" ≠ "enum" → flagged as type change

Fix:
- isEnumOrSetTypeName(lower) covers "enum" and "set".
- formatEnumValueList(values []string) renders the canonical
  `'v1','v2',…` form with single-quote escaping (' → '') per SQL
  convention. Matches pingcap's Tp.String() output AND info_schema's
  COLUMN_TYPE rendering.
- ENUM/SET case takes precedence over length/scale rendering in the
  switch (the value list is the canonical body for these types).

Honest pattern recognition: this is the FOURTH round of Codex catches
on omniBuildColumnTypeString in a single batch:
  #1 (cumulative #21):  BLOB/TINYBLOB/VARBINARY " BINARY" suffix
  #2 (round-2 commit):  DECIMAL(N) zero-scale + ZEROFILL drop
  #3 (round-3 commit):  FLOAT/DOUBLE/REAL precision-hint drop
  #4 (this commit):     ENUM/SET value literal drop

The common failure mode: my builder enumerates the DataType fields I
already knew about (Name + Length + Scale + Unsigned + Zerofill) but
silently drops fields I didn't anticipate (Charset annotations,
EnumValues). The discipline I should have started with:

  enumerate ALL DataType fields (Name, Length, Scale, Unsigned,
  Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) and ask
  per-field: does pingcap's Tp.String() render it? Does the catalog
  store it? If yes to either, my builder must render it.

That's the field-by-field audit that would have caught all four
rounds in one pass. Adding to the pre-batch protocol for any future
advisor that uses Tp.String() comparison against a catalog string.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…mnType (Codex round-5 on batch 11)

Codex P2 round-5 correctly flagged: omniBuildColumnTypeString renders
DataType.Name verbatim ("boolean"), but TiDB/MySQL stores BOOLEAN
columns as tinyint(1) (verified via backend/plugin/schema/tidb/get_database_metadata.go:437-438
and backend/plugin/schema/mysql/get_database_metadata.go:259-261).

Empirical reproduction:

  Pingcap BOOL/BOOLEAN     → Tp.String() = "tinyint(1)"  (canonicalized)
  Pingcap TINYINT(1)       → Tp.String() = "tinyint(1)"  (same)
  Pingcap TINYINT (bare)   → Tp.String() = "tinyint(4)"  (default width)
  Omni BOOL/BOOLEAN        → DataType{Name="BOOLEAN", Length=0}
  Omni TINYINT(1)          → DataType{Name="TINYINT", Length=1}

Catalog stores "tinyint(1)" for boolean columns.

Pre-fix false-positive:
  catalog:    "tinyint(1)"
  user:       ALTER TABLE t MODIFY x BOOLEAN   (no-op against a boolean column)
  omni-built: "boolean"
  normalize:  default branch → identity → "boolean"
  comparison: "boolean" ≠ "tinyint(1)" → flagged as type change

Fix: add `"boolean"` and `"bool"` cases to normalizeColumnType, both
mapping to `"tinyint(1)"`. Placed in normalizeColumnType rather than
omniBuildColumnTypeString because the normalize layer is where the
existing canonicalization conventions live (integer default widths
already follow this pattern: `int` ↔ `int(11)`, `tinyint` ↔ `tinyint(4)`).

Both sides of the comparison go through normalize, so the canonicalization
is symmetric — works regardless of which side originally rendered which
form.

Codex round-5 of 5 on omniBuildColumnTypeString in this PR:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias normalization     → this commit

The field-by-field DataType audit (Name, Length, Scale, Unsigned,
Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) that I
identified in round-4 as the missing pre-batch discipline would NOT
have caught round-5 — this is a TYPE-NAME canonicalization concern,
not a field-rendering concern. The pre-batch discipline addition for
this class: when porting a rule that compares type-strings against
the catalog, audit normalizeColumnType for alias coverage by
cross-referencing MySQL's documented type aliases (BOOL/BOOLEAN ↔
TINYINT(1), DEC ↔ DECIMAL, REAL ↔ DOUBLE, INTEGER ↔ INT, etc.) and
backend/plugin/schema/*/get_database_metadata.go's explicit
normalization map.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…(10,0) (Codex round-6 on batch 11)

Codex P2 round-6 correctly flagged: omniBuildColumnTypeString renders
bare DECIMAL (no parens) as "decimal" (Length=0 → default case),
while pingcap Tp.String() and MySQL INFORMATION_SCHEMA both
canonicalize to "decimal(10,0)" (MySQL defaults M=10, D=0).

Pre-fix false-positive:
  catalog:    "decimal(10,0)"
  user:       ALTER TABLE t MODIFY amount DECIMAL   (no-op against decimal(10,0))
  omni:       Name="DECIMAL" Length=0 Scale=0
  omni-built: "decimal"
  normalize:  default branch → identity → "decimal"
  comparison: "decimal" ≠ "decimal(10,0)" → flagged as type change

Empirical (from prior verification): pingcap renders bare DECIMAL,
DECIMAL(10), and DECIMAL(10,0) all as "decimal(10,0)" — MySQL applies
the default precision regardless of which syntactic form the user
typed.

Fix: add `"decimal"`, `"numeric"`, `"fixed"` cases to
normalizeColumnType, all mapping to `"decimal(10,0)"`. NUMERIC and
FIXED are MySQL aliases for DECIMAL — included defensively for the
same canonicalization story. Same layer as the round-5 BOOLEAN
canonicalization and the integer default-width entries — symmetric
across both sides of the comparison.

Codex round-6 of 6 (so far) on the type-string comparison path:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias                   → round-5 commit
  #6 bare DECIMAL ↔ decimal(10,0) default         → this commit

Refined pre-batch protocol going forward (third axis added):

  For any rule that compares type-strings against catalog, audit:
    (a) omniBuildColumnTypeString — every DataType field that
        pingcap renders or catalog stores. Field-by-field check
        across Name, Length, Scale, Unsigned, Zerofill, Charset,
        Collate, EnumValues, ArrayDim, SRID.
    (b) normalizeColumnType — every MySQL type alias and default-
        application. Cross-reference backend/plugin/schema/*/
        get_database_metadata.go's normalization map.
    (c) Bare-form defaults — for every type with implicit precision
        defaults (DECIMAL → (10,0), INT → (11), TINYINT → (4), etc.),
        verify the bare-form is canonicalized at the normalize layer.

Axes (a) caught rounds 2-4. (b) caught round 5. (c) caught round 6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…nit tests; close round-7 risk with BIT/BINARY/YEAR canonicalization

Peer review on PR #20302 flagged three concerns. Addressing all three.

## #1: Pure-function unit tests (peer recommendation #1)

Six rounds of Codex catches across the PR all carried the same
"no fixture pin due to shared mock catalog cross-engine impact"
deferral. Peer correctly identified this as a test-infrastructure
limitation, not a fundamental constraint — the fixes are pure
(DataType, string) → string transforms and can be tested package-
locally without the advisor/catalog framework.

Added advisor_column_disallow_changing_type_test.go with three
suites:

  - TestOmniBuildColumnTypeString: 31 cases covering rounds 1-7
    (BLOB family, exact-decimal, ZEROFILL, FLOAT/DOUBLE/REAL,
    ENUM/SET escape, bare-form defaults, regulars).

  - TestNormalizeColumnType: 18 cases covering all canonicalization
    rules (BOOLEAN ↔ tinyint(1), bare DECIMAL ↔ decimal(10,0),
    bare BIT/BINARY/YEAR ↔ defaults, integer default widths,
    identity branch).

  - TestTypeStringRoundTripAgainstNormalize: 24 cases asserting the
    load-bearing invariant for columnTypeChanged — that
    normalize(omni-built) == normalize(catalog) across every
    no-op-modify scenario we've fixed. Plus negative cases (actual
    type changes) confirming the invariant correctly rejects them.

~120 LOC of tests covering every Codex round's empirical receipt.
Locks the contract in CI; future refactors of the type-rendering
path that re-introduce any class of bug will fail these tests.

## #2: Round-7 risk check (peer recommendation #2)

Ran the harness across edge type families (BIT, JSON, GEOMETRY,
spatial, BINARY/VARBINARY, COLLATE columns, DATETIME with fractional
seconds, YEAR, integer display widths). Surfaced THREE real round-7
candidates pre-merge:

  - bare BIT     → pingcap "bit(1)",     omni-built "bit"     (mismatch)
  - bare BINARY  → pingcap "binary(1)",  omni-built "binary"  (mismatch)
  - bare YEAR    → pingcap "year(-1)",   omni-built "year"    (mismatch, plus pingcap render is itself a `-1` sentinel artifact; catalog stores year(4))

Plus two expected-divergence cases (cumulative #21 shape — pingcap-
side ` BINARY` / ` CHARACTER SET` annotation artifacts that omni
correctly omits, matching catalog).

Fixed all three at the normalize layer (same as BOOLEAN round-5 +
DECIMAL round-6 pattern):

  case "bit":    return "bit(1)"
  case "binary": return "binary(1)"
  case "year":   return "year(4)"

Symmetric across both comparison sides; bounded by the empirical
verification harness; locked by the unit tests above.

The round-7 risk check finished with all remaining cases either
matching or known-fix-direction. Strong confidence the type-string
comparison path is settled.

## #3: Mysql cardinality claim verified (peer recommendation #3)

Previous PR description's batch-11 commit body claimed:
  "potential mysql regression introduced by the mysql omni
   migration ... verifying pre-omni behavior requires git-show
   + parse test that's out of scope for this PR"

This was an unverified-regression assertion — exactly the class
plan invariant #10 was designed to prevent.

Verified empirically (~3 minutes via git-show 699ff60^):
pre-omni mysql also emitted per-cmd, no break. The "for each item
in AlterListItem ... AddAdvice(...) ..." loop is structurally
identical to the post-omni "for each cmd ... AddAdvice(...)"
loop. Per-cmd cardinality is LONG-STANDING pre-omni behavior
preserved by the omni migration, NOT a regression.

Plan-doc cumulative table entry not needed (no novel pattern;
matches existing #19 framing). PR description correction will
follow in the merge body.

## Net for PR #20302

  - 6 Codex rounds of type-rendering fixes (rounds 1-6)
  - Round-7 risk check found 3 more pre-merge fixes (BIT/BINARY/YEAR)
  - 49 unit-test cases pinning every round's contract
  - mysql cardinality claim corrected per empirical verification

The type-string comparison path on this advisor is now bounded by
unit tests in CI. Future regressions of the rounds-1-7 classes
have a test guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… AST (Phase 1.5 §1.5.3 batch 11) (#20302)

* refactor(advisor/tidb): migrate column_disallow_changing trio to omni AST (Phase 1.5 §1.5.3 batch 11)

Three theme-coupled advisors migrated as one unit. No helper coupling
between them (verified via pre-batch grep) — ship-as-unit is theme-
and reviewer-cadence-driven, not load-bearing.

- advisor_column_disallow_changing.go
- advisor_column_disallow_changing_order.go
- advisor_column_disallow_changing_type.go

All Recipe A. ALTER TABLE only (no CreateTable arm in pingcap-tidb
predecessors; preserved).

35 of 51 advisors merged once this lands (32 → 35). 16 remaining +
1 Class III blocked.

## Single-advice-per-statement cardinality contract preserved

All three pingcap-typed predecessors used a Visitor with `break` after
the first match, emitting at most ONE advice per top-level ALTER TABLE
statement. The omni port replicates this with explicit `break`. **Mysql
analog diverges** — emits per-cmd without breaking, producing N advices
for N matching specs. Cardinality divergence preserved on the tidb side
per invariant #7's "preserve pingcap behavior" rule.

Pinned each advisor with multi-CHANGE/MODIFY fixtures (expected: 1
advice for 2-spec statement).

## changing_type: cumulative #21 — latent pingcap false-positive fixed

Empirical finding from pre-batch protocol step 2a:

Pingcap's `FieldType.String()` rendering appends a ` BINARY` charset
annotation on types stored with binary charset:
  BLOB        → "blob BINARY"
  TINYBLOB    → "tinyblob BINARY"
  VARBINARY(N) → "varbinary(N) BINARY"

The pingcap-typed `disallow_changing_type` rule compared this rendered
string against `column.GetProto().Type` from the catalog (which stores
the bare type name from INFORMATION_SCHEMA — no charset suffix). The
`normalizeColumnType` helper has no "blob" / "blob binary" case, so its
default branch is identity-lowercase. Result:

  "blob" (catalog) ≠ "blob binary" (pingcap render)
  → rule flagged `ALTER TABLE t MODIFY a BLOB` on a BLOB column
    (no-op type change) as a type change.

LATENT FALSE-POSITIVE on the pingcap-typed advisor.

Omni's `DataType.Name` is the bare type name with no charset suffix.
The omni port builds the type string from `Name + Length + Scale +
Unsigned` (omniBuildColumnTypeString helper). Catalog "blob" matches
omni-built "blob" → false-positive silently fixed by this migration.

**Not a regression; a UX improvement.** Same shape as cumulative #18's
"is text" advice content fix.

**Why no fixture pin:** mock test catalog (MockMySQLDatabase.tech_book)
doesn't carry a BLOB column; adding one would have cross-engine impact
(mysql + tidb + mariadb + oceanbase all share it). The fix is locked
conceptually via plan-doc cumulative #21, empirically verified during
pre-batch protocol via parse-through-both-engines test.

## FIRST/AFTER detection for changing_order

Pingcap: `spec.Position.Tp != ast.ColumnPositionNone`.
Omni: `cmd.First || cmd.After != ""` (separate fields; both empty when
no position specified). Verified empirically.

## Helpers

In advisor_column_disallow_changing_type.go (local, not extracted):
- omniBuildColumnTypeString(dt *DataType) string — renders the
  "name[(length[,scale])] [unsigned]" form. Mirrors the mysql analog's
  inline rendering. Local because the rendering specifically targets
  the normalizeColumnType contract used only by this advisor.
- normalizeColumnType — kept structurally identical to the pingcap-
  typed predecessor + mysql analog for fixture parity.

## Cumulative #21 added to plan doc

Documents the BLOB/BINARY charset-suffix latent false-positive fix.
Mysql parallel verified free of the same issue (mysql analog rebuilt
type string from DataType during omni migration, same as my tidb port).

## Mysql cardinality divergence noted but not filed

The three mysql analogs emit per-cmd (no break), which differs from
both pre-omni mysql (likely had break) and pingcap-tidb (had break).
This is a potential mysql regression introduced by the omni migration,
but verifying pre-omni behavior requires git-show + parse test that's
out of scope for this PR. Flagged here for future investigation under
plan invariant #10 if/when a customer reports duplicate advices on
multi-CHANGE statements.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): extract single-shot alter-table helper + fix DECIMAL(N)/ZEROFILL type rendering (Codex round-2 on batch 11)

Two Codex P2 findings + one Sonar duplication signal addressed:

## Codex P2 #1: zero-scale DECIMAL rendering

Empirical reproduction:
  pingcap: DECIMAL(10) → Tp.String() = "decimal(10,0)"   (explicit 0 scale)
  pingcap: DECIMAL(10,0) → Tp.String() = "decimal(10,0)" (canonicalized)
  omni (current): both → DataType{Length=10, Scale=0}
  omni-built (pre-fix): "decimal(10)"   (Scale=0 → no comma form)

Catalog stores "decimal(10,0)" from INFORMATION_SCHEMA — false-positive
on no-op MODIFY when user writes DECIMAL(10) or DECIMAL(10,0).

Fix: omniBuildColumnTypeString now renders explicit scale when
Length > 0 AND the type is decimal-family (decimal/numeric/fixed/
float/double/real). New helper isDecimalFamilyTypeName mirrors the
MySQL canonicalization convention.

## Codex P2 #2: ZEROFILL rendering

Empirical reproduction:
  pingcap: INT ZEROFILL → Tp.String() = "int(11) UNSIGNED ZEROFILL"
  pingcap: INT UNSIGNED ZEROFILL → same
  omni (current): DataType{Unsigned: false-or-true, Zerofill: true}
  omni-built (pre-fix): "int" / "int unsigned"   (zerofill dropped)

Fix: append " zerofill" when Zerofill is set; treat Zerofill as
implying Unsigned in the rendering (pingcap convention; MySQL storage
requires UNSIGNED whenever ZEROFILL is set).

Caveat documented in code: MySQL canonicalizes ZEROFILL display widths
to maximums (int 11 → 10 etc.); the normalizeColumnType map doesn't
cover zerofill cases, so display-width canonicalization differences
may still surface as false-positives on rare ZEROFILL no-op modifies.
Out of scope to fix (ZEROFILL is deprecated in MySQL 8.0.17+).

## Sonar duplication: extract firstAlterCommandMatching

PR #20302 was flagged at 18.8% duplicated lines on new code (59.1% on
advisor_column_disallow_changing.go, 54.5% on advisor_column_disallow_changing_order.go).
The flagged duplication was the same shape across both single-shot
advisors:

  for _, ostmt := range stmts {
      alter, ok := ostmt.Node.(*ast.AlterTableStmt)
      if !ok { continue }
      for _, cmd := range alter.Commands {
          if cmd == nil { continue }
          if <matcher> { append advice; break }
      }
  }

Extracted firstAlterCommandMatching(n, matcher) int in utils.go —
single matcher callback, returns first matching index or -1. Each
single-shot advisor's body collapses to one matcher closure + one
conditional advice emit. Same shape as batch 8's collectIndexFamily*
and batch 10's collectColumnViolations helpers.

advisor_column_disallow_changing_type is structurally different
(catalog lookup, no break) — does NOT use this helper.

## No fixture pins for the rendering fixes

Same constraint as cumulative #21's original framing: the mock test
catalog (MockMySQLDatabase.tech_book + orders) has no decimal(N,0) or
ZEROFILL columns; adding them would have cross-engine impact. The
fixes are locked conceptually via the cumulative #21 plan-doc entry +
the empirical verification recorded in this commit message.

Net: net-zero LOC (helper offsets extraction); duplication eliminated;
two latent false-positive cases fixed; cumulative #21 strengthened.
TestTiDBRules + cross-engine + lint + build all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): restrict zero-scale rendering to exact-decimal types (Codex round-3 on batch 11)

Codex P2 correctly flagged: my omniBuildColumnTypeString from the
previous round treated float/double/real like DECIMAL — always
rendering (length, scale) when Length > 0. Empirical verification
shows MySQL/pingcap behavior differs between exact and approximate
numeric types:

  Exact-precision (DECIMAL / NUMERIC / FIXED):
    DECIMAL       → pingcap "decimal(10,0)"  (defaults applied)
    DECIMAL(10)   → pingcap "decimal(10,0)"  (canonicalized)
    DECIMAL(10,0) → pingcap "decimal(10,0)"
    DECIMAL(10,2) → pingcap "decimal(10,2)"

  Approximate (FLOAT / DOUBLE / REAL):
    FLOAT         → pingcap "float"
    FLOAT(10)     → pingcap "float"          (precision hint DROPPED)
    FLOAT(10,0)   → pingcap "float"          (precision hint DROPPED)
    FLOAT(10,2)   → pingcap "float(10,2)"    (only when scale > 0)
    DOUBLE(10,0)  → pingcap "double"         (same shape)
    REAL          → pingcap "double"         (REAL is aliased to DOUBLE)
    REAL(10,0)    → pingcap "double"
    REAL(10,2)    → pingcap "double(10,2)"

Catalog stores what MySQL info_schema returns — same canonicalization.

Pre-fix behavior with float/double/real in the decimal-family list:
  MODIFY price FLOAT(10) → my port renders "float(10,0)"
  Catalog (real DB):     stores "float"
  Comparison:            DIFFERENT → false-positive

Post-fix (this commit):
- isExactDecimalTypeName covers decimal / numeric / fixed only.
- isApproximateFloatTypeName covers float / double / real — these
  drop both Length and Scale unless Scale > 0 (rendering bare type
  name instead).
- All other types: render (Length) when Length > 0, no scale.

Honest reflection: third rendering shape divergence Codex has caught
on this single advisor (cumulative #21 BLOB BINARY → batch-11 commit-1
DECIMAL(10,0) + ZEROFILL → this commit FLOAT/DOUBLE/REAL). The common
pattern is that pingcap's `Tp.String()` has many type-specific
canonicalization rules that my builder needs to mirror per-family,
not generalize across. The "approximate vs exact" split is the
canonical MySQL distinction and should be the structural framing
for future migration of any rule that renders type-strings for
comparison.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): preserve ENUM/SET value literals in type rendering (Codex round-4 on batch 11)

Codex P2 round-4 correctly flagged: omniBuildColumnTypeString dropped
EnumValues entirely. Empirical reproduction:

  Pingcap ENUM('x','y','z')  → Tp.String() = "enum('x','y','z')"
  Pingcap SET('a','b','c')   → Tp.String() = "set('a','b','c')"
  Omni                       → DataType{Name="ENUM", EnumValues=[x y z]}
  Omni-built (pre-fix)       → "enum"  (values dropped entirely)

Catalog stores the literal value list via MySQL info_schema's
COLUMN_TYPE column (verified per backend/plugin/db/tidb/sync.go).

Pre-fix false-positive:
  catalog: "enum('a','b')"
  user:    ALTER TABLE t MODIFY status ENUM('a','b')   (no-op)
  omni-built: "enum"
  comparison: "enum('a','b')" ≠ "enum" → flagged as type change

Fix:
- isEnumOrSetTypeName(lower) covers "enum" and "set".
- formatEnumValueList(values []string) renders the canonical
  `'v1','v2',…` form with single-quote escaping (' → '') per SQL
  convention. Matches pingcap's Tp.String() output AND info_schema's
  COLUMN_TYPE rendering.
- ENUM/SET case takes precedence over length/scale rendering in the
  switch (the value list is the canonical body for these types).

Honest pattern recognition: this is the FOURTH round of Codex catches
on omniBuildColumnTypeString in a single batch:
  #1 (cumulative #21):  BLOB/TINYBLOB/VARBINARY " BINARY" suffix
  #2 (round-2 commit):  DECIMAL(N) zero-scale + ZEROFILL drop
  #3 (round-3 commit):  FLOAT/DOUBLE/REAL precision-hint drop
  #4 (this commit):     ENUM/SET value literal drop

The common failure mode: my builder enumerates the DataType fields I
already knew about (Name + Length + Scale + Unsigned + Zerofill) but
silently drops fields I didn't anticipate (Charset annotations,
EnumValues). The discipline I should have started with:

  enumerate ALL DataType fields (Name, Length, Scale, Unsigned,
  Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) and ask
  per-field: does pingcap's Tp.String() render it? Does the catalog
  store it? If yes to either, my builder must render it.

That's the field-by-field audit that would have caught all four
rounds in one pass. Adding to the pre-batch protocol for any future
advisor that uses Tp.String() comparison against a catalog string.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize BOOLEAN ↔ tinyint(1) in normalizeColumnType (Codex round-5 on batch 11)

Codex P2 round-5 correctly flagged: omniBuildColumnTypeString renders
DataType.Name verbatim ("boolean"), but TiDB/MySQL stores BOOLEAN
columns as tinyint(1) (verified via backend/plugin/schema/tidb/get_database_metadata.go:437-438
and backend/plugin/schema/mysql/get_database_metadata.go:259-261).

Empirical reproduction:

  Pingcap BOOL/BOOLEAN     → Tp.String() = "tinyint(1)"  (canonicalized)
  Pingcap TINYINT(1)       → Tp.String() = "tinyint(1)"  (same)
  Pingcap TINYINT (bare)   → Tp.String() = "tinyint(4)"  (default width)
  Omni BOOL/BOOLEAN        → DataType{Name="BOOLEAN", Length=0}
  Omni TINYINT(1)          → DataType{Name="TINYINT", Length=1}

Catalog stores "tinyint(1)" for boolean columns.

Pre-fix false-positive:
  catalog:    "tinyint(1)"
  user:       ALTER TABLE t MODIFY x BOOLEAN   (no-op against a boolean column)
  omni-built: "boolean"
  normalize:  default branch → identity → "boolean"
  comparison: "boolean" ≠ "tinyint(1)" → flagged as type change

Fix: add `"boolean"` and `"bool"` cases to normalizeColumnType, both
mapping to `"tinyint(1)"`. Placed in normalizeColumnType rather than
omniBuildColumnTypeString because the normalize layer is where the
existing canonicalization conventions live (integer default widths
already follow this pattern: `int` ↔ `int(11)`, `tinyint` ↔ `tinyint(4)`).

Both sides of the comparison go through normalize, so the canonicalization
is symmetric — works regardless of which side originally rendered which
form.

Codex round-5 of 5 on omniBuildColumnTypeString in this PR:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias normalization     → this commit

The field-by-field DataType audit (Name, Length, Scale, Unsigned,
Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) that I
identified in round-4 as the missing pre-batch discipline would NOT
have caught round-5 — this is a TYPE-NAME canonicalization concern,
not a field-rendering concern. The pre-batch discipline addition for
this class: when porting a rule that compares type-strings against
the catalog, audit normalizeColumnType for alias coverage by
cross-referencing MySQL's documented type aliases (BOOL/BOOLEAN ↔
TINYINT(1), DEC ↔ DECIMAL, REAL ↔ DOUBLE, INTEGER ↔ INT, etc.) and
backend/plugin/schema/*/get_database_metadata.go's explicit
normalization map.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize bare DECIMAL/NUMERIC/FIXED to decimal(10,0) (Codex round-6 on batch 11)

Codex P2 round-6 correctly flagged: omniBuildColumnTypeString renders
bare DECIMAL (no parens) as "decimal" (Length=0 → default case),
while pingcap Tp.String() and MySQL INFORMATION_SCHEMA both
canonicalize to "decimal(10,0)" (MySQL defaults M=10, D=0).

Pre-fix false-positive:
  catalog:    "decimal(10,0)"
  user:       ALTER TABLE t MODIFY amount DECIMAL   (no-op against decimal(10,0))
  omni:       Name="DECIMAL" Length=0 Scale=0
  omni-built: "decimal"
  normalize:  default branch → identity → "decimal"
  comparison: "decimal" ≠ "decimal(10,0)" → flagged as type change

Empirical (from prior verification): pingcap renders bare DECIMAL,
DECIMAL(10), and DECIMAL(10,0) all as "decimal(10,0)" — MySQL applies
the default precision regardless of which syntactic form the user
typed.

Fix: add `"decimal"`, `"numeric"`, `"fixed"` cases to
normalizeColumnType, all mapping to `"decimal(10,0)"`. NUMERIC and
FIXED are MySQL aliases for DECIMAL — included defensively for the
same canonicalization story. Same layer as the round-5 BOOLEAN
canonicalization and the integer default-width entries — symmetric
across both sides of the comparison.

Codex round-6 of 6 (so far) on the type-string comparison path:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias                   → round-5 commit
  #6 bare DECIMAL ↔ decimal(10,0) default         → this commit

Refined pre-batch protocol going forward (third axis added):

  For any rule that compares type-strings against catalog, audit:
    (a) omniBuildColumnTypeString — every DataType field that
        pingcap renders or catalog stores. Field-by-field check
        across Name, Length, Scale, Unsigned, Zerofill, Charset,
        Collate, EnumValues, ArrayDim, SRID.
    (b) normalizeColumnType — every MySQL type alias and default-
        application. Cross-reference backend/plugin/schema/*/
        get_database_metadata.go's normalization map.
    (c) Bare-form defaults — for every type with implicit precision
        defaults (DECIMAL → (10,0), INT → (11), TINYINT → (4), etc.),
        verify the bare-form is canonicalized at the normalize layer.

Axes (a) caught rounds 2-4. (b) caught round 5. (c) caught round 6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin type-rendering rounds 1-7 via pure-function unit tests; close round-7 risk with BIT/BINARY/YEAR canonicalization

Peer review on PR #20302 flagged three concerns. Addressing all three.

## #1: Pure-function unit tests (peer recommendation #1)

Six rounds of Codex catches across the PR all carried the same
"no fixture pin due to shared mock catalog cross-engine impact"
deferral. Peer correctly identified this as a test-infrastructure
limitation, not a fundamental constraint — the fixes are pure
(DataType, string) → string transforms and can be tested package-
locally without the advisor/catalog framework.

Added advisor_column_disallow_changing_type_test.go with three
suites:

  - TestOmniBuildColumnTypeString: 31 cases covering rounds 1-7
    (BLOB family, exact-decimal, ZEROFILL, FLOAT/DOUBLE/REAL,
    ENUM/SET escape, bare-form defaults, regulars).

  - TestNormalizeColumnType: 18 cases covering all canonicalization
    rules (BOOLEAN ↔ tinyint(1), bare DECIMAL ↔ decimal(10,0),
    bare BIT/BINARY/YEAR ↔ defaults, integer default widths,
    identity branch).

  - TestTypeStringRoundTripAgainstNormalize: 24 cases asserting the
    load-bearing invariant for columnTypeChanged — that
    normalize(omni-built) == normalize(catalog) across every
    no-op-modify scenario we've fixed. Plus negative cases (actual
    type changes) confirming the invariant correctly rejects them.

~120 LOC of tests covering every Codex round's empirical receipt.
Locks the contract in CI; future refactors of the type-rendering
path that re-introduce any class of bug will fail these tests.

## #2: Round-7 risk check (peer recommendation #2)

Ran the harness across edge type families (BIT, JSON, GEOMETRY,
spatial, BINARY/VARBINARY, COLLATE columns, DATETIME with fractional
seconds, YEAR, integer display widths). Surfaced THREE real round-7
candidates pre-merge:

  - bare BIT     → pingcap "bit(1)",     omni-built "bit"     (mismatch)
  - bare BINARY  → pingcap "binary(1)",  omni-built "binary"  (mismatch)
  - bare YEAR    → pingcap "year(-1)",   omni-built "year"    (mismatch, plus pingcap render is itself a `-1` sentinel artifact; catalog stores year(4))

Plus two expected-divergence cases (cumulative #21 shape — pingcap-
side ` BINARY` / ` CHARACTER SET` annotation artifacts that omni
correctly omits, matching catalog).

Fixed all three at the normalize layer (same as BOOLEAN round-5 +
DECIMAL round-6 pattern):

  case "bit":    return "bit(1)"
  case "binary": return "binary(1)"
  case "year":   return "year(4)"

Symmetric across both comparison sides; bounded by the empirical
verification harness; locked by the unit tests above.

The round-7 risk check finished with all remaining cases either
matching or known-fix-direction. Strong confidence the type-string
comparison path is settled.

## #3: Mysql cardinality claim verified (peer recommendation #3)

Previous PR description's batch-11 commit body claimed:
  "potential mysql regression introduced by the mysql omni
   migration ... verifying pre-omni behavior requires git-show
   + parse test that's out of scope for this PR"

This was an unverified-regression assertion — exactly the class
plan invariant #10 was designed to prevent.

Verified empirically (~3 minutes via git-show 699ff60^):
pre-omni mysql also emitted per-cmd, no break. The "for each item
in AlterListItem ... AddAdvice(...) ..." loop is structurally
identical to the post-omni "for each cmd ... AddAdvice(...)"
loop. Per-cmd cardinality is LONG-STANDING pre-omni behavior
preserved by the omni migration, NOT a regression.

Plan-doc cumulative table entry not needed (no novel pattern;
matches existing #19 framing). PR description correction will
follow in the merge body.

## Net for PR #20302

  - 6 Codex rounds of type-rendering fixes (rounds 1-6)
  - Round-7 risk check found 3 more pre-merge fixes (BIT/BINARY/YEAR)
  - 49 unit-test cases pinning every round's contract
  - mysql cardinality claim corrected per empirical verification

The type-string comparison path on this advisor is now bounded by
unit tests in CI. Future regressions of the rounds-1-7 classes
have a test guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): move bare-form canonicalization into builder for composability with UNSIGNED/ZEROFILL modifiers (Codex round-8 on batch 11)

Codex P2 (two comments, one underlying issue) + peer review correctly
identified the same gap: bare-form types with modifiers (UNSIGNED,
ZEROFILL) bypassed both the builder's family-specific bare-form
handling AND the normalize layer's canonicalization map.

Empirical reproduction (pingcap renders the canonical default
precision EVEN for bare forms with modifiers):

  DECIMAL UNSIGNED         → "decimal(10,0) UNSIGNED"
  DECIMAL ZEROFILL         → "decimal(10,0) UNSIGNED ZEROFILL"
  DECIMAL UNSIGNED ZEROFILL→ "decimal(10,0) UNSIGNED ZEROFILL"
  INT ZEROFILL             → "int(11) UNSIGNED ZEROFILL"
  TINYINT ZEROFILL         → "tinyint(4) UNSIGNED ZEROFILL"

Catalog stores the same canonical form. My pre-fix builder emitted
"decimal unsigned" / "int unsigned zerofill" — false-positive on
no-op `MODIFY x DECIMAL UNSIGNED` against a `decimal(10,0) unsigned`
column. normalizeColumnType had entries for bare names ("decimal",
"int") but not for bare + modifier ("decimal unsigned",
"int unsigned zerofill") — the Cartesian product is unbounded as a
normalize map.

## Structural fix: canonicalize at builder, not normalize

Per peer recommendation, the fix moves canonicalization to the
builder. Round-3's FLOAT/DOUBLE/REAL path was already builder-based;
making the bare-form canonicalization builder-based too means the
canonical-base + modifier suffix composes naturally:

  buildBaseTypeForm: emits canonical body (decimal(10,0), int(11),
                     tinyint(4), bit(1), binary(1), year(4),
                     tinyint(1) for BOOLEAN — via the new
                     canonicalBareTypeForm map). FLOAT/DOUBLE/REAL
                     keep their bare-form (no MySQL default).
  Modifier suffixes:  appended once at the end (unsigned + zerofill).

Now any bare-form × {UNSIGNED, ZEROFILL, UNSIGNED ZEROFILL}
combination composes correctly without combinatorial normalize
entries. New normalize entries from rounds 5/6/7 (BOOLEAN ↔ tinyint(1),
bare DECIMAL ↔ decimal(10,0), bare BIT/BINARY/YEAR ↔ defaults) stay
as belt-and-suspenders — they handle the case where catalog ever
stores a bare form, even though catalog typically canonicalizes.

## Discipline refinement: axis (c) was incomplete

Post-round-6 the discipline had three axes:
  (a) Field-by-field DataType audit         (caught rounds 2, 3, 4)
  (b) Type-name alias canonicalization      (caught round 5)
  (c) Bare-form defaults                    (caught round 6)

Round 8 reveals (c) was only enumerating bare-form, not bare-form ×
{applicable modifiers}. Revised:

  (c-revised) For each canonical-bare-form type family, enumerate
              the Cartesian product with applicable modifiers
              (UNSIGNED, ZEROFILL, UNSIGNED ZEROFILL for integer +
              exact-decimal families; verify no modifiers for
              BIT/BINARY/YEAR/BOOLEAN; verify FLOAT-family does
              NOT apply default precision under modifiers).

## Tests

Updated TestOmniBuildColumnTypeString:
  - Replaced old round-2 "INT ZEROFILL → int unsigned zerofill"
    case (which was pinning the broken bare-form behavior) with
    the canonical "int(11) unsigned zerofill" output.
  - Added 18 new cases covering the Cartesian product:
    DECIMAL/NUMERIC × {UNSIGNED, ZEROFILL, UNSIGNED+ZEROFILL},
    TINYINT/SMALLINT/MEDIUMINT/INT/BIGINT × {ZEROFILL, UNSIGNED+ZEROFILL,
    UNSIGNED}, FLOAT/DOUBLE × {UNSIGNED} (verifies float-family
    keeps bare form even under modifier).

Updated TestTypeStringRoundTripAgainstNormalize:
  - 6 new round-8 no-op-modify scenarios with bare-form × modifier.
    Catalog stores canonical (decimal(10,0) unsigned etc.); builder
    now produces matching form. normalize(both) yields equal strings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize NUMERIC alias to decimal in builder (Codex round-9 on batch 11)

Codex P2 round-9 correctly flagged: buildBaseTypeForm renders the
exact-decimal family verbatim from DataType.Name. NUMERIC(8,2)
emits "numeric(8,2)"; catalog stores "decimal(8,2)" (per
backend/plugin/schema/tidb/get_database_metadata.go line ~440
which does `strings.Replace(fullType, "numeric", "decimal", 1)`).

Empirical reproduction:

  Pingcap NUMERIC(8,2)   → Tp.String() = "decimal(8,2)"  (canonicalized)
  Pingcap FIXED(8,2)     → Tp.String() = "decimal(8,2)"  (canonicalized)
  Pingcap DEC(8,2)       → Tp.String() = "decimal(8,2)"  (canonicalized)
  Omni NUMERIC(8,2)      → Name="NUMERIC" Length=8 Scale=2   (NOT canonicalized)
  Omni FIXED(8,2)        → Name="DECIMAL" Length=8 Scale=2   (pre-canonicalized)
  Omni DEC(8,2)          → Name="DECIMAL" Length=8 Scale=2   (pre-canonicalized)
  Omni INTEGER(8)        → Name="INT" Length=8               (pre-canonicalized)

So omni pre-normalizes FIXED / DEC / INTEGER but NOT NUMERIC. My
builder previously emitted whatever Name carried — NUMERIC(8,2)
produced "numeric(8,2)" verbatim.

Fix: render the canonical name "decimal" for the exact-decimal
family in buildBaseTypeForm. Same builder-layer pattern as round-8
(canonicalize at the source rather than relying on normalize map
entries). Defensively keeps FIXED in the isExactDecimalTypeName
list even though omni currently pre-normalizes it — future grammar
changes won't introduce a regression.

## Tests

Updated TestOmniBuildColumnTypeString:
  - Replaced "NUMERIC(8) → numeric(8,0)" case (which was pinning
    the broken non-canonicalized output) with "NUMERIC(8) → decimal(8,0)".
  - Added new cases: NUMERIC(8,2), FIXED(8,2) (defensive), and
    NUMERIC(10,2) UNSIGNED.

Updated TestTypeStringRoundTripAgainstNormalize:
  - 2 new round-9 no-op-modify scenarios: NUMERIC(8,2) against
    catalog "decimal(8,2)" and bare NUMERIC against catalog
    "decimal(10,0)".

## Discipline refinement (axis-b extension)

Round 9 reveals axis (b) — type-name alias canonicalization — also
applies to TYPED-form aliases, not just bare-form aliases. Round 5
covered BOOLEAN ↔ tinyint(1) (bare alias). Round 9 covers
NUMERIC(M,D) ↔ DECIMAL(M,D) (typed alias preserving M,D).

Revised (b): For each type alias MySQL canonicalizes, verify the
canonicalization in BOTH the bare form AND every typed form
(typed length/scale/precision combinations). Bare form lives in
canonicalBareTypeForm; typed form lives in buildBaseTypeForm.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…to omni AST + cumulative #28 (Phase 1.5 §1.5.3 batch 17) (#20323)

* refactor(advisor/tidb): migrate index key-number + total-number pair to omni AST + cumulative #28 (Phase 1.5 §1.5.3 batch 17)

Two index-family advisors, both Recipe A:

- advisor_index_key_number_limit: per-statement constraint key count
  check across CreateTable / CreateIndex / AlterTable arms. No
  cross-stmt state. Uses file-local omniIndexKeyCount(c) helper:
  len(IndexColumns) for INDEX/PK/UNIQUE; len(Columns) for FOREIGN
  KEY (which has empty IndexColumns, verified empirically).

- advisor_index_total_number_limit: cross-stmt lineForTable
  aggregator gating FinalMetadata catalog lookup for the final
  per-table index count. Per-arm conditional touches:
    * CreateTable / CreateIndex: UNCONDITIONAL line update
    * ATAddColumn: conditional via omniColumnCreatesIndex (any
      column in the grouped form with inline PK/UNIQUE)
    * ATAddConstraint: conditional via omniConstraintCreatesIndex
      (PK / UNIQUE / INDEX / FULLTEXT; FK / CHECK / SPATIAL NOT
      tracked, matching pre-omni scope)
    * ATChange/ATModifyColumn: conditional via omniColumnCreatesIndex

File-local helpers (single-use, not promoted to utils.go):
- omniIndexKeyCount: keys/columns count by constraint type
- omniConstraintCreatesIndex: PK/UNIQUE/INDEX/FULLTEXT predicate
- omniColumnCreatesIndex: column-level ColConstrPrimaryKey/Unique
  check on column.Constraints
- omniConstraintAdviceName: c.Name with "PRIMARY" fallback for
  empty-name PK (cumulative #28)

Cumulative coverage:
- #2 unique-trio: omni ConstrUnique unifies pingcap's ConstraintUniq
  + UniqKey + UniqIndex. Verified empirically across all 3 forms.
  Single arm covers all mechanically. Pinned with UNIQUE INDEX
  fixture.
- #2 key/index unification: omni ConstrIndex unifies pingcap's
  ConstraintKey + ConstraintIndex. Verified empirically (parsing
  `KEY k(a)` vs `INDEX idx(a)` both yield Type=ConstrIndex).
- #28 (NEW): pingcap accepted non-standard `PRIMARY KEY name (cols)`
  extension and captured `name` as constraint.Name. Omni follows
  standard MySQL grammar (no name on PRIMARY KEY) and drops the
  identifier. Advisor falls back to "PRIMARY" (MySQL canonical
  name) for empty-name PK to avoid ugly empty-backticks. NOT a
  regression; documented as parser-level grammar-acceptance
  divergence (class b — omni normalizes away pingcap-captured
  state). NEW pre-batch audit axis: when advice content embeds
  parser-extracted identifiers, audit grammar-acceptance differential.

Migration count: 43 → 45 omni-migrated; 4 remaining (2 migratable
+ 1 Class III blocked + utils.go bridge).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): expand fixture pins for index family pair (Phase 1.5 §1.5.3 batch 17)

index_key_number_limit.yaml — two changes:
- Cumulative #28 fixture: `CREATE TABLE t(..., primary key pk (cols))`
  now expects "PRIMARY" in advice (was "pk" pre-omni). Pingcap
  accepted the non-standard PRIMARY KEY name syntax; omni follows
  standard MySQL grammar and drops the name; advisor falls back to
  "PRIMARY" canonical.
- Cumulative #2 unique-trio coverage pin: `UNIQUE INDEX ui_all`
  exercises the third pingcap UNIQUE variant (the existing fixture
  already covered UNIQUE KEY; bare UNIQUE not yet pinned). Omni
  unifies all 3 under ConstrUnique; the single-arm port handles all.

index_total_number_limit.yaml — two new touch-path pins:
- ALTER TABLE ADD UNIQUE KEY on tech_book: exercises ATAddConstraint
  arm with omni ConstrUnique. Documents the touch path (lineForTable
  registration) without depending on final catalog count exceeding
  maximum.
- ALTER TABLE ADD COLUMN with inline PRIMARY KEY: exercises
  ATAddColumn arm's omniColumnCreatesIndex check on
  column.Constraints[*].Type==ColConstrPrimaryKey. Cumulative #2
  vicinity: pre-omni used ColumnOptionPrimaryKey on the column's
  Options list; omni represents this on column.Constraints (typed
  list with ColConstrPrimaryKey).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): apply dual-arm ATAddConstraint+ATAddIndex sibling-parity convention (cumulative #17)

Peer review caught that batch 17's two new advisors broke the
established `case ATAddConstraint, ATAddIndex:` sibling-parity
convention (batch 4 naming-trio, batch 8 index spine helper, and
the recommendation explicitly framed in cumulative #17).

Practical impact today: zero — tidb omni's parser empirically emits
only ATAddConstraint for all `ALTER TABLE ADD ...` forms; ATAddIndex
is reserved in the enum but never produced. The dual-arm convention
is over-defensive on the current grammar but cost-free and forward-
compat against potential grammar evolution that may split bare ADD
INDEX from CONSTRAINT-named ADD INDEX, matching the mysql omni
shape that cumulative #8 originally described.

Applied to both new advisors:

- index_key_number_limit: filter changed from
    cmd.Type != ast.ATAddConstraint
  to:
    cmd.Type != ast.ATAddConstraint && cmd.Type != ast.ATAddIndex
- index_total_number_limit: case-arm changed from
    case ast.ATAddConstraint:
  to:
    case ast.ATAddConstraint, ast.ATAddIndex:

Brief inline comment in each site referencing cumulative #17.

No fixture pins added — omni doesn't emit ATAddIndex today, so no
observable behavior change to pin. The convention adherence is for
future-compat only; if/when grammar evolves to emit ATAddIndex,
existing fixtures (named CONSTRAINT-prefixed ADD ...) will catch
regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…cumulative #2 empirical correction

Codex P3 caught: the helper-level comment kept the old "pre-omni
explicitly OMITTED ConstraintUniqKey (long-standing miss)" framing
while the top-level docstring + plan-doc cumulative #2 were
corrected (batch 18 audit) to "ConstraintUniqKey/UniqIndex are
parser-unreachable for these inputs; pre-omni was firing correctly
via the included ConstraintUniq case".

Doc-only fix. No runtime change. Helper comment now consistent with
the empirical truth and points readers at the top-level docstring
for the full cumulative #2 note.

Also dropped the "#29" reference from this helper's comment — the
helper is about UNIQUE-trio unification (#2), not about paren-
wrapped column refs (#29). The two cumulative entries were
conflated in the old comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… (Phase 1.5 §1.5.3 batch 18) (#20324)

* refactor(advisor/tidb): promote omniConstraintAdviceName to utils.go for batch 18 reuse

File-local in batch 17's advisor_index_key_number_limit.go; second
consumer (advisor_index_no_duplicate_column in batch 18) needs the
same cumulative #28 PRIMARY-fallback logic. Promotion to utils.go
per the cross-file helper coupling discipline.

No behavior change — function body identical (one-arg `c
*ast.Constraint` → "PRIMARY" canonical for empty-name PK; returns
c.Name otherwise). The file-local instance is removed from
advisor_index_key_number_limit.go; the function moves above
omniDataTypeNameCompact in utils.go with the existing docstring
preserved + a note about the promotion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): migrate index_no_duplicate_column to omni AST (Phase 1.5 §1.5.3 batch 18)

Recipe A; no cross-stmt state. Three top-level arms (CreateTable /
CreateIndex / AlterTable) with constraint-type filter + duplicate-
column check.

Cumulative #2 coverage (verified empirically): pingcap-tidb's
parser produces `ConstraintUniq` (Tp=4) for ALL three UNIQUE
syntactic forms — bare UNIQUE, UNIQUE KEY, UNIQUE INDEX.
`ConstraintUniqKey` (Tp=5) and `ConstraintUniqIndex` (Tp=6) are
defined in pingcap's enum but the parser never produces them for
these inputs. Pre-omni's defensive case list including all three
was redundant (Tp=5/6 cases unreachable). Omni unifies under
`ConstrUnique` (Tp=1). Single-arm omni port matches pre-omni
behavior mechanically — NO behavior change at the UNIQUE boundary.

Initial speculation framed this as a "silent UX improvement fixing
a pre-omni miss of UniqKey"; empirical verification per invariant
#9 disproved the speculation before commit. Cumulative #29 was
NOT filed (the bug it would have documented doesn't exist). The
invariant-#9 discipline saved a false-positive entry.

Cumulative #28: PRIMARY KEY with non-standard `pk_a (cols)` syntax
has empty omni Name; advice uses omniConstraintAdviceName (now in
utils.go from preceding commit) which falls back to "PRIMARY"
canonical. Existing fixture updated from "pk_a" to "PRIMARY" per
the established #28 framing.

Cumulative #17 sibling-parity: `case ATAddConstraint, ATAddIndex`
dual arm preserved per established convention.

FK column source diverges from INDEX/PK/UNIQUE:
- INDEX/PK/UNIQUE keys stored in `IndexColumns []*IndexColumn`
  (each may carry `Expr` that's *ColumnRef for plain or another
  expression for functional indexes; only plain columns participate
  in the duplicate check, matching pre-omni `if key.Expr == nil`).
- FOREIGN KEY columns stored in `Columns []string` (plain strings;
  IndexColumns empty for FK — verified empirically against omni
  parser source).
File-local `omniConstraintColumnNames(c)` normalizes both shapes
to []string before duplicate detection.

New helpers (file-local — single-use, not promoted to utils.go):
- omniConstraintIsIndexFamily(t): predicate for index-family
  constraint types in the duplicate-column check
- omniIndexTypeString(t): display string for advice content
- omniConstraintColumnNames(c): normalized column-name extraction
- omniHasDuplicateString(names): first-repeat detection

Migration count: 45 → 46 omni-migrated; 3 remaining (1 migratable
[prior_backup_check] + 1 Class III [dml_dry_run] + utils.go bridge).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): expand fixture pins for index_no_duplicate_column (Phase 1.5 §1.5.3 batch 18)

- Cumulative #28 fixture update: `ALTER TABLE ADD PRIMARY KEY pk_a
  (a, a)` now expects "PRIMARY" in advice (was "pk_a" pre-omni).
  Pingcap accepted the non-standard PRIMARY KEY index_name syntax;
  omni follows standard MySQL grammar and drops the identifier;
  advisor falls back to "PRIMARY" canonical (MySQL information_
  schema reports the primary key as "PRIMARY" regardless of any
  user-supplied non-standard name).
- Cumulative #2 parity pin (UNIQUE INDEX): exercises the third
  pingcap UNIQUE syntactic form. All three (UNIQUE, UNIQUE KEY,
  UNIQUE INDEX) parse to the same enum value in both engines
  (pingcap ConstraintUniq=4, omni ConstrUnique=1); the rule
  emits identical "UNIQUE KEY ..." advice for all three.
- Bare UNIQUE (no name) pin: exercises the empty-name path on
  ConstrUnique. omniConstraintAdviceName returns "" for non-PK
  empty-name constraints (the "PRIMARY" fallback only fires on
  PRIMARY KEY); advice content uses empty backticks. Documents
  the contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin cumulative #29 — parenthesized-column wrapping false-NEGATIVE silently fixed (Codex P1 + peer-tightened framing)

Codex P1 caught: omni's parser flattens single-paren-wrapped column
refs `(a)` to inner ColumnRef; pingcap-tidb preserves the paren as
expression (`key.Expr != nil, key.Column == nil`). Pre-omni rule's
`if key.Expr == nil` filter (author intent: skip non-column
expressions) had filter-effect of also skipping paren-wrapped
columns — `INDEX idx((a), (a))` did NOT fire on the duplicate
pre-omni; fires post-omni.

Empirical verification per invariant #9:
- pingcap `((a), (a))` → both keys have Column="" Expr=true
- omni `((a))` → IndexColumn.Expr = *ast.ColumnRef{Column:"a"}
- omni `((a + 1))` → IndexColumn.Expr = *ast.BinaryOperationExpr
  (NOT ColumnRef; correctly skipped by *ColumnRef-only filter)

Disposition: Option A — accept as silent UX improvement.
- MySQL 8.0 spec: single-paren is grouping (same as bare column);
  double-paren is functional-index syntax
- Omni follows spec; pingcap treats single-paren as expression
  (parser quirk, not spec-correct)
- Rule's stated intent ("no duplicate columns in index") aligns
  with spec semantics; pre-omni miss was parser quirk leaking
  through the rule
- Non-idiomatic input shape (nobody writes `((a), (a))`
  intentionally); customer impact ~zero

Peer-tightened framing applied (4 refinements):

1. Reframe as "filter-effect mismatch with rule intent" (not
   "deliberate vs accidental filter") — generalizes to the same
   family as cumulative #21 / #24 / #26.

2. Direction descriptor: false-NEGATIVE silently fixed (NOT
   false-positive). Same family as #21 (parser quirk cleaned up
   by migration) but inverse direction — #21 was a false-POSITIVE
   being silently fixed (pingcap over-fired on no-op BLOB MODIFY);
   #29 is a false-NEGATIVE being silently fixed (pingcap under-
   fired on paren-wrapped duplicate column). Direction matters for
   future cumulative-table consumers scanning for "what shape of
   bug to look for."

3. Paired fixture pins lock the scope precisely:
   - Positive: `INDEX idx((a), (a))` → fires
   - NEGATIVE scope-bounding: `INDEX idx((a + 1), (a + 1))` → does
     NOT fire (real functional index, *BinaryOperationExpr)
   A future refactor of omniIndexColumns to extract bare column
   names from arbitrary expressions could silently broaden coverage
   into real functional indexes; the negative pin catches that.

4. Drop the "Codex P1 ⇒ preservation-required" framing entirely.
   P1 severity means "verify this, customer-shaped input differs"
   — not "must revert." The disposition pipeline (verify →
   classify → decide) gave Option A; severity label doesn't
   override.

NEW pre-batch audit axis added to cumulative #29 in plan-doc:
when pre-omni source contains an input-filter expression
(`if X == nil`, narrow type-assert, etc.), parse the niche-syntax
inputs the filter handles through both engines and compare which
parts of the filter's effect are spec-correct vs parser-quirk-
dependent. Same shape as the type-assertion narrowing audit (#24)
applied to a filter-expression boundary.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(advisor/tidb): fix omniConstraintIsIndexFamily comment to match cumulative #2 empirical correction

Codex P3 caught: the helper-level comment kept the old "pre-omni
explicitly OMITTED ConstraintUniqKey (long-standing miss)" framing
while the top-level docstring + plan-doc cumulative #2 were
corrected (batch 18 audit) to "ConstraintUniqKey/UniqIndex are
parser-unreachable for these inputs; pre-omni was firing correctly
via the included ConstraintUniq case".

Doc-only fix. No runtime change. Helper comment now consistent with
the empirical truth and points readers at the top-level docstring
for the full cumulative #2 note.

Also dropped the "#29" reference from this helper's comment — the
helper is about UNIQUE-trio unification (#2), not about paren-
wrapped column refs (#29). The two cumulative entries were
conflated in the old comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…-grammar coverage (Codex P1, cumulative #30 Codex-fix-1f)

Codex caught: omni's parser rejects Tier-4-deferred DDL grammar
(CREATE/ALTER/DROP SEQUENCE, FLASHBACK DATABASE/TABLE). Per
invariant #2, `getTiDBOmniNodes` soft-fails on omni-rejected stmts.
For builtin_prior_backup_check — a safety gate — this silently
drops DDL classification on these valid tidb statements:

  CREATE SEQUENCE seq1 START 1; UPDATE bar SET x=1;
  FLASHBACK TABLE foo; DELETE FROM bar WHERE id=5;

Pre-omni: pingcap parses both, classifies as DDL via DDLNode
interface → mixed-DDL+DML advice fires.
Post-omni: omni rejects SEQUENCE/FLASHBACK → soft-fail skips →
advisor sees only UPDATE/DELETE → no DDL detected → safety gate
silently misses.

Empirical verification (parse-test through omni): all five inputs
(CREATE/ALTER/DROP SEQUENCE, FLASHBACK DATABASE, FLASHBACK TABLE)
fail omni parsing with syntax error.

Fix: DDL detection switches to the `getTiDBNodes` (pingcap) path.
Pingcap's DDLNode interface (via the ddlNode struct embedding in
ast/base.go:81-88) catches all 22 implementers including Tier-4-
deferred grammar. The advisor still uses `getTiDBOmniNodes` for
per-table DML mixing detection (where omni AST shape is required);
DDL detection sidesteps omni's grammar gaps.

Architectural note: this dual-path approach mirrors mysql's analog,
which uses mysqlparser.ExtractTables alongside the omni AST.
Specialized advisors that need both full-grammar coverage AND
omni-specific AST shape will need both paths until omni grammar
parity catches up.

Side effects:

- omniIsDDLStmt helper REMOVED. Eight rounds of Codex catches on
  omni-only DDL enumeration (cumulative #30 Codex-fix-1c through 1e)
  demonstrated the brittle-enumeration anti-pattern; pingcap's
  authoritative DDLNode interface eliminates the maintenance burden.
- TestOmniIsDDLStmt REMOVED. The helper it tested is gone. End-to-end
  coverage of the DDL-detection path moves to TestTiDBPriorBackupCheckAdvisor.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 14 → 16):

  - "CREATE SEQUENCE + UPDATE fires mixed DDL+DML via pingcap path
    (Codex-fix-1f)": `CREATE SEQUENCE seq1 ...; UPDATE tech_book ...;`
    → MUST fire.
  - "FLASHBACK TABLE + DELETE fires mixed DDL+DML via pingcap path
    (Codex-fix-1f)": `FLASHBACK TABLE tech_book TO tech_book_old;
    DELETE FROM orders ...;` → MUST fire.

Both pins exercise the new pingcap-DDLNode resolution path.

Cumulative #30 algorithm-corrections sub-fix lineage now spans
eight sub-fixes:
  1 — SET-clause-based target extraction
  1b — distinct-base gating
  1c — DDL classification (DropView + AlterDatabase)
  1d — schema-qualified UPDATE target lookup
  1e — schema-aware unqualified-SET resolution
  1f — DDL detection via pingcap DDLNode (drops omniIsDDLStmt)
  2 — case-insensitive grouping key
  2-revised — DBSchema.Name default-DB

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…T — last migratable advisor (Phase 1.5 §1.5.3 batch 19) (#20325)

* refactor(advisor/tidb): migrate builtin_prior_backup_check to omni AST (Phase 1.5 §1.5.3 batch 19 — last migratable advisor)

The final IIb migration: 285→329 LOC expression-tree walker with
catalog-aware unique-WHERE detection. Mechanical port preserving
pre-omni tidb behavior exactly (invariant #7).

Helper translation table:

| Pre-omni | Omni |
|----------|------|
| ast.DDLNode (marker iface) | omniIsDDLStmt (24 concrete types) |
| *ast.UpdateStmt / *ast.DeleteStmt | *omniast.UpdateStmt / *omniast.DeleteStmt |
| update.TableRefs (*TableRefsClause) | update.Tables []TableExpr |
| *ast.BinaryOperationExpr / opcode.* | *omniast.BinaryExpr / BinOp* |
| *ast.ColumnNameExpr | *omniast.ColumnRef |
| ast.ValueExpr (marker iface) | omniIsConstantLit (8 literal types) |
| *ast.TableSource | (no direct equivalent — TableExpr interface) |
| *ast.TableName | *omniast.TableRef |
| *ast.Join | *omniast.JoinClause |
| *ast.SetOprStmt | *omniast.SelectStmt{SetOp != SetOpNone} |
| *ast.SubqueryExpr | *omniast.SubqueryExpr |
| node.Name.L (lowercase) | strings.ToLower(node.Column) |
| stmtNode.OriginTextPosition() | ostmt.FirstTokenLine() |

Audit axes applied:

- #7 (preserve pre-omni): mechanical port; pre-omni semantics
  preserved including the niche "single-table-with-unique-WHERE
  short-circuit" for >maxMixedDMLCount DML batches.
- #19 (case-sensitivity): pre-omni used .L for case-insensitive
  matching. Omni preserves user case via direct strings; explicit
  strings.ToLower / strings.EqualFold at lookup sites.
- #26 (UNION-root): pre-omni extractResultSetNode returned nil for
  *ast.SetOprStmt and *ast.SelectStmt arms. Omni's UNION-rooted
  derived tables (SelectStmt-with-SetOp) are reached only via
  SubqueryExpr at the TableExpr layer; the SubqueryExpr arm
  returns nil (preserved).
- #29 (filter-effect mismatch): isConstantLit enumerates 8 omni
  literal types — pre-omni's ast.ValueExpr interface check.
  Unit-tested for completeness.

NEW unit tests (utils_test.go):
- TestOmniIsConstantLit: 8 literal types + ColumnRef/BinaryExpr/nil
  negatives
- TestOmniIsDDLStmt: 12 DDL types + UPDATE/DELETE/INSERT/SELECT/nil
  negatives

These serve as the primary regression surface for the migration —
the advisor's main behavior is gated on EnablePriorBackup which the
standard test fixture driver gates on. The orphan fixture
test/statement_prior_backup_check.yaml is left untouched (rule was
renamed STATEMENT_→BUILTIN_ at some point; fixture file wasn't
renamed; never wired into tidb_rules_test.go). Future ticket to
properly test-cover this advisor.

Divergence from mysql analog: mysql's prior_backup_check has been
modernized to use mysqlparser.ExtractTables + per-table DML-mixing
detection (~120 LOC). The tidb pre-omni logic is the OLDER shape
(unique-WHERE short-circuit + single mixed-DDL emission). This
migration preserves the older tidb logic exactly per invariant #7.
Future feature ticket: align tidb prior_backup_check with mysql's
modernized version (out of Phase 1.5 scope).

Migration count: 46 → 47 omni-migrated. **All migratable
advisors complete.** 2 remaining: 1 Class III (dml_dry_run,
blocked on omni BATCH grammar) + utils.go bridge. Next: PR 1.5.N+1
dispatcher flip per plan §1.5.N+1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): reshape builtin_prior_backup_check to mysql-aligned modernized logic (batch 19 reshape)

Pre-merge peer review challenged the initial mechanical port:

> "If we're implementing it, why don't just go straight for the
>  modernized version if it's more correct?"

Original framing (invariant #7 by-the-book) was overly conservative:
the pre-omni tidb logic wasn't being tested (orphan fixture, not in
test runner), so "preserving" untested behavior preserves unknowns.
Modernizing to mysql's shape is the right call when:
  1. Phase 1.5 closes after this batch — deferring leaves tidb stale.
  2. Mysql's per-table DML-type mixing is more semantically accurate
     for backup feasibility than the pre-omni count-cap heuristic.
  3. The migration is the right place to align; a future-ticket-punt
     is an anti-pattern.

Reshape changes:

- Replaced unique-WHERE machinery (omniUpdateForOneTableWithUnique,
  omniHasUniqueInWhereClause, omniExtractColumnsInEqualCondition,
  omniIsConstantLit) with per-table DML-type mixing detection
  matching mysql's modernized shape.
- Added size cap check (StatementsTotalSize > MaxSheetCheckSize)
  matching mysql.
- Fixed omniIsDDLStmt enumeration per peer's verified pingcap
  DDLNode set (22 implementers in tidb v8.5.5; 18 mappable to omni
  today, 4 deferred Tier-4 grammar). Initial PR had 14 false-
  positives (User/Role/Function/Trigger/Event/Tablespace/Server
  stmts + DropView + AlterDatabase — pingcap doesn't classify these
  as DDL) and 5 false-negatives (OptimizeTable, RepairTable,
  ResourceGroup trio).

Behavior matrix (vs pre-omni tidb):

  Input                                    pre-omni       new
  -----------------------------------     ----------     ----------
  UPDATE t × 6 (no unique-WHERE)          fires          skips
  UPDATE t WHERE id=5 × 10                skips          skips
  UPDATE t; DELETE FROM t                 skips          FIRES (new)
  Statements > MaxSheetCheckSize          skips          FIRES (new)
  CREATE TABLE; UPDATE                    fires          fires
  Missing bbdataarchive db                fires          fires

Test coverage closes peer's third concern:

- New TestTiDBPriorBackupCheckAdvisor (6 cases) modeled on mysql's
  TestMariaDBPriorBackupCheckAdvisor pattern: standalone test
  passing ListDatabaseNamesFunc + EnablePriorBackup context
  directly. Covers all 4 advice paths (DDL mixing, missing backup
  db, per-table DML mixing, size cap exceeded) + 2 negative cases
  (single UPDATE, UPDATE+DELETE on different tables).
- TestOmniIsDDLStmt expanded from 16 to 36 cases — adds 18 positive
  (full pingcap DDLNode set restricted to omni) + 18 negative
  (DropView, AlterDatabase, User/Role/Function/Trigger/Event/
  Tablespace/Server stmts + DML + nil). The NEGATIVE cases are
  load-bearing — without them a future refactor could re-add
  over-inclusions silently.

Removed:
- Orphan fixture file test/statement_prior_backup_check.yaml. The
  rule type was renamed STATEMENT_→BUILTIN_ at some point; fixture
  file name didn't follow; content was stale (mentioned "Found N
  DELETE, M UPDATE" — neither pre-omni nor mysql analog phrasing).
  Test coverage now comes from the dedicated TestTiDBPriorBackupCheckAdvisor
  function (mysql pattern), not the fixture driver.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(advisor/tidb): pin advice-code change on missing-backup-database in behavior matrix (cumulative #30)

Peer review caught: the reshape commit changed the advice code for
"Need database bbdataarchive ... does not exist" from
code.BuiltinPriorBackupCheck → code.DatabaseNotExists (mysql-aligned),
but the file-level behavior matrix listed this row as "pre: fires →
new: fires" without noting the code change. The numeric code is a
public-API surface — downstream consumers (UI filtering, automation
parsing) may key off the value; a contract change at this boundary
deserves explicit acknowledgment.

This commit makes the delta explicit in the file-level docstring.
No runtime change. Plan-doc cumulative #30 (added in same batch)
captures the full receipt set including the new invariant #7
caveat that permits this kind of mysql-aligned modernization when
pre-omni behavior is untested.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): SET-clause target extraction + canonicalized grouping key (Codex P2 ×2, cumulative #30 algorithm corrections)

Codex caught two real bugs in the batch 19 reshape:

1. UPDATE table refs were taken from the full Tables list, including
   JOIN-only read-only tables. For `UPDATE t1 JOIN t2 ON ... SET
   t1.col = ...; DELETE FROM t2 ...` this produced a false mixed-DML
   warning on t2 — only t1 was actually mutated.

2. Grouping key `fmt.Sprintf("%s.%s", db, table)` used raw strings,
   splitting equivalent references into different buckets:
   - Unqualified `tech_book` vs qualified `mydb.tech_book`
   - Case-different `tech_book` vs `Tech_Book`
   These false-negatives weakened the safety gate.

Codex-fix-1: SET-clause-based target extraction.

New file-local helpers:
- omniBuildTableAliasMap(tables) builds an alias→base-table lookup
  from update.Tables (walks TableRef + JoinClause; SubqueryExpr
  contributes nothing per cumulative #26).
- omniExtractUpdateTargets(setList, aliasMap) walks SET assignments;
  qualified `t.col = ...` resolves via aliasMap; unqualified `col =
  ...` falls through only when aliasMap has exactly 1 entry (single-
  target UPDATE); otherwise skipped (multi-target UPDATE with
  unqualified SET is ambiguous without schema info — false-negative
  is preferable to false-positive at this boundary).

The UpdateStmt arm now uses omniExtractUpdateTargets. The DeleteStmt
arm still uses Tables directly (verified omni parsenodes.go:123 —
DELETE's Tables is the mutation target set; Using[] is filter-only).

Codex-fix-2: canonicalized grouping key.

The per-table grouping key now lowercases both segments AND
substitutes checkCtx.CurrentDatabase when the table reference has
no explicit database qualifier. Equivalent references group into
the same bucket; mixed-DML detection no longer misses on common
authoring patterns.

Regression-direction pins added to TestTiDBPriorBackupCheckAdvisor:
- "UPDATE-JOIN + DELETE-on-joined-table — no false-positive
  (Codex-fix-1)": `UPDATE tech_book JOIN orders SET tech_book.name
  = 'x'; DELETE FROM orders WHERE order_id = 5;` — must NOT fire
  mixed-DML on orders (only tech_book is the UPDATE target).
- "case-insensitive grouping — Tech_Book ≡ tech_book
  (Codex-fix-2)": `UPDATE tech_book ...; DELETE FROM Tech_Book ...;`
  — must fire mixed-DML (same logical table).

Test count: 6 → 8 cases.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): aliased single-table UPDATE + DBSchema.Name default DB (Codex P1 + P2, cumulative #30 Codex-fix-1b / fix-2 revised)

Two follow-on Codex catches on the previous Codex-fix-1 / fix-2 work:

P1 (Codex-fix-1b): aliased single-table UPDATE bypass.

`omniCollectTableAliases` stored TWO lookup entries per aliased
TableRef (alias + bare name → same base). So `UPDATE tech_book AS t
SET id = 1` produced `len(aliasMap) == 2`. The unqualified-column
branch in `omniExtractUpdateTargets` gated on `len(aliasMap) == 1`,
which failed for aliased single-table UPDATEs → UPDATE target was
dropped → following DELETE on same table not flagged as mixed-DML.
False-negative on the safety gate.

Fix: track distinct base tables separately from lookup-map entries.
Restructured the helper return type from
`map[string]priorBackupTable` to a new `updateTableAliasMap` struct
with two fields: `lookup` (alias-or-name → base; for qualified
resolution) and `distinctBases` (deduplicated FROM-clause base
tables; for single-target detection). The "single-target" gate now
checks `len(m.distinctBases) == 1` — correct regardless of alias
multiplicity.

P2 (Codex-fix-2 revised): qualified vs unqualified split when
CurrentDatabase is empty.

Previous Codex-fix-2 used `checkCtx.CurrentDatabase` as the default
database for unqualified references. But CurrentDatabase is NOT
populated in some review paths — backend/runner/plancheck/
statement_advise_executor.go:168-180 constructs advisor.Context
without setting it. So qualified `db1.tech_book` vs unqualified
`tech_book` split into different buckets (`db1.tech_book` vs
`.tech_book`) and mixed-DML on the same table was missed.

Fix: use `checkCtx.DBSchema.GetName()` as the primary default
(mysql analog pattern via `prepareTransformation(checkCtx.DBSchema.Name, ...)`).
The schema being checked IS the implicit database for unqualified
references; DBSchema is reliably populated across review paths
including plancheck. Fall back to CurrentDatabase, then empty
string, in that order.

Resolution moved to extraction time (omniExtractDMLTables +
omniBuildTableAliasMap both take defaultDB), so priorBackupTable
records have populated database fields from the start. The
grouping-key loop simplified: no longer needs runtime fallback.

Regression-direction pins added:

- "aliased single-table UPDATE with unqualified SET (Codex-fix-1b)":
  `UPDATE tech_book AS t SET id = 1; DELETE FROM tech_book ...`
  → MUST fire mixed-DML (single base table despite 2 lookup keys).

- "qualified vs unqualified same-table mixing (Codex-fix-2 revised)":
  `UPDATE test.tech_book ...; DELETE FROM tech_book ...`
  → MUST fire (DBSchema.Name default resolves unqualified ref).

Test count: 8 → 10 cases.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): DDL classification + schema-qualified UPDATE target (Codex P1 ×2, cumulative #30 Codex-fix-1c / fix-1d)

Two new Codex P1 catches following the algorithm-corrections wave:

P1 (Codex-fix-1c): DROP VIEW + ALTER DATABASE classified as non-DDL.

Peer's earlier DDLNode analysis counted compile-time
`_ DDLNode = &XxxStmt{}` declarations in pingcap's ast/ddl.go and
concluded DropViewStmt + AlterDatabaseStmt were absent. Empirical
verification (parse + type-assert .(ast.DDLNode)) reveals:

  - `DROP VIEW v` parses as *ast.DropTableStmt (already in our DDL
    list) in pingcap, but omni splits to *ast.DropViewStmt — my
    exclusion of DropViewStmt was the actual regression.
  - `ALTER DATABASE foo COLLATE = ...` has isDDLNode=true via the
    ddlNode struct embedding (ast/base.go:81-88) which the
    compile-time grep missed.

Both added to omniIsDDLStmt; unit test moves both from negative to
positive lists with #30 Codex-fix-1c comment lineage.

P1 (Codex-fix-1d): schema-qualified UPDATE target misattribution.

For `UPDATE db1.tech_book JOIN db2.tech_book ON ... SET
db1.tech_book.id = 1; DELETE FROM db1.tech_book ...`, my
omniBuildTableAliasMap keyed lookup by bare-name only. Two
TableRefs with Name="tech_book" overwrote each other in the lookup
map (db2.tech_book wins). The `SET db1.tech_book.id = 1` resolution
ignored Column.Schema and matched the bare name → got db2.tech_book
(WRONG). Following DELETE on db1.tech_book recorded as DELETE-only;
UPDATE went to db2 → no mixing detected → false-negative on the
safety gate.

Fix: restructured updateTableAliasMap into four resolution paths:

  - bySchemaName: "schema.name" key → base. Used for Column.Schema
    + Column.Table qualified SET LHS.
  - byAlias: alias → base. Aliases are query-unique; unambiguous.
  - byBareName: bare-name → list of bases. Multiple bases on the
    same bare name → ambiguous reference, skip.
  - distinctBases: deduplicated FROM-clause bases for single-target
    fallback (existing Codex-fix-1b semantic preserved).

Resolution dispatch in omniExtractUpdateTargets:
  1. Column.Schema != "" → bySchemaName (disambiguates).
  2. Column.Schema == "" but Column.Table != "" → byAlias first
     (unambiguous), then byBareName (single match only).
  3. Both empty → single-target fallback via distinctBases.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
test count: 10 → 13):

  - "DROP VIEW + UPDATE fires mixed DDL+DML (Codex-fix-1c)"
  - "ALTER DATABASE + UPDATE fires mixed DDL+DML (Codex-fix-1c)"
  - "schema-qualified UPDATE target disambiguation (Codex-fix-1d)":
    full Codex repro, asserts mixed-DML fires on db1.tech_book.

TestOmniIsDDLStmt: 36 → 36 cases (DropViewStmt + AlterDatabaseStmt
moved from negative to positive; net count unchanged).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): schema-aware column resolution for unqualified SET in multi-table UPDATE (Codex P2, cumulative #30 Codex-fix-1e)

The 7th Codex round on this advisor; closes the last remaining
false-negative on the per-table DML-mixing safety gate.

The case: `UPDATE t1 JOIN t2 ... SET status = 1` where `status`
belongs to only one of the joined tables. Without schema-aware
resolution, my prior code (post-Codex-fix-1b/1d) dropped multi-
target unqualified-SET attribution → no UPDATE target recorded
→ following DELETE on the implicit-target table wasn't flagged
as mixed-DML.

The shipping-the-full-modernization decision (cf. cumulative #30
invariant #7 caveat — "modernization is allowed when properly
gated") commits us to mysql-analog correctness. mysql gets this
right via mysqlparser.ExtractTables (parser-level column→table
resolver with full MySQL grammar knowledge). Tidb omni doesn't
have an equivalent helper; this fix uses the advisor's catalog
metadata (checkCtx.DBSchema) instead.

New helper omniResolveUnqualifiedSETColumn walks dbMetadata for
each candidate base in distinctBases and returns the single base
whose schema owns the column. Multiple matches (column exists on
multiple joined tables) → skip (MySQL itself errors on ambiguous
unqualified refs at execution time; safety gate stays silent
rather than guess). Zero matches → skip (column not in catalog;
either truly missing or unknown schema → can't resolve).

Resolution dispatch in omniExtractUpdateTargets (unqualified
branch) now:
  1. Single-target UPDATE (len(distinctBases) == 1) → use that
     base (existing Codex-fix-1b semantic).
  2. Multi-target UPDATE → omniResolveUnqualifiedSETColumn walks
     catalog → if exactly one base owns the column, use it.
  3. Otherwise → skip (preserved conservative default).

Schema-match policy: bases pointing at databases other than
dbMetadata.Name are skipped — we have no catalog info for them.
Common single-database multi-table UPDATEs are fully covered;
cross-database UPDATEs with unqualified SET fall through to skip
(rare; out of scope without multi-database catalog access).

Regression-direction pin (TestTiDBPriorBackupCheckAdvisor case
count: 13 → 14):
  - "unqualified SET in multi-table UPDATE resolves via catalog
    (Codex-fix-1e)": `UPDATE tech_book INNER JOIN orders SET
    customer_name = 'x'; DELETE FROM orders WHERE order_id = 5`.
    customer_name is in MockMySQLDatabase under orders only →
    catalog walk resolves → UPDATE attributed to orders → DELETE
    on orders → mixed-DML fires.

Phase 2 ticket (out of scope here): factor out a reusable
tidb-side ExtractTables analog so future advisors can share the
column-to-table resolution machinery instead of each advisor
walking dbMetadata directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): switch DDL detection to pingcap DDLNode for Tier-4-grammar coverage (Codex P1, cumulative #30 Codex-fix-1f)

Codex caught: omni's parser rejects Tier-4-deferred DDL grammar
(CREATE/ALTER/DROP SEQUENCE, FLASHBACK DATABASE/TABLE). Per
invariant #2, `getTiDBOmniNodes` soft-fails on omni-rejected stmts.
For builtin_prior_backup_check — a safety gate — this silently
drops DDL classification on these valid tidb statements:

  CREATE SEQUENCE seq1 START 1; UPDATE bar SET x=1;
  FLASHBACK TABLE foo; DELETE FROM bar WHERE id=5;

Pre-omni: pingcap parses both, classifies as DDL via DDLNode
interface → mixed-DDL+DML advice fires.
Post-omni: omni rejects SEQUENCE/FLASHBACK → soft-fail skips →
advisor sees only UPDATE/DELETE → no DDL detected → safety gate
silently misses.

Empirical verification (parse-test through omni): all five inputs
(CREATE/ALTER/DROP SEQUENCE, FLASHBACK DATABASE, FLASHBACK TABLE)
fail omni parsing with syntax error.

Fix: DDL detection switches to the `getTiDBNodes` (pingcap) path.
Pingcap's DDLNode interface (via the ddlNode struct embedding in
ast/base.go:81-88) catches all 22 implementers including Tier-4-
deferred grammar. The advisor still uses `getTiDBOmniNodes` for
per-table DML mixing detection (where omni AST shape is required);
DDL detection sidesteps omni's grammar gaps.

Architectural note: this dual-path approach mirrors mysql's analog,
which uses mysqlparser.ExtractTables alongside the omni AST.
Specialized advisors that need both full-grammar coverage AND
omni-specific AST shape will need both paths until omni grammar
parity catches up.

Side effects:

- omniIsDDLStmt helper REMOVED. Eight rounds of Codex catches on
  omni-only DDL enumeration (cumulative #30 Codex-fix-1c through 1e)
  demonstrated the brittle-enumeration anti-pattern; pingcap's
  authoritative DDLNode interface eliminates the maintenance burden.
- TestOmniIsDDLStmt REMOVED. The helper it tested is gone. End-to-end
  coverage of the DDL-detection path moves to TestTiDBPriorBackupCheckAdvisor.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 14 → 16):

  - "CREATE SEQUENCE + UPDATE fires mixed DDL+DML via pingcap path
    (Codex-fix-1f)": `CREATE SEQUENCE seq1 ...; UPDATE tech_book ...;`
    → MUST fire.
  - "FLASHBACK TABLE + DELETE fires mixed DDL+DML via pingcap path
    (Codex-fix-1f)": `FLASHBACK TABLE tech_book TO tech_book_old;
    DELETE FROM orders ...;` → MUST fire.

Both pins exercise the new pingcap-DDLNode resolution path.

Cumulative #30 algorithm-corrections sub-fix lineage now spans
eight sub-fixes:
  1 — SET-clause-based target extraction
  1b — distinct-base gating
  1c — DDL classification (DropView + AlterDatabase)
  1d — schema-qualified UPDATE target lookup
  1e — schema-aware unqualified-SET resolution
  1f — DDL detection via pingcap DDLNode (drops omniIsDDLStmt)
  2 — case-insensitive grouping key
  2-revised — DBSchema.Name default-DB

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): reinstate DML count-cap with multi-table gate (Codex P1, cumulative #30 Codex-fix-1g)

Codex caught: my reshape removed the pre-omni >5 DML feasibility
gate, but the backup transformer at parser/tidb/backup.go:96-110
still has that constraint. Inputs like 6 UPDATEs across different
tables now pass the advisor but fail at runtime backup generation
with "prior backup cannot handle statements on different tables
more than 5".

The reshape framing ("pre-omni count-cap is stale legacy logic")
was wrong here. The count-cap matches the transformer's CURRENT
behavior:

  generateSQL:
    if len(statementInfoList) <= maxMixedDMLCount {  // 5
        return generateSQLForMixedDML(...)  // OK for ≤5
    }
    return generateSQLForSingleTable(...)  // requires single table

  generateSQLForSingleTable:
    for _, item := range statementInfoList {
        if !equalTable(table, item.table) {
            return nil, errors.Errorf("prior backup cannot handle
              statements on different tables more than %d", ...)
        }
    }

Fix: reinstate the count-cap gate after per-table mixing detection.
When `len(dmlRefs) > maxMixedDMLCount`, compute distinct base
tables across all DML references; if more than one, fire advice
matching the transformer's reject condition.

Single-table batches above the threshold are intentionally allowed
— the transformer's single-table path handles them successfully.

Constant `maxMixedDMLCount = 5` reinstated at advisor level (matches
parser/tidb/backup.go:23). Synced via comment for maintenance.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 16 → 18):

  - "6+ DML across multiple tables fires count-cap (Codex-fix-1g)":
    6 UPDATEs across tech_book + orders → MUST fire.
  - "6+ DML on single table — no count-cap advice (Codex-fix-1g)":
    6 UPDATEs on tech_book only → MUST NOT fire count-cap (single-
    table path handles).

Cumulative #30 sub-fix lineage updated to nine sub-fixes:
  1 — SET-clause-based target extraction
  1b — distinct-base gating
  1c — DDL classification (DropView + AlterDatabase)
  1d — schema-qualified UPDATE target lookup
  1e — schema-aware unqualified-SET resolution
  1f — DDL detection via pingcap DDLNode
  1g — count-cap with multi-table gate (matches transformer)
  2 — case-insensitive grouping key
  2-revised — DBSchema.Name default-DB

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(advisor/tidb): tombstone dead omniIsDDLStmt docstring (peer cleanup)

The 34-line spec block for omniIsDDLStmt remained in the file
after Codex-fix-1f removed the function itself. Reads like a
function spec → future readers grep for the function, find the
docstring, look for the body, get confused.

Replaced with a 5-line tombstone pointing to plan-doc cumulative
#30 for the full algorithm-corrections lineage. The durable
explanation belongs in the plan-doc; in-source tombstones should
be brief.

Plan-doc cumulative #30 amended in the same change-set with:
1. Algorithm-corrections lineage (9 sub-fixes named explicitly)
2. Dual-path pattern formalization (pingcap for full-grammar
   coverage; omni for AST-shape-dependent work) — invoked for
   future advisors hitting Tier-4-grammar gaps or transformer-
   constraint matching
3. Known follow-up: maxMixedDMLCount duplication between advisor
   and parser packages, eligible for a small focused refactor PR

No code/behavior change. Peer review's cleanup item 1 closed
(dead-doc-block); item 3 (dual-path-pattern in plan-doc) closed.
Item 2 (constant duplication) deferred per cross-package
structural-decision rationale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): mirror transformer fallback for unresolved unqualified SET (Codex P1, cumulative #30 Codex-fix-1h)

Codex caught: my `omniResolveUnqualifiedSETColumn` returned nil on
multi-match or zero-match cases. The TiDB backup transformer at
parser/tidb/backup.go:539-576's `resolveUnqualifiedColumns` does
the opposite — falls back to all joined tables on either ambiguity.
Net: my advisor approved inputs the transformer rejected at runtime
(same gap shape as Codex-fix-1g).

Empirical verification of transformer behavior:

  // No metadata → fallback to ALL singleTables
  if schema == nil { ... fall back ... }

  for each col:
      resolved := false
      for each singleTable:
          if tableMetadata.GetColumn(col) != nil {
              result[aliasOrName] = true
              resolved = true
          }
      }
      if !resolved {
          // Column not found anywhere → fall back to ALL
          ... fall back ...
      }

Three divergent paths now mirrored:

  - No metadata available → return ALL distinctBases
  - Column found in N≥1 distinct bases → return THOSE N
    (single-match preserved as Codex-fix-1e subset)
  - Column not found in ANY base → return ALL distinctBases

Resolution dispatch updated: any-match attributes via `add(t)`
loop instead of `len == 1` gate.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 18 → 20):

  - "multi-match unqualified SET attributes to all (Codex-fix-1h)":
    `UPDATE tech_book JOIN orders SET name = 'x'; DELETE FROM
    tech_book` — `name` exists on BOTH tables; new fallback
    attributes UPDATE to both → tech_book has UPDATE+DELETE →
    mixed-DML fires.
  - "zero-match unqualified SET falls back to all (Codex-fix-1h
    count-cap path)": three multi-table UPDATEs with non-existent
    column → each contributes 2 distinctBases entries; total >5
    DML refs across distinct tables → count-cap fires.

Codex-fix-1e single-match case remains correct (single-element
return from the helper).

Cumulative #30 algorithm-corrections lineage now spans 10
sub-fixes; the dual-path architecture is settled.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The omni.go/utils.go comments said the pingcap bridge "will be removed once
dml_dry_run migrates." dml_dry_run has now migrated (PR #20467), but the bridge
is retained: advisor_builtin_prior_backup_check uses pingcap AST for DDL
detection on its dual-path (cumulative #30), and the dispatcher's Option B
fallback still yields pingcap *AST for omni-rejected SQL (FLASHBACK/SEQUENCE/
SPLIT — deferred grammar). Removal is gated on those, not on dml_dry_run.

These comments nearly drove an incorrect "delete the *tidbparser.AST arm"
cleanup that would have broken TestGetTiDBOmniNodesSkipsPingcapFallbackAST and
the invariant #2 soft-fail. Comment-only; no behavior change.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>