Fix migration tool to honor NamingStyle and MapTo (#324)

_sessions/324-migration-honor-naming-mapto/_pr-body.md

@@ -0,0 +1,65 @@
+## Summary
+- Closes #324
+
+Makes the migration tool's `ProjectSchemaReader` honor the two column-naming mechanisms the runtime
+source generator already honors — the `NamingStyle` override and per-column `MapTo("physical")` — so
+`quarry migrate add`/`diff` produce snapshots and DDL whose column names match the physical names the
+runtime actually queries.
+
+## Reason for Change
+The migration reader detected the naming style from a property literally named `Naming` (the real API is
+`NamingStyle`) and never parsed `MapTo`. As a result:
+- Snapshots/DDL used C# **property** names (`UserName`, `CreditLimit`) while the runtime emits SQL using
+  **physical** names (`user_name`, `credit_limit`) — a generated migration created columns the runtime
+  never queries.
+- Adding or removing a mapping between two schema versions produced **zero** diff steps — the change was
+  silently ignored, so the physical column the runtime targets was never created or renamed.
+
+## Impact
+- `src/Quarry.Tool/Schema/ProjectSchemaReader.cs`
+  - `ExtractTableDef` now detects the real `NamingStyle` override, mirroring the runtime
+    `SchemaParser.ExtractNamingStyle` **exactly**: it gates on `!IsStatic && IsOverride` and reads only an
+    expression-bodied member access (`protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`).
+    Honoring any looser form than the runtime would let the two disagree on column names again.
+  - `ExtractColumnDef` now parses `MapTo` — both the chained `.MapTo("x")` and the standalone generic
+    `MapTo<T>("x")` (via a new `GenericNameSyntax` case, mirroring runtime `GetMethodName`). The physical
+    column name becomes `MapTo ?? ToColumnName(prop, style)` and `ColumnDef.MappedName` is populated so the
+    snapshot re-emits `.MapTo(...)` and `SchemaHasher`/`RenameMatcher` stay faithful.
+  - A `Ref`'s foreign-key constraint column now reuses the resolved column name, so it stays consistent
+    when a Ref uses `MapTo`/`NamingStyle`.
+- This fixes both `migrate add` and `migrate diff` (both call `ProjectSchemaReader.ExtractSchemaSnapshot`).
+
+No changes were needed to `SchemaDiffer`, `DdlRenderer`, `MigrationCodeGenerator`, `SnapshotCodeGenerator`,
+or the `ColumnDef` model — they already consume `Name`/`MappedName` correctly (the migration DDL name is
+generated from `ColumnDef.Name`). The runtime `SchemaParser` and the `Quarry.Migration/SchemaResolver`
+(SchemaMap for result materialization) already honored both mechanisms and are untouched.
+
+## Plan items implemented as specified
+- Honor `NamingStyle` and `MapTo` in `ProjectSchemaReader`; de-mask the three naming tests in
+  `ProjectSchemaReaderIndexTests` (fictional `NamingStyle Naming` → the real `override NamingStyle`).
+- Unit coverage for `NamingStyle` and `MapTo` (chained, standalone-generic, override-vs-style precedence,
+  and Exact regression).
+- Diff coverage proving a `MapTo` add/remove produces a rename-or-drop+add, never a silent no-op.
+- End-to-end guard compiling the real committed `AccountSchema` (`credit_limit`) and asserting the
+  extracted name and the generated migration code carry the physical name — matching the runtime SQL
+  already covered by `CrossDialectSchemaTests.Select_MapToColumn_CreditLimit`.
+
+## Gaps in original plan implemented
+- The issue's third location bullet (`DatabaseSchemaReader.cs` / `NormalizeForDiff`) was **stale** — no such
+  file/method exists. The diff already keys columns off `ColumnDef.Name` in `SchemaDiffer`, so once `Name`
+  carries the physical name, add/remove-`MapTo` diffs stop being no-ops with no change needed there.
+- Review found the initial `NamingStyle` detection was *more permissive* than the runtime (it also read
+  initializer/getter-arrow forms and skipped the override guard) — itself a divergence risk. Tightened to
+  mirror the runtime exactly, with added parity tests for getter-arrow, non-override, and non-literal-`MapTo`
+  forms.
+
+## Breaking Changes
+- Consumer-facing: A project that already ran `migrate add` on a schema using `NamingStyle` or `MapTo` has
+  snapshots recorded with **PascalCase** (property) column names. After this fix, its **next** `migrate add`
+  will show a physical-name diff — column rename (or drop+add), foreign-key constraint-name shifts
+  (`FK_{table}_{physicalName}`), and a changed snapshot hash chain (`SchemaHasher` includes `MappedName`).
+  Those projects were already broken (their migrations diverged from what the runtime queries), so this is a
+  one-time **correction**, not a regression. Projects using `Exact` naming and no `MapTo` are unaffected
+  (verified: the committed `1_SimpleMigration`, `Quarry.Sample.Aot`, `Quarry.Sample.WebApp` migrations use
+  `Exact`; the snake_case and `MapTo` samples have no committed migrations).
+- Internal: none.

_sessions/324-migration-honor-naming-mapto/plan.md

@@ -0,0 +1,74 @@
+# Plan: 324 — Migration tool honors NamingStyle and MapTo
+
+## Goal
+Make the migration tool's `ProjectSchemaReader` honor the `NamingStyle` override and per-column
+`MapTo("physical")`, so `quarry migrate add`/`diff` produce snapshots and DDL whose column names match
+the runtime's physical names — and so adding/removing a mapping produces a real diff step, not a silent
+no-op.
+
+## Representation (the key concept)
+`ColumnDef.Name` becomes the **physical** column name (`MapTo-arg ?? ToColumnName(prop.Name, style)`),
+and `ColumnDef.MappedName` is set to the `MapTo` argument (or null). This is required because the
+migration DDL is generated from `col.Name` (`MigrationCodeGenerator` → `DdlRenderer`), which ignore
+`MappedName`. `SchemaDiffer` keys columns by `Name`, so a changed physical name naturally yields
+add/drop/rename steps instead of a no-op. Populating `MappedName` keeps the snapshot artifact's
+`.MapTo(...)` and the `SchemaHasher`/`RenameMatcher` inputs faithful. See workflow.md Decisions.
+
+## Steps
+
+- [x] **Step 1 — `ProjectSchemaReader`: honor `NamingStyle` + `MapTo` (+ Ref FK consistency), and de-mask the 3 tests.**
+  - `ExtractTableDef`: change the naming-style detection from the fictional `prop.Name == "Naming"` to the
+    real `prop.Name == "NamingStyle"`. Parse the value from the expression body (and getter-arrow) as a
+    member access (`NamingStyle.SnakeCase` etc.) → `NamingStyleKind`, keeping `Exact` as default. Mirror
+    runtime `SchemaParser.ExtractNamingStyle`.
+  - `ExtractColumnDef`: add a `MapTo` case to the fluent-chain walk that extracts the first string-literal
+    argument → `mappedName`. Extend the inline method-name extraction to also handle `GenericNameSyntax`
+    (so standalone `MapTo<T>("x")` is recognized, not only chained `.MapTo("x")`). Compute
+    `columnName = mappedName ?? ToColumnName(prop.Name, namingStyle)` and pass `mappedName:` to the
+    `ColumnDef` constructor.
+  - `ExtractTableDef` Ref branch: use the resolved column name (`colDef?.Name ?? ToColumnName(...)`) for the
+    FK constraint column so a MapTo'd/styled Ref column and its FK stay consistent.
+  - Update the 3 masking tests (`ProjectSchemaReaderIndexTests.cs` lines ~90, ~377, ~397):
+    `public NamingStyle Naming => NamingStyle.SnakeCase;` → `protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`.
+    (These must change together with the reader fix, else they'd fail.)
+  - Tests: full existing suite stays green; the de-masked tests now exercise the real API.
+  - Depends on: nothing.
+
+- [x] **Step 2 — Unit coverage: `NamingStyle` and `MapTo` honored (new test file `ProjectSchemaReaderNamingMapToTests.cs`).**
+  - `NamingStyle.SnakeCase` via the real override → `TableDef.NamingStyle == SnakeCase`, columns styled
+    (`UserName` → `user_name`), `MappedName == null`.
+  - `MapTo` chained (`Col<T> X => Mapped<…>().MapTo("x")` and a plain `Identity().MapTo(...)`/`Length().MapTo(...)`)
+    → `Name == "x"`, `MappedName == "x"`.
+  - `MapTo` standalone generic (`Col<string> X => MapTo<string>("x")`) → `Name == "x"`, `MappedName == "x"`.
+  - `MapTo` overrides `NamingStyle` (snake-case schema + a column with `MapTo("explicit")`) → `Name == "explicit"`.
+  - No-MapTo / `Exact` → `Name == "PropertyName"`, `MappedName == null` (regression guard).
+  - Depends on: Step 1.
+
+- [x] **Step 3 — Diff coverage: add/remove `MapTo` is not a no-op.**
+  - Extract v1 (column with `MapTo("credit_limit")`) and v2 (same column without `MapTo`) via
+    `ProjectSchemaReader`; run `SchemaDiffer.Diff(v1, v2)` and assert the result is **non-empty** and
+    contains a `RenameColumn` (or a drop+add) for that column — never zero steps. Assert the reverse
+    direction (add a mapping) likewise produces steps.
+  - Depends on: Step 1.
+
+- [x] **Step 4 — End-to-end guard: `AccountSchema`/`credit_limit` physical-name parity.**
+      Implemented by compiling the real `AccountSchema.cs` + real `Money.cs` (loaded via `[CallerFilePath]`
+      sibling `../Samples`) with a minimal `UserSchema` stub; asserts extracted `credit_limit`/`Balance`
+      and that `MigrationCodeGenerator` output contains `"credit_limit"` and not `"CreditLimit"`.
+  - Extract the **real committed** `AccountSchema` via `ProjectSchemaReader` (compile its source with the
+    minimal supporting types it needs — `Money`, `MoneyMapping`, `UserSchema`; prefer the real sample
+    sources, fall back to inline stubs only if the dependency graph is impractical — decision recorded in
+    Working Notes at implementation time). Assert `CreditLimit` column `Name == "credit_limit"` &
+    `MappedName == "credit_limit"`, and `Balance` (Mapped, no MapTo) `Name == "Balance"` & `MappedName == null`.
+  - Also assert the generated migration/snapshot code for this schema contains `credit_limit` (ties the
+    tool's DDL-bound column name to the runtime's physical name; the runtime side is already covered by
+    `CrossDialectSchemaTests.Select_MapToColumn_CreditLimit`).
+  - Depends on: Step 1.
+
+## Non-goals / notes
+- No change to `SchemaDiffer`, `DdlRenderer`, `MigrationCodeGenerator`, `SnapshotCodeGenerator`, or the
+  `ColumnDef` model — they already consume `Name`/`MappedName` correctly.
+- `Quarry.Migration/SchemaResolver` and `Quarry.Generator/SchemaParser` already honor NamingStyle+MapTo;
+  untouched.
+- Behavioral-correction note (projects that previously ran `migrate add` on a NamingStyle/MapTo schema
+  now see a physical-name diff on their next `migrate add`) goes in the PR body — no CHANGELOG file exists.

_sessions/324-migration-honor-naming-mapto/review.md

@@ -0,0 +1,40 @@
+# Review: 324-migration-honor-naming-mapto
+
+## Classifications
+| ID | Class | Rec | Sev | Section | Finding | Action Taken |
+|----|-------|-----|-----|---------|---------|--------------|
+| F2 | A | A | M | Correctness | NamingStyle detection too permissive vs runtime (no override guard; reads initializer/getter-arrow) → tool/runtime can disagree again | Fixed: `ProjectSchemaReader.cs:113` now gates on `!IsStatic && IsOverride` and reads expression-body member-access only, mirroring `SchemaParser.ExtractNamingStyle`. |
+| F7 | A | A | M | Consistency | NamingStyle extraction diverges from runtime SchemaParser acceptance set (same root as F2) | Fixed by the F2 change (tool now accepts exactly the runtime's form set). Extracting a shared cross-project helper was out of scope; exact mirroring achieves parity. |
+| F1 | B | B | L | Plan | Step-4 stub decision recorded only in a code comment, not workflow.md Working Notes | Fixed: added a Working Note documenting the real-AccountSchema + Money / stub-UserSchema decision. |
+| F4 | B | B | L | Test | NamingStyle path not driven through MigrationCodeGenerator (only MapTo path has the DDL-level assertion) | Fixed: added `NamingStyle_SnakeCase_MigrationCodeUsesStyledColumnNames` asserting the generated migration contains `user_name` and not `UserName`. |
+| F5 | B | B | L | Test | No coverage for the divergent parsing forms (getter-arrow/initializer/non-override) or non-literal MapTo | Fixed: added getter-arrow, non-override, and non-literal-MapTo parity tests (all assert Exact / null-MappedName, matching runtime). |
+| F8 | B | B | M | Integration | Intended compat break (physical-name diffs/hash shift) not yet documented for users (PR body pending) | To be documented in the PR body Breaking Changes section at PR creation (this step). |
+| F3 | D | D | L | Test | Test compilation not checked for diagnostics before extraction | Dismissed: matches the existing `ProjectSchemaReaderIndexTests` harness; assertions target specific physical names that only arise if parsing succeeds, so silent degradation can't produce a false pass. |
+| F6 | D | D | L | Test | ReadSampleSource via [CallerFilePath] has no existence guard (environment-coupled) | Dismissed: CI runs `dotnet build` + `dotnet test --no-build` in one workspace with no PathMap/ContinuousIntegrationBuild, so CallerFilePath resolves to the real path; accepted fixture-loading pattern. |
+
+## Plan Compliance
+| ID | Finding | Sev | Why It Matters |
+| F1 | Step 4 told the author to record the "stub `UserSchema` vs. compile the whole real sample graph" decision in Working Notes at implementation time; it is only captured in a code comment in `ProjectSchemaReaderNamingMapToTests.cs:274-276`, not in `workflow.md` Working Notes. | L | Minor process gap; the rationale is discoverable in the test but the audit trail the plan asked for is missing. |
+
+## Correctness
+| ID | Finding | Sev | Why It Matters |
+| F2 | NamingStyle detection in `ProjectSchemaReader.cs:113-136` does not faithfully mirror runtime `SchemaParser.ExtractNamingStyle` (`SchemaParser.cs:250-288`): it (a) matches any declared member named `NamingStyle` without the runtime's `!IsStatic && IsOverride` guard, (b) does not validate the property type, and (c) reads `Initializer?.Value` and the getter-arrow body — forms the runtime ignores (runtime only reads `propSyntax.ExpressionBody`). For a legal auto-property-initializer override (`protected override NamingStyle NamingStyle { get; } = NamingStyle.SnakeCase;`) or a getter-arrow body, the tool yields styled names while the runtime yields `Exact`, so the migration emits `user_name` columns the runtime never queries — reintroducing the exact #324 class of mismatch, inverted. | M | Undermines the fix's core guarantee (migration names == runtime physical names) for legal C# forms; the safe design is to mirror the runtime's guard set and expression-body-only parsing exactly. |
+
+## Security
+| ID | Finding | Sev | Why It Matters |
+No concerns. Inputs are developer-authored schema source parsed via Roslyn; `MapTo`/`NamingStyle` values flow into generated code exactly as `Name`/`MappedName` did before (property names previously, physical names now) — no new external-input or injection surface introduced by this diff.
+
+## Test Quality
+| ID | Finding | Sev | Why It Matters |
+| F3 | The test compilation (`CreateCompilation`) is never checked for diagnostics before extraction; a missing/incorrect `MetadataReference` would let the semantic model degrade silently (error types still expose `.Name`) rather than fail with a clear cause. | L | A broken reference set could make a test pass for the wrong reason or fail cryptically instead of pinpointing the setup error. |
+| F4 | NamingStyle physical-name parity is asserted only at the `TableDef`/`ColumnDef` level (`NamingStyle_SnakeCase_...`); unlike the MapTo/`credit_limit` case, no test drives a snake_case schema through `MigrationCodeGenerator` (the DDL-bound path). | L | A regression in styled-name emission into the actual migration code would go uncaught, since only the MapTo path has the end-to-end `MigrationCodeGenerator` assertion. |
+| F5 | No coverage for the parsing forms that actually diverge from the runtime (getter-arrow / auto-property-initializer / non-override `NamingStyle` — see F2) nor for `MapTo` with a non-literal argument (e.g. `MapTo(SomeConst)`). | L | The untested edge cases are precisely where tool/runtime can disagree; happy-path-only coverage hides the divergence. |
+| F6 | `ReadSampleSource` locates `../Samples` via `[CallerFilePath]` (`ProjectSchemaReaderNamingMapToTests.cs:266-271`) with no existence guard, so the guard test depends on the compile-time source path still existing at run time. | L | Running the built test assembly on a different machine/path (or with deterministic/path-mapped builds) breaks with `FileNotFoundException`; robust but environment-coupled. |
+
+## Codebase Consistency
+| ID | Finding | Sev | Why It Matters |
+| F7 | The NamingStyle extraction is a bespoke reimplementation rather than aligning with the runtime `SchemaParser.ExtractNamingStyle` reference impl the plan named as the model; the accepted-forms set and guards differ (see F2), so the two schema readers can disagree. The MapTo/`GenericNameSyntax` changes, by contrast, correctly mirror runtime `GetMethodName`/`ParseColumnModifiers` and follow the file's existing `Computed`/`Collation` else-if idiom. | M | Divergence between the tool and runtime readers is the root cause of #324-style bugs; the two should share guard/parse semantics (ideally the same helper) to stay in lockstep. |
+
+## Integration / Breaking Changes
+| ID | Finding | Sev | Why It Matters |
+| F8 | Because `ColumnDef.Name` now carries the physical name, the next `migrate add` on any existing project using `NamingStyle` or `MapTo` produces a non-empty diff: column rename-or-drop+add, FK constraint-name shifts (`FK_{table}_{physicalName}` via `colDef?.Name`), and a changed snapshot hash chain (`SchemaHasher` includes `MappedName`). This is the intended behavioral correction, but it is a real compat break in migration lineage; the plan defers the notice to the PR body, which is not yet created (`workflow.md` `pr:` empty). | M | Users must be told their next migration will contain corrective rename/DDL steps and altered hashes; without the PR-body note the break is silent. |