feat: diag --check-units + grammar fixes

[engalar]

Stacked on #66

[github-actions]

AI Code Review

Critical Issues

• Missing full-stack implementation for MDL grammar changes: The grammar modifications include what appear to be new language features (PLUGGABLEWIDGET widget support, alternative association statement syntax, generic type support in dataType). Per the checklist, new MDL language features must be wired through the full pipeline (AST → visitor → executor), but no corresponding AST node types, visitor handlers, or executor implementations are visible in the diff. Without these, the new grammar rules will not be functional.

Moderate Issues

• Scope creep: The PR combines multiple unrelated changes:

  1. New diagnostic feature (diag --check-units)
  2. Multiple grammar fixes (token additions, rule modifications)
  3. bson_dump format enhancement
  4. Page creation logic improvement

  These should be separated into distinct PRs per the checklist's scope & atomicity guidelines (each PR should be scoped to a single feature or concern).

• Insufficient test coverage: No test files are shown for:

  • The new --check-units diagnostic feature
  • The bson_dump format enhancement
  • The page creation logic changes

Minor Issues

• Directory cleanup risk: In diag.go, the fix functionality removes parent directories after deleting stale mxunit files. While it checks if directories are empty after removal, there's a potential race condition where new files could be added between the file deletion and directory removal checks.

What Looks Good

• The --check-units feature addresses a genuine need for MPR v2 projects (identifying orphan units and stale mxunit files) with clean implementation and useful output
• The bson_dump format addition is straightforward and valuable for roundtrip testing
• The page creation logic fix correctly addresses the Studio Pro RevStatusCache issue by reusing existing UUIDs instead of delete+add
• Grammar fixes appear to target legitimate parsing issues (missing tokens, inconsistent syntax)

Recommendation

Request changes until either:

1. If the grammar changes introduce new MDL language features: Implement the full pipeline (AST node types in mdl/ast/, visitor handlers in mdl/visitor/, executor handlers in mdl/executor/) for those features, OR
2. If the grammar changes are purely fixes to existing functionality: Provide clear justification/documentation that they don't require full-stack implementation, and split the PR into separate changes:
   • One PR for the diag --check-units feature
   • One PR for grammar fixes
   • One PR for bson_dump enhancements
   • One PR for page creation logic fixes

Given the truncation of the diff, verify that AST/visitor/executor changes for the grammar modifications exist in the complete diff before proceeding. If they do exist but weren't shown, this concern can be withdrawn.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[ako]

Review

Scope concerns

This PR bundles four independent changes that should be separate PRs per the repo's checklist:

1. mxcli diag --check-units (new feature)
2. Grammar compatibility fixes (new tokens, parser rule changes)
3. bson dump --format bson (enhancement)
4. All of PR #66's changes (int32→int64, CREATE OR REPLACE PAGE UUID reuse)

The PR description just says "Stacked on #66" — the grammar changes and BSON dump format aren't mentioned at all.

Issues

1. Undisclosed ANTLR visitor generation (+2,635 lines)

Two new generated files are added (mdlparser_base_visitor.go, mdlparser_visitor.go) that don't exist on main. This means the ANTLR code generation config was changed to produce visitors in addition to listeners. This is a significant change:

• It's not mentioned in the PR description
• It's unclear if it was intentional or accidental (e.g., a different ANTLR version or flag)
• The codebase uses the listener pattern (mdl/visitor/ package) — if nobody consumes the visitor interface, these are ~2,600 lines of dead generated code

2. Grammar rules added without visitor/executor wiring

Several new grammar constructs are added but appear to have no corresponding visitor or executor code:

Grammar addition	Visitor handler?	Executor handler?
PLUGGABLEWIDGET token + widgetV3 rule	Not in this PR	Not in this PR
TABCONTAINER / TABPAGE tokens + widgetTypeV3	Not in this PR	Not in this PR
keyword COLON propertyValueV3 (widget property)	Not in this PR	Not in this PR
Parenthesized CREATE ASSOCIATION syntax	Not in this PR	Not in this PR
DATASOURCE EQUALS dataSourceExprV3 (alterPageAssignment)	Not in this PR	Not in this PR

Per the repo's full-stack consistency checklist, new grammar rules must be wired through visitor → executor. Without that, these rules parse but silently do nothing — or worse, hit a nil/unhandled case at runtime.

3. String(unlimited) uses overly broad IDENTIFIER

: STRING_TYPE (LPAREN (NUMBER_LITERAL | IDENTIFIER) RPAREN)?

This accepts String(anything) — not just String(unlimited). A dedicated UNLIMITED token or a semantic check would be safer.

4. keyword COLON propertyValueV3 is very broad

Adding keyword as a widget property name alternative means every keyword in the language can appear as a property name. This risks grammar ambiguities — e.g., FROM : someValue inside a widget block could conflict with association syntax. Was this tested for parser conflicts?

5. ListAllUnitIDs silently swallows scan errors

if err := rows.Scan(&unitID); err != nil {
    continue  // silently skips corrupted rows
}

At minimum this should log or count the errors so users know their Unit table has issues.

6. --fix deletes files without confirmation

os.Remove(fpath) is a destructive operation. The --fix flag auto-deletes stale mxunit files without asking for confirmation. Consider either:

• Printing what would be deleted first (dry-run by default), or
• Requiring an explicit --yes / -y flag for unattended use

The current --fix UX silently deletes and only prints after the fact.

7. All PR #66 issues still apply

Since this is stacked on #66, the same issues apply: debug logging in writer_widgets_custom.go, inconsistent array markers, comment/code mismatches. See PR #66 review.

Minor

• safeInt64 is still a no-op (inherited from #66)
• The .mxunit golden files are still not wired to any test functions
• os.Remove(dir2) / os.Remove(dir1) for cleaning empty parent dirs silently ignore errors — acceptable for non-empty dirs, but a comment explaining the intent would help