refactor: quick-win cleanup — panic stubs, SaveToFile, dead code, error handling, syncWriter

[retran]

Summary

• Delegate 3 panicking SerializeWidget/SerializeDataSource/SerializeDataSourceToOpaque stubs in backend/mpr/backend.go to existing mpr.Serialize* free functions
• Harden SaveToFile — escape single quotes in VACUUM INTO path, remove unused destDB open
• Delete dead code: types.Hash + mpr.Hash (zero callers confirmed via grep)
• Add syncWriter (mutex-wrapped io.Writer) for background catalog goroutine output race in cmd_catalog.go
• Add catalogGen monotonic counter to prevent stale background catalog from overwriting newer foreground catalog
• Fix 6 high-concern discarded errors across cmd_microflows_builder_calls.go, cmd_page_wireframe.go, cmd_pages_builder_v3.go
• Unexport EnsureSqlMgr → ensureSqlMgr

Why

These are zero-feature-change cleanups that reduce panic risk, fix data races, and improve error observability. Each fix is independently safe and backward-compatible. Preparing the codebase for the WASM extraction track.

Testing

• make build && make test && make lint-go pass
• 8 rounds of Copilot review on fork PR (retran/mxcli#4), 0 open threads

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In cmd_microflows_builder_calls.go, cmd_page_wireframe.go, and cmd_pages_builder_v3.go, the warning logs could include more context (e.g., microflow/page name) for better debugging
• In cmd_catalog.go, the log.Printf in outputCatalogResults could use a more specific error format (though %v is acceptable)

What Looks Good

• Panic fixes: Delegating panicking SerializeWidget/SerializeDataSource/SerializeDataSourceToOpaque stubs to existing mpr.Serialize* functions eliminates crash risks
• SaveToFile hardening: Properly escaping single quotes in VACUUM INTO path prevents SQL injection
• Dead code removal: Clean removal of unused types.Hash and mpr.Hash (confirmed zero callers)
• Catalog race solutions:
  • syncWriter mutex-wrapped writer prevents concurrent output races
  • catalogGen monotonic counter correctly prevents stale background catalogs from overwriting newer foreground catalogs
  • Sophisticated and correct solution to a tricky concurrency problem
• Error handling: Fixed 6 high-concern discarded errors across multiple files improves observability
• Unexport: EnsureSqlMgr → ensureSqlMgr follows Go naming conventions for unexported helpers
• All changes are zero-feature-change cleanups that reduce risk and improve reliability
• Testing: make build && make test && make lint-go pass

Recommendation

Approve - The PR consists of high-quality cleanup changes that fix actual bugs (panics, SQL injection, race conditions), improve error handling, remove dead code, and follow best practices without breaking changes. The catalog race solution is particularly well-implemented. All changes are independently safe and backward-compatible, preparing the codebase for the WASM extraction track as intended. Minor logging improvements are negligible and don't block approval.

---

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

[Copilot]

Pull request overview

This PR is a “quick-win” refactor/cleanup focused on reducing panics, improving error handling, removing dead code, and making background catalog refresh safer by preventing stale background results from overwriting newer catalog state.

Changes:

• Propagate previously-discarded errors (notably MPR transaction ID updates, widget inserts, and several executor commands) and replace panic stubs with real serialization implementations.
• Remove unused hashing helpers/tests and minor formatting/dead-code cleanup.
• Add catalog background refresh synchronization primitives (SyncCatalog, catalogMu, catalogGen, syncWriter) and attempt to prevent stale background catalog overwrites.

Reviewed changes

Copilot reviewed 17 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
sdk/mpr/writer_units.go	updateTransactionID() now returns errors; callers now fail fast when transaction ID update fails.
sdk/mpr/utils.go	Removes unused Hash helper.
sdk/microflows/microflows_actions.go	Minor formatting/alignment change only.
mdl/types/id_test.go	Removes tests for deleted Hash.
mdl/types/id.go	Removes dead Hash implementation.
mdl/executor/hierarchy.go	Removes trailing blank lines.
mdl/executor/executor_dispatch.go	Adds catalogMu/catalogGen sync + SyncCatalog callback to support background catalog delivery.
mdl/executor/executor.go	Adds catalog mutex/gen fields; makes Catalog() accessor lock-protected.
mdl/executor/exec_context.go	Adds SyncCatalog callback; unexports EnsureSqlMgr to ensureSqlMgr.
mdl/executor/cmd_sql.go	Updates SQL manager accessor to renamed ensureSqlMgr.
mdl/executor/cmd_pages_builder_v3.go	Stops discarding an entity lookup error; logs warning instead.
mdl/executor/cmd_page_wireframe.go	Stops discarding describePage error; logs warning instead.
mdl/executor/cmd_microflows_builder_calls.go	Stops discarding Java action lookup error; logs warning instead.
mdl/executor/cmd_microflows_builder.go	Formatting/alignment changes in struct fields.
mdl/executor/cmd_catalog.go	Adds syncWriter; attempts to propagate background-built catalog via SyncCatalog; stops discarding writeResult error.
mdl/catalog/catalog.go	Hardens SaveToFile VACUUM path quoting; removes unused destination DB open.
mdl/catalog/builder_pages.go	Stops discarding widget insert errors; returns contextualized error.
mdl/catalog/builder_microflows_test.go	Import ordering tweak.
mdl/catalog/builder.go	Minor formatting/alignment change.
mdl/backend/mpr/backend.go	Replaces panic serialization stubs with delegation to mpr.Serialize* helpers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[retran]

Re: Nemotron review — thanks for the thorough review. The minor suggestions about adding more context to warning logs are already addressed: each log.Printf includes the relevant identifier (Java action name, page name, entity ID, layout name). No changes needed.

[github-actions]

AI Code Review

Critical Issues

• None found.

Moderate Issues

• Scope & atomicity: The PR bundles multiple unrelated cleanups (panic stub delegation, SaveToFile hardening, dead code removal, syncWriter addition, catalogGen counter, error handling fixes, and function unexporting). While each change is independently safe, the checklist recommends PRs be scoped to a single concern. The description uses "and" repeatedly between unrelated items, violating the atomicity principle. However, since all changes are low-risk cleanups preparing for WASM extraction and tests pass, this is acceptable but should be avoided in future PRs.

Minor Issues

• Code quality: In mdl/catalog/builder.go, the bool variable comments gained extra indentation (no functional change).
• Test coverage: The mdl/types/id_test.go changes remove dead test code for the deleted Hash function, which is appropriate. However, the mdl/catalog/builder_microflows_test.go diff shows an import removed and re-added identically (likely a diff artifact), which is harmless but noisy.
• Documentation: No documentation updates were needed for these internal refactors.

What Looks Good

• Bugs & correctness:
  • Replaced 3 panicking stubs in mdl/backend/mpr/backend.go with safe delegations to existing mpr.Serialize* functions, eliminating panic risks.
  • Hardened SaveToFile in mdl/catalog/catalog.go by escaping single quotes in VACUUM INTO paths (preventing SQL injection) and removing unused destDB open.
  • Fixed 6 discarded errors across builder files by logging warnings instead of ignoring them, improving observability.
  • Made updateTransactionID return errors in sdk/mpr/writer_units.go and properly propagated them, fixing silent failure risks.
• Security & robustness:
  • Added syncWriter in mdl/executor/cmd_catalog.go with mutex protection to eliminate background catalog goroutine output races.
  • Added catalogGen monotonic counter to prevent stale background catalogs from overwriting newer foreground refreshes.
  • Removed dead code (types.Hash + mpr.Hash) with zero callers confirmed via grep.
• Code quality:
  • Unexported EnsureSqlMgr → ensureSqlMgr in mdl/executor/exec_context.go and updated caller in cmd_sql.go, improving encapsulation.
  • Applied consistent error handling patterns (checking return values, logging warnings) across multiple files.
  • Cleaned up unused imports and dead struct tags.

Recommendation

Approve. The PR provides valuable cleanup that reduces panic risks, fixes data races, improves error handling, and removes dead code—all independently safe and backward-compatible. While the PR bundles multiple unrelated changes (violating strict scoping principles), each change is low-risk, well-tested, and prepares the codebase for future WASM extraction work. The maintainer should consider splitting such cleanups in future PRs, but this specific set is acceptable for merging.

Post-merge note: Encourage author to file separate issues for future cleanups to maintain atomic PRs.

---

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

[retran]

Re: second Nemotron review —

Scope & atomicity (moderate): Acknowledged. This PR intentionally bundles related cleanup tasks from the WASM extraction prep track (all zero-feature-change, same design doc section). Future PRs in the series (2–7) are scoped to single concerns.

builder.go indentation: This is go fmt correcting comment alignment after the bool type — the previous alignment used wrong tab count. Intentional fix.

builder_microflows_test.go import reorder: Standard goimports alphabetical sorting (model before sdk/microflows). Not a diff artifact — correct behavior.

No changes needed.