Make droppedColumns the single source of truth for the dropped-column
set. It now returns []string (current's iteration order); diffColumns
iterates the slice instead of duplicating the loop, and diffTable
materialises the same slice into a map[string]bool for allPartsDropped
lookups in diffIndexes. Behaviour unchanged.

Addresses Copilot PR #23 review #3 inline comment.

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

Seven inline comments, all valid. Fixes:

parser/directive.go
  - Whitespace tolerance: classifyInlineLine() now goes through
    strings.Fields/tokenize() so tabs and runs of multiple spaces
    between KEY/INDEX/UNIQUE/etc. and the name don't defeat the
    kind classifier. (Copilot #1, two comments on the same issue.)
  - Kind-aware extraction: ExtractInlineRenames() returns
    *InlineRenames with separate Columns / Indexes maps plus an
    Unsupported list, so a column and an index that share a name
    (which happens when a KEY auto-names after its first column)
    no longer compete for the same map key. (Copilot #2.)
  - Directives that resolve to constraints / FKs / PRIMARY KEY /
    anonymous FOREIGN KEY / unrecognised line shapes are now
    recorded in Unsupported instead of silently dropped — the
    parser turns them into errors. (Copilot #3.)
  - renameDirectivePattern no longer accepts `.` in the old name;
    the surrounding comment now matches reality (qualified names
    were never plumbed through and are intentionally rejected).
    (Copilot #6.)
  - ExtractInlineRenames skips the leading-comment block (those
    are statement-level, owned by ExtractStmtRenameFrom) so the
    statement-level directive doesn't get re-attached to the
    `CREATE TABLE …` opener line.

parser/parser.go
  - Uses InlineRenames.Columns / .Indexes to attach RenameFrom by
    kind, not by flat lookup. Errors out on Unsupported entries
    (constraint / FK / dangling directive) so a typo or
    mis-positioned directive fails the parse instead of silently
    degrading into a destructive DROP+CREATE.

diff/rename.go
  - Removes the misleading trailing comment in applyColumnRenames
    (the "propagate to indexes" work happens in the caller, not
    here). (Copilot #4.)
  - Adds rewriteFKColumnRefs(), the FK counterpart to
    rewriteIndexColumnRefs: rewrites FK Columns entries from
    old → new after a column rename so fkEqual stays quiet. Without
    this, a plain column rename emitted DROP FOREIGN KEY +
    ADD CONSTRAINT for any FK on the renamed column. (Copilot #5.)

diff/tables.go
  - Calls rewriteFKColumnRefs after applyColumnRenames.

Tests
  - parser/directive_test.go: rewritten for the kind-aware API,
    plus new cases for whitespace tolerance (tab, multi-space),
    column/index name collision, qualified-name rejection,
    constraint-target = unsupported, dangling-directive = error,
    full ParseSQL error wrapping.
  - diff/rename_test.go: new TestDiffRenameColumnAlsoRewritesFKColumns
    covers the FK-rewrite path end-to-end.

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

Four inline comments, all valid; one is a real bug.

diff/rename.go (BUG fix)
  - applyTableRenames now also rewrites (RefDB, RefTable) on every
    other table's FKs that pointed at the old name. Without this, a
    pure table rename (shop.users → shop.members) made every FK on
    referencing tables diff as DROP FOREIGN KEY + ADD CONSTRAINT,
    and would fail to apply under restrictive --allow-drop=foreign_key
    settings even though only the target name changed.
    (Copilot #4.)

parser/directive.go
  - ValidateDirectives now also checks the *syntax* of recognised
    directives, not just the name. Malformed renamed-from lines
    (qualified names like `db.tbl`, missing argument, trailing junk)
    fail at validation time with a clear "malformed" message instead
    of being silently dropped by the extractor and degrading to
    DROP+CREATE. (Copilot #1.)
  - renameDirectivePattern accepts any backtick-quoted blob as the
    old name, mirroring model.Ident's behaviour for reserved words /
    hyphens / etc. Bareword names still take the existing identifier
    grammar. (Copilot #2.)
  - classifyInlineLine detects the unnamed `KEY (col)` /
    `INDEX (col)` form (tokens[1] starts with "(") and returns
    inlineKindUnknown so the directive is reported as unsupported,
    rather than later surfacing as a confusing "target index '(col)'
    not found" error. (Copilot #3.)

Tests
  - parser/directive_test.go: new cases for qualified-name rejection
    at validate time, missing-arg, trailing-junk, backticked reserved
    word in the directive arg, and unnamed `KEY (col)` form being
    flagged as unsupported. The previously existing "qualified name
    silently rejected" test is reframed to document that it's the
    extractor's behaviour while ValidateDirectives now errors loudly.
  - diff/rename_test.go: TestDiffRenameTableAlsoRewritesCrossTableFKRefs
    covers the cross-table FK rewrite end-to-end (renames
    shop.users → shop.members and asserts shop.posts' FK to it stays
    quiet — no DROP FOREIGN KEY, no ADD CONSTRAINT).

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

Five inline + one suppressed comment, all valid.

parser/directive.go
  - ExtractStmtRenameFrom now returns (string, error) and errors when
    more than one renamed-from directive appears in the leading
    comment block — multiple sources is ambiguous and almost always a
    typo, so failing loudly beats silently letting the last one win.
    (Copilot #5.)
  - ExtractInlineRenames detects two `-- myschema:renamed-from` lines
    stacked with no SQL line between them; the first directive (which
    never attached to anything) is now appended to Unsupported instead
    of being silently overwritten. (Copilot #3.)
  - classifyInlineLine handles column lines whose name is backtick-
    quoted with embedded whitespace (`weird name VARCHAR(64)`) via a
    new leadingBacktickedIdent helper. strings.Fields can't tokenize
    these correctly because the name itself contains spaces; the
    helper does a backtick-aware first-identifier parse. KEY / INDEX
    / CONSTRAINT keywords are MySQL reserved tokens and never quoted,
    so a leading backtick can only be a column name. (Copilot #4.)
  - Doubled backticks inside a backticked old-name (MySQL's escape
    for an embedded backtick) remain rejected by renameDirectivePattern;
    the validator's "malformed directive" error is the right surface.
    Documented as out of scope in the leadingBacktickedIdent comment.
    (Copilot #1, #2.)

parser/parser.go
  - rejectMisplacedRenameDirectives() errors when a renamed-from
    directive is extracted but the surrounding statement isn't
    CREATE TABLE (currently the only kind that consumes directives).
    Wired into the AlterTable, CreateView, and default switch arms
    so a directive on a CREATE VIEW etc. fails the parse instead of
    being silently dropped on the floor and degrading the next plan
    into a destructive DROP+CREATE. (Suppressed Copilot comment on
    parser.go:96.)
  - ExtractStmtRenameFrom call now propagates the new error.

Tests
  - parser/directive_test.go: existing call sites updated for the
    new (string, error) signature; new cases for stacked directives,
    multiple stmt directives error, backticked column with embedded
    space, and renamed-from on CREATE VIEW being rejected by ParseSQL.

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

Seven inline comments, all valid; two are real bugs.

parser/directive.go
  - End-of-loop guard in ExtractInlineRenames: a `pending` directive
    whose target line never arrived (statement body ran out) is now
    appended to Unsupported instead of being silently dropped.
    (Copilot #1.)
  - leadingBacktickedIdent comment rewritten: previous wording had
    curly quotes ("…" instead of `…`) and didn't explain MySQL's
    backtick-doubling escape. New comment is precise: doubled
    backticks remain unsupported; the regex correctly rejects them
    so the validator surfaces "malformed directive". (Copilot #2, #3.)

diff/rename.go
  - applyTableRenames / applyColumnRenames / applyIndexRenames now
    short-circuit when RenameFrom equals the desired name. This
    avoids generating `ALTER TABLE x RENAME TO x` (rejected on some
    MySQL versions, no-op on others) for what is almost always a
    user typo. (Copilot #5, #6, #7.)
  - rewriteConstraintColumnRefs: PRIMARY KEY column lists in
    current.Constraints are rewritten old → new alongside the index
    and FK rewrites, so a renamed-PK column doesn't surface as
    DROP+ADD PRIMARY KEY in diffConstraints. CHECK constraints are
    deliberately skipped — their Definition is a free-form
    expression and rewriting it requires a full SQL parser.
    (Copilot #4.)

diff/tables.go
  - Calls rewriteConstraintColumnRefs after applyColumnRenames,
    alongside the existing index / FK rewrites.

Tests
  - parser/directive_test.go: TestExtractInlineRenamesTrailingPendingUnsupported
    covers the end-of-loop pending path.
  - diff/rename_test.go: TestDiffRenameSelfRenameIsNoOp covers the
    table / column / index self-rename guards in one shot.
    TestDiffRenameColumnAlsoRewritesPKConstraint covers the new PK
    rewrite end-to-end (renames `users.old_id` → `users.id` and
    asserts no DROP/ADD PRIMARY KEY appears).

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

Four inline comments, all valid; one is a real bug.

diff/rename.go + diff/tables.go (BUG fix)
  - rewriteCrossTableFKRefCols(): for every desired table whose
    columns carry RenameFrom, walk all *other* current tables and
    rewrite RefCols on FKs that reference (db, table) when the
    referenced column matches one of the renames. Without this, a
    pure column rename on the parent side (`users.id` → `users.user_id`)
    diff'd every referencing FK as DROP+ADD even though MySQL
    updates the parent-side reference automatically. (Copilot #1.)
  - DiffTables runs the cross-table pass after table renames and
    before the modified-tables loop, so per-table diffTable() sees
    a consistent view.

diff/tables.go (doc)
  - DiffTables doc-comment now warns that rename handling mutates
    `current` in place (re-keys after table rename, updates Name /
    Database / FK Columns / RefCols / index parts / PK constraint
    columns). Production callers in diff_all.go build a fresh
    `current` per invocation, so this is fine; tests that share
    fixtures across subtests should clone first. (Copilot #2.)

parser/directive.go
  - ExtractInlineRenames now skips `# ...` line comments and
    single-line `/* ... */` block comments between a directive and
    its target. Multi-line `/* ... */` is still not unwound — a rare
    case in hand-written CREATE TABLE bodies — and is documented in
    the func comment. Without this, a stray `# foo` between the
    directive and the next column line would have been treated as
    SQL and the directive mis-attached or surfaced as
    "target not found". (Copilot #3.)
  - leadingBacktickedIdent comment rewritten *for real* this time
    (the previous attempt left U+201C "left double quotation mark"
    in place where ASCII backticks were intended). The comment now
    describes MySQL's escape rule in plain English without any
    smart-quote characters. (Copilot #4.)

Tests
  - parser/directive_test.go: TestExtractInlineRenamesSkipsBlockAndHashComments
    covers the new comment-skipping logic.
  - diff/rename_test.go: TestDiffRenameColumnAlsoRewritesCrossTableFKRefCols
    renames `users.id` → `users.user_id` while `posts.fk_user`
    references it, and asserts the FK on posts stays quiet (no
    DROP FOREIGN KEY, no ADD CONSTRAINT) — the cross-table RefCols
    rewrite is exercised end-to-end.

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

Three inline comments, all valid.

parser/directive.go
  - anyDirectivePattern now allows optional whitespace between the
    colon and the directive name. A formatting slip like
    `-- myschema: renamed-from old` previously didn't even match the
    pattern, so the directive was silently ignored. The per-directive
    regex (renameDirectivePattern) stays strict, so such lines turn
    into "malformed directive" errors at validation. (Copilot #1.)

diff/rename.go
  - applyTableRenames / applyColumnRenames / applyIndexRenames each
    pre-validate that no two desired entries declare the same
    RenameFrom source via new duplicate*RenameSource helpers. Today
    a stray duplicate produced a confusing "source ... not found"
    error after the first rename mutated current; now it surfaces
    a dedicated "source ... is referenced by multiple ..." error
    before any mutation. (Copilot #2.)

AGENTS.md
  - The "in scope" entry for renamed-from now correctly states that
    `RENAME INDEX` is MySQL 5.7+, only `RENAME COLUMN` is 8.0+.
    Project baseline is 8.0 (INVISIBLE indexes, CHECK constraints),
    so the 8.0-only RENAME COLUMN sits inside that envelope. The
    earlier "all 8.0+" wording was factually wrong. (Copilot #3.)

Tests
  - parser/directive_test.go: TestValidateDirectivesRejectsSpaceAfterColon
    covers the new validator behaviour for `-- myschema: name`.
  - diff/rename_test.go: TestDiffRenameDuplicateSourceErrors covers
    the duplicate-source guard for the column-rename path (table
    and index paths share the same shape and helper).

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

Three inline comments — one is a real ordering bug, two are leftover
edge cases for the line-scan comment skipper.

diff/tables.go + diff_all.go (BUG fix)
  - TableDiffResult gains a RenameStmts bucket. Table renames
    (`ALTER TABLE … RENAME TO …`) now go there instead of into
    Stmts, and diff_all.go schedules them between view drops and
    FK drops. Without this, a migration that both renames a table
    and drops an FK on it would emit:
        ALTER TABLE new_name DROP FOREIGN KEY fk;   -- (FK drops first)
        ALTER TABLE old_name RENAME TO new_name;
    The FK drop runs against new_name which doesn't exist yet, and
    apply errors out. Renames must precede FK drops so subsequent
    ALTERs target an existing name. (Copilot #3.)

parser/directive.go
  - stripLeadingBlockComment helper handles the `/* note */ <sql>`
    shape: a line that starts with a single-line block comment but
    has SQL after `*/`. Both ExtractStmtRenameFrom and
    ExtractInlineRenames now call it. For stmt extraction, hitting
    SQL after the comment stops the leading-block scan (the `*/`
    line is the first SQL line). For inline extraction, the SQL
    remainder is reprocessed as the target line so a pending
    directive can attach to it (or trip sawSQL on the opener).
    (Copilot #1, #2.)

Tests
  - Existing tests using res.Stmts for table renames updated to use
    res.RenameStmts.
  - YAML apply fixtures (rename_table, rename_column, rename_index)
    continue to pass — diff_all.go emits the rename in the expected
    spot in the merged plan.

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

Three inline comments — two on the line-classification flow, one on
backtick-quoted identifier handling.

parser/directive.go
  - reduceLeadingBlocks() iteratively strips zero-or-more leading
    `/* … */` blocks from a line, returning either the remainder or
    a flag indicating the block didn't close on this line. Both
    ExtractStmtRenameFrom and ExtractInlineRenames now feed every
    line through it before classification, so:
      * `/* header */ -- myschema:renamed-from old` (block then
        directive on the same line) is recognised as a directive,
        not lost as "first SQL line".
      * `/* note */ /* note2 */ name VARCHAR(64)` (two leading
        blocks) reduces correctly.
    The old single-shot stripLeadingBlockComment helper that ran
    only after a dedicated `/* …` branch is removed in favour of
    this unified reduce-then-classify flow. (Copilot #1, #2.)

  - classifyInlineLine grew a backtick-aware fallback path. Index
    and constraint names that are backtick-quoted AND contain
    whitespace (e.g. `KEY \`weird name\` (col)`) get split by
    strings.Fields and were mis-classified, surfacing as
    "target index not found". Two new helpers run before the
    Fields-based path:
      * backtickedNameAfterPrefix(line, prefixes) tries to match
        any of the given keyword prefixes followed by a backticked
        identifier and returns the unquoted name.
      * stripUntilAfterBacktickedName lets the CONSTRAINT branch
        peek past the backticked name to detect a trailing `UNIQUE`
        and route to inlineKindIndex.
    These cover KEY / INDEX / UNIQUE [KEY|INDEX] /
    FULLTEXT [KEY|INDEX] / SPATIAL [KEY|INDEX] / CONSTRAINT shapes
    with backticked-with-whitespace names. (Copilot #3.)

Tests
  - parser/directive_test.go:
    * TestExtractStmtRenameFromBlockCommentBeforeDirectiveSameLine
    * TestExtractInlineRenamesBlockCommentBeforeDirectiveSameLine
    * TestExtractInlineRenamesBacktickedIndexNameWithSpace (KEY,
      UNIQUE KEY, and CONSTRAINT-with-CHECK forms)

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

Four inline comments. Two typo nits, two flow consistency fixes.

Typos
  - parser/directive.go:415 — comment for stripUntilAfterBacktickedName
    contained a curly close-quote (\`name\“) instead of a backtick.
    Replaced. (Copilot #1.)
  - diff/rename_test.go:390 — comment said "RenameFrm"; should be
    "RenameFrom". Fixed. (Copilot #2.)

parser/directive.go (flow consistency)
  - ValidateDirectives now goes through the same
    reduceLeadingBlocks + multi-line block state path the extractors
    use, so a malformed directive after `/* header */` or after
    `*/` on the same multi-line block-close line still errors at
    validation time. Without this, the validator and extractor
    disagreed on what counts as a "directive line", letting bad
    directives slip past validation and be silently ignored.
    (Copilot #3.)
  - ExtractStmtRenameFrom and ExtractInlineRenames re-process the
    suffix after a multi-line `*/` close on the same line, so
    `*/ -- myschema:renamed-from old` still attaches. Previously
    the code cleared inBlock and `continue`'d, dropping the
    directive. (Copilot #4.)

Tests
  - TestValidateDirectivesAfterLeadingBlockComment
  - TestValidateDirectivesAfterMultiLineBlockClose
  - TestExtractStmtRenameFromMultiLineBlockCloseSameLineAsDirective
  - TestExtractInlineRenamesMultiLineBlockCloseSameLineAsDirective

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

Two remaining inline comments (the other two were already addressed
in earlier commits of this branch):

test/scenario/helper.sh
  - The default `_base_dsn` no longer hardcodes 127.0.0.1:3306; it's
    now built from MYSQL_USER / MYSQL_HOST / MYSQL_PORT, so the
    mysql CLI used by setup_db and the myschema driver target the
    same instance even when only MYSQL_PORT is overridden.
    Reordered the var-init block so MYSQL_HOST / MYSQL_PORT /
    MYSQL_USER are resolved before _base_dsn references them.
    (Copilot #2.)

Makefile
  - test-mysql9 / clean-schema-mysql9 stop hardcoding the full DSN
    and just delegate to the existing `test` / `clean-schema`
    targets with MYSQL_PORT=3307. The MYSCHEMA_TEST_DSN template at
    the top of the Makefile is recursively expanded, so the new port
    flows through to the DSN and any MYSQL_HOST / MYSQL_USER
    customisation carries with it. (Copilot #3.)

Verified locally: `make test-mysql9` and `make test-scenario
MYSQL_PORT=3307` both pass against the running 9.x compose service.

Copilot #1 (Makefile `:=` overriding env) was fixed in 0320bfc.
Copilot #4 (compose.yaml comment about profile) was updated when
the profile itself was removed in 1f51540.

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

Three inline comments:

Makefile + AGENTS.md
  - test-mysql9 now overrides both MYSQL_PORT and MYSCHEMA_TEST_DSN
    explicitly. With only MYSQL_PORT overridden, a caller who already
    has MYSCHEMA_TEST_DSN set in their environment would silently hit
    the wrong port (the `?=` template at the top of the Makefile
    skips the recompute when MYSCHEMA_TEST_DSN is already set).
    Setting both keeps the target self-contained.
  - AGENTS.md doc updated to match: explicit on overriding both, and
    notes the rationale.
  - (Copilot #2, #3.)

PR description (no code change)
  - The PR body still claimed mysql9 was profile-gated; that profile
    was removed earlier in the branch and the body was stale.
    Updated via `gh pr edit` to match the current always-on
    behaviour. (Copilot #1.)

Verified: `make test-mysql9` passes against the 9.4 compose service
with no MYSCHEMA_TEST_DSN preset and with one preset to 3306 (the
explicit override now takes priority).

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

Five inline comments, all valid.

helper.sh
  - assert_contains / assert_not_contains now capture stderr too
    (`2>&1`) so a command that prints the relevant text to stderr
    doesn't cause a false negative. (Copilot #1, #2.)
  - Both helpers explicitly validate the invocation: missing `--`
    separator, no command before `--`, or no substring after `--`
    each fail with a clear message instead of letting bash run a
    malformed command and produce an opaque "command not found"
    or hang on stdin. (Copilot #1, #2.)
  - Sanity-checked all four validation paths (`missing sep`,
    `missing cmd`, `missing substring`, `stderr capture`) by
    sourcing helper.sh in a one-shot bash session.

dump_roundtrip.test.sh
  - The dump-failure and apply-failure branches now `summary; exit 1`
    after `fail`, instead of falling through to the next step. The
    cascading errors that followed obscured the original failure;
    failing fast keeps the scenario log focused on the root cause.
    (Copilot #3, #4.)

evolution.test.sh
  - Header comment said "ADD COLUMN with DEFAULT" but the fixture
    drops the DEFAULT (workaround for the catalog default-
    normalisation drift documented in TODO.md). Comment now matches
    reality and points at the TODO entry. (Copilot #5.)

`make test-scenario` (mysql 8.0) — all 22 steps still pass.

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

Three inline comments, all valid.

helper.sh
  - myschema_dump no longer merges stderr into stdout. Callers
    typically redirect stdout to a .sql file (dump_roundtrip
    re-applies the dump), and any stderr text would otherwise be
    embedded in the dump and break re-apply. Plan / apply wrappers
    keep `2>&1` because their callers run them through `out=$(...)`
    and want both streams. (Copilot #1.)

evolution.test.sh
  - Step "06 DROP COLUMN + auto-index" only asserted the plan
    contains DROP COLUMN; if the column-attached index suppression
    regressed and DROP INDEX users_display_name_idx leaked through,
    apply would still succeed (because MySQL has already auto-removed
    the index, but `Error 1091` would surface — actually a real
    failure). Add an explicit assert_not_contains BEFORE run_step
    so the suppression behaviour itself is pinned in plan output,
    independent of apply outcome. (Copilot #2.)

filter.test.sh
  - Step 01's comment claimed `--include=users` hides BOTH sessions
    and logs, but the assertion only checked sessions. Split into
    two explicit assert_not_contains so both filter targets are
    covered. (Copilot #3.)

`make test-scenario` (mysql 8.0) — all 24 steps pass.

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

Copilot review #3 follow-up nit: the helpers only consumed `$1`
after the `--` separator and silently ignored anything after it. So
an unquoted multi-word substring (`-- DROP TABLE foo`) would only
search for "DROP" — passing/failing on the wrong target without any
warning.

Add a `$# -ne 1` check after parsing the separator so a missing
quote fails loudly with a clear message that points at the fix
("too many arguments after '--' (got N); quote the substring").
Sanity-checked by sourcing helper.sh in a one-shot bash session
and confirming the error fires.

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

Five Copilot inline comments on PR #31, all valid:

helper.sh
  - assert_contains / assert_not_contains validate $# >= 1 before
    referencing $1 / shifting. Under `set -u` (which the scenario
    runner enables), a no-arg call would otherwise abort with
    "unbound variable" instead of falling into the helper's own
    fail() path. (Copilot #1, #2.)
  - Replace `echo "$out" | grep ...` with
    `printf '%s\n' "$out" | grep -qF -- ...`. echo can mangle
    output that starts with `-n` / contains escape sequences;
    printf is faithful, and the `--` after `-qF` keeps grep from
    treating a leading `-` in the substring as an option.
    (Copilot #3, #4.)

test/scenario/dump_roundtrip.test.sh
  - `mktemp -d` without a template is GNU-specific; macOS / BSD
    requires one. Pass an explicit template under `$TMPDIR` so the
    scenario runs portably. (Copilot #5.)

`make test-scenario` (mysql 8.0) — all 24 steps still pass.
Sanity-checked the `set -u` path by sourcing helper.sh in a fresh
bash session and calling `assert_contains` with no args; it now
returns via fail() instead of aborting.

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

Three Copilot inline comments on PR #31:

helper.sh
  - `_assert_substring` now rejects an empty needle. `grep -qF -- ""`
    always matches, so `assert_contains ... -- ''` would silently
    pass (and `_not_contains` would always fail) regardless of
    output. Almost always a caller bug — fail loudly with
    "substring is empty". (Copilot #1.)

testdata/rename/04_rename_index.sql
  - Header comment claimed the UNIQUE index was "previously-
    renamed", but step 03 only renames a column; both indexes are
    actually renamed in step 04. Reword to spell out both
    renames explicitly. (Copilot #2.)

filter.test.sh
  - Step labels jumped 02 → 03 mid-block. Renumber so the two
    `--exclude=log*` checks are 02a/02b and the two no-filter
    checks are 03a/03b, matching the comment grouping. (Copilot #3.)

`make test-scenario` (mysql 8.0) — all 25 steps still pass.

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

Three inline comments, plus a refinement uncovered while writing the
new test:

catalog/tables.go + catalog/catalog_test.go (typography fix)
  - Replace stray U+201D ("right double quotation mark") with the
    ASCII `''` literal in the doc comments. Editor / clipboard
    contamination from the previous round; the SQL examples now
    read correctly. (Copilot #1, #2.)

catalog/tables.go (BINARY/VARBINARY split)
  - The original `columnTypeAllowsEmptyStringDefault` excluded
    both BINARY and VARBINARY together. Verifying with information_
    schema showed only fixed-width BINARY surfaces its empty
    default as a hex literal (`0x` for the degenerate case;
    `0x000000…` for non-zero N). VARBINARY (variable-length)
    surfaces the bare empty string just like VARCHAR, so it
    round-trips cleanly via the empty-string path. Narrow the
    exclusion to BINARY only and add VARBINARY to the supported
    list. The function now checks `varbinary` before `binary` so
    HasPrefix doesn't let one match the other.

catalog/catalog_test.go (test for the BINARY exclusion)
  - TestColumnDefaultBinaryEmptyStringIsHex pins the documented
    BINARY behaviour and catches a future refactor that
    accidentally normalises the hex literal to ''. The companion
    TestColumnDefaultEmptyStringNormalisation gains a VARBINARY
    column to exercise the type's now-supported path. (Copilot #3.)

`make test` (8.0) — pass; `make test-scenario` (8.0) — pass.

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

Four Copilot inline comments on PR #33:

catalog/tables.go (doc) + TODO.md
  - Doc and TODO still listed VARBINARY alongside BINARY as "needs
    its own normalisation". The previous round actually moved
    VARBINARY into the supported set (it surfaces its DEFAULT '' as
    the bare empty string, so the existing path round-trips it
    cleanly). Update the function-doc and TODO to spell out that
    only fixed-width BINARY remains broken. (Copilot #1, #2.)

testdata/apply/empty_string_default_no_drift.yml
  - Add a VARBINARY(8) column to the no-drift fixture so the new
    branch is exercised through plan + apply + drift-check, not
    just the catalog unit test. (Copilot #3.)

catalog/tables.go + catalog/catalog_test.go (typography)
  - Bulk replace stray U+201D ("right double quotation mark") with
    ASCII `''` across both files. The previous round's Edit tool
    ate the quote characters; verified the bytes with a follow-up
    grep. (Copilot #4.)

`make test` (8.0) — pass; `make test-scenario` (8.0) — pass.

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

do drift

Previous wording claimed "Integer display widths don't surface from
either side on MySQL 8.0+, so no explicit stripping is needed."
That's wrong, and Copilot caught it. Verified against the live
docker-compose MySQL 8.0:

- catalog (`information_schema.COLUMNS.COLUMN_TYPE`) strips the
  display width — `INT(11)` stored in DDL surfaces as `int`.
- vitess preserves whatever the user wrote in desired SQL —
  `INT(11)` round-trips through `sqlparser.String` as `int(11)`.

myschema doesn't normalise either side, so `INT(11)` in desired SQL
shows up as a `MODIFY COLUMN` drift on every plan. The right
mitigation is on the user side: write the bare type name (`INT`,
`BIGINT`, …) in desired SQL.

Rewrite the bullet to call out the asymmetry directly and mention
the `ZEROFILL` exception (MySQL preserves it in `COLUMN_TYPE` —
verified to come back as `int(5) unsigned zerofill` — so it is
round-trip-safe as long as the implicit `UNSIGNED` is also written).

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

Two nits in this pass:

#1 model/charset.go map "missing cp1252" — false alarm.
Same shape as Copilot's earlier `cp874` / `latin9` claim: the
identifier doesn't actually exist in MySQL 8.0+. Verified live on
8.0 + 9.4: information_schema.CHARACTER_SETS lists cp1250, cp1251,
cp1256, cp1257 — there is no cp1252. The map already covers all 41
stock charsets (verified via TestCharsetDefaultCollationsCoverServer
on both servers). No code change.

#2 diff/tables.go docstring inaccuracy — fixed.
`tableCharsetCollationSQL`'s docstring claimed the emitted DDL was
always `ALTER TABLE … DEFAULT CHARSET=… COLLATE=…`, but the
function only spells out the clauses the desired side actually set
(either / both, depending on what differed). Reword the docstring
to match: `… DEFAULT CHARSET=…` alone, `… COLLATE=…` alone, or both
together — three shapes covered.

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

…d 4 #3 fix)

Round 4's partitionDefEqual fix strips the partition name from
the formatted-body comparison so a `pAB → PAB` desired SQL
change doesn't produce different formatted strings and trip a
spurious REORGANIZE. The fix landed without an explicit
regression test — pin it now with a plan fixture that asserts
the case-only diff produces no executable SQL.

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

#1 Pin the RANGE COLUMNS REORGANIZE end-to-end. The plan-only
   fixture (partition_value_change_reorganize_range_columns.yml)
   covered the generator side, but nothing confirmed MySQL accepts
   `REORGANIZE PARTITION ... INTO (...)` with tuple boundaries or
   that the rewritten boundaries round-trip cleanly. Add the
   matching apply / verify_no_drift fixture.

#2 The case-only partition-name no-op already has a regression test
   (`partition_name_case_only_diff_no_op.yml`, commit 4c6998f).

#3 Pin the per-partition *option*-only diff (here: COMMENT-only)
   end-to-end. partitionDefEqual delegates to
   `parser.FormatPartitionDefinition`, which preserves COMMENT /
   MAX_ROWS / TABLESPACE / etc., so any byte-different definition
   already routed through the REORGANIZE branch — but no fixture
   pinned that MySQL accepts the generated statement or that the
   rewritten options round-trip. New apply fixture
   `partition_per_partition_option_change_reorganize.yml` covers
   both. LIST avoids the RANGE upper-bound cascade so the
   REORGANIZE only touches the one slot whose option actually
   changed.

#4 / #5 / #6 Widen the Coverage / Not-yet-implemented sections of
   AGENTS.md (in scope + v1 cuts) and the TODO.md "Already shipped"
   block from "pure value-change (same names, only `VALUES`
   differs)" to "per-partition definition rewrite ... when both
   sides have the same partition names in the same order — covers
   `VALUES …` boundary tweaks plus COMMENT / MAX_ROWS /
   TABLESPACE / other per-partition option changes that round-trip
   through vitess's PartitionDefinition formatter". Brings the
   docs in line with what diffPartitions actually does (and with
   what CAVEATS.md "Per-partition definition change" already
   describes).

#7 Fix the misleading comment in partitionNameListEqual.
   `parser.NormalizePartitionOption` only lower-cases function /
   column-reference identifiers inside the partition expression —
   it does NOT lower-case partition names. The case-insensitive
   `EqualFold` here is needed precisely because the round-trip
   preserves whatever case the user wrote on the partition name.

Three doc / wording nits:

#1 / #2 `diff/partitions.go` had three places spelling the
   emitted SQL as `REORGANIZE pmax INTO ...` (omitting the
   `PARTITION` keyword). MySQL's actual grammar — and the
   string we emit — is `REORGANIZE PARTITION pmax INTO ...`.
   Updated all three sites (overview comment, inline outcome
   listing, extras-internal-dup explainer) to match. Found
   one more occurrence the reviewer didn't flag and fixed
   it too while I was at it.

#3 PARTITIONING.md "Catch-all interior insert (RANGE)"
   trigger envelope listed the extras-uniqueness rule as
   "names don't reuse any name already present in catalog".
   The round-1 fix to PR #56 extended that to also reject
   duplicates within the extras slice itself, but the doc
   bullet wasn't updated. Reworded to "unique — both vs
   catalog names AND within the extras slice itself" so the
   doc matches the implementation.

Grammar: "apply once" → "applies once".

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

Round 1 review on PR #75 caught eight issues; this commit addresses
all of them.

1. (review #1) loadPreSQL treated any non-empty PreSQL string as
   "set", including whitespace-only values. Whitespace-only env
   var (`MYSCHEMA_PRE_SQL=`) would trigger the mutually-exclusive
   error against a legitimate --pre-sql-file, and skip the no-op
   short-circuit. Fix: TrimSpace both fields before deciding "set".

2. (review #2) loadPreSQL read --pre-sql-file with os.ReadFile,
   which doesn't support the repo's existing `-` for stdin
   convention used by parser.ReadSQLFile (the desired-SQL file
   args already accept it). Fix: route through parser.ReadSQLFile
   so `--pre-sql-file=-` works.

3. (review #3 + #4) runPreSQL was invoked AFTER connect(), so
   flag-validation errors (both flags set, missing file) could be
   masked by a downstream connection failure, and the client
   opened a DB connection it then threw away. Split into:
   - loadPreSQL: validate / read, no DB contact (called BEFORE connect)
   - execPreSQL: run on conn (called AFTER connect)
   apply.go and plan.go updated accordingly.

4. CLI-level mutual exclusion: tag both PreSQL and PreSQLFile with
   kong's `xor:"pre-sql"` so kong rejects the both-set case at
   parse time before our code sees it. The runtime check in
   loadPreSQL stays in place for programmatic API callers
   (Apply / Plan invoked from Go without going through kong).

5. (review #5 / #6 / #7 / #8) Test reshuffle. The original
   suite over-relied on success-only smoke tests that would have
   silently passed even if pre-SQL were skipped:
   - TestApply_PreSQLString and TestApply_PreSQLMultiStatement
     only checked NoError.
   - TestApply_PreSQLAppliesToSession claimed to be the
     "strongest behavioural pin" but probed nothing — the
     connection it would have probed is closed by Apply's defer.
   - TestPlan_PreSQLString same issue.
   Fix: load-bearing pins now feed INVALID pre-SQL and assert
   the apply / plan aborts with a wrapped "pre-sql" error
   containing the exact failing piece. The multi-statement
   split test now uses a payload with an invalid SECOND piece
   and asserts the error references that piece exactly (proves
   the split happened — without splitting, the driver would
   reject the whole concatenated string with a different error).

6. YAML migration: the new harness field `pre_sql` lets
   inline-payload tests live as testdata/apply/pre_sql_*.yml and
   testdata/plan/pre_sql_*.yml fixtures (3 + 1 = 4 new fixtures).
   File-related and programmatic-API tests stay in Go because
   the YAML harness has no `pre_sql_file` field (would need
   fixture-relative path handling, out of scope).

Coverage is unchanged at the package level; loadPreSQL stays at
100% and execPreSQL is exercised by all the real-execution paths.

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

PR #75 round-2 review #3: --pre-sql-file=- and a `-` desired file
argument both read stdin; the second read would hit EOF and
silently truncate either the pre-SQL or the desired SQL. loadPreSQL
now takes the desired file list and fails fast with an explicit
error on the conflict, before any actual stdin read.

Reviews #1 / #2 (USE other_db redirect) deliberately NOT addressed
per direction "USEは拒否しない。ユーザー責任". Pre-SQL is
operator-trusted: myschema runs the SQL as-is without scrubbing,
so a stray USE, destructive DML, or session-state corruption is
the operator's responsibility. AGENTS.md gains a paragraph stating
this explicitly so future readers understand the trust model.

New regression: TestApply_PreSQLStdinConflict pins the stdin-conflict
detection. The pre-existing TestApply_PreSQLFile / FileMissing /
EmptyFile / BothSetError stay as Go tests; YAML harness still
covers the inline / failure / multi-statement paths.

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

- writeDumpSplit error wraps now use %q instead of %s for the path
  field so whitespace, control characters, or other shell-noise in
  a directory name comes through unambiguously: `dump: mkdir
  "/tmp/has space": ...` instead of `dump: mkdir /tmp/has space: ...`.
  Three sites updated: mkdir, table-side WriteFile, view-side
  WriteFile. Existing assertions on substrings ("mkdir",
  "dump: write", "v.sql", "t.sql") survive — `%q` wraps the value
  in quotes but the path content stays in the rendered error.
- TestDump_SplitFlagName splits its two Parse() calls into separate
  t.Run subtests, each with its own kong.Parser and destination
  struct. kong's Parse mutates the destination, and reusing one
  parser across two Parse() calls is technically supported today
  but couples the test to that semantic. Mirroring the pre-sql
  tests' fresh-parser-per-case pattern keeps the regression test
  robust against a future kong upgrade that tightens the contract.

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

Address Copilot review #3 on PR #77:

- Comment in testdata/apply/change_primary_key.yml claimed MySQL
  "can't combine [DROP PK + ADD PK] in a single statement when the
  column set changes." That's wrong on myschema's MySQL 8.0+ baseline:
  the parser explicitly accepts `DROP PRIMARY KEY` followed by
  `ADD PRIMARY KEY` in one alter_specifications list as an atomic
  replacement. New fixture testdata/apply/bulk_alter_pk_replace.yml
  pins that the combined form applies cleanly and round-trips empty
  (verify_no_drift: true) — so the AGENTS.md / CAVEATS.md combinable
  list correctly includes PK constraints under --bulk-alter.
  change_primary_key.yml's comment now describes the two-statement
  default-mode shape it actually pins, and points at the bulk_alter
  variant.
- Plan-side fixture comment in testdata/plan/bulk_alter_combine_with_index_add_separate.yml
  said "Three column adds" but the YAML adds four (a, b, c, email).
  Off-by-one in the description; corrected.

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

Address Copilot review #3 on PR #77:

- Comment in testdata/apply/change_primary_key.yml claimed MySQL
  "can't combine [DROP PK + ADD PK] in a single statement when the
  column set changes." That's wrong on myschema's MySQL 8.0+ baseline:
  the parser explicitly accepts `DROP PRIMARY KEY` followed by
  `ADD PRIMARY KEY` in one alter_specifications list as an atomic
  replacement. New fixture testdata/apply/bulk_alter_pk_replace.yml
  pins that the combined form applies cleanly and round-trips empty
  (verify_no_drift: true) — so the AGENTS.md / CAVEATS.md combinable
  list correctly includes PK constraints under --bulk-alter.
  change_primary_key.yml's comment now describes the two-statement
  default-mode shape it actually pins, and points at the bulk_alter
  variant.
- Plan-side fixture comment in testdata/plan/bulk_alter_combine_with_index_add_separate.yml
  said "Three column adds" but the YAML adds four (a, b, c, email).
  Off-by-one in the description; corrected.

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