Add TODO.md#2
Merged
Merged
Conversation
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AGENTS.mdinto a top-levelTODO.mdchecklist, grouped by area (object coverage, type fidelity, diff ordering, CLI features, directives, drop policy, tests, docs, build/release, cleanup).AGENTS.md: loose CHECK normaliser, default-value parse-tree comparison,--with-txno-op flag, generated-column expression compare, column position support, etc.Test plan
cat TODO.mdrenders as expectedgo build ./...andgo test ./...unaffected🤖 Generated with Claude Code