Skip to content

Add TODO.md#2

Merged
winebarrel merged 1 commit into
mainfrom
add-todo
May 2, 2026
Merged

Add TODO.md#2
winebarrel merged 1 commit into
mainfrom
add-todo

Conversation

@winebarrel

Copy link
Copy Markdown
Owner

Summary

  • Extract the "Not yet implemented" callouts from AGENTS.md into a top-level TODO.md checklist, grouped by area (object coverage, type fidelity, diff ordering, CLI features, directives, drop policy, tests, docs, build/release, cleanup).
  • Adds implementation-level gaps surfaced during the bootstrap that aren't in AGENTS.md: loose CHECK normaliser, default-value parse-tree comparison, --with-tx no-op flag, generated-column expression compare, column position support, etc.

Test plan

  • cat TODO.md renders as expected
  • No source changes; go build ./... and go test ./... unaffected

🤖 Generated with Claude Code

Capture the cuts called out in AGENTS.md plus implementation-level gaps
(default normalisation, CHECK normalisation, --with-tx no-op, etc.) so
future contributors have a single checklist instead of grepping AGENTS.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@winebarrel winebarrel merged commit c5e73ef into main May 2, 2026
@winebarrel winebarrel deleted the add-todo branch May 2, 2026 01:47
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
Two inline comments, both real bugs.

parser/directive.go
  - ExtractStmtRenameFrom now also skips `#` line comments and
    single-line `/* … */` block comments in the leading block.
    Previously it stopped at the first non-`--` line, so a stray
    `# header` or `/* generated */` between such a comment and
    `-- myschema:renamed-from` defeated the scan: the validator
    saw the directive as known, but the extractor never reached it,
    and the rename silently degraded into DROP+CREATE. Mirrors the
    same skip logic in ExtractInlineRenames. (Copilot #1.)

diff/rename.go + diff/tables.go
  - rewriteFKColumnRefs() now takes (db, name) and, for any FK
    whose (RefDB, RefTable) matches that table (i.e. self-
    referential), rewrites RefCols too. The cross-table pass in
    DiffTables explicitly skips the renamed table itself, so without
    this self-ref handling a `users.parent_id REFERENCES users(id)`
    FK would diff as DROP+ADD after renaming `users.id`. (Copilot #2.)
  - diffTable threads `current.Database, current.Name` into the call.

Tests
  - parser/directive_test.go: TestExtractStmtRenameFromSkipsLeadingHashAndBlockComments
    covers `#` and `/* … */` lines preceding the directive in the
    leading block.
  - diff/rename_test.go: TestDiffRenameColumnAlsoRewritesSelfReferentialFK
    covers the self-ref RefCols rewrite end-to-end (renames
    users.id → users.user_id with a self-referential
    fk_parent(parent_id) → users(id), asserts no DROP/ADD on
    fk_parent).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 2, 2026
Two inline comments, both valid.

parser/directive.go
  - classifyInlineLine: extend the unnamed-index guard to the two-word
    keyword forms (UNIQUE KEY, UNIQUE INDEX, FULLTEXT KEY/INDEX,
    SPATIAL KEY/INDEX). Without this, `UNIQUE KEY (col)` tokenizes
    with tokens[2]="(col)" and the directive mis-attached to a name
    of "(col)", later surfacing as "target index '(col)' not found".
    Mirrors the guard already in the plain KEY/INDEX branch.
    (Copilot #1.)
  - tokenize doc fixed: previous comment claimed it peeled off
    `","`, `"("`, `"("`, but the implementation only trims `,(`.
    Comment now matches reality. (Copilot #2.)

Tests
  - parser/directive_test.go: TestExtractInlineRenamesUnnamedTwoWordIndexIsUnsupported
    covers the new guard for `UNIQUE KEY (col)` and
    `FULLTEXT INDEX (col)`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 2, 2026
Two inline comments. Both turn into doc-only changes — no behaviour
change — because the surfaced concerns are intentional design choices
already consistent with the rest of myschema.

diff/rename.go
  - applyColumnRenames doc-comment now states explicitly that
    `RENAME COLUMN` is MySQL 8.0+ syntax, and notes that this matches
    myschema's existing 8.0+ baseline (INVISIBLE indexes, CHECK
    constraints, etc.). Apply against an older server fails loudly
    with a syntax error rather than silently. (Copilot #1.)
  - rewriteIndexColumnRefs doc-comment now documents that
    IndexPart.Expr (functional / expression indexes) is intentionally
    NOT rewritten: rewriting embedded column references inside
    arbitrary SQL expressions requires a real parser, and getting it
    subtly wrong is more dangerous than the alternative. An
    expression index that references a renamed column will surface
    as DROP+CREATE on that one index — functionally correct, rare
    enough that the documented limitation suffices. (Copilot #2.)

AGENTS.md
  - "In scope (v1)" entry for renamed-from now spells out the MySQL
    8.0+ requirement, the full list of cross-cutting rewrites
    (index parts, FK Columns, cross-table FK RefCols, self-ref FK
    RefCols, PK constraint columns), and the documented expression-
    index limitation. Also clarifies that constraint / FK *name*
    renames are out of scope (MySQL has no in-place RENAME for
    these), distinct from constraint definition changes which
    already DROP+ADD.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 2, 2026
Two inline comments, both real edge-case bugs.

parser/directive.go
  - tokenize() now also peels off everything from the first "(" in
    non-backticked tokens. MySQL accepts `KEY idx(col)` (no space
    between the index name and the column-list opener); previously
    tokenize gave `idx(col)` as the name and the directive surfaced
    as "target index not found". A bare token starting with "(" (the
    `KEY (col)` unnamed form) collapses to empty and is dropped.
    (Copilot #1.)
  - classifyInlineLine: `CONSTRAINT <name> UNIQUE [KEY|INDEX] (...)`
    defines a unique *index* in MySQL, renameable via ALTER TABLE …
    RENAME INDEX. myschema models UNIQUE in t.Indexes. The
    classifier now routes such lines to inlineKindIndex with the
    constraint/index name, instead of mis-classifying as constraint
    (which would surface as "constraint rename not supported").
    Other CONSTRAINT shapes (CHECK, FOREIGN KEY, PRIMARY KEY) still
    fall through to inlineKindConstraint. (Copilot #2.)

Tests
  - parser/directive_test.go: TestExtractInlineRenamesNoSpaceIndexNameIsParsed
    covers the no-space form for both `KEY` and `UNIQUE KEY`.
  - parser/directive_test.go: TestExtractInlineRenamesConstraintUniqueIsIndex
    covers `CONSTRAINT … UNIQUE KEY` and the no-keyword form
    `CONSTRAINT … UNIQUE (…)`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
Two Copilot nits on the entry I just added in PR #32:

  - normalizeColumnDefault lives in catalog/tables.go, not
    catalog/catalog.go. (Copilot #1.)
  - The function isn't ENUM/SET/CHAR/temporal-specific — it ignores
    typeName entirely and uses vitess to classify any value as
    bareword-vs-non-bareword. The bug is the `def == ""` short-circuit
    at the top of the function, which returns "" before the bareword
    check runs. The parser side stores `''` (quoted), so they never
    compare equal. (Copilot #2.)

Note also expands the suggested fix: either wrap the empty-string
case for column types where MySQL preserves it, or simply drop the
short-circuit and let vitess classify "".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
Two Copilot inline comments on PR #31:

helper.sh
  - Extract the shared parsing / validation / command-execution
    logic of assert_contains and assert_not_contains into a single
    `_assert_substring <mode>` private helper. The two public
    wrappers shrink to one-liners that only choose the mode, so
    diagnostics and behaviour stay in lockstep going forward.
    (Copilot #2.)

filter.test.sh
  - Step 01 only asserted DROP TABLE for the excluded targets was
    *absent*. An unrelated ALTER on `users` could still slip
    through. Add an explicit "No changes" assertion for the
    `-I users` case so the test pins clean-diff behaviour, not just
    absence-of-drops. (Copilot #1.)

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

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 2, 2026
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>
winebarrel added a commit that referenced this pull request May 3, 2026
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>
winebarrel added a commit that referenced this pull request May 3, 2026
CAVEATS.md "The rule only matters when you handwrite a CREATE TABLE
from scratch" was too narrow. myschema also accepts standalone
`ALTER TABLE … ADD CONSTRAINT … FOREIGN KEY` in desired SQL (it's
listed in AGENTS.md "In scope (v1)"), and the same implicit-index
drift fires there too. Widen the wording so both shapes are covered.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 3, 2026
Both classifyInlineLine CONSTRAINT branches said "Distinguish the
four named-constraint shapes" but the switch only handles three
(UNIQUE / CHECK / FOREIGN). The fourth shape that fits the comment
is `CONSTRAINT name PRIMARY KEY (...)` — legal MySQL syntax, but the
user-supplied name is ignored in favour of the fixed "PRIMARY", so
the directive on it can't drive a real rename and the code
intentionally falls through to "unknown".

Reword both comments as "rename-eligible named-constraint shapes" and
explicitly call out `CONSTRAINT name PRIMARY KEY` as the unhandled
fourth so a reader doesn't think a switch case is missing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 3, 2026
Switch "serialise" → "serialize" in the "What this is" intro to
match the existing "the serialized output is deterministic" comment
in model/table.go. AGENTS.md still has UK-spelled "normalise" in a
few places, but normalising every spelling difference is out of
scope for this docs PR — keep the change narrow to the one Copilot
flagged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 3, 2026
… side

Two related convergence bugs surfaced in the second Copilot pass; both
boil down to "the desired side spelled out only one half of the
charset/collation pair, but the other side carries both".

#1 parser/parser.go column-level COLLATE-only

Per-column collation collapse only used `c.CharacterSet`, which is
nil when the user wrote just `COLLATE …` on the column. The catalog
side resolves the effective charset (column charset, falling back to
the table default) before collapsing, so a redundantly-spelled
default collation collapsed on the catalog side but survived on the
parser side, drifting forever as MODIFY COLUMN.

Fix: use the same effective-charset fallback in the parser pass
(column.CharacterSet ?? table.Charset) before calling
CollapseDefaultCollation.

#2 diff/tables.go don't-care semantics for table-level diff

`tableCharsetCollationSQL` did a strict ptrEq on both fields. When
desired specifies only `COLLATE=…` (Charset=nil), the catalog's
non-nil Charset never matched and the same `ALTER TABLE …
COLLATE=…` got emitted on every plan. Symmetrically, a desired
`DEFAULT CHARSET=…` with no COLLATE would have looped if we'd taken
that path on the Collation field.

Fix: treat a desired field set to nil as "the user didn't say, so
anything matches". The result is convergent semantics for either
side specified alone, with the existing both-non-nil and both-nil
paths unchanged.

Tests
- TestParseColumnCollateOnlyInheritsTableCharset: COLLATE-only column
  with the table-default charset's default collation collapses to nil;
  a non-default COLLATE on a COLLATE-only column survives. Pins #1.
- testdata/plan/charset_collate_only_no_changes.yml: desired SQL has
  `COLLATE=utf8mb4_unicode_ci` only (no DEFAULT CHARSET), matches the
  catalog state, plan is empty. Pins #2 end-to-end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 3, 2026
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>
winebarrel added a commit that referenced this pull request May 3, 2026
"reset to default" semantics

Two more convergence holes the previous pass left open. Both stem
from over-reaching the simplifications I made earlier.

#1 parser table-level COLLATE-only

Last pass added effective-charset fallback for column-level COLLATE-
only, but the *table-level* equivalent was still left bare:

    CREATE TABLE t (...) COLLATE=utf8mb4_0900_ai_ci;

(no DEFAULT CHARSET) → parser had t.Charset=nil so
CollapseDefaultCollation couldn't fire, while catalog (which always
knows the effective charset via information_schema.COLLATIONS) did
collapse — endless `ALTER TABLE … COLLATE=…` loops.

Add `model.CharsetOfCollation(coll)` (splits on the first `_`, with
the `binary` self-named special case) and a parser-side
`effectiveCharsetForCollation` helper that walks the chain
(declared charset → table default → derived from collation name)
before calling CollapseDefaultCollation. Used for both table-level
and column-level normalisation.

#2 diff don't-care semantics was too loose

Last pass treated `desired.Collation == nil` as "don't care", which
silently swallowed a real intent: dropping COLLATE from desired SQL
while keeping DEFAULT CHARSET means "reset to the charset's default
collation". With the catch-all don't-care, that case never emitted
an ALTER even when the catalog carried an explicit non-default
collation.

Refine the comparison:
- desired.Collation != nil → strict ptrEq (unchanged).
- desired.Collation == nil && desired.Charset != nil → catalog must
  be at default (current.Collation == nil after collapse); otherwise
  emit `ALTER TABLE … DEFAULT CHARSET=…` so MySQL resets.
- desired.Charset == nil treated as "no opinion on charset".
- Both nil → handled by the existing early return.

Tests
- model: TestCharsetOfCollation pins prefix split, the `binary`
  special case, mixed-case input, and the no-underscore fallback.
- parser: TestParseTableOptionsCollateOnly has two sub-tests — the
  default collation collapses to nil, a non-default one survives.
- diff: testdata/plan/charset_drop_explicit_collation.yml — catalog
  has explicit COLLATE=utf8mb4_unicode_ci, desired drops it but
  keeps DEFAULT CHARSET=utf8mb4 → plan emits a single
  `ALTER TABLE … DEFAULT CHARSET=utf8mb4` (no COLLATE).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 3, 2026
Two valid nits:

#1 The CAVEATS.md "top-level statements skip" bullet listed the
three statement types that ARE handled (CREATE TABLE / CREATE
VIEW / ALTER TABLE) but didn't call out that user-written
`CREATE INDEX` rides on the ALTER TABLE AST shape and IS
supported. Reword so a reader doesn't conclude `CREATE INDEX`
is one of the skipped types.

#2 The "Impact" paragraph claimed "you'll only notice when the
next plan re-emits the underlying CREATE TABLE without the
column". That's wrong — `plan` emits diff DDL, not the
desired-side CREATE TABLE. The actual symptom is "no observable
plan output for that column": the diff is silent because the
column never made it into the desired model, so the next `plan`
either reports `-- No changes` or just shows the other diffs
in the file, and `dump` over the live DB shows the table without
the column. Rewrite the paragraph with the accurate observable
symptoms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 3, 2026
#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.
winebarrel added a commit that referenced this pull request May 3, 2026
Round 6 raised one behavior bug + one coverage gap, both around
the RANGE upper-bound cascade in the per-partition definition
rewrite branch.

#1 The cascade `last++` fired unconditionally for RANGE tables
   whenever the last changed slot wasn't the final partition.
   That was right for *boundary* edits (changing slot i's
   `VALUES LESS THAN <x>` silently moves slot i+1's implicit
   lower bound, so MySQL needs slot i+1 in the REORGANIZE), but
   wrong for per-partition *option*-only edits (COMMENT /
   MAX_ROWS / TABLESPACE / …) where no boundary moves and
   pulling p_{last+1} in just to restate it unchanged turns a
   metadata-only edit into a second partition rewrite.

   Fix: gate the cascade on the actual boundary moving. New
   `partitionValueRangeEqual` compares only the
   `Options.ValueRange` of the two PartitionDefinitions; the
   cascade now fires only when the boundary at `last`
   genuinely changed.

#2 No apply / verify_no_drift fixture exercised the
   RANGE-specific span-expansion path for option-only diffs, so
   a regression in the cascade logic would have slipped past
   tests even though the docs claimed RANGE option diffs were
   supported. New
   `testdata/apply/partition_per_partition_option_change_reorganize_range.yml`
   pins the narrower behaviour end-to-end (LIST counterpart
   already existed). The fixture was written first and confirmed
   failing on the unfixed code (REORGANIZE PARTITION p2020,
   p2021 …) before the gate was tightened.
winebarrel added a commit that referenced this pull request May 4, 2026
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.
winebarrel added a commit that referenced this pull request May 4, 2026
Fix the IS column reference in the CHECK NOT ENFORCED TODO entry —
information_schema.CHECK_CONSTRAINTS does not carry ENFORCED; the
flag lives on TABLE_CONSTRAINTS, which catalog/tables.go already
joins to CHECK_CONSTRAINTS for exactly this reason.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 4, 2026
Singular "error message" → plural to agree with the two distinct
errors enumerated immediately below.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 4, 2026
Fix the IS column reference in the CHECK NOT ENFORCED TODO entry —
information_schema.CHECK_CONSTRAINTS does not carry ENFORCED; the
flag lives on TABLE_CONSTRAINTS, which catalog/tables.go already
joins to CHECK_CONSTRAINTS for exactly this reason.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 5, 2026
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>
winebarrel added a commit that referenced this pull request May 5, 2026
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>
winebarrel added a commit that referenced this pull request May 5, 2026
- Help text on --split now reads `'-- Wrote N file(s) to <dir>'`,
  matching the cmd's actual `-- Wrote ...` notice (was lowercase
  `wrote` — doc/CLI drift the review flagged).
- splitPath also rejects ':' anywhere in the name plus
  filepath.VolumeName != "". On Windows a name like `C:foo` has a
  volume part that filepath.Join would treat as absolute and
  *discard* dir; the literal ':' scan catches the shape on every
  host (filepath.VolumeName is OS-specific — Linux/macOS always
  return ""), and the VolumeName call stays as a belt-and-
  suspenders guard for any Windows shape the byte scan misses.
  New test cases cover `C:foo`, `\\server\share\x`, and
  `weird:name`, exercised on Linux CI via the explicit ':' check.
- Rewrote the comments on splitPath and TestDump_SplitRejectsUnsafeName
  so they describe what the validator actually does (filesystem-
  shaped path-traversal guards) instead of asserting MySQL's
  identifier rules. Names that merely *contain* '.' (e.g. `my.tbl`)
  are accepted — only the bare `.` / `..` segment sentinels are
  unsafe at the filesystem layer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 5, 2026
Address Copilot review #2 on PR #77, which flagged that the existing
bulk_alter fixtures don't exercise:

  - `-- myschema:renamed-from` flows folded into a multi-spec ALTER.
    `ALTER TABLE t RENAME COLUMN a TO b, ADD COLUMN c …;` puts MySQL
    in charge of the within-statement evaluation order; the diff
    layer's column-ref rewrites are necessary but not sufficient
    proof that the combined shape converges.
  - Same-name `DROP CHECK + ADD CONSTRAINT` replacement rewritten as
    `DROP CHECK x, ADD CONSTRAINT x CHECK (…);`. Wrong relative order
    inside one ALTER would either duplicate-name error (ADD first) or
    leave no constraint at all (DROP first with an evaluation gap),
    so end-to-end coverage was missing.

Both new fixtures pin `verify_no_drift: true` so the round-trip is
asserted on top of the rendered SQL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 5, 2026
Address Copilot review #2 on PR #77, which flagged that the existing
bulk_alter fixtures don't exercise:

  - `-- myschema:renamed-from` flows folded into a multi-spec ALTER.
    `ALTER TABLE t RENAME COLUMN a TO b, ADD COLUMN c …;` puts MySQL
    in charge of the within-statement evaluation order; the diff
    layer's column-ref rewrites are necessary but not sufficient
    proof that the combined shape converges.
  - Same-name `DROP CHECK + ADD CONSTRAINT` replacement rewritten as
    `DROP CHECK x, ADD CONSTRAINT x CHECK (…);`. Wrong relative order
    inside one ALTER would either duplicate-name error (ADD first) or
    leave no constraint at all (DROP first with an evaluation gap),
    so end-to-end coverage was missing.

Both new fixtures pin `verify_no_drift: true` so the round-trip is
asserted on top of the rendered SQL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant