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

[engalar]

Addressed Review Feedback

Rebased on latest main (PR #66 merged via #71), cleaned up scope:

Changes made

1. Dropped PR fix: int32→int64 BSON property values + CREATE OR REPLACE PAGE UUID reuse #66 commit — int32→int64 changes already merged via fix: int32→int64 for BSON property values #71, removed residual diff
2. Removed all grammar changes — PLUGGABLEWIDGET/TABCONTAINER tokens, widgetV3 rules, keyword property names, String(unlimited) IDENTIFIER — these belong to the widget engine v2 branch, not this PR
3. Removed ANTLR visitor files — 2,635 lines of dead generated code (mdlparser_base_visitor.go, mdlparser_visitor.go), codebase uses listener pattern
4. Fixed ListAllUnitIDs error handling — scan errors now return fmt.Errorf instead of silent continue; also returns rows.Err() to catch iteration errors
5. Removed debug logging — timelineCustom log.Printf in writer_widgets_custom.go gone with the dropped commit

Current scope

PR now contains only 2 features (3 files, +119/-1):

• diag --check-units — orphan/stale mxunit file detection for MPR v2
• bson dump --format bson — raw BSON output for roundtrip testing

On --fix without confirmation (issue #7)

The current design requires explicit --fix flag — without it, the command only reports (dry-run by default). The output shows each STALE FILE before the summary line says "Run with --fix to auto-remove stale files". I think this is reasonable since --fix is an opt-in flag, but happy to add --yes if preferred.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• The PR title mentions "grammar fixes" but the provided diff shows no changes to grammar files (mdl/grammar/). If grammar fixes are intended as part of this PR, they should be visible in the diff. However, since the diff doesn't show any grammar modifications and the reviewer instructions specify to only evaluate what's in the diff, this is noted but not blocked.

What Looks Good

• The --check-units diagnostic feature is a valuable addition for MPR v2 projects, helping detect inconsistencies between the Unit table and .mxunit files.
• Implementation follows existing patterns: proper error handling, resource cleanup (deferring rows.Close()), and clear separation of concerns.
• The --format bson addition to bson dump is useful for roundtrip testing as documented, with a clean implementation that writes raw BSON bytes.
• Help text and examples are clear and informative.
• No MDL syntax changes are introduced, so full-stack consistency requirements don't apply (appropriately skipped).
• Security considerations are addressed: file I/O occurs only in diagnostic context (not hot paths), project path validation is present, and destructive operations (--fix) require explicit opt-in.

Recommendation

Approve the PR. The changes are focused, well-implemented, and address a genuine need for MPR v2 project maintenance. The minor discrepancy between the title and diff regarding grammar fixes does not block approval since the diff shows no grammar-related changes and the core functionality is sound.

---

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

[ako]

Review (updated — PR has been rebased and cleaned up)

This is much improved from the earlier version. Down from 18K additions/36 files to 119 additions/3 files. The grammar changes, generated parser files, and PR #66 content have been properly split out. Two focused features remain:

1. mxcli diag --check-units — looks good

Clean implementation. Two issues from my earlier review have been fixed:

• ✅ ListAllUnitIDs now returns errors instead of silently swallowing them (return nil, fmt.Errorf(...))
• ✅ rows.Err() is checked after iteration

Remaining minor items:

• --fix still deletes without confirmation — this is a destructive operation. The current flow (print what's stale, then delete if --fix) is reasonable since you see the output, but a --dry-run default or --yes flag would be safer for scripted use. Non-blocking.

• os.Remove(dir2) / os.Remove(dir1) silently clean parent dirs — this is fine since os.Remove fails on non-empty dirs, but a brief comment would help (e.g., "// Remove parent dirs if empty — os.Remove fails safely on non-empty dirs").

2. bson dump --format bson — clean, minimal

6 lines of logic, does exactly what it says. Good example in the help text.

Title mismatch

PR title says "grammar fixes" but those are no longer in this PR. Consider updating to match the actual content: "feat: diag --check-units + bson dump --format bson" (which matches the commit message).

Looks good to merge.