Entity DDL safety: alter-path attribute checks + create-or-modify data-loss guard

.claude/skills/fix-issue.md

@@ -165,6 +165,8 @@ to the symptom table below, so the next similar issue costs fewer reads.
 | No way to change an existing **enumeration value's caption** in place — `alter enumeration` had only `ADD`/`RENAME`/`DROP VALUE` + `SET COMMENT`; `RENAME VALUE X TO Y` changes the *name*, not the caption. The only route was drop + recreate, which fails while the enum is referenced by an attribute | Missing grammar action + AST op + executor case — a full-stack gap, not a code bug | `mdl/grammar/domains/MDLDomainModel.g4` (`alterEnumerationAction`) → `mdl/ast/ast_enumeration.go` (`AlterEnumOp`) → `mdl/visitor/visitor_enumeration.go` (`ExitAlterEnumerationAction`) → `mdl/executor/cmd_enumerations.go` (`execAlterEnumeration`) | Add `MODIFY VALUE IDENTIFIER CAPTION STRING_LITERAL` (reuses existing `MODIFY`/`CAPTION` tokens, so no lexer change); add `AlterEnumModifyCaption` op reusing the `Caption` field; executor finds the value by name and replaces only the `en_US` translation (preserves the value's ID + other locales). Re-captions in place, so it works while referenced (mxbuild 0 errors) — no drop needed. Value names in `alter` must be plain identifiers (a reserved-word-named value like `Created` can't be targeted — pre-existing, shared by ADD/RENAME/DROP). Tests: `TestAlterEnumeration_ModifyValueCaption` (visitor) + `TestAlterEnumeration_ModifyValueCaption_Mock` (executor); example in `01-domain-model-examples.mdl` |
 | A page with a native **LISTVIEW** over a `database from` (XPath) datasource passes `mx check` but **crashes the browser client at runtime** and redirects to login: `TypeError: Cannot read properties of undefined (reading 'length')` at `processResult` → `retrieveByXPath`. Pluggable Gallery/DataGrid2 database sources are fine | The serialized `Forms$ListViewXPathSource` omitted the arrays the client reads `.length` of. **Codec (default):** `Forms$ListViewSearch` emitted without its `SearchRefs` list — the encoder drops an empty, never-`Set` PartList unless the `$Type` is in `RegisterTypeDefaults` (GridSortBar.SortItems was registered; ListViewSearch was not). **Legacy:** wrote a bogus `Forms$ListViewSort` + a `Paths` key (renamed `SearchRefs` in 7.11.0) and no `Forms$GridSortBar`. NOT a `SortItems` marker issue — an empty `[2]` compiles fine when search is off; the crash is the absent `SearchRefs`. Diagnose by building a Deploy target and reading the compiled `deployment/web/pages/<Page>.js` (the client model) + `mxcli bson dump --type page` on the source | `mdl/backend/modelsdk/widget_write.go` (`init` RegisterTypeDefaults) + `sdk/mpr/writer_widgets_display.go` (`serializeListViewDataSource`, `emptyListViewXPathSource`) | Codec: `RegisterTypeDefaults("Forms$ListViewSearch", {MandatoryLists: []string{"SearchRefs"}})` (emits empty marker-3 list). Legacy: emit `Forms$GridSortBar`/`SortItems` (mirror `SerializeCustomWidgetDataSource`) + `Forms$ListViewSearch`/`SearchRefs` + `ForceFullObjects`; drop the bogus `Sort`/`Paths`. Tests: `TestListViewSourceToGen_SearchRefsEmitted` (codec, encode-level), `TestSerializeListViewDataSource_Database` + `TestEmptyListViewXPathSource_Shape` (legacy). Repro `mdl-examples/bug-tests/listview-database-source-searchrefs.mdl` |
 | Editing `themesource/**/main.scss` (or `theme/web/main.scss`) while `mxcli run --local` serves on `:8080` keeps showing **old styles** — reads exactly like a stale compiled-CSS cache. `rm -rf theme-cache/ .mendix-cache/ deployment/` "fixes" it only because a restart came with it | THREE distinct causes, none a CSS cache: (1) **no `--watch` = no watcher at all** — `mxbuild --serve` only rebuilds on a `/build` request (startup, or a watch tick), so a save changes nothing; (2) **stale process silently adopted** — a leftover serve/runtime on the ports answers the startup readiness probes (`waitReady`/`waitAdminReady` only check "port answers"), so a new run attaches to the OLD process and its own child is torn down by `defer`; a backgrounded `run --local` whose wrapping shell exited non-zero dies while its serve+runtime keep serving; (3) the theme source was **watched by nothing** — the `--watch` signal was model-only (`.mpr`+`mprcontents/`). The incremental theme step itself is FINE: one `/build` after an scss **content** edit does rewrite `theme-cache/web/theme.compiled.css` (verified), so there is no cache to clear | `cmd/mxcli/docker/runlocal.go` — `checkTargetPortsFree` (guard), `themeSourceMTime`/`sourceMTime` (watch signal), `watchAndApply` (generation log) | (2) Refuse to boot when `:8080/:8090/:6543` already answer, with an actionable message (never auto-kill — user's call). (3) Add `theme/`+`themesource/` scss/css/js to the `--watch` mtime-poll signal (`sourceMTime` = max(model, theme)); poll-based so it's container-safe (unlike the rollup chokidar/inotify web-client watcher). Log a build-generation counter (`build #2`) so "did it take?" is answerable. Docs: `docs-site/src/tools/run-local.md` + skill `run-local.md` — "SCSS needs a rebuild (`--watch` or clean restart), never a cache-clear; kill the old serve/runtime first". Tests `TestThemeSourceMTime_WatchesThemeAndThemesource`, `TestCheckTargetPortsFree` |
+| `alter entity M.E add attribute X: autonumber;` (no seed) or `... add attribute Created: AutoCreatedDate;` (renamed AutoX) passes `mxcli check` "Check passed!" but fails the build (CE7247) / silently discards the name — while the SAME attribute in `create entity` is correctly flagged (MDL023 / MDL022) | The per-attribute checks (MDL021/022/023) only ran on `CreateEntityStmt`; the ALTER ENTITY ADD ATTRIBUTE path had no validation at all, so an attribute added later escaped every rule | `mdl/executor/cmd_enumerations.go` (`ValidateAlterEntity`, `validateEntityAttribute`) wired from `cmd/mxcli/cmd_check.go` + `cmd/mxcli/lsp_diagnostics.go` | Extract the CREATE loop body into `validateEntityAttribute(attr, persistent, entityName)`; add `ValidateAlterEntity(stmt)` that runs it on `AlterEntityAddAttribute`. The entity kind isn't known from ALTER, so the persistent-only MDL020 is skipped; the kind-independent MDL021/022/023 all run. Bug-test `mdl-examples/bug-tests/f6-autonumber-seed-alter.fail.mdl`. Findings #6 (alter path) |
+| `create or modify entity M.E ( <subset> )` on an entity that already has more attributes **silently drops** every attribute not re-listed (36→2 attrs seen in practice), then widgets/microflows still bound to them fail the build with CE1613 — and the "already exists" error that leads users here recommends the destructive `create or modify` for a partial edit | `create or modify` rebuilds the entity from the statement alone and REPLACEs the stored one, so any omitted attribute is deleted with no warning; the `NewAlreadyExistsMsg` hint pointed at `create or modify` without distinguishing "replace whole" from "add one" | `mdl/executor/cmd_entities.go` (`droppedEntityMembers`, the warn block before `UpdateEntity`, and the `execCreateEntity` already-exists message) | Warn-only (non-blocking, the user asked to modify): before `UpdateEntity`, diff existing vs replacement members (`droppedEntityMembers` — named attrs case-insensitive + the four audit flags) and print what's dropped + point at `alter entity … add attribute` for incremental edits. Fix the already-exists message to recommend `alter entity` for a member change and reserve `create or modify` for a full replace. Bug-test `mdl-examples/bug-tests/f24-create-or-modify-dataloss.mdl`. Findings #24 |
 
 ---
 

cmd/mxcli/cmd_check.go

@@ -113,6 +113,10 @@ Examples:
 			if entityStmt, ok := stmt.(*ast.CreateEntityStmt); ok {
 				violations = append(violations, executor.ValidateEntity(entityStmt)...)
 			}
+			// Apply the same per-attribute checks to ALTER ENTITY ADD ATTRIBUTE
+			if alterStmt, ok := stmt.(*ast.AlterEntityStmt); ok {
+				violations = append(violations, executor.ValidateAlterEntity(alterStmt)...)
+			}
 			// Check microflow body for common issues
 			if mfStmt, ok := stmt.(*ast.CreateMicroflowStmt); ok {
 				violations = append(violations, executor.ValidateMicroflow(mfStmt)...)

cmd/mxcli/lsp_diagnostics.go

@@ -272,6 +272,9 @@ func (s *mdlServer) runSemanticValidation(text string) []protocol.Diagnostic {
 		if entityStmt, ok := stmt.(*ast.CreateEntityStmt); ok {
 			violations = append(violations, executor.ValidateEntity(entityStmt)...)
 		}
+		if alterStmt, ok := stmt.(*ast.AlterEntityStmt); ok {
+			violations = append(violations, executor.ValidateAlterEntity(alterStmt)...)
+		}
 		if mfStmt, ok := stmt.(*ast.CreateMicroflowStmt); ok {
 			violations = append(violations, executor.ValidateMicroflow(mfStmt)...)
 		}

mdl-examples/bug-tests/f24-create-or-modify-dataloss.mdl

@@ -0,0 +1,18 @@
+-- Findings #24: `create or modify entity` rebuilds the entity from the statement
+-- alone and REPLACES the stored one, so any existing attribute the statement
+-- omits is silently dropped (later surfacing as CE1613 on widgets still bound to
+-- it). This is now non-blocking but WARNED: running the second statement against
+-- a project where Sudoku.Game already has Solution/Difficulty prints a data-loss
+-- warning listing the dropped members, and points at ALTER ENTITY for safe,
+-- incremental edits.
+--
+-- Syntactically valid (this file is a syntax example, not a negative test); the
+-- warning only appears when executed against a project that has the fuller entity.
+
+-- Safe incremental change — edits only the named member, leaves the rest intact:
+alter entity Sudoku.Game add attribute Score: integer;
+
+-- Destructive replace — keep ONLY if you intend to drop everything not listed:
+create or modify entity Sudoku.Game (
+  Puzzle: string
+);

mdl-examples/bug-tests/f6-autonumber-seed-alter.fail.mdl

@@ -0,0 +1,6 @@
+-- Findings #6 (alter path): the seedless-autonumber check (MDL023) also applies
+-- when the attribute is added later via ALTER ENTITY, not only at CREATE. Before
+-- this fix, `alter entity ... add attribute X: autonumber;` returned "Check
+-- passed!" and then failed the build with CE7247 "Value cannot be empty".
+-- Negative test: `mxcli check` MUST fail here.
+alter entity Bug6.Game add attribute PuzzleNo: autonumber;

mdl/executor/bugfix_test.go

@@ -9,8 +9,11 @@ import (
 	"testing"
 
 	"github.com/mendixlabs/mxcli/mdl/ast"
+	"github.com/mendixlabs/mxcli/mdl/backend/mock"
 	"github.com/mendixlabs/mxcli/mdl/linter"
 	"github.com/mendixlabs/mxcli/mdl/visitor"
+	"github.com/mendixlabs/mxcli/model"
+	"github.com/mendixlabs/mxcli/sdk/domainmodel"
 )
 
 // TestValidateDuplicateVariableDeclareRetrieve verifies that DECLARE followed by
@@ -189,6 +192,147 @@ func TestValidateEntityAutoMemberRename(t *testing.T) {
 	}
 }
 
+// TestValidateAlterEntityAddAttribute verifies that the same per-attribute
+// checks applied on CREATE (MDL022 AutoX rename, MDL023 autonumber seed, MDL021
+// reserved words) also run on the ALTER ENTITY ADD ATTRIBUTE path. Previously an
+// attribute added via ALTER escaped these checks entirely (findings #6, half-fix).
+func TestValidateAlterEntityAddAttribute(t *testing.T) {
+	build := func(src string) []linter.Violation {
+		prog, errs := visitor.Build(src)
+		if len(errs) > 0 {
+			t.Fatalf("parse error: %v", errs[0])
+		}
+		return ValidateAlterEntity(prog.Statements[0].(*ast.AlterEntityStmt))
+	}
+	hasMDL := func(vs []linter.Violation, id string) bool {
+		for _, v := range vs {
+			if v.RuleID == id {
+				return true
+			}
+		}
+		return false
+	}
+
+	// MDL023: seedless autonumber added via ALTER.
+	if !hasMDL(build(`alter entity M.G add attribute PuzzleNo: autonumber;`), "MDL023") {
+		t.Error("expected MDL023 for a seedless autonumber added via ALTER")
+	}
+	if hasMDL(build(`alter entity M.G add attribute PuzzleNo: autonumber default 1;`), "MDL023") {
+		t.Error("did not expect MDL023 when the added autonumber has a seed")
+	}
+	// MDL022: AutoX pseudo-type added under a non-canonical name.
+	if !hasMDL(build(`alter entity M.G add attribute StartedAt: autocreateddate;`), "MDL022") {
+		t.Error("expected MDL022 for a renamed autocreateddate added via ALTER")
+	}
+	if hasMDL(build(`alter entity M.G add attribute CreatedDate: autocreateddate;`), "MDL022") {
+		t.Error("did not expect MDL022 when the added AutoX name matches the system member")
+	}
+	// MDL021: reserved word added via ALTER (kind-independent).
+	if !hasMDL(build(`alter entity M.G add attribute Type: string;`), "MDL021") {
+		t.Error("expected MDL021 for a reserved word added via ALTER")
+	}
+	// A plain, valid attribute added via ALTER produces no violations.
+	if vs := build(`alter entity M.G add attribute Score: integer;`); len(vs) != 0 {
+		t.Errorf("expected no violations for a plain added attribute, got %v", vs)
+	}
+	// Non-ADD alter operations are ignored.
+	if vs := build(`alter entity M.G drop attribute Score;`); len(vs) != 0 {
+		t.Errorf("expected no violations for a DROP ATTRIBUTE op, got %v", vs)
+	}
+}
+
+// TestDroppedEntityMembers verifies the helper that lists members a CREATE OR
+// MODIFY replace would delete (findings #24).
+func TestDroppedEntityMembers(t *testing.T) {
+	existing := &domainmodel.Entity{
+		Name: "Game",
+		Attributes: []*domainmodel.Attribute{
+			{Name: "Puzzle"}, {Name: "Solution"}, {Name: "Difficulty"},
+		},
+		HasCreatedDate: true,
+		HasOwner:       true,
+	}
+	replacement := &domainmodel.Entity{
+		Name: "Game",
+		Attributes: []*domainmodel.Attribute{
+			{Name: "puzzle"}, // case-insensitive match — kept
+			{Name: "Score"},  // new — not a drop
+		},
+		HasOwner: true, // kept
+		// HasCreatedDate now false — dropped
+	}
+	dropped := droppedEntityMembers(existing, replacement)
+	got := strings.Join(dropped, ",")
+	// Solution and Difficulty are dropped; Puzzle survives (case-insensitive);
+	// createdDate audit field is dropped; owner survives.
+	for _, want := range []string{"Solution", "Difficulty", "createdDate (system field)"} {
+		if !strings.Contains(got, want) {
+			t.Errorf("expected dropped members to include %q, got %q", want, got)
+		}
+	}
+	for _, unwanted := range []string{"Puzzle", "puzzle", "Score", "owner"} {
+		if strings.Contains(got, unwanted) {
+			t.Errorf("did not expect %q among dropped members, got %q", unwanted, got)
+		}
+	}
+}
+
+// TestCreateOrModifyEntity_WarnsOnDrop verifies that CREATE OR MODIFY ENTITY
+// still applies (non-blocking) but prints a data-loss warning listing the
+// attributes it removes (findings #24).
+func TestCreateOrModifyEntity_WarnsOnDrop(t *testing.T) {
+	mod := mkModule("Sudoku")
+	existing := &domainmodel.Entity{
+		BaseElement: model.BaseElement{ID: nextID("ent")},
+		ContainerID: nextID("dm"),
+		Name:        "Game",
+		Persistable: true,
+		Attributes: []*domainmodel.Attribute{
+			{Name: "Puzzle"}, {Name: "Solution"}, {Name: "Difficulty"},
+		},
+	}
+	dm := &domainmodel.DomainModel{
+		BaseElement: model.BaseElement{ID: nextID("dm")},
+		ContainerID: mod.ID,
+		Entities:    []*domainmodel.Entity{existing},
+	}
+	h := mkHierarchy(mod)
+	withContainer(h, dm.ID, mod.ID)
+	withContainer(h, existing.ContainerID, dm.ID)
+
+	updateCalled := false
+	mb := &mock.MockBackend{
+		IsConnectedFunc:      func() bool { return true },
+		ListModulesFunc:      func() ([]*model.Module, error) { return []*model.Module{mod}, nil },
+		ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { return []*domainmodel.DomainModel{dm}, nil },
+		GetDomainModelFunc:   func(id model.ID) (*domainmodel.DomainModel, error) { return dm, nil },
+		UpdateEntityFunc: func(dmID model.ID, e *domainmodel.Entity) error {
+			updateCalled = true
+			return nil
+		},
+	}
+
+	ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h))
+	// Redeclare only one of the three existing attributes.
+	err := execCreateEntity(ctx, &ast.CreateEntityStmt{
+		Name:           ast.QualifiedName{Module: "Sudoku", Name: "Game"},
+		Kind:           ast.EntityPersistent,
+		CreateOrModify: true,
+		Attributes: []ast.Attribute{
+			{Name: "Puzzle", Type: ast.DataType{Kind: ast.TypeString}},
+		},
+	})
+	assertNoError(t, err)
+	if !updateCalled {
+		t.Fatal("UpdateEntity was not called — modify should still apply (non-blocking)")
+	}
+	out := buf.String()
+	assertContainsStr(t, out, "drops 2 existing member(s)")
+	assertContainsStr(t, out, "Solution")
+	assertContainsStr(t, out, "Difficulty")
+	assertContainsStr(t, out, "alter entity Sudoku.Game add attribute")
+}
+
 // TestValidateEntityNPEReservedWords verifies that non-persistent entities
 // reject runtime-reserved attribute names (CE7247). Issue #552.
 //